[Mesa-dev] [PATCH 1/2] anv: bail out if using loader interface prior to v3

[Chad Versace]

On Tue 24 Jan 2017, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov at collabora.com>
> 
> Strictly speaking we could add support for v2 and earlier. At the same
> time, those tend to be buggy and as such there's limited testing done.

I'm confused by the claim of "limited testing". Before my patch landed
that upgraded anvil to loader interface v3, the driver only supported
loader interface v1. And any differences between v1 and v2 are
negligible enough to not be the cause of any crash.

So... is the real problem
    a. anvil doesn't support loader interface v2, or
    b. Fedora 25 ships a buggy loader, and this patch effectively forces
       the user to upgrade the loader to a version in which the bug is
       fixed.

I have difficulty understanding how (a) could possibly be the problem.
Did some patches land in src/vulkan/wsi that broke the v2 interface? If
so, then this patch is probably justified.

If the actual problem is (b), then I believe this patch is the wrong way
to fix it. The real fix should go into the loader. And this patch
prevents the driver working on systems where it should work.

More comments below.

> Cc: Jason Ekstrand <jason at jlekstrand.net>
> Cc: Shawn Starr <shawn.starr at rogers.com>
> Cc: Chad Versace <chadversary at chromium.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99446
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
> Slightly pedantic, yet explicitly mentioned in the spec as a way to
> detect/manage older loader versions. Would have saved us a crash, so I'm
> wondering if we want it for stable ?
> 
> Shawn considering you still have the old libvulkan.so around can you
> give this and/or 2/2 a test ?
> ---
>  src/intel/vulkan/anv_device.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index f80a36a940..e7aa81883a 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -36,6 +36,8 @@
>  
>  #include "genxml/gen7_pack.h"
>  
> +static uint32_t loader_version;
> +
>  struct anv_dispatch_table dtable;
>  
>  static void
> @@ -739,6 +741,11 @@ VKAPI_ATTR PFN_vkVoidFunction VKAPI_CALL vk_icdGetInstanceProcAddr(
>      VkInstance                                  instance,
>      const char*                                 pName)
>  {
> +   if (loader_version < 3u) {
> +      fprintf(stderr, "WARNING: ANV supports Loader interface v3 or newer, v%u "
> +                      "detected. Update your libvulkan.so.\n", loader_version);
> +      return NULL;
> +   }
>     return anv_GetInstanceProcAddr(instance, pName);
>  }
>  
> @@ -2075,6 +2082,7 @@ vk_icdNegotiateLoaderICDInterfaceVersion(uint32_t* pSupportedVersion)
>      *          vkDestroySurfaceKHR(), and other API which uses VKSurfaceKHR,
>      *          because the loader no longer does so.
>      */
> +   loader_version = *pSupportedVersion;
>     *pSupportedVersion = MIN2(*pSupportedVersion, 3u);
>     return VK_SUCCESS;
>  }

If this patch does land, then This hunk needs fixing. If the driver
doesn't support loader interface version 2, then the loader spec
requires that we return VK_ERROR_INCOMPATIBLE_DRIVER here if
*pSupportedVersion < 3.

The loader spec says:

    If the ICD receiving the call no longer supports the interface
    version provided  by the loader (due to deprecation), then it should
    report VK_ERROR_INCOMPATIBLE_DRIVER error.  Otherwise it sets the
    value pointed by "pSupportedVersion" to the latest interface version
    supported by both the ICD and the loader and returns VK_SUCCESS.