[Mesa-dev] [RFC 01/22] RFC: egl/x11: Support DRI3 v1.1

[Daniel Stone]

Hey Emil,
A few bits from me, since this is actually lfrb's code ...

On 20 June 2017 at 15:19, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> Top-level comments
>
> Build POV:
> - Having the XCB_DRI3_.*_VERSION compile guards is Ok for now. But
> let's drop those as get a xcb release.
> We dont want to be in cases where Mesa is built w/o DRI3 1.1 support
> and one spends time debugging why "nothing works".

I don't think anything should fail ... ? If DRI3v1.1 isn't available
(either client or server), we won't be calling
->createImageWithModifiers, so the old path will only allocate buffers
which can be described without modifiers; either linear, or where the
tiling can be determined by the importer with no explicit information.
Similarly, we fall back gracefully to the single-plane/no-modifier
DRI3 wire requests.

I'm fine with bumping the dependencies personally, but it means that
people will have to update both xcb-proto and libxcb to be able to
build at all on, say, the version of Debian released a couple of days
ago.

> Couple of ideas/questions from the protocol POV:
> - Do we want to send color_space, sample_range, horiz_siting or
> vert_siting information over the wire?

No, we don't. We store them for buffers imported from dmabuf, but
colour_space is only relevant to YUV images (BT.601/BT.709), and the
other three are only relevant to subsampled (i.e. subset of YUV)
images. X11 surfaces are RGB-only, so.

> - Considering we know the number of planes, one could simply pass a
> pointer to the strides/offsets, instead of passing each one explicitly
> (re: xcb_dri3_pixmap_from_buffers)

We could indeed, but it means more fiddly marshalling work on both sides.

>> +#if XCB_DRI3_MAJOR_VERSION > 1 || XCB_DRI3_MINOR_VERSION >= 1
>> +      if (draw->multiplanes_available &&
>> +          draw->ext->image->base.version >= 15 &&
>> +          draw->ext->image->queryDmaBufModifiers &&
>> +          draw->ext->image->createImageWithModifiers) {
> Wondering if we should nuke the >= 15 piece (here or in the callers)
> or keep it everywhere for clarity.

If the image version is less than 15, then the function pointers could
be pointing into uninitialised/other memory, no?

>> +            mod_parts = xcb_dri3_get_supported_modifiers_modifiers(mod_reply);
> I take it this function cannot fail, or we simply don't bother with
> such cases elsewhere?

Cannot fail:
uint32_t *
xcb_dri3_get_supported_modifiers_modifiers (const
xcb_dri3_get_supported_modifiers_reply_t *R)
{
    return (uint32_t *) (R + 1);
}

>> -   xcb_dri3_pixmap_from_buffer(draw->conn,
>> -                               (pixmap = xcb_generate_id(draw->conn)),
>> -                               draw->drawable,
>> -                               buffer->size,
>> -                               width, height, buffer->pitch,
>> -                               depth, buffer->cpp * 8,
>> -                               buffer_fd);
>> +#if XCB_DRI3_MAJOR_VERSION > 1 || XCB_DRI3_MINOR_VERSION >= 1
>> +   if (draw->multiplanes_available) {
> This else looks a bit odd. If we fail to manage multiple buffers
> above, multiplanes_available will still be true, yet we could have a
> DRIImage.
> We should track that (modify multiplanes_available/other) and act
> accordingly here.

You mean if createImageWithModifiers fails? In that case, calling the
multi-plane DRI3 Pixmap creation is still legitimate; the information
the driver gave us isn't any less valid, so we can feed that into the
newer requests. It's just that the driver has taken a different
allocation path.

(I'll let lfrb answer if the fallthrough from createImageWithModifiers
to createImage was intentional or not.)

>> +                                   draw->drawable,
>> +                                   num_planes,
>> +                                   width, height,
>> +                                   buffer->strides[0], buffer->offsets[0],
>> +                                   buffer->strides[1], buffer->offsets[1],
>> +                                   buffer->strides[2], buffer->offsets[2],
>> +                                   buffer->strides[3], buffer->offsets[3],
> num_planes

??

>> +#if XCB_DRI3_MAJOR_VERSION > 1 || XCB_DRI3_MINOR_VERSION >= 1
>> +__DRIimage *
>> +loader_dri3_create_image_from_buffers(xcb_connection_t *c,
>> +                                      xcb_dri3_buffers_from_pixmap_reply_t *bp_reply,
>> +                                      __DRIscreen *dri_screen,
>> +                                      const __DRIimageExtension *image,
>> +                                      void *loaderPrivate)
>> +{
>> +   int                                  *fds;
>> +   uint32_t                             *strides_in, *offsets_in;
>> +   int                                   strides[4], offsets[4];
>> +   uint64_t                              modifier;
>> +   unsigned                              error;
>> +   int                                   i;
>> +
>> +   fds = xcb_dri3_buffers_from_pixmap_reply_fds(c, bp_reply);
>> +   strides_in = xcb_dri3_buffers_from_pixmap_strides(bp_reply);
>> +   offsets_in = xcb_dri3_buffers_from_pixmap_offsets(bp_reply);
>> +   for (i = 0; i < bp_reply->nfd; i++) {
> Assert/error/clamp when xcb goes crazy and bp_reply->nfd >= 4?

If XCB (or the server) was that broken, I can't imagine what else
would go wrong. But sure, with the caveat that 4 is OK.

>> +   return image->createImageFromDmaBufs2(dri_screen,
>> +                                         bp_reply->width,
>> +                                         bp_reply->height,
>> +                                         bp_reply->format,
>> +                                         modifier,
>> +                                         fds, bp_reply->nfd,
>> +                                         strides, offsets,
>
>> +                                         0, 0, 0, 0, /* UNDEFINED */
> Question: Should we extend the protocol to provide this information?

As above, it's only relevant for YUV buffers.

>> @@ -174,6 +178,7 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
>>                            xcb_drawable_t drawable,
>>                            __DRIscreen *dri_screen,
>>                            bool is_different_gpu,
>> +                          bool is_multiplanes_available,
> s/is_multiplanes_available/multiplanes_available/ maybe?
> FWIW we could/should update the is_different_gpu at a later stage.

Update how?

Cheers,
Daniel