[PATCH v5 1/3] lib/igt_sysfs: Add igt_sysfs_get_next_engine

[Cavitt, Jonathan]

-----Original Message-----
From: Kamil Konieczny <kamil.konieczny at linux.intel.com> 
Sent: Friday, November 15, 2024 6:37 AM
To: igt-dev at lists.freedesktop.org
Cc: Cavitt, Jonathan <jonathan.cavitt at intel.com>; Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; Belgaumkar, Vinay <vinay.belgaumkar at intel.com>
Subject: Re: [PATCH v5 1/3] lib/igt_sysfs: Add igt_sysfs_get_next_engine
> 
> Hi Jonathan,
> On 2024-11-07 at 22:52:52 +0000, Jonathan Cavitt wrote:
> > Create a new helper function, igt_sysfs_get_next_engine, that iterates
> > over sysfs/engines and stores the next engine for the user.  The
> > function returns true if the next engine in the list is available, and
> > false otherwise.  The function will be used in the test suite in a later
> > patch.
> > 
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > ---
> >  lib/igt_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++
> >  lib/igt_sysfs.h |  1 +
> >  2 files changed, 37 insertions(+)
> > 
> > diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> > index 26a3aa31fb..3d0dada969 100644
> > --- a/lib/igt_sysfs.c
> > +++ b/lib/igt_sysfs.c
> > @@ -1236,6 +1236,42 @@ static uint16_t xe_get_engine_class(char *name)
> >  	return class;
> >  }
> >  
> > +/**
> > + * igt_sysfs_get_next_engine:
> > + * @engines: fd of the directory engine
> > + * @prior: previous engine from the sysfs/engines list
> > + *
> > + * Iterates over sysfs/engines and stores the next engine in prior.
> > + * Returns true if there are more engines to iterate over, and false otherwise.
> > + */
> > +bool igt_sysfs_get_next_engine(int engines, int *prior)
> > +{
> > +	struct dirent *de;
> > +	DIR *dir;
> > +
> > +	if (!prior)
> 
> This looks buggy, did you mean:
> 	if (!*prior)

Right.  Initially, I was planning on passing NULL here, so !prior would've
been correct, but that's not how that works.

> 
> Btw I do not get how it should work, who will close dir?

According to igt_sysfs_engines, nobody closes it.

> You could either devise for example iterator:
> igt_sysfs_get_first_engine/_next_engine
> 
> or for example igt_sysfs_get_egines_names --> return list of engines
> names (as seen by 'ls engines') and then iterate over returned list?
> 
> List could be na igt_list or something simpler like concatenation of strings
> or N * strings\0 + '\0' zero string as end guard?

Given what this will be used for, maybe I can just return an array of engines opened via openat? 
I can probably make that work.

I'd probably also want some form of helper to close the list and free the array pointer.  Give me
a moment: I'll have that done in an hour or so.
-Jonathan Cavitt

> 
> > +		lseek(engines, 0, SEEK_SET);
> > +	else
> > +		close(*prior);
> > +
> > +	dir = fdopendir(engines);
> 
> This will open dir at every function call.
> 
> Regards,
> Kamil
> 
> > +	if (!dir) {
> > +		close(engines);
> > +		return false;
> > +	}
> > +
> > +	while ((de = readdir(dir))) {
> > +		if (*de->d_name == '.')
> > +			continue;
> > +
> > +		*prior = openat(engines, de->d_name, O_RDONLY);
> > +		if (*prior >= 0)
> > +			return true;
> > +	}
> > +	return false;
> > +}
> > +
> > +
> >  /**
> >   * igt_sysfs_engines:
> >   * @xe: fd of the device
> > diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> > index 852b95053f..bff5ed68e6 100644
> > --- a/lib/igt_sysfs.h
> > +++ b/lib/igt_sysfs.h
> > @@ -163,6 +163,7 @@ typedef struct igt_sysfs_rw_attr {
> >  
> >  void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw);
> >  
> > +bool igt_sysfs_get_next_engine(int engines, int *prior);
> >  void igt_sysfs_engines(int xe, int engines, int gt, bool all, const char **property,
> >  		       void (*test)(int, int, const char **, uint16_t, int));
> >  
> > -- 
> > 2.43.0
> > 
>