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

Fri Apr 12 09:16:49 UTC 2019

On Thu, 11 Apr 2019 15:33:06 -0700
Chia-I Wu <olvaffe at gmail.com> wrote:

> 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:  

> > > 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.

Hi,

I disagree. Your design raises the question: what does it mean if there
is a 'modifier' event with a format that was not in a 'format' event?

If you want to forbid that, you need to write it down.

It also makes the 'format' event completely redundant if there is a
'modifier' event for the same format. This is quite likely the reason
why 'format' event was deprecated.

If you insist on keeping the 'format' event, then it should be
formulated like this:

- send 'format' event for a format that is usable without an explicit
  modifier

- send 'modifier' event for a format that is usable with the given
  explicit modifier

- sending 'modifier' event does not directly imply that also 'format'
  event must be sent, because the two have different meanings.

However, for the reason mentioned below, that will make the format
advertisement asymmetric with creating buffers.

> > > 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.

A flag for "ignore argument m" does avoid the proliferation of
requests, but it seems redundant. DRM_FORMAT_MOD_INVALID has a unique
definition that cannot conflict with anything else, so we can as well
use that. If one passes the flag to use implicit modifiers, then we
should also require the modifier argument to be DRM_FORMAT_MOD_INVALID
anyway for the same reason you want the flag: clarity.

> > > 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.

One more detail: v3 'modifier' events for formats using the implicit
modifier must be sent with DRM_FORMAT_MOD_INVALID. Otherwise a
compositor has no way to advertise a format with the implicit modifier
as clients will ignore the 'format' event.

> > > 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.

Don't both kernel and EGL implementations require all modifiers of
planes to be equal? Was it not considered a mistake to have per-plane
modifiers in both APIs?

The current protocol extension was modelled after the existing DRM and
EGL APIs. I'm not against keeping the interface matching the DRM API.

> 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.

You cannot not send an argument in a request. 0 is valid modifier. This
results in a request that has a flag "ignore argument m" but no special
value to set m to when it needs to be ignored. People looking at the
protocol exchange will see a valid modifier and be confused when it's
not actually used.

I don't see replacing the use of DRM_FORMAT_MOD_INVALID with a new
variable 'bool use_modifier' making anything more clear in a
compositor. You still need the same two paths, you just have one more
variable to look at while the modifier variable will never indicate it
should be ignored.

> > 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)

We're going to need it in any case, to store a not-a-modifier value in
implementations.

The question is, do you require client implementations to send a valid
modifier with an ignore flag instead of an invalid modifier (with or
without an ignore flag).

To me the simplest and most obvious solution is to use
DRM_FORMAT_MOD_INVALID to signify "use implicit modifier", unless we
have separate events and requests without an explicit modifier.

The latter would look like this:
- 'format' -> 'format_mod_implicit'
- 'modifier' -> 'format_mod'
- 'create' -> 'create_mod_implicit' + 'create_mod'
- 'create_immed' -> 'create_immed_mod_implicit' + 'create_immed_mod'

I would be the happiest if the implicit modifier would not need to be
considered at all, meaning we would always have a valid explicit
modifier.

Is there use for implicit modifiers beyond pure legacy support?

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190412/53adc117/attachment-0001.sig>