drm/omap: Rework the rotation-on-crtc hack
diff mbox

Message ID 20170731154235.6402-1-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter July 31, 2017, 3:42 p.m. UTC
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!

v3: Shadow the legacy state to avoid locking issues in get_property
(Maarten).

v4: Review from Laurent
- Move struct omap_crtc_state into omap_crtc.c
- Clean up comments.
- Don't forget to copy the shadowed state over on duplicate.

v5: Don't forget to update the reset handler (Maarten).

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> (v4)
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 | 109 ++++++++++++++++++++++++------------
 1 file changed, 72 insertions(+), 37 deletions(-)

Comments

Maarten Lankhorst Aug. 1, 2017, 5:59 a.m. UTC | #1
Op 31-07-17 om 17:42 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!
>
> v3: Shadow the legacy state to avoid locking issues in get_property
> (Maarten).
>
> v4: Review from Laurent
> - Move struct omap_crtc_state into omap_crtc.c
> - Clean up comments.
> - Don't forget to copy the shadowed state over on duplicate.
>
> v5: Don't forget to update the reset handler (Maarten).
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> (v4)
> 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 | 109 ++++++++++++++++++++++++------------
>  1 file changed, 72 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 14e8a7738b06..9014085c33df 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -26,6 +26,16 @@
>  
>  #include "omap_drv.h"
>  
> +#define to_omap_crtc_state(x) container_of(x, struct omap_crtc_state, base)
> +
> +struct omap_crtc_state {
> +	/* Must be first. */
> +	struct drm_crtc_state base;
> +	/* Shadow values for legacy userspace support. */
> +	unsigned int rotation;
> +	unsigned int zpos;
> +};
> +
>  #define to_omap_crtc(x) container_of(x, struct omap_crtc, base)
>  
>  struct omap_crtc {
> @@ -498,39 +508,35 @@ 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, but keep a shadow copy for the
> +	 * atomic_get_property callback.
> +	 */
> +	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;
With atomic we should try to always make the getprop values accurate, or compositors might have
troubles restoring.

I would update the shadow values in omap_crtc_atomic_check through omap_crtc_atomic_check instead,
with something like this:

+       pri_state = drm_atomic_get_new_plane_state(crtc->primary, state->state);
+       if (pri_state) {
+               struct omap_crtc_state *omap_crtc_state =
+                       to_omap_crtc_state(state);
+
+               omap_crtc_state->zpos = pri_state->zpos;
+               omap_crtc_state->rotation = pri_state->rotation;
+       }

That way even when updating the property through the primary plane, it gets reflected correctly. For example when vt switching
with fbdev.

> +	} else {
> +		return -EINVAL;
>  	}
>  
> -	return -EINVAL;
> +	return 0;
>  }
>  
>  static int omap_crtc_atomic_get_property(struct drm_crtc *crtc,
> @@ -538,28 +544,57 @@ 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);
> +
> +	if (property == crtc->primary->rotation_property)
> +		return omap_state->rotation;
> +	else if (property == priv->zorder_prop)
> +		return omap_state->zpos;
>  
>  	return -EINVAL;
>  }
>  
> +static void omap_crtc_reset(struct drm_crtc *crtc)
> +{
> +	if (crtc->state)
> +		__drm_atomic_helper_crtc_destroy_state(crtc->state);
> +
> +	kfree(crtc->state);
> +	crtc->state = kzalloc(sizeof(struct omap_crtc_state), GFP_KERNEL);
> +
> +	if (crtc->state)
> +		crtc->state->crtc = crtc;
Hm, perhaps add a __drm_atomic_helper_crtc_reset, and (perhaps) for planes too?

Similar to __drm_atomic_helper_connector_reset, in case we want to add something in the future that might need to be initialised. :)
> +}
> +
> +static struct drm_crtc_state *
> +omap_crtc_duplicate_state(struct drm_crtc *crtc)
> +{
> +	struct omap_crtc_state *state, *current_state;
> +
> +	if (WARN_ON(!crtc->state))
> +		return NULL;
> +
> +	current_state = to_omap_crtc_state(crtc->state);
> +
> +	state = kmalloc(sizeof(*state), GFP_KERNEL);
> +	if (state)
> +		__drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);
> +
> +	state->zpos = current_state->zpos;
> +	state->rotation = current_state->rotation;
> +
> +	return &state->base;
> +}
> +
>  static const struct drm_crtc_funcs omap_crtc_funcs = {
> -	.reset = drm_atomic_helper_crtc_reset,
> +	.reset = omap_crtc_reset,
>  	.set_config = drm_atomic_helper_set_config,
>  	.destroy = omap_crtc_destroy,
>  	.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,
Laurent Pinchart Aug. 1, 2017, 10:20 a.m. UTC | #2
Hi Maarten,

On Tuesday 01 Aug 2017 07:59:13 Maarten Lankhorst wrote:
> Op 31-07-17 om 17:42 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!
> > 
> > v3: Shadow the legacy state to avoid locking issues in get_property
> > (Maarten).
> > 
> > v4: Review from Laurent
> > - Move struct omap_crtc_state into omap_crtc.c
> > - Clean up comments.
> > - Don't forget to copy the shadowed state over on duplicate.
> > 
> > v5: Don't forget to update the reset handler (Maarten).
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> (v4)
> > 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 | 109 ++++++++++++++++++++-----------
> >  1 file changed, 72 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 14e8a7738b06..9014085c33df
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c

[snip]

> >  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, but keep a shadow copy for the
> > +	 * atomic_get_property callback.
> > +	 */
> > +	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;
> 
> With atomic we should try to always make the getprop values accurate, or
> compositors might have troubles restoring.
> 
> I would update the shadow values in omap_crtc_atomic_check through
> omap_crtc_atomic_check instead, with something like this:
> 
> +       pri_state = drm_atomic_get_new_plane_state(crtc->primary,
> state->state);
> +       if (pri_state) {
> +               struct omap_crtc_state *omap_crtc_state =
> +                       to_omap_crtc_state(state);
> +
> +               omap_crtc_state->zpos = pri_state->zpos;
> +               omap_crtc_state->rotation = pri_state->rotation;
> +       }
> 
> That way even when updating the property through the primary plane, it gets
> reflected correctly. For example when vt switching with fbdev.

Let's not make it over-complicated. This hack is only needed fo the legacy X 
OMAP modesetting driver. The CRTC zpos and rotation properties should not be 
used through the atomic API.

> > +	} else {
> > +		return -EINVAL;
> >  	}
> > 
> > -	return -EINVAL;
> > +	return 0;
> >  }
Daniel Vetter Aug. 2, 2017, 8:02 a.m. UTC | #3
On Tue, Aug 1, 2017 at 12:20 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tuesday 01 Aug 2017 07:59:13 Maarten Lankhorst wrote:
>> +       pri_state = drm_atomic_get_new_plane_state(crtc->primary,
>> state->state);
>> +       if (pri_state) {
>> +               struct omap_crtc_state *omap_crtc_state =
>> +                       to_omap_crtc_state(state);
>> +
>> +               omap_crtc_state->zpos = pri_state->zpos;
>> +               omap_crtc_state->rotation = pri_state->rotation;
>> +       }
>>
>> That way even when updating the property through the primary plane, it gets
>> reflected correctly. For example when vt switching with fbdev.
>
> Let's not make it over-complicated. This hack is only needed fo the legacy X
> OMAP modesetting driver. The CRTC zpos and rotation properties should not be
> used through the atomic API.

Marten is right, the atomic properties are all such that you can
unconditionally restore them (e.g. fences report a no-op value), to
make compositor switching easier. Not sure anyone implements that, but
I think it's a useful idea to keep. I'll respin (or maybe Maarten
simply submits his patch with a proper sob ...).
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 14e8a7738b06..9014085c33df 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -26,6 +26,16 @@ 
 
 #include "omap_drv.h"
 
+#define to_omap_crtc_state(x) container_of(x, struct omap_crtc_state, base)
+
+struct omap_crtc_state {
+	/* Must be first. */
+	struct drm_crtc_state base;
+	/* Shadow values for legacy userspace support. */
+	unsigned int rotation;
+	unsigned int zpos;
+};
+
 #define to_omap_crtc(x) container_of(x, struct omap_crtc, base)
 
 struct omap_crtc {
@@ -498,39 +508,35 @@  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, but keep a shadow copy for the
+	 * atomic_get_property callback.
+	 */
+	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,28 +544,57 @@  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);
+
+	if (property == crtc->primary->rotation_property)
+		return omap_state->rotation;
+	else if (property == priv->zorder_prop)
+		return omap_state->zpos;
 
 	return -EINVAL;
 }
 
+static void omap_crtc_reset(struct drm_crtc *crtc)
+{
+	if (crtc->state)
+		__drm_atomic_helper_crtc_destroy_state(crtc->state);
+
+	kfree(crtc->state);
+	crtc->state = kzalloc(sizeof(struct omap_crtc_state), GFP_KERNEL);
+
+	if (crtc->state)
+		crtc->state->crtc = crtc;
+}
+
+static struct drm_crtc_state *
+omap_crtc_duplicate_state(struct drm_crtc *crtc)
+{
+	struct omap_crtc_state *state, *current_state;
+
+	if (WARN_ON(!crtc->state))
+		return NULL;
+
+	current_state = to_omap_crtc_state(crtc->state);
+
+	state = kmalloc(sizeof(*state), GFP_KERNEL);
+	if (state)
+		__drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);
+
+	state->zpos = current_state->zpos;
+	state->rotation = current_state->rotation;
+
+	return &state->base;
+}
+
 static const struct drm_crtc_funcs omap_crtc_funcs = {
-	.reset = drm_atomic_helper_crtc_reset,
+	.reset = omap_crtc_reset,
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = omap_crtc_destroy,
 	.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,