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

Thu Sep 19 08:18:04 UTC 2019

On Thu, Sep 19, 2019 at 9:18 AM Pekka Paalanen <ppaalanen at gmail.com> wrote:
>
> 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.

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