On Tue, May 24, 2016 at 05:01:02PM +0300, Laurent Pinchart wrote:
> Hi Ville,
>
> On Thursday 12 May 2016 23:54:09 Ville Syrjälä wrote:
> > On Thu, May 12, 2016 at 11:15:42PM +0300, Laurent Pinchart wrote:
> > > On Thursday 12 May 2016 22:46:05 Ville Syrjälä wrote:
> > >> On Thu, May 12, 2016 at 10:20:12PM +0300, Laurent Pinchart wrote:
> > >>> 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@xxxxxxxxxxx>
> > >>>>>
> > >>>>> 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@xxxxxxxxxxx>
> > >>>>>
> > >>>>> 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@xxxxxxxxxx>
> > >>>>>
> > >>>>> Cc: Inki Dae <inki.dae@xxxxxxxxxxx>
> > >>>>> Cc: Daniel Vetter <daniel@xxxxxxxx>
> > >>>>> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > >>>>> Cc: Joonyoung Shim <jy0922.shim@xxxxxxxxxxx>
> > >>>>> Cc: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>
> > >>>>> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > >>>>> Cc: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
> > >>>>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> > >>>>> Cc: Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>
> > >>>>> Cc: Gustavo Padovan <gustavo@xxxxxxxxxxx>
> > >>>>> Cc: vincent.abriou@xxxxxx
> > >>>>> Cc: fabien.dessenne@xxxxxx
> > >>>>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > >>>>> ---
> > >>>>>
> > >>>>> 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.
> > >>
> > >> It just eliminates duplicate zpos values. It won't re-sort your plane
> > >> states magically or anything like that. Nor does it guarantee that
> > >> the zpos values for the actually visible planes are even contiguous.
> > >
> > > To make normalization useful in my case I need contiguous values starting
> > > at 0.
> >
> > Contigious values for all the planes on the whole device or on one crtc?
> > And what about the visible vs. not plane issue? Is it OK to have an
> > invisible plane somewhere in the middle of the zpos sequence?
>
> In my case it would be per-CRTC, and invisible planes in the middle wouldn't
> be an issue.
>
> To make normalization useful it needs to compute values that are usable by the
> majority of drivers. The question then becomes what should those values be ?
> Generally speaking they should be easy to convert to driver-specific values.
> That's why I believe we need feedback from driver developers on this topic.
>
> > >>> 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.
> > >
> > > Any opinion on this ?
> >
> > Not really. I already asked if we had a good use case for it. Dunno if
> > anyone has one.
>
> Non-atomic userspace needing to swap planes is such a use case. The question
> is whether we want to support it.
I was more asking if anyone is actually doing that, and do they need it
to keep working.
>
> For atomic, the reason I can see to support the same feature is to make it
> easier for userspace to modify the zorder of a plane without requiring updates
> to all other planes. I don't have a strong opinion on that, but we need to
> decide whether we want to support it or not.
Not sure it really helps in any measurable way. It might save a few
properties from getting pushed to the kernel, but that's all. Eg. if
userspace would maintain some kind of stack of planes in the right
zorder, then pushing a new plane into the middle of the stack would
required that it figures out the right zorder for that position.
That would be something like z=clamp(below_z + 1, below_z, above_z).
After that it would have to figure out if the magic conflict resolution
will result in the right zorder should below_z==z or above_z==z,
and if not it would have to change below_z and/or above_z too.
That process could ripple all the way through the entire stack.
Seems to me that it would be far easier to just readjust the z
order for all the planes above from the get go.
>
> > >>> 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 ?
> > >>
> > >> The point of exposing ranges is to give userspace at least some chance
> > >> of getting the order right. Otherwise it'll just have to randomly try
> > >> different orders until something works. Most hardware is quite
> > >> constrained in this.
> > >
> > > Could you educate me by giving a few real world examples ?
> >
> > Well, on Intel we have the simple case on many platforms (bottom->top):
> > primary->sprite->cursor
> > Most PC hardware from some years ago was similar. More modern stuff from
> > the other vendors might be more capable, not sure.
> >
> > We also have another class of Intel hardware which has primary, two
> > sprites, and a cursor. The cursor is always on top, the other three
> > planes can be in any order amongst themselves.
> >
> > The oldest Intel hardware we support is even more flexible. Those had
> > sort of 1 proper primary plane, 2 sprite planes, 1 video overlay, and
> > 2 cursor planes. Normally they're spread among two crtcs, but they
> > can all be moved to a single crtc and then ordering constraints between
> > them are fairly convoluted.
>
> Thank you for the information.
>
> > As far as I can remember OMAP (at least 3 and maybe 4) had a totally
> > fixed order. GFX->VID1->VID2. Perhaps they changed this later, dunno.
>
> OMAP3 has a fixed Z-order, while OMAP4 seems to be configurable according to
> the omapdrm driver.
>
> > One extra complication in this is destination color keying. What that
> > tends to do effectively is push some sprite/overlay plane below the
> > primary and punch a hole into the primary, but otherwise the
> > sprite/overlay sits above the primary. Not sure if we should make this
> > sort of thing explicit in the zpos API or if we should just declare
> > color keying somehow magic and potentially ignore some of the zpos
> > properties.
>
> Color-keying is a quite common feature, I think we should standardize
> properties to support it, which would then require defining the behaviour,
> including how it interacts with Z-order.
>
> > >> Some don't support changing the zpos at all, in which case we would set
> > >> min==max.
> > >
> > > But all this doesn't require adjusting the normalized zpos to take the
> > > original zpos constraints into account, does it ? Why can normalized_zpos
>
> s/can/can't/
>
> > > be constrained to the range 0 to N-1, giving drivers the sorted planes
> > > and then letting them convert that to device-specific values ?
> >
> > Oh, I didn't even notice that it was mixing up the normalized and
> > non-normalized coordinates. Yes, that does seem wrong to me. The fact that
> > the ioctl would have rejected the out of range non-normalized coordinates
> > should already guarantee that the resulting normalized coordinates end
> > up in roughly the right order. That is, at least the order between different
> > "zpos groups" of planes would be already respected. Eg. in the Intel "3
> > freely ordered planes + cursor" case there would be no way to push the
> > cursor below any other plane.
>
> I agree with you, and I still think that computing the zpos_normalized values
> in the 0 to N-1 range (N being the number of visible planes for the CRTC)
> would be the best option.
You can't actually know what's visible when this gets called.
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
