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

Commit Message

Simon Ser Sept. 23, 2019, 12:39 p.m. UTC

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.

While at it, remove the assumption that zpos is only for overlay planes.

Additionally, update the drm_plane_state.zpos docs to clarify that zpos
disambiguation via plane object IDs is a recommendation for drivers, not
something user-space can rely on.

v2: clarify drm_plane_state.zpos docs (Daniel)

v3: zpos is for all planes (Marius, Daniel)

v4: completely reword the drm_plane_state.zpos docs to make it clear the
recommendation to use plane IDs is for drivers in case user-space uses
duplicate zpos values (Pekka)

Signed-off-by: Simon Ser <[e-mail]>
Cc: Pekka Paalanen <[e-mail]>
Cc: Marius Vlad <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
---
 drivers/gpu/drm/drm_blend.c | 8 ++++----
 include/drm/drm_plane.h     | 9 +++++----
 2 files changed, 9 insertions(+), 8 deletions(-)

--
2.23.0

Comments

Pekka Paalanen Sept. 24, 2019, 7:26 a.m. UTC | #1

On Mon, 23 Sep 2019 12:39:20 +0000
Simon Ser <[e-mail]> 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.

Hi Simon,

the commit message still talks about the drivers only, and I think that
is what lead me astray in the first place until I realized the
duplicate zpos value issue concerns only stupid userspace, not that
drivers were allowed to use duplicate zpos values which userspace then
needs to untangle with ID ordering. Drivers never have undefined plane
ordering themselves - if they do, that's a broken driver that doesn't
know what hardware it is driving. If the driver doesn't know, then
userspace definitely cannot know any better - if some userspace does,
that's a gfx driver stack misdesign. So there is no reason for a driver
to use ambiguous zpos values.

Anyway, it's fine. The actual doc below is what matters here.

> 
> While at it, remove the assumption that zpos is only for overlay planes.
> 
> Additionally, update the drm_plane_state.zpos docs to clarify that zpos
> disambiguation via plane object IDs is a recommendation for drivers, not
> something user-space can rely on.
> 
> v2: clarify drm_plane_state.zpos docs (Daniel)
> 
> v3: zpos is for all planes (Marius, Daniel)
> 
> v4: completely reword the drm_plane_state.zpos docs to make it clear the
> recommendation to use plane IDs is for drivers in case user-space uses
> duplicate zpos values (Pekka)
> 
> Signed-off-by: Simon Ser <[e-mail]>
> Cc: Pekka Paalanen <[e-mail]>
> Cc: Marius Vlad <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> ---
>  drivers/gpu/drm/drm_blend.c | 8 ++++----
>  include/drm/drm_plane.h     | 9 +++++----
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index d02709dd2d4a..121481f6aa71 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 planes, see drm_plane_create_zpos_immutable_property().
>   *
>   * pixel blend mode:
>   *	Pixel blend mode is set up with drm_plane_create_blend_mode_property().
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index cd5903ad33f7..328773690851 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -140,10 +140,11 @@ struct drm_plane_state {
>  	 * @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.
> +	 * User-space may set mutable zpos properties so that multiple active
> +	 * planes on the same CRTC have identical zpos values. This is a
> +	 * user-space bug, but drivers can solve the conflict by comparing the
> +	 * plane object IDs; the plane with a higher ID is 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.
> --
> 2.23.0

Reviewed-by: Pekka Paalanen <[e-mail]>


Thanks,
pq

Simon Ser Sept. 24, 2019, 7:34 a.m. UTC | #2

On Tuesday, September 24, 2019 10:26 AM, Pekka Paalanen <[e-mail]> wrote:

> On Mon, 23 Sep 2019 12:39:20 +0000
> Simon Ser [e-mail] 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.
>
> Hi Simon,
>
> the commit message still talks about the drivers only, and I think that
> is what lead me astray in the first place until I realized the
> duplicate zpos value issue concerns only stupid userspace, not that
> drivers were allowed to use duplicate zpos values which userspace then
> needs to untangle with ID ordering.

> Drivers never have undefined plane
> ordering themselves - if they do, that's a broken driver that doesn't
> know what hardware it is driving. If the driver doesn't know, then
> userspace definitely cannot know any better - if some userspace does,
> that's a gfx driver stack misdesign. So there is no reason for a driver
> to use ambiguous zpos values.

This patch in fact explains why duplicate plane IDs are allowed. The
two alternatives are mentioned: either disallow duplicate plane zpos
values, or either make ordering undefined.

I chose to allow duplicate values because some HW might not know the
zpos ordering between e.g. some overlay planes, but might know primary
is under overlays.

If that doesn't make sense at all, I'd be happy to change the spec to
say that duplicate zpos values are a kernel bug. We'll need to make
sure this doesn't happen, e.g. with a check in the function creating
the property.

Note that user-space needs to handle undefined order between planes
anyway in case zpos isn't advertised.

Pekka Paalanen Sept. 24, 2019, 8:48 a.m. UTC | #3

On Tue, 24 Sep 2019 07:34:30 +0000
Simon Ser <[e-mail]> wrote:

> On Tuesday, September 24, 2019 10:26 AM, Pekka Paalanen <[e-mail]> wrote:
> 
> > On Mon, 23 Sep 2019 12:39:20 +0000
> > Simon Ser [e-mail] 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.  
> >
> > Hi Simon,
> >
> > the commit message still talks about the drivers only, and I think that
> > is what lead me astray in the first place until I realized the
> > duplicate zpos value issue concerns only stupid userspace, not that
> > drivers were allowed to use duplicate zpos values which userspace then
> > needs to untangle with ID ordering.  
> 
> > Drivers never have undefined plane
> > ordering themselves - if they do, that's a broken driver that doesn't
> > know what hardware it is driving. If the driver doesn't know, then
> > userspace definitely cannot know any better - if some userspace does,
> > that's a gfx driver stack misdesign. So there is no reason for a driver
> > to use ambiguous zpos values.  
> 
> This patch in fact explains why duplicate plane IDs are allowed. The
> two alternatives are mentioned: either disallow duplicate plane zpos
> values, or either make ordering undefined.
> 
> I chose to allow duplicate values because some HW might not know the
> zpos ordering between e.g. some overlay planes, but might know primary
> is under overlays.

Ok, seems like we are still crossing some streams here. There are two
different and independent cases:
- zpos set by drivers
- zpos set by userspace

Zpos set by drivers into nonsensical (duplicate) values is a driver
bug to me. Drivers do have the opportunity to not advertise zpos at all
if they don't know, even if that makes no sense to me.

I assumed that zpos set by userspace must be allowed to have duplicate
zpos values, because such broken userspace already exists. But do they
exist? If probably not, then atomic check failing on duplicate zpos
would be nice. But there must be some reason why the doc already
mentions that ID ordering rule for making sense of ambiguous userspace.

> If that doesn't make sense at all, I'd be happy to change the spec to
> say that duplicate zpos values are a kernel bug. We'll need to make
> sure this doesn't happen, e.g. with a check in the function creating
> the property.

I thought that was already the case.

> Note that user-space needs to handle undefined order between planes
> anyway in case zpos isn't advertised.

Of course.


Thanks,
pq

Simon Ser Sept. 24, 2019, 8:57 a.m. UTC | #4

On Tuesday, September 24, 2019 11:48 AM, Pekka Paalanen <[e-mail]> wrote:

> On Tue, 24 Sep 2019 07:34:30 +0000
> Simon Ser [e-mail] wrote:
>
> > On Tuesday, September 24, 2019 10:26 AM, Pekka Paalanen [e-mail] wrote:
> >
> > > On Mon, 23 Sep 2019 12:39:20 +0000
> > > Simon Ser [e-mail] 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.
> > >
> > > Hi Simon,
> > > the commit message still talks about the drivers only, and I think that
> > > is what lead me astray in the first place until I realized the
> > > duplicate zpos value issue concerns only stupid userspace, not that
> > > drivers were allowed to use duplicate zpos values which userspace then
> > > needs to untangle with ID ordering.
> >
> > > Drivers never have undefined plane
> > > ordering themselves - if they do, that's a broken driver that doesn't
> > > know what hardware it is driving. If the driver doesn't know, then
> > > userspace definitely cannot know any better - if some userspace does,
> > > that's a gfx driver stack misdesign. So there is no reason for a driver
> > > to use ambiguous zpos values.
> >
> > This patch in fact explains why duplicate plane IDs are allowed. The
> > two alternatives are mentioned: either disallow duplicate plane zpos
> > values, or either make ordering undefined.
> > I chose to allow duplicate values because some HW might not know the
> > zpos ordering between e.g. some overlay planes, but might know primary
> > is under overlays.
>
> Ok, seems like we are still crossing some streams here. There are two
> different and independent cases:
>
> -   zpos set by drivers
> -   zpos set by userspace

I think we sorted out the "zpos set by userspace" properly. Let's just
focus on "zpos set by drivers".

> Zpos set by drivers into nonsensical (duplicate) values is a driver
> bug to me. Drivers do have the opportunity to not advertise zpos at all
> if they don't know, even if that makes no sense to me.

I don't know about that. Let's say a driver knows it has underlay
planes, but doesn't know their relative ordering for each revision of
the HW. Maybe it could just set the same zpos for all underlays to
signal to user-space that these planes are under the primary plane?

I'm no driver developer so I don't know whether this could happen. The
goal of this patch is to allow drivers to provide partial zpos info to
user-space (better than no info at all, especially when overlay planes
have undefined ordering wrt. the primary plane when zpos is not
advertised).

> I assumed that zpos set by userspace must be allowed to have duplicate
> zpos values, because such broken userspace already exists. But do they
> exist? If probably not, then atomic check failing on duplicate zpos
> would be nice. But there must be some reason why the doc already
> mentions that ID ordering rule for making sense of ambiguous userspace.

I don't know, I haven't found anything explaining where these dup zpos
values set by user-space are coming from.

> > If that doesn't make sense at all, I'd be happy to change the spec to
> > say that duplicate zpos values are a kernel bug. We'll need to make
> > sure this doesn't happen, e.g. with a check in the function creating
> > the property.
>
> I thought that was already the case.

No, this patch specifically says duplicate zpos (coming from the driver
or set by user-space) will have undefined ordering.

> > Note that user-space needs to handle undefined order between planes
> > anyway in case zpos isn't advertised.
>
> Of course.
>
> Thanks,
> pq

James Qian Wang Sept. 29, 2019, 6:15 a.m. UTC | #5

On Mon, Sep 23, 2019 at 12:39:20PM +0000, Simon Ser 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.

Some Arm's usage cases about two planes with same zpos.

- "Smart layer"
which can accepts 4 different fbs with different src/display rect,
but this smart layer has one restriction:

4 display rects must have no overlaps in V direction
(A optimization for android window like Toolbar/Navigation bar)

So when map this Smart-layer to drm world, it might be 4 different
drm-planes, but with same zorder to indicate that these 4 planes are
smart-laye planes.

- "VR-Layer"
One VR-layer comprises two different viewports which can be configured
with totoally different fbs, src/display rect.
we use two differernt drm-planes to drive on HW "VR-Layer", and these
two drm-planes must be configured with same zpos.

Thanks
James

> While at it, remove the assumption that zpos is only for overlay planes.
> 
> Additionally, update the drm_plane_state.zpos docs to clarify that zpos
> disambiguation via plane object IDs is a recommendation for drivers, not
> something user-space can rely on.
> 
> v2: clarify drm_plane_state.zpos docs (Daniel)
> 
> v3: zpos is for all planes (Marius, Daniel)
> 
> v4: completely reword the drm_plane_state.zpos docs to make it clear the
> recommendation to use plane IDs is for drivers in case user-space uses
> duplicate zpos values (Pekka)
> 
> Signed-off-by: Simon Ser <[e-mail]>
> Cc: Pekka Paalanen <[e-mail]>
> Cc: Marius Vlad <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> ---
>  drivers/gpu/drm/drm_blend.c | 8 ++++----
>  include/drm/drm_plane.h     | 9 +++++----
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> --
> 2.23.0
> 
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index d02709dd2d4a..121481f6aa71 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 planes, see drm_plane_create_zpos_immutable_property().
>   *
>   * pixel blend mode:
>   *	Pixel blend mode is set up with drm_plane_create_blend_mode_property().
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index cd5903ad33f7..328773690851 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -140,10 +140,11 @@ struct drm_plane_state {
>  	 * @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.
> +	 * User-space may set mutable zpos properties so that multiple active
> +	 * planes on the same CRTC have identical zpos values. This is a
> +	 * user-space bug, but drivers can solve the conflict by comparing the
> +	 * plane object IDs; the plane with a higher ID is 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.

Simon Ser Sept. 29, 2019, 8:30 p.m. UTC | #6

Hi,

> On Mon, Sep 23, 2019 at 12:39:20PM +0000, Simon Ser 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.
>
> Some Arm's usage cases about two planes with same zpos.
>
> - "Smart layer"
> which can accepts 4 different fbs with different src/display rect,
> but this smart layer has one restriction:
>
> 4 display rects must have no overlaps in V direction
> (A optimization for android window like Toolbar/Navigation bar)
>
> So when map this Smart-layer to drm world, it might be 4 different
> drm-planes, but with same zorder to indicate that these 4 planes are
> smart-laye planes.
>
> - "VR-Layer"
> One VR-layer comprises two different viewports which can be configured
> with totoally different fbs, src/display rect.
> we use two differernt drm-planes to drive on HW "VR-Layer", and these
> two drm-planes must be configured with same zpos.

Thanks a lot for your feedback James, that's exactly what I was looking for.
Both of these use-cases make sense to me.

I think user-space should be prepared to handle identical immutable zpos values.
Pekka and Daniel, thoughts?

In any case, this doesn't change the patch itself. Probably something worth
mentionning in the commit message.

Pekka Paalanen Sept. 30, 2019, 7:07 a.m. UTC | #7

On Sun, 29 Sep 2019 20:30:44 +0000
Simon Ser <[e-mail]> wrote:

> Hi,
> 
> > On Mon, Sep 23, 2019 at 12:39:20PM +0000, Simon Ser 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.  
> >
> > Some Arm's usage cases about two planes with same zpos.
> >
> > - "Smart layer"
> > which can accepts 4 different fbs with different src/display rect,
> > but this smart layer has one restriction:
> >
> > 4 display rects must have no overlaps in V direction
> > (A optimization for android window like Toolbar/Navigation bar)
> >
> > So when map this Smart-layer to drm world, it might be 4 different
> > drm-planes, but with same zorder to indicate that these 4 planes are
> > smart-laye planes.
> >
> > - "VR-Layer"
> > One VR-layer comprises two different viewports which can be configured
> > with totoally different fbs, src/display rect.
> > we use two differernt drm-planes to drive on HW "VR-Layer", and these
> > two drm-planes must be configured with same zpos.  
> 
> Thanks a lot for your feedback James, that's exactly what I was looking for.
> Both of these use-cases make sense to me.
> 
> I think user-space should be prepared to handle identical immutable zpos values.
> Pekka and Daniel, thoughts?

Hi,

given those explained use cases, sure, I agree.

> In any case, this doesn't change the patch itself. Probably something worth
> mentionning in the commit message.

Yes, recording these use cases would be very nice.


Thanks,
pq

Daniel Vetter Oct. 8, 2019, 9:59 a.m. UTC | #8

On Mon, Sep 30, 2019 at 10:07:07AM +0300, Pekka Paalanen wrote:
> On Sun, 29 Sep 2019 20:30:44 +0000
> Simon Ser <[e-mail]> wrote:
> 
> > Hi,
> > 
> > > On Mon, Sep 23, 2019 at 12:39:20PM +0000, Simon Ser 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.  
> > >
> > > Some Arm's usage cases about two planes with same zpos.
> > >
> > > - "Smart layer"
> > > which can accepts 4 different fbs with different src/display rect,
> > > but this smart layer has one restriction:
> > >
> > > 4 display rects must have no overlaps in V direction
> > > (A optimization for android window like Toolbar/Navigation bar)
> > >
> > > So when map this Smart-layer to drm world, it might be 4 different
> > > drm-planes, but with same zorder to indicate that these 4 planes are
> > > smart-laye planes.
> > >
> > > - "VR-Layer"
> > > One VR-layer comprises two different viewports which can be configured
> > > with totoally different fbs, src/display rect.
> > > we use two differernt drm-planes to drive on HW "VR-Layer", and these
> > > two drm-planes must be configured with same zpos.  
> > 
> > Thanks a lot for your feedback James, that's exactly what I was looking for.
> > Both of these use-cases make sense to me.
> > 
> > I think user-space should be prepared to handle identical immutable zpos values.
> > Pekka and Daniel, thoughts?
> 
> Hi,
> 
> given those explained use cases, sure, I agree.
> 
> > In any case, this doesn't change the patch itself. Probably something worth
> > mentionning in the commit message.
> 
> Yes, recording these use cases would be very nice.

There's a lot more hw that does the same tricks (qualcom is one for sure).
Imo before we encode this we should make sure that:
a) everyone is happy with this new uapi
b) drivers are consistent
c) maybe even testcases in igt
d) definitely an open source user
e) no breaking of existing userspace

I.e. definitely a separate patch.
-Daniel

Pekka Paalanen Oct. 8, 2019, 11:39 a.m. UTC | #9

On Tue, 8 Oct 2019 11:59:04 +0200
Daniel Vetter <[e-mail]> wrote:

> On Mon, Sep 30, 2019 at 10:07:07AM +0300, Pekka Paalanen wrote:
> > On Sun, 29 Sep 2019 20:30:44 +0000
> > Simon Ser <[e-mail]> wrote:
> >   
> > > Hi,
> > >   
> > > > On Mon, Sep 23, 2019 at 12:39:20PM +0000, Simon Ser 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.    
> > > >
> > > > Some Arm's usage cases about two planes with same zpos.
> > > >
> > > > - "Smart layer"
> > > > which can accepts 4 different fbs with different src/display rect,
> > > > but this smart layer has one restriction:
> > > >
> > > > 4 display rects must have no overlaps in V direction
> > > > (A optimization for android window like Toolbar/Navigation bar)
> > > >
> > > > So when map this Smart-layer to drm world, it might be 4 different
> > > > drm-planes, but with same zorder to indicate that these 4 planes are
> > > > smart-laye planes.
> > > >
> > > > - "VR-Layer"
> > > > One VR-layer comprises two different viewports which can be configured
> > > > with totoally different fbs, src/display rect.
> > > > we use two differernt drm-planes to drive on HW "VR-Layer", and these
> > > > two drm-planes must be configured with same zpos.    
> > > 
> > > Thanks a lot for your feedback James, that's exactly what I was looking for.
> > > Both of these use-cases make sense to me.
> > > 
> > > I think user-space should be prepared to handle identical immutable zpos values.
> > > Pekka and Daniel, thoughts?  
> > 
> > Hi,
> > 
> > given those explained use cases, sure, I agree.
> >   
> > > In any case, this doesn't change the patch itself. Probably something worth
> > > mentionning in the commit message.  
> > 
> > Yes, recording these use cases would be very nice.  
> 
> There's a lot more hw that does the same tricks (qualcom is one for sure).
> Imo before we encode this we should make sure that:
> a) everyone is happy with this new uapi

Sorry, what new UAPI?

We're just trying to make the documentation match what currently
happens, right?


Thanks,
pq


> b) drivers are consistent
> c) maybe even testcases in igt
> d) definitely an open source user
> e) no breaking of existing userspace
> 
> I.e. definitely a separate patch.
> -Daniel

Daniel Vetter Oct. 8, 2019, 3:03 p.m. UTC | #10

On Tue, Oct 8, 2019 at 1:39 PM Pekka Paalanen <[e-mail]> wrote:
>
> On Tue, 8 Oct 2019 11:59:04 +0200
> Daniel Vetter <[e-mail]> wrote:
>
> > On Mon, Sep 30, 2019 at 10:07:07AM +0300, Pekka Paalanen wrote:
> > > On Sun, 29 Sep 2019 20:30:44 +0000
> > > Simon Ser <[e-mail]> wrote:
> > >
> > > > Hi,
> > > >
> > > > > On Mon, Sep 23, 2019 at 12:39:20PM +0000, Simon Ser 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.
> > > > >
> > > > > Some Arm's usage cases about two planes with same zpos.
> > > > >
> > > > > - "Smart layer"
> > > > > which can accepts 4 different fbs with different src/display rect,
> > > > > but this smart layer has one restriction:
> > > > >
> > > > > 4 display rects must have no overlaps in V direction
> > > > > (A optimization for android window like Toolbar/Navigation bar)
> > > > >
> > > > > So when map this Smart-layer to drm world, it might be 4 different
> > > > > drm-planes, but with same zorder to indicate that these 4 planes are
> > > > > smart-laye planes.
> > > > >
> > > > > - "VR-Layer"
> > > > > One VR-layer comprises two different viewports which can be configured
> > > > > with totoally different fbs, src/display rect.
> > > > > we use two differernt drm-planes to drive on HW "VR-Layer", and these
> > > > > two drm-planes must be configured with same zpos.
> > > >
> > > > Thanks a lot for your feedback James, that's exactly what I was looking for.
> > > > Both of these use-cases make sense to me.
> > > >
> > > > I think user-space should be prepared to handle identical immutable zpos values.
> > > > Pekka and Daniel, thoughts?
> > >
> > > Hi,
> > >
> > > given those explained use cases, sure, I agree.
> > >
> > > > In any case, this doesn't change the patch itself. Probably something worth
> > > > mentionning in the commit message.
> > >
> > > Yes, recording these use cases would be very nice.
> >
> > There's a lot more hw that does the same tricks (qualcom is one for sure).
> > Imo before we encode this we should make sure that:
> > a) everyone is happy with this new uapi
>
> Sorry, what new UAPI?
>
> We're just trying to make the documentation match what currently
> happens, right?

It's so much new uapi that I've sent out a few reverts for 5.4 (it
landed in that merge window) to undo the stuff the arm driver team has
done (it didn't come with userspace, proper discussion on dri-devel,
docs or testcases in igt). I also just spotted that a leftover is that
arm/komeda still computes its own version of normalized_zpos, which
probably should be ditched too (it's not actually different from the
one in helpers without the reverted uapi).

So yeah, separate patch :-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
[phone] - http://blog.ffwll.ch

Simon Ser Oct. 8, 2019, 3:11 p.m. UTC | #11

On Tuesday, October 8, 2019 6:03 PM, Daniel Vetter <[e-mail]> wrote:

> > > > > In any case, this doesn't change the patch itself. Probably something worth
> > > > > mentionning in the commit message.
> > > >
> > > > Yes, recording these use cases would be very nice.
> > >
> > > There's a lot more hw that does the same tricks (qualcom is one for sure).
> > > Imo before we encode this we should make sure that:
> > > a) everyone is happy with this new uapi
> >
> > Sorry, what new UAPI?
> > We're just trying to make the documentation match what currently
> > happens, right?
>
> It's so much new uapi that I've sent out a few reverts for 5.4 (it
> landed in that merge window) to undo the stuff the arm driver team has
> done (it didn't come with userspace, proper discussion on dri-devel,
> docs or testcases in igt). I also just spotted that a leftover is that
> arm/komeda still computes its own version of normalized_zpos, which
> probably should be ditched too (it's not actually different from the
> one in helpers without the reverted uapi).
>
> So yeah, separate patch :-)

I don't get it. Do you want to split the docs changes for user-space,
only keeping the doc changes for drivers in this patch?

User-space could already see duplicate zpos because of the non-atomic
API. I don't think this introduces a new uAPI.

Daniel Vetter Oct. 8, 2019, 3:16 p.m. UTC | #12

On Tue, Oct 8, 2019 at 5:11 PM Simon Ser <[e-mail]> wrote:
>
> On Tuesday, October 8, 2019 6:03 PM, Daniel Vetter <[e-mail]> wrote:
>
> > > > > > In any case, this doesn't change the patch itself. Probably something worth
> > > > > > mentionning in the commit message.
> > > > >
> > > > > Yes, recording these use cases would be very nice.
> > > >
> > > > There's a lot more hw that does the same tricks (qualcom is one for sure).
> > > > Imo before we encode this we should make sure that:
> > > > a) everyone is happy with this new uapi
> > >
> > > Sorry, what new UAPI?
> > > We're just trying to make the documentation match what currently
> > > happens, right?
> >
> > It's so much new uapi that I've sent out a few reverts for 5.4 (it
> > landed in that merge window) to undo the stuff the arm driver team has
> > done (it didn't come with userspace, proper discussion on dri-devel,
> > docs or testcases in igt). I also just spotted that a leftover is that
> > arm/komeda still computes its own version of normalized_zpos, which
> > probably should be ditched too (it's not actually different from the
> > one in helpers without the reverted uapi).
> >
> > So yeah, separate patch :-)
>
> I don't get it. Do you want to split the docs changes for user-space,
> only keeping the doc changes for drivers in this patch?
>
> User-space could already see duplicate zpos because of the non-atomic
> API. I don't think this introduces a new uAPI.

I'm talking specifically about the "duplicated zpos values indicate
special cloned planes like in the arm example" clarification. Not
about splitting the zpos documentation any more, that's indeed just
documenting existing uapi. But assigning the special meaning to
duplicated zpos values (instead of just "can happen because non-atomic
legacy userspace"), that is new uapi. Especially if we allow
duplicated zpos for immutable properties (afaik that doesn't exist
yet).
-Daniel

Simon Ser Oct. 8, 2019, 3:23 p.m. UTC | #13

On Tuesday, October 8, 2019 6:16 PM, Daniel Vetter <[e-mail]> wrote:

> On Tue, Oct 8, 2019 at 5:11 PM Simon Ser [e-mail] wrote:
>
> > On Tuesday, October 8, 2019 6:03 PM, Daniel Vetter [e-mail] wrote:
> >
> > > > > > > In any case, this doesn't change the patch itself. Probably something worth
> > > > > > > mentionning in the commit message.
> > > > > >
> > > > > > Yes, recording these use cases would be very nice.
> > > > >
> > > > > There's a lot more hw that does the same tricks (qualcom is one for sure).
> > > > > Imo before we encode this we should make sure that:
> > > > > a) everyone is happy with this new uapi
> > > >
> > > > Sorry, what new UAPI?
> > > > We're just trying to make the documentation match what currently
> > > > happens, right?
> > >
> > > It's so much new uapi that I've sent out a few reverts for 5.4 (it
> > > landed in that merge window) to undo the stuff the arm driver team has
> > > done (it didn't come with userspace, proper discussion on dri-devel,
> > > docs or testcases in igt). I also just spotted that a leftover is that
> > > arm/komeda still computes its own version of normalized_zpos, which
> > > probably should be ditched too (it's not actually different from the
> > > one in helpers without the reverted uapi).
> > > So yeah, separate patch :-)
> >
> > I don't get it. Do you want to split the docs changes for user-space,
> > only keeping the doc changes for drivers in this patch?
> > User-space could already see duplicate zpos because of the non-atomic
> > API. I don't think this introduces a new uAPI.
>
> I'm talking specifically about the "duplicated zpos values indicate
> special cloned planes like in the arm example" clarification. Not
> about splitting the zpos documentation any more, that's indeed just
> documenting existing uapi. But assigning the special meaning to
> duplicated zpos values (instead of just "can happen because non-atomic
> legacy userspace"), that is new uapi. Especially if we allow
> duplicated zpos for immutable properties (afaik that doesn't exist
> yet).

Oh, I see. That makes sense, will send a new version.

Patch

diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index d02709dd2d4a..121481f6aa71 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 planes, see drm_plane_create_zpos_immutable_property().
  *
  * pixel blend mode:
  *	Pixel blend mode is set up with drm_plane_create_blend_mode_property().
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index cd5903ad33f7..328773690851 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -140,10 +140,11 @@  struct drm_plane_state {
 	 * @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.
+	 * User-space may set mutable zpos properties so that multiple active
+	 * planes on the same CRTC have identical zpos values. This is a
+	 * user-space bug, but drivers can solve the conflict by comparing the
+	 * plane object IDs; the plane with a higher ID is 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.