[Mesa-dev] [PATCH 3/3] anv: Support loader interface version 2

[Emil Velikov]

On 11 January 2017 at 03:59, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Tue, Jan 10, 2017 at 7:17 PM, Chad Versace <chadversary at chromium.org>
> wrote:
>>
>> Loader interface v2 differs from v1 in that the first ICD entrypoint
>> called by the loader is vk_icdNegotiateLoaderICDInterfaceVersion(), not
>> vk_icdGetInstanceProcAddr(). The ICD must statically expose this
>> entrypoint.
>> ---
>>  src/intel/vulkan/anv_device.c | 43
>> +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 43 insertions(+)
>>
>> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
>> index 219c611408..89e427f342 100644
>> --- a/src/intel/vulkan/anv_device.c
>> +++ b/src/intel/vulkan/anv_device.c
>> @@ -2034,3 +2034,46 @@ void anv_DestroyFramebuffer(
>>
>>     vk_free2(&device->alloc, pAllocator, fb);
>>  }
>> +
>> +/* Suppress warnings because vk_icd.h does not declare this function.
>> + * Clang supports this pragma too.
>> + */
>> +#pragma GCC diagnostic push
>> +#pragma GCC diagnostic ignored "-Wmissing-prototypes"

Since neither new nor old headers provide a declaration I'd just add
one here, add a comment "XXX: vk_icd.h does not provide the
declaration of ..." and drop the pragmas ?

>> +PUBLIC VKAPI_ATTR VkResult VKAPI_CALL
>> +vk_icdNegotiateLoaderICDInterfaceVersion(uint32_t* pSupportedVersion)
>> +{
>> +   /* For the full details on loader interface versioning, see
>> +    *
>> <https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/blob/master/loader/LoaderAndLayerInterface.md>.
>> +    * What follows is a condensed summary, to help you navigate the large
>> and
>> +    * confusing official doc.
>> +    *
>> +    *   - Loader interface v0 is incompatible with later versions. We
>> don't
>> +    *     support it.
>> +    *
>> +    *   - In loader interface v1:
>> +    *       - The first ICD entrypoint called by the loader is
>> +    *         vk_icdGetInstanceProcAddr(). The ICD must statically expose
>> this
>> +    *         entrypoint.
>> +    *       - The ICD must statically expose no other Vulkan symbol
>> unless it is
>> +    *         linked with -Bsymbolic.
>> +    *       - Each dispatchable Vulkan handle created by the ICD must be
>> +    *         a pointer to a struct whose first member is VK_LOADER_DATA.
>> The
>> +    *         ICD must initialize VK_LOADER_DATA.loadMagic to
>> ICD_LOADER_MAGIC.
>> +    *       - The loader implements vkCreate{PLATFORM}SurfaceKHR() and
>> +    *         vkDestroySurfaceKHR(). The ICD must be capable of working
>> with
>> +    *         such loader-managed surfaces.
>> +    *
>> +    *    - Loader interface v2 differs from v1 in:
>> +    *       - The first ICD entrypoint called by the loader is
>> +    *         vk_icdNegotiateLoaderICDInterfaceVersion(). The ICD must
>> +    *         statically expose this entrypoint.
>> +    *
>> +    *    - Loader interface v3 differs from v2 in:
>> +    *        - The ICD must implement vkCreate{PLATFORM}Surface() and
>> +    *          vkDestroySurface() because the loader no longer does so.
Nit: "...SurfaceKHR, vkDestroySurfaceKHR() and other API which uses
KHR_surface ..."

>
>
> We've implemented these since the dawn of time.  I think we can claim v3.
>
I believe you're spot on. We're missing support for some extensions
(VK_KHR_display, VK_KHR_{android,mir,win32}_surface) and thus the
respective API, but that's orthogonal.

>>
>> +    */
>> +   *pSupportedVersion = 2;
Since we should advertise the lowest version common across both loader
and ICD, the following will be the more robust solution ?

 *pSupportedVersion = MIN(*pSupportedVersion, 3);


Reading through the v3 changes, I'm leaning that we want the series in
-stable, since a) it's not new functionality b) things might end up
pretty nasty as one mixes old loader + new icd and vise-versa.
What do you guys thing ?

Thanks
Emil