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

[Emil Velikov]

On 10 July 2017 at 22:26, Louis-Francis Ratté-Boulianne
<lfrb at collabora.com> wrote:
> Hi,
>
> On Tue, 2017-06-20 at 15:19 +0100, Emil Velikov wrote:
>> > +            for (i = 0; i < count; i++) {
>> > +               modifiers[i] = (uint64_t) mod_parts[i * 2] << 32;
>> > +               modifiers[i] |= (uint64_t) mod_parts[i * 2 + 1] &
>> > 0xffffff;
>> > +            }
>> > +         }
>> > +
>> > +         free(mod_reply);
>> > +
>> > +         buffer->image = draw->ext->image-
>> > >createImageWithModifiers(draw->dri_screen,
>> > +
>> >   width, height,
>> > +
>> >   format,
>> > +
>> >   modifiers, count,
>> > +
>> >   buffer);
>> > +         free(modifiers);
>> > +      }
>> > +#endif
>> > +
>> > +      if (!buffer->image)
>>
>> Does not align with all the error paths above. There we bail out, yet
>> here we fall-back to the old extension.
>> Perhaps change the former to come here? One could even more that
>> whole
>> hunk into a separate function.
>
> The rationale is that we only try to create a buffer with the supported
> modifiers. If it doesn't work, it's still sensible to try the old path
> as it's better to have a DRI image without any optimization than none.
>
Getting confused here...
In the hunk that you've dropped, if the (optional) xcb comms/malloc
fails we error out (goto no_image). At the same if
createImageWithModifiers() fails we fall-back w/o modifiers.

So the dependencies cannot fail, why the depende can ... why? What
would happen if we have new X server (DRI3 1.1 capable), Mesa, xcb,
while an old X driver?
IMHO if any of the dependencies fail, fallback to w/o mods.


>> +   }
>> >
>> > -   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.
>
> What can fail above (and not be critical) is creating an image with
> modifiers. I grant you that it means the resulting image will only have
> one plane, but that doesn't make it "bad" to use
> xcb_dri3_pixmap_from_buffers. multiplanes_available simply means that
> the X server actually supports using multiple planes.
>
Whether it's "bad" would depend on the implementation. I'm thinking
more of confusing and/or misleading.
Can you please add a couple of words as inline comment. Anything like
what you/Dan said seems reasonable IMHO.

Speaking of implementation, I might throw some questions.

BTW, Mesa's branch (feature freeze) is ~21st July. Personally it seems
perfectly possible to get this work in - the frontend bits at least.
What do you think?

Thanks
Emil