From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id 89A6A10E0E6 for ; Sat, 1 Jul 2023 18:18:33 +0000 (UTC) Date: Sat, 01 Jul 2023 10:58:23 -0700 Message-ID: <87jzvj8qf4.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Himal Prasad Ghimiray In-Reply-To: <20230627180805.4189160-4-himal.prasad.ghimiray@intel.com> References: <20230627180805.4189160-1-himal.prasad.ghimiray@intel.com> <20230627180805.4189160-4-himal.prasad.ghimiray@intel.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [igt-dev] [PATCH v4 3/4] tests/xe/xe_guc_pc: Change the sysfs paths List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Upadhyay , igt-dev@lists.freedesktop.org, Badal Nilawar Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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