Re: [PATCH] drm/omap: Rework the rotation-on-crtc hack

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm/omap: Rework the rotation-on-crtc hack
Date: Mon, 31 Jul 2017 14:57:33 +0300	[thread overview]
Message-ID: <31291963.BFqn2rOcCK@avalon> (raw)
In-Reply-To: <20170731105419.6505-1-daniel.vetter@ffwll.ch>

Hi Daniel,

Thank you for the patch.

On Monday 31 Jul 2017 12:54:19 Daniel Vetter wrote:
> 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

s/propety/property/

> 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!
> 
> v3: Shadow the legacy state to avoid locking issues in get_property
> (Maarten).
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 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 | 84 ++++++++++++++++++----------------
>  drivers/gpu/drm/omapdrm/omap_drv.h  |  9 ++++
>  2 files changed, 57 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 14e8a7738b06..fc8b25748f10
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -498,39 +498,34 @@ 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 omap_crtc_state *omap_state = to_omap_crtc_state(state);
> +	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.

Nitpicking, you can reformat the comment to go towards the 80 columns limit.

> +	 */
> +	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;
> +		omap_state->rotation = val;
> +	} else if (property == priv->zorder_prop) {
> +		plane_state->zpos = val;
> +		omap_state->zpos = val;
> +	} else {
> +		return -EINVAL;
>  	}
> 
> -	return -EINVAL;
> +	return 0;
>  }
> 
>  static int omap_crtc_atomic_get_property(struct drm_crtc *crtc,
> @@ -538,20 +533,37 @@ 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;
> +	struct omap_crtc_state *omap_state = to_omap_crtc_state(state);
> +
> +	/*
> +	 * 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.

Isn't this comment outdated ?

> +	 */
> +	if (property == crtc->primary->rotation_property)
> +		return omap_state->rotation;
> +	else if (property == priv->zorder_prop)
> +		return omap_state->zpos;
> 
>  	return -EINVAL;
>  }
> 
> +static struct drm_crtc_state *
> +omap_crtc_duplicate_state(struct drm_crtc *crtc)
> +{
> +	struct omap_crtc_state *state;
> +
> +	if (WARN_ON(!crtc->state))
> +		return NULL;
> +
> +	state = kmalloc(sizeof(*state), GFP_KERNEL);
> +	if (state)
> +		__drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);

No need to copy the zpos and rotation fields ?

> +	return &state->base;
> +}
> +
>  static const struct drm_crtc_funcs omap_crtc_funcs = {
>  	.reset = drm_atomic_helper_crtc_reset,
>  	.set_config = drm_atomic_helper_set_config,
> @@ -559,7 +571,7 @@ static const struct drm_crtc_funcs omap_crtc_funcs = {
>  	.page_flip = drm_atomic_helper_page_flip,
>  	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>  	.set_property = drm_atomic_helper_crtc_set_property,
> -	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> +	.atomic_duplicate_state = omap_crtc_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>  	.atomic_set_property = omap_crtc_atomic_set_property,
>  	.atomic_get_property = omap_crtc_atomic_get_property,
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
> b/drivers/gpu/drm/omapdrm/omap_drv.h index 4bd1e9070b31..11703c174cd8
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -115,6 +115,15 @@ static inline void omap_fbdev_free(struct drm_device
> *dev) }
>  #endif
> 
> +struct omap_crtc_state {
> +	/* must be first */

Capitalizing comments would be appreciated :-)

> +	struct drm_crtc_state base;
> +	/* shadow values for legacy userspace support */
> +	unsigned int rotation;
> +	unsigned int zpos;
> +};
> +#define to_omap_crtc_state(x) container_of(x, struct omap_crtc_state, base)

Could you please define this structure in omap_crtc.c, as it's internal to the 
CRTC handling code ?

>  struct videomode *omap_crtc_timings(struct drm_crtc *crtc);
>  enum omap_channel omap_crtc_channel(struct drm_crtc *crtc);
>  void omap_crtc_pre_init(void);

-- 
Regards,

Laurent Pinchart

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