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

[Ghimiray, Himal Prasad]

Hi Ashutosh,

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> Sent: 27 June 2023 04:04
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>
> Cc: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>; igt-
> dev at lists.freedesktop.org; Iddamsetty, Aravind
> <aravind.iddamsetty at intel.com>; Kumar, Janga Rahul
> <janga.rahul.kumar at intel.com>; Dugast, Francois
> <francois.dugast at intel.com>; Roper, Matthew D
> <matthew.d.roper at intel.com>
> Subject: Re: [PATCH v2 2/4] lib/igt_sysfs: Handling gt related sysfs uapi
> changes
> 
> 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.
Agreed.
> 
> 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.
To me  xe_for_each_gt_under_each_tile more appropriate in comparison to 
xe_for_each_tile_and_gt. Purpose here is to iterate all gt specific sysfs entries under each tile. 
> 
> 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__)

In rev1. I had used this implementation. But problem is igt_sysfs_get_num_tiles(xe__) 
will be calculated for each iteration and want to avoid it.
> 
> 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 :/

Patch https://patchwork.freedesktop.org/series/118927/ is moving
Gt specific sysfs entries into tile. 
from: /sys/class/drm/cardX/device/gtN/sysfsX to : /sys/class/drm/cardX/device/tileN/gtN/sysfsX

Agreed KMD should we exposing the number of gt's per tile. But as of now we don't have such queries. 
But we are certain that none of the platform supports multitile and multigt per tile so above check ensures. 
If number of gt's are equal to number of tiles then sysfs should be read in above order.  If number of gt is not equal to number of tiles
Which will be case in MTL , gt0 and gt1 should be under tile0 only.
Usage of this iterator can be seen from https://patchwork.freedesktop.org/patch/543998/?series=119801&rev=1

BR
Himal 


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