On Tue, Mar 6, 2018 at 8:02 AM, Iago Toral <ito...@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 <ito...@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.veli...@collabora.com> > > > > -Emil > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
