On 10 July 2017 at 22:26, Louis-Francis Ratté-Boulianne
<l...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
