Hi Ville,
On Thursday 12 May 2016 22:46:05 Ville Syrjälä wrote:
> On Thu, May 12, 2016 at 10:20:12PM +0300, Laurent Pinchart wrote:
> > On Thursday 12 May 2016 14:51:55 Ville Syrjälä wrote:
> >> On Thu, May 12, 2016 at 12:28:19PM +0200, Benjamin Gaignard wrote:
> >>> From: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> >>>
> >>> This patch adds support for generic plane's zpos property property
> >>> with well-defined semantics:
> >>> - added zpos properties to plane 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>
> >>>
> >>> Compare to Marek's original patch zpos property is now specific to
> >>> each plane and no more to the core.
> >>> Normalize function take care of the range of per plane defined range
> >>> before set normalized_zpos.
> >>>
> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx>
> >>>
> >>> Cc: Inki Dae <inki.dae@xxxxxxxxxxx>
> >>> Cc: Daniel Vetter <daniel@xxxxxxxx>
> >>> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> >>> Cc: Joonyoung Shim <jy0922.shim@xxxxxxxxxxx>
> >>> Cc: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>
> >>> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> >>> Cc: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
> >>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> >>> Cc: Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>
> >>> Cc: Gustavo Padovan <gustavo@xxxxxxxxxxx>
> >>> Cc: vincent.abriou@xxxxxx
> >>> Cc: fabien.dessenne@xxxxxx
> >>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >>> ---
> >>>
> >>> Documentation/DocBook/gpu.tmpl | 10 ++
> >>> drivers/gpu/drm/Makefile | 2 +-
> >>> drivers/gpu/drm/drm_atomic.c | 4 +
> >>> drivers/gpu/drm/drm_atomic_helper.c | 6 +
> >>> drivers/gpu/drm/drm_blend.c | 229 ++++++++++++++++++++++++++++
> >>> drivers/gpu/drm/drm_crtc_internal.h | 3 +
> >>> include/drm/drm_crtc.h | 28 +++++
> >>> 7 files changed, 281 insertions(+), 1 deletion(-)
> >>> create mode 100644 drivers/gpu/drm/drm_blend.c
> >
> > [snip]
> >
> >>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> >>> new file mode 100644
> >>> index 0000000..eef59bb
> >>> --- /dev/null
> >>> +++ b/drivers/gpu/drm/drm_blend.c
> >>> @@ -0,0 +1,229 @@
> >
> > [snip]
> >
> >>> +static int drm_atomic_state_zpos_cmp(const void *a, const void *b)
> >>> +{
> >>> + const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
> >>> + const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
> >>> +
> >>> + if (sa->zpos != sb->zpos)
> >>> + return sa->zpos - sb->zpos;
> >>> + else
> >>> + return sa->plane->base.id - sb->plane->base.id;
> >>> +}
> >>
> >> Still not really convinced this normalization is a good idea when we
> >> have atomic. It made sense before atomic since otherwise you might not
> >> be able to swap the zpos of two planes without shutting one of them
> >> down, but it makes interpreting the zpos prop values more confusing. And
> >> with atomic we don't actually need it AFAICS. Or do we have an actual
> >> use case for this behaviour?
> >
> > Speaking selfishly about my use case (rcar-du-drm), I need to sort planes
> > in display order from 0 to N-1 as that's how the hardware is programmed.
> > Normalization gives me an array of sorted planes, which is exactly what I
> > need. I expect other drivers to have similar requirements, so
> > normalization in the core will allow code sharing.
>
> It just eliminates duplicate zpos values. It won't re-sort your plane
> states magically or anything like that. Nor does it guarantee that
> the zpos values for the actually visible planes are even contiguous.
To make normalization useful in my case I need contiguous values starting at
0.
> > Now, is your concern that allowing two planes to have the same zpos isn't
> > needed with the atomic API ? I agree with that, but it would also make it
> > impossible to swap two planes using the legacy API implemented on top of
> > atomic drivers. I'll let others decide whether this is considered as a
> > concern.
Any opinion on this ?
> > While at it, Benjamin told me that you've requested configurable min/max
> > values per plane, what are your use cases for that ? On the same topic,
> > please see below.
> >
> >>> +
> >>> +/**
> >>> + * drm_atomic_helper_crtc_normalize_zpos - calculate normalized zpos
> >>> values
> >>> + * @crtc: crtc with planes, which have to be considered for
> >>> normalization
> >>> + * @crtc_state: new atomic state to apply
> >>> + *
> >>> + * This function checks new states of all planes assigned to given
> >>> crtc and
> >>> + * calculates normalized zpos value for them. Planes are compared
> >>> first by their
> >>> + * zpos values, then by plane id (if zpos equals). Plane with lowest
> >>> zpos value
> >>> + * is at the bottom. The plane_state->normalized_zpos is then filled
> >>> with unique
> >>> + * values from 0 to number of active planes in crtc minus one.
> >>> + *
> >>> + * RETURNS
> >>> + * Zero for success or -errno
> >>> + */
> >>> +int drm_atomic_helper_crtc_normalize_zpos(struct drm_crtc *crtc,
> >>> + struct drm_crtc_state *crtc_state)
> >>> +{
> >>> + struct drm_atomic_state *state = crtc_state->state;
> >>> + struct drm_device *dev = crtc->dev;
> >>> + int total_planes = dev->mode_config.num_total_plane;
> >>> + struct drm_plane_state **states;
> >>> + struct drm_plane *plane;
> >>> + int i, zpos, n = 0;
> >>> + int ret = 0;
> >>> +
> >>> + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] calculating normalized zpos
> >>> values\n",
> >>> + crtc->base.id, crtc->name);
> >>> +
> >>> + states = kmalloc(total_planes * sizeof(*states), GFP_KERNEL);
> >>
> >> kmalloc_array(..., GFP_TEMPORARY);
> >>
> >>> + if (!states)
> >>> + return -ENOMEM;
> >>> +
> >>> + /*
> >>> + * Normalization process might create new states for planes which
> >>> + * normalized_zpos has to be recalculated.
> >>> + */
> >>> + drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> >>> + struct drm_plane_state *plane_state =
> >>> + drm_atomic_get_plane_state(state, plane);
> >>> + if (IS_ERR(plane_state)) {
> >>> + ret = PTR_ERR(plane_state);
> >>> + goto done;
> >>> + }
> >>
> >> Hmm. So if we change the zpos for any plane, we get to re-check every
> >> plane. Not the most efficient thing perhaps, since on some hardware
> >> you might only have a subset of planes that can even change their
> >> zpos. OTOH I suppose it does make life a bit simpler when you don't
> >> have to think which planes might be affected, so I guess it's fine.
> >>
> >>> + states[n++] = plane_state;
> >>> + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] processing zpos value %d\n",
> >>> + plane->base.id, plane->name,
> >>> + plane_state->zpos);
> >>> + }
> >>> +
> >>> + sort(states, n, sizeof(*states), drm_atomic_state_zpos_cmp, NULL);
> >>> +
> >>> + for (i = 0, zpos = 0; i < n; i++, zpos++) {
> >>> + plane = states[i]->plane;
> >>> +
> >>> + zpos = max_t(int, zpos, states[i]->zpos);
> >
> > The purpose of this code is to support the per-plane min/max zpos values.
> > As Benjamin explained, for example if the zpos range is [0,2] for plane X
> > and [4,6] for plane Y, we need to be sure the min zpos value of plane Y
> > is 4. To this I replied that's exactly what I'd rather avoid. I think the
> > core code should take care of sorting planes, taking driver-specific
> > constraints into account when possible (too exotic constraints can always
> > be implemented directly by drivers), and then leave to the drivers the
> > responsibility of taking the sorted planes array and computing whatever
> > hardware-specific configuration is needed. Sorting planes from 0 to N-1
> > seems to be like a better API than having semi-random values for the
> > normalized zpos. Do we have use cases that wouldn't be covered by a
> > simpler implementation ?
>
> The point of exposing ranges is to give userspace at least some chance
> of getting the order right. Otherwise it'll just have to randomly try
> different orders until something works. Most hardware is quite
> constrained in this.
Could you educate me by giving a few real world examples ?
> Some don't support changing the zpos at all, in which case we would set
> min==max.
But all this doesn't require adjusting the normalized zpos to take the
original zpos constraints into account, does it ? Why can normalized_zpos be
constrained to the range 0 to N-1, giving drivers the sorted planes and then
letting them convert that to device-specific values ?
> >>> + states[i]->normalized_zpos = zpos;
> >>> + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] normalized zpos value %d\n",
> >>> + plane->base.id, plane->name, zpos);
> >>> + }
> >>> + crtc_state->zpos_changed = true;
> >>> +
> >>> +done:
> >>> + kfree(states);
> >>> + return ret;
> >>> +}
> >>> +EXPORT_SYMBOL(drm_atomic_helper_crtc_normalize_zpos);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
