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

[PATCH 0/4] 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 2/4 and 3/4 are fixes for Exynos DRM driver, which are required
to properly implement generic zpos handling. However they can be also
merged regardless of the rest of the patches (if there is a need for
further discussion on the generic zpos property feature).

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

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Patch summary:

Marek Szyprowski (4):
  drm: add generic zpos property
  drm/exynos: crtc: rework atomic_{begin,flush}
  drm/exynos: mixer: properly update all planes on the same vblank event
  drm/exynos: use generic code for managing zpos plane property

 Documentation/DocBook/gpu.tmpl                | 14 +++++-
 drivers/gpu/drm/drm_atomic.c                  |  4 ++
 drivers/gpu/drm/drm_atomic_helper.c           | 52 ++++++++++++++++++++
 drivers/gpu/drm/drm_crtc.c                    | 13 +++++
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 14 +++---
 drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 14 +++---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 20 ++------
 drivers/gpu/drm/exynos/exynos_drm_drv.h       | 11 ++---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 14 +++---
 drivers/gpu/drm/exynos/exynos_drm_plane.c     | 68 +++++++--------------------
 drivers/gpu/drm/exynos/exynos_mixer.c         | 53 ++++++++++++++++-----
 include/drm/drm_atomic_helper.h               |  2 +
 include/drm/drm_crtc.h                        | 13 +++++
 13 files changed, 187 insertions(+), 105 deletions(-)

-- 
1.9.2

[PATCH 1/4] drm: add support for 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

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 | 52 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc.c          | 13 ++++++++++
 include/drm/drm_atomic_helper.h     |  2 ++
 include/drm/drm_crtc.h              | 13 ++++++++++
 6 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 6c6e81a9eaf4..e81acd999891 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
+		plane's 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 268d37f26960..de3ca33eb696 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
@@ -2781,3 +2782,54 @@ void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
 	kfree(state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state);
+
+/**
+ * __drm_atomic_helper_plane_zpos_cmp - compare zpos value of two planes
+ * @a: pointer to first plane
+ * @b: pointer to second plane
+ *
+ * This function is used for comparing two planes while sorting them to assign
+ * a normalized zpos values. Planes are compared first by their zpos values,
+ * then in case they equal, by plane id.
+ */
+static int __drm_atomic_helper_plane_zpos_cmp(const void *a, const void *b)
+{
+	const struct drm_plane *pa = *(struct drm_plane **)a;
+	const struct drm_plane *pb = *(struct drm_plane **)b;
+	int zpos_a = 0, zpos_b = 0;
+
+	if (pa->state)
+		zpos_a = pa->state->zpos << 16;
+	if (pb->state)
+		zpos_b = pb->state->zpos << 16;
+
+	zpos_a += pa->base.id;
+	zpos_b += pb->base.id;
+
+	return zpos_a - zpos_b;
+}
+
+/**
+ * drm_atomic_helper_normalize_zpos - calculate normalized zpos values
+ * @planes: arrays of pointer to planes to consider for normalization
+ * @count: number of planes in the above array
+ *
+ * This function takes arrays of pointers to planes and calculates normalized
+ * zpos value for them taking into account each planes[i]->state->zpos value
+ * and plane's id (if zpos equals). The plane[i]->state->normalized_zpos is
+ * then filled with uniqe values from 0 to count-1.
+ * Note: a side effect of this function is the fact that the planes array will
+ * be modified (sorted). It is up to the called to construct planes array with
+ * all planes that have been assigned to given crtc.
+ */
+void drm_atomic_helper_normalize_zpos(struct drm_plane *planes[], int count)
+{
+	int i;
+
+	sort(planes, count, sizeof(struct drm_plane *),
+	     __drm_atomic_helper_plane_zpos_cmp, NULL);
+	for (i = 0; i < count; i++)
+		if (planes[i]->state)
+			planes[i]->state->normalized_zpos = i;
+}
+EXPORT_SYMBOL(drm_atomic_helper_normalize_zpos);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 62fa95fa5471..51474ea179f6 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5879,6 +5879,19 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_mode_create_rotation_property);
 
+struct drm_property *drm_plane_create_zpos_property(struct drm_device *dev)
+{
+	return drm_property_create_range(dev, 0, "zpos", 0, 255);
+}
+EXPORT_SYMBOL(drm_plane_create_zpos_property);
+
+struct drm_property *drm_plane_create_zpos_immutable_property(struct drm_device *dev)
+{
+	return drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, "zpos",
+					 0, 255);
+}
+EXPORT_SYMBOL(drm_plane_create_zpos_immutable_property);
+
 /**
  * DOC: Tile group
  *
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index a286cce98720..2a7ade5ad8bd 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -141,6 +141,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
 void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
 					  struct drm_connector_state *state);
 
+void drm_atomic_helper_normalize_zpos(struct drm_plane *planes[], int count);
+
 /**
  * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
  * @plane: the loop cursor
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3b040b355472..5021aa0237b9 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1243,6 +1243,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 +1266,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 +2090,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 +2493,10 @@ 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 struct drm_property *drm_plane_create_zpos_property(struct drm_device *dev);
+
+extern struct drm_property *drm_plane_create_zpos_immutable_property(struct drm_device *dev);
+
 /* Helpers */
 
 static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
-- 
1.9.2

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

Re: [PATCH 1/4] drm: add support for generic zpos property

[Daniel Vetter]

On Tue, Jan 05, 2016 at 01:52:50PM +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
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

lgtm I think. Longer-term we want to think whether we don't want to
extract such extensions into separate files, and push the kerneldoc into
an overview DOC: section in there. Just to keep things more closely
together. Benjamin with drm/sti also needs this, so cc'ing him.

> ---
>  Documentation/DocBook/gpu.tmpl      | 14 ++++++++--
>  drivers/gpu/drm/drm_atomic.c        |  4 +++
>  drivers/gpu/drm/drm_atomic_helper.c | 52 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c          | 13 ++++++++++
>  include/drm/drm_atomic_helper.h     |  2 ++
>  include/drm/drm_crtc.h              | 13 ++++++++++
>  6 files changed, 96 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 6c6e81a9eaf4..e81acd999891 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" >a??rotationa??</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" >a??scaling modea??</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
> +		plane's 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 268d37f26960..de3ca33eb696 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
> @@ -2781,3 +2782,54 @@ void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>  	kfree(state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state);
> +
> +/**
> + * __drm_atomic_helper_plane_zpos_cmp - compare zpos value of two planes
> + * @a: pointer to first plane
> + * @b: pointer to second plane

Generally we don't do kerneldoc for non-exported functions. If you want
just keep the text itself as a comment, or better move it into the
official interface docs for drm_atomic_helper_normalize_zpos().

> + *
> + * This function is used for comparing two planes while sorting them to assign
> + * a normalized zpos values. Planes are compared first by their zpos values,
> + * then in case they equal, by plane id.
> + */
> +static int __drm_atomic_helper_plane_zpos_cmp(const void *a, const void *b)
> +{
> +	const struct drm_plane *pa = *(struct drm_plane **)a;
> +	const struct drm_plane *pb = *(struct drm_plane **)b;
> +	int zpos_a = 0, zpos_b = 0;
> +
> +	if (pa->state)
> +		zpos_a = pa->state->zpos << 16;
> +	if (pb->state)
> +		zpos_b = pb->state->zpos << 16;
> +
> +	zpos_a += pa->base.id;
> +	zpos_b += pb->base.id;
> +
> +	return zpos_a - zpos_b;
> +}
> +
> +/**
> + * drm_atomic_helper_normalize_zpos - calculate normalized zpos values
> + * @planes: arrays of pointer to planes to consider for normalization
> + * @count: number of planes in the above array
> + *
> + * This function takes arrays of pointers to planes and calculates normalized
> + * zpos value for them taking into account each planes[i]->state->zpos value
> + * and plane's id (if zpos equals). The plane[i]->state->normalized_zpos is
> + * then filled with uniqe values from 0 to count-1.
> + * Note: a side effect of this function is the fact that the planes array will
> + * be modified (sorted). It is up to the called to construct planes array with
> + * all planes that have been assigned to given crtc.
> + */
> +void drm_atomic_helper_normalize_zpos(struct drm_plane *planes[], int count)
> +{
> +	int i;
> +
> +	sort(planes, count, sizeof(struct drm_plane *),
> +	     __drm_atomic_helper_plane_zpos_cmp, NULL);
> +	for (i = 0; i < count; i++)
> +		if (planes[i]->state)
> +			planes[i]->state->normalized_zpos = i;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_normalize_zpos);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 62fa95fa5471..51474ea179f6 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5879,6 +5879,19 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_mode_create_rotation_property);
>  

Please add minimal kerneldoc for these two, with the suggestions that
drivers should use drm_atomic_helper_normalize_zpos(). Also, I'd
duplicated the semantic definition here (and explain why immutable
exists) from the gpu.tmpl table.

> +struct drm_property *drm_plane_create_zpos_property(struct drm_device *dev)
> +{
> +	return drm_property_create_range(dev, 0, "zpos", 0, 255);

The zpos property should be stored in dev_priv->mode_config.zpos_prop. We
need it there so that generic core code (in drm_atomic_plane_set_property)
can decode it. That way drivers only need to set up&attach it the prop,
but otherwise can deal with just the decoded values.

Another consideration: Should we reset zpos to something sane in the fbdev
helpers, like we do for rotation?

Thanks, Daniel
> +}
> +EXPORT_SYMBOL(drm_plane_create_zpos_property);
> +
> +struct drm_property *drm_plane_create_zpos_immutable_property(struct drm_device *dev)
> +{
> +	return drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, "zpos",
> +					 0, 255);
> +}
> +EXPORT_SYMBOL(drm_plane_create_zpos_immutable_property);
> +
>  /**
>   * DOC: Tile group
>   *
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index a286cce98720..2a7ade5ad8bd 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -141,6 +141,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>  void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>  					  struct drm_connector_state *state);
>  
> +void drm_atomic_helper_normalize_zpos(struct drm_plane *planes[], int count);
> +
>  /**
>   * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
>   * @plane: the loop cursor
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3b040b355472..5021aa0237b9 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1243,6 +1243,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 +1266,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 +2090,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 +2493,10 @@ 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 struct drm_property *drm_plane_create_zpos_property(struct drm_device *dev);
> +
> +extern struct drm_property *drm_plane_create_zpos_immutable_property(struct drm_device *dev);
> +
>  /* Helpers */
>  
>  static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
> -- 
> 1.9.2
> 

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] drm: add support for generic zpos property

[Daniel Vetter]

On Tue, Jan 05, 2016 at 01:52:50PM +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
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

lgtm I think. Longer-term we want to think whether we don't want to
extract such extensions into separate files, and push the kerneldoc into
an overview DOC: section in there. Just to keep things more closely
together. Benjamin with drm/sti also needs this, so cc'ing him.

> ---
>  Documentation/DocBook/gpu.tmpl      | 14 ++++++++--
>  drivers/gpu/drm/drm_atomic.c        |  4 +++
>  drivers/gpu/drm/drm_atomic_helper.c | 52 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c          | 13 ++++++++++
>  include/drm/drm_atomic_helper.h     |  2 ++
>  include/drm/drm_crtc.h              | 13 ++++++++++
>  6 files changed, 96 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 6c6e81a9eaf4..e81acd999891 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
> +		plane's 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 268d37f26960..de3ca33eb696 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
> @@ -2781,3 +2782,54 @@ void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>  	kfree(state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state);
> +
> +/**
> + * __drm_atomic_helper_plane_zpos_cmp - compare zpos value of two planes
> + * @a: pointer to first plane
> + * @b: pointer to second plane

Generally we don't do kerneldoc for non-exported functions. If you want
just keep the text itself as a comment, or better move it into the
official interface docs for drm_atomic_helper_normalize_zpos().

> + *
> + * This function is used for comparing two planes while sorting them to assign
> + * a normalized zpos values. Planes are compared first by their zpos values,
> + * then in case they equal, by plane id.
> + */
> +static int __drm_atomic_helper_plane_zpos_cmp(const void *a, const void *b)
> +{
> +	const struct drm_plane *pa = *(struct drm_plane **)a;
> +	const struct drm_plane *pb = *(struct drm_plane **)b;
> +	int zpos_a = 0, zpos_b = 0;
> +
> +	if (pa->state)
> +		zpos_a = pa->state->zpos << 16;
> +	if (pb->state)
> +		zpos_b = pb->state->zpos << 16;
> +
> +	zpos_a += pa->base.id;
> +	zpos_b += pb->base.id;
> +
> +	return zpos_a - zpos_b;
> +}
> +
> +/**
> + * drm_atomic_helper_normalize_zpos - calculate normalized zpos values
> + * @planes: arrays of pointer to planes to consider for normalization
> + * @count: number of planes in the above array
> + *
> + * This function takes arrays of pointers to planes and calculates normalized
> + * zpos value for them taking into account each planes[i]->state->zpos value
> + * and plane's id (if zpos equals). The plane[i]->state->normalized_zpos is
> + * then filled with uniqe values from 0 to count-1.
> + * Note: a side effect of this function is the fact that the planes array will
> + * be modified (sorted). It is up to the called to construct planes array with
> + * all planes that have been assigned to given crtc.
> + */
> +void drm_atomic_helper_normalize_zpos(struct drm_plane *planes[], int count)
> +{
> +	int i;
> +
> +	sort(planes, count, sizeof(struct drm_plane *),
> +	     __drm_atomic_helper_plane_zpos_cmp, NULL);
> +	for (i = 0; i < count; i++)
> +		if (planes[i]->state)
> +			planes[i]->state->normalized_zpos = i;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_normalize_zpos);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 62fa95fa5471..51474ea179f6 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5879,6 +5879,19 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_mode_create_rotation_property);
>  

Please add minimal kerneldoc for these two, with the suggestions that
drivers should use drm_atomic_helper_normalize_zpos(). Also, I'd
duplicated the semantic definition here (and explain why immutable
exists) from the gpu.tmpl table.

> +struct drm_property *drm_plane_create_zpos_property(struct drm_device *dev)
> +{
> +	return drm_property_create_range(dev, 0, "zpos", 0, 255);

The zpos property should be stored in dev_priv->mode_config.zpos_prop. We
need it there so that generic core code (in drm_atomic_plane_set_property)
can decode it. That way drivers only need to set up&attach it the prop,
but otherwise can deal with just the decoded values.

Another consideration: Should we reset zpos to something sane in the fbdev
helpers, like we do for rotation?

Thanks, Daniel
> +}
> +EXPORT_SYMBOL(drm_plane_create_zpos_property);
> +
> +struct drm_property *drm_plane_create_zpos_immutable_property(struct drm_device *dev)
> +{
> +	return drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, "zpos",
> +					 0, 255);
> +}
> +EXPORT_SYMBOL(drm_plane_create_zpos_immutable_property);
> +
>  /**
>   * DOC: Tile group
>   *
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index a286cce98720..2a7ade5ad8bd 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -141,6 +141,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>  void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>  					  struct drm_connector_state *state);
>  
> +void drm_atomic_helper_normalize_zpos(struct drm_plane *planes[], int count);
> +
>  /**
>   * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
>   * @plane: the loop cursor
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3b040b355472..5021aa0237b9 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1243,6 +1243,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 +1266,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 +2090,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 +2493,10 @@ 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 struct drm_property *drm_plane_create_zpos_property(struct drm_device *dev);
> +
> +extern struct drm_property *drm_plane_create_zpos_immutable_property(struct drm_device *dev);
> +
>  /* Helpers */
>  
>  static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
> -- 
> 1.9.2
> 

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] drm: add support for generic zpos property

[Marek Szyprowski]

Hello,

On 2016-01-07 14:59, Daniel Vetter wrote:
> On Tue, Jan 05, 2016 at 01:52:50PM +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
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> lgtm I think. Longer-term we want to think whether we don't want to
> extract such extensions into separate files, and push the kerneldoc into
> an overview DOC: section in there. Just to keep things more closely
> together. Benjamin with drm/sti also needs this, so cc'ing him.

Besides sti and exynos, zpos is also already implemented in rcar, mdp5 
and omap
drivers. I'm not sure what should be done in case of omap, which uses 
this property
with different name ("zorder" instead of "zpos").

>> ---
>>   Documentation/DocBook/gpu.tmpl      | 14 ++++++++--
>>   drivers/gpu/drm/drm_atomic.c        |  4 +++
>>   drivers/gpu/drm/drm_atomic_helper.c | 52 +++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/drm_crtc.c          | 13 ++++++++++
>>   include/drm/drm_atomic_helper.h     |  2 ++
>>   include/drm/drm_crtc.h              | 13 ++++++++++
>>   6 files changed, 96 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
>> index 6c6e81a9eaf4..e81acd999891 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" >a??rotationa??</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" >a??scaling modea??</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
>> +		plane's 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 268d37f26960..de3ca33eb696 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
>> @@ -2781,3 +2782,54 @@ void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>>   	kfree(state);
>>   }
>>   EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state);
>> +
>> +/**
>> + * __drm_atomic_helper_plane_zpos_cmp - compare zpos value of two planes
>> + * @a: pointer to first plane
>> + * @b: pointer to second plane
> Generally we don't do kerneldoc for non-exported functions. If you want
> just keep the text itself as a comment, or better move it into the
> official interface docs for drm_atomic_helper_normalize_zpos().

okay

>> + *
>> + * This function is used for comparing two planes while sorting them to assign
>> + * a normalized zpos values. Planes are compared first by their zpos values,
>> + * then in case they equal, by plane id.
>> + */
>> +static int __drm_atomic_helper_plane_zpos_cmp(const void *a, const void *b)
>> +{
>> +	const struct drm_plane *pa = *(struct drm_plane **)a;
>> +	const struct drm_plane *pb = *(struct drm_plane **)b;
>> +	int zpos_a = 0, zpos_b = 0;
>> +
>> +	if (pa->state)
>> +		zpos_a = pa->state->zpos << 16;
>> +	if (pb->state)
>> +		zpos_b = pb->state->zpos << 16;
>> +
>> +	zpos_a += pa->base.id;
>> +	zpos_b += pb->base.id;
>> +
>> +	return zpos_a - zpos_b;
>> +}
>> +
>> +/**
>> + * drm_atomic_helper_normalize_zpos - calculate normalized zpos values
>> + * @planes: arrays of pointer to planes to consider for normalization
>> + * @count: number of planes in the above array
>> + *
>> + * This function takes arrays of pointers to planes and calculates normalized
>> + * zpos value for them taking into account each planes[i]->state->zpos value
>> + * and plane's id (if zpos equals). The plane[i]->state->normalized_zpos is
>> + * then filled with uniqe values from 0 to count-1.
>> + * Note: a side effect of this function is the fact that the planes array will
>> + * be modified (sorted). It is up to the called to construct planes array with
>> + * all planes that have been assigned to given crtc.
>> + */
>> +void drm_atomic_helper_normalize_zpos(struct drm_plane *planes[], int count)
>> +{
>> +	int i;
>> +
>> +	sort(planes, count, sizeof(struct drm_plane *),
>> +	     __drm_atomic_helper_plane_zpos_cmp, NULL);
>> +	for (i = 0; i < count; i++)
>> +		if (planes[i]->state)
>> +			planes[i]->state->normalized_zpos = i;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_normalize_zpos);
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 62fa95fa5471..51474ea179f6 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -5879,6 +5879,19 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
>>   }
>>   EXPORT_SYMBOL(drm_mode_create_rotation_property);
>>   
> Please add minimal kerneldoc for these two, with the suggestions that
> drivers should use drm_atomic_helper_normalize_zpos(). Also, I'd
> duplicated the semantic definition here (and explain why immutable
> exists) from the gpu.tmpl table.
>
>> +struct drm_property *drm_plane_create_zpos_property(struct drm_device *dev)
>> +{
>> +	return drm_property_create_range(dev, 0, "zpos", 0, 255);
> The zpos property should be stored in dev_priv->mode_config.zpos_prop. We
> need it there so that generic core code (in drm_atomic_plane_set_property)
> can decode it. That way drivers only need to set up&attach it the prop,
> but otherwise can deal with just the decoded values.

Okay.

> Another consideration: Should we reset zpos to something sane in the fbdev
> helpers, like we do for rotation?

Yes and no. fbdev will enable only one (primary) plane per crtc, so zpos 
set to
zero will be properly "normalized" and then interpreted by the driver. 
However
if no fbdev is used and the driver doesn't subclass drm_plane_state, then
drm_atomic_helper_plane_reset() should set it to some sane initial 
values (the
best would be to use initial values passed to 
drm_object_attach_property() on
plane initialization). The only problem is that I didn't find any good 
place to
store the initial zpos value. In case of exynos, drm_plane_state is 
subclassed,
so I can set it in my own exynos_drm_plane_reset() method.

> Thanks, Daniel
>> ...

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] drm: add support for generic zpos property

[Marek Szyprowski]

Hello,

On 2016-01-07 14:59, Daniel Vetter wrote:
> On Tue, Jan 05, 2016 at 01:52:50PM +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
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> lgtm I think. Longer-term we want to think whether we don't want to
> extract such extensions into separate files, and push the kerneldoc into
> an overview DOC: section in there. Just to keep things more closely
> together. Benjamin with drm/sti also needs this, so cc'ing him.

Besides sti and exynos, zpos is also already implemented in rcar, mdp5 
and omap
drivers. I'm not sure what should be done in case of omap, which uses 
this property
with different name ("zorder" instead of "zpos").

>> ---
>>   Documentation/DocBook/gpu.tmpl      | 14 ++++++++--
>>   drivers/gpu/drm/drm_atomic.c        |  4 +++
>>   drivers/gpu/drm/drm_atomic_helper.c | 52 +++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/drm_crtc.c          | 13 ++++++++++
>>   include/drm/drm_atomic_helper.h     |  2 ++
>>   include/drm/drm_crtc.h              | 13 ++++++++++
>>   6 files changed, 96 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
>> index 6c6e81a9eaf4..e81acd999891 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
>> +		plane's 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 268d37f26960..de3ca33eb696 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
>> @@ -2781,3 +2782,54 @@ void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>>   	kfree(state);
>>   }
>>   EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state);
>> +
>> +/**
>> + * __drm_atomic_helper_plane_zpos_cmp - compare zpos value of two planes
>> + * @a: pointer to first plane
>> + * @b: pointer to second plane
> Generally we don't do kerneldoc for non-exported functions. If you want
> just keep the text itself as a comment, or better move it into the
> official interface docs for drm_atomic_helper_normalize_zpos().

okay

>> + *
>> + * This function is used for comparing two planes while sorting them to assign
>> + * a normalized zpos values. Planes are compared first by their zpos values,
>> + * then in case they equal, by plane id.
>> + */
>> +static int __drm_atomic_helper_plane_zpos_cmp(const void *a, const void *b)
>> +{
>> +	const struct drm_plane *pa = *(struct drm_plane **)a;
>> +	const struct drm_plane *pb = *(struct drm_plane **)b;
>> +	int zpos_a = 0, zpos_b = 0;
>> +
>> +	if (pa->state)
>> +		zpos_a = pa->state->zpos << 16;
>> +	if (pb->state)
>> +		zpos_b = pb->state->zpos << 16;
>> +
>> +	zpos_a += pa->base.id;
>> +	zpos_b += pb->base.id;
>> +
>> +	return zpos_a - zpos_b;
>> +}
>> +
>> +/**
>> + * drm_atomic_helper_normalize_zpos - calculate normalized zpos values
>> + * @planes: arrays of pointer to planes to consider for normalization
>> + * @count: number of planes in the above array
>> + *
>> + * This function takes arrays of pointers to planes and calculates normalized
>> + * zpos value for them taking into account each planes[i]->state->zpos value
>> + * and plane's id (if zpos equals). The plane[i]->state->normalized_zpos is
>> + * then filled with uniqe values from 0 to count-1.
>> + * Note: a side effect of this function is the fact that the planes array will
>> + * be modified (sorted). It is up to the called to construct planes array with
>> + * all planes that have been assigned to given crtc.
>> + */
>> +void drm_atomic_helper_normalize_zpos(struct drm_plane *planes[], int count)
>> +{
>> +	int i;
>> +
>> +	sort(planes, count, sizeof(struct drm_plane *),
>> +	     __drm_atomic_helper_plane_zpos_cmp, NULL);
>> +	for (i = 0; i < count; i++)
>> +		if (planes[i]->state)
>> +			planes[i]->state->normalized_zpos = i;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_normalize_zpos);
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 62fa95fa5471..51474ea179f6 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -5879,6 +5879,19 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
>>   }
>>   EXPORT_SYMBOL(drm_mode_create_rotation_property);
>>   
> Please add minimal kerneldoc for these two, with the suggestions that
> drivers should use drm_atomic_helper_normalize_zpos(). Also, I'd
> duplicated the semantic definition here (and explain why immutable
> exists) from the gpu.tmpl table.
>
>> +struct drm_property *drm_plane_create_zpos_property(struct drm_device *dev)
>> +{
>> +	return drm_property_create_range(dev, 0, "zpos", 0, 255);
> The zpos property should be stored in dev_priv->mode_config.zpos_prop. We
> need it there so that generic core code (in drm_atomic_plane_set_property)
> can decode it. That way drivers only need to set up&attach it the prop,
> but otherwise can deal with just the decoded values.

Okay.

> Another consideration: Should we reset zpos to something sane in the fbdev
> helpers, like we do for rotation?

Yes and no. fbdev will enable only one (primary) plane per crtc, so zpos 
set to
zero will be properly "normalized" and then interpreted by the driver. 
However
if no fbdev is used and the driver doesn't subclass drm_plane_state, then
drm_atomic_helper_plane_reset() should set it to some sane initial 
values (the
best would be to use initial values passed to 
drm_object_attach_property() on
plane initialization). The only problem is that I didn't find any good 
place to
store the initial zpos value. In case of exynos, drm_plane_state is 
subclassed,
so I can set it in my own exynos_drm_plane_reset() method.

> Thanks, Daniel
>> ...

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] drm: add support for generic zpos property

[Daniel Vetter]

On Thu, Jan 07, 2016 at 03:33:36PM +0100, Marek Szyprowski wrote:
> Hello,
> 
> On 2016-01-07 14:59, Daniel Vetter wrote:
> >On Tue, Jan 05, 2016 at 01:52:50PM +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
> >>
> >>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >lgtm I think. Longer-term we want to think whether we don't want to
> >extract such extensions into separate files, and push the kerneldoc into
> >an overview DOC: section in there. Just to keep things more closely
> >together. Benjamin with drm/sti also needs this, so cc'ing him.
> 
> Besides sti and exynos, zpos is also already implemented in rcar, mdp5 and
> omap
> drivers. I'm not sure what should be done in case of omap, which uses this
> property
> with different name ("zorder" instead of "zpos").

Argh, it escaped badly already :( Wrt omap I'd just leave it be (atomic
conversion should maybe try to get rid of it though), but for everyone
else it would indeed be nice if the could convert over ... Mostly it
should boil down to removing/replacing code to register the prop, and
looking at the new core one to figure out what to do.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/4] drm/exynos: crtc: rework atomic_{begin,flush}

[Marek Szyprowski]

Some CRTC drivers (like Exynos DRM Mixer) can handle blocking register
updates only on per-device level, not per-plane level. This patch changes
exynos_crts atomic_begin/atomic_flush callbacks to handle the entire crtc,
instead of given planes, so driver can handle both cases on their own.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 14 ++++++++------
 drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 14 ++++++++------
 drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 20 ++++----------------
 drivers/gpu/drm/exynos/exynos_drm_drv.h       | 10 ++++------
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 14 ++++++++------
 5 files changed, 32 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index 77073d8faaa3..1bf6a21130c7 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -248,15 +248,16 @@ static void decon_shadow_protect_win(struct decon_context *ctx, int win,
 		       protect ? ~0 : 0);
 }
 
-static void decon_atomic_begin(struct exynos_drm_crtc *crtc,
-					struct exynos_drm_plane *plane)
+static void decon_atomic_begin(struct exynos_drm_crtc *crtc)
 {
 	struct decon_context *ctx = crtc->ctx;
+	int i;
 
 	if (test_bit(BIT_SUSPENDED, &ctx->flags))
 		return;
 
-	decon_shadow_protect_win(ctx, plane->index, true);
+	for (i = ctx->first_win; i < WINDOWS_NR; i++)
+		decon_shadow_protect_win(ctx, i, true);
 }
 
 #define BIT_VAL(x, e, s) (((x) & ((1 << ((e) - (s) + 1)) - 1)) << (s))
@@ -336,15 +337,16 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
 	decon_set_bits(ctx, DECON_UPDATE, STANDALONE_UPDATE_F, ~0);
 }
 
-static void decon_atomic_flush(struct exynos_drm_crtc *crtc,
-				struct exynos_drm_plane *plane)
+static void decon_atomic_flush(struct exynos_drm_crtc *crtc)
 {
 	struct decon_context *ctx = crtc->ctx;
+	int i;
 
 	if (test_bit(BIT_SUSPENDED, &ctx->flags))
 		return;
 
-	decon_shadow_protect_win(ctx, plane->index, false);
+	for (i = ctx->first_win; i < WINDOWS_NR; i++)
+		decon_shadow_protect_win(ctx, i, false);
 
 	if (ctx->out_type == IFTYPE_I80)
 		set_bit(BIT_WIN_UPDATED, &ctx->flags);
diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index 8911f965b06c..52bda3b42fe0 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -385,15 +385,16 @@ static void decon_shadow_protect_win(struct decon_context *ctx,
 	writel(val, ctx->regs + SHADOWCON);
 }
 
-static void decon_atomic_begin(struct exynos_drm_crtc *crtc,
-					struct exynos_drm_plane *plane)
+static void decon_atomic_begin(struct exynos_drm_crtc *crtc)
 {
 	struct decon_context *ctx = crtc->ctx;
+	int i;
 
 	if (ctx->suspended)
 		return;
 
-	decon_shadow_protect_win(ctx, plane->index, true);
+	for (i = 0; i < WINDOWS_NR; i++)
+		decon_shadow_protect_win(ctx, i, true);
 }
 
 static void decon_update_plane(struct exynos_drm_crtc *crtc,
@@ -517,15 +518,16 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
 	writel(val, ctx->regs + DECON_UPDATE);
 }
 
-static void decon_atomic_flush(struct exynos_drm_crtc *crtc,
-					struct exynos_drm_plane *plane)
+static void decon_atomic_flush(struct exynos_drm_crtc *crtc)
 {
 	struct decon_context *ctx = crtc->ctx;
+	int i;
 
 	if (ctx->suspended)
 		return;
 
-	decon_shadow_protect_win(ctx, plane->index, false);
+	for (i = 0; i < WINDOWS_NR; i++)
+		decon_shadow_protect_win(ctx, i, false);
 }
 
 static void decon_init(struct decon_context *ctx)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index efecd916f797..150a43025861 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -68,32 +68,20 @@ static void exynos_crtc_atomic_begin(struct drm_crtc *crtc,
 				     struct drm_crtc_state *old_crtc_state)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-	struct drm_plane *plane;
 
 	exynos_crtc->event = crtc->state->event;
 
-	drm_atomic_crtc_for_each_plane(plane, crtc) {
-		struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
-
-		if (exynos_crtc->ops->atomic_begin)
-			exynos_crtc->ops->atomic_begin(exynos_crtc,
-							exynos_plane);
-	}
+	if (exynos_crtc->ops->atomic_begin)
+		exynos_crtc->ops->atomic_begin(exynos_crtc);
 }
 
 static void exynos_crtc_atomic_flush(struct drm_crtc *crtc,
 				     struct drm_crtc_state *old_crtc_state)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-	struct drm_plane *plane;
 
-	drm_atomic_crtc_for_each_plane(plane, crtc) {
-		struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
-
-		if (exynos_crtc->ops->atomic_flush)
-			exynos_crtc->ops->atomic_flush(exynos_crtc,
-							exynos_plane);
-	}
+	if (exynos_crtc->ops->atomic_flush)
+		exynos_crtc->ops->atomic_flush(exynos_crtc);
 }
 
 static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index f0827dbebb7d..17b5ded72ff1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -123,8 +123,8 @@ struct exynos_drm_plane_config {
  * @wait_for_vblank: wait for vblank interrupt to make sure that
  *	hardware overlay is updated.
  * @atomic_check: validate state
- * @atomic_begin: prepare a window to receive a update
- * @atomic_flush: mark the end of a window update
+ * @atomic_begin: prepare device to receive an update
+ * @atomic_flush: mark the end of device update
  * @update_plane: apply hardware specific overlay data to registers.
  * @disable_plane: disable hardware specific overlay.
  * @te_handler: trigger to transfer video image at the tearing effect
@@ -144,14 +144,12 @@ struct exynos_drm_crtc_ops {
 	void (*wait_for_vblank)(struct exynos_drm_crtc *crtc);
 	int (*atomic_check)(struct exynos_drm_crtc *crtc,
 			    struct drm_crtc_state *state);
-	void (*atomic_begin)(struct exynos_drm_crtc *crtc,
-			      struct exynos_drm_plane *plane);
+	void (*atomic_begin)(struct exynos_drm_crtc *crtc);
 	void (*update_plane)(struct exynos_drm_crtc *crtc,
 			     struct exynos_drm_plane *plane);
 	void (*disable_plane)(struct exynos_drm_crtc *crtc,
 			      struct exynos_drm_plane *plane);
-	void (*atomic_flush)(struct exynos_drm_crtc *crtc,
-			      struct exynos_drm_plane *plane);
+	void (*atomic_flush)(struct exynos_drm_crtc *crtc);
 	void (*te_handler)(struct exynos_drm_crtc *crtc);
 	void (*clock_enable)(struct exynos_drm_crtc *crtc, bool enable);
 };
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 6ae1b1e55783..70194d0e4fe4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -622,26 +622,28 @@ static void fimd_shadow_protect_win(struct fimd_context *ctx,
 	writel(val, ctx->regs + reg);
 }
 
-static void fimd_atomic_begin(struct exynos_drm_crtc *crtc,
-			       struct exynos_drm_plane *plane)
+static void fimd_atomic_begin(struct exynos_drm_crtc *crtc)
 {
 	struct fimd_context *ctx = crtc->ctx;
+	int i;
 
 	if (ctx->suspended)
 		return;
 
-	fimd_shadow_protect_win(ctx, plane->index, true);
+	for (i = 0; i < WINDOWS_NR; i++)
+		fimd_shadow_protect_win(ctx, i, true);
 }
 
-static void fimd_atomic_flush(struct exynos_drm_crtc *crtc,
-			       struct exynos_drm_plane *plane)
+static void fimd_atomic_flush(struct exynos_drm_crtc *crtc)
 {
 	struct fimd_context *ctx = crtc->ctx;
+	int i;
 
 	if (ctx->suspended)
 		return;
 
-	fimd_shadow_protect_win(ctx, plane->index, false);
+	for (i = 0; i < WINDOWS_NR; i++)
+		fimd_shadow_protect_win(ctx, i, false);
 }
 
 static void fimd_update_plane(struct exynos_drm_crtc *crtc,
-- 
1.9.2

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

[PATCH 3/4] drm/exynos: mixer: properly update all planes on the same vblank event

[Marek Szyprowski]

This patch also moves mixer_vsync_set_update() to newly introduced
mixer_atomic_begin/flush callbacks. This ensures that all mixer planes
will be updated on the same vsync event.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index bf148dc3623c..b5fbc1cbf024 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -516,7 +516,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
 	}
 
 	spin_lock_irqsave(&res->reg_slock, flags);
-	mixer_vsync_set_update(ctx, false);
 
 	/* interlace or progressive scan mode */
 	val = (ctx->interlace ? ~0 : 0);
@@ -567,7 +566,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
 	mixer_cfg_vp_blend(ctx);
 	mixer_run(ctx);
 
-	mixer_vsync_set_update(ctx, true);
 	spin_unlock_irqrestore(&res->reg_slock, flags);
 
 	mixer_regs_dump(ctx);
@@ -642,7 +640,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 		ctx->interlace = false;
 
 	spin_lock_irqsave(&res->reg_slock, flags);
-	mixer_vsync_set_update(ctx, false);
 
 	/* setup format */
 	mixer_reg_writemask(res, MXR_GRAPHIC_CFG(win),
@@ -691,7 +688,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 
 	mixer_run(ctx);
 
-	mixer_vsync_set_update(ctx, true);
 	spin_unlock_irqrestore(&res->reg_slock, flags);
 
 	mixer_regs_dump(ctx);
@@ -718,7 +714,6 @@ static void mixer_win_reset(struct mixer_context *ctx)
 	unsigned long flags;
 
 	spin_lock_irqsave(&res->reg_slock, flags);
-	mixer_vsync_set_update(ctx, false);
 
 	mixer_reg_writemask(res, MXR_CFG, MXR_CFG_DST_HDMI, MXR_CFG_DST_MASK);
 
@@ -749,7 +744,6 @@ static void mixer_win_reset(struct mixer_context *ctx)
 	if (ctx->vp_enabled)
 		mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_VP_ENABLE);
 
-	mixer_vsync_set_update(ctx, true);
 	spin_unlock_irqrestore(&res->reg_slock, flags);
 }
 
@@ -980,6 +974,16 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
 	mixer_reg_writemask(res, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
 }
 
+static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
+{
+	struct mixer_context *mixer_ctx = crtc->ctx;
+
+	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
+		return;
+
+	mixer_vsync_set_update(mixer_ctx, false);
+}
+
 static void mixer_update_plane(struct exynos_drm_crtc *crtc,
 			       struct exynos_drm_plane *plane)
 {
@@ -1009,12 +1013,18 @@ static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
 		return;
 
 	spin_lock_irqsave(&res->reg_slock, flags);
-	mixer_vsync_set_update(mixer_ctx, false);
-
 	mixer_cfg_layer(mixer_ctx, plane->index, 0, false);
+	spin_unlock_irqrestore(&res->reg_slock, flags);
+}
+
+static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
+{
+	struct mixer_context *mixer_ctx = crtc->ctx;
+
+	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
+		return;
 
 	mixer_vsync_set_update(mixer_ctx, true);
-	spin_unlock_irqrestore(&res->reg_slock, flags);
 }
 
 static void mixer_wait_for_vblank(struct exynos_drm_crtc *crtc)
@@ -1055,6 +1065,8 @@ static void mixer_enable(struct exynos_drm_crtc *crtc)
 
 	pm_runtime_get_sync(ctx->dev);
 
+	mixer_vsync_set_update(ctx, false);
+
 	mixer_reg_writemask(res, MXR_STATUS, ~0, MXR_STATUS_SOFT_RESET);
 
 	if (test_bit(MXR_BIT_VSYNC, &ctx->flags)) {
@@ -1063,6 +1075,8 @@ static void mixer_enable(struct exynos_drm_crtc *crtc)
 	}
 	mixer_win_reset(ctx);
 
+	mixer_vsync_set_update(ctx, true);
+
 	set_bit(MXR_BIT_POWERED, &ctx->flags);
 }
 
@@ -1113,8 +1127,10 @@ static const struct exynos_drm_crtc_ops mixer_crtc_ops = {
 	.enable_vblank		= mixer_enable_vblank,
 	.disable_vblank		= mixer_disable_vblank,
 	.wait_for_vblank	= mixer_wait_for_vblank,
+	.atomic_begin		= mixer_atomic_begin,
 	.update_plane		= mixer_update_plane,
 	.disable_plane		= mixer_disable_plane,
+	.atomic_flush		= mixer_atomic_flush,
 	.atomic_check		= mixer_atomic_check,
 };
 
-- 
1.9.2

[PATCH 4/4] 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   |  1 -
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 68 ++++++++-----------------------
 drivers/gpu/drm/exynos/exynos_mixer.c     | 19 +++++++--
 3 files changed, 34 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 17b5ded72ff1..244ae6c4482c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -217,7 +217,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..4afa5dfda9e0 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,27 @@ 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)
+	if (immutable) {
+		if (!dev->mode_config.zpos_immutable_property)
+			dev->mode_config.zpos_immutable_property =
+				drm_plane_create_zpos_immutable_property(dev);
+		if (!dev->mode_config.zpos_immutable_property)
+			return;
+
+		prop = dev->mode_config.zpos_immutable_property;
+	} else {
+		if (!dev->mode_config.zpos_property)
+			dev->mode_config.zpos_property =
+					drm_plane_create_zpos_property(dev);
+		if (!dev->mode_config.zpos_property)
 			return;
 
-		dev_priv->plane_zpos_property = prop;
+		prop = dev->mode_config.zpos_property;
 	}
 
 	drm_object_attach_property(&plane->base, prop, zpos);
@@ -344,7 +311,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..6ab68eca0004 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -34,6 +34,7 @@
 #include <linux/component.h>
 
 #include <drm/exynos_drm.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "exynos_drm_drv.h"
 #include "exynos_drm_crtc.h"
@@ -95,6 +96,8 @@ struct mixer_context {
 	struct drm_device	*drm_dev;
 	struct exynos_drm_crtc	*crtc;
 	struct exynos_drm_plane	planes[MIXER_WIN_NR];
+	struct drm_plane	*zpos_planes[MIXER_WIN_NR];
+	int			num_planes;
 	int			pipe;
 	unsigned long		flags;
 	bool			interlace;
@@ -478,6 +481,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 +566,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 +591,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 +683,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 */
@@ -981,6 +986,12 @@ static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
 	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
 		return;
 
+	/*
+	 * Normalizing up to 3 planes is cheap, no need for checking
+	 * if zpos has been really changed.
+	 */
+	drm_atomic_helper_normalize_zpos(mixer_ctx->zpos_planes,
+					 mixer_ctx->num_planes);
 	mixer_vsync_set_update(mixer_ctx, false);
 }
 
@@ -1203,13 +1214,15 @@ static int mixer_bind(struct device *dev, struct device *manager, void *data)
 
 	for (i = 0; i < MIXER_WIN_NR; i++) {
 		if (i == VP_DEFAULT_WIN && !ctx->vp_enabled)
-			continue;
+			break;
 
 		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
 					1 << ctx->pipe, &plane_configs[i]);
 		if (ret)
 			return ret;
+		ctx->zpos_planes[i] = &ctx->planes[i].base;
 	}
+	ctx->num_planes = i;
 
 	exynos_plane = &ctx->planes[DEFAULT_WIN];
 	ctx->crtc = exynos_drm_crtc_create(drm_dev, &exynos_plane->base,
-- 
1.9.2

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

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

[Inki Dae]

Hi Marek,


2016년 01월 05일 21:52에 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 2/4 and 3/4 are fixes for Exynos DRM driver, which are required

Picked them up. Especially, 2/4 is a good patch.

Thanks,
Inki Dae

> to properly implement generic zpos handling. However they can be also
> merged regardless of the rest of the patches (if there is a need for
> further discussion on the generic zpos property feature).
> 
> Patches are based on top of latest exynos-drm-next branch.
> 
> Best regards
> Marek Szyprowski
> Samsung R&D Institute Poland
> 
> 
> Patch summary:
> 
> Marek Szyprowski (4):
>   drm: add generic zpos property
>   drm/exynos: crtc: rework atomic_{begin,flush}
>   drm/exynos: mixer: properly update all planes on the same vblank event
>   drm/exynos: use generic code for managing zpos plane property
> 
>  Documentation/DocBook/gpu.tmpl                | 14 +++++-
>  drivers/gpu/drm/drm_atomic.c                  |  4 ++
>  drivers/gpu/drm/drm_atomic_helper.c           | 52 ++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c                    | 13 +++++
>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 14 +++---
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 14 +++---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 20 ++------
>  drivers/gpu/drm/exynos/exynos_drm_drv.h       | 11 ++---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 14 +++---
>  drivers/gpu/drm/exynos/exynos_drm_plane.c     | 68 +++++++--------------------
>  drivers/gpu/drm/exynos/exynos_mixer.c         | 53 ++++++++++++++++-----
>  include/drm/drm_atomic_helper.h               |  2 +
>  include/drm/drm_crtc.h                        | 13 +++++
>  13 files changed, 187 insertions(+), 105 deletions(-)
>