GNOME Bugzilla – Bug 773473
kmssink: support display mode setting
Last modified: 2017-04-03 09:10:43 UTC
The kmssink relied on another application for setting the display mode of the connector. It would be more convenient if the kmssink would take care of the mode setting if there is no active display mode. The proposed patches extend the kms sink with mode setting capabilities. If no display mode has been activated, the kmssink reports the supported modes via allowed caps and, after negotiation with upstream it, sets the display mode according to the negotiated caps. If the size of the video does not match a supported display mode, the negotiation fails. I did not want to assume an expected behavior for this case, but leave scaling or cropping to other elements in the pipeline. The patch set also removes the use of the kmslib, because the kmslib limits the allocation of drm buffer objects. For example, buffer objects returned by kmslib always use 32 bpp. This is also kind of related to bug 766468. Could you please review the patches and give feedback if the mode setting functionality makes sense. It was tested on a Freescale i.MX6 board.
Created attachment 338403 [details] [review] kmssink: support mode setting and base plane rendering
Created attachment 338404 [details] [review] kmssink: set caps based on supported display modes
Created attachment 338405 [details] [review] kmssink: remove custom gst_kms_sink_get_times
Created attachment 338406 [details] [review] kmssink: add parameter force-base-plane
Created attachment 338407 [details] [review] kmssink: read supported color formats from plane
Created attachment 338408 [details] [review] kmssink: do not use libkms
Created attachment 338409 [details] [review] kmssink: configure display mode during set_caps
Created attachment 338410 [details] [review] kmssink: add support for planar and non-32 bpp color formats
Created attachment 338411 [details] [review] kmssink: do not simplify caps
Thanks! I just glanced the patches and, overall, they look good. Though I would review them and test them after the 1.10 release. Is that OK for you?
Review of attachment 338411 [details] [review]: Is there a way we could simplify the range before we append it ?
(In reply to Nicolas Dufresne (stormer) from comment #11) > Review of attachment 338411 [details] [review] [review]: > > Is there a way we could simplify the range before we append it ? Yes, I can switch the inner and outer loop and simplify the range of formats. I haven't thought about it but it totally makes sense. I will update attachment 338407 [details] [review] accordingly and remove attachment 338411 [details] [review] in the next iteration.
(In reply to Víctor Manuel Jáquez Leal from comment #10) > Thanks! > > I just glanced the patches and, overall, they look good. Though I would > review them and test them after the 1.10 release. Is that OK for you? Thank you. I am looking forward to your review.
Review of attachment 338403 [details] [review]: This patch doesn't apply cleanly in master. Anyway, I'm merging it manually. A couple comments, just nitpicking: Some changes are just reindentations because 'if' nesting. I wonder if is it possible to use the idiom if (something) goto label; In my opinion is more readable if it is used wisely :D
Review of attachment 338403 [details] [review]: ::: sys/kms/gstkmssink.c @@ +1229,3 @@ + src.x << 16, src.y << 16, src.w << 16, src.h << 16); + if (ret) { + if (self->can_scale) { where does this come? I can't find this in master. But it looks interesting :D
Review of attachment 338403 [details] [review]: ::: sys/kms/gstkmssink.c @@ +1163,3 @@ GST_TRACE_OBJECT (self, "displaying fb %d", fb_id); + if (self->plane_id == -1) { I would move this branch to avoid the cluttering of the function @@ +1177,3 @@ + waiting = TRUE; + ret = drmModePageFlip (self->fd, self->crtc_id, fb_id, + DRM_MODE_PAGE_FLIP_EVENT, &waiting); is this possible if the driver doesn't support async page flip? @@ +1178,3 @@ + ret = drmModePageFlip (self->fd, self->crtc_id, fb_id, + DRM_MODE_PAGE_FLIP_EVENT, &waiting); + if (ret != 0) if (ret) -- to follow the current code style @@ +1183,1 @@ + while (waiting) { perhaps we could avoid code duplication by making a function if this loop waiting for an event
Review of attachment 338405 [details] [review]: ::: sys/kms/gstkmssink.c @@ -910,3 @@ - *end = *start + GST_BUFFER_DURATION (buf); - else { - if (GST_VIDEO_INFO_FPS_N (&self->vinfo) > 0) { the only reason to use this method, is because, if the buffer doesn't have a duration, a duration will be calculated with the provided framerate. though I gladly remove this if we can foresee that this is not possible, nor desirable.
Review of attachment 338408 [details] [review]: lgtm
Review of attachment 338410 [details] [review]: ::: sys/kms/gstkmsallocator.c @@ +177,3 @@ + GST_ERROR_OBJECT (allocator, "Unsupported color format"); + free (kmsmem->bo); + return FALSE; can make this a function? get_bpp_from_format () for example if it returns -1, destroy the bo @@ +194,3 @@ + break; + default: + arg.height = GST_VIDEO_INFO_HEIGHT (vinfo); ditto
Created attachment 339324 [details] [review] kmssink: do not get kms bo pitch for planar formats
Created attachment 339325 [details] [review] kmssink: remove dependency on libkms
Created attachment 339326 [details] [review] kmssink: support mode setting and base plane rendering
Created attachment 339327 [details] [review] kmssink: set mode based on framebuffer configuration
Created attachment 339328 [details] [review] kmssink: allow only resolutions supported by connector
Created attachment 339329 [details] [review] kmssink: add parameter force-modesetting
Created attachment 339330 [details] [review] kmssink: configure display mode during set_caps
Thanks for all the comments. The latest update contains a lot of changes to improve the readability of the patches. I hope, I didn't mess up anything regarding the patch review process. attachment 339325 [details] [review] seems to have some issues with some pixel formats, especially interleaved YUV formats. The general patch should be fine, as it is probably related to bpp, pitches and offset. I will look into it. Im not sure, if attachment 339330 [details] [review] is a good idea. On the one hand, this allows to set the mode during set_caps, but I still have to keep the memory until the first page flip, which is kind of ugly. Any opinion would be helpful.
(In reply to Víctor Manuel Jáquez Leal from comment #16) > Review of attachment 338403 [details] [review] [review]: > > ::: sys/kms/gstkmssink.c > @@ +1177,3 @@ > + waiting = TRUE; > + ret = drmModePageFlip (self->fd, self->crtc_id, fb_id, > + DRM_MODE_PAGE_FLIP_EVENT, &waiting); > > is this possible if the driver doesn't support async page flip? From my understanding, async page flip and drmModePageFlip are not the same. You should be able to always do a normal page flip. To trigger an async page flip, you would set the DRM_MODE_PAGE_FLIP_ASYNC flag. In fact, the handling of the vblank and page flip in the gst_kms_sink_sync() function is the reason why I implemented gst_kms_page_flip(). I need the page flip, but the imx drm does not have the async page flip capability. Maybe if this is sorted out, I could just use the gst_kms_sink_sync() function.
Review of attachment 339325 [details] [review]: ::: configure.ac @@ -2386,3 @@ AG_GST_CHECK_FEATURE(KMS, [drm/kms libraries], kms, [ AG_GST_PKG_CHECK_MODULES(GST_ALLOCATORS, gstreamer-allocators-1.0) - PKG_CHECK_MODULES([DRM], [libdrm >= 2.4.55 libkms], HAVE_KMS=yes, HAVE_KMS=no) This patch doesn't apply cleanly since the name is KMS_DRM, not DRM as here ::: sys/kms/gstkmsallocator.c @@ +335,3 @@ kmsmem = (GstKMSMemory *) mem; if (kmsmem->bo) + kmsmem->bo->refs--; did you forget to unmap the bo when refs == 0?
(In reply to Víctor Manuel Jáquez Leal from comment #29) > Review of attachment 339325 [details] [review] [review]: > > ::: configure.ac > @@ -2386,3 @@ > AG_GST_CHECK_FEATURE(KMS, [drm/kms libraries], kms, [ > AG_GST_PKG_CHECK_MODULES(GST_ALLOCATORS, gstreamer-allocators-1.0) > - PKG_CHECK_MODULES([DRM], [libdrm >= 2.4.55 libkms], HAVE_KMS=yes, > HAVE_KMS=no) > > This patch doesn't apply cleanly since the name is KMS_DRM, not DRM as here Whoops. I wrote the patches against 1.10.0 instead of master. > ::: sys/kms/gstkmsallocator.c > @@ +335,3 @@ > kmsmem = (GstKMSMemory *) mem; > if (kmsmem->bo) > + kmsmem->bo->refs--; > > did you forget to unmap the bo when refs == 0? The buffer object is unmapped and destroyed in gst_kms_allocator_memory_reset.
Created attachment 339367 [details] [review] kmssink: remove dependency on libkms
(In reply to Michael Tretter from comment #30) > (In reply to Víctor Manuel Jáquez Leal from comment #29) > > ::: sys/kms/gstkmsallocator.c > > @@ +335,3 @@ > > kmsmem = (GstKMSMemory *) mem; > > if (kmsmem->bo) > > + kmsmem->bo->refs--; > > > > did you forget to unmap the bo when refs == 0? > > The buffer object is unmapped and destroyed in > gst_kms_allocator_memory_reset. I'm not sure about this approach. I would unmap when the map_counter reach zero, otherwise a bad code could unmap forever. gst_kms_allocator_memory_reset() should force the unmap, if is still mapped with a g_warning message, and then destroy the dumb buffer. Also, I'm wondering if we should add mutexes when mapping-unmapping if there are threads using the buffers. But that could go in other patch.
(In reply to Víctor Manuel Jáquez Leal from comment #32) > (In reply to Michael Tretter from comment #30) > > (In reply to Víctor Manuel Jáquez Leal from comment #29) > > > ::: sys/kms/gstkmsallocator.c > > > @@ +335,3 @@ > > > kmsmem = (GstKMSMemory *) mem; > > > if (kmsmem->bo) > > > + kmsmem->bo->refs--; > > > > > > did you forget to unmap the bo when refs == 0? > > > > The buffer object is unmapped and destroyed in > > gst_kms_allocator_memory_reset. > > I'm not sure about this approach. I would unmap when the map_counter reach > zero, otherwise a bad code could unmap forever. > gst_kms_allocator_memory_reset() should force the unmap, if is still mapped > with a g_warning message, and then destroy the dumb buffer. I used the same approach as implemented in libkms. I remember some problems with unmapping when the map_counter is zero, but right not, it seems to work fine. I will do some more testing and update the patch. > Also, I'm wondering if we should add mutexes when mapping-unmapping if there > are threads using the buffers. But that could go in other patch. I had the same question. As libkms didn't use mutexes for the reference counter, I didn't use mutexes, but it sounds like a good idea.
Attachment 338405 [details] pushed as b52c39e - kmssink: remove custom gst_kms_sink_get_times Attachment 339324 [details] pushed as 4f19d5d - kmssink: do not get kms bo pitch for planar formats
Created attachment 340140 [details] [review] 0001-kmssink-remove-dependency-on-libkms.patch I reworked the patch that removes libkms.
(In reply to Michael Tretter from comment #35) > Created attachment 340140 [details] [review] [review] > 0001-kmssink-remove-dependency-on-libkms.patch > > I reworked the patch that removes libkms. Cool! thanks! Could you try with the mutexes? any result to share? Right now I'm away from my toys to test the patches. But they look good for me now. I'll push them next Monday or Tuesday.
Comment on attachment 340140 [details] [review] 0001-kmssink-remove-dependency-on-libkms.patch I've polished the commit * initialize the ioctl argument structure rather than memset * clean up the header includes * initlized with g_malloc0() kms_bo structure (and use g_free) * move the definition of kms_bo structure into the gstkmsallocator.c * updated the meson.build file * removed the kms_driver structure * changed ensure_kms_driver() to check_fd() * use g_atomic_int_*() to handle the mapping count reference
I'm testing the patch set in a minnowboard, no debugging yet, but kmssink always reports empty caps, so no video can be negotiated using gst-play-1.0
Review of attachment 339328 [details] [review]: this patch breaks the functionality of kmssink in minnowboard.
Review of attachment 339326 [details] [review]: ::: sys/kms/gstkmssink.c @@ +1220,3 @@ + goto bail; + + goto skip_set_plane; Just to get the facts right. Does this mean that, if modesetting_enabled == TRUE, the buffer is going to be ALWAYS flipped WITHOUT SetPlane() being called?? Is this another rendering code path? The cropping meta will be ignored. Is that correct?
the brown paper bag commit: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=79cd4bb44b782f6c69e6316e0aa19c597cafeb0f
Review of attachment 339328 [details] [review]: ::: sys/kms/gstkmssink.c @@ +417,3 @@ + count_modes = conn->count_modes; + else + count_modes = 1; here seems to be the problem: if we change it to if (conn) count_modes = conn->count_modes; if (count_modes == 0) count_modes = 1;
(In reply to Víctor Manuel Jáquez Leal from comment #42) > Review of attachment 339328 [details] [review] [review]: > > ::: sys/kms/gstkmssink.c > @@ +417,3 @@ > + count_modes = conn->count_modes; > + else > + count_modes = 1; > > here seems to be the problem: if we change it to > > if (conn) > count_modes = conn->count_modes; > > if (count_modes == 0) > count_modes = 1; It sounds like the kms driver does not correctly report the supported modes. Without this information, the kmssink does not know the available modes and won't be able to set the mode. I thought that if a connector is found, the connector would always report supported modes. If there are connectors which do not report modes, the condition should be cheched in gst_kms_sink_start() after find_main_monitor() and modesetting should be disabled entirely, falling back to using planes.
(In reply to Víctor Manuel Jáquez Leal from comment #40) > Review of attachment 339326 [details] [review] [review]: > > ::: sys/kms/gstkmssink.c > @@ +1220,3 @@ > + goto bail; > + > + goto skip_set_plane; > > Just to get the facts right. Does this mean that, if modesetting_enabled == > TRUE, the buffer is going to be ALWAYS flipped WITHOUT SetPlane() being > called?? Is this another rendering code path? > > The cropping meta will be ignored. Is that correct? Yes, if modesetting is enabled, the page flip will flip to the kms_memory that backs the current GstBuffer, i.e., set it as the current framebuffer and ignore the cropping meta.
(In reply to Víctor Manuel Jáquez Leal from comment #37) > Comment on attachment 340140 [details] [review] [review] > 0001-kmssink-remove-dependency-on-libkms.patch > > I've polished the commit > > * initialize the ioctl argument structure rather than memset > * clean up the header includes > * initlized with g_malloc0() kms_bo structure (and use g_free) > * move the definition of kms_bo structure into the gstkmsallocator.c > * updated the meson.build file > * removed the kms_driver structure > * changed ensure_kms_driver() to check_fd() > * use g_atomic_int_*() to handle the mapping count reference Thanks for the polishing. I'll try to learn from it for the future. :)
Created attachment 340774 [details] [review] kmssink: add mode setting and base plane rendering The kmssink assumed that the mode was already set by another application and used an overlay plane for displaying the frames. Use the preferred mode of the monitor and render to the base plane if the crtc does not have a valid mode. https://bugzilla.gnome.org/show_bug.cgi?id=773473 Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
Comment on attachment 340774 [details] [review] kmssink: add mode setting and base plane rendering Sorry for the patch ping pong. Since I haven't setup an environment for this use case yet, can you try this patch instead of attachment 339326 [details] [review]?? If I understand correctly, there is no need of gst_kms_page_flip(), just set self->buffer_id = fb_id and call gst_kms_sink_sync() -- which knows if modesetting is enabled. Thanks
(In reply to Michael Tretter from comment #43) > (In reply to Víctor Manuel Jáquez Leal from comment #42) > > Review of attachment 339328 [details] [review] [review] [review]: > > > > ::: sys/kms/gstkmssink.c > > @@ +417,3 @@ > > + count_modes = conn->count_modes; > > + else > > + count_modes = 1; > > > > here seems to be the problem: if we change it to > > > > if (conn) > > count_modes = conn->count_modes; > > > > if (count_modes == 0) > > count_modes = 1; > > It sounds like the kms driver does not correctly report the supported modes. > Without this information, the kmssink does not know the available modes and > won't be able to set the mode. > > I thought that if a connector is found, the connector would always report > supported modes. If there are connectors which do not report modes, the > condition should be cheched in gst_kms_sink_start() after > find_main_monitor() and modesetting should be disabled entirely, falling > back to using planes. That is happening, but it also needs to be handle when building the allowed caps. I guess I need to merge that modification on attachment 339328 [details] [review]
(In reply to Víctor Manuel Jáquez Leal from comment #47) > Comment on attachment 340774 [details] [review] [review] > kmssink: add mode setting and base plane rendering > > Sorry for the patch ping pong. > > Since I haven't setup an environment for this use case yet, can you try this > patch instead of attachment 339326 [details] [review] [review]?? > > If I understand correctly, there is no need of gst_kms_page_flip(), just set > self->buffer_id = fb_id and call gst_kms_sink_sync() -- which knows if > modesetting is enabled. I have tested it in a Odroid U2 (exynos) and it works :)
(In reply to Víctor Manuel Jáquez Leal from comment #47) > Comment on attachment 340774 [details] [review] [review] > kmssink: add mode setting and base plane rendering > > Sorry for the patch ping pong. > > Since I haven't setup an environment for this use case yet, can you try this > patch instead of attachment 339326 [details] [review] [review]?? > > If I understand correctly, there is no need of gst_kms_page_flip(), just set > self->buffer_id = fb_id and call gst_kms_sink_sync() -- which knows if > modesetting is enabled. > > Thanks The patch works on i.MX6 as well. It makes sense to update self->buffer_id, but I am not sure about concurrent accesses (if ever possible). It should be fine.
Attachment 339327 [details] pushed as ca96cc6 - kmssink: set mode based on framebuffer configuration Attachment 339329 [details] pushed as dff77fd - kmssink: add parameter force-modesetting Attachment 339330 [details] pushed as 649364b - kmssink: configure display mode during set_caps Attachment 340774 [details] pushed as f52baad - kmssink: add mode setting and base plane rendering
Not entirely sure about this patch: commit 3d0c5dd58e2d8383f6f2d9d8a9d759fca25ee61d Author: Michael Tretter <m.tretter@pengutronix.de> Date: Tue Nov 8 15:27:51 2016 +0100 kmssink: allow only supported resolutions If the input buffers have a different size than the display, the frames would have to be scaled or positioned on the display. The kmssink cannot decide which behaviour would be appropriate for which use case. In order to avoid scaling or positioning of the input stream, allow only the supported connector resolutions in the sink caps. https://bugzilla.gnome.org/show_bug.cgi?id=773473 Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> Wouldn't the more logical thing to do be to either make it a property or take the simplest approach to make things work? (Here probably: positioning/cropping) Compare with ximagesink, which can't scale, yet still it won't refuse caps that are not exactly the size of the window and will crop or center the image as needed. Or is this not possible/desirable in this case?
(In reply to Tim-Philipp Müller from comment #52) > Not entirely sure about this patch: > > commit 3d0c5dd58e2d8383f6f2d9d8a9d759fca25ee61d > Author: Michael Tretter <m.tretter@pengutronix.de> > Date: Tue Nov 8 15:27:51 2016 +0100 > > kmssink: allow only supported resolutions > > If the input buffers have a different size than the display, the frames > would have to be scaled or positioned on the display. The kmssink cannot > decide which behaviour would be appropriate for which use case. > > In order to avoid scaling or positioning of the input stream, allow only > the supported connector resolutions in the sink caps. > > https://bugzilla.gnome.org/show_bug.cgi?id=773473 > > Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> > > > Wouldn't the more logical thing to do be to either make it a property or > take the simplest approach to make things work? (Here probably: > positioning/cropping) The kmssink is responsible for setting the video mode which includes the resolution. If the input resolution does not match the supported mode, which mode should be selected? - the preferred mode of the monitor - the mode that results in the least cropping - the mode that results in the smallest letterbox I am not sure which behavior would be correct or suitable as default. Furthermore, the kmssink then would need to propose caps that it prefers the resolution of the default mode, supports the resolutions of the available modes, but in the end supports all resolutions. I don't see how I could express this in caps. > Compare with ximagesink, which can't scale, yet still it won't refuse caps > that are not exactly the size of the window and will crop or center the > image as needed. > > Or is this not possible/desirable in this case? It would be really nice if the kmssink would just accept caps that do not exactly match an available mode of the display and I struggled with it myself. But because of the aforementioned problems and because I am using the kmssink mainly for testing, I decided that it would be easier to add cropping/positioning elements to the pipeline instead of implementing it within the kmssink.(In reply to Tim-Philipp Müller from comment #52) > Not entirely sure about this patch: > > commit 3d0c5dd58e2d8383f6f2d9d8a9d759fca25ee61d > Author: Michael Tretter <m.tretter@pengutronix.de> > Date: Tue Nov 8 15:27:51 2016 +0100 > > kmssink: allow only supported resolutions > > If the input buffers have a different size than the display, the frames > would have to be scaled or positioned on the display. The kmssink cannot > decide which behaviour would be appropriate for which use case. > > In order to avoid scaling or positioning of the input stream, allow only > the supported connector resolutions in the sink caps. > > https://bugzilla.gnome.org/show_bug.cgi?id=773473 > > Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> > > > Wouldn't the more logical thing to do be to either make it a property or > take the simplest approach to make things work? (Here probably: > positioning/cropping) > > Compare with ximagesink, which can't scale, yet still it won't refuse caps > that are not exactly the size of the window and will crop or center the > image as needed. > > Or is this not possible/desirable in this case? It would be really nice if the kmssink would just accept caps that do not exactly match an available mode of the display and I struggled with it myself. Unfortunately, the kmssink is also responsible for setting the video mode including the output resolution which leads to two problems: If the input resolution does not match the supported mode, which mode should be selected? - the preferred mode of the monitor - the mode that results in the least cropping - the mode that results in the smallest letterbox I am not sure which behavior would be correct or suitable as default. Furthermore, the kmssink then would need to propose caps that it prefers the resolution of the default mode, supports the resolutions of the available modes, but in the end supports all resolutions. This leads to difficulties with elements that also support various output resolutions, e.g., scalers or cropping elements and the videotestsrc, and I don't see how I could express this in caps.
Somehow, I also pasted my draft into the previous reply. Please ignore it and just consider this one: (In reply to Tim-Philipp Müller from comment #52) > Not entirely sure about this patch: > > commit 3d0c5dd58e2d8383f6f2d9d8a9d759fca25ee61d > Author: Michael Tretter <m.tretter@pengutronix.de> > Date: Tue Nov 8 15:27:51 2016 +0100 > > kmssink: allow only supported resolutions > > If the input buffers have a different size than the display, the frames > would have to be scaled or positioned on the display. The kmssink cannot > decide which behaviour would be appropriate for which use case. > > In order to avoid scaling or positioning of the input stream, allow only > the supported connector resolutions in the sink caps. > > https://bugzilla.gnome.org/show_bug.cgi?id=773473 > > Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> > > > Wouldn't the more logical thing to do be to either make it a property or > take the simplest approach to make things work? (Here probably: > positioning/cropping) > > Compare with ximagesink, which can't scale, yet still it won't refuse caps > that are not exactly the size of the window and will crop or center the > image as needed. > > Or is this not possible/desirable in this case? It would be really nice if the kmssink would just accept caps that do not exactly match an available mode of the display and I struggled with it myself. Unfortunately, the kmssink is also responsible for setting the video mode including the output resolution which leads to two problems: If the input resolution does not match the supported mode, which mode should be selected? - the preferred mode of the monitor - the mode that results in the least cropping - the mode that results in the smallest letterbox I am not sure which behavior would be correct or suitable as default. Furthermore, the kmssink then would need to propose caps that it prefers the resolution of the default mode, supports the resolutions of the available modes, but in the end supports all resolutions. This leads to difficulties with elements that also support various output resolutions, e.g., scalers or cropping elements and the videotestsrc, and I don't see how I could express this in caps.