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

Fri Apr 12 17:26:56 UTC 2019

I agree using DRM_FORMAT_MOD_INVALID for implicit modifier is simplest.
Let's move the discussion to the other patch which does that.

On Fri, Apr 12, 2019 at 2:17 AM Pekka Paalanen <ppaalanen at gmail.com> wrote:

> 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 is written 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.
>
Yes.  Explicit, implicit, and invalid can all be separated for clarity, if
we model after the kernel.

> > > > 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?
>
I don't have the answer.  But I will leave it out of this discussion.

>
> 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?
>
It is only for legacy support to me.

>
>
> Thanks,
> pq
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190412/3903c627/attachment-0001.html>