On Tue, Apr 19, 2016 at 9:03 PM, Emil Velikov <[email protected]> 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 <[email protected]> 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 <[email protected]> >> Cc: Rob Clark <[email protected]> >> Cc: Emil Velikov <[email protected]> >> Signed-off-by: Rob Herring <[email protected]> >> --- >> 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 > [email protected] > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
