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(-)