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

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

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