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 m.szyprowski@samsung.com
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 benjamin.gaignard@linaro.org
Cc: Inki Dae inki.dae@samsung.com Cc: Daniel Vetter daniel@ffwll.ch Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Joonyoung Shim jy0922.shim@samsung.com Cc: Seung-Woo Kim sw0312.kim@samsung.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Krzysztof Kozlowski k.kozlowski@samsung.com Cc: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com Cc: Tobias Jakobi tjakobi@math.uni-bielefeld.de Cc: Gustavo Padovan gustavo@padovan.org Cc: vincent.abriou@st.com Cc: fabien.dessenne@st.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com
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 0000000..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 only. For drivers that don't use zpos at all you could maybe avoid calling this function.
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)
states[i]->normalized_zpos = zpos;DRM_DEBUG_ATOMIC("[PLANE:%d:%s] normalized zpos value %d\n",plane->base.id, plane->name, zpos);- }
- crtc_state->zpos_changed = true;
+done:
- kfree(states);
- return ret;
+} +EXPORT_SYMBOL(drm_atomic_helper_crtc_normalize_zpos);
+/**
- drm_atomic_helper_normalize_zpos - calculate normalized zpos values for
all + * crtcs
- @dev: DRM device
- @state: atomic state of DRM device
- This function calculates normalized zpos value for all modified planes
in + * the provided atomic state of DRM device. For more information, see +
- drm_atomic_helper_crtc_normalize_zpos() function.
- RETURNS
- Zero for success or -errno
- */
+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;
- 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;
+} +EXPORT_SYMBOL(drm_atomic_helper_normalize_zpos);