On 24 January 2017 at 15:41, Chad Versace <chadvers...@chromium.org> wrote: > On Tue 24 Jan 2017, Emil Velikov wrote: >> From: Emil Velikov <emil.veli...@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 <ja...@jlekstrand.net> >> Cc: Shawn Starr <shawn.st...@rogers.com> >> Cc: Chad Versace <chadvers...@chromium.org> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99446 >> Signed-off-by: Emil Velikov <emil.veli...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
