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 <ppaala...@gmail.com> wrote: > On Thu, 11 Apr 2019 15:33:06 -0700 > Chia-I Wu <olva...@gmail.com> wrote: > > > On Thu, Apr 11, 2019 at 7:12 AM Simon Ser <cont...@emersion.fr> wrote: > > > > > On Thursday, April 11, 2019 11:48 AM, Pekka Paalanen < > ppaala...@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 >
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel