[PATCH v2] PCI: create revision file in sysfs

[Emil Velikov]

Hi Bjorn,

On 11 November 2016 at 14:49, Bjorn Helgaas <helgaas at kernel.org> wrote:
> On Fri, Nov 11, 2016 at 12:31:47AM +0000, Emil Velikov wrote:
>> On 10 November 2016 at 23:59, Bjorn Helgaas <helgaas at kernel.org> wrote:
>> > Hi Emil,
>> >
>> > On Thu, Nov 10, 2016 at 01:14:35PM +0000, Emil Velikov wrote:
>> >> On 10 November 2016 at 07:13, Greg KH <gregkh at linuxfoundation.org> wrote:
>> >> > On Wed, Nov 09, 2016 at 04:56:07PM +0000, Emil Velikov wrote:
>> >> >> From: Emil Velikov <emil.velikov at collabora.com>
>> >> >>
>> >> >> Currently the revision isn't available via sysfs/libudev thus if one
>> >> >> wants to know the value they need to read through the config file.
>> >> >>
>> >> >> This in itself wakes/powers up the device, causing unwanted delays.
>> >> >>
>> >> >> There are at least two userspace components which could make use the new
>> >> >> file - libpciaccess and libdrm. At the moment the former will wake up
>> >> >> _every_ PCI device for simple invocation of glxinfo [when using Mesa
>> >> >> 10.0+ drivers]. While the latter [in association with Mesa 13.0] can
>> >> >> lead to 2-3 second delays while starting firefox, thunderbird or
>> >> >> chromium.
>> >
>> > I agree, these unwanted delays are completely unacceptable.  My
>> > question is whether we should fix them by exporting more information
>> > from the kernel, or by changing the way the userspace components work.
>> >
>> > It should not take anywhere near 2 seconds to wake up a PCI device.
>> > That makes me think there's a more serious problem than just a lack of
>> > caching for the revision field, e.g., maybe we're looking at far more
>> > PCI devices than we need to, or we're doing it many times to the same
>> > device, or ...
>> >
>> > If I understand correctly, the delay was bisected to
>> > https://cgit.freedesktop.org/mesa/mesa/commit/?id=be239326aa4f, which
>> > removed a bunch of code that looked up the vendor and device IDs, and
>> > replaced it with drmGetDevice().  And apparently drmGetDevice(), in
>> > this path:
>> >
>> >   drmGetDevice
>> >     drmProcessPciDevice
>> >       drmParsePciDeviceInfo
>> >
>> > is a little more thorough in that it looks up the *revision* in
>> > addition to the vendor and device IDs.  So we pay the cost for the
>> > revision even though in this instance we don't care about the revision
>> > at all.
>> >
>> Above all, apologies for all the "lovely" code that you had to go
>> through for these.
>> And yes, you've got it spot on.
>>
>> > drmParsePciDeviceInfo() currently reads the whole config header from
>> > sysfs (https://cgit.freedesktop.org/drm/libdrm/tree/xf86drm.c#n2949),
>> > but I think you're extending that to try the vendor, device,
>> > subsystem_vendor, subsystem_device, and (if present) revision sysfs
>> > files first (http://www.spinics.net/lists/dri-devel/msg122319.html).
>> >
>> Yes, making the revision file optional and "faking it" was my first
>> thought, esp. since we don't have any users of it (yet).
>> Although people are not too keen on it, so we'll likely opt for
>> revision-less API.
>>
>> > Bottom line, I guess I'm not super opposed to this, but I do feel like
>> > we're making a kernel change to cover up a userspace problem, and I
>> > think it would be better to push on that userspace problem a little
>> > more.
>> >
>> Yes, definitely we can beat some sense into userspace. Yet that
>> shouldn't be a deterrent for exposing the revision.
>
> Maybe.  If we speed things up by extending this kernel ABI, there's
> much less incentive to optimize the userspace stuff.  I feel a little
> bit like an enabler for undesirable userspace behavior :)
>
Yes, fixing userspace to not do silly things is the goal. But at the
same time even if userspace is perfect, there is no reason to power on
the device just to get the revision field, is it ?
Especially since everything else is readily available.

>> As hinted before the other prominent user libpciaccess wakes up probes
>> _every_ pci device.
>
> Is it really necessary to probe *every* PCI device?  That doesn't
> sound like a scalable design.
>
> As you can tell, the argument that "we should add this kernel ABI to
> make suboptimal userspace algorithms go faster" doesn't feel very
> compelling to me.
>
"Don't shoot the messenger" comes to mind. I'm just the stupid^Wnice
person who's trying to untangle unfortunate design decisions - don't
force me to rewrite more than a dozen pieces of software, please ?
Even then, I wonder how long it'll take for those to hit end users.

Yes I see your concern - userspace does do stupid stuff. Yet it
[sometimes] must know the information and the current way of
retrieving it (waking up the device) is quite sub-optimal.

Thanks
Emil
P.S. Some drivers have custom ioctls to retrieve the device info
(incl. revision). Surely we won't want to continue promoting/assisting
that ?