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

[Zbigniew Kempczyński]

On Fri, Jul 12, 2019 at 10:20:37AM +0200, Vasilev, Oleg wrote:
> On Thu, 2019-07-11 at 12:30 +0200, Zbigniew Kempczyński wrote:
> > Change adds device selection based on implemented filters.
> > Different filters can be added to address different device
> > selection possibilities.
> > 
> > New device selection uses --device filter or IGT_DEVICE environent
> > variable. Selection of many devices can be done by using --device
> > argument multiple times. As IGT_DEVICE enviroment can be set
> > once ';' is recognized as filter separator.
> > 
> > Tool 'lsgpu' which uses device scanning feature was added.
> 
> Hi,
> 
> Would you mind splitting the patch into several commits? It is quite
> hard to review this as a whole. For example:
> 1. Device selection
> 2. lsgpu
> 3. Documentation
> 4. ...
>

Hi.

Yes, I'll split the patch and resend the patch.

> There are some comments below.
> 

<cut>

> > +static GHashTable *blacklist_keys_ht;      //sysattrs we don't want
> > to read
> > +static GHashTable *gpu_pci_class_ht;       //gpu pci classes we know
> > +static GHashTable *gpu_vendor_ht;          //search id ->
> > vendor_spec mapping
> > +static GHashTable *filter_definition_ht;   //supported filters
> > (pci=..., etc.)
> 
> Are you sure we need hashtables for all this stuff? Mappings with 5-20
> records should work with linear search well enough...

There's always design decision to use or not to use specific data structures.
Using hash tables in glib is easy and well documented. Hash table behaves
better than glibc hashtables. I decided to use them because in the code
I need to answer the question does a specific key exists or what is its
value.  

<cut>

> > +void igt_devices_print_vendors(void)
> > +{
> > +	struct name_value *list = &gpu_vendor_list[0];
> > +	printf("Recognized vendors:\n");
> > +
> > +	printf("%-16s %-32s %-32s %-16s\n", "Device type", "id",
> 
> Theese %-32s and %-16s look very bulky when you run the command in non-
> fullscreen terminal. 

Agree. I will display this list in more condensed form to not to exceed 
80-chars in a row.

<cut>

> > +static struct filter_func f_vgem = { .filter_function =
> > filter_module,
> > +				     .help = "vgem:[card=%d]",
> Can we use simpler syntax for filters with only card=%d?
> 
> Like vkms:0 instead of vkms:card=0. Or maybe set a default such that
> "vkms" would equal to "vkms:card=0".
>
At the moment I assumed we will specify filters using:
filtername:key=value,key2=value2,...

That was for simpler implementation. Syntax where you have 

filtername:value 

lead to additional check in the code I wanted to avoid. As split
filtername:... is done using ':' change filter to select first
matching card could be:
--device 'vgem:' 
Of course I need to handle null in filter value as first vgem card.
I'll add that one if this satisfy your needs.

> > +static const char *usage_str =
> > +	"usage: lsgpu [options]\n\n"
> > +	"Options:\n"
> > +	"  -r, --show-props            Show device properties\n"
> > +	"  -a, --show-attrs            Show device attributes\n"
> 
> What is the difference between those?
> Maybe merge those two options into one?

Udev distincts information about devices to properties and attributes
(sysattrs). Use 'lsgpu -d' on some card to see the difference.

> 
> > +	"  -i, --print-simple          Print devices as simple list
> > (default)\n"
> > +	"  -d, --print-details         Print devices with details\n"
> > +	"  -v, --list-vendors          List recognized vendors\n"
> > +	"  -B, --bind-modules          Bind modules to unbound PCI
> > cards\n"
> > +	"  -u, --unbind-module pci_id  Unbind module from pci id
> > device\n"
> > +	"  -b, --bind-module pci_id    Bind module to pci id device\n"
> > +	"  -l, --list-filter-types     List registered device filters
> > types\n"
> 
> Maybe add those to help?

Could you clarify?

> 
> > +	"  -s, --set-filter filter     Set filter for processing
> > devices\n"
> > +	"  -m, --match-device filter   Find device matching to
> > filter\n"
> > +	"  -D, --device filter         Device filter, can be given
> > multiple times\n"
> 
> What is the difference between theese three options?
> 
> What is the semantics of multiple --device? 

-s sets filter before displaying discovered devices. This doesn't
lead to loading module (so for example PCI card can be displayed
even if module is not loaded yet, same for discoverable devices).

-m matches card, what means returning all data (important are
/dev/dri/cardX, /dev/dri/renderDX and chipset) in a way similar
to stat() (so client has to provide ptr to allocated data structure,
to avoid additional api to free structure if it could contain
few memory allocated strings). In this case code ensures module
for card is loaded (otherwise it couldn't return device names).

-D/--device - tests multiple device selection API. Each 
--device calls stores such filter inside array. If you separate
filters with ';' it will split and add each filter to the array.

> 
> Oleg
> 

Zbigniew