<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> I have comments on patch 2 and 18 (below). For the rest, Reviewed-by: Samuel Iglesias Gonsálvez <a class="moz-txt-link-rfc2396E" href="mailto:siglesias@igalia.com"><siglesias@igalia.com></a> On 20/01/18 20:11, Jason Ekstrand wrote: <blockquote type="cite" cite="mid:1516475516-14033-1-git-send-email-jason.ekstrand@intel.com"> <pre wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:siglesias@igalia.com"><siglesias@igalia.com></a> 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</pre> </blockquote> 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? Sam <blockquote type="cite" cite="mid:1516475516-14033-1-git-send-email-jason.ekstrand@intel.com"> <pre wrap=""> 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 </pre> </blockquote> </body> </html>