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

[Peter Wu]

On Wed, Nov 02, 2016 at 11:47:03AM +0000, Emil Velikov wrote:
> On 2 November 2016 at 11:14, Peter Wu <peter at 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 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>
> >
> > 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 at 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 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?

Kind regards,
Peter