Hi Daniel,
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".
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?
- 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)
Everything else is quite minor, although there's quite a few nitpicks.
On 8 June 2017 at 19:43, Daniel Stone <dani...@collabora.com> wrote:
> From: Louis-Francis Ratté-Boulianne <l...@collabora.com>
>
> Add support for DRI3 v1.1, which allows pixmaps to be backed by
> multi-planar buffers, or those with format modifiers. This is both
> for allocating render buffers, as well as EGLImage imports from a
> native pixmap (EGL_NATIVE_PIXMAP_KHR).
>
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> ---
> src/egl/drivers/dri2/egl_dri2.c | 3 +
> src/egl/drivers/dri2/egl_dri2.h | 1 +
> src/egl/drivers/dri2/platform_x11_dri3.c | 62 +++++++-
> src/glx/dri3_glx.c | 10 +-
> src/loader/loader_dri3_helper.c | 250
> +++++++++++++++++++++++++------
> src/loader/loader_dri3_helper.h | 16 +-
> 6 files changed, 293 insertions(+), 49 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 7175e827c9..5b0799fcc4 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -804,6 +804,9 @@ dri2_setup_extensions(_EGLDisplay *disp)
> if (!dri2_bind_extensions(dri2_dpy, mandatory_core_extensions,
> extensions, false))
> return EGL_FALSE;
>
> + dri2_dpy->multibuffers_available &=
> + (dri2_dpy->image && dri2_dpy->image->base.version >= 15);
> +
Store dri3major/minor and set the variable at once (like we do for
egl/dri2 and in glx) or try Eric's suggestions?
> dri2_bind_extensions(dri2_dpy, optional_core_extensions, extensions,
> true);
> return EGL_TRUE;
> }
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index a71b4489cd..a1a676d867 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -194,6 +194,7 @@ struct dri2_egl_display
> xcb_screen_t *screen;
> int swap_available;
> #ifdef HAVE_DRI3
> + int multibuffers_available;
Please make this bool. Eric has just addressed all the other
int-pretending-to-be-bool variables.
> struct loader_dri3_extensions loader_dri3_ext;
> #endif
> #endif
> diff --git a/src/egl/drivers/dri2/platform_x11_dri3.c
> b/src/egl/drivers/dri2/platform_x11_dri3.c
> index 041da3208d..0db2d7872a 100644
> --- a/src/egl/drivers/dri2/platform_x11_dri3.c
> +++ b/src/egl/drivers/dri2/platform_x11_dri3.c
> @@ -198,7 +198,9 @@ dri3_create_surface(_EGLDriver *drv, _EGLDisplay *disp,
> EGLint type,
>
> if (loader_dri3_drawable_init(dri2_dpy->conn, drawable,
> dri2_dpy->dri_screen,
> - dri2_dpy->is_different_gpu, dri_config,
> + dri2_dpy->is_different_gpu,
> + dri2_dpy->multibuffers_available,
> + dri_config,
> &dri2_dpy->loader_dri3_ext,
> &egl_dri3_vtable,
> &dri3_surf->loader_drawable)) {
> @@ -344,15 +346,71 @@ dri3_create_image_khr_pixmap(_EGLDisplay *disp,
> _EGLContext *ctx,
> return &dri2_img->base;
> }
>
> +#if XCB_DRI3_MAJOR_VERSION > 1 || XCB_DRI3_MINOR_VERSION >= 1
While we will nuke these before 2.1 comes about, let's be more
explicit and check for "major || (major && minor)".
The run-time checks are more important and should be updated as well.
> +static _EGLImage *
> +dri3_create_image_khr_pixmap_from_buffers(_EGLDisplay *disp, _EGLContext
> *ctx,
> + EGLClientBuffer buffer,
> + const EGLint *attr_list)
> +{
> + struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> + struct dri2_egl_image *dri2_img;
> + xcb_dri3_buffers_from_pixmap_cookie_t bp_cookie;
> + xcb_dri3_buffers_from_pixmap_reply_t *bp_reply;
> + xcb_drawable_t drawable;
> +
> + drawable = (xcb_drawable_t) (uintptr_t) buffer;
> + bp_cookie = xcb_dri3_buffers_from_pixmap(dri2_dpy->conn, drawable);
> + bp_reply = xcb_dri3_buffers_from_pixmap_reply(dri2_dpy->conn,
> + bp_cookie, NULL);
> +
> + if (!bp_reply) {
> + _eglError(EGL_BAD_ALLOC, "xcb_dri3_buffer_from_pixmap");
> + return NULL;
Error message and return value differs with the cases below - please
unify. Leaning slightly towards the below examples.
> + }
> +
> + dri2_img = malloc(sizeof *dri2_img);
> + if (!dri2_img) {
> + _eglError(EGL_BAD_ALLOC, "dri3_create_image_khr");
> + return EGL_NO_IMAGE_KHR;
> + }
> +
> + if (!_eglInitImage(&dri2_img->base, disp)) {
Unrelated: I could swear I nuked the return type of _eglInitImage().
Patches coming in a second.
> + free(dri2_img);
> + return EGL_NO_IMAGE_KHR;
> + }
> +
> + dri2_img->dri_image =
> loader_dri3_create_image_from_buffers(dri2_dpy->conn,
> + bp_reply,
> +
> dri2_dpy->dri_screen,
> +
> dri2_dpy->image,
> + dri2_img);
> + free(bp_reply);
> +
> + if (!dri2_img->dri_image) {
> + _eglError(EGL_BAD_ATTRIBUTE, "dri3_create_image_khr");
> + free(dri2_img);
> + return EGL_NO_IMAGE_KHR;
> + }
> +
> + return &dri2_img->base;
> +}
> +#endif
> +
> static _EGLImage *
> dri3_create_image_khr(_EGLDriver *drv, _EGLDisplay *disp,
> _EGLContext *ctx, EGLenum target,
> EGLClientBuffer buffer, const EGLint *attr_list)
> {
> (void) drv;
> + struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
Compiler will flag as set-but-unused variable, but shouldn't be too annoying.
>
> switch (target) {
> case EGL_NATIVE_PIXMAP_KHR:
> +#if XCB_DRI3_MAJOR_VERSION > 1 || XCB_DRI3_MINOR_VERSION >= 1
> + if (dri2_dpy->multibuffers_available)
> + return dri3_create_image_khr_pixmap_from_buffers(disp, ctx, buffer,
> + attr_list);
> +#endif
> return dri3_create_image_khr_pixmap(disp, ctx, buffer, attr_list);
> default:
> return dri2_create_image_khr(drv, disp, ctx, target, buffer,
> attr_list);
> @@ -504,6 +562,8 @@ dri3_x11_connect(struct dri2_egl_display *dri2_dpy)
> free(error);
> return EGL_FALSE;
> }
> + dri2_dpy->multibuffers_available = dri3_query->major_version > 1 ||
> + dri3_query->minor_version >= 1;
As mentioned above this should be major || (major && minor)
> free(dri3_query);
>
> present_query =
> diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
> index 509160697f..4b599ef897 100644
> --- a/src/glx/dri3_glx.c
> +++ b/src/glx/dri3_glx.c
> @@ -374,7 +374,10 @@ dri3_create_drawable(struct glx_screen *base, XID
> xDrawable,
> {
> struct dri3_drawable *pdraw;
> struct dri3_screen *psc = (struct dri3_screen *) base;
> + const struct dri3_display *const pdp = (struct dri3_display *)
> + base->display->dri3Display;
> __GLXDRIconfigPrivate *config = (__GLXDRIconfigPrivate *) config_base;
> + int has_multibuffer = 0;
Please use bool + false/true.
>
> pdraw = calloc(1, sizeof(*pdraw));
> if (!pdraw)
> @@ -385,11 +388,16 @@ dri3_create_drawable(struct glx_screen *base, XID
> xDrawable,
> pdraw->base.drawable = drawable;
> pdraw->base.psc = &psc->base;
>
> + if ((psc->image && psc->image->base.version >= 15) &&
> + (pdp->dri3Major > 1 || pdp->dri3Minor >= 1))
Ditto - ... major > 1 || (major == 1 && minor >= 1)
> + has_multibuffer = 1;
> +
> (void) __glXInitialize(psc->base.dpy);
>
> if (loader_dri3_drawable_init(XGetXCBConnection(base->dpy),
> xDrawable, psc->driScreen,
> - psc->is_different_gpu, config->driConfig,
> + psc->is_different_gpu, has_multibuffer,
> + config->driConfig,
> &psc->loader_dri3_ext, &glx_dri3_vtable,
> &pdraw->loader_drawable)) {
> free(pdraw);
> diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
> index 493a7f5218..3908512b9f 100644
> --- a/src/loader/loader_dri3_helper.c
> +++ b/src/loader/loader_dri3_helper.c
> @@ -40,6 +40,10 @@
> #define DRI_CONF_VBLANK_DEF_INTERVAL_1 2
> #define DRI_CONF_VBLANK_ALWAYS_SYNC 3
>
> +#ifndef DRM_FORMAT_MOD_INVALID
> +#define DRM_FORMAT_MOD_INVALID ((1ULL<<56) - 1)
> +#endif
> +
> static inline void
> dri3_fence_reset(xcb_connection_t *c, struct loader_dri3_buffer *buffer)
> {
> @@ -128,6 +132,7 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
> xcb_drawable_t drawable,
> __DRIscreen *dri_screen,
> bool is_different_gpu,
> + bool multiplanes_available,
> const __DRIconfig *dri_config,
> struct loader_dri3_extensions *ext,
> const struct loader_dri3_vtable *vtable,
> @@ -145,6 +150,7 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
> draw->drawable = drawable;
> draw->dri_screen = dri_screen;
> draw->is_different_gpu = is_different_gpu;
> + draw->multiplanes_available = multiplanes_available;
>
> draw->have_back = 0;
> draw->have_fake_front = 0;
> @@ -814,6 +820,27 @@ dri3_cpp_for_format(uint32_t format) {
> }
> }
>
> +/* the DRIimage createImage function takes __DRI_IMAGE_FORMAT codes, while
> + * the createImageFromFds call takes __DRI_IMAGE_FOURCC codes. To avoid
> + * complete confusion, just deal in __DRI_IMAGE_FORMAT codes for now and
> + * translate to __DRI_IMAGE_FOURCC codes in the call to createImageFromFds
> + */
> +static int
> +image_format_to_fourcc(int format)
> +{
> +
> + /* Convert from __DRI_IMAGE_FORMAT to __DRI_IMAGE_FOURCC (sigh) */
> + switch (format) {
> + case __DRI_IMAGE_FORMAT_SARGB8: return __DRI_IMAGE_FOURCC_SARGB8888;
> + case __DRI_IMAGE_FORMAT_RGB565: return __DRI_IMAGE_FOURCC_RGB565;
> + case __DRI_IMAGE_FORMAT_XRGB8888: return __DRI_IMAGE_FOURCC_XRGB8888;
> + case __DRI_IMAGE_FORMAT_ARGB8888: return __DRI_IMAGE_FOURCC_ARGB8888;
> + case __DRI_IMAGE_FORMAT_ABGR8888: return __DRI_IMAGE_FOURCC_ABGR8888;
> + case __DRI_IMAGE_FORMAT_XBGR8888: return __DRI_IMAGE_FOURCC_XBGR8888;
> + }
> + return 0;
> +}
> +
Keep code motion as separate commit, pretty please.
> /** loader_dri3_alloc_render_buffer
> *
> * Use the driver createImage function to construct a __DRIimage, then
> @@ -830,8 +857,10 @@ dri3_alloc_render_buffer(struct loader_dri3_drawable
> *draw, unsigned int format,
> xcb_pixmap_t pixmap;
> xcb_sync_fence_t sync_fence;
> struct xshmfence *shm_fence;
> - int buffer_fd, fence_fd;
> - int stride;
> + int buffer_fds[4], fence_fd;
> + uint32_t fourcc_format;
> + int num_planes = 0;
> + int i, mod;
>
> /* Create an xshmfence object and
> * prepare to send that to the X server
> @@ -855,14 +884,67 @@ dri3_alloc_render_buffer(struct loader_dri3_drawable
> *draw, unsigned int format,
> if (!buffer->cpp)
> goto no_image;
>
> + fourcc_format = image_format_to_fourcc(format);
> + if (!fourcc_format)
> + goto no_image;
Should be called only in the multiplanes_available case.
> +
> if (!draw->is_different_gpu) {
> - buffer->image = draw->ext->image->createImage(draw->dri_screen,
We're about to have 10+ cases of "draw->ext->image" - introduce a
(short) local variable?
> - width, height,
> - format,
> - __DRI_IMAGE_USE_SHARE |
> - __DRI_IMAGE_USE_SCANOUT |
> -
> __DRI_IMAGE_USE_BACKBUFFER,
> - buffer);
> +#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.
> + xcb_dri3_get_supported_modifiers_cookie_t mod_cookie;
> + xcb_dri3_get_supported_modifiers_reply_t *mod_reply;
> + xcb_generic_error_t *error = NULL;
> + uint64_t *modifiers = NULL;
> + uint32_t *mod_parts;
> + int i, count;
num_modifiers is uint32_t - please change the count declaration to be the same.
> +
> + mod_cookie = xcb_dri3_get_supported_modifiers(draw->conn,
> + draw->drawable,
> + fourcc_format);
> + mod_reply = xcb_dri3_get_supported_modifiers_reply(draw->conn,
> + mod_cookie,
> + &error);
> + if (!mod_reply)
> + goto no_image;
> +
> + count = mod_reply->num_modifiers;
> + if (count) {
> + modifiers = malloc(count * sizeof(uint64_t));
> + if (!modifiers) {
> + free(mod_reply);
> + goto no_image;
> + }
> +
> + 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?
> + 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.
> + buffer->image = draw->ext->image->createImage(draw->dri_screen,
> + width, height,
> + format,
> + __DRI_IMAGE_USE_SHARE
> |
> +
> __DRI_IMAGE_USE_SCANOUT |
> +
> __DRI_IMAGE_USE_BACKBUFFER,
> + buffer);
> +
> pixmap_buffer = buffer->image;
>
> if (!buffer->image)
> @@ -890,25 +972,82 @@ dri3_alloc_render_buffer(struct loader_dri3_drawable
> *draw, unsigned int format,
> goto no_linear_buffer;
> }
>
> - /* X wants the stride, so ask the image for it
> + /* X want some information about the planes, so ask the image for it
> */
> - if (!draw->ext->image->queryImage(pixmap_buffer,
> __DRI_IMAGE_ATTRIB_STRIDE,
> - &stride))
> - goto no_buffer_attrib;
> -
> - buffer->pitch = stride;
> + draw->ext->image->queryImage(pixmap_buffer, __DRI_IMAGE_ATTRIB_NUM_PLANES,
> + &num_planes);
> + if (num_planes <= 0)
> + num_planes = 1;
> +
> + for (i = 0; i < num_planes; i++) {
> + __DRIimage *image = draw->ext->image->fromPlanar(pixmap_buffer, i,
> NULL);
> + int ret;
> + if (!image)
> + image = pixmap_buffer;
> + ret = draw->ext->image->queryImage(image, __DRI_IMAGE_ATTRIB_FD,
> + &buffer_fds[i]);
> + if (!ret) {
> + if (image != pixmap_buffer)
> + draw->ext->image->destroyImage(image);
> + goto no_buffer_attrib;
> + }
> + ret = draw->ext->image->queryImage(image, __DRI_IMAGE_ATTRIB_STRIDE,
> + &buffer->strides[i]);
> + if (!ret) {
> + if (image != pixmap_buffer)
> + draw->ext->image->destroyImage(image);
> + goto no_buffer_attrib;
> + }
Bikeshed, yet it should make the code shorter/easier to read.
ret = queryImage(... _FD, &foo);
ret &= queryImage(... _STRIDE, &bar);
if !ret {
if (image != pixmap_buffer)
destroyImage(image);
goto no_buffer_attrib;
}
> + ret = draw->ext->image->queryImage(image, __DRI_IMAGE_ATTRIB_OFFSET,
> + &buffer->offsets[i]);
> + if (!ret)
> + buffer->offsets[i] = 0;
> + if (image != pixmap_buffer)
> + draw->ext->image->destroyImage(image);
> + }
>
> - if (!draw->ext->image->queryImage(pixmap_buffer, __DRI_IMAGE_ATTRIB_FD,
> - &buffer_fd))
> - goto no_buffer_attrib;
> + if (!draw->ext->image->queryImage(pixmap_buffer,
> + __DRI_IMAGE_ATTRIB_MODIFIER_UPPER,
> + &mod)) {
> + buffer->modifier = DRM_FORMAT_MOD_INVALID;
> + } else {
> + buffer->modifier = (uint64_t) mod << 32;
> + if (!draw->ext->image->queryImage(pixmap_buffer,
> + __DRI_IMAGE_ATTRIB_MODIFIER_LOWER,
> + &mod)) {
> + buffer->modifier = DRM_FORMAT_MOD_INVALID;
> + } else {
> + buffer->modifier |= (uint64_t)(mod & 0xffffffff);
> + }
Moar bikeshed:
ret = query(... UPPER, &mod);
modifier = (uint64_t) mod << 32;
ret &= query(... LOWER, &mod);
modifier = (uint64_t) (mod & 0xffffffff);
if !ret
modifier = INVALID;
> + }
>
> - 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.
> + xcb_dri3_pixmap_from_buffers(draw->conn,
> + (pixmap = xcb_generate_id(draw->conn)),
Move the xcb_generate_id() call a few lines above.
> + 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
> + fourcc_format,
> + buffer->modifier >> 32,
> + buffer->modifier & 0xffffffff,
> + buffer_fds);
> + }
> + else
> +#endif
> + {
> + xcb_dri3_pixmap_from_buffer(draw->conn,
> + (pixmap = xcb_generate_id(draw->conn)),
... and simply use "pixmap" here.
> + draw->drawable,
> + buffer->size,
> + width, height, buffer->strides[0],
> + depth, buffer->cpp * 8,
> + buffer_fds[0]);
> + }
>
> xcb_dri3_fence_from_fd(draw->conn,
> pixmap,
> @@ -930,6 +1069,8 @@ dri3_alloc_render_buffer(struct loader_dri3_drawable
> *draw, unsigned int format,
> return buffer;
>
> no_buffer_attrib:
> + while (--i >= 0)
> + close(buffer_fds[i]);
I think Eric is spot on here - it seems like we leak an fd.
> draw->ext->image->destroyImage(pixmap_buffer);
> no_linear_buffer:
> if (draw->is_different_gpu)
> @@ -1037,27 +1178,6 @@ dri3_update_drawable(__DRIdrawable *driDrawable,
> return true;
> }
>
> -/* the DRIimage createImage function takes __DRI_IMAGE_FORMAT codes, while
> - * the createImageFromFds call takes __DRI_IMAGE_FOURCC codes. To avoid
> - * complete confusion, just deal in __DRI_IMAGE_FORMAT codes for now and
> - * translate to __DRI_IMAGE_FOURCC codes in the call to createImageFromFds
> - */
> -static int
> -image_format_to_fourcc(int format)
> -{
> -
> - /* Convert from __DRI_IMAGE_FORMAT to __DRI_IMAGE_FOURCC (sigh) */
> - switch (format) {
> - case __DRI_IMAGE_FORMAT_SARGB8: return __DRI_IMAGE_FOURCC_SARGB8888;
> - case __DRI_IMAGE_FORMAT_RGB565: return __DRI_IMAGE_FOURCC_RGB565;
> - case __DRI_IMAGE_FORMAT_XRGB8888: return __DRI_IMAGE_FOURCC_XRGB8888;
> - case __DRI_IMAGE_FORMAT_ARGB8888: return __DRI_IMAGE_FOURCC_ARGB8888;
> - case __DRI_IMAGE_FORMAT_ABGR8888: return __DRI_IMAGE_FOURCC_ABGR8888;
> - case __DRI_IMAGE_FORMAT_XBGR8888: return __DRI_IMAGE_FOURCC_XBGR8888;
> - }
> - return 0;
> -}
> -
> __DRIimage *
> loader_dri3_create_image(xcb_connection_t *c,
> xcb_dri3_buffer_from_pixmap_reply_t *bp_reply,
> @@ -1099,6 +1219,44 @@ loader_dri3_create_image(xcb_connection_t *c,
> return ret;
> }
>
> +#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?
> + strides[i] = strides_in[i];
> + offsets[i] = offsets_in[i];
In hindsight making the DRI interface use uint32_t might have been a
good idea ;-)
> + }
> +
> + modifier = (uint64_t) bp_reply->modifier_hi << 32;
> + modifier |= ((uint64_t) bp_reply->modifier_lo) & 0xffffffff;
> +
> + 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?
> + &error, loaderPrivate);
> +}
> +#endif
> +
> /** dri3_get_pixmap_buffer
> *
> * Get the DRM object for a pixmap from the X server and
> diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h
> index a865e46355..6bac25e607 100644
> --- a/src/loader/loader_dri3_helper.h
> +++ b/src/loader/loader_dri3_helper.h
> @@ -61,8 +61,11 @@ struct loader_dri3_buffer {
> bool busy; /* Set on swap, cleared on IdleNotify */
> bool own_pixmap; /* We allocated the pixmap ID, free on
> destroy */
>
> + uint32_t num_planes;
> uint32_t size;
> - uint32_t pitch;
> + int strides[4];
> + int offsets[4];
> + uint64_t modifier;
> uint32_t cpp;
> uint32_t flags;
> uint32_t width, height;
> @@ -125,6 +128,7 @@ struct loader_dri3_drawable {
> /* Information about the GPU owning the buffer */
> __DRIscreen *dri_screen;
> bool is_different_gpu;
> + bool multiplanes_available;
>
> /* Present extension capabilities
> */
> @@ -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.
Thanks
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
