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

Tue Jun 27 06:02:53 UTC 2023

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?

> 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().

> >
> > 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.

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.

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
> > >