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

[Emil Velikov]

Hi Dan,

On 20 June 2017 at 16:18, Daniel Stone <daniel at fooishbar.org> wrote:
> 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.
>
Any performance/power usage testing will produce inaccurate results.
Hence by some definition it may "fail" or "not work".

Regardless of the name used, it will take a while to track down and fix.

> 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.
>
We already pull, build and install a dozen components on the Travis-CI
instance for Mesa.
People are more than welcome to reuse/copy it.

>> 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.
>
I thought there was someone working on support for non-RGB surfaces?
Must have been day-dreaming ;-)

>> - 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.
>
Ack. I take it that the X/XCB people will prefer the less-fiddly option.

>>> +#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?
>
Agreed. The version check is present both here and in the EGL/GLX code.
Wondering if we cannot simplify/unify that, but running short on ideas.

>>> +            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);
> }
>
Great.

>>> -   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.)
>
I believe you're spot on - need to think about it again, just in case.

>>> +                                   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
>
> ??
>
Misplaces copy/paste.

>>> @@ -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?
>
Actually, never mind.

Thanks
Emil