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.veli...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
