radv: Allow physical device interfaces to be included in device extensions

Submitted by Keith Packard on Oct. 12, 2018, 8:38 p.m.

Details

Message ID 20181012203844.29891-1-keithp@keithp.com
State New
Headers show
Series "radv: Allow physical device interfaces to be included in device extensions" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Keith Packard Oct. 12, 2018, 8:38 p.m.
According to the Vulkan spec:

"Vulkan 1.0 initially required all new physical-device-level extension
 functionality to be structured within an instance extension. In order
 to avoid using an instance extension, which often requires loader
 support, physical-device-level extension functionality may be
 implemented within device extensions"

The code that checks for enabled extension APIs currently only passes
functions with VkDevice or VkCommandBuffer as their first
argument. This patch extends that to also allow functions with
VkPhysicalDevice parameters, in support of the above quote from the
Vulkan spec.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 src/amd/vulkan/radv_entrypoints_gen.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/amd/vulkan/radv_entrypoints_gen.py b/src/amd/vulkan/radv_entrypoints_gen.py
index 377b544c2aa..69e6fc3e0eb 100644
--- a/src/amd/vulkan/radv_entrypoints_gen.py
+++ b/src/amd/vulkan/radv_entrypoints_gen.py
@@ -352,7 +352,7 @@  class Entrypoint(EntrypointBase):
         self.return_type = return_type
         self.params = params
         self.guard = guard
-        self.device_command = len(params) > 0 and (params[0].type == 'VkDevice' or params[0].type == 'VkQueue' or params[0].type == 'VkCommandBuffer')
+        self.device_command = len(params) > 0 and (params[0].type == 'VkPhysicalDevice' or params[0].type == 'VkDevice' or params[0].type == 'VkQueue' or params[0].type == 'VkCommandBuffer')
 
     def prefixed_name(self, prefix):
         assert self.name.startswith('vk')

Comments

On Fri, Oct 12, 2018 at 10:38 PM Keith Packard <keithp@keithp.com> wrote:
>
> According to the Vulkan spec:
>
> "Vulkan 1.0 initially required all new physical-device-level extension
>  functionality to be structured within an instance extension. In order
>  to avoid using an instance extension, which often requires loader
>  support, physical-device-level extension functionality may be
>  implemented within device extensions"
>
> The code that checks for enabled extension APIs currently only passes
> functions with VkDevice or VkCommandBuffer as their first
> argument. This patch extends that to also allow functions with
> VkPhysicalDevice parameters, in support of the above quote from the
> Vulkan spec.
>

Also "To obtain a function pointer for a physical-device-level command
from a device extension, an application can use vkGetInstanceProcAddr.
"

As far as I can tell the device_command member is only to make sure we
return NULL from vkGetDeviceProcAddr, and per the spec (3.1 table 2)
we still have to return NULL there for functions which take
VkPhysicalDevice? So the old behavior seems correct to me.

> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  src/amd/vulkan/radv_entrypoints_gen.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/amd/vulkan/radv_entrypoints_gen.py b/src/amd/vulkan/radv_entrypoints_gen.py
> index 377b544c2aa..69e6fc3e0eb 100644
> --- a/src/amd/vulkan/radv_entrypoints_gen.py
> +++ b/src/amd/vulkan/radv_entrypoints_gen.py
> @@ -352,7 +352,7 @@ class Entrypoint(EntrypointBase):
>          self.return_type = return_type
>          self.params = params
>          self.guard = guard
> -        self.device_command = len(params) > 0 and (params[0].type == 'VkDevice' or params[0].type == 'VkQueue' or params[0].type == 'VkCommandBuffer')
> +        self.device_command = len(params) > 0 and (params[0].type == 'VkPhysicalDevice' or params[0].type == 'VkDevice' or params[0].type == 'VkQueue' or params[0].type == 'VkCommandBuffer')
>
>      def prefixed_name(self, prefix):
>          assert self.name.startswith('vk')
> --
> 2.19.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Sat, Oct 13, 2018 at 10:58 AM Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
wrote:

> On Fri, Oct 12, 2018 at 10:38 PM Keith Packard <keithp@keithp.com> wrote:
> >
> > According to the Vulkan spec:
> >
> > "Vulkan 1.0 initially required all new physical-device-level extension
> >  functionality to be structured within an instance extension. In order
> >  to avoid using an instance extension, which often requires loader
> >  support, physical-device-level extension functionality may be
> >  implemented within device extensions"
> >
> > The code that checks for enabled extension APIs currently only passes
> > functions with VkDevice or VkCommandBuffer as their first
> > argument. This patch extends that to also allow functions with
> > VkPhysicalDevice parameters, in support of the above quote from the
> > Vulkan spec.
> >
>
> Also "To obtain a function pointer for a physical-device-level command
> from a device extension, an application can use vkGetInstanceProcAddr.
> "
>
> As far as I can tell the device_command member is only to make sure we
> return NULL from vkGetDeviceProcAddr, and per the spec (3.1 table 2)
> we still have to return NULL there for functions which take
> VkPhysicalDevice? So the old behavior seems correct to me.
>

I think anv is ignoring that line in the table which is why it works for
us.  If only someone wrote tests for these things...

I think the correct interpretation would be that any physical device
functions that are part of a core version or instance extension should
yield NULL but any physical device functions from a device extension should
return a valid function pointer.  Sadly, that behavior is kind-of a pain to
implement. :-(


> > Signed-off-by: Keith Packard <keithp@keithp.com>
> > ---
> >  src/amd/vulkan/radv_entrypoints_gen.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/amd/vulkan/radv_entrypoints_gen.py
> b/src/amd/vulkan/radv_entrypoints_gen.py
> > index 377b544c2aa..69e6fc3e0eb 100644
> > --- a/src/amd/vulkan/radv_entrypoints_gen.py
> > +++ b/src/amd/vulkan/radv_entrypoints_gen.py
> > @@ -352,7 +352,7 @@ class Entrypoint(EntrypointBase):
> >          self.return_type = return_type
> >          self.params = params
> >          self.guard = guard
> > -        self.device_command = len(params) > 0 and (params[0].type ==
> 'VkDevice' or params[0].type == 'VkQueue' or params[0].type ==
> 'VkCommandBuffer')
> > +        self.device_command = len(params) > 0 and (params[0].type ==
> 'VkPhysicalDevice' or params[0].type == 'VkDevice' or params[0].type ==
> 'VkQueue' or params[0].type == 'VkCommandBuffer')
> >
> >      def prefixed_name(self, prefix):
> >          assert self.name.startswith('vk')
> > --
> > 2.19.1
> >
> > _______________________________________________
> > 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
>
On Sat, Oct 13, 2018 at 6:12 PM Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> On Sat, Oct 13, 2018 at 10:58 AM Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote:
>>
>> On Fri, Oct 12, 2018 at 10:38 PM Keith Packard <keithp@keithp.com> wrote:
>> >
>> > According to the Vulkan spec:
>> >
>> > "Vulkan 1.0 initially required all new physical-device-level extension
>> >  functionality to be structured within an instance extension. In order
>> >  to avoid using an instance extension, which often requires loader
>> >  support, physical-device-level extension functionality may be
>> >  implemented within device extensions"
>> >
>> > The code that checks for enabled extension APIs currently only passes
>> > functions with VkDevice or VkCommandBuffer as their first
>> > argument. This patch extends that to also allow functions with
>> > VkPhysicalDevice parameters, in support of the above quote from the
>> > Vulkan spec.
>> >
>>
>> Also "To obtain a function pointer for a physical-device-level command
>> from a device extension, an application can use vkGetInstanceProcAddr.
>> "
>>
>> As far as I can tell the device_command member is only to make sure we
>> return NULL from vkGetDeviceProcAddr, and per the spec (3.1 table 2)
>> we still have to return NULL there for functions which take
>> VkPhysicalDevice? So the old behavior seems correct to me.
>
>
> I think anv is ignoring that line in the table which is why it works for us.  If only someone wrote tests for these things...
>
> I think the correct interpretation would be that any physical device functions that are part of a core version or instance extension should yield NULL but any physical device functions from a device extension should return a valid function pointer.  Sadly, that behavior is kind-of a pain to implement. :-(

How would you read that into the spec? As quoted above it explicitly
tells you to use vkGetInstanceProcAddr for VkPhysicalDevice functions,
even if they are based on a device extension. (And you cannot really
use vkGetDeviceProcAddr anyway as the typical usecase is before you've
created a device).
>
>>
>> > Signed-off-by: Keith Packard <keithp@keithp.com>
>> > ---
>> >  src/amd/vulkan/radv_entrypoints_gen.py | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/src/amd/vulkan/radv_entrypoints_gen.py b/src/amd/vulkan/radv_entrypoints_gen.py
>> > index 377b544c2aa..69e6fc3e0eb 100644
>> > --- a/src/amd/vulkan/radv_entrypoints_gen.py
>> > +++ b/src/amd/vulkan/radv_entrypoints_gen.py
>> > @@ -352,7 +352,7 @@ class Entrypoint(EntrypointBase):
>> >          self.return_type = return_type
>> >          self.params = params
>> >          self.guard = guard
>> > -        self.device_command = len(params) > 0 and (params[0].type == 'VkDevice' or params[0].type == 'VkQueue' or params[0].type == 'VkCommandBuffer')
>> > +        self.device_command = len(params) > 0 and (params[0].type == 'VkPhysicalDevice' or params[0].type == 'VkDevice' or params[0].type == 'VkQueue' or params[0].type == 'VkCommandBuffer')
>> >
>> >      def prefixed_name(self, prefix):
>> >          assert self.name.startswith('vk')
>> > --
>> > 2.19.1
>> >
>> > _______________________________________________
>> > 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
On Sat, Oct 13, 2018 at 11:24 AM Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
wrote:

> On Sat, Oct 13, 2018 at 6:12 PM Jason Ekstrand <jason@jlekstrand.net>
> wrote:
> >
> > On Sat, Oct 13, 2018 at 10:58 AM Bas Nieuwenhuizen <
> bas@basnieuwenhuizen.nl> wrote:
> >>
> >> On Fri, Oct 12, 2018 at 10:38 PM Keith Packard <keithp@keithp.com>
> wrote:
> >> >
> >> > According to the Vulkan spec:
> >> >
> >> > "Vulkan 1.0 initially required all new physical-device-level extension
> >> >  functionality to be structured within an instance extension. In order
> >> >  to avoid using an instance extension, which often requires loader
> >> >  support, physical-device-level extension functionality may be
> >> >  implemented within device extensions"
> >> >
> >> > The code that checks for enabled extension APIs currently only passes
> >> > functions with VkDevice or VkCommandBuffer as their first
> >> > argument. This patch extends that to also allow functions with
> >> > VkPhysicalDevice parameters, in support of the above quote from the
> >> > Vulkan spec.
> >> >
> >>
> >> Also "To obtain a function pointer for a physical-device-level command
> >> from a device extension, an application can use vkGetInstanceProcAddr.
> >> "
> >>
> >> As far as I can tell the device_command member is only to make sure we
> >> return NULL from vkGetDeviceProcAddr, and per the spec (3.1 table 2)
> >> we still have to return NULL there for functions which take
> >> VkPhysicalDevice? So the old behavior seems correct to me.
> >
> >
> > I think anv is ignoring that line in the table which is why it works for
> us.  If only someone wrote tests for these things...
> >
> > I think the correct interpretation would be that any physical device
> functions that are part of a core version or instance extension should
> yield NULL but any physical device functions from a device extension should
> return a valid function pointer.  Sadly, that behavior is kind-of a pain to
> implement. :-(
>
> How would you read that into the spec? As quoted above it explicitly
> tells you to use vkGetInstanceProcAddr for VkPhysicalDevice functions,
> even if they are based on a device extension. (And you cannot really
> use vkGetDeviceProcAddr anyway as the typical usecase is before you've
> created a device).
>

Because I was reading the wrong chunk of spec. :-(  You are correct and
radv is like doing the right thing and anv is doing the wrong thing.

--Jason


> >
> >>
> >> > Signed-off-by: Keith Packard <keithp@keithp.com>
> >> > ---
> >> >  src/amd/vulkan/radv_entrypoints_gen.py | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/src/amd/vulkan/radv_entrypoints_gen.py
> b/src/amd/vulkan/radv_entrypoints_gen.py
> >> > index 377b544c2aa..69e6fc3e0eb 100644
> >> > --- a/src/amd/vulkan/radv_entrypoints_gen.py
> >> > +++ b/src/amd/vulkan/radv_entrypoints_gen.py
> >> > @@ -352,7 +352,7 @@ class Entrypoint(EntrypointBase):
> >> >          self.return_type = return_type
> >> >          self.params = params
> >> >          self.guard = guard
> >> > -        self.device_command = len(params) > 0 and (params[0].type ==
> 'VkDevice' or params[0].type == 'VkQueue' or params[0].type ==
> 'VkCommandBuffer')
> >> > +        self.device_command = len(params) > 0 and (params[0].type ==
> 'VkPhysicalDevice' or params[0].type == 'VkDevice' or params[0].type ==
> 'VkQueue' or params[0].type == 'VkCommandBuffer')
> >> >
> >> >      def prefixed_name(self, prefix):
> >> >          assert self.name.startswith('vk')
> >> > --
> >> > 2.19.1
> >> >
> >> > _______________________________________________
> >> > 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
>
On Sat, Oct 13, 2018 at 11:27 AM Jason Ekstrand <jason@jlekstrand.net>
wrote:

> On Sat, Oct 13, 2018 at 11:24 AM Bas Nieuwenhuizen <
> bas@basnieuwenhuizen.nl> wrote:
>
>> On Sat, Oct 13, 2018 at 6:12 PM Jason Ekstrand <jason@jlekstrand.net>
>> wrote:
>> >
>> > On Sat, Oct 13, 2018 at 10:58 AM Bas Nieuwenhuizen <
>> bas@basnieuwenhuizen.nl> wrote:
>> >>
>> >> On Fri, Oct 12, 2018 at 10:38 PM Keith Packard <keithp@keithp.com>
>> wrote:
>> >> >
>> >> > According to the Vulkan spec:
>> >> >
>> >> > "Vulkan 1.0 initially required all new physical-device-level
>> extension
>> >> >  functionality to be structured within an instance extension. In
>> order
>> >> >  to avoid using an instance extension, which often requires loader
>> >> >  support, physical-device-level extension functionality may be
>> >> >  implemented within device extensions"
>> >> >
>> >> > The code that checks for enabled extension APIs currently only passes
>> >> > functions with VkDevice or VkCommandBuffer as their first
>> >> > argument. This patch extends that to also allow functions with
>> >> > VkPhysicalDevice parameters, in support of the above quote from the
>> >> > Vulkan spec.
>> >> >
>> >>
>> >> Also "To obtain a function pointer for a physical-device-level command
>> >> from a device extension, an application can use vkGetInstanceProcAddr.
>> >> "
>> >>
>> >> As far as I can tell the device_command member is only to make sure we
>> >> return NULL from vkGetDeviceProcAddr, and per the spec (3.1 table 2)
>> >> we still have to return NULL there for functions which take
>> >> VkPhysicalDevice? So the old behavior seems correct to me.
>> >
>> >
>> > I think anv is ignoring that line in the table which is why it works
>> for us.  If only someone wrote tests for these things...
>> >
>> > I think the correct interpretation would be that any physical device
>> functions that are part of a core version or instance extension should
>> yield NULL but any physical device functions from a device extension should
>> return a valid function pointer.  Sadly, that behavior is kind-of a pain to
>> implement. :-(
>>
>> How would you read that into the spec? As quoted above it explicitly
>> tells you to use vkGetInstanceProcAddr for VkPhysicalDevice functions,
>> even if they are based on a device extension. (And you cannot really
>> use vkGetDeviceProcAddr anyway as the typical usecase is before you've
>> created a device).
>>
>
> Because I was reading the wrong chunk of spec. :-(  You are correct and
> radv is like doing the right thing and anv is doing the wrong thing.
>

Actually, I think anv is doing the right thing too.  Now I have no idea why
Keith was having problems.

--Jason



>
>
>> >
>> >>
>> >> > Signed-off-by: Keith Packard <keithp@keithp.com>
>> >> > ---
>> >> >  src/amd/vulkan/radv_entrypoints_gen.py | 2 +-
>> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/src/amd/vulkan/radv_entrypoints_gen.py
>> b/src/amd/vulkan/radv_entrypoints_gen.py
>> >> > index 377b544c2aa..69e6fc3e0eb 100644
>> >> > --- a/src/amd/vulkan/radv_entrypoints_gen.py
>> >> > +++ b/src/amd/vulkan/radv_entrypoints_gen.py
>> >> > @@ -352,7 +352,7 @@ class Entrypoint(EntrypointBase):
>> >> >          self.return_type = return_type
>> >> >          self.params = params
>> >> >          self.guard = guard
>> >> > -        self.device_command = len(params) > 0 and (params[0].type
>> == 'VkDevice' or params[0].type == 'VkQueue' or params[0].type ==
>> 'VkCommandBuffer')
>> >> > +        self.device_command = len(params) > 0 and (params[0].type
>> == 'VkPhysicalDevice' or params[0].type == 'VkDevice' or params[0].type ==
>> 'VkQueue' or params[0].type == 'VkCommandBuffer')
>> >> >
>> >> >      def prefixed_name(self, prefix):
>> >> >          assert self.name.startswith('vk')
>> >> > --
>> >> > 2.19.1
>> >> >
>> >> > _______________________________________________
>> >> > 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
>>
>
Jason Ekstrand <jason@jlekstrand.net> writes:

> Actually, I think anv is doing the right thing too.  Now I have no idea why
> Keith was having problems.

anv is happily returning a valid pointer and radv is not?

In any case, I've switched to using vkGetInstanceProcAddr for the
VkPhysicalDevice function and it works fine with both drivers.
On October 13, 2018 19:50:00 "Keith Packard" <keithp@keithp.com> wrote:

> Jason Ekstrand <jason@jlekstrand.net> writes:
>
>> Actually, I think anv is doing the right thing too.  Now I have no idea why
>> Keith was having problems.
>
> anv is happily returning a valid pointer and radv is not?
>
> In any case, I've switched to using vkGetInstanceProcAddr for the
> VkPhysicalDevice function and it works fine with both drivers.

Using vkGetInstanceProcAddr is the right thing to do. The fact that anv 
allows you to is a bug which should be investigated and fixed.  I looked at 
it a bit today and I'm still not sure how it's happening.

--Jason