Re: libpciaccess and users depending on the revision field

[Adam Jackson]

On Wed, 2016-11-02 at 13:00 +, Emil Velikov wrote:

> 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.

You should fall back. That field definitely has real meaning for many
of the older UMS drivers (from a quick grep, at least: ast, geode,
mach64, mga, s3, sis and tseng).

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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  wrote:
> On Wed, Nov 02, 2016 at 11:47:03AM +, 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_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 
> Date:   Fri Nov 14 15:54:54 2003 +
>
> 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
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel