On 8 November 2016 at 18:36, Mauro Santos <registo.mailling@xxxxxxxxx> 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@xxxxxxxxx> wrote: >>>> On 08-11-2016 15:57, Emil Velikov wrote: >>>>> On 8 November 2016 at 15:27, Mauro Santos <registo.mailling@xxxxxxxxx> wrote: >>>>>> On 08-11-2016 15:00, Emil Velikov wrote: >>>>>>> On 8 November 2016 at 13:38, Mauro Santos <registo.mailling@xxxxxxxxx> wrote: >>>>>>>> On 08-11-2016 11:06, Emil Velikov wrote: >>>>>>>>> On 1 November 2016 at 18:47, Mauro Santos <registo.mailling@xxxxxxxxx> wrote: >>>>>>>>>> On 01-11-2016 18:13, Emil Velikov wrote: >>>>>>>>>>> From: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> >>>>>>>>>>> >>>>>>>>>>> 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@xxxxxxx> >>>>>>>>>>> Cc: Mauro Santos <registo.mailling@xxxxxxxxx> >>>>>>>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502 >>>>>>>>>>> Signed-off-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> >>>>>>>>>>> --- >>>>>>>>>>> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
