[Mesa-dev] [PATCH 3/3] anv: Support loader interface version 2

[Chad Versace]

On Wed 11 Jan 2017, Emil Velikov wrote:
> On 11 January 2017 at 03:59, Jason Ekstrand <jason at jlekstrand.net> wrote:
> > On Tue, Jan 10, 2017 at 7:17 PM, Chad Versace <chadversary at chromium.org>
> > wrote:
> >>
> >> Loader interface v2 differs from v1 in that the first ICD entrypoint
> >> called by the loader is vk_icdNegotiateLoaderICDInterfaceVersion(), not
> >> vk_icdGetInstanceProcAddr(). The ICD must statically expose this
> >> entrypoint.
> >> ---
> >>  src/intel/vulkan/anv_device.c | 43
> >> +++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 43 insertions(+)



> >> +/* Suppress warnings because vk_icd.h does not declare this function.
> >> + * Clang supports this pragma too.
> >> + */
> >> +#pragma GCC diagnostic push
> >> +#pragma GCC diagnostic ignored "-Wmissing-prototypes"
> 
> Since neither new nor old headers provide a declaration I'd just add
> one here, add a comment "XXX: vk_icd.h does not provide the
> declaration of ..." and drop the pragmas ?

Sure. I'll resubmit with the pragma dropped and a different comment.

> >> +    *    - Loader interface v3 differs from v2 in:
> >> +    *        - The ICD must implement vkCreate{PLATFORM}Surface() and
> >> +    *          vkDestroySurface() because the loader no longer does so.

> Nit: "...SurfaceKHR, vkDestroySurfaceKHR() and other API which uses
> KHR_surface ..."

Done. I'll resubmit with fixed comment.

> >
> > We've implemented these since the dawn of time.  I think we can claim v3.
> >
> I believe you're spot on. We're missing support for some extensions
> (VK_KHR_display, VK_KHR_{android,mir,win32}_surface) and thus the
> respective API, but that's orthogonal.
> 
> >>
> >> +    */
> >> +   *pSupportedVersion = 2;

> Since we should advertise the lowest version common across both loader
> and ICD, the following will be the more robust solution ?
> 
>  *pSupportedVersion = MIN(*pSupportedVersion, 3);

I hardcoded 2 because no loader will call
vk_icdNegotiateLoaderICDInterfaceVersion() with a version less than 2.
But, now since we're going to advertise v3, I agree with you. We should
use MIN.

> Reading through the v3 changes, I'm leaning that we want the series in
> -stable, since a) it's not new functionality b) things might end up
> pretty nasty as one mixes old loader + new icd and vise-versa.
> What do you guys thing ?

That sounds good to me. I'd like to hear Jason's opinion too on
backporting this. (Though I expect him to say yes).