[vlc-devel] [PATCH 1/2] linux: Fix CRTC code in KMS plugin.

Juha-Pekka Heikkila juhapekka.heikkila at gmail.com
Mon Jan 14 14:44:02 CET 2019


Thanks for looking at my code Remi, comments below. I'll fix the code a 
bit and send new version of this patch.

/Juha-Pekka

On 13.1.2019 12.07, Rémi Denis-Courmont wrote:
> Le torstaina 10. tammikuuta 2019, 14.21.53 EET Juha-Pekka Heikkila a écrit :
>> Add missed drmModeSetCrtc(..) call. This does two things. Firstly
>> if requested chroma is not on default CRTC we'll get plane showing
>> correctly. Secondly this allow exiting gracefully in situtation
>> where Xserver is running in the background but has released
>> DRM master for VLC to use.
>>
>> Added printing out in debug information available connectors.
>>
>> Small detail fixes.
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>> ---
>>   modules/video_output/kms.c | 94
>> ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 79
>> insertions(+), 15 deletions(-)
>>
>> diff --git a/modules/video_output/kms.c b/modules/video_output/kms.c
>> index bd65ee8..789df96 100644
>> --- a/modules/video_output/kms.c
>> +++ b/modules/video_output/kms.c
>> @@ -53,9 +53,11 @@
>>   #define DEVICE_LONGTEXT \
>>       "Framebuffer device to use for rendering (usually /dev/dri/card0)."
>>
>> +#define VLC_CHROMA_PARAM "kms-vlc-chroma"
>>   #define VLC_CHROMA_TEXT "Image format used by VLC"
>>   #define VLC_CHROMA_LONGTEXT "Chroma fourcc request to VLC for output
>> format"
>>
>> +#define DRM_CHROMA_PARAM "kms-drm-chroma"
>>   #define DRM_CHROMA_TEXT "Image format used by DRM"
>>   #define DRM_CHROMA_LONGTEXT "Chroma fourcc override for DRM framebuffer
>> format selection"
>>
>> @@ -93,6 +95,10 @@ struct vout_display_sys_t {
>>   /*
>>    * modeset information
>>    */
>> +    uint32_t        connector_id;
>> +    drmModeModeInfo used_mode;
>> +    uint32_t        old_crtc_id;
>> +    drmModeCrtc     *old_crtc;
>>       uint32_t        crtc;
>>       uint32_t        plane_id;
>>
>> @@ -122,7 +128,7 @@ static void DestroyFB(vout_display_sys_t const *sys,
>> uint32_t const buf) static deviceRval CreateFB(vout_display_t *vd, const
>> int buf)
>>   {
>>       vout_display_sys_t *sys = vd->sys;
>> -    struct drm_mode_create_dumb create_req = { .width = sys->width,
>> +    struct drm_mode_create_dumb create_req = { .width = (sys->width|511)+1,
>> .height = sys->height, .bpp = 32 };
> 
> Uh? Are you sure you want 1..512 bytes of padding? I would rather expect
> 0..511 bytes.

Yes, you are correct. Just above these lines I have my ALIGN macro to 
which I was somehow blind to while writing this. I think I'll at the 
same go fix this code bit more.

> 
>>       struct drm_mode_destroy_dumb destroy_req;
>> @@ -252,9 +258,8 @@ static deviceRval FindCRTC(vout_display_t *vd,
>> drmModeRes const *res, for (i = 0; i < conn->count_encoders; i++) {
>>           enc = drmModeGetEncoder(sys->drm_fd, conn->encoders[i]);
>>
>> -        for (j = 0; enc && j < res->count_crtcs; ++j) {
>> -            if (ffs(enc->possible_crtcs) == j && res->crtcs[j]) {
>> -                msg_Dbg(vd, "Got CRTC %d", res->crtcs[j]);
>> +        for (j = 0; enc && j < res->count_crtcs; j++) {
>> +            if (enc->possible_crtcs & (1 << j)) {
>>                   sys->crtc = res->crtcs[j];
>>                   drmModeFreeEncoder(enc);
>>                   return drvSuccess;
>> @@ -278,10 +283,10 @@ static deviceRval SetupDevice(vout_display_t *vd,
>> drmModeRes const *res, if (conn->connection != DRM_MODE_CONNECTED ||
>> conn->count_modes == 0) return drvTryNext;
>>
>> -    sys->width = conn->modes[0].hdisplay;
>> -    sys->height = conn->modes[0].vdisplay;
>> -    msg_Dbg(vd, "Mode resolution for connector %u is %ux%u",
>> -            conn->connector_id, sys->width, sys->height);
>> +    memcpy(&sys->used_mode, &conn->modes[0], sizeof(sys->used_mode));
>> +    sys->width = sys->used_mode.hdisplay;
>> +    sys->height = sys->used_mode.vdisplay;
>> +    msg_Dbg(vd, "Mode resolution is %ux%u", sys->width, sys->height);
>>
>>       ret = FindCRTC(vd, res, conn);
>>       if (ret != drvSuccess) {
>> @@ -302,6 +307,7 @@ static deviceRval SetupDevice(vout_display_t *vd,
>> drmModeRes const *res, msg_Dbg(vd, "Created HW framebuffer %d/%d", c+1,
>> MAXHWBUF); }
>>       }
>> +    sys->connector_id = conn->connector_id;
>>       return drvSuccess;
>>   }
>>
>> @@ -505,8 +511,34 @@ static int OpenDisplay(vout_display_t *vd)
>>       drmModeConnector *conn;
>>       drmModeRes *modeRes;
>>       int c;
>> +    uint32_t i;
> 
> Keep automatic variables as local as possible.
> 
>>       bool found_connector = false;
>>
>> +    static const struct
>> +    {
>> +        uint32_t    type;
>> +        const char* name;
>> +    } connectors[] = {
>> +        { .type = DRM_MODE_CONNECTOR_VGA, .name = "vga"},
>> +        { .type = DRM_MODE_CONNECTOR_DVII, .name = "DVII"},
>> +        { .type = DRM_MODE_CONNECTOR_DVID, .name = "DVID"},
>> +        { .type = DRM_MODE_CONNECTOR_DVIA, .name = "DVIA"},
>> +        { .type = DRM_MODE_CONNECTOR_Composite, .name = "Composite"},
>> +        { .type = DRM_MODE_CONNECTOR_SVIDEO, .name = "SVIDEO"},
>> +        { .type = DRM_MODE_CONNECTOR_LVDS, .name = "LVDS"},
>> +        { .type = DRM_MODE_CONNECTOR_Component, .name = ""},
>> +        { .type = DRM_MODE_CONNECTOR_9PinDIN, .name = "Component"},
>> +        { .type = DRM_MODE_CONNECTOR_DisplayPort, .name = "DisplayPort"},
>> +        { .type = DRM_MODE_CONNECTOR_HDMIA, .name = "HDMIA"},
>> +        { .type = DRM_MODE_CONNECTOR_HDMIB, .name = "HDMIB"},
>> +        { .type = DRM_MODE_CONNECTOR_TV, .name = "TV"},
>> +        { .type = DRM_MODE_CONNECTOR_eDP, .name = "embedded DisplayPort"},
>> +        { .type = DRM_MODE_CONNECTOR_VIRTUAL, .name = "VIRTUAL"},
>> +        { .type = DRM_MODE_CONNECTOR_DSI, .name = "DSI"},
>> +        { .type = DRM_MODE_CONNECTOR_DPI, .name = "DPI"},
>> +        { .type = DRM_MODE_CONNECTOR_Unknown, .name = "Unknown"}
>> +    };
>> +
>>       /*
>>        * Open framebuffer device
>>        */
>> @@ -544,12 +576,22 @@ static int OpenDisplay(vout_display_t *vd)
>>           goto err_out;
>>       }
>>
>> -    for (c = 0; c < modeRes->count_connectors && sys->crtc == 0; c++) {
>> -
>> +    for (c = 0; c < modeRes->count_connectors; c++) {
>>           conn = drmModeGetConnector(sys->drm_fd, modeRes->connectors[c]);
>>           if (conn == NULL)
>>               continue;
>>
>> +        for (i = 0; i < ARRAY_SIZE(connectors)-1; i++) {
>> +            if (conn->connector_type == connectors[i].type)
>> +                break;
>> +        }
>> +        msg_Dbg(vd, "** Connector #%d type %s, connected [%c]", c,
>> +                connectors[i].name,
>> +                (conn->connection==DRM_MODE_CONNECTED)?'X':' ');
>> +
>> +        if (sys->crtc != 0)
>> +            continue;
>> +
>>           found_connector = true;
>>
>>           ret = SetupDevice(vd, modeRes, conn);
>> @@ -655,6 +697,16 @@ static void Display(vout_display_t *vd, picture_t
>> *picture) vout_display_sys_t *sys = vd->sys;
>>       int i;
>>
>> +    if (sys->old_crtc == NULL) {
>> +        sys->old_crtc = drmModeGetCrtc(sys->drm_fd, sys->crtc);
>> +        if(drmModeSetCrtc(sys->drm_fd, sys->crtc,sys->fb[0],
>> +                          0, 0,
>> +                          &sys->connector_id, 1,
>> +                          &sys->used_mode)) {
>> +            msg_Err(vd, "Cannot do modesetting");
>> +        }
>> +    }
>> +
>>       if (drmModeSetPlane(sys->drm_fd, sys->plane_id, sys->crtc,
>>                            sys->fb[sys->front_buf], 0,
>>                            0, 0, sys->width, sys->height,
>> @@ -680,8 +732,20 @@ static void Close(vout_display_t *vd)
>>   {
>>       vout_display_sys_t *sys = vd->sys;
>>
>> -    if (sys->pool)
>> +    if (sys->pool) {
>> +        if(drmModeSetCrtc(sys->drm_fd,
>> +                   sys->old_crtc->crtc_id,
>> +                   sys->old_crtc->buffer_id,
>> +                   sys->old_crtc->x,
>> +                   sys->old_crtc->y,
>> +                   &sys->connector_id,
>> +                   1,
>> +                   &sys->old_crtc->mode))
>> +            msg_Err(vd, "error modeset xit");
> 
> Does this really make sense? I can imagine that this works properly if DRM
> users are started and stopped in strictly stacked order. But otherwise?

I will need to do this for two reasons. First if CRTC was changed on 
connector (this is actually unlikely) I need to set it back. Second when 
running with text terminal having Xorg on background default buffer_id 
is not where text terminal is rendered (at least on my Skylake w/ 
Ubuntu), if I don't set buffer_id back to what it was I will have 
terminal with blank screen.

With kms there's no stacking, just one owner.

> 
>> +        drmModeFreeCrtc(sys->old_crtc);
>> +
>>           picture_pool_Release(sys->pool);
>> +    }
>>
>>       if (sys->drm_fd)
>>           drmDropMaster(sys->drm_fd);
>> @@ -709,7 +773,7 @@ static int Open(vout_display_t *vd, const
>> vout_display_cfg_t *cfg, if (!sys)
>>           return VLC_ENOMEM;
>>
>> -    chroma = var_InheritString(vd, "kms-vlc-chroma");
>> +    chroma = var_InheritString(vd, VLC_CHROMA_PARAM);
>>       if (chroma) {
>>           local_vlc_chroma = vlc_fourcc_GetCodecFromString(VIDEO_ES, chroma);
>>
>> @@ -728,7 +792,7 @@ static int Open(vout_display_t *vd, const
>> vout_display_cfg_t *cfg, msg_Dbg(vd, "Chroma %4s invalid, using default",
>> chroma); }
>>
>> -    chroma = var_InheritString(vd, "kms-drm-chroma");
>> +    chroma = var_InheritString(vd, DRM_CHROMA_PARAM);
>>       if (chroma) {
>>           local_drm_chroma = VLC_FOURCC(chroma[0], chroma[1], chroma[2],
>>                                         chroma[3]);
>> @@ -777,9 +841,9 @@ vlc_module_begin ()
>>       set_subcategory(SUBCAT_VIDEO_VOUT)
>>       add_loadfile(KMS_VAR, "/dev/dri/card0", DEVICE_TEXT, DEVICE_LONGTEXT)
>>
>> -    add_string( "kms-vlc-chroma", NULL, VLC_CHROMA_TEXT,
>> VLC_CHROMA_LONGTEXT, +    add_string(VLC_CHROMA_PARAM, NULL,
>> VLC_CHROMA_TEXT, VLC_CHROMA_LONGTEXT, true)
>> -    add_string( "kms-drm-chroma", NULL, DRM_CHROMA_TEXT,
>> DRM_CHROMA_LONGTEXT, +    add_string(DRM_CHROMA_PARAM, NULL,
>> DRM_CHROMA_TEXT, DRM_CHROMA_LONGTEXT, true)
>>       set_description("Linux kernel mode setting video output")
>>       set_capability("vout display", 30)
> 
> 



More information about the vlc-devel mailing list