[Mesa-dev] ANDROID: eglCreateImageKHR missing modifiers

[Tomasz Figa]

On Tue, Jul 24, 2018 at 10:25 PM Martin Fuzzey
<martin.fuzzey at flowbird.group> wrote:
>
> Hi Thomasz,
>
> thanks for your reply
>
> On 21/07/18 04:27, Tomasz Figa wrote:
> >
> > As you noticed, this adds back the dependency on gralloc handle
> > structure. Moreover, it actually adds a dependency on the handle
> > having a binary format compatible with gralloc_gbm_handle_t. This is
> > not something that we can do in upstream Mesa, since there are more
> > platforms running gralloc implementations other than gbm_gralloc.
>
> Looking at it a bit more is it really that bad?
>
> The compile time dependency is on gralloc_handle.h which is actually
> part of libdrm not a specific gralloc implementation.
> Does mesa need to compile with old versions of libdrm?
> 0r is the structure going to be removed from libdrm?
>
> As you say there is also the runtime dependency on the binary format of
> the handle structure *but* checking the magic will ensure that if we run
> with a different implementation of gralloc which doesn't have the same
> magic we will gracefully fall back to not supporting modifiers, just as
> we would with a new optional gralloc perform call.
> We should probably also check the numFds, numInts field in the
> native_handle_t header to ensure that we can safely access the magic
> field to protect against any other gralloc implementations having a tiny
> handle format.
>

Alright, if we ensure that we can safely check for the right handle
format at runtime then I don't have anything against it.

Obviously it would be much better if we could have a generic way to
handle this, but given the Android way of doing things it doesn't
sound likely.

Best regards,
Tomasz

>
> >>
> >> --- a/src/egl/drivers/dri2/platform_android.c
> >> +++ b/src/egl/drivers/dri2/platform_android.c
> >> @@ -43,6 +43,8 @@
> >>    #include "gralloc_drm.h"
> >>    #endif /* HAVE_DRM_GRALLOC */
> >>
> >> +#include <gralloc_handle.h>
> >> +
> >>    #define ALIGN(val, align)      (((val) + (align) - 1) & ~((align) - 1))
> >>
> >>    struct droid_yuv_format {
> >> @@ -801,6 +803,9 @@ droid_create_image_from_prime_fd(_EGLDisplay *disp,
> >> _EGLContext *ctx,
> >>                                     struct ANativeWindowBuffer *buf, int fd)
> >>    {
> >>       unsigned int pitch;
> >> +   struct gralloc_gbm_handle_t *grh;
> >> +   uint64_t modifier = 0;
> >> +   bool have_modifier = false;
> >>
> >>       if (is_yuv(buf->format)) {
> >>          _EGLImage *image;
> >> @@ -829,17 +834,34 @@ droid_create_image_from_prime_fd(_EGLDisplay
> >> *disp, _EGLContext *ctx,
> >>          return NULL;
> >>       }
> >>
> >> -   const EGLint attr_list[] = {
> >> +   grh = (struct gralloc_gbm_handle_t *)buf->handle;
> >> +   if (grh->magic == GRALLOC_HANDLE_MAGIC) {
> >> +      modifier = grh->modifier;
> >> +      have_modifier = true;
> >> +   }
> >> +
> >> +   EGLint attr_list[] = {
> >>          EGL_WIDTH, buf->width,
> >>          EGL_HEIGHT, buf->height,
> >>          EGL_LINUX_DRM_FOURCC_EXT, fourcc,
> >>          EGL_DMA_BUF_PLANE0_FD_EXT, fd,
> >>          EGL_DMA_BUF_PLANE0_PITCH_EXT, pitch,
> >>          EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0,
> >> +      EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT, modifier & 0xffffffff,
> >> +      EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT, modifier >> 32,
> >>          EGL_NONE, 0
> >>       };
> >>
> >> -   return dri2_create_image_dma_buf(disp, ctx, NULL, attr_list);
> >> +   if (!have_modifier) {
> >> +      for (int i=0; i < ARRAY_SIZE(attr_list); i+=2) {
> >> +         if (attr_list[i] == EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT) {
> >> +            attr_list[i] = EGL_NONE;
> >> +            break;
> >> +         }
> >> +      }
> >> +   }
> >> +
> >> +   return dri2_create_image_dma_buf(disp, ctx, NULL, (const EGLint
> >> *)attr_list);
> >>    }
> >>
> >> What is the preferred way of fixing this?
> > Unfortunately, I don't have a very good solution for this. In Chrome
> > OS we just don't support modifiers in EGL/GLES on Android. Obviously
> > it would be a good idea to have some way to query gralloc for the
> > modifier, but I don't see Android providing any generic way to do it.
> >
> > The least evil I can think of as a makeshift for now could be an
> > optional gralloc0 perform call that returns the modifier for given
> > buffer.
>
> An advantage of a new perform call would be that other gralloc
> implementations could be extended to support it without changing their
> handle structure but is that really likely?
>
> Regards,
>
> Martin
>