<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">On Thu, Apr 11, 2019 at 7:12 AM Simon Ser <<a href="mailto:contact@emersion.fr" target="_blank">contact@emersion.fr</a>> wrote:<br></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 Thursday, April 11, 2019 11:48 AM, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote:<br>
> Hi,<br>
><br>
> at first hand, I would keep Weston's behaviour, because that is what<br>
> the current protocol specification says exactly. The specification text<br>
> is very clear on the intent: the old format event is not to be used, if<br>
> both the client and server support the version with the modifier event.<br>
> When the protocol will be stabilized (which is always an ABI break),<br>
> the format event without a modifier will be removed.<br></blockquote><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> The difference to EGL API does not seem to be making anything more<br>
> difficult either.<br>
><br>
> Technically there is little difference between using two events with<br>
> and without a modifier, and one event with a special no-modifier value.<br>
> A technical reason to favour two different events would be bandwidth<br>
> saving, since then one doesn't need to send the dummy modifier. Can we<br>
> make that argument? If yes, that would be a good justification to make<br>
> the change you propose.<br>
<br>
Do we have a lot of these? My machines don't have any (i915 driver).<br></blockquote><div>No, bandwidth should not be a concern. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
But anyway, I thought that DRM_FORMAT_MOD_INVALID was being phased out?<br></blockquote><div>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.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> At first read of your commit message, I got the feeling you want<br>
> formats to be advertised by both events: format event to declare a<br>
> format at all, plus modifier events to list any modifiers. That is also<br>
> what your proposed spec text reads, so I'd recommend clarifying that<br>
> implementations need to send only one of the two events per format. Or<br>
> both if the same format can be used with and without a modifier?<br></blockquote><div>Your interpretation is correct: format events are for formats, and modifier events are for modifiers.</div><div><br></div><div>This is more intuitive comparing to Weston's.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> If we have two events, one without and one with a modifier, should we<br>
> also then have two (well, four actually, for the immed variant)<br>
> requests to create a buffer: one without and one with a modifier? I<br>
> think it would be nice to have these consistent unless there is a<br>
> reason not to.<br>
><br>
> I can find justification both ways, and whether we need to fix Weston<br>
> or Mutter is not that important to me. I might want to avoid the four<br>
> create requests case though.<br>
<br>
I agree that either way, some compositors will need to be fixed, and that we<br>
shouldn't decide based on current implementations.</blockquote><div>At the advertising phase, supported formats and modifiers are unambiguously advertised.</div><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> I'm not sure the versioning fully works out, if we have:<br>
> - version 1 and 2 have only 'format' event<br>
> - version 3 has 'modifier' event, with 'format' event not to be used<br>
> - version 4 wants to use both 'modifier' and 'format' events<br>
<br>
Yeah, that would be a little bit annoying.</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Implementation would need to adhere to these, so Mutter would still<br>
> need fixing for clients that bind version 3. I don't think we should<br>
> retro-actively change the semantics of an existing version.<br>
<br>
If the protocol isn't ambiguous, it would be dangerous to change semantics<br>
indeed.<br></blockquote><div>When it comes to implementations, my view is</div><div><br></div><div> v1/v2: `format' event</div><div> v3: add `modifier' event and `format' event becomes optional (i.e., clients ignores it)</div><div> v4: `format' should not be optional</div><div> </div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> I notice that currently the modifier is in<br>
> zwp_linux_buffer_params_v1.add request which makes the modifier<br>
> per-plane, but I seem to recall that in practise modifiers are nowadays<br>
> used only per-buffer, not per-plane, so the modifier argument should<br>
> move to the create and create_immed requests.<br>
<br>
You're right, modifiers are now per-buffer and not per-plane.<br></blockquote><div>This diverges from what EGL and kernel do.  If we are sure it will be fine down the road, I am fine with it.</div><div><br></div><div>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?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> We also need to consider <a href="https://patchwork.freedesktop.org/patch/263061/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/patch/263061/</a> .<br>
><br>
> I would propose the following roadmap:<br>
><br>
> 1. Do not un-deprecate format events; instead clarify the modifier on<br>
>    create and fix Mutter's behaviour.<br>
><br>
> 2. Get <a href="https://patchwork.freedesktop.org/patch/263061/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/patch/263061/</a> merged.<br>
><br>
> 3. Stop using wl_drm completely when both server and client support the<br>
>    latest zwp_linux_dmabuf.<br>
><br>
> 4. Do ABI-breaking cleanups, and bump the major or if we are sure<br>
>    enough then declare the extension stable.<br>
><br>
> The reason why I would throw the 'format' event away as it is is that<br>
> then it will be consistent with the create requests: use<br>
> DRM_FORMAT_MOD_INVALID for "no modifier" in both events and requests. I </blockquote><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> would assume that the bandwidth savings would not be significant,<br>
> considering everything is aiming to have explicit modifiers.<br>
><br>
> How would that sound?<br>
<br>
This makes a lot of sense to me. +1.<br></blockquote><div><div>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</div><div><br></div><div><div>/*</div><div> * Invalid Modifier</div><div> *</div><div> * This modifier can be used as a sentinel to terminate the format modifiers</div><div> * list, or to initialize a variable with an invalid modifier. It might also be</div><div> * used to report an error back to userspace for certain APIs.</div><div> */</div><div>#define DRM_FORMAT_MOD_INVALID<span style="white-space:pre">       </span>fourcc_mod_code(NONE, DRM_FORMAT_RESERVED)</div></div><br><br></div></div></div></div></div></div>