Re: [PATCH v4] drm/omap: plane zpos/zorder management improvements

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Peter,

On Monday, 8 January 2018 10:20:24 EET Peter Ujfalusi wrote:
> On 2018-01-05 16:04, Laurent Pinchart wrote:
> > On Friday, 5 January 2018 13:30:37 EET Peter Ujfalusi wrote:
> >> Use the plane index as default zpos for all planes. Even if the
> >> application is not setting zpos/zorder explicitly we will have unique
> >> zpos for each plane.
> >> 
> >> Enforce that all planes must have unique zpos on the given crtc.
> > 
> > Could you explain the rationale for that in the commit message, what's
> > wrong with duplicate zpos values ?
> 
> Planes with identical zpos is only 'valid' _if_ they are not
> overlapping, if they do overlap then it is - imho - not a valid
> configuration anyway (which one should be on top?).
> Based on my tests the plane with lower planeID is going to disappear
> from the screen if we have duplicated zpos.

Please see my reply to Tomi on this topic. I'm not against the change, but I 
think the rationale should be captured in the commit message.

> > Isn't there a risk of breaking the non-atomic userspace with this ?
> > Without atomic commits userspace can't change the zpos of multiple planes
> > in one go, so it might be impossible to reorder planes without going
> > through a state that has duplicated zpos values.
> 
> Two planes occupying the same position on the screen is not valid
> (again, imho).

At the hardware level for the DSS, sure. According to the KMS API, however, it 
is valid, even if the conflict resolution is driver-dependent.

> If application wants to swap two planes, then it must disable one, move the
> other to the new position, then enable and move the first plane.

Applications don't do that at the moment, so there's a risk of breakage. As 
the current behaviour is undefined we might not considered that as a problem, 
but there's a risk of returning an error for an operation that currently 
succeeds.

Personally I think all applications should move to the atomic API and handle 
zpos explicitly so I don't mind too much :)

> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> >> ---
> >> Hi,
> >> 
> >> Changes since v3:
> >> - Use drm_plane_index() instead of storing the same index wothin
> >>   omap_plane struct
> >> - Optimize the zpos validation loop so we avoid extra checks.
> >> 
> >> Changes since v2:
> >> - The check for duplicate zpos is moved to omap_crtc
> >> 
> >> Changes since v1:
> >> - Dropped the zpos normalization related patches
> >> - New patch based on the discussion, see commit message.
> >> 
> >> Regards,
> >> Peter
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_crtc.c  | 36 +++++++++++++++++++++++++++++-
> >>  drivers/gpu/drm/omapdrm/omap_plane.c | 15 ++++-----------
> >>  2 files changed, 39 insertions(+), 12 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel