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

Commit Message

Emil Velikov Nov. 9, 2016, 6:08 p.m. UTC

From: Emil Velikov <[e-mail]>

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 <[e-mail]>
Cc: Mauro Santos <[e-mail]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
Signed-off-by: Emil Velikov <[e-mail]>
---
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(-)

Comments

Michel Dänzer Nov. 10, 2016, 8 a.m. UTC | #1

On 10/11/16 03:08 AM, Emil Velikov wrote:
> From: Emil Velikov <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Mauro Santos <[e-mail]>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> Signed-off-by: Emil Velikov <[e-mail]>
> ---
> 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.

"drmDeviceUseRevisionFile" seems slightly misleading — the intent is "I
don't care about the revision", not "I want you to use the revision file".


> +static int drmParsePciDeviceInfo(const char *d_name,
> +                                 drmPciDeviceInfoPtr device)
> +{
> +#ifdef __linux__
> +    if (use_revision_file)
> +        return parse_separate_sysfs_files(d_name, device);
> +
> +    return parse_config_sysfs_file(d_name, device);

I might be better to always try parse_separate_sysfs_files first, but
bail from there if the revision files don't exist and the new API wasn't
called.

Nicolai Hähnle Nov. 10, 2016, 12:40 p.m. UTC | #2

On 09.11.2016 19:08, Emil Velikov wrote:
> From: Emil Velikov <[e-mail]>
>
> 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 <[e-mail]>
> Cc: Mauro Santos <[e-mail]>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> Signed-off-by: Emil Velikov <[e-mail]>
> ---
> 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.

Cheers,
Nicolai

> +
>  /**
>   * Output a message to stderr.
>   *
> @@ -2946,10 +2953,56 @@ static int drmGetMaxNodeName(void)
>             3 /* length of the node number */;
>  }
>
> -static int drmParsePciDeviceInfo(const char *d_name,
> -                                 drmPciDeviceInfoPtr device)
> -{
>  #ifdef __linux__
> +static int parse_separate_sysfs_files(const char *d_name,
> +                                      drmPciDeviceInfoPtr device)
> +{
> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
> +    static const char *attrs[] = {
> +      "revision", /* XXX: make sure it's always first, see note below */
> +      "vendor",
> +      "device",
> +      "subsystem_vendor",
> +      "subsystem_device",
> +    };
> +    char path[PATH_MAX + 1];
> +    unsigned int data[ARRAY_SIZE(attrs)];
> +    FILE *fp;
> +    int ret;
> +
> +    for (unsigned i = 0; i < ARRAY_SIZE(attrs); i++) {
> +        snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/%s",
> +                 d_name, attrs[i]);
> +        fp = fopen(path, "r");
> +        if (!fp) {
> +            /* Note: First we check the revision, since older kernels
> +             * may not have it. Default to zero in such cases. */
> +            if (i == 0) {
> +                data[i] = 0;
> +                continue;
> +            }
> +            return -errno;
> +        }
> +
> +        ret = fscanf(fp, "%x", &data[i]);
> +        fclose(fp);
> +        if (ret != 1)
> +            return -errno;
> +
> +    }
> +
> +    device->revision_id = data[0] & 0xff;
> +    device->vendor_id = data[1] & 0xffff;
> +    device->device_id = data[2] & 0xffff;
> +    device->subvendor_id = data[3] & 0xffff;
> +    device->subdevice_id = data[4] & 0xffff;
> +
> +    return 0;
> +}
> +
> +static int parse_config_sysfs_file(const char *d_name,
> +                                  drmPciDeviceInfoPtr device)
> +{
>      char path[PATH_MAX + 1];
>      unsigned char config[64];
>      int fd, ret;
> @@ -2971,6 +3024,17 @@ static int drmParsePciDeviceInfo(const char *d_name,
>      device->subdevice_id = config[46] | (config[47] << 8);
>
>      return 0;
> +}
> +#endif
> +
> +static int drmParsePciDeviceInfo(const char *d_name,
> +                                 drmPciDeviceInfoPtr device)
> +{
> +#ifdef __linux__
> +    if (use_revision_file)
> +        return parse_separate_sysfs_files(d_name, device);
> +
> +    return parse_config_sysfs_file(d_name, device);
>  #else
>  #warning "Missing implementation of drmParsePciDeviceInfo"
>      return -EINVAL;
> diff --git a/xf86drm.h b/xf86drm.h
> index 481d882..d1ebc32 100644
> --- a/xf86drm.h
> +++ b/xf86drm.h
> @@ -796,6 +796,17 @@ extern void drmFreeDevice(drmDevicePtr *device);
>  extern int drmGetDevices(drmDevicePtr devices[], int max_devices);
>  extern void drmFreeDevices(drmDevicePtr devices[], int count);
>
> +
> +/**
> + * Force any sequential calls to drmGetDevice/drmGetDevices to use the
> + * revision sysfs file instead of config one. The latter wakes up the device
> + * if powered off.
> + *
> + * A value of 0 will be returned for the revision_id field if the sysfs file
> + * is missing. DO NOT USE THIS if you depend on a correct revision_id.
> + */
> +extern void drmDeviceUseRevisionFile(void);
> +
>  #if defined(__cplusplus)
>  }
>  #endif
>

Emil Velikov Nov. 10, 2016, 1:38 p.m. UTC | #3

On 10 November 2016 at 12:40, Nicolai Hähnle <[e-mail]> wrote:
> On 09.11.2016 19:08, Emil Velikov wrote:
>>
>> From: Emil Velikov <[e-mail]>
>>
>> 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 <[e-mail]>
>> Cc: Mauro Santos <[e-mail]>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>> Signed-off-by: Emil Velikov <[e-mail]>
>> ---
>> 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] ?

I can do that, as long as he's OK with the idea/approach.

Thanks
Emil
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 !

Michel Dänzer Nov. 14, 2016, 9:56 a.m. UTC | #4

On 10/11/16 10:38 PM, Emil Velikov wrote:
> On 10 November 2016 at 12:40, Nicolai Hähnle <[e-mail]> wrote:
>> On 09.11.2016 19:08, Emil Velikov wrote:
>>>
>>> From: Emil Velikov <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Cc: Mauro Santos <[e-mail]>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>> Signed-off-by: Emil Velikov <[e-mail]>
>>> ---
>>> 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. :)

Emil Velikov Nov. 14, 2016, 10:46 a.m. UTC | #5

On 14 November 2016 at 09:56, Michel Dänzer <[e-mail]> wrote:
> On 10/11/16 10:38 PM, Emil Velikov wrote:
>> On 10 November 2016 at 12:40, Nicolai Hähnle <[e-mail]> wrote:
>>> On 09.11.2016 19:08, Emil Velikov wrote:
>>>>
>>>> From: Emil Velikov <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Cc: Mauro Santos <[e-mail]>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>>> Signed-off-by: Emil Velikov <[e-mail]>
>>>> ---
>>>> 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.
>
>
Makes perfect sense, tyvm !

>> 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. :)
>
Sticking with libdrm3.so.X + libdrm3.pc should sort that, right ? That
plus a simple sed job to make the symbols different/unique.

Regardless, anything libdrm3 is unrelated to the topic in question.
Emil

Patch

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;
+}
+
 /**
  * Output a message to stderr.
  *
@@ -2946,10 +2953,56 @@  static int drmGetMaxNodeName(void)
            3 /* length of the node number */;
 }
 
-static int drmParsePciDeviceInfo(const char *d_name,
-                                 drmPciDeviceInfoPtr device)
-{
 #ifdef __linux__
+static int parse_separate_sysfs_files(const char *d_name,
+                                      drmPciDeviceInfoPtr device)
+{
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
+    static const char *attrs[] = {
+      "revision", /* XXX: make sure it's always first, see note below */
+      "vendor",
+      "device",
+      "subsystem_vendor",
+      "subsystem_device",
+    };
+    char path[PATH_MAX + 1];
+    unsigned int data[ARRAY_SIZE(attrs)];
+    FILE *fp;
+    int ret;
+
+    for (unsigned i = 0; i < ARRAY_SIZE(attrs); i++) {
+        snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/%s",
+                 d_name, attrs[i]);
+        fp = fopen(path, "r");
+        if (!fp) {
+            /* Note: First we check the revision, since older kernels
+             * may not have it. Default to zero in such cases. */
+            if (i == 0) {
+                data[i] = 0;
+                continue;
+            }
+            return -errno;
+        }
+
+        ret = fscanf(fp, "%x", &data[i]);
+        fclose(fp);
+        if (ret != 1)
+            return -errno;
+
+    }
+
+    device->revision_id = data[0] & 0xff;
+    device->vendor_id = data[1] & 0xffff;
+    device->device_id = data[2] & 0xffff;
+    device->subvendor_id = data[3] & 0xffff;
+    device->subdevice_id = data[4] & 0xffff;
+
+    return 0;
+}
+
+static int parse_config_sysfs_file(const char *d_name,
+                                  drmPciDeviceInfoPtr device)
+{
     char path[PATH_MAX + 1];
     unsigned char config[64];
     int fd, ret;
@@ -2971,6 +3024,17 @@  static int drmParsePciDeviceInfo(const char *d_name,
     device->subdevice_id = config[46] | (config[47] << 8);
 
     return 0;
+}
+#endif
+
+static int drmParsePciDeviceInfo(const char *d_name,
+                                 drmPciDeviceInfoPtr device)
+{
+#ifdef __linux__
+    if (use_revision_file)
+        return parse_separate_sysfs_files(d_name, device);
+
+    return parse_config_sysfs_file(d_name, device);
 #else
 #warning "Missing implementation of drmParsePciDeviceInfo"
     return -EINVAL;
diff --git a/xf86drm.h b/xf86drm.h
index 481d882..d1ebc32 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -796,6 +796,17 @@  extern void drmFreeDevice(drmDevicePtr *device);
 extern int drmGetDevices(drmDevicePtr devices[], int max_devices);
 extern void drmFreeDevices(drmDevicePtr devices[], int count);
 
+
+/**
+ * Force any sequential calls to drmGetDevice/drmGetDevices to use the
+ * revision sysfs file instead of config one. The latter wakes up the device
+ * if powered off.
+ *
+ * A value of 0 will be returned for the revision_id field if the sysfs file
+ * is missing. DO NOT USE THIS if you depend on a correct revision_id.
+ */
+extern void drmDeviceUseRevisionFile(void);
+
 #if defined(__cplusplus)
 }
 #endif