[PATCH] drm: two planes with the same zpos have undefined ordering

[Pekka Paalanen]

On Mon, 16 Sep 2019 09:19:09 +0000
Simon Ser <contact at emersion.fr> wrote:

> > On Tue, 10 Sep 2019 11:20:16 +0000
> > Simon Ser <contact at emersion.fr> wrote:
> >  
> > > On Tuesday, September 10, 2019 1:38 PM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > >  
> > > > On Tue, 10 Sep 2019 10:09:55 +0000
> > > > Simon Ser contact at emersion.fr 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 at emersion.fr
> > > > >
> > > > > ---------------------------------------------
> > > > >
> > > > > 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.

> [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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190919/0fb7170d/attachment.sig>