[Mesa-dev] [PATCH 00/21] anv: Follow the rules for vkGet*ProcAddr

[Samuel Iglesias Gonsálvez]

On 23/01/18 09:01, Jason Ekstrand wrote:
> On Mon, Jan 22, 2018 at 11:46 PM, Samuel Iglesias Gonsálvez
> <siglesias at igalia.com <mailto:siglesias at igalia.com>> wrote:
>
>     I have comments on patch 2 and 18 (below). For the rest,
>
>     Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>     <mailto:siglesias at igalia.com>
>
>     On 20/01/18 20:11, Jason Ekstrand wrote:
>>     Our previous scheme for Get*ProcAddr was to just return what we could and
>>     not care about the details.  This meant that GetInstanceProcAddr returned
>>     all anv_ entrypoints and GetDeviceProcAddr would return the per-gen
>>     entrypoint and fall back to anv_.  We figured that this was a perfectly
>>     reasonable and Vulkan thing to do and that the loader could sort out the
>>     nasty details.  We were wrong.
>>
>>     The Vulkan spec has some very specific rules about what vkGet*ProcAddr is
>>     supposed to do in various cases.  In particular, you're supposed to return
>>     NULL for any extension entrypoints which have not explicitly been enabled.
>>     Also, vkGetInstanceProcAddr is supposed to return entrypoints for device
>>     functionality even if they have to be trampoline entrypoints.  In 99% of
>>     case, the loader takes care of all these details for us.  However, what I
>>     hear from the loader people is that they can't do it all and that the
>>     drivers should also follow the rules.
>>
>>     On the upside, this means that our driver, short of exposing a few symbols,
>>     is now a completely stand-alone Vulkan implementation and doesn't require a
>>     loader.
>>
>>     Cc: Samuel Iglesias Gonsálvez <siglesias at igalia.com> <mailto:siglesias at igalia.com>
>>
>>     Jason Ekstrand (21):
>>       anv/meson: Make anv_entrypoints_gen.py depend on anv_extensions.py
>>       anv: Split anv_extensions.py into two files
>>       anv/meson: Simplify some dependency and flag tracking
>>       anv/extensions: Generate a header file with extension tables
>>       anv: Use tables for instance extension wrangling
>>       anv: Add a per-instance table of enabled extensions
>>       anv: Use tables for device extension wrangling
>>       anv: Add a per-device table of enabled extensions
>>       anv/entrypoints: Add an Entrypoint class
>>       anv/entrypoints: Add a LAYERS helper variable
>>       anv/entrypoints: Split entrypoint index lookup into its own function
>>       anv/entrypoints: Expose the different dispatch tables
>>       anv/entrypoints: Parse entrypoints before extensions/features
>>       anv/extensions: Fix VkVersion::c_vk_version for patch == None
>>       anv: Properly NULL for GetInstanceProcAddr with a null instance
>>       anv: Add a per-instance dispatch table
>>       anv: Add a per-device dispatch table
>>       anv: Only advertise enabled entrypoints
>
>     In this patch (patch 18), I think there is an error in
>     anv_entrypoint_is_enabled() :
>
>     + return !device || device->${e.extension.name
>     <http://e.extension.name>[3:]};
>
>     As it is expected anv_entrypoint_is_enabled() to return false for
>     disabled extensions, I think this should be: return device &&
>     device->${e.extension.name <http://e.extension.name>[3:]};
>
>     Am I missing something?
>
>
> Yeah.  Vulkan requires that GetInstanceProcAddr returns a pointer for
> any device extensions which may be available.  For
> GetInstanceProcAddr, we pass NULL in for device and this will mean
> that we return true for all device extensions.  For GetDeviceProcAddr,
> device is non-NULL and we return true if and only if the extension is
> enabled.
>  
OK, thanks. Then, you have my R-b for this patch and for patch 2 once
you fix the build with autotools.

Sam

>     Sam
>
>>       anv/entrypoints: Use an named tuple for params
>>       anv: Return trampoline entrypoints from GetInstanceProcAddr
>>       HACK: Return instance entrypoints from GetDeviceProcAddr
>>
>>      src/intel/Makefile.sources              |   3 +-
>>      src/intel/Makefile.vulkan.am <http://Makefile.vulkan.am>            |  15 +-
>>      src/intel/vulkan/anv_device.c           | 174 ++++++++++++++++++-
>>      src/intel/vulkan/anv_entrypoints_gen.py | 297 +++++++++++++++++++++++---------
>>      src/intel/vulkan/anv_extensions.py      | 157 +----------------
>>      src/intel/vulkan/anv_extensions_gen.py  | 202 ++++++++++++++++++++++
>>      src/intel/vulkan/anv_private.h          |  16 +-
>>      src/intel/vulkan/meson.build            |  50 ++++--
>>      8 files changed, 653 insertions(+), 261 deletions(-)
>>      create mode 100644 src/intel/vulkan/anv_extensions_gen.py
>>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180123/c1b086f2/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180123/c1b086f2/attachment-0001.sig>