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

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: [PATCH v7] drm/omap: Rework the rotation-on-crtc hack
Date: Mon, 7 Aug 2017 12:20:06 +0200	[thread overview]
Message-ID: <a6a10a4f-2ebc-5f81-00bd-5e906967f384@linux.intel.com> (raw)
In-Reply-To: <CAKMK7uEO2ie4cKM8yn6aKPX_oUEwNeJLiBzLn+n=pxQ3RXxOSw@mail.gmail.com>

Op 04-08-17 om 12:02 schreef Daniel Vetter:
> On Fri, Aug 4, 2017 at 11:57 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> Here you go, compile tested version. :)
>>> ----8<----
>>> 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).
>>> v6: Update omap_crtc_state shadow values in omap_crtc_atomic_check (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>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/omapdrm/omap_crtc.c | 118 +++++++++++++++++++++++++-----------
>>>  1 file changed, 81 insertions(+), 37 deletions(-)
>> This makes all the CRTC properties disappear...
> Strange, we should still register them, that's really surprising.
>
>> This doesn't depend on anything in drm-next, does it? I'm currently on
>> v4.13-rc3. I'll look a bit more on what's going on later today.
> Not that I know of. Later patches will, but this one should be stand-alone.
> -Daniel

----->8--------
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).
v6: Update omap_crtc_state shadow values in omap_crtc_atomic_check (Maarten).
v7:
- Fix get_property to return 0 and set value in *val (Maarten).
- Update comment in set_property for changes in v6 (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>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 124 ++++++++++++++++++++++++------------
 1 file changed, 85 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 14e8a7738b06..09e05e002703 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 {
@@ -445,6 +455,8 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc *crtc)
 static int omap_crtc_atomic_check(struct drm_crtc *crtc,
 				struct drm_crtc_state *state)
 {
+	struct drm_plane_state *pri_state;
+
 	if (state->color_mgmt_changed && state->gamma_lut) {
 		uint length = state->gamma_lut->length /
 			sizeof(struct drm_color_lut);
@@ -453,6 +465,16 @@ static int omap_crtc_atomic_check(struct drm_crtc *crtc,
 			return -EINVAL;
 	}
 
+	pri_state = drm_atomic_get_new_plane_state(state->state, crtc->primary);
+	if (pri_state) {
+		struct omap_crtc_state *omap_crtc_state =
+			to_omap_crtc_state(state);
+
+		/* Mirror new values for zpos and rotation in omap_crtc_state */
+		omap_crtc_state->zpos = pri_state->zpos;
+		omap_crtc_state->rotation = pri_state->rotation;
+	}
+
 	return 0;
 }
 
@@ -498,39 +520,32 @@ 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, the shadow copy will be assigned in the
+	 * omap_crtc_atomic_check callback. This way updates to plane state will
+	 * always be mirrored in the crtc state correctly.
+	 */
+	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,28 +553,59 @@ 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)
+		*val = omap_state->rotation;
+	else if (property == priv->zorder_prop)
+		*val = omap_state->zpos;
+	else
+		return -EINVAL;
+
+	return 0;
+}
+
+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 -EINVAL;
+	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,
-- 
2.11.0


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