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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux