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@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 | 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
--
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
