[Mesa-dev] [PATCH] anv: Implement VK_KHR_get_physical_device_properties2

[Lionel Landwerlin]

On 24/01/17 00:50, Jason Ekstrand wrote:
> On Mon, Jan 23, 2017 at 4:25 PM, Chad Versace 
> <chadversary at chromium.org <mailto:chadversary at chromium.org>> wrote:
>
>     On Mon 23 Jan 2017, Jason Ekstrand wrote:
>     > On Mon, Jan 23, 2017 at 3:31 PM, Chad Versace
>     <chadversary at chromium.org <mailto:chadversary at chromium.org>> wrote:
>     >
>     >     On Mon 23 Jan 2017, Jason Ekstrand wrote:
>     >     > On Mon, Jan 23, 2017 at 2:28 PM, Chad Versace
>     <chadversary at chromium.org <mailto:chadversary at chromium.org>>
>     >     wrote:
>     >     >
>     >     >     Implement each vkFoo2KHR() by trivially passing it
>     through to the
>     >     >     original vkFoo().
>     >     >
>     >     >
>     >     > As I mentioned to Lionel when he wrote basically this
>     exact same patch, I
>     >     think
>     >     > that may be backwards.  I can see two ways of doing this
>     long-term:
>     >
>     >     If we look into the future, my patch is indeed backwards.
>     >     >
>     >     > 1) Implement all of the queries (of a particular type) in
>     a single
>     >     function and
>     >     > the legacy query calls the query2 variant and then copies
>     the data over.
>     >
>     >     Option 1 is definitely better than my patch.
>     >
>     >     > 2) Implement each query as its own function and the
>     queries2 function
>     >     loops
>     >     > over the data structures calling the appropriate function
>     on each one.
>     >
>     >     I don't see exactly what you're proposing in option 2. Do
>     you mean, for
>     >     example,
>     >     that vkGetPhysicalDeviceFormatProperties2KHR() would, for
>     each structure
>     >     chained off of the input and output structs, including the
>     toplevel
>     >     structs themselves, call some function specific to those
>     structs?
>     >
>     >
>     > I mean it would be
>     >
>     > for (struct_base *s = pPhysicalDeviceProperties; s; s = s->pNext) {
>     >    switch (s->type) {
>     >    case VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_PROPERTIES2: {
>     >       VkPhysicalDeviceProperties2 *props = s;
>     >       anv_GetPhysicalDeviceProperties(pdev, &props->props);
>     >       break;
>     >    case VK_STRUCTURE_TYPE_SOMETHING_ELSE:
>     >       VkSomethingElse *thing = s;
>     >       anv_get_something_else(pdev, thing);
>     >       break;
>     >    ...
>     >    default:
>     >       assert(!"Invalid structure type");
>     >    }
>     > }
>
>     All vkGetFoo2KHR() funcs have output structs; only a subset have input
>     structs. Therefore, if we choose to do option 2, for uniformity's sake
>     we should implement it by iterating over the output structs, even when
>     input structs are present.
>
>     What do you think?
>
>
> You bring up an interesting point.  I'm wondering if we don't want to 
> do the helper thing and also pass the query info struct to all of the 
> helpers.  If they want to pull information out of chained children, 
> it's their job to crawl through and find them.  Otherwise, we would 
> have to come up with some sort of weird double-iterator and I can't 
> imagine that ending well.
>
> The more I think about this, the more convinced I become that we want 
> a helper per chaining query so maybe your patch is actually ok modulo 
> adding some for loops when it comes time to extend one of the 
> queries.  I think I'd be a fan of adding the for loops now though.
>
>     Also, about that assertion in the default case... I believe
>     drivers are
>     required to ignore unrecongized extension structs. From the Vulkan
>     1.0.38 spec:
>
>        Any component of the implementation (the loader, any enabled
>     layers,
>        and drivers) must skip over, without processing (other than
>     reading the
>        sType and pNext members) any chained structures with sType
>     values not
>        defined by extensions supported by that component.
>
>
> Right...
>

What do you think about https://patchwork.freedesktop.org/patch/134841/ ?
Is that close enough to what you have in mind?

Thanks,

-
Lionel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170124/85cc2dcd/attachment.html>