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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Benjamin,

Thank you for the patch.

Could you please reply to Ville's comment to v1 of this patch (posted on April 
the 25th) ?

Please also see below for additional comments.

On Monday 13 Jun 2016 11:21:23 Benjamin Gaignard wrote:
> version 4:
> - make sure that normalized zpos value is stay
>   in the defined property range and warn user if not
> 
> 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@samsung.com>
> 
> 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@linaro.org>
> 
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: vincent.abriou@st.com
> Cc: fabien.dessenne@st.com
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  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         | 230 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc_internal.h |   3 +
>  include/drm/drm_crtc.h              |  30 +++++
>  7 files changed, 284 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..9a5361a
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -0,0 +1,230 @@

[snip]

> +/**
> + * 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_array(total_planes, sizeof(*states), 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;
> +		}
> +		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);
> +
> +		WARN_ON(zpos > plane->zpos_property->values[1]);

This crashes if the plane doesn't have a zpos property. The simplest fix is to 
write the check as

		WARN_ON(plane->zpos_property &&
			zpos > plane->zpos_property->values[1]);

but I wonder how we should handle drivers that instantiate a zpos property for 
a subdev of the planes only. For drivers that don't use zpos at all you could 
maybe avoid calling this function.

Additionally, this check is triggered with the rcar-du-drm driver when 
performing the following operations:

- Perform a mode set with the CRTC primary plane only (that plane doesn't have 
a zpos property)

- Add 7 overlay planes with zpos values 1 to 7 (their zpos property range is 
1-7)

- Modify the zpos value of all overlay planes one by one to 7 to 1 (setting 
zpos 7 for plane 1 first, then zpos 6 for plane 2, ...)

I get normalized zpos values such as

[   84.892927] [PLANE:39:plane-8] normalized zpos value 9
[   85.899284] [PLANE:25:plane-0] normalized zpos value 0
[   85.904488] [PLANE:37:plane-7] normalized zpos value 2
[   85.909633] [PLANE:35:plane-6] normalized zpos value 3
[   85.914793] [PLANE:33:plane-5] normalized zpos value 4
[   85.919936] [PLANE:31:plane-4] normalized zpos value 5
[   85.925100] [PLANE:29:plane-3] normalized zpos value 6
[   85.930245] [PLANE:27:plane-2] normalized zpos value 7

(plane 25 is the primary plane, all other planes are the overlay planes, added 
in the order 27, 29, 31, 33, 35, 37, 37 in the sequence above)

> +		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);
> +
> +/**
> + * drm_atomic_helper_normalize_zpos - calculate normalized zpos values for
> all + *				      crtcs
> + * @dev: DRM device
> + * @state: atomic state of DRM device
> + *
> + * This function calculates normalized zpos value for all modified planes
> in + * the provided atomic state of DRM device. For more information, see +
> * drm_atomic_helper_crtc_normalize_zpos() function.
> + *
> + * RETURNS
> + * Zero for success or -errno
> + */
> +int drm_atomic_helper_normalize_zpos(struct drm_device *dev,
> +				     struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
> +	int i, ret = 0;
> +
> +	for_each_plane_in_state(state, plane, plane_state, i) {
> +		crtc = plane_state->crtc;
> +		if (!crtc)
> +			continue;
> +		if (plane->state->zpos != plane_state->zpos) {
> +			crtc_state =
> +				drm_atomic_get_existing_crtc_state(state, 
crtc);
> +			crtc_state->zpos_changed = true;
> +		}
> +	}
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (crtc_state->plane_mask != crtc->state->plane_mask ||
> +		    crtc_state->zpos_changed) {
> +			ret = drm_atomic_helper_crtc_normalize_zpos(crtc,
> +								    
crtc_state);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_normalize_zpos);

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel