[igt-dev] [PATCH i-g-t v6 2/4] lib/igt_sysfs: Handling gt related sysfs uapi changes

[Ghimiray, Himal Prasad]

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> Sent: 06 July 2023 05:47
> To: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>
> Cc: igt-dev at lists.freedesktop.org; Kamil Konieczny
> <kamil.konieczny at linux.intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty at intel.com>; Upadhyay, Tejas
> <tejas.upadhyay at intel.com>; Kumar, Janga Rahul
> <janga.rahul.kumar at intel.com>; Dugast, Francois
> <francois.dugast at intel.com>; Roper, Matthew D
> <matthew.d.roper at intel.com>
> Subject: Re: [PATCH i-g-t v6 2/4] lib/igt_sysfs: Handling gt related sysfs uapi
> changes
> 
> On Wed, 05 Jul 2023 06:18:51 -0700, Himal Prasad Ghimiray wrote:
> >
> > Patch https://patchwork.freedesktop.org/series/118927/
> > is moving gt sysfs parent under tile folder.
> >
> > With the above patch path for sysfs changes:
> > from: /sys/class/drm/cardX/device/gtN/ to :
> > /sys/class/drm/cardX/device/tileN/gtN
> >
> > Adding xe_gt_sysfs_path function to access new path.
> >
> > v2:
> > - Calculate number of tiles once within iterator. (Rahul)
> >
> > v3:
> > - Drop usage of local variable for tilecount.
> > - Change order of tile and gt. (Ashutosh)
> >
> > v4:
> > - Drop xe_for_each_gt_under_each_tile macro
> >   add xe_gt_sysfs_path to return path. (Ashutosh)
> > - Support older dir path along with new. (kamil)
> >
> > v5:
> > - Initialize the variable and use for loop instead of while.
> >
> > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > Cc: Aravind Iddamsetty <aravind.iddamsetty at intel.com>
> > Cc: Upadhyay <tejas.upadhyay at intel.com>
> > Cc: Janga Rahul Kumar <janga.rahul.kumar at intel.com>
> > Cc: Francois Dugast <francois.dugast at intel.com>
> > Cc: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> > ---
> >  lib/igt_sysfs.c | 40 ++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_sysfs.h |  1 +
> >  2 files changed, 41 insertions(+)
> >
> > diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c index
> > 2ea9eb1b9..32d78cfb6 100644
> > --- a/lib/igt_sysfs.c
> > +++ b/lib/igt_sysfs.c
> > @@ -46,6 +46,7 @@
> >  #include "igt_device.h"
> >  #include "igt_io.h"
> >
> > +#define XE_MAX_GT_SYSFS_PATH 3
> >  /**
> >   * SECTION:igt_sysfs
> >   * @short_description: Support code for sysfs features  @@ -933,3
> >+934,42 @@ void igt_sysfs_engines(int xe, int engines, const char
> **property,
> >		close(engine_fd);
> >	}
> >  }
> > +
> > +/**
> > + * xe_gt_sysfs_path::
> > + * @sysfs: fd of sysfs
> > + * @gt_id: gt id
> > + *
> > + * This function returns the path for gtX directory  */ char
> > +*xe_gt_sysfs_path(int sysfs, int gt_id) {
> > +	char path[XE_MAX_GT_SYSFS_PATH][32];
> > +	struct stat gtpathstat;
> > +	char *gt_path;
> > +	int i;
> > +
> > +	/* Path for multitile platforms with single gt on each tile
> > +	 * Tile id will be same as gt id.
> > +	 */
> > +	snprintf(path[0], sizeof(path[0]), "device/tile%d/gt%d/", gt_id,
> > +gt_id);
> > +
> > +	/* Path for single tile platforms with multiple gt's
> > +	 * all gt's will be under tile0 eg: MTL.
> > +	 */
> > +	snprintf(path[1], sizeof(path[1]), "device/tile0/gt%d/", gt_id);
> > +
> > +	/* Path not under tile.
> > +	 * To support older dir structure.
> > +	 */
> > +	snprintf(path[2], sizeof(path[2]), "device/gt%d/", gt_id);
> 
> Is this needed now? If not I'd say get rid of it.
@Kamil Konieczny to comment on this. He recommended to support old dir structure.
> 
> > +
> > +	for (i = 0; i < XE_MAX_GT_SYSFS_PATH; i++) {
> > +		if (!fstatat(sysfs, path[i], &gtpathstat, 0) &&
> S_ISDIR(gtpathstat.st_mode)) {
> > +			gt_path = malloc(sizeof(path[i]));
> > +			strcpy(gt_path, path[i]);
> > +			return gt_path;
> > +		}
> > +	}
> > +	igt_assert_f(false, "No gt%d dir found in sysfs", gt_id); }
> 
> I prefer this: start with tile0, look for all gt%d under under tile0 and see if gtN
> is there, repeat for next tile, till we have gone over all tiles. This will work
> always whereas the code above breaks as soon as we have a platform which
> has say: tile0/gt0, tile0/gt1, tile1/gt2.
Sure. Will address.
> 
> Since we are doing the work, with just a little extra work, do a good job of it.
> 
> Thanks.
> --
> Ashutosh
> 
> 
> 
> 
> 
> > diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h index
> > c31765542..a937ee7b5 100644
> > --- a/lib/igt_sysfs.h
> > +++ b/lib/igt_sysfs.h
> > @@ -153,6 +153,7 @@ typedef struct igt_sysfs_rw_attr {  }
> > igt_sysfs_rw_attr_t;
> >
> >  void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw);
> > +char *xe_gt_sysfs_path(int sysfs, int gt_id);
> >
> >  void igt_sysfs_engines(int xe, int engines, const char **property,
> >		       void (*test)(int, int, const char **));
> > --
> > 2.25.1
> >