Re: [PATCH v4] drm/omap: plane zpos/zorder management improvements

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

Hi Peter,

Thank you for the patch and sorry for the late review.

On Friday, 5 January 2018 13:30:37 EET Peter Ujfalusi wrote:
> Use the plane index as default zpos for all planes. Even if the
> application is not setting zpos/zorder explicitly we will have unique zpos
> for each plane.
> 
> Enforce that all planes must have unique zpos on the given crtc.

Could you explain the rationale for that in the commit message, what's wrong 
with duplicate zpos values ?

Isn't there a risk of breaking the non-atomic userspace with this ? Without 
atomic commits userspace can't change the zpos of multiple planes in one go, 
so it might be impossible to reorder planes without going through a state that 
has duplicated zpos values.

> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
> Hi,
> 
> Changes since v3:
> - Use drm_plane_index() instead of storing the same index wothin omap_plane
>   struct
> - Optimize the zpos validation loop so we avoid extra checks.
> 
> Changes since v2:
> - The check for duplicate zpos is moved to omap_crtc
> 
> Changes since v1:
> - Dropped the zpos normalization related patches
> - New patch based on the discussion, see commit message.
> 
> Regards,
> Peter
> 
>  drivers/gpu/drm/omapdrm/omap_crtc.c  | 36 ++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/omapdrm/omap_plane.c | 15 ++++-----------
>  2 files changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index cc85c16cbc2a..c68e3a4f5b7d
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -452,6 +452,40 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc
> *crtc) }
>  }
> 
> +static int omap_crtc_validate_zpos(struct drm_crtc *crtc,
> +				   struct drm_crtc_state *state)
> +{
> +	struct drm_device *ddev = crtc->dev;
> +	struct drm_plane *p1;
> +	const struct drm_plane_state *pstate1;
> +
> +	/* Check the crts's planes against duplicated zpos value */
> +	drm_atomic_crtc_state_for_each_plane_state(p1, pstate1, state) {
> +		struct drm_plane *p2 = p1;
> +
> +		list_for_each_entry_continue(p2, &ddev->mode_config.plane_list,
> +					     head) {
> +			const struct drm_plane_state *pstate2;
> +
> +			if (!(state->plane_mask & (1 << drm_plane_index(p2))))
> +				continue;
> +
> +			pstate2 = __drm_atomic_get_current_plane_state(
> +							state->state, p2);
> +			if (!pstate2)
> +				continue;
> +
> +			if (pstate1->zpos == pstate2->zpos) {
> +				DBG("crtc%u: zpos must be unique (zpos: %u)",
> +				crtc->index, pstate1->zpos);
> +				return -EINVAL;
> +			}
> +		}
> +	}

That seems a bit complicated to me. Couldn't we use a single loop and a zpos 
bitfield ? Something like the following (untested) code.

	struct drm_device *ddev = crtc->dev;
	const struct drm_plane_state *plane_state;
	struct drm_plane *plane;
	unsigned int zpos_mask = 0;

	/* Check the crts's planes against duplicated zpos value */
	drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, state) {
		if (zpos_mask & BIT(plane_state->zpos)) {
			DBG("crtc%u: zpos must be unique (zpos: %u)",
			    crtc->index, plane_state->zpos);
			return -EINVAL;
		}

		zpos_mask |= BIT(plane_state->zpos); 
	}

	return 0;

> +	return 0;
> +}
> +
>  static int omap_crtc_atomic_check(struct drm_crtc *crtc,
>  				struct drm_crtc_state *state)
>  {
> @@ -475,7 +509,7 @@ static int omap_crtc_atomic_check(struct drm_crtc *crtc,
> omap_crtc_state->rotation = pri_state->rotation;
>  	}
> 
> -	return 0;
> +	return omap_crtc_validate_zpos(crtc, state);
>  }
> 
>  static void omap_crtc_atomic_begin(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> b/drivers/gpu/drm/omapdrm/omap_plane.c index 15e5d5d325c6..d0057a23a5ac
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -99,8 +99,7 @@ static void omap_plane_atomic_disable(struct drm_plane
> *plane, struct omap_plane *omap_plane = to_omap_plane(plane);
> 
>  	plane->state->rotation = DRM_MODE_ROTATE_0;
> -	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
> -			   ? 0 : omap_plane->id;
> +	plane->state->zpos = drm_plane_index(plane);
> 
>  	priv->dispc_ops->ovl_enable(omap_plane->id, false);
>  }
> @@ -186,18 +185,12 @@ void omap_plane_install_properties(struct drm_plane
> *plane,
> 
>  static void omap_plane_reset(struct drm_plane *plane)
>  {
> -	struct omap_plane *omap_plane = to_omap_plane(plane);
> -
>  	drm_atomic_helper_plane_reset(plane);
>  	if (!plane->state)
>  		return;
> 
> -	/*
> -	 * Set the zpos default depending on whether we are a primary or overlay
> -	 * plane.
> -	 */
> -	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
> -			   ? 0 : omap_plane->id;
> +	/* Set the zpos to the plane index. */
> +	plane->state->zpos = drm_plane_index(plane);
>  }
> 
>  static int omap_plane_atomic_set_property(struct drm_plane *plane,
> @@ -297,7 +290,7 @@ struct drm_plane *omap_plane_init(struct drm_device
> *dev, drm_plane_helper_add(plane, &omap_plane_helper_funcs);
> 
>  	omap_plane_install_properties(plane, &plane->base);
> -	drm_plane_create_zpos_property(plane, 0, 0, num_planes - 1);
> +	drm_plane_create_zpos_property(plane, idx, 0, num_planes - 1);
> 
>  	return plane;

-- 
Regards,

Laurent Pinchart

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