<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Mar 6, 2018 at 8:02 AM, Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, 2018-03-05 at 12:11 +0000, Emil Velikov wrote: > Hi Iago, > > Top level questions: > > I think this and the original commit should go to stable right? I am not sure if this qualifies for stable: these patches don't fix any user-visible bugs. If an application was calling vkGetDeviceProcAddr to get pointers to non-device functions (which is incorrect by the spec) the previous behavior would allow it to get away with it without issues, bit with these patches it will start to crash since it will receive NULL pointers. > The dispatch in 17.3 is very different making, yet 18.0 should be > perfectly fine. > > Mildly related - anv is missing a special case for following three > extensions. Should we port those over from radv? > > vkCreateInstance vkEnumerateInstanceExtensionProperties > vkEnumerateInstanceLayerProperties What is the exception exactly? Right now we report NULL for these from vkGetDeviceProcAddr which is the right thing to do. </blockquote><div> </div><div>He's talking about the 3 instance commands that we should return in the GetInstanceProcAddr even if instance is NULL, but anv handles that inside anv_device.c anyway, so I don't think there is anything to be done there. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > Bas, you might want to port these over? > > On 5 March 2018 at 10:46, Iago Toral Quiroga <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> > wrote: > > af5f2322d0c64 addressed this for extension commands, but the spec > > mandates > > this behavior also for core API commands. From the Vulkan spec, > > Table 2. vkGetDeviceProcAddr behavior: > > > > device pname return > > ---------------------------------------------------------- > > (..) > > device core device-level command fp > > (...) > > > > See that it specifically states "device-level". > > > > Since the vk.xml file doesn't state if core commands are instance > > or > > device level, we identify device level commands as the ones that > > take a > > VkDevice, VkQueue or VkCommandBuffer as their first parameter. > > > > Fixes test failures in new work-in-progress CTS tests. > > AFAICT this and the original patch are in-light of the following > github issue, right? > If so, please add it to the commit message. > > <a href="https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/issu" rel="noreferrer" target="_blank">https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/issu</a> > es/2323 Yes, they are both related to that. I'll add the reference in the commit log. <div class="HOEnZb"><div class="h5"> > > --- > > src/intel/vulkan/anv_entrypoints_gen.py | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/src/intel/vulkan/anv_entrypoints_gen.py > > b/src/intel/vulkan/anv_entrypoints_gen.py > > index 34ffedb116..f0bfc3b6f8 100644 > > --- a/src/intel/vulkan/anv_entrypoints_gen.py > > +++ b/src/intel/vulkan/anv_entrypoints_gen.py > > @@ -222,7 +222,11 @@ anv_entrypoint_is_enabled(int index, uint32_t > > core_version, > > % for e in entrypoints: > > case ${e.num}: > > % if e.core_version: > > - return ${e.core_version.c_vk_version()} <= core_version; > > + % if e.instance_dispatch: > > + return !device && ${e.core_version.c_vk_version()} <= > > core_version; > > + % else: > > + return ${e.core_version.c_vk_version()} <= core_version; > > + % endif > > % elif e.extension: > > % if e.extension.type == 'instance': > > return !device && instance->${<a href="http://e.extension.name" rel="noreferrer" target="_blank">e.extension.name</a>[3:]}; > > @@ -350,6 +354,12 @@ def cal_hash(name): > > > > EntrypointParam = namedtuple('EntrypointParam', 'type name decl') > > > > +DeviceChildrenList = ['VkDevice', 'VkQueue', 'VkCommandBuffer' ] > > + > > +def is_device_child(name): > > + """Tell if the given type name is a VkDevice or one of its > > children.""" > > + return name in DeviceChildrenList > > + > > class Entrypoint(object): > > def __init__(self, name, return_type, params, guard = None): > > <a href="http://self.name" rel="noreferrer" target="_blank">self.name</a> = name > > @@ -358,6 +368,7 @@ class Entrypoint(object): > > self.guard = guard > > self.enabled = False > > self.num = None > > + self.instance_dispatch = not > > is_device_child(params[0].type) > > On a quick look this seems odd - one is interested in instance > dispatch, yet checking for device. > There are some subtleties about it in the above mentioned report. > > Please add a small comment about those. > > With the link + trivial comment the patch is > Reviewed-by: Emil Velikov <<a href="mailto:emil.velikov@collabora.com">emil.velikov@collabora.com</a>> > > -Emil > _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>