[PATCH v3 0/3] drm/exynos: introduce generic zpos property

[PATCH v3 0/3] drm/exynos: introduce generic zpos property

[Marek Szyprowski]

Hello all,

This patch series is a continuation of rework of blending support in
Exynos DRM driver. Some background can be found here:
http://www.spinics.net/lists/dri-devel/msg96969.html

Daniel Vetter suggested that zpos property should be made generic, with
well-defined semantics. This patchset is my proposal for such generic
zpos property:
- added zpos properties to drm core and plane state structures,
- added helpers for normalizing zpos properties of given set of planes,
- well defined semantics: planes are sorted by zpos values and then plane
  id value if zpos equals.

Patches are based on top of latest exynos-drm-next branch.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland

Changelog:

v3:
- on request of Daniel Vetter, moved all normalization process to DRM
  core, drivers can simply use plane_state->normalized_zpos in their
  atomic_check/update callbacks with no additional changes needed
- updated documentation

v2: http://www.spinics.net/lists/dri-devel/msg98093.html
- dropped 2 fixes for Exynos DRM, which got merged in meantime
- added more comments and kernel docs for core functions as suggested
  by Daniel Vetter
- reworked initialization of zpos properties (moved assiging property
  class to common code), now the code in the driver is even simpler
- while reworking of intialization of zpos property code, did the same
  change to generic rotation property

v1: http://www.spinics.net/lists/dri-devel/msg97709.html
- initial version

Patch summary:

Marek Szyprowski (3):
  drm: add generic zpos property
  drm/exynos: use generic code for managing zpos plane property
  drm: simplify initialization of rotation property

 Documentation/DocBook/gpu.tmpl                  |  14 ++-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  10 +-
 drivers/gpu/drm/drm_atomic.c                    |   4 +
 drivers/gpu/drm/drm_atomic_helper.c             | 116 ++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc.c                      |  82 +++++++++++++++--
 drivers/gpu/drm/exynos/exynos_drm_drv.h         |   2 -
 drivers/gpu/drm/exynos/exynos_drm_plane.c       |  66 +++-----------
 drivers/gpu/drm/exynos/exynos_mixer.c           |   6 +-
 drivers/gpu/drm/i915/intel_display.c            |   6 +-
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c       |   3 +-
 drivers/gpu/drm/omapdrm/omap_drv.c              |   3 +-
 include/drm/drm_crtc.h                          |  18 +++-
 12 files changed, 250 insertions(+), 80 deletions(-)

-- 
1.9.2

[PATCH v3 1/3] drm: add generic zpos property

[Marek Szyprowski]

This patch adds support for generic plane's zpos property property with
well-defined semantics:
- added zpos properties to drm core and plane state structures
- added helpers for normalizing zpos properties of given set of planes
- well defined semantics: planes are sorted by zpos values and then plane
  id value if zpos equals

Normalized zpos values are calculated automatically when generic
muttable zpos property has been initialized. Drivers can simply use
plane_state->normalized_zpos in their atomic_check and/or plane_update
callbacks without any additional calls to DRM core.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 Documentation/DocBook/gpu.tmpl      |  14 ++++-
 drivers/gpu/drm/drm_atomic.c        |   4 ++
 drivers/gpu/drm/drm_atomic_helper.c | 116 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc.c          |  53 ++++++++++++++++
 include/drm/drm_crtc.h              |  14 +++++
 5 files changed, 199 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 6c6e81a9eaf4..f6b7236141b6 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -2004,7 +2004,7 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >Description/Restrictions</td>
 	</tr>
 	<tr>
-	<td rowspan="37" valign="top" >DRM</td>
+	<td rowspan="38" valign="top" >DRM</td>
 	<td valign="top" >Generic</td>
 	<td valign="top" >“rotation”</td>
 	<td valign="top" >BITMASK</td>
@@ -2256,7 +2256,7 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >property to suggest an Y offset for a connector</td>
 	</tr>
 	<tr>
-	<td rowspan="3" valign="top" >Optional</td>
+	<td rowspan="4" valign="top" >Optional</td>
 	<td valign="top" >“scaling mode”</td>
 	<td valign="top" >ENUM</td>
 	<td valign="top" >{ "None", "Full", "Center", "Full aspect" }</td>
@@ -2280,6 +2280,16 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >TBD</td>
 	</tr>
 	<tr>
+	<td valign="top" > "zpos" </td>
+	<td valign="top" >RANGE</td>
+	<td valign="top" >Min=0, Max=255</td>
+	<td valign="top" >Plane</td>
+	<td valign="top" >Plane's 'z' position during blending (0 for background, 255 for frontmost).
+		If two planes assigned to same CRTC have equal zpos values, the plane with higher plane
+		id is treated as closer to front. Can be IMMUTABLE if driver doesn't support changing
+		planes' order.</td>
+	</tr>
+	<tr>
 	<td rowspan="20" valign="top" >i915</td>
 	<td rowspan="2" valign="top" >Generic</td>
 	<td valign="top" >"Broadcast RGB"</td>
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 6a21e5c378c1..97bb069cb6a3 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -614,6 +614,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		state->src_h = val;
 	} else if (property == config->rotation_property) {
 		state->rotation = val;
+	} else if (property == config->zpos_property) {
+		state->zpos = val;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);
@@ -670,6 +672,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 		*val = state->src_h;
 	} else if (property == config->rotation_property) {
 		*val = state->rotation;
+	} else if (property == config->zpos_property) {
+		*val = state->zpos;
 	} else if (plane->funcs->atomic_get_property) {
 		return plane->funcs->atomic_get_property(plane, state, property, val);
 	} else {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d0d4b2ff7c21..257946fac94b 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -31,6 +31,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic_helper.h>
 #include <linux/fence.h>
+#include <linux/sort.h>
 
 /**
  * DOC: overview
@@ -507,6 +508,117 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
 
+static int drm_atomic_state_zpos_cmp(const void *a, const void *b)
+{
+	const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
+	const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
+	int zpos_a = (sa->zpos << 16) + sa->plane->base.id;
+	int zpos_b = (sb->zpos << 16) + sb->plane->base.id;
+
+	return zpos_a - zpos_b;
+}
+
+/**
+ * drm_atomic_helper_crtc_normalize_zpos - calculate normalized zpos values
+ * @crtc: crtc with planes, which have to be considered for normalization
+ * @crtc_state: new atomic state to apply
+ *
+ * This function checks new states of all planes assigned to given crtc and
+ * calculates normalized zpos value for them. Planes are compared first by their
+ * zpos values, then by plane id (if zpos equals). Plane with lowest zpos value
+ * is at the bottom. The plane_state->normalized_zpos is then filled with uniqe
+ * values from 0 to number of active planes in crtc minus one.
+ *
+ * RETURNS
+ * Zero for success or -errno
+ */
+int drm_atomic_helper_crtc_normalize_zpos(struct drm_crtc *crtc,
+					      struct drm_crtc_state *crtc_state)
+{
+	struct drm_atomic_state *state = crtc_state->state;
+	struct drm_device *dev = crtc->dev;
+	int total_planes = dev->mode_config.num_total_plane;
+	struct drm_plane_state **states;
+	struct drm_plane *plane;
+	int i, n = 0;
+	int ret = 0;
+
+	DRM_DEBUG_ATOMIC("[CRTC:%d:%s] calculating normalized zpos values\n",
+			 crtc->base.id, crtc->name);
+
+	states = kmalloc(total_planes * sizeof(*states), GFP_KERNEL);
+	if (!states)
+		return -ENOMEM;
+
+	/*
+	 * Normalization process might create new states for planes which
+	 * normalized_zpos has to be recalculated.
+	 */
+	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
+		struct drm_plane_state *plane_state =
+			drm_atomic_get_plane_state(state, plane);
+		if (IS_ERR(plane_state)) {
+			ret = PTR_ERR(plane_state);
+			goto fail;
+		}
+		states[n++] = plane_state;
+		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] processing zpos value %d\n",
+				 plane->base.id, plane->name, plane_state->zpos);
+	}
+
+	sort(states, n, sizeof(*states), drm_atomic_state_zpos_cmp, NULL);
+
+	for (i = 0; i < n; i++) {
+		plane = states[i]->plane;
+		states[i]->normalized_zpos = i;
+		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] normalized zpos value %d\n",
+				 plane->base.id, plane->name, i);
+	}
+fail:
+	kfree(states);
+	return ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_crtc_normalize_zpos);
+
+static int drm_atomic_helper_normalize_zpos(struct drm_device *dev,
+					    struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	struct drm_plane *plane;
+	struct drm_plane_state *plane_state;
+	int i, ret = 0;
+
+	/*
+	 * If zpos_property is not initialized, then it makes no sense
+	 * to calculate normalized zpos values.
+	 */
+	if (!dev->mode_config.zpos_property)
+		return 0;
+
+	for_each_plane_in_state(state, plane, plane_state, i) {
+		crtc = plane_state->crtc;
+		if (!crtc)
+			continue;
+		if (plane->state->zpos != plane_state->zpos) {
+			crtc_state =
+				drm_atomic_get_existing_crtc_state(state, crtc);
+			crtc_state->zpos_changed = true;
+		}
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (crtc_state->plane_mask != crtc->state->plane_mask ||
+		    crtc_state->zpos_changed) {
+			ret = drm_atomic_helper_crtc_normalize_zpos(crtc,
+								    crtc_state);
+			if (ret)
+				return ret;
+		}
+	}
+	return 0;
+}
+
 /**
  * drm_atomic_helper_check_planes - validate state object for planes changes
  * @dev: DRM device
@@ -532,6 +644,10 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
 	struct drm_plane_state *plane_state;
 	int i, ret = 0;
 
+	ret = drm_atomic_helper_normalize_zpos(dev, state);
+	if (ret)
+		return ret;
+
 	for_each_plane_in_state(state, plane, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 62fa95fa5471..54a21e7c1ca5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5880,6 +5880,59 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
 EXPORT_SYMBOL(drm_mode_create_rotation_property);
 
 /**
+ * drm_mode_create_zpos_property - create muttable zpos property
+ * @dev: DRM device
+ *
+ * This function initializes generic muttable zpos property and enables support
+ * for it in drm core. Drivers can then attach this property to planes to enable
+ * support for configurable planes arrangement during blending operation.
+ * Drivers can also use drm_atomic_helper_normalize_zpos() function to calculate
+ * drm_plane_state->normalized_zpos values.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_create_zpos_property(struct drm_device *dev)
+{
+	struct drm_property *prop;
+
+	prop = drm_property_create_range(dev, 0, "zpos", 0, 255);
+	if (!prop)
+		return -ENOMEM;
+
+	dev->mode_config.zpos_property = prop;
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_zpos_property);
+
+/**
+ * drm_plane_create_zpos_property - create immuttable zpos property
+ * @dev: DRM device
+ *
+ * This function initializes generic immuttable zpos property and enables
+ * support for it in drm core. Using this property driver lets userspace
+ * to get the arrangement of the planes for blending operation and notifies
+ * it that the hardware (or driver) doesn't support changing of the planes'
+ * order.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_create_zpos_immutable_property(struct drm_device *dev)
+{
+	struct drm_property *prop;
+
+	prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, "zpos",
+					 0, 255);
+	if (!prop)
+		return -ENOMEM;
+
+	dev->mode_config.zpos_immutable_property = prop;
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_zpos_immutable_property);
+
+/**
  * DOC: Tile group
  *
  * Tile groups are used to represent tiled monitors with a unique
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3b040b355472..0607ad2ce918 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -305,6 +305,7 @@ struct drm_plane_helper_funcs;
  * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
  * @active_changed: crtc_state->active has been toggled.
  * @connectors_changed: connectors to this crtc have been updated
+ * @zpos_changed: zpos values of planes on this crtc have been updated
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
  * @last_vblank_count: for helpers and drivers to capture the vblank of the
  * 	update to ensure framebuffer cleanup isn't done too early
@@ -331,6 +332,7 @@ struct drm_crtc_state {
 	bool mode_changed : 1;
 	bool active_changed : 1;
 	bool connectors_changed : 1;
+	bool zpos_changed : 1;
 
 	/* attached planes bitmask:
 	 * WARNING: transitional helpers do not maintain plane_mask so
@@ -1243,6 +1245,9 @@ struct drm_connector {
  *	plane (in 16.16)
  * @src_w: width of visible portion of plane (in 16.16)
  * @src_h: height of visible portion of plane (in 16.16)
+ * @zpos: priority of the given plane on crtc (optional)
+ * @normalized_zpos: normalized value of zpos: uniqe, range from 0 to
+ *	(number of planes - 1) for given crtc
  * @state: backpointer to global drm_atomic_state
  */
 struct drm_plane_state {
@@ -1263,6 +1268,10 @@ struct drm_plane_state {
 	/* Plane rotation */
 	unsigned int rotation;
 
+	/* Plane zpos */
+	unsigned int zpos;
+	unsigned int normalized_zpos;
+
 	struct drm_atomic_state *state;
 };
 
@@ -2083,6 +2092,8 @@ struct drm_mode_config {
 	struct drm_property *tile_property;
 	struct drm_property *plane_type_property;
 	struct drm_property *rotation_property;
+	struct drm_property *zpos_property;
+	struct drm_property *zpos_immutable_property;
 	struct drm_property *prop_src_x;
 	struct drm_property *prop_src_y;
 	struct drm_property *prop_src_w;
@@ -2484,6 +2495,9 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
 extern unsigned int drm_rotation_simplify(unsigned int rotation,
 					  unsigned int supported_rotations);
 
+extern int drm_mode_create_zpos_property(struct drm_device *dev);
+extern int drm_mode_create_zpos_immutable_property(struct drm_device *dev);
+
 /* Helpers */
 
 static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
-- 
1.9.2

[PATCH v3 2/3] drm/exynos: use generic code for managing zpos plane property

[Marek Szyprowski]

This patch replaces zpos property handling custom code in Exynos DRM
driver with calls to generic DRM code.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.h   |  2 -
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 66 +++++++------------------------
 drivers/gpu/drm/exynos/exynos_mixer.c     |  6 ++-
 3 files changed, 18 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 17b5ded72ff1..816537886e4e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -64,7 +64,6 @@ struct exynos_drm_plane_state {
 	struct exynos_drm_rect src;
 	unsigned int h_ratio;
 	unsigned int v_ratio;
-	unsigned int zpos;
 };
 
 static inline struct exynos_drm_plane_state *
@@ -217,7 +216,6 @@ struct exynos_drm_private {
 	 * this array is used to be aware of which crtc did it request vblank.
 	 */
 	struct drm_crtc *crtc[MAX_CRTC];
-	struct drm_property *plane_zpos_property;
 
 	unsigned long da_start;
 	unsigned long da_space_size;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index d86227236f55..a434d3a6bb90 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -137,9 +137,9 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
 
 	exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL);
 	if (exynos_state) {
-		exynos_state->zpos = exynos_plane->config->zpos;
 		plane->state = &exynos_state->base;
 		plane->state->plane = plane;
+		plane->state->zpos = exynos_plane->config->zpos;
 	}
 }
 
@@ -155,7 +155,6 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane)
 		return NULL;
 
 	__drm_atomic_helper_plane_duplicate_state(plane, &copy->base);
-	copy->zpos = exynos_state->zpos;
 	return &copy->base;
 }
 
@@ -168,43 +167,6 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane,
 	kfree(old_exynos_state);
 }
 
-static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane,
-						struct drm_plane_state *state,
-						struct drm_property *property,
-						uint64_t val)
-{
-	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
-	struct exynos_drm_plane_state *exynos_state =
-					to_exynos_plane_state(state);
-	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
-	const struct exynos_drm_plane_config *config = exynos_plane->config;
-
-	if (property == dev_priv->plane_zpos_property &&
-	    (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS))
-		exynos_state->zpos = val;
-	else
-		return -EINVAL;
-
-	return 0;
-}
-
-static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane,
-					  const struct drm_plane_state *state,
-					  struct drm_property *property,
-					  uint64_t *val)
-{
-	const struct exynos_drm_plane_state *exynos_state =
-		container_of(state, const struct exynos_drm_plane_state, base);
-	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
-
-	if (property == dev_priv->plane_zpos_property)
-		*val = exynos_state->zpos;
-	else
-		return -EINVAL;
-
-	return 0;
-}
-
 static struct drm_plane_funcs exynos_plane_funcs = {
 	.update_plane	= drm_atomic_helper_update_plane,
 	.disable_plane	= drm_atomic_helper_disable_plane,
@@ -213,8 +175,6 @@ static struct drm_plane_funcs exynos_plane_funcs = {
 	.reset		= exynos_drm_plane_reset,
 	.atomic_duplicate_state = exynos_drm_plane_duplicate_state,
 	.atomic_destroy_state = exynos_drm_plane_destroy_state,
-	.atomic_set_property = exynos_drm_plane_atomic_set_property,
-	.atomic_get_property = exynos_drm_plane_atomic_get_property,
 };
 
 static int
@@ -302,20 +262,21 @@ static const struct drm_plane_helper_funcs plane_helper_funcs = {
 };
 
 static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
-					      unsigned int zpos)
+					      unsigned int zpos, bool immutable)
 {
 	struct drm_device *dev = plane->dev;
-	struct exynos_drm_private *dev_priv = dev->dev_private;
 	struct drm_property *prop;
 
-	prop = dev_priv->plane_zpos_property;
-	if (!prop) {
-		prop = drm_property_create_range(dev, 0, "zpos",
-						 0, MAX_PLANE - 1);
-		if (!prop)
-			return;
-
-		dev_priv->plane_zpos_property = prop;
+	if (immutable) {
+		if (!dev->mode_config.zpos_immutable_property)
+			if (drm_mode_create_zpos_immutable_property(dev))
+				return;
+		prop = dev->mode_config.zpos_immutable_property;
+	} else {
+		if (!dev->mode_config.zpos_property)
+			if (drm_mode_create_zpos_property(dev))
+				return;
+		prop = dev->mode_config.zpos_property;
 	}
 
 	drm_object_attach_property(&plane->base, prop, zpos);
@@ -344,7 +305,8 @@ int exynos_plane_init(struct drm_device *dev,
 	exynos_plane->index = index;
 	exynos_plane->config = config;
 
-	exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos);
+	exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos,
+			   !(config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS));
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index b5fbc1cbf024..d00c201773c9 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -478,6 +478,7 @@ static void vp_video_buffer(struct mixer_context *ctx,
 	struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
 	struct mixer_resources *res = &ctx->mixer_res;
 	struct drm_framebuffer *fb = state->base.fb;
+	unsigned int priority = state->base.normalized_zpos + 1;
 	unsigned long flags;
 	dma_addr_t luma_addr[2], chroma_addr[2];
 	bool tiled_mode = false;
@@ -562,7 +563,7 @@ static void vp_video_buffer(struct mixer_context *ctx,
 
 	mixer_cfg_scan(ctx, mode->vdisplay);
 	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
-	mixer_cfg_layer(ctx, plane->index, state->zpos + 1, true);
+	mixer_cfg_layer(ctx, plane->index, priority, true);
 	mixer_cfg_vp_blend(ctx);
 	mixer_run(ctx);
 
@@ -587,6 +588,7 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 	struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
 	struct mixer_resources *res = &ctx->mixer_res;
 	struct drm_framebuffer *fb = state->base.fb;
+	unsigned int priority = state->base.normalized_zpos + 1;
 	unsigned long flags;
 	unsigned int win = plane->index;
 	unsigned int x_ratio = 0, y_ratio = 0;
@@ -678,7 +680,7 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 
 	mixer_cfg_scan(ctx, mode->vdisplay);
 	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
-	mixer_cfg_layer(ctx, win, state->zpos + 1, true);
+	mixer_cfg_layer(ctx, win, priority, true);
 	mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
 
 	/* layer update mandatory for mixer 16.0.33.0 */
-- 
1.9.2

[PATCH v3 3/3] drm: simplify initialization of rotation property

[Marek Szyprowski]

This patch simplifies initialization of generic rotation property and
aligns the code to match recently introduced function for intializing
generic zpos property. It also adds missing documentation.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 10 ++++-----
 drivers/gpu/drm/drm_crtc.c                      | 29 ++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c            |  6 ++---
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c       |  3 +--
 drivers/gpu/drm/omapdrm/omap_drv.c              |  3 +--
 include/drm/drm_crtc.h                          |  4 ++--
 6 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index 1ffe9c329c46..4f9606cdf0f2 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -967,12 +967,10 @@ atmel_hlcdc_plane_create_properties(struct drm_device *dev)
 	if (!props->alpha)
 		return ERR_PTR(-ENOMEM);
 
-	dev->mode_config.rotation_property =
-			drm_mode_create_rotation_property(dev,
-							  BIT(DRM_ROTATE_0) |
-							  BIT(DRM_ROTATE_90) |
-							  BIT(DRM_ROTATE_180) |
-							  BIT(DRM_ROTATE_270));
+	drm_mode_create_rotation_property(dev, BIT(DRM_ROTATE_0) |
+					       BIT(DRM_ROTATE_90) |
+					       BIT(DRM_ROTATE_180) |
+					       BIT(DRM_ROTATE_270));
 	if (!dev->mode_config.rotation_property)
 		return ERR_PTR(-ENOMEM);
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 54a21e7c1ca5..99faccb63ce3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5861,10 +5861,23 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_mode_config_cleanup);
 
-struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
-						       unsigned int supported_rotations)
+/**
+ * drm_mode_create_rotation_property - create generic rotation property
+ * @dev: DRM device
+ * @supported_rotations: bitmask of supported rotation modes
+ *
+ * This function initializes generic rotation property and enables support
+ * for it in drm core. Drivers can then attach this property to planes to enable
+ * support for different rotation modes.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_create_rotation_property(struct drm_device *dev,
+				      unsigned int supported_rotations)
 {
-	static const struct drm_prop_enum_list props[] = {
+	struct drm_property *prop;
+	static const struct drm_prop_enum_list values[] = {
 		{ DRM_ROTATE_0,   "rotate-0" },
 		{ DRM_ROTATE_90,  "rotate-90" },
 		{ DRM_ROTATE_180, "rotate-180" },
@@ -5873,9 +5886,13 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
 		{ DRM_REFLECT_Y,  "reflect-y" },
 	};
 
-	return drm_property_create_bitmask(dev, 0, "rotation",
-					   props, ARRAY_SIZE(props),
-					   supported_rotations);
+	prop = drm_property_create_bitmask(dev, 0, "rotation", values,
+				ARRAY_SIZE(values), supported_rotations);
+	if (!prop)
+		return -ENOMEM;
+
+	dev->mode_config.rotation_property = prop;
+	return 0;
 }
 EXPORT_SYMBOL(drm_mode_create_rotation_property);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 02f6ccb848a9..5b7ba46491a0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14042,8 +14042,7 @@ void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *
 		if (INTEL_INFO(dev)->gen >= 9)
 			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
 
-		dev->mode_config.rotation_property =
-			drm_mode_create_rotation_property(dev, flags);
+		drm_mode_create_rotation_property(dev, flags);
 	}
 	if (dev->mode_config.rotation_property)
 		drm_object_attach_property(&plane->base.base,
@@ -14179,8 +14178,7 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 
 	if (INTEL_INFO(dev)->gen >= 4) {
 		if (!dev->mode_config.rotation_property)
-			dev->mode_config.rotation_property =
-				drm_mode_create_rotation_property(dev,
+			drm_mode_create_rotation_property(dev,
 							BIT(DRM_ROTATE_0) |
 							BIT(DRM_ROTATE_180));
 		if (dev->mode_config.rotation_property)
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
index 432c09836b0e..8defeec0d453 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
@@ -76,8 +76,7 @@ static void mdp5_plane_install_rotation_property(struct drm_device *dev,
 		return;
 
 	if (!dev->mode_config.rotation_property)
-		dev->mode_config.rotation_property =
-			drm_mode_create_rotation_property(dev,
+		drm_mode_create_rotation_property(dev,
 			BIT(DRM_REFLECT_X) | BIT(DRM_REFLECT_Y));
 
 	if (dev->mode_config.rotation_property)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index dfafdb602ad2..c6ce2b31f1c5 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -304,8 +304,7 @@ static int omap_modeset_init_properties(struct drm_device *dev)
 	struct omap_drm_private *priv = dev->dev_private;
 
 	if (priv->has_dmm) {
-		dev->mode_config.rotation_property =
-			drm_mode_create_rotation_property(dev,
+		drm_mode_create_rotation_property(dev,
 				BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_90) |
 				BIT(DRM_ROTATE_180) | BIT(DRM_ROTATE_270) |
 				BIT(DRM_REFLECT_X) | BIT(DRM_REFLECT_Y));
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 0607ad2ce918..9cb6a57af51b 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2490,8 +2490,8 @@ extern int drm_format_plane_cpp(uint32_t format, int plane);
 extern int drm_format_horz_chroma_subsampling(uint32_t format);
 extern int drm_format_vert_chroma_subsampling(uint32_t format);
 extern const char *drm_get_format_name(uint32_t format);
-extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
-							      unsigned int supported_rotations);
+extern int drm_mode_create_rotation_property(struct drm_device *dev,
+					     unsigned int supported_rotations);
 extern unsigned int drm_rotation_simplify(unsigned int rotation,
 					  unsigned int supported_rotations);
 
-- 
1.9.2

Re: [PATCH v3 0/3] drm/exynos: introduce generic zpos property

[Daniel Vetter]

On Tue, Jan 12, 2016 at 02:39:17PM +0100, Marek Szyprowski wrote:
> Hello all,
> 
> This patch series is a continuation of rework of blending support in
> Exynos DRM driver. Some background can be found here:
> http://www.spinics.net/lists/dri-devel/msg96969.html
> 
> Daniel Vetter suggested that zpos property should be made generic, with
> well-defined semantics. This patchset is my proposal for such generic
> zpos property:
> - added zpos properties to drm core and plane state structures,
> - added helpers for normalizing zpos properties of given set of planes,
> - well defined semantics: planes are sorted by zpos values and then plane
>   id value if zpos equals.
> 
> Patches are based on top of latest exynos-drm-next branch.
> 
> Best regards
> Marek Szyprowski
> Samsung R&D Institute Poland
> 
> Changelog:
> 
> v3:
> - on request of Daniel Vetter, moved all normalization process to DRM
>   core, drivers can simply use plane_state->normalized_zpos in their
>   atomic_check/update callbacks with no additional changes needed
> - updated documentation
> 
> v2: http://www.spinics.net/lists/dri-devel/msg98093.html
> - dropped 2 fixes for Exynos DRM, which got merged in meantime
> - added more comments and kernel docs for core functions as suggested
>   by Daniel Vetter
> - reworked initialization of zpos properties (moved assiging property
>   class to common code), now the code in the driver is even simpler
> - while reworking of intialization of zpos property code, did the same
>   change to generic rotation property
> 
> v1: http://www.spinics.net/lists/dri-devel/msg97709.html
> - initial version

Yeah I think that looks overall rather neat now. Probably best if someone
from the exynos team reviews it all in detail, and then we could pull it
in through drm-misc.

Cheers, Daniel

> 
> Patch summary:
> 
> Marek Szyprowski (3):
>   drm: add generic zpos property
>   drm/exynos: use generic code for managing zpos plane property
>   drm: simplify initialization of rotation property
> 
>  Documentation/DocBook/gpu.tmpl                  |  14 ++-
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  10 +-
>  drivers/gpu/drm/drm_atomic.c                    |   4 +
>  drivers/gpu/drm/drm_atomic_helper.c             | 116 ++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c                      |  82 +++++++++++++++--
>  drivers/gpu/drm/exynos/exynos_drm_drv.h         |   2 -
>  drivers/gpu/drm/exynos/exynos_drm_plane.c       |  66 +++-----------
>  drivers/gpu/drm/exynos/exynos_mixer.c           |   6 +-
>  drivers/gpu/drm/i915/intel_display.c            |   6 +-
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c       |   3 +-
>  drivers/gpu/drm/omapdrm/omap_drv.c              |   3 +-
>  include/drm/drm_crtc.h                          |  18 +++-
>  12 files changed, 250 insertions(+), 80 deletions(-)
> 
> -- 
> 1.9.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 1/3] drm: add generic zpos property

[Benjamin Gaignard]

[-- Attachment #1.1: Type: text/plain, Size: 15601 bytes --]

I have been able to test it on sti driver.

Thanks to have done the effort to make zpos generic.

You can add my tested-by on this patch.

Benjamin

2016-01-12 14:39 GMT+01:00 Marek Szyprowski <m.szyprowski@samsung.com>:

> This patch adds support for generic plane's zpos property property with
> well-defined semantics:
> - added zpos properties to drm core and plane state structures
> - added helpers for normalizing zpos properties of given set of planes
> - well defined semantics: planes are sorted by zpos values and then plane
>   id value if zpos equals
>
> Normalized zpos values are calculated automatically when generic
> muttable zpos property has been initialized. Drivers can simply use
> plane_state->normalized_zpos in their atomic_check and/or plane_update
> callbacks without any additional calls to DRM core.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  Documentation/DocBook/gpu.tmpl      |  14 ++++-
>  drivers/gpu/drm/drm_atomic.c        |   4 ++
>  drivers/gpu/drm/drm_atomic_helper.c | 116
> ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c          |  53 ++++++++++++++++
>  include/drm/drm_crtc.h              |  14 +++++
>  5 files changed, 199 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/DocBook/gpu.tmpl
> b/Documentation/DocBook/gpu.tmpl
> index 6c6e81a9eaf4..f6b7236141b6 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -2004,7 +2004,7 @@ void intel_crt_init(struct drm_device *dev)
>         <td valign="top" >Description/Restrictions</td>
>         </tr>
>         <tr>
> -       <td rowspan="37" valign="top" >DRM</td>
> +       <td rowspan="38" valign="top" >DRM</td>
>         <td valign="top" >Generic</td>
>         <td valign="top" >“rotation”</td>
>         <td valign="top" >BITMASK</td>
> @@ -2256,7 +2256,7 @@ void intel_crt_init(struct drm_device *dev)
>         <td valign="top" >property to suggest an Y offset for a
> connector</td>
>         </tr>
>         <tr>
> -       <td rowspan="3" valign="top" >Optional</td>
> +       <td rowspan="4" valign="top" >Optional</td>
>         <td valign="top" >“scaling mode”</td>
>         <td valign="top" >ENUM</td>
>         <td valign="top" >{ "None", "Full", "Center", "Full aspect" }</td>
> @@ -2280,6 +2280,16 @@ void intel_crt_init(struct drm_device *dev)
>         <td valign="top" >TBD</td>
>         </tr>
>         <tr>
> +       <td valign="top" > "zpos" </td>
> +       <td valign="top" >RANGE</td>
> +       <td valign="top" >Min=0, Max=255</td>
> +       <td valign="top" >Plane</td>
> +       <td valign="top" >Plane's 'z' position during blending (0 for
> background, 255 for frontmost).
> +               If two planes assigned to same CRTC have equal zpos
> values, the plane with higher plane
> +               id is treated as closer to front. Can be IMMUTABLE if
> driver doesn't support changing
> +               planes' order.</td>
> +       </tr>
> +       <tr>
>         <td rowspan="20" valign="top" >i915</td>
>         <td rowspan="2" valign="top" >Generic</td>
>         <td valign="top" >"Broadcast RGB"</td>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 6a21e5c378c1..97bb069cb6a3 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -614,6 +614,8 @@ int drm_atomic_plane_set_property(struct drm_plane
> *plane,
>                 state->src_h = val;
>         } else if (property == config->rotation_property) {
>                 state->rotation = val;
> +       } else if (property == config->zpos_property) {
> +               state->zpos = val;
>         } else if (plane->funcs->atomic_set_property) {
>                 return plane->funcs->atomic_set_property(plane, state,
>                                 property, val);
> @@ -670,6 +672,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>                 *val = state->src_h;
>         } else if (property == config->rotation_property) {
>                 *val = state->rotation;
> +       } else if (property == config->zpos_property) {
> +               *val = state->zpos;
>         } else if (plane->funcs->atomic_get_property) {
>                 return plane->funcs->atomic_get_property(plane, state,
> property, val);
>         } else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index d0d4b2ff7c21..257946fac94b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <linux/fence.h>
> +#include <linux/sort.h>
>
>  /**
>   * DOC: overview
> @@ -507,6 +508,117 @@ drm_atomic_helper_check_modeset(struct drm_device
> *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
>
> +static int drm_atomic_state_zpos_cmp(const void *a, const void *b)
> +{
> +       const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
> +       const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
> +       int zpos_a = (sa->zpos << 16) + sa->plane->base.id;
> +       int zpos_b = (sb->zpos << 16) + sb->plane->base.id;
> +
> +       return zpos_a - zpos_b;
> +}
> +
> +/**
> + * drm_atomic_helper_crtc_normalize_zpos - calculate normalized zpos
> values
> + * @crtc: crtc with planes, which have to be considered for normalization
> + * @crtc_state: new atomic state to apply
> + *
> + * This function checks new states of all planes assigned to given crtc
> and
> + * calculates normalized zpos value for them. Planes are compared first
> by their
> + * zpos values, then by plane id (if zpos equals). Plane with lowest zpos
> value
> + * is at the bottom. The plane_state->normalized_zpos is then filled with
> uniqe
> + * values from 0 to number of active planes in crtc minus one.
> + *
> + * RETURNS
> + * Zero for success or -errno
> + */
> +int drm_atomic_helper_crtc_normalize_zpos(struct drm_crtc *crtc,
> +                                             struct drm_crtc_state
> *crtc_state)
> +{
> +       struct drm_atomic_state *state = crtc_state->state;
> +       struct drm_device *dev = crtc->dev;
> +       int total_planes = dev->mode_config.num_total_plane;
> +       struct drm_plane_state **states;
> +       struct drm_plane *plane;
> +       int i, n = 0;
> +       int ret = 0;
> +
> +       DRM_DEBUG_ATOMIC("[CRTC:%d:%s] calculating normalized zpos
> values\n",
> +                        crtc->base.id, crtc->name);
> +
> +       states = kmalloc(total_planes * sizeof(*states), GFP_KERNEL);
> +       if (!states)
> +               return -ENOMEM;
> +
> +       /*
> +        * Normalization process might create new states for planes which
> +        * normalized_zpos has to be recalculated.
> +        */
> +       drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> +               struct drm_plane_state *plane_state =
> +                       drm_atomic_get_plane_state(state, plane);
> +               if (IS_ERR(plane_state)) {
> +                       ret = PTR_ERR(plane_state);
> +                       goto fail;
> +               }
> +               states[n++] = plane_state;
> +               DRM_DEBUG_ATOMIC("[PLANE:%d:%s] processing zpos value
> %d\n",
> +                                plane->base.id, plane->name,
> plane_state->zpos);
> +       }
> +
> +       sort(states, n, sizeof(*states), drm_atomic_state_zpos_cmp, NULL);
> +
> +       for (i = 0; i < n; i++) {
> +               plane = states[i]->plane;
> +               states[i]->normalized_zpos = i;
> +               DRM_DEBUG_ATOMIC("[PLANE:%d:%s] normalized zpos value
> %d\n",
> +                                plane->base.id, plane->name, i);
> +       }
> +fail:
> +       kfree(states);
> +       return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_crtc_normalize_zpos);
> +
> +static int drm_atomic_helper_normalize_zpos(struct drm_device *dev,
> +                                           struct drm_atomic_state *state)
> +{
> +       struct drm_crtc *crtc;
> +       struct drm_crtc_state *crtc_state;
> +       struct drm_plane *plane;
> +       struct drm_plane_state *plane_state;
> +       int i, ret = 0;
> +
> +       /*
> +        * If zpos_property is not initialized, then it makes no sense
> +        * to calculate normalized zpos values.
> +        */
> +       if (!dev->mode_config.zpos_property)
> +               return 0;
> +
> +       for_each_plane_in_state(state, plane, plane_state, i) {
> +               crtc = plane_state->crtc;
> +               if (!crtc)
> +                       continue;
> +               if (plane->state->zpos != plane_state->zpos) {
> +                       crtc_state =
> +                               drm_atomic_get_existing_crtc_state(state,
> crtc);
> +                       crtc_state->zpos_changed = true;
> +               }
> +       }
> +
> +       for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +               if (crtc_state->plane_mask != crtc->state->plane_mask ||
> +                   crtc_state->zpos_changed) {
> +                       ret = drm_atomic_helper_crtc_normalize_zpos(crtc,
> +
>  crtc_state);
> +                       if (ret)
> +                               return ret;
> +               }
> +       }
> +       return 0;
> +}
> +
>  /**
>   * drm_atomic_helper_check_planes - validate state object for planes
> changes
>   * @dev: DRM device
> @@ -532,6 +644,10 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>         struct drm_plane_state *plane_state;
>         int i, ret = 0;
>
> +       ret = drm_atomic_helper_normalize_zpos(dev, state);
> +       if (ret)
> +               return ret;
> +
>         for_each_plane_in_state(state, plane, plane_state, i) {
>                 const struct drm_plane_helper_funcs *funcs;
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 62fa95fa5471..54a21e7c1ca5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5880,6 +5880,59 @@ struct drm_property
> *drm_mode_create_rotation_property(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_mode_create_rotation_property);
>
>  /**
> + * drm_mode_create_zpos_property - create muttable zpos property
> + * @dev: DRM device
> + *
> + * This function initializes generic muttable zpos property and enables
> support
> + * for it in drm core. Drivers can then attach this property to planes to
> enable
> + * support for configurable planes arrangement during blending operation.
> + * Drivers can also use drm_atomic_helper_normalize_zpos() function to
> calculate
> + * drm_plane_state->normalized_zpos values.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_mode_create_zpos_property(struct drm_device *dev)
> +{
> +       struct drm_property *prop;
> +
> +       prop = drm_property_create_range(dev, 0, "zpos", 0, 255);
> +       if (!prop)
> +               return -ENOMEM;
> +
> +       dev->mode_config.zpos_property = prop;
> +       return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_zpos_property);
> +
> +/**
> + * drm_plane_create_zpos_property - create immuttable zpos property
> + * @dev: DRM device
> + *
> + * This function initializes generic immuttable zpos property and enables
> + * support for it in drm core. Using this property driver lets userspace
> + * to get the arrangement of the planes for blending operation and
> notifies
> + * it that the hardware (or driver) doesn't support changing of the
> planes'
> + * order.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_mode_create_zpos_immutable_property(struct drm_device *dev)
> +{
> +       struct drm_property *prop;
> +
> +       prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
> "zpos",
> +                                        0, 255);
> +       if (!prop)
> +               return -ENOMEM;
> +
> +       dev->mode_config.zpos_immutable_property = prop;
> +       return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_zpos_immutable_property);
> +
> +/**
>   * DOC: Tile group
>   *
>   * Tile groups are used to represent tiled monitors with a unique
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3b040b355472..0607ad2ce918 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -305,6 +305,7 @@ struct drm_plane_helper_funcs;
>   * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
>   * @active_changed: crtc_state->active has been toggled.
>   * @connectors_changed: connectors to this crtc have been updated
> + * @zpos_changed: zpos values of planes on this crtc have been updated
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached
> planes
>   * @last_vblank_count: for helpers and drivers to capture the vblank of
> the
>   *     update to ensure framebuffer cleanup isn't done too early
> @@ -331,6 +332,7 @@ struct drm_crtc_state {
>         bool mode_changed : 1;
>         bool active_changed : 1;
>         bool connectors_changed : 1;
> +       bool zpos_changed : 1;
>
>         /* attached planes bitmask:
>          * WARNING: transitional helpers do not maintain plane_mask so
> @@ -1243,6 +1245,9 @@ struct drm_connector {
>   *     plane (in 16.16)
>   * @src_w: width of visible portion of plane (in 16.16)
>   * @src_h: height of visible portion of plane (in 16.16)
> + * @zpos: priority of the given plane on crtc (optional)
> + * @normalized_zpos: normalized value of zpos: uniqe, range from 0 to
> + *     (number of planes - 1) for given crtc
>   * @state: backpointer to global drm_atomic_state
>   */
>  struct drm_plane_state {
> @@ -1263,6 +1268,10 @@ struct drm_plane_state {
>         /* Plane rotation */
>         unsigned int rotation;
>
> +       /* Plane zpos */
> +       unsigned int zpos;
> +       unsigned int normalized_zpos;
> +
>         struct drm_atomic_state *state;
>  };
>
> @@ -2083,6 +2092,8 @@ struct drm_mode_config {
>         struct drm_property *tile_property;
>         struct drm_property *plane_type_property;
>         struct drm_property *rotation_property;
> +       struct drm_property *zpos_property;
> +       struct drm_property *zpos_immutable_property;
>         struct drm_property *prop_src_x;
>         struct drm_property *prop_src_y;
>         struct drm_property *prop_src_w;
> @@ -2484,6 +2495,9 @@ extern struct drm_property
> *drm_mode_create_rotation_property(struct drm_device
>  extern unsigned int drm_rotation_simplify(unsigned int rotation,
>                                           unsigned int
> supported_rotations);
>
> +extern int drm_mode_create_zpos_property(struct drm_device *dev);
> +extern int drm_mode_create_zpos_immutable_property(struct drm_device
> *dev);
> +
>  /* Helpers */
>
>  static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
> --
> 1.9.2
>
>


-- 

Benjamin Gaignard

Graphic Working Group

Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs

Follow *Linaro: *Facebook <http://www.facebook.com/pages/Linaro> | Twitter
<http://twitter.com/#!/linaroorg> | Blog
<http://www.linaro.org/linaro-blog/>

[-- Attachment #1.2: Type: text/html, Size: 20388 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v3 1/3] drm: add generic zpos property

[Ville Syrjälä]

On Tue, Jan 12, 2016 at 02:39:18PM +0100, Marek Szyprowski wrote:
> This patch adds support for generic plane's zpos property property with
> well-defined semantics:
> - added zpos properties to drm core and plane state structures
> - added helpers for normalizing zpos properties of given set of planes
> - well defined semantics: planes are sorted by zpos values and then plane
>   id value if zpos equals
> 
> Normalized zpos values are calculated automatically when generic
> muttable zpos property has been initialized. Drivers can simply use
> plane_state->normalized_zpos in their atomic_check and/or plane_update
> callbacks without any additional calls to DRM core.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  Documentation/DocBook/gpu.tmpl      |  14 ++++-
>  drivers/gpu/drm/drm_atomic.c        |   4 ++
>  drivers/gpu/drm/drm_atomic_helper.c | 116 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c          |  53 ++++++++++++++++
>  include/drm/drm_crtc.h              |  14 +++++
>  5 files changed, 199 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 6c6e81a9eaf4..f6b7236141b6 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -2004,7 +2004,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Description/Restrictions</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="37" valign="top" >DRM</td>
> +	<td rowspan="38" valign="top" >DRM</td>
>  	<td valign="top" >Generic</td>
>  	<td valign="top" >“rotation”</td>
>  	<td valign="top" >BITMASK</td>
> @@ -2256,7 +2256,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >property to suggest an Y offset for a connector</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="3" valign="top" >Optional</td>
> +	<td rowspan="4" valign="top" >Optional</td>
>  	<td valign="top" >“scaling mode”</td>
>  	<td valign="top" >ENUM</td>
>  	<td valign="top" >{ "None", "Full", "Center", "Full aspect" }</td>
> @@ -2280,6 +2280,16 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >TBD</td>
>  	</tr>
>  	<tr>
> +	<td valign="top" > "zpos" </td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=255</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Plane's 'z' position during blending (0 for background, 255 for frontmost).
> +		If two planes assigned to same CRTC have equal zpos values, the plane with higher plane
> +		id is treated as closer to front. Can be IMMUTABLE if driver doesn't support changing
> +		planes' order.</td>

I don't think this is going to work very well. Mixing the same range
of 0-255 for all planes, with potentially some of them being
IMMUTABLE for sure won't end well, or at least won't allow userspace to
see if there are any constraints between the zpos of the planes.

So I think what we should do is let the driver specify the valid range,
and get rid of the obj id based conflict resolution in favor of just
rejecting conflicts outright. In cases where you can't move the planes
between crtcs, the driver ought to specify the range based on the
number of planes present on the crtc. If planes can be moved betweens
crtcs the range obviously needs to be larger to accomodate all the
possible planes on the crtc.

Eg. on Intel VLV/CHV we could have the following setup:
primary  zpos 0-2
sprite 0 zpos 0-2
sprite 1 zpos 0-2
cursor   zpos 3

That makes it very clear the curso is always on top, and the other
planes can be rearranged more or less freely. These planes can't be
moved between crtcs, so each there's an identical set of planes for
each crtc.

On old Intel hw (gen2/3) we could have something like:
plane A zpos 0-3
plane B zpos 0-3
plane C zpos 0-3
overlay zpos 0-3
cursor B zpos 4
cursor A zpos 5

Most of these planes can be moved between crtcs, and IIRC there
are probably more constraints on exactly how they can be stacked, but
this is at least fairly close to the truth. Again the cursors are always
on top, and in this case the order between the two cursor planes is also
fixed.

I did originally have the same obj id based sorting idea (and even
posted some kind of a patch for it IIRC) but that was before atomic
existed, so there was a real need to allow reordering the planes with
just a single setplane ioctl call. With atomic I don't see any real
benefit from it the obj id based sorting, and as I've noted there are
definitely downsides to it.

> +	</tr>
> +	<tr>
>  	<td rowspan="20" valign="top" >i915</td>
>  	<td rowspan="2" valign="top" >Generic</td>
>  	<td valign="top" >"Broadcast RGB"</td>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 6a21e5c378c1..97bb069cb6a3 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -614,6 +614,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		state->src_h = val;
>  	} else if (property == config->rotation_property) {
  		state->rotation = val;
> +	} else if (property == config->zpos_property) {
> +		state->zpos = val;
>  	} else if (plane->funcs->atomic_set_property) {
>  		return plane->funcs->atomic_set_property(plane, state,
>  				property, val);
> @@ -670,6 +672,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  		*val = state->src_h;
>  	} else if (property == config->rotation_property) {
>  		*val = state->rotation;
> +	} else if (property == config->zpos_property) {
> +		*val = state->zpos;
>  	} else if (plane->funcs->atomic_get_property) {
>  		return plane->funcs->atomic_get_property(plane, state, property, val);
>  	} else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d0d4b2ff7c21..257946fac94b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <linux/fence.h>
> +#include <linux/sort.h>
>  
>  /**
>   * DOC: overview
> @@ -507,6 +508,117 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
>  
> +static int drm_atomic_state_zpos_cmp(const void *a, const void *b)
> +{
> +	const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
> +	const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
> +	int zpos_a = (sa->zpos << 16) + sa->plane->base.id;
> +	int zpos_b = (sb->zpos << 16) + sb->plane->base.id;
> +
> +	return zpos_a - zpos_b;
> +}
> +
> +/**
> + * drm_atomic_helper_crtc_normalize_zpos - calculate normalized zpos values
> + * @crtc: crtc with planes, which have to be considered for normalization
> + * @crtc_state: new atomic state to apply
> + *
> + * This function checks new states of all planes assigned to given crtc and
> + * calculates normalized zpos value for them. Planes are compared first by their
> + * zpos values, then by plane id (if zpos equals). Plane with lowest zpos value
> + * is at the bottom. The plane_state->normalized_zpos is then filled with uniqe
> + * values from 0 to number of active planes in crtc minus one.
> + *
> + * RETURNS
> + * Zero for success or -errno
> + */
> +int drm_atomic_helper_crtc_normalize_zpos(struct drm_crtc *crtc,
> +					      struct drm_crtc_state *crtc_state)
> +{
> +	struct drm_atomic_state *state = crtc_state->state;
> +	struct drm_device *dev = crtc->dev;
> +	int total_planes = dev->mode_config.num_total_plane;
> +	struct drm_plane_state **states;
> +	struct drm_plane *plane;
> +	int i, n = 0;
> +	int ret = 0;
> +
> +	DRM_DEBUG_ATOMIC("[CRTC:%d:%s] calculating normalized zpos values\n",
> +			 crtc->base.id, crtc->name);
> +
> +	states = kmalloc(total_planes * sizeof(*states), GFP_KERNEL);
> +	if (!states)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Normalization process might create new states for planes which
> +	 * normalized_zpos has to be recalculated.
> +	 */
> +	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> +		struct drm_plane_state *plane_state =
> +			drm_atomic_get_plane_state(state, plane);
> +		if (IS_ERR(plane_state)) {
> +			ret = PTR_ERR(plane_state);
> +			goto fail;
> +		}
> +		states[n++] = plane_state;
> +		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] processing zpos value %d\n",
> +				 plane->base.id, plane->name, plane_state->zpos);
> +	}
> +
> +	sort(states, n, sizeof(*states), drm_atomic_state_zpos_cmp, NULL);
> +
> +	for (i = 0; i < n; i++) {
> +		plane = states[i]->plane;
> +		states[i]->normalized_zpos = i;
> +		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] normalized zpos value %d\n",
> +				 plane->base.id, plane->name, i);
> +	}
> +fail:
> +	kfree(states);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_crtc_normalize_zpos);
> +
> +static int drm_atomic_helper_normalize_zpos(struct drm_device *dev,
> +					    struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
> +	int i, ret = 0;
> +
> +	/*
> +	 * If zpos_property is not initialized, then it makes no sense
> +	 * to calculate normalized zpos values.
> +	 */
> +	if (!dev->mode_config.zpos_property)
> +		return 0;
> +
> +	for_each_plane_in_state(state, plane, plane_state, i) {
> +		crtc = plane_state->crtc;
> +		if (!crtc)
> +			continue;
> +		if (plane->state->zpos != plane_state->zpos) {
> +			crtc_state =
> +				drm_atomic_get_existing_crtc_state(state, crtc);
> +			crtc_state->zpos_changed = true;
> +		}
> +	}
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (crtc_state->plane_mask != crtc->state->plane_mask ||
> +		    crtc_state->zpos_changed) {
> +			ret = drm_atomic_helper_crtc_normalize_zpos(crtc,
> +								    crtc_state);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
>  /**
>   * drm_atomic_helper_check_planes - validate state object for planes changes
>   * @dev: DRM device
> @@ -532,6 +644,10 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  	struct drm_plane_state *plane_state;
>  	int i, ret = 0;
>  
> +	ret = drm_atomic_helper_normalize_zpos(dev, state);
> +	if (ret)
> +		return ret;
> +
>  	for_each_plane_in_state(state, plane, plane_state, i) {
>  		const struct drm_plane_helper_funcs *funcs;
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 62fa95fa5471..54a21e7c1ca5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5880,6 +5880,59 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_mode_create_rotation_property);
>  
>  /**
> + * drm_mode_create_zpos_property - create muttable zpos property
> + * @dev: DRM device
> + *
> + * This function initializes generic muttable zpos property and enables support
> + * for it in drm core. Drivers can then attach this property to planes to enable
> + * support for configurable planes arrangement during blending operation.
> + * Drivers can also use drm_atomic_helper_normalize_zpos() function to calculate
> + * drm_plane_state->normalized_zpos values.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_mode_create_zpos_property(struct drm_device *dev)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(dev, 0, "zpos", 0, 255);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	dev->mode_config.zpos_property = prop;
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_zpos_property);
> +
> +/**
> + * drm_plane_create_zpos_property - create immuttable zpos property
> + * @dev: DRM device
> + *
> + * This function initializes generic immuttable zpos property and enables
> + * support for it in drm core. Using this property driver lets userspace
> + * to get the arrangement of the planes for blending operation and notifies
> + * it that the hardware (or driver) doesn't support changing of the planes'
> + * order.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_mode_create_zpos_immutable_property(struct drm_device *dev)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, "zpos",
> +					 0, 255);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	dev->mode_config.zpos_immutable_property = prop;
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_zpos_immutable_property);
> +
> +/**
>   * DOC: Tile group
>   *
>   * Tile groups are used to represent tiled monitors with a unique
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3b040b355472..0607ad2ce918 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -305,6 +305,7 @@ struct drm_plane_helper_funcs;
>   * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
>   * @active_changed: crtc_state->active has been toggled.
>   * @connectors_changed: connectors to this crtc have been updated
> + * @zpos_changed: zpos values of planes on this crtc have been updated
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @last_vblank_count: for helpers and drivers to capture the vblank of the
>   * 	update to ensure framebuffer cleanup isn't done too early
> @@ -331,6 +332,7 @@ struct drm_crtc_state {
>  	bool mode_changed : 1;
>  	bool active_changed : 1;
>  	bool connectors_changed : 1;
> +	bool zpos_changed : 1;
>  
>  	/* attached planes bitmask:
>  	 * WARNING: transitional helpers do not maintain plane_mask so
> @@ -1243,6 +1245,9 @@ struct drm_connector {
>   *	plane (in 16.16)
>   * @src_w: width of visible portion of plane (in 16.16)
>   * @src_h: height of visible portion of plane (in 16.16)
> + * @zpos: priority of the given plane on crtc (optional)
> + * @normalized_zpos: normalized value of zpos: uniqe, range from 0 to
> + *	(number of planes - 1) for given crtc
>   * @state: backpointer to global drm_atomic_state
>   */
>  struct drm_plane_state {
> @@ -1263,6 +1268,10 @@ struct drm_plane_state {
>  	/* Plane rotation */
>  	unsigned int rotation;
>  
> +	/* Plane zpos */
> +	unsigned int zpos;
> +	unsigned int normalized_zpos;
> +
>  	struct drm_atomic_state *state;
>  };
>  
> @@ -2083,6 +2092,8 @@ struct drm_mode_config {
>  	struct drm_property *tile_property;
>  	struct drm_property *plane_type_property;
>  	struct drm_property *rotation_property;
> +	struct drm_property *zpos_property;
> +	struct drm_property *zpos_immutable_property;
>  	struct drm_property *prop_src_x;
>  	struct drm_property *prop_src_y;
>  	struct drm_property *prop_src_w;
> @@ -2484,6 +2495,9 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
>  extern unsigned int drm_rotation_simplify(unsigned int rotation,
>  					  unsigned int supported_rotations);
>  
> +extern int drm_mode_create_zpos_property(struct drm_device *dev);
> +extern int drm_mode_create_zpos_immutable_property(struct drm_device *dev);
> +
>  /* Helpers */
>  
>  static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
> -- 
> 1.9.2

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 1/3] drm: add generic zpos property

[Marek Szyprowski]

Hello,

On 2016-01-14 11:46, Ville Syrjälä wrote:
> On Tue, Jan 12, 2016 at 02:39:18PM +0100, Marek Szyprowski wrote:
>> This patch adds support for generic plane's zpos property property with
>> well-defined semantics:
>> - added zpos properties to drm core and plane state structures
>> - added helpers for normalizing zpos properties of given set of planes
>> - well defined semantics: planes are sorted by zpos values and then plane
>>    id value if zpos equals
>>
>> Normalized zpos values are calculated automatically when generic
>> muttable zpos property has been initialized. Drivers can simply use
>> plane_state->normalized_zpos in their atomic_check and/or plane_update
>> callbacks without any additional calls to DRM core.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   Documentation/DocBook/gpu.tmpl      |  14 ++++-
>>   drivers/gpu/drm/drm_atomic.c        |   4 ++
>>   drivers/gpu/drm/drm_atomic_helper.c | 116 ++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/drm_crtc.c          |  53 ++++++++++++++++
>>   include/drm/drm_crtc.h              |  14 +++++
>>   5 files changed, 199 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
>> index 6c6e81a9eaf4..f6b7236141b6 100644
>> --- a/Documentation/DocBook/gpu.tmpl
>> +++ b/Documentation/DocBook/gpu.tmpl
>> @@ -2004,7 +2004,7 @@ void intel_crt_init(struct drm_device *dev)
>>   	<td valign="top" >Description/Restrictions</td>
>>   	</tr>
>>   	<tr>
>> -	<td rowspan="37" valign="top" >DRM</td>
>> +	<td rowspan="38" valign="top" >DRM</td>
>>   	<td valign="top" >Generic</td>
>>   	<td valign="top" >“rotation”</td>
>>   	<td valign="top" >BITMASK</td>
>> @@ -2256,7 +2256,7 @@ void intel_crt_init(struct drm_device *dev)
>>   	<td valign="top" >property to suggest an Y offset for a connector</td>
>>   	</tr>
>>   	<tr>
>> -	<td rowspan="3" valign="top" >Optional</td>
>> +	<td rowspan="4" valign="top" >Optional</td>
>>   	<td valign="top" >“scaling mode”</td>
>>   	<td valign="top" >ENUM</td>
>>   	<td valign="top" >{ "None", "Full", "Center", "Full aspect" }</td>
>> @@ -2280,6 +2280,16 @@ void intel_crt_init(struct drm_device *dev)
>>   	<td valign="top" >TBD</td>
>>   	</tr>
>>   	<tr>
>> +	<td valign="top" > "zpos" </td>
>> +	<td valign="top" >RANGE</td>
>> +	<td valign="top" >Min=0, Max=255</td>
>> +	<td valign="top" >Plane</td>
>> +	<td valign="top" >Plane's 'z' position during blending (0 for background, 255 for frontmost).
>> +		If two planes assigned to same CRTC have equal zpos values, the plane with higher plane
>> +		id is treated as closer to front. Can be IMMUTABLE if driver doesn't support changing
>> +		planes' order.</td>
> I don't think this is going to work very well. Mixing the same range
> of 0-255 for all planes, with potentially some of them being
> IMMUTABLE for sure won't end well, or at least won't allow userspace to
> see if there are any constraints between the zpos of the planes.
>
> So I think what we should do is let the driver specify the valid range,
> and get rid of the obj id based conflict resolution in favor of just
> rejecting conflicts outright. In cases where you can't move the planes
> between crtcs, the driver ought to specify the range based on the
> number of planes present on the crtc. If planes can be moved betweens
> crtcs the range obviously needs to be larger to accomodate all the
> possible planes on the crtc.
>
> Eg. on Intel VLV/CHV we could have the following setup:
> primary  zpos 0-2
> sprite 0 zpos 0-2
> sprite 1 zpos 0-2
> cursor   zpos 3
>
> That makes it very clear the curso is always on top, and the other
> planes can be rearranged more or less freely. These planes can't be
> moved between crtcs, so each there's an identical set of planes for
> each crtc.
>
> On old Intel hw (gen2/3) we could have something like:
> plane A zpos 0-3
> plane B zpos 0-3
> plane C zpos 0-3
> overlay zpos 0-3
> cursor B zpos 4
> cursor A zpos 5
>
> Most of these planes can be moved between crtcs, and IIRC there
> are probably more constraints on exactly how they can be stacked, but
> this is at least fairly close to the truth. Again the cursors are always
> on top, and in this case the order between the two cursor planes is also
> fixed.

I wasn't aware of a hardware, which has limited configuration of zpos only
to some planes. I thought only about 2 cases: either completely configurable
planes arrangement, or planes fixed to some hw dependent order. I see no
problem to let drivers to define their own limits for mutable zpos property.

Now the question is weather we should allow to set the non-zero minimal 
value
for mutable zpos? I can imagine that there might be a hardware, which has
fixed background plane and a few configurable overlay planes. I assume that
in such case, the calculated normalized_zpos should still start from zero.

> I did originally have the same obj id based sorting idea (and even
> posted some kind of a patch for it IIRC) but that was before atomic
> existed, so there was a real need to allow reordering the planes with
> just a single setplane ioctl call. With atomic I don't see any real
> benefit from it the obj id based sorting, and as I've noted there are
> definitely downsides to it.

What are the downsides of using obj id as additional sorting criteria? It
solves all the ambiguities and simplifies checks (there is no need to check
if there are 2 planes of the same zpos value).

>> +	</tr>
>> +	<tr>
>>   	<td rowspan="20" valign="top" >i915</td>
>>   	<td rowspan="2" valign="top" >Generic</td>
>>   	<td valign="top" >"Broadcast RGB"</td>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 6a21e5c378c1..97bb069cb6a3 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -614,6 +614,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>   		state->src_h = val;
>>   	} else if (property == config->rotation_property) {
>    		state->rotation = val;
>> +	} else if (property == config->zpos_property) {
>> +		state->zpos = val;
>>   	} else if (plane->funcs->atomic_set_property) {
>>   		return plane->funcs->atomic_set_property(plane, state,
>>   				property, val);
>> @@ -670,6 +672,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>   		*val = state->src_h;
>>   	} else if (property == config->rotation_property) {
>>   		*val = state->rotation;
>> +	} else if (property == config->zpos_property) {
>> +		*val = state->zpos;
>>   	} else if (plane->funcs->atomic_get_property) {
>>   		return plane->funcs->atomic_get_property(plane, state, property, val);
>>   	} else {
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index d0d4b2ff7c21..257946fac94b 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -31,6 +31,7 @@
>>   #include <drm/drm_crtc_helper.h>
>>   #include <drm/drm_atomic_helper.h>
>>   #include <linux/fence.h>
>> +#include <linux/sort.h>
>>   
>>   /**
>>    * DOC: overview
>> @@ -507,6 +508,117 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>>   }
>>   EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
>>   
>> +static int drm_atomic_state_zpos_cmp(const void *a, const void *b)
>> +{
>> +	const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
>> +	const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
>> +	int zpos_a = (sa->zpos << 16) + sa->plane->base.id;
>> +	int zpos_b = (sb->zpos << 16) + sb->plane->base.id;
>> +
>> +	return zpos_a - zpos_b;
>> +}
>> +
>> +/**
>> + * drm_atomic_helper_crtc_normalize_zpos - calculate normalized zpos values
>> + * @crtc: crtc with planes, which have to be considered for normalization
>> + * @crtc_state: new atomic state to apply
>> + *
>> + * This function checks new states of all planes assigned to given crtc and
>> + * calculates normalized zpos value for them. Planes are compared first by their
>> + * zpos values, then by plane id (if zpos equals). Plane with lowest zpos value
>> + * is at the bottom. The plane_state->normalized_zpos is then filled with uniqe
>> + * values from 0 to number of active planes in crtc minus one.
>> + *
>> + * RETURNS
>> + * Zero for success or -errno
>> + */
>> +int drm_atomic_helper_crtc_normalize_zpos(struct drm_crtc *crtc,
>> +					      struct drm_crtc_state *crtc_state)
>> +{
>> +	struct drm_atomic_state *state = crtc_state->state;
>> +	struct drm_device *dev = crtc->dev;
>> +	int total_planes = dev->mode_config.num_total_plane;
>> +	struct drm_plane_state **states;
>> +	struct drm_plane *plane;
>> +	int i, n = 0;
>> +	int ret = 0;
>> +
>> +	DRM_DEBUG_ATOMIC("[CRTC:%d:%s] calculating normalized zpos values\n",
>> +			 crtc->base.id, crtc->name);
>> +
>> +	states = kmalloc(total_planes * sizeof(*states), GFP_KERNEL);
>> +	if (!states)
>> +		return -ENOMEM;
>> +
>> +	/*
>> +	 * Normalization process might create new states for planes which
>> +	 * normalized_zpos has to be recalculated.
>> +	 */
>> +	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
>> +		struct drm_plane_state *plane_state =
>> +			drm_atomic_get_plane_state(state, plane);
>> +		if (IS_ERR(plane_state)) {
>> +			ret = PTR_ERR(plane_state);
>> +			goto fail;
>> +		}
>> +		states[n++] = plane_state;
>> +		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] processing zpos value %d\n",
>> +				 plane->base.id, plane->name, plane_state->zpos);
>> +	}
>> +
>> +	sort(states, n, sizeof(*states), drm_atomic_state_zpos_cmp, NULL);
>> +
>> +	for (i = 0; i < n; i++) {
>> +		plane = states[i]->plane;
>> +		states[i]->normalized_zpos = i;
>> +		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] normalized zpos value %d\n",
>> +				 plane->base.id, plane->name, i);
>> +	}
>> +fail:
>> +	kfree(states);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_crtc_normalize_zpos);
>> +
>> +static int drm_atomic_helper_normalize_zpos(struct drm_device *dev,
>> +					    struct drm_atomic_state *state)
>> +{
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *crtc_state;
>> +	struct drm_plane *plane;
>> +	struct drm_plane_state *plane_state;
>> +	int i, ret = 0;
>> +
>> +	/*
>> +	 * If zpos_property is not initialized, then it makes no sense
>> +	 * to calculate normalized zpos values.
>> +	 */
>> +	if (!dev->mode_config.zpos_property)
>> +		return 0;
>> +
>> +	for_each_plane_in_state(state, plane, plane_state, i) {
>> +		crtc = plane_state->crtc;
>> +		if (!crtc)
>> +			continue;
>> +		if (plane->state->zpos != plane_state->zpos) {
>> +			crtc_state =
>> +				drm_atomic_get_existing_crtc_state(state, crtc);
>> +			crtc_state->zpos_changed = true;
>> +		}
>> +	}
>> +
>> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +		if (crtc_state->plane_mask != crtc->state->plane_mask ||
>> +		    crtc_state->zpos_changed) {
>> +			ret = drm_atomic_helper_crtc_normalize_zpos(crtc,
>> +								    crtc_state);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>>   /**
>>    * drm_atomic_helper_check_planes - validate state object for planes changes
>>    * @dev: DRM device
>> @@ -532,6 +644,10 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>>   	struct drm_plane_state *plane_state;
>>   	int i, ret = 0;
>>   
>> +	ret = drm_atomic_helper_normalize_zpos(dev, state);
>> +	if (ret)
>> +		return ret;
>> +
>>   	for_each_plane_in_state(state, plane, plane_state, i) {
>>   		const struct drm_plane_helper_funcs *funcs;
>>   
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 62fa95fa5471..54a21e7c1ca5 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -5880,6 +5880,59 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
>>   EXPORT_SYMBOL(drm_mode_create_rotation_property);
>>   
>>   /**
>> + * drm_mode_create_zpos_property - create muttable zpos property
>> + * @dev: DRM device
>> + *
>> + * This function initializes generic muttable zpos property and enables support
>> + * for it in drm core. Drivers can then attach this property to planes to enable
>> + * support for configurable planes arrangement during blending operation.
>> + * Drivers can also use drm_atomic_helper_normalize_zpos() function to calculate
>> + * drm_plane_state->normalized_zpos values.
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +int drm_mode_create_zpos_property(struct drm_device *dev)
>> +{
>> +	struct drm_property *prop;
>> +
>> +	prop = drm_property_create_range(dev, 0, "zpos", 0, 255);
>> +	if (!prop)
>> +		return -ENOMEM;
>> +
>> +	dev->mode_config.zpos_property = prop;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_mode_create_zpos_property);
>> +
>> +/**
>> + * drm_plane_create_zpos_property - create immuttable zpos property
>> + * @dev: DRM device
>> + *
>> + * This function initializes generic immuttable zpos property and enables
>> + * support for it in drm core. Using this property driver lets userspace
>> + * to get the arrangement of the planes for blending operation and notifies
>> + * it that the hardware (or driver) doesn't support changing of the planes'
>> + * order.
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +int drm_mode_create_zpos_immutable_property(struct drm_device *dev)
>> +{
>> +	struct drm_property *prop;
>> +
>> +	prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, "zpos",
>> +					 0, 255);
>> +	if (!prop)
>> +		return -ENOMEM;
>> +
>> +	dev->mode_config.zpos_immutable_property = prop;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_mode_create_zpos_immutable_property);
>> +
>> +/**
>>    * DOC: Tile group
>>    *
>>    * Tile groups are used to represent tiled monitors with a unique
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 3b040b355472..0607ad2ce918 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -305,6 +305,7 @@ struct drm_plane_helper_funcs;
>>    * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
>>    * @active_changed: crtc_state->active has been toggled.
>>    * @connectors_changed: connectors to this crtc have been updated
>> + * @zpos_changed: zpos values of planes on this crtc have been updated
>>    * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>>    * @last_vblank_count: for helpers and drivers to capture the vblank of the
>>    * 	update to ensure framebuffer cleanup isn't done too early
>> @@ -331,6 +332,7 @@ struct drm_crtc_state {
>>   	bool mode_changed : 1;
>>   	bool active_changed : 1;
>>   	bool connectors_changed : 1;
>> +	bool zpos_changed : 1;
>>   
>>   	/* attached planes bitmask:
>>   	 * WARNING: transitional helpers do not maintain plane_mask so
>> @@ -1243,6 +1245,9 @@ struct drm_connector {
>>    *	plane (in 16.16)
>>    * @src_w: width of visible portion of plane (in 16.16)
>>    * @src_h: height of visible portion of plane (in 16.16)
>> + * @zpos: priority of the given plane on crtc (optional)
>> + * @normalized_zpos: normalized value of zpos: uniqe, range from 0 to
>> + *	(number of planes - 1) for given crtc
>>    * @state: backpointer to global drm_atomic_state
>>    */
>>   struct drm_plane_state {
>> @@ -1263,6 +1268,10 @@ struct drm_plane_state {
>>   	/* Plane rotation */
>>   	unsigned int rotation;
>>   
>> +	/* Plane zpos */
>> +	unsigned int zpos;
>> +	unsigned int normalized_zpos;
>> +
>>   	struct drm_atomic_state *state;
>>   };
>>   
>> @@ -2083,6 +2092,8 @@ struct drm_mode_config {
>>   	struct drm_property *tile_property;
>>   	struct drm_property *plane_type_property;
>>   	struct drm_property *rotation_property;
>> +	struct drm_property *zpos_property;
>> +	struct drm_property *zpos_immutable_property;
>>   	struct drm_property *prop_src_x;
>>   	struct drm_property *prop_src_y;
>>   	struct drm_property *prop_src_w;
>> @@ -2484,6 +2495,9 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
>>   extern unsigned int drm_rotation_simplify(unsigned int rotation,
>>   					  unsigned int supported_rotations);
>>   
>> +extern int drm_mode_create_zpos_property(struct drm_device *dev);
>> +extern int drm_mode_create_zpos_immutable_property(struct drm_device *dev);
>> +
>>   /* Helpers */
>>   
>>   static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
>> -- 
>> 1.9.2

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

Re: [PATCH v3 1/3] drm: add generic zpos property

[Daniel Vetter]

On Fri, Jan 15, 2016 at 10:09:14AM +0100, Marek Szyprowski wrote:
> Hello,
> 
> On 2016-01-14 11:46, Ville Syrjälä wrote:
> >On Tue, Jan 12, 2016 at 02:39:18PM +0100, Marek Szyprowski wrote:
> >>This patch adds support for generic plane's zpos property property with
> >>well-defined semantics:
> >>- added zpos properties to drm core and plane state structures
> >>- added helpers for normalizing zpos properties of given set of planes
> >>- well defined semantics: planes are sorted by zpos values and then plane
> >>   id value if zpos equals
> >>
> >>Normalized zpos values are calculated automatically when generic
> >>muttable zpos property has been initialized. Drivers can simply use
> >>plane_state->normalized_zpos in their atomic_check and/or plane_update
> >>callbacks without any additional calls to DRM core.
> >>
> >>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >>---
> >>  Documentation/DocBook/gpu.tmpl      |  14 ++++-
> >>  drivers/gpu/drm/drm_atomic.c        |   4 ++
> >>  drivers/gpu/drm/drm_atomic_helper.c | 116 ++++++++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/drm_crtc.c          |  53 ++++++++++++++++
> >>  include/drm/drm_crtc.h              |  14 +++++
> >>  5 files changed, 199 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> >>index 6c6e81a9eaf4..f6b7236141b6 100644
> >>--- a/Documentation/DocBook/gpu.tmpl
> >>+++ b/Documentation/DocBook/gpu.tmpl
> >>@@ -2004,7 +2004,7 @@ void intel_crt_init(struct drm_device *dev)
> >>  	<td valign="top" >Description/Restrictions</td>
> >>  	</tr>
> >>  	<tr>
> >>-	<td rowspan="37" valign="top" >DRM</td>
> >>+	<td rowspan="38" valign="top" >DRM</td>
> >>  	<td valign="top" >Generic</td>
> >>  	<td valign="top" >“rotation”</td>
> >>  	<td valign="top" >BITMASK</td>
> >>@@ -2256,7 +2256,7 @@ void intel_crt_init(struct drm_device *dev)
> >>  	<td valign="top" >property to suggest an Y offset for a connector</td>
> >>  	</tr>
> >>  	<tr>
> >>-	<td rowspan="3" valign="top" >Optional</td>
> >>+	<td rowspan="4" valign="top" >Optional</td>
> >>  	<td valign="top" >“scaling mode”</td>
> >>  	<td valign="top" >ENUM</td>
> >>  	<td valign="top" >{ "None", "Full", "Center", "Full aspect" }</td>
> >>@@ -2280,6 +2280,16 @@ void intel_crt_init(struct drm_device *dev)
> >>  	<td valign="top" >TBD</td>
> >>  	</tr>
> >>  	<tr>
> >>+	<td valign="top" > "zpos" </td>
> >>+	<td valign="top" >RANGE</td>
> >>+	<td valign="top" >Min=0, Max=255</td>
> >>+	<td valign="top" >Plane</td>
> >>+	<td valign="top" >Plane's 'z' position during blending (0 for background, 255 for frontmost).
> >>+		If two planes assigned to same CRTC have equal zpos values, the plane with higher plane
> >>+		id is treated as closer to front. Can be IMMUTABLE if driver doesn't support changing
> >>+		planes' order.</td>
> >I don't think this is going to work very well. Mixing the same range
> >of 0-255 for all planes, with potentially some of them being
> >IMMUTABLE for sure won't end well, or at least won't allow userspace to
> >see if there are any constraints between the zpos of the planes.
> >
> >So I think what we should do is let the driver specify the valid range,
> >and get rid of the obj id based conflict resolution in favor of just
> >rejecting conflicts outright. In cases where you can't move the planes
> >between crtcs, the driver ought to specify the range based on the
> >number of planes present on the crtc. If planes can be moved betweens
> >crtcs the range obviously needs to be larger to accomodate all the
> >possible planes on the crtc.
> >
> >Eg. on Intel VLV/CHV we could have the following setup:
> >primary  zpos 0-2
> >sprite 0 zpos 0-2
> >sprite 1 zpos 0-2
> >cursor   zpos 3
> >
> >That makes it very clear the curso is always on top, and the other
> >planes can be rearranged more or less freely. These planes can't be
> >moved between crtcs, so each there's an identical set of planes for
> >each crtc.
> >
> >On old Intel hw (gen2/3) we could have something like:
> >plane A zpos 0-3
> >plane B zpos 0-3
> >plane C zpos 0-3
> >overlay zpos 0-3
> >cursor B zpos 4
> >cursor A zpos 5
> >
> >Most of these planes can be moved between crtcs, and IIRC there
> >are probably more constraints on exactly how they can be stacked, but
> >this is at least fairly close to the truth. Again the cursors are always
> >on top, and in this case the order between the two cursor planes is also
> >fixed.
> 
> I wasn't aware of a hardware, which has limited configuration of zpos only
> to some planes. I thought only about 2 cases: either completely configurable
> planes arrangement, or planes fixed to some hw dependent order. I see no
> problem to let drivers to define their own limits for mutable zpos property.
> 
> Now the question is weather we should allow to set the non-zero minimal
> value
> for mutable zpos? I can imagine that there might be a hardware, which has
> fixed background plane and a few configurable overlay planes. I assume that
> in such case, the calculated normalized_zpos should still start from zero.

The usual approach is to switch the property from a global one in
dev->mode_config.*_prop to a per-object one in e.g. plane->zpos_prop. We
can still register the name, but have different limits/types. That way you
could register a global mutable zpos with the range 0-3 and an immutable
zpos with values 4 or 5 for cursors. The generic decoding would still be
fairly simple.

	} else if (property == plane->zpos_property) {
		state->zpos = val;
	} else if (property == config->zpos_property) {
		state->zpos = val;

Plus some checking that no one attached both the global and obj-local
property of the same name to the same object.

Since this is a fairly simple add-on change and current drivers that
support zpos don't need it I think it makes total sense to do this as a
follow-up when we need it.

But might be good to document this somewhere (either commit message or
kerneldoc for zpos).

> >I did originally have the same obj id based sorting idea (and even
> >posted some kind of a patch for it IIRC) but that was before atomic
> >existed, so there was a real need to allow reordering the planes with
> >just a single setplane ioctl call. With atomic I don't see any real
> >benefit from it the obj id based sorting, and as I've noted there are
> >definitely downsides to it.
> 
> What are the downsides of using obj id as additional sorting criteria? It
> solves all the ambiguities and simplifies checks (there is no need to check
> if there are 2 planes of the same zpos value).

I think the sorting also has an upside that it avoids having to change all
the drivers, or adding some kind of flag. Drivers which don't support zpos
just get a sorted normlized zpos. If we don't do that we'd have to put
some unique default zpos in, which is going to make things messy.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 1/3] drm: add generic zpos property

[Ville Syrjälä]

On Fri, Jan 15, 2016 at 11:12:25AM +0100, Daniel Vetter wrote:
> On Fri, Jan 15, 2016 at 10:09:14AM +0100, Marek Szyprowski wrote:
> > Hello,
> > 
> > On 2016-01-14 11:46, Ville Syrjälä wrote:
> > >On Tue, Jan 12, 2016 at 02:39:18PM +0100, Marek Szyprowski wrote:
> > >>This patch adds support for generic plane's zpos property property with
> > >>well-defined semantics:
> > >>- added zpos properties to drm core and plane state structures
> > >>- added helpers for normalizing zpos properties of given set of planes
> > >>- well defined semantics: planes are sorted by zpos values and then plane
> > >>   id value if zpos equals
> > >>
> > >>Normalized zpos values are calculated automatically when generic
> > >>muttable zpos property has been initialized. Drivers can simply use
> > >>plane_state->normalized_zpos in their atomic_check and/or plane_update
> > >>callbacks without any additional calls to DRM core.
> > >>
> > >>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > >>---
> > >>  Documentation/DocBook/gpu.tmpl      |  14 ++++-
> > >>  drivers/gpu/drm/drm_atomic.c        |   4 ++
> > >>  drivers/gpu/drm/drm_atomic_helper.c | 116 ++++++++++++++++++++++++++++++++++++
> > >>  drivers/gpu/drm/drm_crtc.c          |  53 ++++++++++++++++
> > >>  include/drm/drm_crtc.h              |  14 +++++
> > >>  5 files changed, 199 insertions(+), 2 deletions(-)
> > >>
> > >>diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> > >>index 6c6e81a9eaf4..f6b7236141b6 100644
> > >>--- a/Documentation/DocBook/gpu.tmpl
> > >>+++ b/Documentation/DocBook/gpu.tmpl
> > >>@@ -2004,7 +2004,7 @@ void intel_crt_init(struct drm_device *dev)
> > >>  	<td valign="top" >Description/Restrictions</td>
> > >>  	</tr>
> > >>  	<tr>
> > >>-	<td rowspan="37" valign="top" >DRM</td>
> > >>+	<td rowspan="38" valign="top" >DRM</td>
> > >>  	<td valign="top" >Generic</td>
> > >>  	<td valign="top" >“rotation”</td>
> > >>  	<td valign="top" >BITMASK</td>
> > >>@@ -2256,7 +2256,7 @@ void intel_crt_init(struct drm_device *dev)
> > >>  	<td valign="top" >property to suggest an Y offset for a connector</td>
> > >>  	</tr>
> > >>  	<tr>
> > >>-	<td rowspan="3" valign="top" >Optional</td>
> > >>+	<td rowspan="4" valign="top" >Optional</td>
> > >>  	<td valign="top" >“scaling mode”</td>
> > >>  	<td valign="top" >ENUM</td>
> > >>  	<td valign="top" >{ "None", "Full", "Center", "Full aspect" }</td>
> > >>@@ -2280,6 +2280,16 @@ void intel_crt_init(struct drm_device *dev)
> > >>  	<td valign="top" >TBD</td>
> > >>  	</tr>
> > >>  	<tr>
> > >>+	<td valign="top" > "zpos" </td>
> > >>+	<td valign="top" >RANGE</td>
> > >>+	<td valign="top" >Min=0, Max=255</td>
> > >>+	<td valign="top" >Plane</td>
> > >>+	<td valign="top" >Plane's 'z' position during blending (0 for background, 255 for frontmost).
> > >>+		If two planes assigned to same CRTC have equal zpos values, the plane with higher plane
> > >>+		id is treated as closer to front. Can be IMMUTABLE if driver doesn't support changing
> > >>+		planes' order.</td>
> > >I don't think this is going to work very well. Mixing the same range
> > >of 0-255 for all planes, with potentially some of them being
> > >IMMUTABLE for sure won't end well, or at least won't allow userspace to
> > >see if there are any constraints between the zpos of the planes.
> > >
> > >So I think what we should do is let the driver specify the valid range,
> > >and get rid of the obj id based conflict resolution in favor of just
> > >rejecting conflicts outright. In cases where you can't move the planes
> > >between crtcs, the driver ought to specify the range based on the
> > >number of planes present on the crtc. If planes can be moved betweens
> > >crtcs the range obviously needs to be larger to accomodate all the
> > >possible planes on the crtc.
> > >
> > >Eg. on Intel VLV/CHV we could have the following setup:
> > >primary  zpos 0-2
> > >sprite 0 zpos 0-2
> > >sprite 1 zpos 0-2
> > >cursor   zpos 3
> > >
> > >That makes it very clear the curso is always on top, and the other
> > >planes can be rearranged more or less freely. These planes can't be
> > >moved between crtcs, so each there's an identical set of planes for
> > >each crtc.
> > >
> > >On old Intel hw (gen2/3) we could have something like:
> > >plane A zpos 0-3
> > >plane B zpos 0-3
> > >plane C zpos 0-3
> > >overlay zpos 0-3
> > >cursor B zpos 4
> > >cursor A zpos 5
> > >
> > >Most of these planes can be moved between crtcs, and IIRC there
> > >are probably more constraints on exactly how they can be stacked, but
> > >this is at least fairly close to the truth. Again the cursors are always
> > >on top, and in this case the order between the two cursor planes is also
> > >fixed.
> > 
> > I wasn't aware of a hardware, which has limited configuration of zpos only
> > to some planes. I thought only about 2 cases: either completely configurable
> > planes arrangement, or planes fixed to some hw dependent order. I see no
> > problem to let drivers to define their own limits for mutable zpos property.
> > 
> > Now the question is weather we should allow to set the non-zero minimal
> > value
> > for mutable zpos? I can imagine that there might be a hardware, which has
> > fixed background plane and a few configurable overlay planes. I assume that
> > in such case, the calculated normalized_zpos should still start from zero.
> 
> The usual approach is to switch the property from a global one in
> dev->mode_config.*_prop to a per-object one in e.g. plane->zpos_prop. We
> can still register the name, but have different limits/types. That way you
> could register a global mutable zpos with the range 0-3 and an immutable
> zpos with values 4 or 5 for cursors. The generic decoding would still be
> fairly simple.
> 
> 	} else if (property == plane->zpos_property) {
> 		state->zpos = val;
> 	} else if (property == config->zpos_property) {
> 		state->zpos = val;
> 
> Plus some checking that no one attached both the global and obj-local
> property of the same name to the same object.
> 
> Since this is a fairly simple add-on change and current drivers that
> support zpos don't need it I think it makes total sense to do this as a
> follow-up when we need it.
> 
> But might be good to document this somewhere (either commit message or
> kerneldoc for zpos).
> 
> > >I did originally have the same obj id based sorting idea (and even
> > >posted some kind of a patch for it IIRC) but that was before atomic
> > >existed, so there was a real need to allow reordering the planes with
> > >just a single setplane ioctl call. With atomic I don't see any real
> > >benefit from it the obj id based sorting, and as I've noted there are
> > >definitely downsides to it.
> > 
> > What are the downsides of using obj id as additional sorting criteria? It
> > solves all the ambiguities and simplifies checks (there is no need to check
> > if there are 2 planes of the same zpos value).
> 
> I think the sorting also has an upside that it avoids having to change all
> the drivers, or adding some kind of flag. Drivers which don't support zpos
> just get a sorted normlized zpos. If we don't do that we'd have to put
> some unique default zpos in, which is going to make things messy.

Why would it make it messy? We already assign the obj ids in order (in
practice), so assigning a unique zpos in order would end up doing with
the same result.

-- 
Ville Syrjälä
Intel OTC