Re: [PATCH v3 1/3] drm: add generic zpos property

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jan 15, 2016 at 11:12:25AM +0100, Daniel Vetter wrote:
> On Fri, Jan 15, 2016 at 10:09:14AM +0100, Marek Szyprowski wrote:
> > Hello,
> > 
> > On 2016-01-14 11:46, Ville Syrjälä wrote:
> > >On Tue, Jan 12, 2016 at 02:39:18PM +0100, Marek Szyprowski wrote:
> > >>This patch adds support for generic plane's zpos property property with
> > >>well-defined semantics:
> > >>- added zpos properties to drm core and plane state structures
> > >>- added helpers for normalizing zpos properties of given set of planes
> > >>- well defined semantics: planes are sorted by zpos values and then plane
> > >>   id value if zpos equals
> > >>
> > >>Normalized zpos values are calculated automatically when generic
> > >>muttable zpos property has been initialized. Drivers can simply use
> > >>plane_state->normalized_zpos in their atomic_check and/or plane_update
> > >>callbacks without any additional calls to DRM core.
> > >>
> > >>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > >>---
> > >>  Documentation/DocBook/gpu.tmpl      |  14 ++++-
> > >>  drivers/gpu/drm/drm_atomic.c        |   4 ++
> > >>  drivers/gpu/drm/drm_atomic_helper.c | 116 ++++++++++++++++++++++++++++++++++++
> > >>  drivers/gpu/drm/drm_crtc.c          |  53 ++++++++++++++++
> > >>  include/drm/drm_crtc.h              |  14 +++++
> > >>  5 files changed, 199 insertions(+), 2 deletions(-)
> > >>
> > >>diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> > >>index 6c6e81a9eaf4..f6b7236141b6 100644
> > >>--- a/Documentation/DocBook/gpu.tmpl
> > >>+++ b/Documentation/DocBook/gpu.tmpl
> > >>@@ -2004,7 +2004,7 @@ void intel_crt_init(struct drm_device *dev)
> > >>  	<td valign="top" >Description/Restrictions</td>
> > >>  	</tr>
> > >>  	<tr>
> > >>-	<td rowspan="37" valign="top" >DRM</td>
> > >>+	<td rowspan="38" valign="top" >DRM</td>
> > >>  	<td valign="top" >Generic</td>
> > >>  	<td valign="top" >“rotation”</td>
> > >>  	<td valign="top" >BITMASK</td>
> > >>@@ -2256,7 +2256,7 @@ void intel_crt_init(struct drm_device *dev)
> > >>  	<td valign="top" >property to suggest an Y offset for a connector</td>
> > >>  	</tr>
> > >>  	<tr>
> > >>-	<td rowspan="3" valign="top" >Optional</td>
> > >>+	<td rowspan="4" valign="top" >Optional</td>
> > >>  	<td valign="top" >“scaling mode”</td>
> > >>  	<td valign="top" >ENUM</td>
> > >>  	<td valign="top" >{ "None", "Full", "Center", "Full aspect" }</td>
> > >>@@ -2280,6 +2280,16 @@ void intel_crt_init(struct drm_device *dev)
> > >>  	<td valign="top" >TBD</td>
> > >>  	</tr>
> > >>  	<tr>
> > >>+	<td valign="top" > "zpos" </td>
> > >>+	<td valign="top" >RANGE</td>
> > >>+	<td valign="top" >Min=0, Max=255</td>
> > >>+	<td valign="top" >Plane</td>
> > >>+	<td valign="top" >Plane's 'z' position during blending (0 for background, 255 for frontmost).
> > >>+		If two planes assigned to same CRTC have equal zpos values, the plane with higher plane
> > >>+		id is treated as closer to front. Can be IMMUTABLE if driver doesn't support changing
> > >>+		planes' order.</td>
> > >I don't think this is going to work very well. Mixing the same range
> > >of 0-255 for all planes, with potentially some of them being
> > >IMMUTABLE for sure won't end well, or at least won't allow userspace to
> > >see if there are any constraints between the zpos of the planes.
> > >
> > >So I think what we should do is let the driver specify the valid range,
> > >and get rid of the obj id based conflict resolution in favor of just
> > >rejecting conflicts outright. In cases where you can't move the planes
> > >between crtcs, the driver ought to specify the range based on the
> > >number of planes present on the crtc. If planes can be moved betweens
> > >crtcs the range obviously needs to be larger to accomodate all the
> > >possible planes on the crtc.
> > >
> > >Eg. on Intel VLV/CHV we could have the following setup:
> > >primary  zpos 0-2
> > >sprite 0 zpos 0-2
> > >sprite 1 zpos 0-2
> > >cursor   zpos 3
> > >
> > >That makes it very clear the curso is always on top, and the other
> > >planes can be rearranged more or less freely. These planes can't be
> > >moved between crtcs, so each there's an identical set of planes for
> > >each crtc.
> > >
> > >On old Intel hw (gen2/3) we could have something like:
> > >plane A zpos 0-3
> > >plane B zpos 0-3
> > >plane C zpos 0-3
> > >overlay zpos 0-3
> > >cursor B zpos 4
> > >cursor A zpos 5
> > >
> > >Most of these planes can be moved between crtcs, and IIRC there
> > >are probably more constraints on exactly how they can be stacked, but
> > >this is at least fairly close to the truth. Again the cursors are always
> > >on top, and in this case the order between the two cursor planes is also
> > >fixed.
> > 
> > I wasn't aware of a hardware, which has limited configuration of zpos only
> > to some planes. I thought only about 2 cases: either completely configurable
> > planes arrangement, or planes fixed to some hw dependent order. I see no
> > problem to let drivers to define their own limits for mutable zpos property.
> > 
> > Now the question is weather we should allow to set the non-zero minimal
> > value
> > for mutable zpos? I can imagine that there might be a hardware, which has
> > fixed background plane and a few configurable overlay planes. I assume that
> > in such case, the calculated normalized_zpos should still start from zero.
> 
> The usual approach is to switch the property from a global one in
> dev->mode_config.*_prop to a per-object one in e.g. plane->zpos_prop. We
> can still register the name, but have different limits/types. That way you
> could register a global mutable zpos with the range 0-3 and an immutable
> zpos with values 4 or 5 for cursors. The generic decoding would still be
> fairly simple.
> 
> 	} else if (property == plane->zpos_property) {
> 		state->zpos = val;
> 	} else if (property == config->zpos_property) {
> 		state->zpos = val;
> 
> Plus some checking that no one attached both the global and obj-local
> property of the same name to the same object.
> 
> Since this is a fairly simple add-on change and current drivers that
> support zpos don't need it I think it makes total sense to do this as a
> follow-up when we need it.
> 
> But might be good to document this somewhere (either commit message or
> kerneldoc for zpos).
> 
> > >I did originally have the same obj id based sorting idea (and even
> > >posted some kind of a patch for it IIRC) but that was before atomic
> > >existed, so there was a real need to allow reordering the planes with
> > >just a single setplane ioctl call. With atomic I don't see any real
> > >benefit from it the obj id based sorting, and as I've noted there are
> > >definitely downsides to it.
> > 
> > What are the downsides of using obj id as additional sorting criteria? It
> > solves all the ambiguities and simplifies checks (there is no need to check
> > if there are 2 planes of the same zpos value).
> 
> I think the sorting also has an upside that it avoids having to change all
> the drivers, or adding some kind of flag. Drivers which don't support zpos
> just get a sorted normlized zpos. If we don't do that we'd have to put
> some unique default zpos in, which is going to make things messy.

Why would it make it messy? We already assign the obj ids in order (in
practice), so assigning a unique zpos in order would end up doing with
the same result.

-- 
Ville Syrjälä
Intel OTC