diff mbox

[v2,4/5] drm: omapdrm: Store the Z order in the plane state zpos field

Message ID 20170415091632.7285-5-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Pinchart April 15, 2017, 9:16 a.m. UTC 
The DRM core implements a standard "zpos" property to control planes
ordering. The omapdrm driver implements a similar property named
"zorder". Although we can't switch to DRM core handling of the "zpos"
property for backward compatibility reasons, we can store the zorder
value in the drm_plane_state zpos field, saving us from having to
implement custom plane state handling.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_plane.c | 73 +++++-------------------------------
 1 file changed, 10 insertions(+), 63 deletions(-)

Comments

Tomi Valkeinen April 24, 2017, 9:40 a.m. UTC | #1 
On 15/04/17 12:16, Laurent Pinchart wrote:
> The DRM core implements a standard "zpos" property to control planes
> ordering. The omapdrm driver implements a similar property named
> "zorder". Although we can't switch to DRM core handling of the "zpos"
> property for backward compatibility reasons, we can store the zorder
> value in the drm_plane_state zpos field, saving us from having to
> implement custom plane state handling.

I'm fine with the zpos change, but I'd like to keep the omap_plane_state
around. It'll be empty after this patch, but we have a bunch of
properties we need to add. Some can be added as common DRM properties,
but perhaps not all. It's so much easier to handle those patches if the
plumbing is there already, instead of adding it back.

 Tomi
Laurent Pinchart April 24, 2017, 2 p.m. UTC | #2 
Hi Tomi,

On Monday 24 Apr 2017 12:40:10 Tomi Valkeinen wrote:
> On 15/04/17 12:16, Laurent Pinchart wrote:
> > The DRM core implements a standard "zpos" property to control planes
> > ordering. The omapdrm driver implements a similar property named
> > "zorder". Although we can't switch to DRM core handling of the "zpos"
> > property for backward compatibility reasons, we can store the zorder
> > value in the drm_plane_state zpos field, saving us from having to
> > implement custom plane state handling.
> 
> I'm fine with the zpos change, but I'd like to keep the omap_plane_state
> around. It'll be empty after this patch, but we have a bunch of
> properties we need to add. Some can be added as common DRM properties,
> but perhaps not all. It's so much easier to handle those patches if the
> plumbing is there already, instead of adding it back.

How about reverting the needed parts of this patch at that time then ? I think 
it would be better than keeping useless code around until a hypothetical time 
when it will be needed.
Tomi Valkeinen April 25, 2017, 9:01 a.m. UTC | #3 
On 24/04/17 17:00, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Monday 24 Apr 2017 12:40:10 Tomi Valkeinen wrote:
>> On 15/04/17 12:16, Laurent Pinchart wrote:
>>> The DRM core implements a standard "zpos" property to control planes
>>> ordering. The omapdrm driver implements a similar property named
>>> "zorder". Although we can't switch to DRM core handling of the "zpos"
>>> property for backward compatibility reasons, we can store the zorder
>>> value in the drm_plane_state zpos field, saving us from having to
>>> implement custom plane state handling.
>>
>> I'm fine with the zpos change, but I'd like to keep the omap_plane_state
>> around. It'll be empty after this patch, but we have a bunch of
>> properties we need to add. Some can be added as common DRM properties,
>> but perhaps not all. It's so much easier to handle those patches if the
>> plumbing is there already, instead of adding it back.
> 
> How about reverting the needed parts of this patch at that time then ? I think 
> it would be better than keeping useless code around until a hypothetical time 
> when it will be needed.

I kind of agree, but based on my experience, I disagree =).

We have a bunch of HW properties for both crtcs and planes which are not
supported in mainline. I have patches for those. It's difficult to
upstream them, because the aim should be to get common properties.
Creating common properties is Hard. So I need to carry those patches,
maybe for a long time, which means rebasing, which means gazillion
conflicts if the mainline version doesn't have the omap custom state for
crtc and plane.

In the minimum, the change for the zorder and the change that removes
the omap_plane_state should be separate. Then the removal patch can be
reverted, and the amount of conflicts drops to half a gazillion.

 Tomi
diff mbox

Patch

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 9168154d749e..521dd2ea519a 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -39,18 +39,6 @@  struct omap_plane {
 	uint32_t formats[32];
 };
 
-struct omap_plane_state {
-	struct drm_plane_state base;
-
-	unsigned int zorder;
-};
-
-static inline struct omap_plane_state *
-to_omap_plane_state(struct drm_plane_state *state)
-{
-	return container_of(state, struct omap_plane_state, base);
-}
-
 static int omap_plane_prepare_fb(struct drm_plane *plane,
 				 struct drm_plane_state *new_state)
 {
@@ -73,7 +61,6 @@  static void omap_plane_atomic_update(struct drm_plane *plane,
 	struct omap_drm_private *priv = plane->dev->dev_private;
 	struct omap_plane *omap_plane = to_omap_plane(plane);
 	struct drm_plane_state *state = plane->state;
-	struct omap_plane_state *omap_state = to_omap_plane_state(state);
 	struct omap_overlay_info info;
 	struct omap_drm_window win;
 	int ret;
@@ -85,7 +72,7 @@  static void omap_plane_atomic_update(struct drm_plane *plane,
 	info.rotation = OMAP_DSS_ROT_0;
 	info.global_alpha = 0xff;
 	info.mirror = 0;
-	info.zorder = omap_state->zorder;
+	info.zorder = state->zpos;
 
 	memset(&win, 0, sizeof(win));
 	win.rotation = state->rotation;
@@ -138,11 +125,10 @@  static void omap_plane_atomic_disable(struct drm_plane *plane,
 				      struct drm_plane_state *old_state)
 {
 	struct omap_drm_private *priv = plane->dev->dev_private;
-	struct omap_plane_state *omap_state = to_omap_plane_state(plane->state);
 	struct omap_plane *omap_plane = to_omap_plane(plane);
 
 	plane->state->rotation = DRM_ROTATE_0;
-	omap_state->zorder = plane->type == DRM_PLANE_TYPE_PRIMARY
+	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
 			   ? 0 : omap_plane->id;
 
 	priv->dispc_ops->ovl_enable(omap_plane->id, false);
@@ -227,56 +213,20 @@  void omap_plane_install_properties(struct drm_plane *plane,
 	drm_object_attach_property(obj, priv->zorder_prop, 0);
 }
 
-static struct drm_plane_state *
-omap_plane_atomic_duplicate_state(struct drm_plane *plane)
-{
-	struct omap_plane_state *state;
-	struct omap_plane_state *copy;
-
-	if (WARN_ON(!plane->state))
-		return NULL;
-
-	state = to_omap_plane_state(plane->state);
-	copy = kmemdup(state, sizeof(*state), GFP_KERNEL);
-	if (copy == NULL)
-		return NULL;
-
-	__drm_atomic_helper_plane_duplicate_state(plane, &copy->base);
-
-	return &copy->base;
-}
-
-static void omap_plane_atomic_destroy_state(struct drm_plane *plane,
-					    struct drm_plane_state *state)
-{
-	__drm_atomic_helper_plane_destroy_state(state);
-	kfree(to_omap_plane_state(state));
-}
-
 static void omap_plane_reset(struct drm_plane *plane)
 {
 	struct omap_plane *omap_plane = to_omap_plane(plane);
-	struct omap_plane_state *omap_state;
-
-	if (plane->state) {
-		omap_plane_atomic_destroy_state(plane, plane->state);
-		plane->state = NULL;
-	}
 
-	omap_state = kzalloc(sizeof(*omap_state), GFP_KERNEL);
-	if (omap_state == NULL)
+	drm_atomic_helper_plane_reset(plane);
+	if (!plane->state)
 		return;
 
 	/*
-	 * Set defaults depending on whether we are a primary or overlay
+	 * Set the zpos default depending on whether we are a primary or overlay
 	 * plane.
 	 */
-	omap_state->zorder = plane->type == DRM_PLANE_TYPE_PRIMARY
+	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
 			   ? 0 : omap_plane->id;
-	omap_state->base.rotation = DRM_ROTATE_0;
-
-	plane->state = &omap_state->base;
-	plane->state->plane = plane;
 }
 
 static int omap_plane_atomic_set_property(struct drm_plane *plane,
@@ -285,10 +235,9 @@  static int omap_plane_atomic_set_property(struct drm_plane *plane,
 					  uint64_t val)
 {
 	struct omap_drm_private *priv = plane->dev->dev_private;
-	struct omap_plane_state *omap_state = to_omap_plane_state(state);
 
 	if (property == priv->zorder_prop)
-		omap_state->zorder = val;
+		state->zpos = val;
 	else
 		return -EINVAL;
 
@@ -301,11 +250,9 @@  static int omap_plane_atomic_get_property(struct drm_plane *plane,
 					  uint64_t *val)
 {
 	struct omap_drm_private *priv = plane->dev->dev_private;
-	const struct omap_plane_state *omap_state =
-		container_of(state, const struct omap_plane_state, base);
 
 	if (property == priv->zorder_prop)
-		*val = omap_state->zorder;
+		*val = state->zpos;
 	else
 		return -EINVAL;
 
@@ -318,8 +265,8 @@  static const struct drm_plane_funcs omap_plane_funcs = {
 	.reset = omap_plane_reset,
 	.destroy = omap_plane_destroy,
 	.set_property = drm_atomic_helper_plane_set_property,
-	.atomic_duplicate_state = omap_plane_atomic_duplicate_state,
-	.atomic_destroy_state = omap_plane_atomic_destroy_state,
+	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
 	.atomic_set_property = omap_plane_atomic_set_property,
 	.atomic_get_property = omap_plane_atomic_get_property,
 };