Re: [igt-dev] [PATCH v2 1/4] lib/igt_sysfs: Add support to query number of tiles

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 26 Jun 2023 03:24:05 -0700, Upadhyay, Tejas wrote:
>
> > -----Original Message-----
> > From: Ghimiray, Himal Prasad <himal.prasad.ghimiray@intel.com>
> > Sent: Friday, June 23, 2023 5:20 PM
> > To: igt-dev@lists.freedesktop.org
> > Cc: Ghimiray, Himal Prasad <himal.prasad.ghimiray@intel.com>; Iddamsetty,
> > Aravind <aravind.iddamsetty@intel.com>; Upadhyay, Tejas
> > <tejas.upadhyay@intel.com>; Kumar, Janga Rahul
> > <janga.rahul.kumar@intel.com>
> > Subject: [PATCH v2 1/4] lib/igt_sysfs: Add support to query number of tiles
> >
> > With tile and GT seperation in KMD, we need to know number of tiles
> > supported by platform.
> > We will need to access tile associated properties from IGT.
> > Hence adding iterator for all supported tiles.
> >
> > v2:
> > - Calculate number of tiles once within iterator. (Rahul)
> > - Use snprintf instead of sprintf.
> >
> > Cc: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
> > Cc: Upadhyay <tejas.upadhyay@intel.com>
> > Cc: Janga Rahul Kumar <janga.rahul.kumar@intel.com>
> > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > ---
> >  lib/igt_sysfs.c | 30 ++++++++++++++++++++++++++++++  lib/igt_sysfs.h |  8
> > ++++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c index 35a4faa9..495b0288 100644
> > --- a/lib/igt_sysfs.c
> > +++ b/lib/igt_sysfs.c
> > @@ -287,6 +287,36 @@ int igt_sysfs_get_num_gt(int device)
> >	return num_gts;
> >  }
> >
> > +/**
> > + * igt_sysfs_get_num_tiles:
> > + * @device: fd of the device
> > + *
> > + * Reads number of tiles sysfs entries.
> > + * Asserts for at least one tile entry.
> > + *
> > + * Returns: Number of tiles.
> > + */
> > +int igt_sysfs_get_num_tiles(int device) {
> > +	int num_tiles;
> > +	char sysfs[48];
> > +	char tiledir[96];
> > +
> > +	num_tiles =0;
> > +	if (!igt_sysfs_path(device, sysfs, sizeof(sysfs)))
> > +		return -1;
> > +
> > +	do {
> > +		igt_assert(snprintf(tiledir, sizeof(tiledir), "%s/device/tile%d",
> > +				    sysfs, num_tiles) < sizeof(tiledir));
>
> I feel this is not best way to count tiles, as because of some reason if
> we don't create tile sysfs dir , wrong number of tiles will be detected
> and then logic to follow for_each_gt/tile. But for now as we don't have
> anything supporting in kernel right now to give tile info we can rely on
> this.
>
> Reviewed-by: Tejas Upadhyay <tejas.upadhyay@intel.com>

I don't see the point of adding temporary solutions like this. Why not add
XE_QUERY_CONFIG_TILE_COUNT under drm_xe_query_config in the kernel?

>
> > +		num_tiles++;
> > +	} while (!access(tiledir, F_OK));
> > +
> > +	num_tiles--;
> > +	igt_assert_f(num_tiles > 0, "No tiles sysfs entry is found.");
> > +	return num_tiles;
> > +}
> > +
> >  /**
> >   * igt_sysfs_write:
> >   * @dir: directory for the device from igt_sysfs_open() diff --git
> > a/lib/igt_sysfs.h b/lib/igt_sysfs.h index 978b6906..de2c9a86 100644
> > --- a/lib/igt_sysfs.h
> > +++ b/lib/igt_sysfs.h
> > @@ -38,6 +38,11 @@
> >	     (dirfd__ = igt_sysfs_gt_open(i915__, gt__)) != -1; \
> >	     close(dirfd__), gt__++)
> >
> > +#define for_each_sysfs_tile_dirfd(xe__, tile__, tile_cnt__) \
> > +	for (tile__ = 0, tile_cnt__ = igt_sysfs_get_num_tiles(xe__); \
> > +	     tile__ < tile_cnt__; \
> > +	     ++tile__)

Where is dirfd here which should be a fd to an open sysfs directory as in
for_each_sysfs_gt_dirfd?

> > +
> >  #define i915_for_each_gt for_each_sysfs_gt_dirfd
> >
> >  #define igt_sysfs_rps_write(dir, id, data, len) \ @@ -73,6 +78,8 @@  #define
> > igt_sysfs_rps_set_boolean(dir, id, value) \
> >	igt_sysfs_set_boolean(dir, igt_sysfs_dir_id_to_name(dir, id), value)
> >
> > +#define xe_for_each_tile for_each_sysfs_tile_dirfd
> > +
> >  enum i915_attr_id {
> >	RPS_ACT_FREQ_MHZ,
> >	RPS_CUR_FREQ_MHZ,
> > @@ -97,6 +104,7 @@ int igt_sysfs_open(int device);  char
> > *igt_sysfs_gt_path(int device, int gt, char *path, int pathlen);  int
> > igt_sysfs_gt_open(int device, int gt);  int igt_sysfs_get_num_gt(int device);
> > +int igt_sysfs_get_num_tiles(int device);
> >  bool igt_sysfs_has_attr(int dir, const char *attr);  const char
> > *igt_sysfs_dir_id_to_name(int dir, enum i915_attr_id id);  const char
> > *igt_sysfs_path_id_to_name(const char *path, enum i915_attr_id id);
> > --
> > 2.25.1
>