On Thu, Sep 19, 2019 at 9:18 AM Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote: > > On Mon, 16 Sep 2019 09:19:09 +0000 > Simon Ser <contact@xxxxxxxxxxx> wrote: > > > > On Tue, 10 Sep 2019 11:20:16 +0000 > > > Simon Ser <contact@xxxxxxxxxxx> wrote: > > > > > > > On Tuesday, September 10, 2019 1:38 PM, Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote: > > > > > > > > > On Tue, 10 Sep 2019 10:09:55 +0000 > > > > > Simon Ser contact@xxxxxxxxxxx wrote: > > > > > > > > > > > Currently the property docs don't specify whether it's okay for two planes to > > > > > > have the same zpos value and what user-space should expect in this case. > > > > > > The rule mentionned in the past was to disambiguate with object IDs. However > > > > > > some drivers break this rule (that's why the ordering is documented as > > > > > > unspecified in case the zpos property is missing). Additionally it doesn't > > > > > > really make sense for a driver to user identical zpos values if it knows their > > > > > > relative position: the driver can just pick different values instead. > > > > > > So two solutions would make sense: either disallow completely identical zpos > > > > > > values for two different planes, either make the ordering unspecified. To allow > > > > > > drivers that don't know the relative ordering between two planes to still > > > > > > expose the zpos property, choose the latter solution. > > > > > > > > > > > > Signed-off-by: Simon Ser contact@xxxxxxxxxxx > > > > > > > > > > > > --------------------------------------------- > > > > > > > > > > > > Err, I'm sorry about the double-post. I sent this to intel-gfx by mistake. > > > > > > drivers/gpu/drm/drm_blend.c | 8 ++++---- > > > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c > > > > > > index d02709dd2d4a..51bd5454e50a 100644 > > > > > > --- a/drivers/gpu/drm/drm_blend.c > > > > > > +++ b/drivers/gpu/drm/drm_blend.c > > > > > > @@ -132,10 +132,10 @@ > > > > > > > > > > > > - planes. Without this property the primary plane is always below the cursor > > > > > > - plane, and ordering between all other planes is undefined. The positive > > > > > > - Z axis points towards the user, i.e. planes with lower Z position values > > > > > > > > > > > > - - are underneath planes with higher Z position values. Note that the Z > > > > > > - - position value can also be immutable, to inform userspace about the > > > > > > - - hard-coded stacking of overlay planes, see > > > > > > - - drm_plane_create_zpos_immutable_property(). > > > > > > > > > > > > - - are underneath planes with higher Z position values. Two planes with the > > > > > > - - same Z position value have undefined ordering. Note that the Z position > > > > > > - - value can also be immutable, to inform userspace about the hard-coded > > > > > > - - stacking of overlay planes, see drm_plane_create_zpos_immutable_property(). > > > > > > - > > > > > > - pixel blend mode: > > > > > > - Pixel blend mode is set up with drm_plane_create_blend_mode_property(). > > > > > > > > > > Hi, > > > > > > > > > > this seems to contradict what the docs say in another place: > > > > > > > > Except this comment is about drm_plane_state.zpos, an internal DRM > > > > property. This is not about the zpos property itself. > > > > > > Hi, > > > > > > then I'm very confused. What's the difference? > > > > > > The text you are changing was specifically added to explain uAPI > > > behaviour, that is, what the userspace sees and does with the zpos > > > property in uAPI. > > > > > > Having two conflicting pieces of documentation is confusing, especially > > > when it is not absolutely clear which one is for driver implementors > > > and which one for uAPI consumers, or that one must ignore the other doc > > > depending on who you are. > > > > Yes, I agree that this is confusing. > > > > To be completely honest, I don't really care what the outcome of this > > discussion is, as long as there are no conflicting documentation comments. > > Hi, > > that is exactly what I want too. > > > So, my interpretation of the docs is that there are: > > > > 1. Some documentation for KMS properties, that is, what is exposed to > > user-space via DRM ioctls. This is the KMS uAPI. > > 2. Some documentation for kernel drivers, that is, internal DRM state that can > > be set by kernel developers. This includes helper functions and common > > structs. > > > > Obviously as user-space developers we only care about (1). > > Theoretically yes, but the problem is that one cannot make that > distinction. I'm pretty sure both categories of comments are not > complete, and answers to some questions in one must be dug up from the > other, if documented at all. > > So you cannot use wording that looks conflicting between the two. If > the wording is correct nevertheless, it needs more explaining on how it > doesn't actually conflict, so that people randomly reading just one > side or the other don't get the wrong idea. > > > Now, back to zpos' case: there are two doc comments about zpos. > > > > The first one is [1], the one this patch changes: > > > > > zpos: > > > Z position is set up with drm_plane_create_zpos_immutable_property() and > > > drm_plane_create_zpos_property(). It controls the visibility of overlapping > > > planes. Without this property the primary plane is always below the cursor > > > plane, and ordering between all other planes is undefined. The positive Z > > > axis points towards the user, i.e. planes with lower Z position values are > > > underneath planes with higher Z position values. Note that the Z position > > > value can also be immutable, to inform userspace about the hard-coded > > > stacking of overlay planes, see drm_plane_create_zpos_immutable_property(). > > > > This is in the "Plane Composition Properties". I believe this paragraph > > documents the standard plane properties (standard as in not driver-specific). > > So I'd say this documents the KMS uAPI. > > > > The second one is [2], the one you've quoted: > > > > > zpos > > > > > > Priority of the given plane on crtc (optional). > > > > > > Note that multiple active planes on the same crtc can have an identical zpos > > > value. The rule to solving the conflict is to compare the plane object IDs; > > > the plane with a higher ID must be stacked on top of a plane with a lower ID. > > > > > > See drm_plane_create_zpos_property() and > > > drm_plane_create_zpos_immutable_property() for more details. > > > > This is in the "Plane Functions Reference" section, more precisely in the > > "struct drm_plane_state" docs. I believe this is really just about the common > > DRM struct that can be used by all drivers. This struct isn't exposed to > > user-space. It's just an implementation detail of DRM. > > > > (We can see that even without this patch, these two comments already > > kind of conflict. The first one says that without zpos ordering is > > undefined. The second one says that two identical zpos values means the > > plane ID should be used. However drm_plane_state is zero-filled, so a > > driver not supporting zpos would have all drm_plane_state.zpos fields > > set to zero? Since they are all equal, is the object ID ordering rule > > relevant?) > > Right, and we are suffering from that confusion already. Should > userspace use ID order if zpos property is not there or not? I have no > idea. Nope. I think the only options for this case are: - file bug against upstream driver so they add zpos - you magically know how planes work on that hw - you don't overlap planes at all - cursor is above primary, that much we can guarantee Yes it's kinda uapi fail we didn't add zpos from the start :-/ -Daniel > > > [1]: https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html#plane-composition-properties > > [2]: https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html#plane-functions-reference > > > Thanks, > pq -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
