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

[Kamil Konieczny]

Hi Ashutosh,

On 2023-07-01 at 10:58:23 -0700, Dixit, Ashutosh wrote:
> 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().
> 
> 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

+Cc Riana as she is also using gt* path.

Regards,
Kamil