[Mesa-dev] ANDROID: eglCreateImageKHR missing modifiers

[Martin Fuzzey]

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.


>>
>> --- 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