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@xxxxxxxxxxx>
> > >>---
> > >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
