[Mesa-dev] [PATCH] egl: android: add dma-buf fd support
Rob Herring
robh at kernel.org
Wed Apr 20 02:52:34 UTC 2016
On Tue, Apr 19, 2016 at 8: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 ?
Okay.
> 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.
For the GEM handle case, I just tried not to change things. I think
the android-x86 folks have tested a similar version of this and they
would use the GEM handle case.
[...]
>> + 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.
Okay, I'm not sure why Varad added it then.
>> + 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.
What exactly determines libEGL and dri module are dmabuf capable? The
image loader extension?
>
>> + 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[].
How can it be const when most of the values are variables?
[...]
>> @@ -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.
So dri2_loader and image loader are mutually exclusive?
> 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.
gralloc opens the render node and passes the fd to mesa. The old
gralloc that android-x86 is still using (for the KMS part) uses the
card node.
> 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.
Okay. Could I key off of that for dma-buf support as well?
>
>> - 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.
I'm not really a fan of adding yet another thing to Android device
config that has to be set correctly or things fall over. There are
enough already. Surely, the code can be smart enough to do the right
thing based on which node or drv PRIME support.
> Looking at this patch and Varad's work
> there a hunk missing here [1]. Did you not come across the issue in
> question ?
I don't think it is a real issue. I traced the code for
createNewDrawable and it only saves dri2_surf ptr, but never
references the contents of it. And I didn't find any problems when
reverting it and testing, so I dropped that patch.
>
> Apologies that I'm the bearer of bad news and thank you for working on this.
Apologies for pretending I know what I'm doing. :)
Rob
More information about the mesa-dev
mailing list