[Mesa-dev] [PATCH] loader: use drmDeviceUseRevisionFile to prevent waking up the GPUs

[Nicolai Hähnle]

On 09.11.2016 19:09, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov at collabora.com>
>
> Currently the drmGetDevice[s] API parses the config sysfs file to
> retrieve the revision field.
>
> That is required since there's no separate file (nor a libudev call
> afaict) that can be used. At the same time doing so causes the device to
> be awaken, so if an application creates/destroys a GL context multiple
> times they can observe noticeable delays. 2-3s in the case of
> firefox/thunderbird/chromium + radeon module.
>
> There's a kernel patch on the PCI mailing list, but until then we can
> use drmDeviceUseRevisionFile() which will use separate sysfs files and
> default to 0 if the revision one is missing.
>
> Since we don't care about the revision id, we can use that.
>
> Cc: "13.0" <mesa-stable at lists.freedesktop.org>

Can a patch that raises the minimum build requirements go into a stable 
release? I agree that fixing this is important, just wondering whether 
there are downstream problems we need to be aware of.

I also have a concern with the API, which I've sent as a response to the 
libdrm patch.

Thanks,
Nicolai


> Cc: Mauro Santos <registo.mailling at gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
> The libdrm version is tentative. An alternative solution is to use
> revision file [and default to 0] behind the scenes.
>
> Both options are rather nasty, so any preferences and suggestions are
> greatly appreciated.
>
> Mauro, feel free to drop the version hunk (or bump the libdrm version
> while building).
> ---
>  configure.ac        | 2 +-
>  src/loader/loader.c | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index d215b63..f529f2cf 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -68,7 +68,7 @@ OPENCL_VERSION=1
>  AC_SUBST([OPENCL_VERSION])
>
>  dnl Versions for external dependencies
> -LIBDRM_REQUIRED=2.4.66
> +LIBDRM_REQUIRED=2.4.72
>  LIBDRM_RADEON_REQUIRED=2.4.56
>  LIBDRM_AMDGPU_REQUIRED=2.4.63
>  LIBDRM_INTEL_REQUIRED=2.4.61
> diff --git a/src/loader/loader.c b/src/loader/loader.c
> index fe90307..e2e4156 100644
> --- a/src/loader/loader.c
> +++ b/src/loader/loader.c
> @@ -275,6 +275,7 @@ drm_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id)
>     drmDevicePtr device;
>     int ret;
>
> +   drmDeviceUseRevisionFile();

That API is nasty.

>     if (drmGetDevice(fd, &device) == 0) {
>        if (device->bustype == DRM_BUS_PCI) {
>           *vendor_id = device->deviceinfo.pci->vendor_id;
>