[Mesa-dev] [PATCH] Fix Android 5.1 build and runtime issues

[Bish, Jim]

On 07/28/2015 02:38 PM, Chad Versace wrote:
> On Tue 28 Jul 2015, Emil Velikov wrote:
>> Hello Jim,
>>
>> On 28 July 2015 at 02:57, Bish, Jim <jim.bish at intel.com> wrote:
>>> From: Jim Bish <jim.bish at intel.com>
>>>
>> Would you mind splitting this into separate patches ? Adding a few
>> words in the commit log(s) would be highly preferable.
> 
> Yes, please submit separate patches. "One patch does one thing, and does
> that one thing well".

working on update.  will get it out as soon as I can.  
> 
>>
>>> ---
>>>  Android.common.mk                           | 10 ++++++++++
>>>  Android.mk                                  |  3 +++
>>>  src/mesa/Android.libmesa_dricore.mk         |  2 +-
>>>  src/mesa/drivers/dri/i965/intel_fbo.c       |  6 +++++-
>>>  src/mesa/drivers/dri/i965/intel_tex_image.c |  4 +++-
>>>  5 files changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Android.common.mk b/Android.common.mk
>>> index d662d60..68c2e1b 100644
>>> --- a/Android.common.mk
>>> +++ b/Android.common.mk
>>> @@ -76,6 +76,16 @@ LOCAL_CFLAGS += \
>>>         -D__STDC_LIMIT_MACROS
>>>  endif
>>>
>>> +# add libdrm if there are hardware drivers
>>> +ifneq ($(filter-out swrast,$(MESA_GPU_DRIVERS)),)
>>> +LOCAL_CFLAGS += -DHAVE_LIBDRM
>>
>>> +LOCAL_CFLAGS += \
>>> +        -I$(DRM_TOP)/include/drm \
>>> +        -I$(DRM_TOP) \
>>> +        -I$(DRM_GRALLOC_TOP)
> 
>> The above includes should _not_ be needed. Please make sure that
>> LOCAL_EXPORT_C_INCLUDE_DIRS for libdrm and friends are set correctly.
>> The former two are addressed upstream [1] while there is a patch for
>> the last one on mesa-dev (as there is no upstream for drm_gralloc) [2]
will look at the incs again.  
>>
>> Do you know of any plans to use upstream libdrm ? Or at least
>> rebase/merge with upstream changes ?
would be nice but as far as I know there is not plan to update.  Perhaps 
they will for security or some other compelling reason.  BTW, for the Nexus player
project we had to push a completely separate libdrm to AOSP.  Really bad but we were forced 
to do that.
>>
>>
>>> +LOCAL_SHARED_LIBRARIES += libdrm
>>> +endif
>>> +
> 
> I'm not an Android.mk expert, but this feels too high-level to add
> libdrm to LOCAL_SHARED_LIBRARIES. I suspect this should be done in the
> i965 directory instead.
> 
>> The rest of the hunk seems ok, but please mention why in the commit log.
>>
>>>  LOCAL_CPPFLAGS += \
>>>         $(if $(filter true,$(MESA_LOLLIPOP_BUILD)),-D_USING_LIBCXX) \
>>>         -Wno-error=non-virtual-dtor \
>>> diff --git a/Android.mk b/Android.mk
>>> index ed160fb..8f523bb 100644
>>> --- a/Android.mk
>>> +++ b/Android.mk
>>> @@ -45,6 +45,9 @@ endif
>>>  MESA_COMMON_MK := $(MESA_TOP)/Android.common.mk
>>>  MESA_PYTHON2 := python
>>>
>>> +DRM_TOP := external/drm
>>> +DRM_GRALLOC_TOP := hardware/drm_gralloc
>>> +
>> Please don't. See above for details.
>>
>>>  classic_drivers := i915 i965
>>>  gallium_drivers := swrast freedreno i915g ilo nouveau r300g r600g radeonsi vmwgfx vc4
>>>
>>> diff --git a/src/mesa/Android.libmesa_dricore.mk b/src/mesa/Android.libmesa_dricore.mk
>>> index 2e308b8..fef76c8 100644
>>> --- a/src/mesa/Android.libmesa_dricore.mk
>>> +++ b/src/mesa/Android.libmesa_dricore.mk
>>> @@ -50,7 +50,7 @@ endif # MESA_ENABLE_ASM
>>>  ifeq ($(ARCH_X86_HAVE_SSE4_1),true)
>>>  LOCAL_SRC_FILES += \
>>>         main/streaming-load-memcpy.c \
>>> -       mesa/main/sse_minmax.c
>>> +       main/sse_minmax.c
>>>  LOCAL_CFLAGS := \
>>>         -msse4.1 \
>>>         -DUSE_SSE41
>>> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c b/src/mesa/drivers/dri/i965/intel_fbo.c
>>> index 87ba624..e6cdd76 100644
>>> --- a/src/mesa/drivers/dri/i965/intel_fbo.c
>>> +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
>>> @@ -355,13 +355,17 @@ intel_image_target_renderbuffer_storage(struct gl_context *ctx,
>>>                                               screen->loaderPrivate);
>>>     if (image == NULL)
>>>        return;
>>> -
>>> +#ifndef HAVE_ANDROID_PLATFORM
>>> +   /*
>>> +    * Disable this check for Android
>>> +    */
>>>     if (image->planar_format && image->planar_format->nplanes > 1) {
>>>        _mesa_error(ctx, GL_INVALID_OPERATION,
>>>              "glEGLImageTargetRenderbufferStorage(planar buffers are not "
>>>                 "supported as render targets.");
>>>        return;
>>>     }
>>> +#endif
>> Imho this should be a separate patch, with the commit and/or comment
>> providing more information.
> 
> This should be in a separate patch. But...
> 
> You can't disable this check! Not without a heavy justification. If an
> application binds a planar EGLImage as renderbuffer storage, does i965
> correctly translate and scatter the fragment shader's output color to
> the separate planes?  If not, what *does* i965 do?
> 
> Add some comments explaining that issue in the patch.
> 
>>
>>>
>>>     /* __DRIimage is opaque to the core so it has to be checked here */
>>>     switch (image->format) {
>>> diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c b/src/mesa/drivers/dri/i965/intel_tex_image.c
>>> index 3611280..4b3e9cf 100644
>>> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
>>> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
>>> @@ -317,8 +317,10 @@ intel_image_target_texture_2d(struct gl_context *ctx, GLenum target,
>>>     if (image == NULL)
>>>        return;
>>>
>>> +#ifndef HAVE_ANDROID_PLATFORM
>>>     /* We support external textures only for EGLImages created with
>>>      * EGL_EXT_image_dma_buf_import. We may lift that restriction in the future.
>>> +    * We have to remove this restriction for Android builds.
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Why? That deserves an explanation.
> 
>>>      */
>>>     if (target == GL_TEXTURE_EXTERNAL_OES && !image->dma_buf_imported) {
>>>        _mesa_error(ctx, GL_INVALID_OPERATION,
>>> @@ -326,7 +328,7 @@ intel_image_target_texture_2d(struct gl_context *ctx, GLenum target,
>>>                 "for images created with EGL_EXT_image_dma_buf_import");
>>>        return;
>>>     }
>>> -
>>> +#endif
> 
>> Ditto.
>>
>> Chad,
>> I was under the impression that both restrictions were lifted with
>> your EGL_EXT_image_dma_buf_import
>> /OES_EGL_image_external work. Would you know, off hand, what is still missing ?
> 
> I didn't lift all restrictions. I lifted the restriction that
> prevented glEGLImageTargetRenderbufferStorage and
> glEGLImageTargetTexture2D from using non-planar dma_buf-backed EGLImages
> as storage. In other words, you can now import an RGB EGLImage, render
> to it, and texture from it. See commit a76dc15b2b3.
> 
> I did not alter restrictions related to planar formats.
>