[PATCH RFC wayland-protocols] unstable/linux-dmabuf: add wp_linux_dmabuf_device_hint

[Simon Ser]

Hi,

Thanks for your review!

> On Thu, 1 Nov 2018 at 16:45, Simon Ser <contact at emersion.fr> wrote:
> > This commit introduces a new wp_linux_dmabuf_device_hints object. This object
> > advertizes a preferred device via a file descriptor and a set of preferred
> > formats/modifiers.
>
> s/advertizes/advertises/g (including in the XML doc)

Ah, it seems that for once British English and American English agree on the
spelling. Noted!

> I also think this would be better called
> wp_linux_dmabuf_surface_hints, since the change over the dmabuf
> protocol is that it's surface-specific.

Right. The intent was to be able to re-use this object for hints not bound to
surfaces in the future. But better not to try to think of all possible
extensions (which will probably have different requirements).

Updated to use wp_linux_dmabuf_surface_hints.

> > +    <event name="primary_device">
> > +      <description summary="preferred primary device">
> > +        This event advertizes the primary device that the server prefers. There
> > +        is exactly one primary device.
> > +      </description>
> > +      <arg name="fd" type="fd" summary="device file descriptor"/>
> > +    </event>
>
> I _think_ this might want to refer to separate objects.
>
> When we receive an FD from the server, we don't know what device it
> refers to, so we have to open the device to probe it. Opening the
> device can be slow: if a device is in a low PCI power state, it can be
> a couple of seconds to physically power up the device and then wait
> for it to initialise before we can interrogate it.
>
> One way around this would be to have a separate wp_linux_dmabuf_device
> object, lazily sent as a new object in an event by the root
> wp_linux_dmabuf object, with the per-surface hints then referring to a
> previously-sent device. This would allow clients to only probe each
> device once per EGLDisplay, rather than once per EGLSurface.

I see. One other way to fix this issue would be to keep the protocol as-is but
to make the client use stat(3p) to check if it doesn't already know about the
device. Per the POSIX spec [1]:

> The st_ino and st_dev fields taken together uniquely identify the file within
> the system.

This would remove the overhead and complexity of server-allocated objects, which
are hard to teardown.

But I'm maybe missing some use-cases here?

[1]: http://pubs.opengroup.org/onlinepubs/009696699/basedefs/sys/stat.h.html

> > +    <event name="modifier">
> > +      <description summary="preferred buffer format modifier">
> > +        This event advertises the formats that the server prefers, along with
> > +        the modifiers preferred for each format.
> > +
> > +        For the definition of the format and modifier codes, see the
> > +        wp_linux_buffer_params::create request.
> > +      </description>
> > +      <arg name="format" type="uint" summary="DRM_FORMAT code"/>
> > +      <arg name="modifier_hi" type="uint"
> > +           summary="high 32 bits of layout modifier"/>
> > +      <arg name="modifier_lo" type="uint"
> > +           summary="low 32 bits of layout modifier"/>
> > +    </event>
>
> I think we want another event here, to group sets of modifiers
> together by preference.
>
> For example, say the surface could be directly scanned out, but only
> if it uses the linear or X-tiled modifiers. Our surface-preferred
> modifiers would be LINEAR + X_TILED. However, the client may not be
> able to produce that combination. If the GPU still supports Y_TILED,
> then we want to indicate that the client _can_ use Y_TILED if it needs
> to, but _should_ use LINEAR or X_TILED.
>
> DRI3 implements this by sending sets of modifiers in 'tranches', which
> are arrays of arrays, which in this case would be:
> tranches = {
>   [0 /* optimal */] = {
>     { .format = XRGB8888, .modifier = LINEAR }
>     { .format = XRGB8888, .modifier = X_TILED }
>   },
>   [1 /* less optimal */] = {
>     { .format = XRGB8888, .modifier = Y_TILED }
>   }
> }
>
> I imagine the best way to do it with Wayland events would be to add a
> 'marker' event to indicate the border between these tranches. So we
> would send:
>   modifier(XRGB8888, LINEAR)
>   modifier(XRGB8888, X_TILED)
>   barrier()
>   modifier(XRGB8888, Y_TILED)
>   barrier()
>   done()
>
> For a simple 'GPU composition or scanout' case, this would only be two
> tranches, which are 'most optimal' and 'fallback'. For multiple GPUs
> though, we could end up with three tranches: scanout-capable,
> same-GPU-composition, or cross-GPU-composition. Similarly, if we take
> media recording into account, we could end up with more than two
> tranches.
>
> What do you think?

This seems like a good idea. Other solutions include having an enum for tranches
(preferred, fallback, etc) but that restricts the number of tranches. Using
tranche indexes makes the protocol more complicated. So your idea LGTM.

I'll also change the wording from "preferred" to "supported in order of
preference".

I have another question: what if the compositor doesn't know about the preferred
device? For instance if it's running nested in another Wayland compositor that
doesn't support this new protocol version. Maybe we should make all events
optional to let the compositor say "I have no idea"?