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

[Jason Ekstrand]

On Mon, Jan 22, 2018 at 11:46 PM, Samuel Iglesias Gonsálvez <
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>
> <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> <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[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[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.


> 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            |  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/059e1cb3/attachment.html>