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

[Dixit, Ashutosh]

On Mon, 26 Jun 2023 03:48:16 -0700, Upadhyay, Tejas wrote:
>
> > -----Original Message-----
> > From: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>
> > Sent: Friday, June 23, 2023 5:20 PM
> > To: igt-dev at lists.freedesktop.org
> > Cc: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>; Iddamsetty,
> > Aravind <aravind.iddamsetty at intel.com>; Upadhyay, Tejas
> > <tejas.upadhyay at intel.com>; Kumar, Janga Rahul
> > <janga.rahul.kumar at intel.com>; Dugast, Francois
> > <francois.dugast at intel.com>; Dixit, Ashutosh <ashutosh.dixit at intel.com>;
> > Roper, Matthew D <matthew.d.roper at 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 at intel.com>
> > Cc: Upadhyay <tejas.upadhyay at intel.com>
> > Cc: Janga Rahul Kumar <janga.rahul.kumar at intel.com>
> > Cc: Francois Dugast <francois.dugast at intel.com>
> > Cc: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at 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 at intel.com>
>
> >  enum i915_attr_id {
> >	RPS_ACT_FREQ_MHZ,
> >	RPS_CUR_FREQ_MHZ,
> > --
> > 2.25.1
>