[PATCH libdrm v2] xf86drm: Parse the separate files to retrieve the vendor, ... info

[Michel Dänzer]

On 10/11/16 10:38 PM, Emil Velikov wrote:
> On 10 November 2016 at 12:40, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>> On 09.11.2016 19:08, Emil Velikov wrote:
>>>
>>> From: Emil Velikov <emil.velikov at collabora.com>
>>>
>>> Parsing config sysfs file wakes up the device. The latter of which may
>>> be slow and isn't required to begin with.
>>>
>>> Reading through config is/was required since the revision is not
>>> available by other means, although with a kernel patch in the way that
>>> is about to change.
>>>
>>> Since returning 0 when one might expect a valid value is a no-go add a
>>> workaround drmDeviceUseRevisionFile() which one can use to say "I don't
>>> care if the revision file returns 0."
>>>
>>> v2: Complete rework - add new API to control the method, instead of
>>> changing it underneat the users' feet.
>>>
>>> Cc: Michel Dänzer <michel.daenzer at amd.com>
>>> 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>
>>> ---
>>> I don't have a strong preference for/against this or v1.
>>>
>>> With this Mesa will require a 2 line patch. With v1 things will get
>>> fixed magically, when rebuilt against newer libdrm.
>>>
>>> Mauro I'll send the mesa patch in a second. Feel free to give it a spin.
>>>
>>> Thanks
>>> ---
>>>  xf86drm.c | 70
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>  xf86drm.h | 11 ++++++++++
>>>  2 files changed, 78 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xf86drm.c b/xf86drm.c
>>> index 52add5e..676effc 100644
>>> --- a/xf86drm.c
>>> +++ b/xf86drm.c
>>> @@ -113,6 +113,13 @@ void drmSetServerInfo(drmServerInfoPtr info)
>>>      drm_server_info = info;
>>>  }
>>>
>>> +static bool use_revision_file;
>>> +
>>> +void drmDeviceUseRevisionFile(void)
>>> +{
>>> +    use_revision_file = true;
>>> +}
>>
>>
>> I think this API of setting a magic hidden global variable is really nasty.
>>
>> Can we add new drmGetDevice[s] entry points with a flags parameter and a
>> flag (per Michel's suggestion) which says "I don't care about the revision
>> ID"? It kind of sucks because they'll have to get a silly name like
>> drmGetDevice[s]2, but I think that's still better than magic global state.
>>
> AFACS Michel suggested (in his latest reply) to have
> parse_separate_sysfs_files always and make the lack of revision file
> fatal - falling back to ./config. Not a separate API [+ flags] ?

You're mixing up two separate issues. Sorry if I was unclear before, let
me try again:

My first comment was about the "drmDeviceUseRevisionFile" name being
misleading and not reflecting the intent. For this issue, I think
Nicolai's suggestion is even better than just fixing the name.

My other comment was that parse_separate_sysfs_files should be tried
first even if the revision information is needed, and should only bail
in that case if the separate revision sysfs files are missing, in which
case parse_config_sysfs_file can be used as a fallback.


> P.S. /me is dreaming of the day when [wh]e'll get to drop 90% of
> libdrm and it's hacks - viva la libdrm3 !

Given the issues with bumping SONAME, I'm afraid you'll have to continue
dreaming for a long time. :)


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer