On Tue, Jan 24, 2017 at 8:17 AM, Emil Velikov <[email protected]> wrote:
> On 24 January 2017 at 15:41, Chad Versace <[email protected]> > wrote: > > On Tue 24 Jan 2017, Emil Velikov wrote: > >> From: Emil Velikov <[email protected]> > >> > >> 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. > For what it's worth, Fedora is in the process of updating their loader... 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. > > More comments below. > > > >> Cc: Jason Ekstrand <[email protected]> > >> Cc: Shawn Starr <[email protected]> > >> Cc: Chad Versace <[email protected]> > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99446 > >> Signed-off-by: Emil Velikov <[email protected]> > >> --- > >> 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 > [email protected] > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
