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 <chadvers...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
