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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jan 09, 2018 at 01:45:56PM +0200, 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.
> 
> Planes with identical zpos value will result undefined behavior:
> disappearing planes, screen flickering and it is not supported by the
> hardware.
> 
> Enforce that all planes must have unique zpos on the given crtc by
> returning error when duplicate zpos value is requested.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
> Hi,
> 
> Changes since v4:
> - further simplify the zpos checking by using a mask and a single loop
> - Commit message has been extended
> 
> 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.

Sorry for jumping in late to the party, but except when you have a really,
really, really good reason for why omapdrm has to not normalize zpos like
the other drivers do in this case, then I think we should be consistent.

An inconsistent kms uapi is a lot worse than an uapi with some design
issues: The latter just means we eventually need v2, the former guarantees
we need v2.

Thanks, Daniel

> 
> Regards,
> Peter
> 
>  drivers/gpu/drm/omapdrm/omap_crtc.c  | 23 ++++++++++++++++++++++-
>  drivers/gpu/drm/omapdrm/omap_plane.c | 15 ++++-----------
>  2 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 1b8154e58d18..941a6440fc8e 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -486,6 +486,27 @@ 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_plane *plane;
> +	const struct drm_plane_state *pstate;
> +	unsigned int zpos_mask = 0;
> +
> +	/* Check the crts's planes against duplicated zpos value */
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, state) {
> +		if (zpos_mask & BIT(pstate->zpos)) {
> +			DBG("crtc%u: zpos must be unique (zpos: %u)",
> +			    crtc->index, pstate->zpos);
> +			return -EINVAL;
> +		}
> +
> +		zpos_mask |= BIT(pstate->zpos);
> +	}
> +
> +	return 0;
> +}
> +
>  static int omap_crtc_atomic_check(struct drm_crtc *crtc,
>  				struct drm_crtc_state *state)
>  {
> @@ -509,7 +530,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 7d789d1551a1..39f25210bef1 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -97,8 +97,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);
>  }
> @@ -184,18 +183,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,
> @@ -295,7 +288,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;
>  
> -- 
> Peter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel