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

Daniel Vetter
2019-07-15T09:31:19Z

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