[PATCH] linux-dmabuf: un-deprecate format events

[Chia-I Wu]

On Thu, Apr 11, 2019 at 7:12 AM Simon Ser <contact at emersion.fr> wrote:

> On Thursday, April 11, 2019 11:48 AM, Pekka Paalanen <ppaalanen at gmail.com>
> wrote:
> > Hi,
> >
> > at first hand, I would keep Weston's behaviour, because that is what
> > the current protocol specification says exactly. The specification text
> > is very clear on the intent: the old format event is not to be used, if
> > both the client and server support the version with the modifier event.
> > When the protocol will be stabilized (which is always an ABI break),
> > the format event without a modifier will be removed.
>
v3 does not consider implicit modifiers as it seems (and Weston uses the
DRM_FORMAT_MOD_INVALID hack).  I don't think this is a good justification.

>
> > The difference to EGL API does not seem to be making anything more
> > difficult either.
> >
> > Technically there is little difference between using two events with
> > and without a modifier, and one event with a special no-modifier value.
> > A technical reason to favour two different events would be bandwidth
> > saving, since then one doesn't need to send the dummy modifier. Can we
> > make that argument? If yes, that would be a good justification to make
> > the change you propose.
>
> Do we have a lot of these? My machines don't have any (i915 driver).
>
No, bandwidth should not be a concern.

>
> But anyway, I thought that DRM_FORMAT_MOD_INVALID was being phased out?
>
Kernel never returns it and never accepts it.  It has a separate flag
(DRM_MODE_FB_MODIFIERS) to indicate if the modifier is explicit or
implicit, and DRM_FORMAT_MOD_INVALID is not a valid explicit modifier.


>
> > At first read of your commit message, I got the feeling you want
> > formats to be advertised by both events: format event to declare a
> > format at all, plus modifier events to list any modifiers. That is also
> > what your proposed spec text reads, so I'd recommend clarifying that
> > implementations need to send only one of the two events per format. Or
> > both if the same format can be used with and without a modifier?
>
Your interpretation is correct: format events are for formats, and modifier
events are for modifiers.

This is more intuitive comparing to Weston's.

>
> > If we have two events, one without and one with a modifier, should we
> > also then have two (well, four actually, for the immed variant)
> > requests to create a buffer: one without and one with a modifier? I
> > think it would be nice to have these consistent unless there is a
> > reason not to.
> >
> > I can find justification both ways, and whether we need to fix Weston
> > or Mutter is not that important to me. I might want to avoid the four
> > create requests case though.
>
> I agree that either way, some compositors will need to be fixed, and that
> we
> shouldn't decide based on current implementations.

At the advertising phase, supported formats and modifiers are unambiguously
advertised.

At the creation phase, you are right that it is hard to express the intent
to create a buffer with implicit modifier.  I think it is best to add a new
bit to flags, to express the intent unambiguously.




> > I'm not sure the versioning fully works out, if we have:
> > - version 1 and 2 have only 'format' event
> > - version 3 has 'modifier' event, with 'format' event not to be used
> > - version 4 wants to use both 'modifier' and 'format' events
>
> Yeah, that would be a little bit annoying.


> > Implementation would need to adhere to these, so Mutter would still
> > need fixing for clients that bind version 3. I don't think we should
> > retro-actively change the semantics of an existing version.
>
> If the protocol isn't ambiguous, it would be dangerous to change semantics
> indeed.
>
When it comes to implementations, my view is

 v1/v2: `format' event
 v3: add `modifier' event and `format' event becomes optional (i.e.,
clients ignores it)
 v4: `format' should not be optional

An implementation can support all versions by always sending `format'
events, and only send modifier events when version >= 3.  A client can pick
any version that suits it.


> > I notice that currently the modifier is in
> > zwp_linux_buffer_params_v1.add request which makes the modifier
> > per-plane, but I seem to recall that in practise modifiers are nowadays
> > used only per-buffer, not per-plane, so the modifier argument should
> > move to the create and create_immed requests.
>
> You're right, modifiers are now per-buffer and not per-plane.
>
This diverges from what EGL and kernel do.  If we are sure it will be fine
down the road, I am fine with it.

But it also makes implementations more complex to support both older and
new versions.  This was brought up above.  So I think it is also a concern?


>
> > We also need to consider https://patchwork.freedesktop.org/patch/263061/
> .
> >
> > I would propose the following roadmap:
> >
> > 1. Do not un-deprecate format events; instead clarify the modifier on
> >    create and fix Mutter's behaviour.
> >
> > 2. Get https://patchwork.freedesktop.org/patch/263061/ merged.
> >
> > 3. Stop using wl_drm completely when both server and client support the
> >    latest zwp_linux_dmabuf.
> >
> > 4. Do ABI-breaking cleanups, and bump the major or if we are sure
> >    enough then declare the extension stable.
> >
> > The reason why I would throw the 'format' event away as it is is that
> > then it will be consistent with the create requests: use
> > DRM_FORMAT_MOD_INVALID for "no modifier" in both events and requests. I

I think going the kernel way is also justified: DRM_FORMAT_MOD_INVALID is
invalid in both events and requests.  The `flags' parameter in requests
decides if the modifier is explicit or implicit.  We tried to use
DRM_FORMAT_MOD_INVALID wisely, in Mutter or in Weston.  But it led to
confusions.

> would assume that the bandwidth savings would not be significant,
> > considering everything is aiming to have explicit modifiers.
> >
> > How would that sound?
>
> This makes a lot of sense to me. +1.
>
I defended Mutter's behavior here, but I am on the fence.  To me, it boils
down to, should we use DRM_FORMAT_MOD_INVALID cleverly?  And this is what
kernel says about the modifier

/*
 * Invalid Modifier
 *
 * This modifier can be used as a sentinel to terminate the format modifiers
 * list, or to initialize a variable with an invalid modifier. It might
also be
 * used to report an error back to userspace for certain APIs.
 */
#define DRM_FORMAT_MOD_INVALID fourcc_mod_code(NONE, DRM_FORMAT_RESERVED)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190411/d2ff3b30/attachment.html>