[Mesa-dev] [PATCH 23/23] anv: Implement VK_ANDROID_native_buffer

[Chad Versace]

On Wed 13 Sep 2017, Emil Velikov wrote:
> Hi Chad,
> 
> On the topic of keep this in the driver vs separate module - I think
> it makes sense to keep it internal.
> 
> Sure radv/others won't be able to reuse the code [that easily], at the
> same time it gives greater control and one could make more optimal
> decisions.
> 
> On 2 September 2017 at 09:17, Chad Versace <chadversary at chromium.org> wrote:
> 
> > diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources
> > index f6a69f65455..e4a371cbe3f 100644
> > --- a/src/intel/Makefile.sources
> > +++ b/src/intel/Makefile.sources
> > @@ -228,6 +228,9 @@ VULKAN_FILES := \
> >         vulkan/anv_wsi.c \
> >         vulkan/vk_format_info.h
> >
> > +VULKAN_ANDROID_FILES := \
> > +       vulkan/anv_android.c
> > +
> >  VULKAN_WSI_WAYLAND_FILES := \
> >         vulkan/anv_wsi_wayland.c
> >
> > diff --git a/src/intel/Makefile.vulkan.am b/src/intel/Makefile.vulkan.am
> > index d1b1132ed2e..bc4b12768ad 100644
> > --- a/src/intel/Makefile.vulkan.am
> > +++ b/src/intel/Makefile.vulkan.am
> > @@ -147,8 +147,12 @@ VULKAN_LIB_DEPS = \
> >         -lm
> >
> >  if HAVE_PLATFORM_ANDROID
> > +VULKAN_CPPFLAGS += \
> > +       -I$(top_srcdir)/include/android \
> > +       $(ANDROID_CPPFLAGS)
> >  VULKAN_CFLAGS += $(ANDROID_CFLAGS)
> >  VULKAN_LIB_DEPS += $(ANDROID_LIBS)
> > +VULKAN_SOURCES += $(VULKAN_ANDROID_FILES)
> >  endif
> >
> I think we'll need a simialar hunk for Android. Once people are
> settled on the approach myself or Tapani can propose something.
> 
> 
> >  if HAVE_PLATFORM_X11
> > diff --git a/src/intel/vulkan/anv_android.c b/src/intel/vulkan/anv_android.c
> > new file mode 100644
> > index 00000000000..24274510cd6
> > --- /dev/null
> > +++ b/src/intel/vulkan/anv_android.c
> 
> > +#include <vulkan/vk_icd.h>
> 
> > +
> > +static void UNUSED
> > +static_asserts(void)
> > +{
> > +   STATIC_ASSERT(HWVULKAN_DISPATCH_MAGIC == ICD_LOADER_MAGIC);
> > +}

> Seems like a left over? With that gone, there should be no need for vk_icd.h.

I intended to leave this in because throughout anvil we do

  api_object->_loader_data.loaderMagic = ICD_LOADER_MAGIC

and that needs to work correctly even on Android. The static assertion
here will give us ample warning if the two values ever change.

> > +PUBLIC struct hwvulkan_module_t HAL_MODULE_INFO_SYM = {
> Unrelated: any ideas why these are not const? All the HAL exports I've
> seen always lack the notation.

Surprisingly, the HAL API expects it to be non-const in
hw_module_method_t. The 'device' output parameter to
hw_module_method_t::open() is non-const, and it contains a reference to
hw_module_t. Declaring HAL_MODULE_INFO_SYM as const produces compiler
warnings (or errors, depending on CFLAGS) in the body of anv_hal_open().

As precedent, Chrome OS's gralloc HAL (not written by me) also declares
HAL_MODULE_INFO_SYM as non-const.


> 
> > +VkResult
> > +anv_QueueSignalReleaseImageANDROID(
> > +      VkQueue             queue,
> > +      uint32_t            waitSemaphoreCount,
> > +      const VkSemaphore*  pWaitSemaphores,
> > +      VkImage             image,
> > +      int*                pNativeFenceFd)
> > +{
> > +   VkResult result;
> > +
> > +   if (waitSemaphoreCount == 0)
> > +      goto done;
> > +
> > +   result = anv_QueueSubmit(queue,
> > +      1, (VkSubmitInfo[]) {
> Nit: Please stay consistent with other anv_QueueSubmit users.
> 
> Namely, move "1," on the previous line and use a pointer
> &(VkSubmitInfo), dropping the sentinel.

Huh. My style used to be the dominant style in anvil, before the meta
paths were deleted. And it continues to be pervasive in radv.

I don't care right now, though. I'll change it.

By the way, I don't know what sentinel you refer to here. I don't see
a sentinel.


> > +         {
> > +            .sType = VK_STRUCTURE_TYPE_SUBMIT_INFO,
> > +            .waitSemaphoreCount = 1,
> > +            .pWaitSemaphores = pWaitSemaphores,
> > +         },
> > +      },
> > +      (VkFence) VK_NULL_HANDLE);
> 
> 
> > @@ -1440,10 +1440,20 @@ VkResult anv_AllocateMemory(
> >     struct anv_device_memory *mem;
> >     VkResult result = VK_SUCCESS;
> >
> > +   /* VK_ANDROID_native_buffer defines VkNativeBufferANDROID as an extension
> > +    * of VkImageCreateInfo. We abuse the struct by chaining it to
> > +    * VkMemoryAllocateInfo in the implementation of vkCreateImage.
> > +    */
> > +   const VkNativeBufferANDROID *gralloc_info = NULL;
> > +#ifdef ANDROID
> 
> On one hand I'm thinking that we could drop the ifdef guard here and
> the rest of the patch.
> At the same time any solution will lead to a few annoying issues:
> 
>  - The -I$(top_srcdir)/include/android is Android only -> breaking the
> !Android builds

The -I line was a mistake remaining from an old branch. It doesn't break
the build, though. The compiler silently ignores -I dirs that don't
exist.

>  - The header within ^^ is unconditionally included

Yes, I intentionally included the header unconditionally. It reduces the
#ifdef mess in the driver.

I'm sending v2 now, which changes some of this.

> The above two can be resolved trivially by
>  a) conditionally including the header, or
>  b) making the -I unconditional
> 
> That in itself will lead to other issues:
> a)
>  - VkNativeBufferANDROID is unknown at combile time - WA with redeclaration
> Leading to build warnings since that's a C11 feature - Matt fixed a
> few warnings like that recently.
> 
> b)
>  - missing headers - system/window.h at least (from vk_android_native_buffer.h)
> 
> 
> Personal preference is to:
>  - conditionally include the header, providing a fallback redeclaration
>  - silence the compiler to not emit warnings
> 
> Then we could even drop all (but one) of the ifdef ANDROID guards ;-)
> 
> How does that sound?
> Emil
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev