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

[Emil Velikov]

On 24 January 2017 at 15:41, Chad Versace <chadversary at chromium.org> wrote:
> 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.
>
I fully agree with your reasoning.

B is the one to blame here. I may have gone overzealous with the
wording/approach, but the idea is there - how do we deal with issues,
reported against Mesa (ANV/RADV) where the problems seems to be in the
loader.
We don't want to have the behaviour we had with OpenGL where people
jump to assumptions that ANV/RADV is broken because "it works" with
binary driver FOO. Even when the crash/issue is outside Mesa.

Looking at git log (as per the bugreport) I wonder if encouraging
people to use updated loader (as this patch does) isn't that bad of a
thing. Esp. since distros might not always see a reason otherwise.

> 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.

Again, fully agree.

Thanks
Emil