Hi Ville,
On Thursday 12 May 2016 14:51:55 Ville Syrjälä wrote:
On Thu, May 12, 2016 at 12:28:19PM +0200, Benjamin Gaignard wrote:
From: Marek Szyprowski m.szyprowski@samsung.com
This patch adds support for generic plane's zpos property property with well-defined semantics:
- added zpos properties to 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 | 229 +++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_crtc_internal.h | 3 + include/drm/drm_crtc.h | 28 +++++ 7 files changed, 281 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..eef59bb --- /dev/null +++ b/drivers/gpu/drm/drm_blend.c @@ -0,0 +1,229 @@
[snip]
+static int drm_atomic_state_zpos_cmp(const void *a, const void *b) +{
- const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
- const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
- if (sa->zpos != sb->zpos)
return sa->zpos - sb->zpos;- else
return sa->plane->base.id - sb->plane->base.id;+}
Still not really convinced this normalization is a good idea when we have atomic. It made sense before atomic since otherwise you might not be able to swap the zpos of two planes without shutting one of them down, but it makes interpreting the zpos prop values more confusing. And with atomic we don't actually need it AFAICS. Or do we have an actual use case for this behaviour?
Speaking selfishly about my use case (rcar-du-drm), I need to sort planes in display order from 0 to N-1 as that's how the hardware is programmed. Normalization gives me an array of sorted planes, which is exactly what I need. I expect other drivers to have similar requirements, so normalization in the core will allow code sharing.
Now, is your concern that allowing two planes to have the same zpos isn't needed with the atomic API ? I agree with that, but it would also make it impossible to swap two planes using the legacy API implemented on top of atomic drivers. I'll let others decide whether this is considered as a concern.
While at it, Benjamin told me that you've requested configurable min/max values per plane, what are your use cases for that ? On the same topic, please see below.
+/**
- 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(total_planes * sizeof(*states), GFP_KERNEL);
kmalloc_array(..., 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;}Hmm. So if we change the zpos for any plane, we get to re-check every plane. Not the most efficient thing perhaps, since on some hardware you might only have a subset of planes that can even change their zpos. OTOH I suppose it does make life a bit simpler when you don't have to think which planes might be affected, so I guess it's fine.
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);
The purpose of this code is to support the per-plane min/max zpos values. As Benjamin explained, for example if the zpos range is [0,2] for plane X and [4,6] for plane Y, we need to be sure the min zpos value of plane Y is 4. To this I replied that's exactly what I'd rather avoid. I think the core code should take care of sorting planes, taking driver-specific constraints into account when possible (too exotic constraints can always be implemented directly by drivers), and then leave to the drivers the responsibility of taking the sorted planes array and computing whatever hardware-specific configuration is needed. Sorting planes from 0 to N-1 seems to be like a better API than having semi-random values for the normalized zpos. Do we have use cases that wouldn't be covered by a simpler implementation ?
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);