On 23/01/18 09:01, Jason Ekstrand wrote: > On Mon, Jan 22, 2018 at 11:46 PM, Samuel Iglesias Gonsálvez > <[email protected] <mailto:[email protected]>> wrote: > > I have comments on patch 2 and 18 (below). For the rest, > > Reviewed-by: Samuel Iglesias Gonsálvez <[email protected]> > <mailto:[email protected]> > > 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 <[email protected]> >> <mailto:[email protected]> >> >> 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 >> > >
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
