<div dir="ltr"><div>I agree using DRM_FORMAT_MOD_INVALID for implicit modifier is simplest. Let's move the discussion to the other patch which does that.</div><div> </div><div>On Fri, Apr 12, 2019 at 2:17 AM Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: </div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, 11 Apr 2019 15:33:06 -0700 Chia-I Wu <<a href="mailto:olvaffe@gmail.com" target="_blank">olvaffe@gmail.com</a>> wrote: > On Thu, Apr 11, 2019 at 7:12 AM Simon Ser <<a href="mailto:contact@emersion.fr" target="_blank">contact@emersion.fr</a>> wrote: > > > On Thursday, April 11, 2019 11:48 AM, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> > > 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. </blockquote><div>It is written down. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. </blockquote><div>Yes. Explicit, implicit, and invalid can all be separated for clarity, if we model after the kernel.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > > 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? </blockquote><div>I don't have the answer. But I will leave it out of this discussion.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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 <a href="https://patchwork.freedesktop.org/patch/263061/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/patch/263061/</a> > > . > > > > > > 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 <a href="https://patchwork.freedesktop.org/patch/263061/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/patch/263061/</a> 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). </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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? </blockquote><div>It is only for legacy support to me.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Thanks, pq </blockquote></div></div>