Re: [PATCH 1/8] drm/omap: Simplify the rotation-on-crtc hack

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 1/8] drm/omap: Simplify the rotation-on-crtc hack
Date: Tue, 25 Jul 2017 10:47:04 +0200	[thread overview]
Message-ID: <9a42cbd2-70e1-0654-2146-6cc4c8e058aa@linux.intel.com> (raw)
In-Reply-To: <20170725080122.20548-2-daniel.vetter@ffwll.ch>

Op 25-07-17 om 10:01 schreef Daniel Vetter:
> I want/need to rework the core property handling, and this hack is
> getting in the way. But since it's a non-standard propety only used by
> legacy userspace we know that this will only every be called from
> ioctl code. And never on some other free-standing state struct, where
> this old hack wouldn't work either.
>
> v2: don't forget zorder and get_property!
>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 64 ++++++++++++++++---------------------
>  1 file changed, 28 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 14e8a7738b06..efa525442e7d 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -498,39 +498,30 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
>  	spin_unlock_irq(&crtc->dev->event_lock);
>  }
>  
> -static bool omap_crtc_is_plane_prop(struct drm_crtc *crtc,
> -	struct drm_property *property)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct omap_drm_private *priv = dev->dev_private;
> -
> -	return property == priv->zorder_prop ||
> -		property == crtc->primary->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)
>  {
> -	if (omap_crtc_is_plane_prop(crtc, 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);
> +	struct omap_drm_private *priv = crtc->dev->dev_private;
> +	struct drm_plane_state *plane_state;
>  
> -		return drm_atomic_plane_set_property(plane, plane_state,
> -				property, val);
> -	}
> +	/*
> +	 * 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, crtc->primary);
> +	if (IS_ERR(plane_state))
> +		return PTR_ERR(plane_state);
> +
> +	if (property == crtc->primary->rotation_property)
> +		plane_state->rotation = val;
> +	else if (property == priv->zorder_prop)
> +		plane_state->zpos = val;
> +	else
> +		return -EINVAL;
>  
> -	return -EINVAL;
> +	return 0;
>  }
>  
>  static int omap_crtc_atomic_get_property(struct drm_crtc *crtc,
> @@ -538,16 +529,17 @@ static int omap_crtc_atomic_get_property(struct drm_crtc *crtc,
>  					 struct drm_property *property,
>  					 uint64_t *val)
>  {
> -	if (omap_crtc_is_plane_prop(crtc, 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);
> -	}
> +	struct omap_drm_private *priv = crtc->dev->dev_private;
> +
> +	/*
> +	 * Remap to the plane rotation/zorder property. We can peek at the plane
> +	 * state directly since holding the crtc locks gives you a read-lock on
> +	 * the plane state.
> +	 */
> +	if (property == crtc->primary->rotation_property)
> +		return crtc->primary->state->rotation;
> +	else if (property == priv->zorder_prop)
> +		return crtc->primary->state->zpos;
>  
>  	return -EINVAL;
>  }

acquire_ctx for crtc's getprop too then?

The comment about read lock is only valid when the plane is bound to the crtc. In general this is not always the case.
You can only peak at plane->state when crtc->state->plane_mask & BIT(drm_plane_index(plane)) is true.

I think we might need -EDEADLK handling for getprop then, even though it's not optimal. :(

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx