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

[Mauro Santos <registo.mailling@gmail.com>]

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

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

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

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

> Thanks
> Emil
> 


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