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

[Emil Velikov]

On 13 September 2017 at 22:15, Chad Versace <chadversary at chromium.org> wrote:
>> > +
>> > +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.
>
Right, my bad.

>> > +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.
>
Right. I'm a little concerned since the HALs contain a bunch of
function pointers, which one can override and perhaps abuse. That's a
topic for another day though.

>
>>
>> > +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.
>
Apologies, did not mean to rock the boat :-\

> By the way, I don't know what sentinel you refer to here. I don't see
> a sentinel.
>
My lack of formal CS education (Electronics Engineer here) shows...

>
>> > +         {
>> > +            .sType = VK_STRUCTURE_TYPE_SUBMIT_INFO,
>> > +            .waitSemaphoreCount = 1,
>> > +            .pWaitSemaphores = pWaitSemaphores,
>> > +         },
>> > +      },

>> > +      (VkFence) VK_NULL_HANDLE);
... I meant this instance here.

Thanks
Emil