[PATCH libdrm] xf86drm: Parse the separate files to retrieve the vendor/device info
Mauro Santos
registo.mailling at gmail.com
Tue Nov 8 18:36:20 UTC 2016
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 at gmail.com> wrote:
>>> On 08-11-2016 15:57, Emil Velikov wrote:
>>>> On 8 November 2016 at 15:27, Mauro Santos <registo.mailling at gmail.com> wrote:
>>>>> On 08-11-2016 15:00, Emil Velikov wrote:
>>>>>> On 8 November 2016 at 13:38, Mauro Santos <registo.mailling at gmail.com> wrote:
>>>>>>> On 08-11-2016 11:06, Emil Velikov wrote:
>>>>>>>> On 1 November 2016 at 18:47, Mauro Santos <registo.mailling at gmail.com> wrote:
>>>>>>>>> On 01-11-2016 18:13, Emil Velikov wrote:
>>>>>>>>>> From: Emil Velikov <emil.velikov at collabora.com>
>>>>>>>>>>
>>>>>>>>>> Parsing config sysfs file wakes up the device. The latter of which may
>>>>>>>>>> be slow and isn't required to begin with.
>>>>>>>>>>
>>>>>>>>>> Reading through config is/was required since the revision is not
>>>>>>>>>> available by other means, although with a kernel patch in the way we can
>>>>>>>>>> 'cheat' temporarily.
>>>>>>>>>>
>>>>>>>>>> That should be fine, since no open-source project has ever used the
>>>>>>>>>> value.
>>>>>>>>>>
>>>>>>>>>> Cc: Michel Dänzer <michel.daenzer at amd.com>
>>>>>>>>>> Cc: Mauro Santos <registo.mailling at gmail.com>
>>>>>>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98502
>>>>>>>>>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>>>>>>>>>> ---
>>>>>>>>>> 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
More information about the dri-devel
mailing list