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

[Emil Velikov]

On 10 November 2016 at 12:44, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> 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 agree that bumping the requirement is quite nasty and my first
suggestion [rework the libdrm implementation to silently return
revision = 0] might seem nastier.

> 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.
>
Couldn't agree more - I even mentioned it a few lines above :-\ But
even with the GetDevice(s)2 API, one will still require a libdrm
version bump. Either way - let's keep that into the libdrm thread.

Thanks
Emil