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

[Bas Nieuwenhuizen]

On Tue, Mar 6, 2018 at 8:02 AM, Iago Toral <itoral at igalia.com> wrote:

> 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.
>

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.

>
> > 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
> >
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20180306/092447de/attachment.html>