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

[Emil Velikov]

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.

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

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


> +         {
> +            .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 header within ^^ is unconditionally included
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