GNOME Bugzilla – Bug 796985
kmssink modesetting fails for multi-planar buffers
Last modified: 2018-08-21 15:55:45 UTC
Created attachment 373369 [details] [review] kmssink: configure mode setting from video info drmModeGetFB returns -EINVAL for multi-planar framebuffers, causing configure_mode_setting to fail for multi-planar buffers such as I420 or NV12. Only buffer width and height are needed to select a mode, and those can be determined from GstVideoInfo as well.
Review of attachment 373369 [details] [review]: lgtm
Review of attachment 373369 [details] [review]: ::: sys/kms/gstkmssink.c @@ -441,3 @@ goto connector_failed; - fb = drmModeGetFB (self->fd, fb_id); On the plus side, we found that drmModeGetFB returns a handle that must be freed, and was currently leaked.
(In reply to Nicolas Dufresne (ndufresne) from comment #3) > Review of attachment 373369 [details] [review] [review]: > > ::: sys/kms/gstkmssink.c > @@ -441,3 @@ > goto connector_failed; > > - fb = drmModeGetFB (self->fd, fb_id); > > On the plus side, we found that drmModeGetFB returns a handle that must be > freed, and was currently leaked. It won't matter any more, but as far I see in the code, it was freed: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/sys/kms/gstkmssink.c#n468
It's tricky, as it's creating a DUMB, and this need to also be freed using DRM_IOCTL_MODE_DESTROY_DUMB. But does not matter any-more. A bit like what we have in the KMS allocator. The refcounting is weirdly implemented on kernel side.
Review of attachment 373369 [details] [review]: ::: sys/kms/gstkmssink.c @@ +420,3 @@ GstKMSMemory *kmsmem; + guint32 w; + guint32 h; I'll drop these two, this is not really needed.
Review of attachment 373369 [details] [review]: Thanks for the patch Philip. Only suggestion I have is to directly use GST_VIDEO_INFO_HEIGHT and GST_VIDEO_INFO_WIDTH instead of using local declarations. Please refer in-code comments for reference. Regards, -Devarsh ::: sys/kms/gstkmssink.c @@ +420,3 @@ GstKMSMemory *kmsmem; + guint32 w; + guint32 h; In my opinion, above declarations can be avoided, please see my next comment. @@ +445,2 @@ for (i = 0; i < conn->count_modes; i++) { + if (conn->modes[i].vdisplay == h && conn->modes[i].hdisplay == w) { I think we can avoid local integer declarations for width and height and directly use GST macros as shown below : if (conn->modes[i].vdisplay == GST_VIDEO_INFO_HEIGHT (vinfo); && conn->modes[i].hdisplay == GST_VIDEO_INFO_WIDTH (vinfo);)
Created attachment 373417 [details] [review] kmssink: configure mode setting from video info drmModeGetFB returns -EINVAL for multi-planar framebuffers. Instead of depending on the framebuffer dimensions to select the mode, use width and height from GstVideoInfo, which was used to create the framebuffer in the first place. This enables kmssink to display multi-planar formats such as I420 or NV12 with modesetting enabled.
Just updated accordingly.
I've double check, gst_kms_allocator_bo_alloc() may modify stride/offset/size but not width and height, so this should have no side effect.
Attachment 373417 [details] pushed as 62a194c - kmssink: configure mode setting from video info
Shall we backport this to 1.14 ?