[PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Emil Velikov]

From: Emil Velikov <emil.velikov@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 we can
'cheat' temporarily.

That should be fine, since no open-source project has ever used the
value.

Cc: Michel Dänzer <michel.daenzer@amd.com>
Cc: Mauro Santos <registo.mailling@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Mauro can you apply this against libdrm and rebuild it. You do _not_
need to rebuild mesa afterwords.

Thanks
---
 xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index 52add5e..5a5100c 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
                                  drmPciDeviceInfoPtr device)
 {
 #ifdef __linux__
+#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 char config[64];
-    int fd, ret;
+    unsigned int data[ARRAY_SIZE(attrs)];
+    FILE *fp;
+    int ret;
 
-    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
-    fd = open(path, O_RDONLY);
-    if (fd < 0)
-        return -errno;
+    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 = read(fd, config, sizeof(config));
-    close(fd);
-    if (ret < 0)
-        return -errno;
+        ret = fscanf(fp, "%x", &data[i]);
+        fclose(fp);
+        if (ret != 1)
+            return -errno;
+
+    }
 
-    device->vendor_id = config[0] | (config[1] << 8);
-    device->device_id = config[2] | (config[3] << 8);
-    device->revision_id = config[8];
-    device->subvendor_id = config[44] | (config[45] << 8);
-    device->subdevice_id = config[46] | (config[47] << 8);
+    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;
 #else
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Mauro Santos]

On 01-11-2016 18:13, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@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 we can
> 'cheat' temporarily.
> 
> That should be fine, since no open-source project has ever used the
> value.
> 
> Cc: Michel Dänzer <michel.daenzer@amd.com>
> Cc: Mauro Santos <registo.mailling@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> Mauro can you apply this against libdrm and rebuild it. You do _not_
> need to rebuild mesa afterwords.
> 
> Thanks
> ---
>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 52add5e..5a5100c 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>                                   drmPciDeviceInfoPtr device)
>  {
>  #ifdef __linux__
> +#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 char config[64];
> -    int fd, ret;
> +    unsigned int data[ARRAY_SIZE(attrs)];
> +    FILE *fp;
> +    int ret;
>  
> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
> -    fd = open(path, O_RDONLY);
> -    if (fd < 0)
> -        return -errno;
> +    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 = read(fd, config, sizeof(config));
> -    close(fd);
> -    if (ret < 0)
> -        return -errno;
> +        ret = fscanf(fp, "%x", &data[i]);
> +        fclose(fp);
> +        if (ret != 1)
> +            return -errno;
> +
> +    }
>  
> -    device->vendor_id = config[0] | (config[1] << 8);
> -    device->device_id = config[2] | (config[3] << 8);
> -    device->revision_id = config[8];
> -    device->subvendor_id = config[44] | (config[45] << 8);
> -    device->subdevice_id = config[46] | (config[47] << 8);
> +    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;
>  #else
> 

I have applied this against libdrm 2.4.71 and I don't see any delays
when starting firefox/chromium/thunderbird/glxgears.

There is also no indication in dmesg that the dGPU is being
reinitialized when starting the programs where I've detected the problem.

-- 
Mauro Santos
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Fwd: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Michel Dänzer]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]


The first attached patch will result in drmParsePciDeviceInfo always
reporting revision 0 on kernels without the second attached patch. Will
that be an issue for the amdgpu-pro stack?

Please follow up directly to the patch e-mails with any comments on the
patches.


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

[-- Attachment #2: [PATCH] PCI: create revision file in sysfs.eml --]
[-- Type: message/rfc822, Size: 8682 bytes --]

From: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Cc: "Jammy Zhou" <Jammy.Zhou-5C7GfCeVMHo@public.gmane.org>, emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, "Michel Dänzer" <michel.daenzer-5C7GfCeVMHo@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "Bjorn Helgaas" <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Subject: [PATCH] PCI: create revision file in sysfs
Date: Tue,  1 Nov 2016 15:42:32 +0000
Message-ID: <20161101154232.6451-1-emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

From: Emil Velikov <emil.velikov@collabora.com>

Currently the revision isn't available via sysfs/libudev thus if one
wants to know the value they need to read through the config file.

This in itself wakes/powers up the device, causing unwanted delay
since it can be quite costly.

Expose the revision as a separate file, just like we do for the device,
vendor, their subsystem version and class.

Cc: Jammy Zhou <Jammy.Zhou@amd.com>
Cc: Michel Dänzer <michel.daenzer@amd.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Gents, I'm not subscribed to the mailing list so please keep me in the
CC chain.

Thanks
Emil
---
 drivers/pci/pci-sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index bcd10c7..0666287 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -50,6 +50,7 @@ pci_config_attr(vendor, "0x%04x\n");
 pci_config_attr(device, "0x%04x\n");
 pci_config_attr(subsystem_vendor, "0x%04x\n");
 pci_config_attr(subsystem_device, "0x%04x\n");
+pci_config_attr(revision, "0x%02x\n");
 pci_config_attr(class, "0x%06x\n");
 pci_config_attr(irq, "%u\n");
 
@@ -568,6 +569,7 @@ static struct attribute *pci_dev_attrs[] = {
 	&dev_attr_device.attr,
 	&dev_attr_subsystem_vendor.attr,
 	&dev_attr_subsystem_device.attr,
+	&dev_attr_revision.attr,
 	&dev_attr_class.attr,
 	&dev_attr_irq.attr,
 	&dev_attr_local_cpus.attr,
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #3: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info.eml --]
[-- Type: message/rfc822, Size: 10424 bytes --]

From: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Cc: "Michel Dänzer" <michel.daenzer-5C7GfCeVMHo@public.gmane.org>, emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, "Mauro Santos" <registo.mailling-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Subject: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info
Date: Tue,  1 Nov 2016 18:13:34 +0000
Message-ID: <20161101181334.8225-1-emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

From: Emil Velikov <emil.velikov@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 we can
'cheat' temporarily.

That should be fine, since no open-source project has ever used the
value.

Cc: Michel Dänzer <michel.daenzer@amd.com>
Cc: Mauro Santos <registo.mailling@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Mauro can you apply this against libdrm and rebuild it. You do _not_
need to rebuild mesa afterwords.

Thanks
---
 xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index 52add5e..5a5100c 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
                                  drmPciDeviceInfoPtr device)
 {
 #ifdef __linux__
+#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 char config[64];
-    int fd, ret;
+    unsigned int data[ARRAY_SIZE(attrs)];
+    FILE *fp;
+    int ret;
 
-    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
-    fd = open(path, O_RDONLY);
-    if (fd < 0)
-        return -errno;
+    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 = read(fd, config, sizeof(config));
-    close(fd);
-    if (ret < 0)
-        return -errno;
+        ret = fscanf(fp, "%x", &data[i]);
+        fclose(fp);
+        if (ret != 1)
+            return -errno;
+
+    }
 
-    device->vendor_id = config[0] | (config[1] << 8);
-    device->device_id = config[2] | (config[3] << 8);
-    device->revision_id = config[8];
-    device->subvendor_id = config[44] | (config[45] << 8);
-    device->subdevice_id = config[46] | (config[47] << 8);
+    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;
 #else
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #4: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Peter Wu]

On Tue, Nov 01, 2016 at 06:13:34PM +0000, Emil Velikov wrote:
>  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 we can
> 'cheat' temporarily.
> 
> That should be fine, since no open-source project has ever used the
> value.
> 
> Cc: Michel Dänzer <michel.daenzer@amd.com>
> Cc: Mauro Santos <registo.mailling@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

See below for one observation. Other than that, strace confirms that
the new sysfs files are being accessed.

Reviewed-and-tested-by: Peter Wu <peter@lekensteyn.nl>

This was tested with Linux 4.8.4 (unpatched) and libdrm 2.4.71 (+this
patch) with i915 + nouveau. Tested with running glxgears and glxinfo.
Note that glxinfo still accesses 'config' due to libpciaccess.

+ drm_intel_get_aperture_sizes
  + drm_intel_probe_agp_aperture_size
    + pci_system_init()
      + pci_system_linux_sysfs_create
        + populate_entries
          + pci_device_linux_sysfs_read() <-- offending function
    + pci_device_find_by_slot(0, 0, 2, 0)

That populate_entries function can probably use a fix similar to this
one.

> ---
> Mauro can you apply this against libdrm and rebuild it. You do _not_
> need to rebuild mesa afterwords.
> 
> Thanks
> ---
>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 52add5e..5a5100c 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>                                   drmPciDeviceInfoPtr device)
>  {
>  #ifdef __linux__
> +#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];

The size for snprintf already includes the nul-terminator, so strictly
speaking the +1 is not needed. It does not hurt either though.

> -    unsigned char config[64];
> -    int fd, ret;
> +    unsigned int data[ARRAY_SIZE(attrs)];
> +    FILE *fp;
> +    int ret;
>  
> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
> -    fd = open(path, O_RDONLY);
> -    if (fd < 0)
> -        return -errno;
> +    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 = read(fd, config, sizeof(config));
> -    close(fd);
> -    if (ret < 0)
> -        return -errno;
> +        ret = fscanf(fp, "%x", &data[i]);
> +        fclose(fp);
> +        if (ret != 1)
> +            return -errno;
> +
> +    }
>  
> -    device->vendor_id = config[0] | (config[1] << 8);
> -    device->device_id = config[2] | (config[3] << 8);
> -    device->revision_id = config[8];
> -    device->subvendor_id = config[44] | (config[45] << 8);
> -    device->subdevice_id = config[46] | (config[47] << 8);
> +    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;
>  #else
> -- 
> 2.10.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Emil Velikov]

On 2 November 2016 at 11:14, Peter Wu <peter@lekensteyn.nl> wrote:
> On Tue, Nov 01, 2016 at 06:13:34PM +0000, Emil Velikov wrote:
>>  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 we can
>> 'cheat' temporarily.
>>
>> That should be fine, since no open-source project has ever used the
>> value.
>>
>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>> Cc: Mauro Santos <registo.mailling@gmail.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>
> See below for one observation. Other than that, strace confirms that
> the new sysfs files are being accessed.
>
> Reviewed-and-tested-by: Peter Wu <peter@lekensteyn.nl>
>
> This was tested with Linux 4.8.4 (unpatched) and libdrm 2.4.71 (+this
> patch) with i915 + nouveau. Tested with running glxgears and glxinfo.
> Note that glxinfo still accesses 'config' due to libpciaccess.
>
> + drm_intel_get_aperture_sizes
>   + drm_intel_probe_agp_aperture_size
>     + pci_system_init()
>       + pci_system_linux_sysfs_create
>         + populate_entries
>           + pci_device_linux_sysfs_read() <-- offending function
>     + pci_device_find_by_slot(0, 0, 2, 0)
>
> That populate_entries function can probably use a fix similar to this
> one.
>
Indeed it would, although we ought to check if that won't cause any
(extra) regressions.

Skimming through my distro (Arch) the following depend on libpciaccess:

intel-gpu-tools
libdrm
libvirt
radeontool
spice-vdagent
vbetool
xorg-server

Of which the first two are safe, while the last one isn't + it even
exports the revision to DDX via xf86str.h's GDevRec::chipRev
Which gives us even more places to check through. Can you lend a hand ?

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Peter Wu]

On Wed, Nov 02, 2016 at 11:47:03AM +0000, Emil Velikov wrote:
> On 2 November 2016 at 11:14, Peter Wu <peter@lekensteyn.nl> wrote:
> > On Tue, Nov 01, 2016 at 06:13:34PM +0000, Emil Velikov wrote:
> >>  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 we can
> >> 'cheat' temporarily.
> >>
> >> That should be fine, since no open-source project has ever used the
> >> value.
> >>
> >> Cc: Michel Dänzer <michel.daenzer@amd.com>
> >> Cc: Mauro Santos <registo.mailling@gmail.com>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> >> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> >
> > See below for one observation. Other than that, strace confirms that
> > the new sysfs files are being accessed.
> >
> > Reviewed-and-tested-by: Peter Wu <peter@lekensteyn.nl>
> >
> > This was tested with Linux 4.8.4 (unpatched) and libdrm 2.4.71 (+this
> > patch) with i915 + nouveau. Tested with running glxgears and glxinfo.
> > Note that glxinfo still accesses 'config' due to libpciaccess.
> >
> > + drm_intel_get_aperture_sizes
> >   + drm_intel_probe_agp_aperture_size
> >     + pci_system_init()
> >       + pci_system_linux_sysfs_create
> >         + populate_entries
> >           + pci_device_linux_sysfs_read() <-- offending function
> >     + pci_device_find_by_slot(0, 0, 2, 0)
> >
> > That populate_entries function can probably use a fix similar to this
> > one.
> >
> Indeed it would, although we ought to check if that won't cause any
> (extra) regressions.
> 
> Skimming through my distro (Arch) the following depend on libpciaccess:
> 
> intel-gpu-tools

Does not use the "revision" field.

> libdrm

Does not use the "revision" field of libpciaccess.

While amdgpu has the "pci_rev" line below, it is copied from the
response of an ioctl, not pciaccess, so it is safe:

    drm_private int amdgpu_query_gpu_info_init(amdgpu_device_handle dev)
    {
        int r, i;

        r = amdgpu_query_info(dev, AMDGPU_INFO_DEV_INFO, sizeof(dev->dev_info),
                      &dev->dev_info);
        // ...
        dev->info.pci_rev_id = dev->dev_info.pci_rev;

> libvirt
> radeontool
> spice-vdagent
> vbetool

These four do not use the "revision" field and are safe.

> xorg-server
> 
> Of which the first two are safe, while the last one isn't + it even
> exports the revision to DDX via xf86str.h's GDevRec::chipRev

xorg-server internally does not use the revision field, but it exports
them as you have observed:

    GDevRec::chipRev
        (copied from libpciaccess, struct pci_device::revision)
    XF86ConfDevicePtr::dev_chiprev (copied from GDevRec::chipRev)

Not knowing what the users are, I tried to have a look at the git logs
for "chipRev", but the change introducing it is not that helpful:

    commit ded6147bfb5d75ff1e67c858040a628b61bc17d1
    Author: Kaleb Keithley <kaleb@freedesktop.org>
    Date:   Fri Nov 14 15:54:54 2003 +0000

        R6.6 is the Xorg base-line
    ...
     609 files changed, 262690 insertions(+)

To play safe, you could fallback to "config" in sysfs when "revision" is
not found, that would allow older kernels to work with no regressions in
the information while reducing the runtime wakeups for newer kernels.

> Which gives us even more places to check through. Can you lend a hand ?
> 
> Thanks
> Emil

I am also on Arch, what other places are you thinking about? DDXes or
other users of libpciaccess?

Kind regards,
Peter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Emil Velikov]

HI all,

On 2 November 2016 at 03:07, Michel Dänzer <michel@daenzer.net> wrote:
>
> The first attached patch will result in drmParsePciDeviceInfo always
> reporting revision 0 on kernels without the second attached patch. Will
> that be an issue for the amdgpu-pro stack?
>
> Please follow up directly to the patch e-mails with any comments on the
> patches.
>
Fleshing out the question from the actual patches:

Do the AMDGPU-PRO or the AMD stack [as a whole] depend on the revision
field as returned by the drmDevice API ?

Since we have a lovely bug in libdrm and might roll out a release
soonish it'll be great to have this squashed/merged as well.

Thanks
Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Michel Dänzer]

On 05/11/16 03:14 AM, Emil Velikov wrote:
> On 2 November 2016 at 03:07, Michel Dänzer <michel@daenzer.net> wrote:
>>
>> The first attached patch will result in drmParsePciDeviceInfo always
>> reporting revision 0 on kernels without the second attached patch. Will
>> that be an issue for the amdgpu-pro stack?
>>
>> Please follow up directly to the patch e-mails with any comments on the
>> patches.
>>
> Fleshing out the question from the actual patches:
> 
> Do the AMDGPU-PRO or the AMD stack [as a whole] depend on the revision
> field as returned by the drmDevice API ?

One answer is that https://patchwork.freedesktop.org/patch/120132/ uses
the revision ID. In this case a wrong revision ID would only cause a
cosmetic issue, but I can imagine that other code in the amdgpu-pro
stack really needs the correct revision ID to accurately identify the GPU.


> Since we have a lovely bug in libdrm and might roll out a release
> soonish it'll be great to have this squashed/merged as well.

I hope the release can wait for the patch above to land as well.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Emil Velikov]

On 7 November 2016 at 09:14, Michel Dänzer <michel@daenzer.net> wrote:
> On 05/11/16 03:14 AM, Emil Velikov wrote:
>> On 2 November 2016 at 03:07, Michel Dänzer <michel@daenzer.net> wrote:
>>>
>>> The first attached patch will result in drmParsePciDeviceInfo always
>>> reporting revision 0 on kernels without the second attached patch. Will
>>> that be an issue for the amdgpu-pro stack?
>>>
>>> Please follow up directly to the patch e-mails with any comments on the
>>> patches.
>>>
>> Fleshing out the question from the actual patches:
>>
>> Do the AMDGPU-PRO or the AMD stack [as a whole] depend on the revision
>> field as returned by the drmDevice API ?
>
> One answer is that https://patchwork.freedesktop.org/patch/120132/ uses
Nice, I really like this ! Wonder if others will do the same, rather
than duplicating it throughout ddx/mesa/other drivers. That aside,

> the revision ID. In this case a wrong revision ID would only cause a
> cosmetic issue, but I can imagine that other code in the amdgpu-pro
> stack really needs the correct revision ID to accurately identify the GPU.
>
Don't mean to sound rude, but I was hoping for a definite answer.
Regardless, do you/fellow AMD devs, any preference on how to go with
this bug [1] ?

Add an override to force use of the revision file - be that envvar,
new API {drmDeviceUseRevisionFile, drmDevice...v2}, or revert the 12 +
commits (pulling only the offending one won't cut it). Obviously I'm
not a huge fan of the last one :-\

[1] https://bugs.freedesktop.org/show_bug.cgi?id=98502

>
>> Since we have a lovely bug in libdrm and might roll out a release
>> soonish it'll be great to have this squashed/merged as well.
>
> I hope the release can wait for the patch above to land as well.
>
Atm, we crash for anyone using !pci devices, so I'd like to spare
that. So it'll be great if this lands in the next days/week.

Thanks
Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Michel Dänzer]

On 07/11/16 08:30 PM, Emil Velikov wrote:
> On 7 November 2016 at 09:14, Michel Dänzer <michel@daenzer.net> wrote:
>> On 05/11/16 03:14 AM, Emil Velikov wrote:
>>> On 2 November 2016 at 03:07, Michel Dänzer <michel@daenzer.net> wrote:
>>>>
>>>> The first attached patch will result in drmParsePciDeviceInfo always
>>>> reporting revision 0 on kernels without the second attached patch. Will
>>>> that be an issue for the amdgpu-pro stack?
>>>>
>>>> Please follow up directly to the patch e-mails with any comments on the
>>>> patches.
>>>>
>>> Fleshing out the question from the actual patches:
>>>
>>> Do the AMDGPU-PRO or the AMD stack [as a whole] depend on the revision
>>> field as returned by the drmDevice API ?
>>
>> One answer is that https://patchwork.freedesktop.org/patch/120132/ uses
>> the revision ID. In this case a wrong revision ID would only cause a
>> cosmetic issue, but I can imagine that other code in the amdgpu-pro
>> stack really needs the correct revision ID to accurately identify the GPU.
>>
> Don't mean to sound rude, but I was hoping for a definite answer.

So was I. :}

Digging further, the above patch actually doesn't use the revision_id
field but amdgpu kernel driver functionality to determine the revision.
Given that such functionality exists, I don't think we have do any more
special consideration of either amdgpu stack.


> Regardless, do you/fellow AMD devs, any preference on how to go with
> this bug [1] ?
> 
> Add an override to force use of the revision file - be that envvar,
> new API {drmDeviceUseRevisionFile, drmDevice...v2}, or revert the 12 +
> commits (pulling only the offending one won't cut it). Obviously I'm
> not a huge fan of the last one :-\
> 
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502

I'm afraid I don't have any particularly strong opinion to offer here.
But it seems weird to me to have an API which pretends to provide the
revision ID, but it can actually be incorrect. (The same would apply to
any other information, not just the revision ID in particular)


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Emil Velikov]

On 1 November 2016 at 18:47, Mauro Santos <registo.mailling@gmail.com> wrote:
> On 01-11-2016 18:13, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov@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 we can
>> 'cheat' temporarily.
>>
>> That should be fine, since no open-source project has ever used the
>> value.
>>
>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>> Cc: Mauro Santos <registo.mailling@gmail.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>> ---
>> Mauro can you apply this against libdrm and rebuild it. You do _not_
>> need to rebuild mesa afterwords.
>>
>> Thanks
>> ---
>>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 35 insertions(+), 15 deletions(-)
>>
>> diff --git a/xf86drm.c b/xf86drm.c
>> index 52add5e..5a5100c 100644
>> --- a/xf86drm.c
>> +++ b/xf86drm.c
>> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>>                                   drmPciDeviceInfoPtr device)
>>  {
>>  #ifdef __linux__
>> +#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 char config[64];
>> -    int fd, ret;
>> +    unsigned int data[ARRAY_SIZE(attrs)];
>> +    FILE *fp;
>> +    int ret;
>>
>> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
>> -    fd = open(path, O_RDONLY);
>> -    if (fd < 0)
>> -        return -errno;
>> +    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 = read(fd, config, sizeof(config));
>> -    close(fd);
>> -    if (ret < 0)
>> -        return -errno;
>> +        ret = fscanf(fp, "%x", &data[i]);
>> +        fclose(fp);
>> +        if (ret != 1)
>> +            return -errno;
>> +
>> +    }
>>
>> -    device->vendor_id = config[0] | (config[1] << 8);
>> -    device->device_id = config[2] | (config[3] << 8);
>> -    device->revision_id = config[8];
>> -    device->subvendor_id = config[44] | (config[45] << 8);
>> -    device->subdevice_id = config[46] | (config[47] << 8);
>> +    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;
>>  #else
>>
>
> I have applied this against libdrm 2.4.71 and I don't see any delays
> when starting firefox/chromium/thunderbird/glxgears.
>
> There is also no indication in dmesg that the dGPU is being
> reinitialized when starting the programs where I've detected the problem.
>
Thanks Mauro. Can you give this a try alongside the kernel fix [1] ?
I'd love to get the latter merged soon(ish).
Independent of the kernel side, I might need to go another way for
libdrm/mesa so I'll CC you on future patches.

Your help is greatly appreciated !

Thanks
Emil

[1] http://patchwork.ozlabs.org/patch/689975/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Emil Velikov]

On 8 November 2016 at 08:44, Michel Dänzer <michel@daenzer.net> wrote:
> On 07/11/16 08:30 PM, Emil Velikov wrote:
>> On 7 November 2016 at 09:14, Michel Dänzer <michel@daenzer.net> wrote:
>>> On 05/11/16 03:14 AM, Emil Velikov wrote:
>>>> On 2 November 2016 at 03:07, Michel Dänzer <michel@daenzer.net> wrote:
>>>>>
>>>>> The first attached patch will result in drmParsePciDeviceInfo always
>>>>> reporting revision 0 on kernels without the second attached patch. Will
>>>>> that be an issue for the amdgpu-pro stack?
>>>>>
>>>>> Please follow up directly to the patch e-mails with any comments on the
>>>>> patches.
>>>>>
>>>> Fleshing out the question from the actual patches:
>>>>
>>>> Do the AMDGPU-PRO or the AMD stack [as a whole] depend on the revision
>>>> field as returned by the drmDevice API ?
>>>
>>> One answer is that https://patchwork.freedesktop.org/patch/120132/ uses
>>> the revision ID. In this case a wrong revision ID would only cause a
>>> cosmetic issue, but I can imagine that other code in the amdgpu-pro
>>> stack really needs the correct revision ID to accurately identify the GPU.
>>>
>> Don't mean to sound rude, but I was hoping for a definite answer.
>
> So was I. :}
>
> Digging further, the above patch actually doesn't use the revision_id
> field but amdgpu kernel driver functionality to determine the revision.
> Given that such functionality exists, I don't think we have do any more
> special consideration of either amdgpu stack.
>
>
>> Regardless, do you/fellow AMD devs, any preference on how to go with
>> this bug [1] ?
>>
>> Add an override to force use of the revision file - be that envvar,
>> new API {drmDeviceUseRevisionFile, drmDevice...v2}, or revert the 12 +
>> commits (pulling only the offending one won't cut it). Obviously I'm
>> not a huge fan of the last one :-\
>>
>> [1] https://bugs.freedesktop.org/show_bug.cgi?id=98502
>
> I'm afraid I don't have any particularly strong opinion to offer here.
> But it seems weird to me to have an API which pretends to provide the
> revision ID, but it can actually be incorrect. (The same would apply to
> any other information, not just the revision ID in particular)
>
Indeed it is quite nasty. A quick and short solution, which doesn't
break existing users, is to have
drmDeviceForceUseRevisionFile(). I'll give it a stab, adding a juicy
comment/doc about it.

As we get to libdrm3 we can remove this/other hacks, but until then
this will do just fine ?

Thanks
Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Mauro Santos]

On 08-11-2016 11:06, Emil Velikov wrote:
> On 1 November 2016 at 18:47, Mauro Santos <registo.mailling@gmail.com> wrote:
>> On 01-11-2016 18:13, Emil Velikov wrote:
>>> From: Emil Velikov <emil.velikov@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 we can
>>> 'cheat' temporarily.
>>>
>>> That should be fine, since no open-source project has ever used the
>>> value.
>>>
>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>> Cc: Mauro Santos <registo.mailling@gmail.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>> ---
>>> Mauro can you apply this against libdrm and rebuild it. You do _not_
>>> need to rebuild mesa afterwords.
>>>
>>> Thanks
>>> ---
>>>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>>>  1 file changed, 35 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/xf86drm.c b/xf86drm.c
>>> index 52add5e..5a5100c 100644
>>> --- a/xf86drm.c
>>> +++ b/xf86drm.c
>>> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>>>                                   drmPciDeviceInfoPtr device)
>>>  {
>>>  #ifdef __linux__
>>> +#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 char config[64];
>>> -    int fd, ret;
>>> +    unsigned int data[ARRAY_SIZE(attrs)];
>>> +    FILE *fp;
>>> +    int ret;
>>>
>>> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
>>> -    fd = open(path, O_RDONLY);
>>> -    if (fd < 0)
>>> -        return -errno;
>>> +    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 = read(fd, config, sizeof(config));
>>> -    close(fd);
>>> -    if (ret < 0)
>>> -        return -errno;
>>> +        ret = fscanf(fp, "%x", &data[i]);
>>> +        fclose(fp);
>>> +        if (ret != 1)
>>> +            return -errno;
>>> +
>>> +    }
>>>
>>> -    device->vendor_id = config[0] | (config[1] << 8);
>>> -    device->device_id = config[2] | (config[3] << 8);
>>> -    device->revision_id = config[8];
>>> -    device->subvendor_id = config[44] | (config[45] << 8);
>>> -    device->subdevice_id = config[46] | (config[47] << 8);
>>> +    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;
>>>  #else
>>>
>>
>> I have applied this against libdrm 2.4.71 and I don't see any delays
>> when starting firefox/chromium/thunderbird/glxgears.
>>
>> There is also no indication in dmesg that the dGPU is being
>> reinitialized when starting the programs where I've detected the problem.
>>
> Thanks Mauro. Can you give this a try alongside the kernel fix [1] ?
> I'd love to get the latter merged soon(ish).
> Independent of the kernel side, I might need to go another way for
> libdrm/mesa so I'll CC you on future patches.
> 
> Your help is greatly appreciated !
> 
> Thanks
> Emil
> 
> [1] http://patchwork.ozlabs.org/patch/689975/
> 

I have applied the patch on top of kernel 4.8.6 and I'm using the libdrm
with the patch you sent me previously.

With this patch things still seem work fine, I don't see any of the
problems I've seen before, but I don't know how to confirm that the
value from sysfs is now being used by libdrm instead of defaulting to zero.

The values in /sys/class/drm/card{0,1}/device/revision match what is
reported by lspci for the iGPU and dGPU.
I don't know if it's related but the revision file in
/sys/class/pci_bus/0000\:03/device/ does show 0xf1 for both GPUs, which
is different from what is shown in /sys/class/drm/card{0,1}/device/revision.

-- 
Mauro Santos
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Emil Velikov]

On 8 November 2016 at 13:38, Mauro Santos <registo.mailling@gmail.com> wrote:
> On 08-11-2016 11:06, Emil Velikov wrote:
>> On 1 November 2016 at 18:47, Mauro Santos <registo.mailling@gmail.com> wrote:
>>> On 01-11-2016 18:13, Emil Velikov wrote:
>>>> From: Emil Velikov <emil.velikov@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 we can
>>>> 'cheat' temporarily.
>>>>
>>>> That should be fine, since no open-source project has ever used the
>>>> value.
>>>>
>>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>>> Cc: Mauro Santos <registo.mailling@gmail.com>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>>> ---
>>>> Mauro can you apply this against libdrm and rebuild it. You do _not_
>>>> need to rebuild mesa afterwords.
>>>>
>>>> Thanks
>>>> ---
>>>>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>>>>  1 file changed, 35 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/xf86drm.c b/xf86drm.c
>>>> index 52add5e..5a5100c 100644
>>>> --- a/xf86drm.c
>>>> +++ b/xf86drm.c
>>>> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>>>>                                   drmPciDeviceInfoPtr device)
>>>>  {
>>>>  #ifdef __linux__
>>>> +#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 char config[64];
>>>> -    int fd, ret;
>>>> +    unsigned int data[ARRAY_SIZE(attrs)];
>>>> +    FILE *fp;
>>>> +    int ret;
>>>>
>>>> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
>>>> -    fd = open(path, O_RDONLY);
>>>> -    if (fd < 0)
>>>> -        return -errno;
>>>> +    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 = read(fd, config, sizeof(config));
>>>> -    close(fd);
>>>> -    if (ret < 0)
>>>> -        return -errno;
>>>> +        ret = fscanf(fp, "%x", &data[i]);
>>>> +        fclose(fp);
>>>> +        if (ret != 1)
>>>> +            return -errno;
>>>> +
>>>> +    }
>>>>
>>>> -    device->vendor_id = config[0] | (config[1] << 8);
>>>> -    device->device_id = config[2] | (config[3] << 8);
>>>> -    device->revision_id = config[8];
>>>> -    device->subvendor_id = config[44] | (config[45] << 8);
>>>> -    device->subdevice_id = config[46] | (config[47] << 8);
>>>> +    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;
>>>>  #else
>>>>
>>>
>>> I have applied this against libdrm 2.4.71 and I don't see any delays
>>> when starting firefox/chromium/thunderbird/glxgears.
>>>
>>> There is also no indication in dmesg that the dGPU is being
>>> reinitialized when starting the programs where I've detected the problem.
>>>
>> Thanks Mauro. Can you give this a try alongside the kernel fix [1] ?
>> I'd love to get the latter merged soon(ish).
>> Independent of the kernel side, I might need to go another way for
>> libdrm/mesa so I'll CC you on future patches.
>>
>> Your help is greatly appreciated !
>>
>> Thanks
>> Emil
>>
>> [1] http://patchwork.ozlabs.org/patch/689975/
>>
>
> I have applied the patch on top of kernel 4.8.6 and I'm using the libdrm
> with the patch you sent me previously.
>
> With this patch things still seem work fine, I don't see any of the
> problems I've seen before, but I don't know how to confirm that the
> value from sysfs is now being used by libdrm instead of defaulting to zero.
>
Grr my bad. $libdrm_builddir/tests/drmdevice should do it. You might
need to explicitly build it (cd tests && make drmdevice)

> The values in /sys/class/drm/card{0,1}/device/revision match what is
> reported by lspci for the iGPU and dGPU.
> I don't know if it's related but the revision file in
> /sys/class/pci_bus/0000\:03/device/ does show 0xf1 for both GPUs, which
> is different from what is shown in /sys/class/drm/card{0,1}/device/revision.
>
That's perfect, thanks.

0000:03 is (in all likelihood) is the PCI bridge. You can check with
the device uevent/PCI_SLOT_NAME and cross reference with lspci.


Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Mauro Santos]

On 08-11-2016 15:00, Emil Velikov wrote:
> On 8 November 2016 at 13:38, Mauro Santos <registo.mailling@gmail.com> wrote:
>> On 08-11-2016 11:06, Emil Velikov wrote:
>>> On 1 November 2016 at 18:47, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>> On 01-11-2016 18:13, Emil Velikov wrote:
>>>>> From: Emil Velikov <emil.velikov@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 we can
>>>>> 'cheat' temporarily.
>>>>>
>>>>> That should be fine, since no open-source project has ever used the
>>>>> value.
>>>>>
>>>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>>>> Cc: Mauro Santos <registo.mailling@gmail.com>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>>>> ---
>>>>> Mauro can you apply this against libdrm and rebuild it. You do _not_
>>>>> need to rebuild mesa afterwords.
>>>>>
>>>>> Thanks
>>>>> ---
>>>>>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>>>>>  1 file changed, 35 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/xf86drm.c b/xf86drm.c
>>>>> index 52add5e..5a5100c 100644
>>>>> --- a/xf86drm.c
>>>>> +++ b/xf86drm.c
>>>>> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>>>>>                                   drmPciDeviceInfoPtr device)
>>>>>  {
>>>>>  #ifdef __linux__
>>>>> +#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 char config[64];
>>>>> -    int fd, ret;
>>>>> +    unsigned int data[ARRAY_SIZE(attrs)];
>>>>> +    FILE *fp;
>>>>> +    int ret;
>>>>>
>>>>> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
>>>>> -    fd = open(path, O_RDONLY);
>>>>> -    if (fd < 0)
>>>>> -        return -errno;
>>>>> +    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 = read(fd, config, sizeof(config));
>>>>> -    close(fd);
>>>>> -    if (ret < 0)
>>>>> -        return -errno;
>>>>> +        ret = fscanf(fp, "%x", &data[i]);
>>>>> +        fclose(fp);
>>>>> +        if (ret != 1)
>>>>> +            return -errno;
>>>>> +
>>>>> +    }
>>>>>
>>>>> -    device->vendor_id = config[0] | (config[1] << 8);
>>>>> -    device->device_id = config[2] | (config[3] << 8);
>>>>> -    device->revision_id = config[8];
>>>>> -    device->subvendor_id = config[44] | (config[45] << 8);
>>>>> -    device->subdevice_id = config[46] | (config[47] << 8);
>>>>> +    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;
>>>>>  #else
>>>>>
>>>>
>>>> I have applied this against libdrm 2.4.71 and I don't see any delays
>>>> when starting firefox/chromium/thunderbird/glxgears.
>>>>
>>>> There is also no indication in dmesg that the dGPU is being
>>>> reinitialized when starting the programs where I've detected the problem.
>>>>
>>> Thanks Mauro. Can you give this a try alongside the kernel fix [1] ?
>>> I'd love to get the latter merged soon(ish).
>>> Independent of the kernel side, I might need to go another way for
>>> libdrm/mesa so I'll CC you on future patches.
>>>
>>> Your help is greatly appreciated !
>>>
>>> Thanks
>>> Emil
>>>
>>> [1] http://patchwork.ozlabs.org/patch/689975/
>>>
>>
>> I have applied the patch on top of kernel 4.8.6 and I'm using the libdrm
>> with the patch you sent me previously.
>>
>> With this patch things still seem work fine, I don't see any of the
>> problems I've seen before, but I don't know how to confirm that the
>> value from sysfs is now being used by libdrm instead of defaulting to zero.
>>
> Grr my bad. $libdrm_builddir/tests/drmdevice should do it. You might
> need to explicitly build it (cd tests && make drmdevice)
> 

When running drmdevice as my user it still wakes up the dGPU. The
correct device revisions are being reported by I suppose that is not
being read from sysfs.

sudo dmesg -C && dmesg -w & ./drmdevice

device[0]
	available_nodes 0007
	nodes
		nodes[0] /dev/dri/card1
		nodes[1] /dev/dri/controlD65
		nodes[2] /dev/dri/renderD129
	bustype 0000
	businfo
		pci
			domain	0000
			bus	03
			dev	00
			func	0
	deviceinfo
		pci
			vendor_id	1002
			device_id	6600
			subvendor_id	17aa
			subdevice_id	5049
			revision_id	81

Opening device 0 node /dev/dri/card1
[ 7155.613212] [drm] probing gen 2 caps for device 8086:9d18 = 9724043/e
[ 7155.613218] [drm] enabling PCIE gen 3 link speeds, disable with
radeon.pcie_gen2=0
[ 7157.006460] [drm] PCIE GART of 2048M enabled (table at
0x00000000001D6000).
[ 7157.006603] radeon 0000:03:00.0: WB enabled
[ 7157.006607] radeon 0000:03:00.0: fence driver on ring 0 use gpu addr
0x0000000080000c00 and cpu addr 0xffff88042bb1fc00
[ 7157.006610] radeon 0000:03:00.0: fence driver on ring 1 use gpu addr
0x0000000080000c04 and cpu addr 0xffff88042bb1fc04
[ 7157.006612] radeon 0000:03:00.0: fence driver on ring 2 use gpu addr
0x0000000080000c08 and cpu addr 0xffff88042bb1fc08
[ 7157.006614] radeon 0000:03:00.0: fence driver on ring 3 use gpu addr
0x0000000080000c0c and cpu addr 0xffff88042bb1fc0c
[ 7157.006616] radeon 0000:03:00.0: fence driver on ring 4 use gpu addr
0x0000000080000c10 and cpu addr 0xffff88042bb1fc10
[ 7157.007411] radeon 0000:03:00.0: fence driver on ring 5 use gpu addr
0x0000000000075a18 and cpu addr 0xffffc90002235a18
[ 7157.109456] radeon 0000:03:00.0: failed VCE resume (-110).
[ 7157.340129] [drm] ring test on 0 succeeded in 1 usecs
[ 7157.340136] [drm] ring test on 1 succeeded in 1 usecs
[ 7157.340142] [drm] ring test on 2 succeeded in 1 usecs
[ 7157.340153] [drm] ring test on 3 succeeded in 4 usecs
[ 7157.340163] [drm] ring test on 4 succeeded in 4 usecs
[ 7157.517337] [drm] ring test on 5 succeeded in 2 usecs
[ 7157.517344] [drm] UVD initialized successfully.
[ 7157.517392] [drm] ib test on ring 0 succeeded in 0 usecs
[ 7157.517431] [drm] ib test on ring 1 succeeded in 0 usecs
[ 7157.517474] [drm] ib test on ring 2 succeeded in 0 usecs
[ 7157.517510] [drm] ib test on ring 3 succeeded in 0 usecs
[ 7157.517546] [drm] ib test on ring 4 succeeded in 0 usecs
[ 7158.169561] [drm] ib test on ring 5 succeeded
device[0]
	available_nodes 0007
	nodes
		nodes[0] /dev/dri/card1
		nodes[1] /dev/dri/controlD65
		nodes[2] /dev/dri/renderD129
	bustype 0000
	businfo
		pci
			domain	0000
			bus	03
			dev	00
			func	0
	deviceinfo
		pci
			vendor_id	1002
			device_id	6600
			subvendor_id	17aa
			subdevice_id	5049
			revision_id	81

Opening device 0 node /dev/dri/controlD65
Failed - Permission denied (13)
Opening device 0 node /dev/dri/renderD129
device[0]
	available_nodes 0007
	nodes
		nodes[0] /dev/dri/card1
		nodes[1] /dev/dri/controlD65
		nodes[2] /dev/dri/renderD129
	bustype 0000
	businfo
		pci
			domain	0000
			bus	03
			dev	00
			func	0
	deviceinfo
		pci
			vendor_id	1002
			device_id	6600
			subvendor_id	17aa
			subdevice_id	5049
			revision_id	81

device[1]
	available_nodes 0007
	nodes
		nodes[0] /dev/dri/card0
		nodes[1] /dev/dri/controlD64
		nodes[2] /dev/dri/renderD128
	bustype 0000
	businfo
		pci
			domain	0000
			bus	00
			dev	02
			func	0
	deviceinfo
		pci
			vendor_id	8086
			device_id	1916
			subvendor_id	17aa
			subdevice_id	5049
			revision_id	07

Opening device 1 node /dev/dri/card0
device[1]
	available_nodes 0007
	nodes
		nodes[0] /dev/dri/card0
		nodes[1] /dev/dri/controlD64
		nodes[2] /dev/dri/renderD128
	bustype 0000
	businfo
		pci
			domain	0000
			bus	00
			dev	02
			func	0
	deviceinfo
		pci
			vendor_id	8086
			device_id	1916
			subvendor_id	17aa
			subdevice_id	5049
			revision_id	07

Opening device 1 node /dev/dri/controlD64
Failed - Permission denied (13)
Opening device 1 node /dev/dri/renderD128
device[1]
	available_nodes 0007
	nodes
		nodes[0] /dev/dri/card0
		nodes[1] /dev/dri/controlD64
		nodes[2] /dev/dri/renderD128
	bustype 0000
	businfo
		pci
			domain	0000
			bus	00
			dev	02
			func	0
	deviceinfo
		pci
			vendor_id	8086
			device_id	1916
			subvendor_id	17aa
			subdevice_id	5049
			revision_id	07




>> The values in /sys/class/drm/card{0,1}/device/revision match what is
>> reported by lspci for the iGPU and dGPU.
>> I don't know if it's related but the revision file in
>> /sys/class/pci_bus/0000\:03/device/ does show 0xf1 for both GPUs, which
>> is different from what is shown in /sys/class/drm/card{0,1}/device/revision.
>>
> That's perfect, thanks.
> 
> 0000:03 is (in all likelihood) is the PCI bridge. You can check with
> the device uevent/PCI_SLOT_NAME and cross reference with lspci.
> 

This is right on the money, uevent/PCI_SLOT_NAME does point to the PCI
bridge.

> 
> Emil
> 


-- 
Mauro Santos
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Emil Velikov]

On 8 November 2016 at 15:27, Mauro Santos <registo.mailling@gmail.com> wrote:
> On 08-11-2016 15:00, Emil Velikov wrote:
>> On 8 November 2016 at 13:38, Mauro Santos <registo.mailling@gmail.com> wrote:
>>> On 08-11-2016 11:06, Emil Velikov wrote:
>>>> On 1 November 2016 at 18:47, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>> On 01-11-2016 18:13, Emil Velikov wrote:
>>>>>> From: Emil Velikov <emil.velikov@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 we can
>>>>>> 'cheat' temporarily.
>>>>>>
>>>>>> That should be fine, since no open-source project has ever used the
>>>>>> value.
>>>>>>
>>>>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>>>>> Cc: Mauro Santos <registo.mailling@gmail.com>
>>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>>>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>>>>> ---
>>>>>> Mauro can you apply this against libdrm and rebuild it. You do _not_
>>>>>> need to rebuild mesa afterwords.
>>>>>>
>>>>>> Thanks
>>>>>> ---
>>>>>>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>>>>>>  1 file changed, 35 insertions(+), 15 deletions(-)
>>>>>>
>>>>>> diff --git a/xf86drm.c b/xf86drm.c
>>>>>> index 52add5e..5a5100c 100644
>>>>>> --- a/xf86drm.c
>>>>>> +++ b/xf86drm.c
>>>>>> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>>>>>>                                   drmPciDeviceInfoPtr device)
>>>>>>  {
>>>>>>  #ifdef __linux__
>>>>>> +#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 char config[64];
>>>>>> -    int fd, ret;
>>>>>> +    unsigned int data[ARRAY_SIZE(attrs)];
>>>>>> +    FILE *fp;
>>>>>> +    int ret;
>>>>>>
>>>>>> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
>>>>>> -    fd = open(path, O_RDONLY);
>>>>>> -    if (fd < 0)
>>>>>> -        return -errno;
>>>>>> +    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 = read(fd, config, sizeof(config));
>>>>>> -    close(fd);
>>>>>> -    if (ret < 0)
>>>>>> -        return -errno;
>>>>>> +        ret = fscanf(fp, "%x", &data[i]);
>>>>>> +        fclose(fp);
>>>>>> +        if (ret != 1)
>>>>>> +            return -errno;
>>>>>> +
>>>>>> +    }
>>>>>>
>>>>>> -    device->vendor_id = config[0] | (config[1] << 8);
>>>>>> -    device->device_id = config[2] | (config[3] << 8);
>>>>>> -    device->revision_id = config[8];
>>>>>> -    device->subvendor_id = config[44] | (config[45] << 8);
>>>>>> -    device->subdevice_id = config[46] | (config[47] << 8);
>>>>>> +    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;
>>>>>>  #else
>>>>>>
>>>>>
>>>>> I have applied this against libdrm 2.4.71 and I don't see any delays
>>>>> when starting firefox/chromium/thunderbird/glxgears.
>>>>>
>>>>> There is also no indication in dmesg that the dGPU is being
>>>>> reinitialized when starting the programs where I've detected the problem.
>>>>>
>>>> Thanks Mauro. Can you give this a try alongside the kernel fix [1] ?
>>>> I'd love to get the latter merged soon(ish).
>>>> Independent of the kernel side, I might need to go another way for
>>>> libdrm/mesa so I'll CC you on future patches.
>>>>
>>>> Your help is greatly appreciated !
>>>>
>>>> Thanks
>>>> Emil
>>>>
>>>> [1] http://patchwork.ozlabs.org/patch/689975/
>>>>
>>>
>>> I have applied the patch on top of kernel 4.8.6 and I'm using the libdrm
>>> with the patch you sent me previously.
>>>
>>> With this patch things still seem work fine, I don't see any of the
>>> problems I've seen before, but I don't know how to confirm that the
>>> value from sysfs is now being used by libdrm instead of defaulting to zero.
>>>
>> Grr my bad. $libdrm_builddir/tests/drmdevice should do it. You might
>> need to explicitly build it (cd tests && make drmdevice)
>>
>
> When running drmdevice as my user it still wakes up the dGPU. The
> correct device revisions are being reported by I suppose that is not
> being read from sysfs.
>
Based on the output you're spot on - doesn't seem like the revision
sysfs gets used. Most likely drmdevice is linked/using the pre-patch
(system/local?) libdrm.so ?

Note:
tests/drmdevice uses the in-tree libdrm.so while the
tests/.libs/drmdevice should use the system libdrm.so.
To check which one gets picked you can use $LD_DEBUG=libs ./drmdevice

Thanks again !
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Mauro Santos]

On 08-11-2016 15:57, Emil Velikov wrote:
> On 8 November 2016 at 15:27, Mauro Santos <registo.mailling@gmail.com> wrote:
>> On 08-11-2016 15:00, Emil Velikov wrote:
>>> On 8 November 2016 at 13:38, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>> On 08-11-2016 11:06, Emil Velikov wrote:
>>>>> On 1 November 2016 at 18:47, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>>> On 01-11-2016 18:13, Emil Velikov wrote:
>>>>>>> From: Emil Velikov <emil.velikov@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 we can
>>>>>>> 'cheat' temporarily.
>>>>>>>
>>>>>>> That should be fine, since no open-source project has ever used the
>>>>>>> value.
>>>>>>>
>>>>>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>>>>>> Cc: Mauro Santos <registo.mailling@gmail.com>
>>>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>>>>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>>>>>> ---
>>>>>>> Mauro can you apply this against libdrm and rebuild it. You do _not_
>>>>>>> need to rebuild mesa afterwords.
>>>>>>>
>>>>>>> Thanks
>>>>>>> ---
>>>>>>>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>>>>>>>  1 file changed, 35 insertions(+), 15 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xf86drm.c b/xf86drm.c
>>>>>>> index 52add5e..5a5100c 100644
>>>>>>> --- a/xf86drm.c
>>>>>>> +++ b/xf86drm.c
>>>>>>> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>>>>>>>                                   drmPciDeviceInfoPtr device)
>>>>>>>  {
>>>>>>>  #ifdef __linux__
>>>>>>> +#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 char config[64];
>>>>>>> -    int fd, ret;
>>>>>>> +    unsigned int data[ARRAY_SIZE(attrs)];
>>>>>>> +    FILE *fp;
>>>>>>> +    int ret;
>>>>>>>
>>>>>>> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
>>>>>>> -    fd = open(path, O_RDONLY);
>>>>>>> -    if (fd < 0)
>>>>>>> -        return -errno;
>>>>>>> +    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 = read(fd, config, sizeof(config));
>>>>>>> -    close(fd);
>>>>>>> -    if (ret < 0)
>>>>>>> -        return -errno;
>>>>>>> +        ret = fscanf(fp, "%x", &data[i]);
>>>>>>> +        fclose(fp);
>>>>>>> +        if (ret != 1)
>>>>>>> +            return -errno;
>>>>>>> +
>>>>>>> +    }
>>>>>>>
>>>>>>> -    device->vendor_id = config[0] | (config[1] << 8);
>>>>>>> -    device->device_id = config[2] | (config[3] << 8);
>>>>>>> -    device->revision_id = config[8];
>>>>>>> -    device->subvendor_id = config[44] | (config[45] << 8);
>>>>>>> -    device->subdevice_id = config[46] | (config[47] << 8);
>>>>>>> +    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;
>>>>>>>  #else
>>>>>>>
>>>>>>
>>>>>> I have applied this against libdrm 2.4.71 and I don't see any delays
>>>>>> when starting firefox/chromium/thunderbird/glxgears.
>>>>>>
>>>>>> There is also no indication in dmesg that the dGPU is being
>>>>>> reinitialized when starting the programs where I've detected the problem.
>>>>>>
>>>>> Thanks Mauro. Can you give this a try alongside the kernel fix [1] ?
>>>>> I'd love to get the latter merged soon(ish).
>>>>> Independent of the kernel side, I might need to go another way for
>>>>> libdrm/mesa so I'll CC you on future patches.
>>>>>
>>>>> Your help is greatly appreciated !
>>>>>
>>>>> Thanks
>>>>> Emil
>>>>>
>>>>> [1] http://patchwork.ozlabs.org/patch/689975/
>>>>>
>>>>
>>>> I have applied the patch on top of kernel 4.8.6 and I'm using the libdrm
>>>> with the patch you sent me previously.
>>>>
>>>> With this patch things still seem work fine, I don't see any of the
>>>> problems I've seen before, but I don't know how to confirm that the
>>>> value from sysfs is now being used by libdrm instead of defaulting to zero.
>>>>
>>> Grr my bad. $libdrm_builddir/tests/drmdevice should do it. You might
>>> need to explicitly build it (cd tests && make drmdevice)
>>>
>>
>> When running drmdevice as my user it still wakes up the dGPU. The
>> correct device revisions are being reported by I suppose that is not
>> being read from sysfs.
>>
> Based on the output you're spot on - doesn't seem like the revision
> sysfs gets used. Most likely drmdevice is linked/using the pre-patch
> (system/local?) libdrm.so ?
> 

I've been using the patched libdrm.so ever since you sent me the patch
for libdrm and I've recompiled libdrm today to get drmdevice so both the
system's and in-tree libdrm.so should have the patch. Arch's PKGBUILD
does have a non default --enable-udev configure parameter, could that
make any difference?

> Note:
> tests/drmdevice uses the in-tree libdrm.so while the
> tests/.libs/drmdevice should use the system libdrm.so.
> To check which one gets picked you can use $LD_DEBUG=libs ./drmdevice
> 

I've tried tests/drmdevice and tests/.libs/drmdevice and both wake up
the dGPU.

With tests/drmdevice one of the last lines says:
calling fini: /tmpfs/libdrm-2.4.71/.libs/libdrm.so.2

With tests/.libs/drmdevice I get:
calling fini: /usr/lib/libdrm.so.2 [0]

So if I'm reading this right, in the first case it should definitely be
using the patched in-tree libdrm.so while in the second case it should
use the system's libdrm.so, which is already patched and fixed the
startup delay with firefox/thunderbird/chromium/glxgears and should use
sysfs.


> Thanks again !
> Emil
> 


-- 
Mauro Santos
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Emil Velikov]

On 8 November 2016 at 16:57, Mauro Santos <registo.mailling@gmail.com> wrote:
> On 08-11-2016 15:57, Emil Velikov wrote:
>> On 8 November 2016 at 15:27, Mauro Santos <registo.mailling@gmail.com> wrote:
>>> On 08-11-2016 15:00, Emil Velikov wrote:
>>>> On 8 November 2016 at 13:38, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>> On 08-11-2016 11:06, Emil Velikov wrote:
>>>>>> On 1 November 2016 at 18:47, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>>>> On 01-11-2016 18:13, Emil Velikov wrote:
>>>>>>>> From: Emil Velikov <emil.velikov@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 we can
>>>>>>>> 'cheat' temporarily.
>>>>>>>>
>>>>>>>> That should be fine, since no open-source project has ever used the
>>>>>>>> value.
>>>>>>>>
>>>>>>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>>>>>>> Cc: Mauro Santos <registo.mailling@gmail.com>
>>>>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>>>>>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>>>>>>> ---
>>>>>>>> Mauro can you apply this against libdrm and rebuild it. You do _not_
>>>>>>>> need to rebuild mesa afterwords.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> ---
>>>>>>>>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>>>>>>>>  1 file changed, 35 insertions(+), 15 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/xf86drm.c b/xf86drm.c
>>>>>>>> index 52add5e..5a5100c 100644
>>>>>>>> --- a/xf86drm.c
>>>>>>>> +++ b/xf86drm.c
>>>>>>>> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>>>>>>>>                                   drmPciDeviceInfoPtr device)
>>>>>>>>  {
>>>>>>>>  #ifdef __linux__
>>>>>>>> +#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 char config[64];
>>>>>>>> -    int fd, ret;
>>>>>>>> +    unsigned int data[ARRAY_SIZE(attrs)];
>>>>>>>> +    FILE *fp;
>>>>>>>> +    int ret;
>>>>>>>>
>>>>>>>> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
>>>>>>>> -    fd = open(path, O_RDONLY);
>>>>>>>> -    if (fd < 0)
>>>>>>>> -        return -errno;
>>>>>>>> +    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 = read(fd, config, sizeof(config));
>>>>>>>> -    close(fd);
>>>>>>>> -    if (ret < 0)
>>>>>>>> -        return -errno;
>>>>>>>> +        ret = fscanf(fp, "%x", &data[i]);
>>>>>>>> +        fclose(fp);
>>>>>>>> +        if (ret != 1)
>>>>>>>> +            return -errno;
>>>>>>>> +
>>>>>>>> +    }
>>>>>>>>
>>>>>>>> -    device->vendor_id = config[0] | (config[1] << 8);
>>>>>>>> -    device->device_id = config[2] | (config[3] << 8);
>>>>>>>> -    device->revision_id = config[8];
>>>>>>>> -    device->subvendor_id = config[44] | (config[45] << 8);
>>>>>>>> -    device->subdevice_id = config[46] | (config[47] << 8);
>>>>>>>> +    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;
>>>>>>>>  #else
>>>>>>>>
>>>>>>>
>>>>>>> I have applied this against libdrm 2.4.71 and I don't see any delays
>>>>>>> when starting firefox/chromium/thunderbird/glxgears.
>>>>>>>
>>>>>>> There is also no indication in dmesg that the dGPU is being
>>>>>>> reinitialized when starting the programs where I've detected the problem.
>>>>>>>
>>>>>> Thanks Mauro. Can you give this a try alongside the kernel fix [1] ?
>>>>>> I'd love to get the latter merged soon(ish).
>>>>>> Independent of the kernel side, I might need to go another way for
>>>>>> libdrm/mesa so I'll CC you on future patches.
>>>>>>
>>>>>> Your help is greatly appreciated !
>>>>>>
>>>>>> Thanks
>>>>>> Emil
>>>>>>
>>>>>> [1] http://patchwork.ozlabs.org/patch/689975/
>>>>>>
>>>>>
>>>>> I have applied the patch on top of kernel 4.8.6 and I'm using the libdrm
>>>>> with the patch you sent me previously.
>>>>>
>>>>> With this patch things still seem work fine, I don't see any of the
>>>>> problems I've seen before, but I don't know how to confirm that the
>>>>> value from sysfs is now being used by libdrm instead of defaulting to zero.
>>>>>
>>>> Grr my bad. $libdrm_builddir/tests/drmdevice should do it. You might
>>>> need to explicitly build it (cd tests && make drmdevice)
>>>>
>>>
>>> When running drmdevice as my user it still wakes up the dGPU. The
>>> correct device revisions are being reported by I suppose that is not
>>> being read from sysfs.
>>>
>> Based on the output you're spot on - doesn't seem like the revision
>> sysfs gets used. Most likely drmdevice is linked/using the pre-patch
>> (system/local?) libdrm.so ?
>>
>
> I've been using the patched libdrm.so ever since you sent me the patch
> for libdrm and I've recompiled libdrm today to get drmdevice so both the
> system's and in-tree libdrm.so should have the patch. Arch's PKGBUILD
> does have a non default --enable-udev configure parameter, could that
> make any difference?
>
The --enable-udev does not make any difference.

The rest does not make sense - the exact same functions are used by
drmdevice and mesa, yet it two different results are produced :-\
Or something very funny is happening and reading the device/vendor
file does _not_ wake the device, while the revision one does.

Can you pull out the kernel patch and check drmdevice/dmesg with
patched libdrm ?

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Mauro Santos]

On 08-11-2016 17:13, Emil Velikov wrote:
> On 8 November 2016 at 16:57, Mauro Santos <registo.mailling@gmail.com> wrote:
>> On 08-11-2016 15:57, Emil Velikov wrote:
>>> On 8 November 2016 at 15:27, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>> On 08-11-2016 15:00, Emil Velikov wrote:
>>>>> On 8 November 2016 at 13:38, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>>> On 08-11-2016 11:06, Emil Velikov wrote:
>>>>>>> On 1 November 2016 at 18:47, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>>>>> On 01-11-2016 18:13, Emil Velikov wrote:
>>>>>>>>> From: Emil Velikov <emil.velikov@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 we can
>>>>>>>>> 'cheat' temporarily.
>>>>>>>>>
>>>>>>>>> That should be fine, since no open-source project has ever used the
>>>>>>>>> value.
>>>>>>>>>
>>>>>>>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>>>>>>>> Cc: Mauro Santos <registo.mailling@gmail.com>
>>>>>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>>>>>>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>>>>>>>> ---
>>>>>>>>> Mauro can you apply this against libdrm and rebuild it. You do _not_
>>>>>>>>> need to rebuild mesa afterwords.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> ---
>>>>>>>>>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>>>>>>>>>  1 file changed, 35 insertions(+), 15 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/xf86drm.c b/xf86drm.c
>>>>>>>>> index 52add5e..5a5100c 100644
>>>>>>>>> --- a/xf86drm.c
>>>>>>>>> +++ b/xf86drm.c
>>>>>>>>> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>>>>>>>>>                                   drmPciDeviceInfoPtr device)
>>>>>>>>>  {
>>>>>>>>>  #ifdef __linux__
>>>>>>>>> +#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 char config[64];
>>>>>>>>> -    int fd, ret;
>>>>>>>>> +    unsigned int data[ARRAY_SIZE(attrs)];
>>>>>>>>> +    FILE *fp;
>>>>>>>>> +    int ret;
>>>>>>>>>
>>>>>>>>> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
>>>>>>>>> -    fd = open(path, O_RDONLY);
>>>>>>>>> -    if (fd < 0)
>>>>>>>>> -        return -errno;
>>>>>>>>> +    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 = read(fd, config, sizeof(config));
>>>>>>>>> -    close(fd);
>>>>>>>>> -    if (ret < 0)
>>>>>>>>> -        return -errno;
>>>>>>>>> +        ret = fscanf(fp, "%x", &data[i]);
>>>>>>>>> +        fclose(fp);
>>>>>>>>> +        if (ret != 1)
>>>>>>>>> +            return -errno;
>>>>>>>>> +
>>>>>>>>> +    }
>>>>>>>>>
>>>>>>>>> -    device->vendor_id = config[0] | (config[1] << 8);
>>>>>>>>> -    device->device_id = config[2] | (config[3] << 8);
>>>>>>>>> -    device->revision_id = config[8];
>>>>>>>>> -    device->subvendor_id = config[44] | (config[45] << 8);
>>>>>>>>> -    device->subdevice_id = config[46] | (config[47] << 8);
>>>>>>>>> +    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;
>>>>>>>>>  #else
>>>>>>>>>
>>>>>>>>
>>>>>>>> I have applied this against libdrm 2.4.71 and I don't see any delays
>>>>>>>> when starting firefox/chromium/thunderbird/glxgears.
>>>>>>>>
>>>>>>>> There is also no indication in dmesg that the dGPU is being
>>>>>>>> reinitialized when starting the programs where I've detected the problem.
>>>>>>>>
>>>>>>> Thanks Mauro. Can you give this a try alongside the kernel fix [1] ?
>>>>>>> I'd love to get the latter merged soon(ish).
>>>>>>> Independent of the kernel side, I might need to go another way for
>>>>>>> libdrm/mesa so I'll CC you on future patches.
>>>>>>>
>>>>>>> Your help is greatly appreciated !
>>>>>>>
>>>>>>> Thanks
>>>>>>> Emil
>>>>>>>
>>>>>>> [1] http://patchwork.ozlabs.org/patch/689975/
>>>>>>>
>>>>>>
>>>>>> I have applied the patch on top of kernel 4.8.6 and I'm using the libdrm
>>>>>> with the patch you sent me previously.
>>>>>>
>>>>>> With this patch things still seem work fine, I don't see any of the
>>>>>> problems I've seen before, but I don't know how to confirm that the
>>>>>> value from sysfs is now being used by libdrm instead of defaulting to zero.
>>>>>>
>>>>> Grr my bad. $libdrm_builddir/tests/drmdevice should do it. You might
>>>>> need to explicitly build it (cd tests && make drmdevice)
>>>>>
>>>>
>>>> When running drmdevice as my user it still wakes up the dGPU. The
>>>> correct device revisions are being reported by I suppose that is not
>>>> being read from sysfs.
>>>>
>>> Based on the output you're spot on - doesn't seem like the revision
>>> sysfs gets used. Most likely drmdevice is linked/using the pre-patch
>>> (system/local?) libdrm.so ?
>>>
>>
>> I've been using the patched libdrm.so ever since you sent me the patch
>> for libdrm and I've recompiled libdrm today to get drmdevice so both the
>> system's and in-tree libdrm.so should have the patch. Arch's PKGBUILD
>> does have a non default --enable-udev configure parameter, could that
>> make any difference?
>>
> The --enable-udev does not make any difference.
> 
> The rest does not make sense - the exact same functions are used by
> drmdevice and mesa, yet it two different results are produced :-\
> Or something very funny is happening and reading the device/vendor
> file does _not_ wake the device, while the revision one does.
> 

If I do 'cat revision' for the dGPU it does not wake up the device so I
guess we can scratch that.

> Can you pull out the kernel patch and check drmdevice/dmesg with
> patched libdrm ?
> 

Same behavior as before, both versions of drmdevice wake up the dGPU.
Could it be that some other lib called by drmdevice and not called by
other programs is doing something to wake up the dGPU?

> Thanks
> Emil
> 


-- 
Mauro Santos
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Mauro Santos]

On 08-11-2016 18:08, Mauro Santos wrote:
> On 08-11-2016 17:13, Emil Velikov wrote:
>> On 8 November 2016 at 16:57, Mauro Santos <registo.mailling@gmail.com> wrote:
>>> On 08-11-2016 15:57, Emil Velikov wrote:
>>>> On 8 November 2016 at 15:27, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>> On 08-11-2016 15:00, Emil Velikov wrote:
>>>>>> On 8 November 2016 at 13:38, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>>>> On 08-11-2016 11:06, Emil Velikov wrote:
>>>>>>>> On 1 November 2016 at 18:47, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>>>>>> On 01-11-2016 18:13, Emil Velikov wrote:
>>>>>>>>>> From: Emil Velikov <emil.velikov@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 we can
>>>>>>>>>> 'cheat' temporarily.
>>>>>>>>>>
>>>>>>>>>> That should be fine, since no open-source project has ever used the
>>>>>>>>>> value.
>>>>>>>>>>
>>>>>>>>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>>>>>>>>> Cc: Mauro Santos <registo.mailling@gmail.com>
>>>>>>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>>>>>>>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>>>>>>>>> ---
>>>>>>>>>> Mauro can you apply this against libdrm and rebuild it. You do _not_
>>>>>>>>>> need to rebuild mesa afterwords.
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> ---
>>>>>>>>>>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>>>>>>>>>>  1 file changed, 35 insertions(+), 15 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/xf86drm.c b/xf86drm.c
>>>>>>>>>> index 52add5e..5a5100c 100644
>>>>>>>>>> --- a/xf86drm.c
>>>>>>>>>> +++ b/xf86drm.c
>>>>>>>>>> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>>>>>>>>>>                                   drmPciDeviceInfoPtr device)
>>>>>>>>>>  {
>>>>>>>>>>  #ifdef __linux__
>>>>>>>>>> +#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 char config[64];
>>>>>>>>>> -    int fd, ret;
>>>>>>>>>> +    unsigned int data[ARRAY_SIZE(attrs)];
>>>>>>>>>> +    FILE *fp;
>>>>>>>>>> +    int ret;
>>>>>>>>>>
>>>>>>>>>> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
>>>>>>>>>> -    fd = open(path, O_RDONLY);
>>>>>>>>>> -    if (fd < 0)
>>>>>>>>>> -        return -errno;
>>>>>>>>>> +    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 = read(fd, config, sizeof(config));
>>>>>>>>>> -    close(fd);
>>>>>>>>>> -    if (ret < 0)
>>>>>>>>>> -        return -errno;
>>>>>>>>>> +        ret = fscanf(fp, "%x", &data[i]);
>>>>>>>>>> +        fclose(fp);
>>>>>>>>>> +        if (ret != 1)
>>>>>>>>>> +            return -errno;
>>>>>>>>>> +
>>>>>>>>>> +    }
>>>>>>>>>>
>>>>>>>>>> -    device->vendor_id = config[0] | (config[1] << 8);
>>>>>>>>>> -    device->device_id = config[2] | (config[3] << 8);
>>>>>>>>>> -    device->revision_id = config[8];
>>>>>>>>>> -    device->subvendor_id = config[44] | (config[45] << 8);
>>>>>>>>>> -    device->subdevice_id = config[46] | (config[47] << 8);
>>>>>>>>>> +    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;
>>>>>>>>>>  #else
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I have applied this against libdrm 2.4.71 and I don't see any delays
>>>>>>>>> when starting firefox/chromium/thunderbird/glxgears.
>>>>>>>>>
>>>>>>>>> There is also no indication in dmesg that the dGPU is being
>>>>>>>>> reinitialized when starting the programs where I've detected the problem.
>>>>>>>>>
>>>>>>>> Thanks Mauro. Can you give this a try alongside the kernel fix [1] ?
>>>>>>>> I'd love to get the latter merged soon(ish).
>>>>>>>> Independent of the kernel side, I might need to go another way for
>>>>>>>> libdrm/mesa so I'll CC you on future patches.
>>>>>>>>
>>>>>>>> Your help is greatly appreciated !
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Emil
>>>>>>>>
>>>>>>>> [1] http://patchwork.ozlabs.org/patch/689975/
>>>>>>>>
>>>>>>>
>>>>>>> I have applied the patch on top of kernel 4.8.6 and I'm using the libdrm
>>>>>>> with the patch you sent me previously.
>>>>>>>
>>>>>>> With this patch things still seem work fine, I don't see any of the
>>>>>>> problems I've seen before, but I don't know how to confirm that the
>>>>>>> value from sysfs is now being used by libdrm instead of defaulting to zero.
>>>>>>>
>>>>>> Grr my bad. $libdrm_builddir/tests/drmdevice should do it. You might
>>>>>> need to explicitly build it (cd tests && make drmdevice)
>>>>>>
>>>>>
>>>>> When running drmdevice as my user it still wakes up the dGPU. The
>>>>> correct device revisions are being reported by I suppose that is not
>>>>> being read from sysfs.
>>>>>
>>>> Based on the output you're spot on - doesn't seem like the revision
>>>> sysfs gets used. Most likely drmdevice is linked/using the pre-patch
>>>> (system/local?) libdrm.so ?
>>>>
>>>
>>> I've been using the patched libdrm.so ever since you sent me the patch
>>> for libdrm and I've recompiled libdrm today to get drmdevice so both the
>>> system's and in-tree libdrm.so should have the patch. Arch's PKGBUILD
>>> does have a non default --enable-udev configure parameter, could that
>>> make any difference?
>>>
>> The --enable-udev does not make any difference.
>>
>> The rest does not make sense - the exact same functions are used by
>> drmdevice and mesa, yet it two different results are produced :-\
>> Or something very funny is happening and reading the device/vendor
>> file does _not_ wake the device, while the revision one does.
>>
> 
> If I do 'cat revision' for the dGPU it does not wake up the device so I
> guess we can scratch that.
> 
>> Can you pull out the kernel patch and check drmdevice/dmesg with
>> patched libdrm ?
>>
> 
> Same behavior as before, both versions of drmdevice wake up the dGPU.
> Could it be that some other lib called by drmdevice and not called by
> other programs is doing something to wake up the dGPU?
> 
>> Thanks
>> Emil
>>
> 
> 

Well, I _think_ I might have found the problem and it's not with libdrm
if I'm correct.

I was now thinking and trying to check if drmdevice might be trying to
read some other information that would wake up the dGPU, and it does.

If you check the mixed output of drmdevice/dmesg I have posted(?) before
you can see that it wakes up the device when this message is shown:
Opening device 0 node /dev/dri/card1

Note the path is /dev/dri/card1. I've tried a 'cat /dev/dri/card1' and
sure enough the dGPU woke up, so I'd say libdrm and the kernel patch are
working just fine as they don't touch anything in /dev but only in /sys.

The initial output without the kernel patch says:
device[0]
	available_nodes 0007
	nodes
		nodes[0] /dev/dri/card1
		nodes[1] /dev/dri/controlD65
		nodes[2] /dev/dri/renderD129
	bustype 0000
	businfo
		pci
			domain	0000
			bus	03
			dev	00
			func	0
	deviceinfo
		pci
			vendor_id	1002
			device_id	6600
			subvendor_id	17aa
			subdevice_id	5049
			revision_id	00


If you recheck what I have posted previously after applying the kernel
patch it says revision_id 81, so I'd say it's working and we've been
following a red herring all along.

I suppose I have unintentionally misled you, I'm sorry for that.

-- 
Mauro Santos
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info

[Emil Velikov]

On 8 November 2016 at 18:36, Mauro Santos <registo.mailling@gmail.com> wrote:
> On 08-11-2016 18:08, Mauro Santos wrote:
>> On 08-11-2016 17:13, Emil Velikov wrote:
>>> On 8 November 2016 at 16:57, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>> On 08-11-2016 15:57, Emil Velikov wrote:
>>>>> On 8 November 2016 at 15:27, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>>> On 08-11-2016 15:00, Emil Velikov wrote:
>>>>>>> On 8 November 2016 at 13:38, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>>>>> On 08-11-2016 11:06, Emil Velikov wrote:
>>>>>>>>> On 1 November 2016 at 18:47, Mauro Santos <registo.mailling@gmail.com> wrote:
>>>>>>>>>> On 01-11-2016 18:13, Emil Velikov wrote:
>>>>>>>>>>> From: Emil Velikov <emil.velikov@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 we can
>>>>>>>>>>> 'cheat' temporarily.
>>>>>>>>>>>
>>>>>>>>>>> That should be fine, since no open-source project has ever used the
>>>>>>>>>>> value.
>>>>>>>>>>>
>>>>>>>>>>> Cc: Michel Dänzer <michel.daenzer@amd.com>
>>>>>>>>>>> Cc: Mauro Santos <registo.mailling@gmail.com>
>>>>>>>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>>>>>>>>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>>>>>>>>>> ---
>>>>>>>>>>> Mauro can you apply this against libdrm and rebuild it. You do _not_
>>>>>>>>>>> need to rebuild mesa afterwords.
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> ---
>>>>>>>>>>>  xf86drm.c | 50 +++++++++++++++++++++++++++++++++++---------------
>>>>>>>>>>>  1 file changed, 35 insertions(+), 15 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/xf86drm.c b/xf86drm.c
>>>>>>>>>>> index 52add5e..5a5100c 100644
>>>>>>>>>>> --- a/xf86drm.c
>>>>>>>>>>> +++ b/xf86drm.c
>>>>>>>>>>> @@ -2950,25 +2950,45 @@ static int drmParsePciDeviceInfo(const char *d_name,
>>>>>>>>>>>                                   drmPciDeviceInfoPtr device)
>>>>>>>>>>>  {
>>>>>>>>>>>  #ifdef __linux__
>>>>>>>>>>> +#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 char config[64];
>>>>>>>>>>> -    int fd, ret;
>>>>>>>>>>> +    unsigned int data[ARRAY_SIZE(attrs)];
>>>>>>>>>>> +    FILE *fp;
>>>>>>>>>>> +    int ret;
>>>>>>>>>>>
>>>>>>>>>>> -    snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config", d_name);
>>>>>>>>>>> -    fd = open(path, O_RDONLY);
>>>>>>>>>>> -    if (fd < 0)
>>>>>>>>>>> -        return -errno;
>>>>>>>>>>> +    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 = read(fd, config, sizeof(config));
>>>>>>>>>>> -    close(fd);
>>>>>>>>>>> -    if (ret < 0)
>>>>>>>>>>> -        return -errno;
>>>>>>>>>>> +        ret = fscanf(fp, "%x", &data[i]);
>>>>>>>>>>> +        fclose(fp);
>>>>>>>>>>> +        if (ret != 1)
>>>>>>>>>>> +            return -errno;
>>>>>>>>>>> +
>>>>>>>>>>> +    }
>>>>>>>>>>>
>>>>>>>>>>> -    device->vendor_id = config[0] | (config[1] << 8);
>>>>>>>>>>> -    device->device_id = config[2] | (config[3] << 8);
>>>>>>>>>>> -    device->revision_id = config[8];
>>>>>>>>>>> -    device->subvendor_id = config[44] | (config[45] << 8);
>>>>>>>>>>> -    device->subdevice_id = config[46] | (config[47] << 8);
>>>>>>>>>>> +    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;
>>>>>>>>>>>  #else
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I have applied this against libdrm 2.4.71 and I don't see any delays
>>>>>>>>>> when starting firefox/chromium/thunderbird/glxgears.
>>>>>>>>>>
>>>>>>>>>> There is also no indication in dmesg that the dGPU is being
>>>>>>>>>> reinitialized when starting the programs where I've detected the problem.
>>>>>>>>>>
>>>>>>>>> Thanks Mauro. Can you give this a try alongside the kernel fix [1] ?
>>>>>>>>> I'd love to get the latter merged soon(ish).
>>>>>>>>> Independent of the kernel side, I might need to go another way for
>>>>>>>>> libdrm/mesa so I'll CC you on future patches.
>>>>>>>>>
>>>>>>>>> Your help is greatly appreciated !
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Emil
>>>>>>>>>
>>>>>>>>> [1] http://patchwork.ozlabs.org/patch/689975/
>>>>>>>>>
>>>>>>>>
>>>>>>>> I have applied the patch on top of kernel 4.8.6 and I'm using the libdrm
>>>>>>>> with the patch you sent me previously.
>>>>>>>>
>>>>>>>> With this patch things still seem work fine, I don't see any of the
>>>>>>>> problems I've seen before, but I don't know how to confirm that the
>>>>>>>> value from sysfs is now being used by libdrm instead of defaulting to zero.
>>>>>>>>
>>>>>>> Grr my bad. $libdrm_builddir/tests/drmdevice should do it. You might
>>>>>>> need to explicitly build it (cd tests && make drmdevice)
>>>>>>>
>>>>>>
>>>>>> When running drmdevice as my user it still wakes up the dGPU. The
>>>>>> correct device revisions are being reported by I suppose that is not
>>>>>> being read from sysfs.
>>>>>>
>>>>> Based on the output you're spot on - doesn't seem like the revision
>>>>> sysfs gets used. Most likely drmdevice is linked/using the pre-patch
>>>>> (system/local?) libdrm.so ?
>>>>>
>>>>
>>>> I've been using the patched libdrm.so ever since you sent me the patch
>>>> for libdrm and I've recompiled libdrm today to get drmdevice so both the
>>>> system's and in-tree libdrm.so should have the patch. Arch's PKGBUILD
>>>> does have a non default --enable-udev configure parameter, could that
>>>> make any difference?
>>>>
>>> The --enable-udev does not make any difference.
>>>
>>> The rest does not make sense - the exact same functions are used by
>>> drmdevice and mesa, yet it two different results are produced :-\
>>> Or something very funny is happening and reading the device/vendor
>>> file does _not_ wake the device, while the revision one does.
>>>
>>
>> If I do 'cat revision' for the dGPU it does not wake up the device so I
>> guess we can scratch that.
>>
>>> Can you pull out the kernel patch and check drmdevice/dmesg with
>>> patched libdrm ?
>>>
>>
>> Same behavior as before, both versions of drmdevice wake up the dGPU.
>> Could it be that some other lib called by drmdevice and not called by
>> other programs is doing something to wake up the dGPU?
>>
>>> Thanks
>>> Emil
>>>
>>
>>
>
> Well, I _think_ I might have found the problem and it's not with libdrm
> if I'm correct.
>
> I was now thinking and trying to check if drmdevice might be trying to
> read some other information that would wake up the dGPU, and it does.
>
> If you check the mixed output of drmdevice/dmesg I have posted(?) before
> you can see that it wakes up the device when this message is shown:
> Opening device 0 node /dev/dri/card1
>
> Note the path is /dev/dri/card1. I've tried a 'cat /dev/dri/card1' and
> sure enough the dGPU woke up, so I'd say libdrm and the kernel patch are
> working just fine as they don't touch anything in /dev but only in /sys.
>
> The initial output without the kernel patch says:
> device[0]
>         available_nodes 0007
>         nodes
>                 nodes[0] /dev/dri/card1
>                 nodes[1] /dev/dri/controlD65
>                 nodes[2] /dev/dri/renderD129
>         bustype 0000
>         businfo
>                 pci
>                         domain  0000
>                         bus     03
>                         dev     00
>                         func    0
>         deviceinfo
>                 pci
>                         vendor_id       1002
>                         device_id       6600
>                         subvendor_id    17aa
>                         subdevice_id    5049
>                         revision_id     00
>
>
> If you recheck what I have posted previously after applying the kernel
> patch it says revision_id 81, so I'd say it's working and we've been
> following a red herring all along.
>
> I suppose I have unintentionally misled you, I'm sorry for that.
>
Grr how did I miss the dead obvious - open($node) _does_ wake the device.
There's nothing to apologise for and thanks for the help !

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

From: Emil Velikov <emil.velikov@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@amd.com>
Cc: Mauro Santos <registo.mailling@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
Signed-off-by: Emil Velikov <emil.velikov@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;
+}
+
 /**
  * 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
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

On 10/11/16 03:08 AM, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@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@amd.com>
> Cc: Mauro Santos <registo.mailling@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> Signed-off-by: Emil Velikov <emil.velikov@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.

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


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Nicolai Hähnle]

On 09.11.2016 19:08, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@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@amd.com>
> Cc: Mauro Santos <registo.mailling@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
> Signed-off-by: Emil Velikov <emil.velikov@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.

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

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

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 !
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@gmail.com> wrote:
>> On 09.11.2016 19:08, Emil Velikov wrote:
>>>
>>> From: Emil Velikov <emil.velikov@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@amd.com>
>>> Cc: Mauro Santos <registo.mailling@gmail.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>> Signed-off-by: Emil Velikov <emil.velikov@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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

On 14 November 2016 at 09:56, Michel Dänzer <michel@daenzer.net> wrote:
> On 10/11/16 10:38 PM, Emil Velikov wrote:
>> On 10 November 2016 at 12:40, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
>>> On 09.11.2016 19:08, Emil Velikov wrote:
>>>>
>>>> From: Emil Velikov <emil.velikov@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@amd.com>
>>>> Cc: Mauro Santos <registo.mailling@gmail.com>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>>> Signed-off-by: Emil Velikov <emil.velikov@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.
>
>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel