[igt-dev] [PATCH v4 3/4] tests/xe/xe_guc_pc: Change the sysfs paths

[Ghimiray, Himal Prasad]

Hi Ashutosh,

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> Sent: 01 July 2023 23:28
> To: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>
> Cc: igt-dev at lists.freedesktop.org; Upadhyay, Tejas
> <tejas.upadhyay at intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty at intel.com>; Kamil Konieczny
> <kamil.konieczny at linux.intel.com>; Nilawar, Badal
> <badal.nilawar at intel.com>; Tauro, Riana <riana.tauro at intel.com>; Gupta,
> Anshuman <anshuman.gupta at intel.com>
> Subject: Re: [PATCH v4 3/4] tests/xe/xe_guc_pc: Change the sysfs paths
> 
> On Tue, 27 Jun 2023 11:08:04 -0700, Himal Prasad Ghimiray wrote:
> >
> 
> Hi Himal,
> 
> > diff --git a/tests/xe/xe_guc_pc.c b/tests/xe/xe_guc_pc.c index
> > 89d5ae9e..2900ff09 100644
> > --- a/tests/xe/xe_guc_pc.c
> > +++ b/tests/xe/xe_guc_pc.c
> > @@ -133,23 +133,25 @@ static void exec_basic(int fd, struct
> drm_xe_engine_class_instance *eci,
> >	xe_vm_destroy(fd, vm);
> >  }
> >
> > -static int set_freq(int sysfs, int gt_id, const char *freq_name,
> > uint32_t freq)
> > +static int set_freq(int sysfs, int tile_id, int gt_id, const char
> > +*freq_name, uint32_t freq)
> >  {
> >	int ret = -EAGAIN;
> >	char path[32];
> >
> > -	sprintf(path, "device/gt%d/freq_%s", gt_id, freq_name);
> > +	igt_assert(snprintf(path, sizeof(path),
> "device/tile%d/gt%d/freq_%s",
> > +			    tile_id, gt_id, freq_name) < sizeof(path));
> >	while (ret == -EAGAIN)
> >		ret = igt_sysfs_printf(sysfs, path, "%u", freq);
> >	return ret;
> >  }
> 
> /snip/
> 
> > @@ -162,37 +164,37 @@ static uint32_t get_freq(int sysfs, int gt_id, const
> char *freq_name)
> >   * Run type: BAT
> >   */
> >
> > -static void test_freq_basic_api(int sysfs, int gt_id)
> > +static void test_freq_basic_api(int sysfs, int tile_id, int gt_id)
> >  {
> > -	uint32_t rpn = get_freq(sysfs, gt_id, "rpn");
> > -	uint32_t rpe = get_freq(sysfs, gt_id, "rpe");
> > -	uint32_t rp0 = get_freq(sysfs, gt_id, "rp0");
> > +	uint32_t rpn = get_freq(sysfs, tile_id, gt_id, "rpn");
> > +	uint32_t rpe = get_freq(sysfs, tile_id, gt_id, "rpe");
> > +	uint32_t rp0 = get_freq(sysfs, tile_id, gt_id, "rp0");
> 
> /snip/
> 
> > @@ -373,7 +375,7 @@ igt_main
> >  {
> >	struct drm_xe_engine_class_instance *hwe;
> >	int fd;
> > -	int gt;
> > +	int gt, tile;
> >	static int sysfs = -1;
> >	int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> >	uint32_t stash_min;
> > @@ -386,24 +388,24 @@ igt_main
> >		sysfs = igt_sysfs_open(fd);
> >		igt_assert(sysfs != -1);
> >
> > -		/* The defaults are the same. Stashing the gt0 is enough */
> > -		stash_min = get_freq(sysfs, 0, "min");
> > -		stash_max = get_freq(sysfs, 0, "max");
> > +		/* The defaults are the same. Stashing the gt0 in tile0 is
> enough */
> > +		stash_min = get_freq(sysfs, 0, 0, "min");
> > +		stash_max = get_freq(sysfs, 0, 0, "max");
> >	}
> >
> >	igt_subtest("freq_basic_api") {
> > -		xe_for_each_gt(fd, gt)
> > -			test_freq_basic_api(sysfs, gt);
> > +		xe_for_each_gt_under_each_tile(fd, tile, gt)
> > +			test_freq_basic_api(sysfs, tile, gt);
> 
> Basically I have decided I disagree with the entire approach here, so I
> disagree with Patches 2 and 3 in this series. Patch 2 is especially hacky but I
> think it's not needed. Let's do this differently. My points:
> 
> * gt id's are unique (so e.g. on MTL we have tile0/gt0 and tile0/gt1, on
>   PVC we have tile0/gt0 and tile1/gt1).
> 
> * The code in xe_guc_pc.c touches quantities (freq's etc.) which are
>   associated with gt's. It doesn't care about tiles and which gt is present
>   on which tile etc. It just cares about gt's. So it wrong to change the
>   whole code to start worrying about tiles now as this patch is doing (via
>   xe_for_each_gt_under_each_tile()).
> 
> * So instead what we need to do is very simple. Let's write a function
>   which will return the sysfs path for a gt (this function will need to be
>   tile aware so it will return things like "device/tile0/gt0",
>   "device/tile0/gt1", "device/tile1/gt1" etc.). The function prototype is
>   something like:
> 
>   char *xe_gt_sysfs_path(int gt_id);
> 
>   How it is implemented probably doesn't matter much. It could e.g. go
>   through the sysfs directory tree to find the path of a gt or it could use
>   info on how gts are situated on tiles for particular platforms (MTL vs
>   PVC as shown above e.g.). Either approach is fine, the platform approach
>   is simpler.
> 
>   So once we have this function implemented, in the above
>   set_freq()/get_freq() functions we could just use the gt path returned by
>   this function to read/write the sysfs. We don't need to make the entire
>   file tile aware. We just need to modify set_freq()/get_freq().
> 
My intention of introducing patch 2 was to ensure uniformity and applicability in 
case gt specific debugfs and other entries also move under tiles. 
If recommendation is to use something sysfs specific, will implement xe_gt_sysfs_path(int gt_id);
And update this test accordingly in next patch.

BR
Himal 
> If this doesn't work for some reason please let me know why. Please don't
> merge this series till we resolve this.
> 
> Thanks.
> --
> Ashutosh