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

[Jason Ekstrand]

On Tue, Mar 13, 2018 at 8:33 AM, Emil Velikov 
wrote:

> On 13 March 2018 at 15:02, Jason Ekstrand  wrote:
> > On Tue, Mar 13, 2018 at 7:56 AM, Emil Velikov 
> > wrote:
> >>
> >> On 12 March 2018 at 08:40, Iago Toral Quiroga 
> 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 pnamereturn
> >> > --
> >> > (..)
> >> > device core device-level commandfp
> >> > (...)
> >> >
> >> > 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.
> >> >
> >> > Also see the public issue:
> >> >
> >> > https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/
> issues/2323
> >> >
> >> > v2:
> >> >   - Include reference to github issue (Emil)
> >> >   - Rebased on top of Vulkan 1.1 changes.
> >> >
> >> > Reviewed-by: Emil Velikov  (v1)
> >> > ---
> >> >
> >> > Emil, I had to rebase the patch on top of Jason's 1.1 changes. He had
> >> > already
> >> > accounted for device dispatches in that work, so now I just build on
> top
> >> > of
> >> > that now. With that, I am not sure whether the comment you were asking
> >> > for makes
> >> > sense in this patch any more (I think it should have gone in Jason's,
> >> > when he
> >> > added is_device_entrypoint()). I you want a comment for that I can
> send
> >> > another patch to include it, or maybe ammend the first patch in this
> >> > series to
> >> > include that. However, do notice that the comment you were referring
> to
> >> > has
> >> > been removed from the spec, since now it is clearly stated that only
> >> > core device-level commands return non-NULL pointers, so I think my
> >> > preference
> >> > would be to not add ny comments.
> >> >
> >> The suggestion was aimed as a reference, for anyone who missed the
> >> specific hunk in the spec update.
> >> There should be none, so yeah - don't bother ;-)
> >>
> >> > Also, this version won't do for 18.0, but I guess we can still use the
> >> > previous version for that if you want to put it in.
> >> >
> >> I would love to apply that one, if you don't mind.
> >
> >
> > I would rather we not back-port this to 18.0 at least not without
> > significant testing.  From what I've heard from the WG, it looks lie the
> two
> > id games will be broken with it and the loader is not yet patched with a
> > work-around.
> >
> Guess I have misunderstood Lenny's reply [1] - it indicated that
> Wolfenstein/Doom are fine with said change.
>

There have been what you might call "further developments". :-)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 13 March 2018 at 15:02, Jason Ekstrand  wrote:
> On Tue, Mar 13, 2018 at 7:56 AM, Emil Velikov 
> wrote:
>>
>> On 12 March 2018 at 08:40, Iago Toral Quiroga  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 pnamereturn
>> > --
>> > (..)
>> > device core device-level commandfp
>> > (...)
>> >
>> > 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.
>> >
>> > Also see the public issue:
>> >
>> > https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/issues/2323
>> >
>> > v2:
>> >   - Include reference to github issue (Emil)
>> >   - Rebased on top of Vulkan 1.1 changes.
>> >
>> > Reviewed-by: Emil Velikov  (v1)
>> > ---
>> >
>> > Emil, I had to rebase the patch on top of Jason's 1.1 changes. He had
>> > already
>> > accounted for device dispatches in that work, so now I just build on top
>> > of
>> > that now. With that, I am not sure whether the comment you were asking
>> > for makes
>> > sense in this patch any more (I think it should have gone in Jason's,
>> > when he
>> > added is_device_entrypoint()). I you want a comment for that I can send
>> > another patch to include it, or maybe ammend the first patch in this
>> > series to
>> > include that. However, do notice that the comment you were referring to
>> > has
>> > been removed from the spec, since now it is clearly stated that only
>> > core device-level commands return non-NULL pointers, so I think my
>> > preference
>> > would be to not add ny comments.
>> >
>> The suggestion was aimed as a reference, for anyone who missed the
>> specific hunk in the spec update.
>> There should be none, so yeah - don't bother ;-)
>>
>> > Also, this version won't do for 18.0, but I guess we can still use the
>> > previous version for that if you want to put it in.
>> >
>> I would love to apply that one, if you don't mind.
>
>
> I would rather we not back-port this to 18.0 at least not without
> significant testing.  From what I've heard from the WG, it looks lie the two
> id games will be broken with it and the loader is not yet patched with a
> work-around.
>
Guess I have misunderstood Lenny's reply [1] - it indicated that
Wolfenstein/Doom are fine with said change.
Dully noted, I _might_ bring this up _only_ after checking that the
games continue to work correctly.

Thanks!
Emil

[1] 
https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/issues/2323#issuecomment-367810941
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jason Ekstrand]

On Tue, Mar 13, 2018 at 7:56 AM, Emil Velikov 
wrote:

> On 12 March 2018 at 08:40, Iago Toral Quiroga  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 pnamereturn
> > --
> > (..)
> > device core device-level commandfp
> > (...)
> >
> > 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.
> >
> > Also see the public issue:
> > https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/
> issues/2323
> >
> > v2:
> >   - Include reference to github issue (Emil)
> >   - Rebased on top of Vulkan 1.1 changes.
> >
> > Reviewed-by: Emil Velikov  (v1)
> > ---
> >
> > Emil, I had to rebase the patch on top of Jason's 1.1 changes. He had
> already
> > accounted for device dispatches in that work, so now I just build on top
> of
> > that now. With that, I am not sure whether the comment you were asking
> for makes
> > sense in this patch any more (I think it should have gone in Jason's,
> when he
> > added is_device_entrypoint()). I you want a comment for that I can send
> > another patch to include it, or maybe ammend the first patch in this
> series to
> > include that. However, do notice that the comment you were referring to
> has
> > been removed from the spec, since now it is clearly stated that only
> > core device-level commands return non-NULL pointers, so I think my
> preference
> > would be to not add ny comments.
> >
> The suggestion was aimed as a reference, for anyone who missed the
> specific hunk in the spec update.
> There should be none, so yeah - don't bother ;-)
>
> > Also, this version won't do for 18.0, but I guess we can still use the
> > previous version for that if you want to put it in.
> >
> I would love to apply that one, if you don't mind.
>

I would rather we not back-port this to 18.0 at least not without
significant testing.  From what I've heard from the WG, it looks lie the
two id games will be broken with it and the loader is not yet patched with
a work-around.

--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 12 March 2018 at 08:40, Iago Toral Quiroga  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 pnamereturn
> --
> (..)
> device core device-level commandfp
> (...)
>
> 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.
>
> Also see the public issue:
> https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/issues/2323
>
> v2:
>   - Include reference to github issue (Emil)
>   - Rebased on top of Vulkan 1.1 changes.
>
> Reviewed-by: Emil Velikov  (v1)
> ---
>
> Emil, I had to rebase the patch on top of Jason's 1.1 changes. He had already
> accounted for device dispatches in that work, so now I just build on top of
> that now. With that, I am not sure whether the comment you were asking for 
> makes
> sense in this patch any more (I think it should have gone in Jason's, when he
> added is_device_entrypoint()). I you want a comment for that I can send
> another patch to include it, or maybe ammend the first patch in this series to
> include that. However, do notice that the comment you were referring to has
> been removed from the spec, since now it is clearly stated that only
> core device-level commands return non-NULL pointers, so I think my preference
> would be to not add ny comments.
>
The suggestion was aimed as a reference, for anyone who missed the
specific hunk in the spec update.
There should be none, so yeah - don't bother ;-)

> Also, this version won't do for 18.0, but I guess we can still use the
> previous version for that if you want to put it in.
>
I would love to apply that one, if you don't mind.

As you saw yourself, there would be no regressions due to the wrapping
done by the loader.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Iago Toral]

On Mon, 2018-03-12 at 07:37 -0700, Jason Ekstrand wrote:
> On Mon, Mar 12, 2018 at 1:40 AM, Iago Toral Quiroga  m> 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 pnamereturn
> > 
> > --
> > 
> > (..)
> > 
> > device core device-level commandfp
> > 
> > (...)
> > 
> > 
> > 
> > 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.
> > 
> > 
> > 
> > Also see the public issue:
> > 
> > https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/is
> > sues/2323
> > 
> > 
> > 
> > v2:
> > 
> >   - Include reference to github issue (Emil)
> > 
> >   - Rebased on top of Vulkan 1.1 changes.
> > 
> > 
> > 
> > Reviewed-by: Emil Velikov  (v1)
> > 
> > ---
> > 
> > 
> > 
> > Emil, I had to rebase the patch on top of Jason's 1.1 changes. He
> > had already
> > 
> > accounted for device dispatches in that work, so now I just build
> > on top of
> > 
> > that now. With that, I am not sure whether the comment you were
> > asking for makes
> > 
> > sense in this patch any more (I think it should have gone in
> > Jason's, when he
> > 
> > added is_device_entrypoint()). I you want a comment for that I can
> > send
> > 
> > another patch to include it, or maybe ammend the first patch in
> > this series to
> > 
> > include that. However, do notice that the comment you were
> > referring to has
> > 
> > been removed from the spec, since now it is clearly stated that
> > only
> > 
> > core device-level commands return non-NULL pointers, so I think my
> > preference
> > 
> > would be to not add ny comments.
> > 
> > 
> > 
> > Also, this version won't do for 18.0, but I guess we can still use
> > the
> > 
> > previous version for that if you want to put it in.
> > 
> > 
> > 
> >  src/intel/vulkan/anv_entrypoints_gen.py | 6 +-
> > 
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > 
> > 
> > diff --git a/src/intel/vulkan/anv_entrypoints_gen.py
> > b/src/intel/vulkan/anv_entrypoints_gen.py
> > 
> > index 27f1aa..8e0d036469 100644
> > 
> > --- a/src/intel/vulkan/anv_entrypoints_gen.py
> > 
> > +++ b/src/intel/vulkan/anv_entrypoints_gen.py
> > 
> > @@ -274,7 +274,11 @@ anv_entrypoint_is_enabled(int index, uint32_t
> > core_version,
> > 
> > case ${e.num}:
> > 
> >/* ${e.name} */
> > 
> > % if e.core_version:
> > 
> > -  return ${e.core_version.c_vk_version()} <= core_version;
> > 
> > +  % if not e.is_device_entrypoint():
> > 
> > + return !device && ${e.core_version.c_vk_version()} <=
> > core_version;
> > 
> > +  % else:
> > 
> > + return ${e.core_version.c_vk_version()} <= core_version;
> > 
> > +  % endif
> 
> Any particular reason why you did "if not" instead of just flipping
> the then and else blocks?

Not really, I can change that. 
> > % elif e.extensions:
> > 
> >   % for ext in e.extensions:
> > 
> > % if ext.type == 'instance':
> > 
> > --
> > 
> > 2.14.1
> > 
> > 
> > ___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jason Ekstrand]

On Mon, Mar 12, 2018 at 1:40 AM, Iago Toral Quiroga 
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 pnamereturn
> --
> (..)
> device core device-level commandfp
> (...)
>
> 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.
>
> Also see the public issue:
> https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/
> issues/2323
>
> v2:
>   - Include reference to github issue (Emil)
>   - Rebased on top of Vulkan 1.1 changes.
>
> Reviewed-by: Emil Velikov  (v1)
> ---
>
> Emil, I had to rebase the patch on top of Jason's 1.1 changes. He had
> already
> accounted for device dispatches in that work, so now I just build on top of
> that now. With that, I am not sure whether the comment you were asking for
> makes
> sense in this patch any more (I think it should have gone in Jason's, when
> he
> added is_device_entrypoint()). I you want a comment for that I can send
> another patch to include it, or maybe ammend the first patch in this
> series to
> include that. However, do notice that the comment you were referring to has
> been removed from the spec, since now it is clearly stated that only
> core device-level commands return non-NULL pointers, so I think my
> preference
> would be to not add ny comments.
>
> Also, this version won't do for 18.0, but I guess we can still use the
> previous version for that if you want to put it in.
>
>  src/intel/vulkan/anv_entrypoints_gen.py | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/vulkan/anv_entrypoints_gen.py
> b/src/intel/vulkan/anv_entrypoints_gen.py
> index 27f1aa..8e0d036469 100644
> --- a/src/intel/vulkan/anv_entrypoints_gen.py
> +++ b/src/intel/vulkan/anv_entrypoints_gen.py
> @@ -274,7 +274,11 @@ anv_entrypoint_is_enabled(int index, uint32_t
> core_version,
> case ${e.num}:
>/* ${e.name} */
> % if e.core_version:
> -  return ${e.core_version.c_vk_version()} <= core_version;
> +  % if not e.is_device_entrypoint():
> + return !device && ${e.core_version.c_vk_version()} <=
> core_version;
> +  % else:
> + return ${e.core_version.c_vk_version()} <= core_version;
> +  % endif
>

Any particular reason why you did "if not" instead of just flipping the
then and else blocks?


> % elif e.extensions:
>   % for ext in e.extensions:
> % if ext.type == 'instance':
> --
> 2.14.1
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev