| Message ID | 20161101181334.8225-1-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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
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/
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.
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
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 >
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
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 >
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
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 >
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.
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
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