Re: [PATCH 32/33] drm/omap: fix crtc->plane property delegation

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

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:48:07 Tomi Valkeinen wrote:
> Before universal planes we had to have plane specific properties for the
> crtc too, as on the hardware level a crtc uses a plane. In other words,
> e.g. 'zorder' property was added to both planes and crtcs, and
> omap_crtc.c would delegate the property set/get to the primary plane.
> 
> However, the delegation was a bit too generic, delegating all property
> set/get calls to planes. Thus it's possible to set, say, FB_ID, on a
> crtc, which gets redirected to  the primary plane.
> 
> This is not standard, and shouldn't be allowed. To keep backward
> compatibility, we still need to redirect the properties we supported
> earlier for crtcs, namely 'zorder' and 'rotation'.
> 
> This patch redirects only the allowed properties from crtcs to planes.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 58 +++++++++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 86b77c4f299a..280d80781635
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -419,24 +419,40 @@ static void omap_crtc_atomic_flush(struct drm_crtc
> *crtc, }
>  }
> 
> +static bool omap_crtc_is_plane_prop(struct drm_device *dev,
> +	struct drm_property *property)
> +{
> +	struct omap_drm_private *priv = dev->dev_private;
> +
> +	return	property == priv->zorder_prop ||

s/return\t/return / ?

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Good catch.

> +		property == dev->mode_config.rotation_property;
> +}
> +
>  static int omap_crtc_atomic_set_property(struct drm_crtc *crtc,
>  					 struct drm_crtc_state *state,
>  					 struct drm_property *property,
>  					 uint64_t val)
>  {
> -	struct drm_plane_state *plane_state;
> -	struct drm_plane *plane = crtc->primary;
> +	struct drm_device *dev = crtc->dev;
> 
> -	/*
> -	 * Delegate property set to the primary plane. Get the plane state and
> -	 * set the property directly.
> -	 */
> +	if (omap_crtc_is_plane_prop(dev, property)) {
> +		struct drm_plane_state *plane_state;
> +		struct drm_plane *plane = crtc->primary;
> +
> +		/*
> +		 * Delegate property set to the primary plane. Get the plane
> +		 * state and set the property directly.
> +		 */
> 
> -	plane_state = drm_atomic_get_plane_state(state->state, plane);
> -	if (IS_ERR(plane_state))
> -		return PTR_ERR(plane_state);
> +		plane_state = drm_atomic_get_plane_state(state->state, plane);
> +		if (IS_ERR(plane_state))
> +			return PTR_ERR(plane_state);
> 
> -	return drm_atomic_plane_set_property(plane, plane_state, property, val);
> +		return drm_atomic_plane_set_property(plane, plane_state,
> +				property, val);
> +	}
> +
> +	return -EINVAL;
>  }
> 
>  static int omap_crtc_atomic_get_property(struct drm_crtc *crtc,
> @@ -444,14 +460,20 @@ static int omap_crtc_atomic_get_property(struct
> drm_crtc *crtc, struct drm_property *property,
>  					 uint64_t *val)
>  {
> -	/*
> -	 * Delegate property get to the primary plane. The
> -	 * drm_atomic_plane_get_property() function isn't exported, but can be
> -	 * called through drm_object_property_get_value() as that will call
> -	 * drm_atomic_get_property() for atomic drivers.
> -	 */
> -	return drm_object_property_get_value(&crtc->primary->base, property,
> -					     val);
> +	struct drm_device *dev = crtc->dev;
> +
> +	if (omap_crtc_is_plane_prop(dev, property)) {
> +		/*
> +		 * Delegate property get to the primary plane. The
> +		 * drm_atomic_plane_get_property() function isn't exported, but
> +		 * can be called through drm_object_property_get_value() as that
> +		 * will call drm_atomic_get_property() for atomic drivers.
> +		 */
> +		return drm_object_property_get_value(&crtc->primary->base,
> +				property, val);
> +	}
> +
> +	return -EINVAL;
>  }
> 
>  static const struct drm_crtc_funcs omap_crtc_funcs = {

-- 
Regards,

Laurent Pinchart

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