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

Tue Jun 27 07:06:01 UTC 2023

Hi Ashutosh,

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> Sent: 27 June 2023 11:33
> To: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>
> Cc: Upadhyay, Tejas <tejas.upadhyay 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 21:22:33 -0700, Ghimiray, Himal Prasad wrote:
> >
> > Hi Ashutosh,
> 
> Hi Himal,
> 
> >
> > > -----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.
> 
> This is a nit but anyway what about this?
Sorry I missed it earlier. What you suggested is logical and more appropriate. 
Will change the order of arguments.
> 
> > 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.
> 
> As far as I see it, the compiler should optimize any such repetitions. We can
> even add num_tiles to 'struct xe_device' in xe_device_get() (as if it's
> obtained from the kernel), similar to xe_number_gt().
In that case, my preference would be using  (xe_number_gt(xe__) == igt_sysfs_get_num_tiles(xe__) 
Instead of populating num_tiles in struct xe_device because other members of the structure are  being intitialized 
on the basis of ioctl call. 
Please confirm what will be more appropriate.

> 
> > >
> > > 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.
> 
> So the code only works for 'num_tiles == 1' or 'num_tiles == num_gt' (1 gt
> per tile), correct? At the minimum we should probably add this assert in the
> init section of the for loop.
Correct, as of now code only caters multitile and multigt exclusively. 
Will need to enhance in case of future platform supporting both.
AFAIK adding assert/comparison in init section of for loop is not possible.
And I can't make it multiple statement macro because that will break in case of handling 
iterator in if-else condition.
> 
> The correct way to do it is of course to traverse the tile/gt directory tree
> which would be able to handle general tile/gt combinations.
> 
The issue with this approach is for multi tile the gt indexing is same as tile indexing.
Tile0/gt0 and tile1/gt1. Multiple loop with extra conditions will be overkill and confusing to handle 
the scenario. 
1) First determine number of gt's_per_tile writing the logic as finding number of tiles.
2) For(each_tile)
      for(each_gt) run loop till total numbers of gt's because indexing doesn't start with 0. [ skip all other index's apart from index same as tile]

> Anyway I'll let other people decide whether what we have here is ok or not
> to merge.
> 
> Regards,
> Ashutosh
> 
> 
> > 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
> > > >