[PATCH v4 1/4] drm: add generic zpos property
2016-06-22 1:45 GMT+02:00 Laurent Pinchart :
> Hi Benjamin,
>
> Thank you for the patch.
>
> Could you please reply to Ville's comment to v1 of this patch (posted on April
> the 25th) ?
>
For each iteration of the patches we have swap between two solutions:
- have normalization done between 0 to N-1 without taking care of zpos
property range
- take care of zpos property range while computing normalized zpos
First the solution is the most simple and the more robust so I will
definitively go for it.
[snip]
>> + sort(states, n, sizeof(*states), drm_atomic_state_zpos_cmp, NULL);
>> +
>> + for (i = 0, zpos = 0; i < n; i++, zpos++) {
>> + plane = states[i]->plane;
>> +
>> + zpos = max_t(int, zpos, states[i]->zpos);
>> +
>> + WARN_ON(zpos > plane->zpos_property->values[1]);
>
> This crashes if the plane doesn't have a zpos property. The simplest fix is to
> write the check as
>
> WARN_ON(plane->zpos_property &&
> zpos > plane->zpos_property->values[1]);
Thanks I will add that in v5
> but I wonder how we should handle drivers that instantiate a zpos property for
> a subdev of the planes only. For drivers that don't use zpos at all you could
> maybe avoid calling this function.
>
I think it will be difficult to mix planes with and without zpos property.
My suggestion is to use immutable zpos property for primary and cursor plane.
drm_atomic_helper_crtc_normalize_zpos() is only called if plane zpos
value change
so if driver don't use it normalization will not be done.
> Additionally, this check is triggered with the rcar-du-drm driver when
> performing the following operations:
>
> - Perform a mode set with the CRTC primary plane only (that plane doesn't have
> a zpos property)
>
> - Add 7 overlay planes with zpos values 1 to 7 (their zpos property range is
> 1-7)
>
> - Modify the zpos value of all overlay planes one by one to 7 to 1 (setting
> zpos 7 for plane 1 first, then zpos 6 for plane 2, ...)
>
> I get normalized zpos values such as
>
> [ 84.892927] [PLANE:39:plane-8] normalized zpos value 9
> [ 85.899284] [PLANE:25:plane-0] normalized zpos value 0
> [ 85.904488] [PLANE:37:plane-7] normalized zpos value 2
> [ 85.909633] [PLANE:35:plane-6] normalized zpos value 3
> [ 85.914793] [PLANE:33:plane-5] normalized zpos value 4
> [ 85.919936] [PLANE:31:plane-4] normalized zpos value 5
> [ 85.925100] [PLANE:29:plane-3] normalized zpos value 6
> [ 85.930245] [PLANE:27:plane-2] normalized zpos value 7
>
> (plane 25 is the primary plane, all other planes are the overlay planes, added
> in the order 27, 29, 31, 33, 35, 37, 37 in the sequence above)
>
I'm not able to reproduce you problem on my setup with one primary and
two overlay.
When back to 0 to N-1 normalization algo you should see the problem anymore
[snip]
> --
> Regards,
>
> Laurent Pinchart
>
--
Benjamin Gaignard
Graphic Working Group
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
[PATCH v4 1/4] drm: add generic zpos property
Hi Benjamin,
Thank you for the patch.
Could you please reply to Ville's comment to v1 of this patch (posted on April
the 25th) ?
Please also see below for additional comments.
On Monday 13 Jun 2016 11:21:23 Benjamin Gaignard wrote:
> version 4:
> - make sure that normalized zpos value is stay
> in the defined property range and warn user if not
>
> This patch adds support for generic plane's zpos property property with
> well-defined semantics:
> - added zpos properties to plane 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
>
> Compare to Marek's original patch zpos property is now specific to each
> plane and no more to the core.
> Normalize function take care of the range of per plane defined range
> before set normalized_zpos.
>
> Signed-off-by: Benjamin Gaignard
>
> Cc: Inki Dae
> Cc: Daniel Vetter
> Cc: Ville Syrjala
> Cc: Joonyoung Shim
> Cc: Seung-Woo Kim
> Cc: Andrzej Hajda
> Cc: Krzysztof Kozlowski
> Cc: Bartlomiej Zolnierkiewicz
> Cc: Tobias Jakobi
> Cc: Gustavo Padovan
> Cc: vincent.abriou at st.com
> Cc: fabien.dessenne at st.com
> Cc: Laurent Pinchart
> ---
> Documentation/DocBook/gpu.tmpl | 10 ++
> drivers/gpu/drm/Makefile| 2 +-
> drivers/gpu/drm/drm_atomic.c| 4 +
> drivers/gpu/drm/drm_atomic_helper.c | 6 +
> drivers/gpu/drm/drm_blend.c | 230 +
> drivers/gpu/drm/drm_crtc_internal.h | 3 +
> include/drm/drm_crtc.h | 30 +
> 7 files changed, 284 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/drm_blend.c
[snip]
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> new file mode 100644
> index 000..9a5361a
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -0,0 +1,230 @@
[snip]
> +/**
> + * 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
> unique
> + * 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, zpos, n = 0;
> + int ret = 0;
> +
> + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] calculating normalized zpos values\n",
> + crtc->base.id, crtc->name);
> +
> + states = kmalloc_array(total_planes, sizeof(*states), GFP_TEMPORARY);
> + 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 done;
> + }
> + 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, zpos = 0; i < n; i++, zpos++) {
> + plane = states[i]->plane;
> +
> + zpos = max_t(int, zpos, states[i]->zpos);
> +
> + WARN_ON(zpos > plane->zpos_property->values[1]);
This crashes if the plane doesn't have a zpos property. The simplest fix is to
write the check as
WARN_ON(plane->zpos_property &&
zpos > plane->zpos_property->values[1]);
but I wonder how we should handle drivers that instantiate a zpos property for
a subdev of the planes
[PATCH v4 1/4] drm: add generic zpos property
version 4:
- make sure that normalized zpos value is stay
in the defined property range and warn user if not
This patch adds support for generic plane's zpos property property with
well-defined semantics:
- added zpos properties to plane 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
Compare to Marek's original patch zpos property is now specific to each
plane and no more to the core.
Normalize function take care of the range of per plane defined range
before set normalized_zpos.
Signed-off-by: Benjamin Gaignard
Cc: Inki Dae
Cc: Daniel Vetter
Cc: Ville Syrjala
Cc: Joonyoung Shim
Cc: Seung-Woo Kim
Cc: Andrzej Hajda
Cc: Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz
Cc: Tobias Jakobi
Cc: Gustavo Padovan
Cc: vincent.abriou at st.com
Cc: fabien.dessenne at st.com
Cc: Laurent Pinchart
---
Documentation/DocBook/gpu.tmpl | 10 ++
drivers/gpu/drm/Makefile| 2 +-
drivers/gpu/drm/drm_atomic.c| 4 +
drivers/gpu/drm/drm_atomic_helper.c | 6 +
drivers/gpu/drm/drm_blend.c | 230
drivers/gpu/drm/drm_crtc_internal.h | 3 +
include/drm/drm_crtc.h | 30 +
7 files changed, 284 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/drm_blend.c
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index dac18b4..8baac27 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -2146,6 +2146,16 @@ void intel_crt_init(struct drm_device *dev)
the underlying hardware).
+"zpos"
+ RANGE
+ Min= driver dependent, Max= driver dependent
+ Plane
+ Plane's 'z' position during blending operation (0 for
background, highest 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. Exact value range is driver dependent.
+
+
i915
Generic
"Broadcast RGB"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index be43afb..dbec0ea 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -2,7 +2,7 @@
# Makefile for the drm device driver. This driver provides support for the
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
-drm-y := drm_auth.o drm_bufs.o drm_cache.o \
+drm-y := drm_auth.o drm_bufs.o drm_blend.o drm_cache.o \
drm_context.o drm_dma.o \
drm_fops.o drm_gem.o drm_ioctl.o drm_irq.o \
drm_lock.o drm_memory.o drm_drv.o drm_vm.o \
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 5e4b820..812b0f0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -690,6 +690,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 == plane->zpos_property) {
+ state->zpos = val;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -746,6 +748,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 == plane->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 bb98d74..9fbe08c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -32,6 +32,8 @@
#include
#include
+#include "drm_crtc_internal.h"
+
/**
* DOC: overview
*
@@ -592,6 +594,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
