[igt-dev] [PATCH i-g-t v1 1/1] Introduce new method of device selection

[Daniel Vetter]

On Mon, Jul 15, 2019 at 06:25:04AM +0000, Kempczynski, Zbigniew wrote:
> On Thu, 2019-07-11 at 14:43 +0200, Daniel Vetter wrote:
> > Instead of text, can we somehow integrate into our gtkdoc stuff? I know
> > gtkdoc isn't as flexible as the kernel by far, but random .txt files isn't
> > better either ...
> 
> Agree. I'll rewrite this to gtkdoc.
> 
> > 
> > >  lib/Makefile.sources            |    2 +
> > >  lib/drmtest.c                   |  151 +++-
> > >  lib/drmtest.h                   |    9 +
> > >  lib/igt_core.c                  |   13 +
> > >  lib/igt_device_scan.c           | 1425 +++++++++++++++++++++++++++++++
> > >  lib/igt_device_scan.h           |   72 ++
> > >  lib/meson.build                 |    1 +
> > >  tools/Makefile.sources          |    1 +
> > >  tools/lsgpu.c                   |  285 +++++++
> > >  tools/meson.build               |    1 +
> > >  11 files changed, 2073 insertions(+), 2 deletions(-)
> > >  create mode 100644 docs/multi-device-selection.txt
> > >  create mode 100644 lib/igt_device_scan.c
> > >  create mode 100644 lib/igt_device_scan.h
> > >  create mode 100644 tools/lsgpu.c
> > 
> > Wrt the design itself: I thought the last discussion on this we agreed on
> > basing this on udev filters. Not reinvinting an entire device
> > parser/filter ourselves (which this does here), because that means more
> > work on arm and anywhere else. Do we have a case of lost in communication
> > between all the various mail threads going on here?
> 
> I've used udev filters for scanning separate bus and returning all devices 
> on it. I haven't use all of its filters when I realized that it doesn't allow 
> filtering properly on some sysattrs. It resolves symbolic links only 
> for limited set of attributes what is a real problem - you can't set 
> sysattr filter because value of such attribute is NULL. 
> 
> Another thing is that udev provides properties / attributes as list instead 
> of key / value hash table. So if you use --device ... multiple times 
> you'll iterate over such lists multiple time to get appropriate 'key=value'
> instead getting it directly from hashtable. 
> 
> That's why I decided resolve each udev device to internal igt_device
> once, allowing people who want to write filter just direct access to 
> properties / sysattrs. Missing symbolic links resolving is also done
> in that step so filter author doesn't need to do this on his own 
> each time he would iterate over udev prop/sysattrs lists.
> 
> So regarding your sentence in lost communication - I didn't wanted to 
> bother anyone with problems described above. I'm not sure even if 
> I would send a patch to udev team regarding symbolic links they
> would accept it (someone maybe relies on this behavior now, so it
> could break compatibility). And we don't have single udev implementation
> (eudev) too.

The entire point of review is to create a shared understanding of the
problems involved. Dropping that stuff is pretty crucial.

Also, I'm not really following your description. Maybe there's a gap
between the udev library interface and what I can get at the command line.
But if I look at

# udevadm info -e

And for a specific device, the pile of links/higher level directories,
then I think we should be able to find everything.

Maybe another part of the misunderstanding: Imo we don't want to identify
physical devices, we want to identify drm_devices. Module reload is very
much the exception, and the trickery we have to let igt/lib load the
module if it's not there is imo a bit a hack. Aside from for vgem, where
it makes some sense at least.

So rough algorithimg I had in mind:
1. iterate all drm_devices in sysfs
2. walk the link to physical device
3. go up the hierarchy

Anywhere where we spot a name=value pair that matches what we filter, we
stop, and use that device. Plus augmented with the udevadm info -e stuff,
so you can add arbitrary additional stuff on top. Zero reinvented wheel
needed in igt.

Aside: Maybe we should ditch the module autoload stuff again, except for
vgem. Really no idea why that's needed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch