libpciaccess and users depending on the revision field

[Emil Velikov]

[Adjusting CC list - moving to xorg-devel + dropping the libdrm users]

Gist:
The kernel does not expose separate file for the revision field, yet
there's a [freshly sent] kernel patch to address that.
Thus libpciaccess's reads through the config file and wakes up the
device, which we want to avoid where possible.

Question is - can we cheat [and return 0/-1] if the kernel does not
create the file or should we fall-back to reading the config.

On 2 November 2016 at 12:32, Peter Wu <peter at lekensteyn.nl> wrote:
> On Wed, Nov 02, 2016 at 11:47:03AM +0000, Emil Velikov wrote:
...
>> 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 at 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?
>
For xserver users one would need to check through the repos [1]
although I'd suspect that only drivers (older video ones) [2] make use
of GDevRec::chipRev.

>From the following local checkouts only the via driver uses it.
xf86-video-amdgpu
xf86-video-ati
xf86-video-freedreno
xf86-video-intel
xf86-video-nouveau
xf86-video-opentegra
xf86-video-via
libICE
libxcb
libXext
libX11
libXcomposite
libXv

Personally I'm leaning towards the "cheat" option, but it'll be great
to hear what others think on the topic.

Thanks
Emil

[1] https://cgit.freedesktop.org/xorg
[2] https://cgit.freedesktop.org/xorg/driver