[Mesa-dev] [PATCH v2 11/11] egl/wayland: Use linux-dmabuf interface for buffers

[Daniel Stone]

Hi,

On 13 July 2017 at 17:39, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On July 13, 2017 4:13:04 AM Daniel Stone <daniels at collabora.com> wrote:
>> @@ -643,17 +695,68 @@ create_wl_buffer(struct dri2_egl_display *dri2_dpy,
>> +   if (dri2_dpy->wl_dmabuf && dri2_dpy->image->base.version >= 15) {
>> +      struct zwp_linux_buffer_params_v1 *params;
>> +      int mod_hi, mod_lo;
>> +      int i;
>> +
>> +      dri2_dpy->image->queryImage(image,
>> __DRI_IMAGE_ATTRIB_MODIFIER_UPPER,
>> +                                  &mod_hi);
>> +      dri2_dpy->image->queryImage(image,
>> __DRI_IMAGE_ATTRIB_MODIFIER_LOWER,
>> +                                  &mod_lo);
>
>
> What if the image wasn't actually created with modifiers?  Shouldn't we fall
> back to the old interface in that case?

I'm quite relaxed about that; I would consider any driver implementing
DRIImage v15 and not supporting an accurate modifier query to be
broken. Regardless of how the image was created.

Given that, I don't see why we should fall back to a no-modifier
interface, not least as doing so would also require a DRIImage attrib
specifying whether the image was created/imported via a modifier-aware
or non-modifier-aware interface. Seems fairly brittle, and easier to
just mandate that people accurately describe their buffers on query.

>> +static void
>> +dmabuf_handle_modifier(void *data, struct zwp_linux_dmabuf_v1 *dmabuf,
>> +                       uint32_t format, uint32_t modifier_hi,
>> +                       uint32_t modifier_lo)
>> +{
>> +   struct dri2_egl_display *dri2_dpy = data;
>> +   uint64_t *mod = NULL;
>> +
>> +   switch (format) {
>> +   case WL_DRM_FORMAT_ARGB8888:
>> +      mod = u_vector_add(&dri2_dpy->wl_modifiers.argb8888);
>> +      break;
>> +   case WL_DRM_FORMAT_XRGB8888:
>> +      mod = u_vector_add(&dri2_dpy->wl_modifiers.xrgb8888);
>> +      break;
>> +   case WL_DRM_FORMAT_RGB565:
>> +      mod = u_vector_add(&dri2_dpy->wl_modifiers.rgb565);
>> +      break;
>> +   default:
>> +      break;
>> +   }
>> +
>> +   if (!mod)
>> +      return;
>> +
>> +   *mod = (uint64_t) modifier_hi << 32;
>> +   *mod |= (uint64_t) (modifier_lo & 0xffffffff);
>> +}
>> +
>> +static const struct zwp_linux_dmabuf_v1_listener dmabuf_listener = {
>> +   .format = dmabuf_ignore_format,
>> +   .modifier = dmabuf_handle_modifier,
>
>
> Pardon my ignorance but what happens if the modifiers available change?
> That doesn't seem to be handled at all here.  That's a rather important case
> for us because scanout only supports CCS_E on two planes and doesn't support
> Y-tiling on gen8 and earlier.  If this is going to get us the perf
> improvement we're looking for, then we need the ability to renegotiate when
> a surface goes fulalscreen.

It's a good question, but - they don't change.

For it to change, we'd need a per-surface interface, as
zwp_linux_dmabuf_v1 is per-display. It'd need to be a bit more complex
in order to be useful, though. Specifically, you'd need a
'done'/'frame' event, but you'd also need to divide your lists of
suggested modifiers into tranches. The compositor needs to hand out
_all_ formats supported for GPU compositing (GPU is the fallback of
last resort, so it makes sense to maximise what it can accept in order
to give yourself the best chance), so we need a way to indicate 'these
are the most optimal [read: scanout] formats, so try these first; if
you don't get anywhere, try the next lot instead'. Concretely
speaking, for scanout candidates this would be [X_TILED, LINEAR
//tranche// Y_TILED] on gen8, or [Y_TILED, X_TILED, LINEAR //tranche//
Y_CCS] for gen9 pipe-C scanout. We'd then need to use that to actually
reallocate.

It is definitely doable, but it's a ton of typing. For gen9+, I don't
think there'll be too many people noticing that their third head is
slightly less performant? Pre-gen9 is more troubling due to lack of
tiling, but really you could just disable Y-tiling entirely there for
no performance regression at all, given that it's not supported before
this patch ...

Cheers,
Daniel