GNOME Bugzilla – Bug 768183
kmssink: only works on overlay planes
Last modified: 2016-10-31 14:25:48 UTC
Created attachment 330580 [details] modetest output for Peach Pi Chromebook By default, only overlay planes are returned by the DRM_IOCTL_MODE_GETPLANERESOURCES ioctl unless a client user-space application sets the DRM_CLIENT_CAP_UNIVERSAL_PLANES capability bit to also get the primary and cursor planes. So if a CRTC only has primary planes and not overlay planes associated, kmssink will fail to find a suitable plane for the CRTC, i.e: kmssink gstkmssink.c:482:gst_kms_sink_start:<kmssink0> Could not find a plane for crtc I'm having this issue when trying to use the CRTC of the HDMI connector in both an Exynos5422 Odroid XU4 board and an Exynos5800 Peach Pi Chromebook, were the HDMI CRTC only has a primary plane associated but no overlay plane. I've attached the modetest output on my Peach Pi Chromebook. I came up with two possible approaches to solve this: a) Add a plane-id property so a specific plane can be used instead kmssink finding the first overlay plane that's possible to use with a CRTC, i.e: $ gst-launch-1.0 foo ! kmssink connector-id=36 plane-id=30 The disadvantage of this approach is that it won't work automagically as is the case with overlay planes. b) Add a universal-planes property to specify that universal planes is supported and not restrict the output to only overlay planes, i.e: $ gst-launch-1.0 foo ! kmssink connector-id=36 universal-planes=true The disadvantage of this approach is that I believe it would be preferable to use an overlay plane instead of a primary plane but find_plane_for_crtc () will just return the first possible plane regardless of its type (the type can be queried but with another ioctl call so maybe as a follow-up patch). I'll add patches for both approaches but these are also not mutually exclusive. Even when either of these could solve the issue, I think having both options makes sense since solve slightly different things: choosing a specific plane to use vs allowing to use primary planes.
Created attachment 330582 [details] [review] [PATCH 1/2] kmssink: add a plane-id property
Created attachment 330584 [details] [review] [PATCH 2/2] kmssink: add universal-plane property
I have checked these and they look good to me. Suggested to Javier to ask for comments here since it adds properties and those should be added with care. In this case they make a lot of sense but any API changes should be done thoroughly. Thanks Javier! :)
Review of attachment 330582 [details] [review]: This patch makes perfectly sense to me.
Review of attachment 330584 [details] [review]: I'm not sure about this. I wonder if we could add it as a fallback: if a plane cannot be found, then enable this capability and ask again for a plane.
@Nicolas, I would like to know what do you think. And thanks @Javier :DDD
I think you bring a good point, in the end we want it to just work on most platforms, even if it means RGB only. How much work is it to run this twice, once without, and if there is no overlay, run it again ?
@Victor, @Nicolas yes, I in fact considered that option but then discarded since I thought it would be kind of brute force. But as @Nicolas said to me, sometimes we have to do this due lack of information. I'll propose an alternate patch for 2/2 that does the fallback as you suggested. Probably later today. Thanks a lot for your feedback!
Created attachment 330620 [details] [review] [PATCH 2/2] kmssink: fallback to universal planes if no overlay plane is found Hello, this patch supersedes "[PATCH 2/2] kmssink: add universal-plane property" and fallbacks to universal planes if no overlay plane associated with the CRTC is found as suggested by @Victor, instead of using a universal-plane property like the previous approach.
Comment on attachment 330584 [details] [review] [PATCH 2/2] kmssink: add universal-plane property Forgot to mark this patch as superseded when attaching the new patch, so I'm marking it as obsolete. Sorry for the noise...
Review of attachment 330620 [details] [review]: Looks neat. Just a nitpìck :( ::: sys/kms/gstkmssink.c @@ +495,3 @@ plane_failed: { GST_ERROR_OBJECT (self, "Could not find a plane for crtc"); I would demote this message to a WARNING or lesser.
Right, I think the correct would be to only show the error message on the second try (when universal_planes == TRUE) and not if another try with universal planes is going to be made. What do you think of that instead?
Created attachment 330623 [details] [review] [PATCH v2 2/2] kmssink: fallback to universal planes if no overlay plane is found @Victor, I attached a v2 of patch 2/2 which I think addresses correctly the issue you pointed out. Please let me know what you think. Thanks a lot for your feedback!
Comment on attachment 330623 [details] [review] [PATCH v2 2/2] kmssink: fallback to universal planes if no overlay plane is found lgtm! Thanks!
Comment on attachment 330582 [details] [review] [PATCH 1/2] kmssink: add a plane-id property lgtm!
I will wait to @Nicolas if he has any input and if not, tomorrow I'll push them. Thanks @Javier!
Awesome, thanks a lot for your help @Victor!
Review of attachment 330623 [details] [review]: Looks good to me to.
Review of attachment 330582 [details] [review]: Looks good to me to.