[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