<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jan 22, 2018 at 11:46 PM, Samuel Iglesias Gonsálvez <span dir="ltr"><<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
I have comments on patch 2 and 18 (below). For the rest,<br>
<br>
Reviewed-by: Samuel Iglesias Gonsálvez <a class="m_-9087214539823618488moz-txt-link-rfc2396E" href="mailto:siglesias@igalia.com" target="_blank"><siglesias@igalia.com></a><span class=""><br>
<br>
On 20/01/18 20:11, Jason Ekstrand wrote:<br>
</span><div><div class="h5"><blockquote type="cite">
<pre>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="m_-9087214539823618488moz-txt-link-rfc2396E" href="mailto:siglesias@igalia.com" target="_blank"><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>
<br></div></div>
In this patch (patch 18), I think there is an error in
anv_entrypoint_is_enabled() :<br>
<br>
+ return !device || device->${<a href="http://e.extension.name" target="_blank">e.extension.name</a>[3:]<wbr>};<br>
<br>
As it is expected anv_entrypoint_is_enabled() to return false for
disabled extensions, I think this should be: return device
&& device->${<a href="http://e.extension.name" target="_blank">e.extension.name</a>[3:]<wbr>};<br>
<br>
Am I missing something?<br></div></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
Sam<span class=""><br>
<br>
<blockquote type="cite">
<pre> 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/<a href="http://Makefile.vulkan.am" target="_blank">Makefile.vulkan.am</a> | 15 +-
src/intel/vulkan/anv_device.c | 174 ++++++++++++++++++-
src/intel/vulkan/anv_<wbr>entrypoints_gen.py | 297 +++++++++++++++++++++++-------<wbr>--
src/intel/vulkan/anv_<wbr>extensions.py | 157 +----------------
src/intel/vulkan/anv_<wbr>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_<wbr>extensions_gen.py
</pre>
</blockquote>
<br>
</span></div>
</blockquote></div><br></div></div>