Re: [igt-dev] [PATCH v2 2/4] lib/igt_sysfs: Handling gt related sysfs uapi changes

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

On Mon, 26 Jun 2023 03:48:16 -0700, Upadhyay, Tejas wrote:
>
> > -----Original Message-----
> > From: Ghimiray, Himal Prasad <himal.prasad.ghimiray@intel.com>
> > Sent: Friday, June 23, 2023 5:20 PM
> > To: igt-dev@lists.freedesktop.org
> > Cc: Ghimiray, Himal Prasad <himal.prasad.ghimiray@intel.com>; Iddamsetty,
> > Aravind <aravind.iddamsetty@intel.com>; Upadhyay, Tejas
> > <tejas.upadhyay@intel.com>; Kumar, Janga Rahul
> > <janga.rahul.kumar@intel.com>; Dugast, Francois
> > <francois.dugast@intel.com>; Dixit, Ashutosh <ashutosh.dixit@intel.com>;
> > Roper, Matthew D <matthew.d.roper@intel.com>
> > Subject: [PATCH v2 2/4] lib/igt_sysfs: Handling gt related sysfs uapi changes
> >
> > 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_for_each_gt_under_each_tile macro to access new path.
> >
> > v2:
> > - Calculate number of tiles once within iterator. (Rahul)
> >
> > Cc: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
> > Cc: Upadhyay <tejas.upadhyay@intel.com>
> > Cc: Janga Rahul Kumar <janga.rahul.kumar@intel.com>
> > Cc: Francois Dugast <francois.dugast@intel.com>
> > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > ---
> >  lib/igt_sysfs.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h index de2c9a86..42bf2741 100644
> > --- a/lib/igt_sysfs.h
> > +++ b/lib/igt_sysfs.h
> > @@ -80,6 +80,12 @@
> >
> >  #define xe_for_each_tile for_each_sysfs_tile_dirfd
> >
> > +/* FIXME: Need to revisit if GT indexing under TILE changes from KMD */
> > +#define xe_for_each_gt_under_each_tile(xe__, gt__, tile__, tile_cnt__) \
> > +	for (gt__ = 0, tile__ = 0, tile_cnt__ = igt_sysfs_get_num_tiles(xe__) ; \
> > +	     gt__ < xe_number_gt(xe__); \
> > +	     (xe_number_gt(xe__) == tile_cnt__) ? ++gt__, ++tile__ : ++gt__)
> > +
>
> This is with consideration of indexing and also considering equal (or all
> GT counts are on one tile) GT counts on each tile. Consider case when
> 2GTs on 1 tile and 1 GT on other tile. But for all current platforms we
> have it should work, need to revisit when any of those scenario comes.

Yes because of this like tile info should be properly exposed from the xe
kmd query api. Or we could do the sysfs as we did on i915.

For the above how about a shorter name like "xe_for_each_tile_and_gt"?

Also I would change the order of tile__ and gt__ arguments since that is
more logical, gt's are under tiles.

Also I don't think tile_cnt__ should be in the macro args, we could just do
this:

#define xe_for_each_tile_and_gt(xe__, , tile__, gt__) \
	for (gt__ = 0, tile__ = 0; \
	     gt__ < xe_number_gt(xe__); \
	     (xe_number_gt(xe__) == igt_sysfs_get_num_tiles(xe__) ? ++gt__, ++tile__ : ++gt__)

So that is possible, but why are we comparing num_gt with num_tiles? So no
idea what's going on here. It should be num gt's per tile if they are all
the same :/

>
> Reviewed-by: Tejas Upadhyay <tejas.upadhyay@intel.com>
>
> >  enum i915_attr_id {
> >	RPS_ACT_FREQ_MHZ,
> >	RPS_CUR_FREQ_MHZ,
> > --
> > 2.25.1
>