[Mesa-stable] [Mesa-dev] [PATCH] anv/entrypoints: VkGetDeviceProcAddr returns NULL for core instance commands

[Iago Toral]

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.

> Bas, you might want to port these over?
> 
> On 5 March 2018 at 10:46, Iago Toral Quiroga <itoral at igalia.com>
> 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.
> 
> https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/issu
> es/2323

Yes, they are both related to that. I'll add the reference in the
commit log.

> > ---
> >  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->${e.extension.name[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):
> >          self.name = 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 <emil.velikov at collabora.com>
> 
> -Emil
>