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

[Laurent Pinchart]

Hi Tomi,

On Monday, 8 January 2018 12:58:28 EET Tomi Valkeinen wrote:
> On 08/01/18 12:43, Laurent Pinchart wrote:
> > On Monday, 8 January 2018 10:59:29 EET Tomi Valkeinen wrote:
> >> On 08/01/18 10:20, 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?).
> >> 
> >> For DSS it's clear. It is an invalid HW configuration to have multiple
> >> planes with the same zpos in the same crtc. I believe the result is
> >> undefined HW behavior.
> >> 
> >> So we either return an error, or the kernel normalizes zpos'es.
> >> Normalizing means the kernel is guessing what the end result should be,
> >> so I like error better.
> > 
> > I wouldn't call that guessing :-) Duplicate zpos values result in plane
> > being sorted based on the plane ID (this is obviously
> > implementation-dependent, I mean this is the currently implemented
> > behaviour), which I don't think is an issue in itself. A userspace zpos
> > API that resolves conflicting zpos values that way isn't a broken API,
> > even if its behaviour might be considered a bit complex or cumbersome.
> > 
> > I'm not against forbidding duplicate zpos values, but I think the
> > rationale
> > should be captured in the kernel message.
> > 
> > There's also a risk of breaking non-atomic userspace (as explained in my
> 
> I think they are broken already (on omapdrm): the driver doesn't deal
> with this at the moment in any way, which leads to undefined behavior. I
> may remember wrong, but I think I have seen sync losts connected to bad
> z setup.
> 
> But you are right, it's also possible that nothing bad seems to happen,
> if the only side effect is just that a plane disappears for a moment.
> 
> And if this behavior for non-atocic apps is normal, and other drivers
> allow it, then I do agree that we have to normalize instead of returning
> an error.
> 
> > previous e-mail), as well as atomic userspace that doesn't set zpos
> > explicitly if run after an application that changed the zpos values.
> > True, that would today result in an undefined behaviour, so this might
> > not be considered a problem.
> 
> Yes, this is a subject I have complained a few times: DRM keeping the
> state. I think that will be a problem with many other properties too. An
> app could change a platform specific property, which no other app is
> aware of, and after that no other app would work correctly.
> 
> I believe each app has to know all the DRM properties and set them
> accordingly on startup, and/or each app has to reset the properties when
> exiting. Unfortunately I think both cases are not realistic.

I agree with you. I'd prefer restoring a sane default state on last close. Is 
there any reason not to do so ?

-- 
Regards,

Laurent Pinchart