[Mesa-dev] [PATCH] egl: android: add dma-buf fd support
Emil Velikov
emil.l.velikov at gmail.com
Wed Apr 20 01:03:15 UTC 2016
Hi Rob,
Please bear in mind that there's a fair bit of comments, but before
all don't mix refactoring and new code. Please ?
On 15 April 2016 at 17:03, Rob Herring <robh at kernel.org> wrote:
> Add support for creating images from Android native buffers with dma-buf
> fd. As dma-buf support also requires DRI image loader extension, add
> that as well.
>
> This is based on several originally patches written by Varad Gautam.
> I've collapsed them down to one and done a bit of reformatting. dma-bufs
> and GEM handles are now both supported instead of being compile time
> selection.
How did you test this ? Afaict making this (at least in current shape)
isn't possible to be a runtime decision.
> The dma-buf support is also re-written to use common
> dri2_create_image_dma_buf function in egl_dri2.c.
>
> Cc: Varad Gautam <varadgautam at gmail.com>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Emil Velikov <emil.velikov at collabora.com>
> Signed-off-by: Rob Herring <robh at kernel.org>
> ---
> src/egl/drivers/dri2/egl_dri2.h | 1 +
> src/egl/drivers/dri2/platform_android.c | 206 +++++++++++++++++++++++++-------
> 2 files changed, 165 insertions(+), 42 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index ef79939..a0fbe6f 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -285,6 +285,7 @@ struct dri2_egl_surface
> #ifdef HAVE_ANDROID_PLATFORM
> struct ANativeWindow *window;
> struct ANativeWindowBuffer *buffer;
> + __DRIimage *dri_image;
>
> /* EGL-owned buffers */
> __DRIbuffer *local_buffers[__DRI_BUFFER_COUNT];
> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
> index 41840aa..233a829 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -64,6 +64,45 @@ get_format_bpp(int native)
> return bpp;
> }
>
> +/* createImageFromFds requires fourcc format */
> +static int get_fourcc(int format)
> +{
> + switch(format) {
> + case __DRI_IMAGE_FORMAT_RGB565: return __DRI_IMAGE_FOURCC_RGB565;
> + case __DRI_IMAGE_FORMAT_ARGB8888: return __DRI_IMAGE_FOURCC_ARGB8888;
> + case __DRI_IMAGE_FORMAT_XRGB8888: return __DRI_IMAGE_FOURCC_XRGB8888;
> + case __DRI_IMAGE_FORMAT_ABGR8888: return __DRI_IMAGE_FOURCC_ABGR8888;
> + case __DRI_IMAGE_FORMAT_XBGR8888: return __DRI_IMAGE_FOURCC_XBGR8888;
> + }
> + return -1;
> +}
> +
> +static int get_format(int format)
> +{
> + switch (format) {
> + case HAL_PIXEL_FORMAT_BGRA_8888: return __DRI_IMAGE_FORMAT_ARGB8888;
> + case HAL_PIXEL_FORMAT_RGB_565: return __DRI_IMAGE_FORMAT_RGB565;
> + case HAL_PIXEL_FORMAT_RGBA_8888: return __DRI_IMAGE_FORMAT_ABGR8888;
> + case HAL_PIXEL_FORMAT_RGBX_8888: return __DRI_IMAGE_FORMAT_XBGR8888;
> + case HAL_PIXEL_FORMAT_RGB_888:
> + /* unsupported */
> + default:
> + _eglLog(_EGL_WARNING, "unsupported native buffer format 0x%x", format);
> + }
> + return -1;
> +}
> +static int
> +get_native_buffer_fd(struct ANativeWindowBuffer *buf)
> +{
> + native_handle_t *handle = (native_handle_t *)buf->handle;
> + /*
> + * Various gralloc implementations exist, but the dma-buf fd tends
> + * to be first. Access it directly to avoid a dependency on specific
> + * gralloc versions.
> + */
> + return (handle && handle->numFds) ? handle->data[0] : -1;
> +}
> +
> static int
> get_native_buffer_name(struct ANativeWindowBuffer *buf)
> {
> @@ -297,6 +336,85 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
> return EGL_TRUE;
> }
>
> +
> +static int
> +get_back_bo(struct dri2_egl_surface *dri2_surf)
> +{
> + struct dri2_egl_display *dri2_dpy =
> + dri2_egl_display(dri2_surf->base.Resource.Display);
> + int format;
> + int offset = 0, fd;
> + int name;
> +
> + if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
> + /* try to dequeue the next back buffer */
> + if (!dri2_surf->buffer && !droid_window_dequeue_buffer(dri2_surf))
> + return -1;
> +
> + /* free outdated buffers and update the surface size */
> + if (dri2_surf->base.Width != dri2_surf->buffer->width ||
> + dri2_surf->base.Height != dri2_surf->buffer->height) {
> + droid_free_local_buffers(dri2_surf);
> + dri2_surf->base.Width = dri2_surf->buffer->width;
> + dri2_surf->base.Height = dri2_surf->buffer->height;
> + }
> + }
> +
Might want to flesh this out into separate function. The same hunk is
in droid_get_buffers_with_format() already.
> + if(dri2_surf->buffer == NULL)
> + return -1;
> +
> + format = get_format(dri2_surf->buffer->format);
> +
> + fd = get_native_buffer_fd(dri2_surf->buffer);
> + if (fd >= 0) {
As we're missing the dri2_loader extension during init, this should
always be. If it's not ... things have gone horribly wrong.
> + int stride = dri2_surf->buffer->stride *
> + get_format_bpp(dri2_surf->buffer->format);
> + dri2_surf->dri_image =
> + dri2_dpy->image->createImageFromFds(dri2_dpy->dri_screen,
> + dri2_surf->base.Width,
> + dri2_surf->base.Height,
> + get_fourcc(format),
> + &fd,
> + 1,
> + &stride,
> + &offset,
> + dri2_surf);
> + return 0;
> + }
With the above said all that follows here should be nuked imho. Adding
dri2 fallbacks in image/dri3 code does not sound like a good idea.
> + name = get_native_buffer_name(dri2_surf->buffer);
> + if (name) {
> + dri2_surf->dri_image =
> + dri2_dpy->image->createImageFromName(dri2_dpy->dri_screen,
> + dri2_surf->base.Width,
> + dri2_surf->base.Height,
> + format,
> + name,
> + dri2_surf->buffer->stride,
> + dri2_surf);
> + return 0;
> + }
> + return -1;
> +}
> +
> +static int
> +droid_image_get_buffers(__DRIdrawable *driDrawable,
> + unsigned int format,
> + uint32_t *stamp,
> + void *loaderPrivate,
> + uint32_t buffer_mask,
> + struct __DRIimageList *images)
> +{
> + struct dri2_egl_surface *dri2_surf = loaderPrivate;
> +
> + if (get_back_bo(dri2_surf) < 0)
> + return 0;
> +
> + images->image_mask = __DRI_IMAGE_BUFFER_BACK;
> + images->back = dri2_surf->dri_image;
> +
> + return 1;
> +}
> +
> static EGLBoolean
> droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw)
> {
> @@ -325,13 +443,14 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw)
> }
>
> static _EGLImage *
> -dri2_create_image_android_native_buffer(_EGLDisplay *disp, _EGLContext *ctx,
> +dri2_create_image_android_native_buffer(_EGLDriver *drv, _EGLDisplay *disp,
> + _EGLContext *ctx,
> struct ANativeWindowBuffer *buf)
> {
> struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> struct dri2_egl_image *dri2_img;
> - int name;
> - EGLint format;
> + __DRIimage *dri_image;
> + int name, fd;
>
> if (ctx != NULL) {
> /* From the EGL_ANDROID_image_native_buffer spec:
> @@ -351,59 +470,54 @@ dri2_create_image_android_native_buffer(_EGLDisplay *disp, _EGLContext *ctx,
> return NULL;
> }
>
> + fd = get_native_buffer_fd(buf);
> + if (fd >= 0) {
Replying on get_native_buffer_fd feels strange. Imagine the following:
- libEGL capable of image/dri3/dmabuf
- dri module also capable.
- gralloc implementation not capable.
- libEGL and dri attempt to work with image/dri3... and suddenly
gralloc forces them into dri2. Then again not (m)any things are done
with it, plus both sides of the equation (predominantly the dri
module) has no idea that it has to fall back.
> + EGLint attr_list[14];
> + attr_list[0] = EGL_WIDTH;
> + attr_list[1] = buf->width;
> + attr_list[2] = EGL_HEIGHT;
> + attr_list[3] = buf->height;
> + attr_list[4] = EGL_LINUX_DRM_FOURCC_EXT;
> + attr_list[5] = get_fourcc(get_format(buf->format));
> + attr_list[6] = EGL_DMA_BUF_PLANE0_FD_EXT;
> + attr_list[7] = fd;
> + attr_list[8] = EGL_DMA_BUF_PLANE0_PITCH_EXT;
> + attr_list[9] = buf->stride * get_format_bpp(buf->format);
> + attr_list[10] = EGL_DMA_BUF_PLANE0_OFFSET_EXT;
> + attr_list[11] = 0;
> + attr_list[12] = EGL_NONE;
> +
Just throw these directly into a const attr_list[].
> + return dri2_create_image_khr(drv, disp, ctx, EGL_LINUX_DMA_BUF_EXT,
> + NULL, attr_list);
Having a close look at the helper and ... damn, is that a sight for sore eyes.
Some issues for posterity and/or anyone interested in sorting them out.
They are not something that you have to do in order to get the patch merged.
- dri2_create_image_from_dri() helper gives false premise(s) and
leaks. Just unwind it ?
- dri2_create_image_khr_texture_error() is called even when there is
no error to log.
- _eglInitImage() should return void
- missing error checking/error reporting/leaks after dri_image = foo()
- using the result from _eglParseImageAttribList() before checking
that is succeeds.
- _eglParseImageAttribList() already sets an eglError when needed. No
need to do it again in its caller.
> + }
> +
> name = get_native_buffer_name(buf);
> if (!name) {
> _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR");
> return NULL;
> }
>
> - /* see the table in droid_add_configs_for_visuals */
> - switch (buf->format) {
> - case HAL_PIXEL_FORMAT_BGRA_8888:
> - format = __DRI_IMAGE_FORMAT_ARGB8888;
> - break;
> - case HAL_PIXEL_FORMAT_RGB_565:
> - format = __DRI_IMAGE_FORMAT_RGB565;
> - break;
> - case HAL_PIXEL_FORMAT_RGBA_8888:
> - format = __DRI_IMAGE_FORMAT_ABGR8888;
> - break;
> - case HAL_PIXEL_FORMAT_RGBX_8888:
> - format = __DRI_IMAGE_FORMAT_XBGR8888;
> - break;
> - case HAL_PIXEL_FORMAT_RGB_888:
> - /* unsupported */
> - default:
> - _eglLog(_EGL_WARNING, "unsupported native buffer format 0x%x", buf->format);
> - return NULL;
> - break;
> - }
> + dri_image =
> + dri2_dpy->image->createImageFromName(dri2_dpy->dri_screen,
> + buf->width,
> + buf->height,
> + get_format(buf->format),
> + name,
> + buf->stride,
> + dri2_img);
>
> dri2_img = calloc(1, sizeof(*dri2_img));
> if (!dri2_img) {
> _eglError(EGL_BAD_ALLOC, "droid_create_image_mesa_drm");
> return NULL;
> }
> + dri2_img->dri_image = dri_image;
>
> if (!_eglInitImage(&dri2_img->base, disp)) {
> free(dri2_img);
> return NULL;
> }
>
> - dri2_img->dri_image =
> - dri2_dpy->image->createImageFromName(dri2_dpy->dri_screen,
> - buf->width,
> - buf->height,
> - format,
> - name,
> - buf->stride,
> - dri2_img);
> - if (!dri2_img->dri_image) {
> - free(dri2_img);
> - _eglError(EGL_BAD_ALLOC, "droid_create_image_mesa_drm");
> - return NULL;
> - }
> -
> return &dri2_img->base;
> }
>
> @@ -414,7 +528,7 @@ droid_create_image_khr(_EGLDriver *drv, _EGLDisplay *disp,
> {
> switch (target) {
> case EGL_NATIVE_BUFFER_ANDROID:
> - return dri2_create_image_android_native_buffer(disp, ctx,
> + return dri2_create_image_android_native_buffer(drv, disp, ctx,
> (struct ANativeWindowBuffer *) buffer);
> default:
> return dri2_create_image_khr(drv, disp, ctx, target, buffer, attr_list);
> @@ -661,6 +775,13 @@ static struct dri2_egl_display_vtbl droid_display_vtbl = {
> .get_dri_drawable = dri2_surface_get_dri_drawable,
> };
>
> +static const __DRIimageLoaderExtension droid_image_loader_extension = {
> + .base = { __DRI_IMAGE_LOADER, 1 },
> +
> + .getBuffers = droid_image_get_buffers,
> + .flushFrontBuffer = droid_flush_front_buffer,
> +};
> +
> EGLBoolean
> dri2_initialize_android(_EGLDriver *drv, _EGLDisplay *dpy)
> {
> @@ -702,9 +823,10 @@ dri2_initialize_android(_EGLDriver *drv, _EGLDisplay *dpy)
> droid_get_buffers_with_format;
>
> dri2_dpy->extensions[0] = &dri2_dpy->dri2_loader_extension.base;
As mentioned above, without the dri2_loader extension you will only
hit the image/dri3/dmabuf paths. At the same time, adding both won't
work ... or maybe it will.
Afaict as you're going to opening the card node, which partially
defeats the purpose of image/dri3/dmabuf as you will still need the
auth (guessing that you've tested it with the kernel hack ?). If I
misread the code and the render node is opened the dri2 path just
won't work.
Looking at the other backends - wayland does a related thing:
- Open a node
- Am I render node ? No, add the dri2_loader extension to the list.
> - dri2_dpy->extensions[1] = &image_lookup_extension.base;
> - dri2_dpy->extensions[2] = &use_invalidate.base;
> - dri2_dpy->extensions[3] = NULL;
> + dri2_dpy->extensions[1] = &droid_image_loader_extension.base;
> + dri2_dpy->extensions[2] = &image_lookup_extension.base;
> + dri2_dpy->extensions[3] = &use_invalidate.base;
> + dri2_dpy->extensions[4] = NULL;
>
Also we might want to get these (and the dri2_loader_extension) as
static (const) data. Again .. not something you have to do.
So all in all after scratching my head a few times and thinking "this
cannot be what I was a while back" and looking at Varad'd original
work... The work that you've done to get the code capable of graceful
fall-back, just doesn't work. IMHO that's an impossible task, after
the dri module has retrieved the image/dri2 loader extension. So I'd
suggesting keeping up Varad's work, modulo the odd cleanup/fix like
using dri2_create_image_dma_buf. In order to solve the build
permutation just add a workaround (envvar), that controls what was
once ifdef DMABUF. One can set it (the variable) automatically in the
android init scripts, based on the kernel module in the same way that
the gralloc provider is set. Looking at this patch and Varad's work
there a hunk missing here [1]. Did you not come across the issue in
question ?
Apologies that I'm the bearer of bad news and thank you for working on this.
Emil
[1] https://github.com/varadgautam/mesa/commit/96c533fd62db4bb21f0a778ad16848da2df24fd6
More information about the mesa-dev
mailing list