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

[Jason Ekstrand]

On Tue, Jan 24, 2017 at 8:17 AM, Emil Velikov <emil.l.velikov at gmail.com>
wrote:

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

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 <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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170124/1728bf22/attachment-0001.html>