[Mesa-dev] [PATCH] egl: android: add dma-buf fd support
Rob Clark
robdclark at gmail.com
Wed Apr 20 01:43:55 UTC 2016
On Tue, Apr 19, 2016 at 9:03 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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.
Just a drive-by comment, but seems like most of the main drivers (x86
and arm, minus perhaps the x86 server gpus, which are probably not
interesting in this context) support DRIVER_PRIME + DRIVER_RENDER..
So possibly we don't need to keep support for a runtime decision.. and
almost certainly all the drivers that support atomic (which I think is
required for what Rob is working on) support prime+render, so in the
worst case we could have a compile time decision between "old world"
and "new world"?
It would ofc be good to make sure we don't break things for
android-x86, but I think they would stick on dri2 + pre-atomic state
w/ compile time build decisions until enough of the desktop gpu's
support atomic.. although maybe it helps to switch them over to
prime+render without switching to gbm_gralloc and kms hwc stuff yet?
Not sure.. but at least seems like a possibility if it makes things
easier on the mesa side..
BR,
-R
>> 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list