<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jan 24, 2017 at 8:17 AM, Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 24 January 2017 at 15:41, Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> wrote: > On Tue 24 Jan 2017, Emil Velikov wrote: >> From: Emil Velikov <<a href="mailto:emil.velikov@collabora.com">emil.velikov@collabora.com</a>> >> >> 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. </blockquote><div> </div><div>For what it's worth, Fedora is in the process of updating their loader... </div><div>Also, I think we will want to do this eventually but not yet. One of these days, I'm going to rewrite the WSI implementation *again* to make it do something useful in CreateFooSurface. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > More comments below. > >> Cc: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> >> Cc: Shawn Starr <<a href="mailto:shawn.starr@rogers.com">shawn.starr@rogers.com</a>> >> Cc: Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> >> Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=99446" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=99446</a> >> Signed-off-by: Emil Velikov <<a href="mailto:emil.velikov@collabora.com">emil.velikov@collabora.com</a>> >> --- >> 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. </div></div>Again, fully agree. Thanks Emil <div class="HOEnZb"><div class="h5">_______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>