Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
On Tue, 22 Feb 2022 00:57:02 -0800, Andi Shyti wrote: > Old thread, new comment below at the bottom. Please take a look. Thanks. > Hi Tvrtko and Joonas, > > > > > > > Now tiles have their own sysfs interfaces under the gt/ > > > > > > directory. Because RC6 is a property that can be configured on a > > > > > > tile basis, then each tile should have its own interface > > > > > > > > > > > > The new sysfs structure will have a similar layout for the 4 tile > > > > > > case: > > > > > > > > > > > > /sys/.../card0 > > > > > >\u251c\u2500\u2500 gt > > > > > >\u2502 \u251c\u2500\u2500 gt0 > > > > > >\u2502 \u2502 \u251c\u2500\u2500 id > > > > > >\u2502 \u2502 \u251c\u2500\u2500 rc6_enable > > > > > >\u2502 \u2502 \u251c\u2500\u2500 rc6_residency_ms > > > > > >. . . > > > > > >. . . > > > > > >. . > > > > > >\u2502 \u2514\u2500\u2500 gtN > > > > > >\u2502 \u251c\u2500\u2500 id > > > > > >\u2502 \u251c\u2500\u2500 rc6_enable > > > > > >\u2502 \u251c\u2500\u2500 rc6_residency_ms > > > > > >\u2502 . > > > > > >\u2502 . > > > > > >\u2502 > > > > > >\u2514\u2500\u2500 power/-+ > > > > > > \u251c\u2500\u2500 rc6_enable|Original > > > > > > interface > > > > > > \u251c\u2500\u2500 rc6_residency_ms +-> kept as > > > > > > existing ABI; > > > > > > . |it multiplexes over > > > > > > . |the GTs > > > > > > -+ > > > > > > > > > > > > The existing interfaces have been kept in their original location > > > > > > to preserve the existing ABI. They act on all the GTs: when > > > > > > reading they provide the average value from all the GTs. > > > > > > > > > > Average feels very odd to me. I'd ask if we can get away providing an > > > > > errno > > > > > instead? Or tile zero data? > > > > > > Tile zero data is always wrong, in my opinion. If we have round-robin > > > scaling workloads like some media cases, part of the system load might > > > just disappear when it goes to tile 1. > > > > I was thinking that in conjunction with deprecated log message it wouldn't > > be wrong - I mean if the route take was to eventually retire the legacy > > files altogether. > > that's a good point... do we want to treat the legacy interfaces > as an error or do we want to make them a feature? As the > discussion is turning those interfaces are becoming a feature. > But what are we going to do with the coming interfaces? > > E.g. in the future we will have the rc6_enable/disable that can > be a command, so that we will add the "_store" interface per > tile. What are we going to do with the above interfaces? Are we > going to add a multiplexed command as well? > > > > When we have frequency readbacks without control, returning MAX() across > > > tiles would be the logical thing. The fact that parts of the hardware can > > > be clocked lower when one part is fully utilized is the "new feature". > > > > > > After that we're only really left with the rc6_residency_ms. And that is > > > the tough one. I'm inclined that MIN() across tiles would be the right > > > answer. If you are fully utilizing a single tile, you should be able to > > > see it. > > > So we have MIN, AVG or SUM, or errno, or remove the file (which is > > just a different kind of errno?) to choose from. :) > > in this case it would just be MIN and MAX. At the end we have > here only two types of interface: frequencies and residency_ms. > For the first type we would use 'max', for the second 'min'. We have the comment below from Lowren about this about showing MAX for freq. Could someone reply. Thanks. On Sun, 06 Nov 2022 08:54:04 -0800, Lawson, Lowren H wrote: Why show maximum? Wouldn’t average be more accurate to the user experience? As a user, I expect the ‘card’ frequency to be relatively accurate to the entire card. If I see 1.6GHz, but the card is behaving as if it’s running a 1.0 & 1.6GHz on the different compute tiles, I’m going to see a massive decrease in compute workload performance while at ‘maximum’ frequency.
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
Hi Andrzej,
> > Now tiles have their own sysfs interfaces under the gt/
> > directory. Because RC6 is a property that can be configured on a
> > tile basis, then each tile should have its own interface
> >
> > The new sysfs structure will have a similar layout for the 4 tile
> > case:
> >
> > /sys/.../card0
> > ├── gt
> > │ ├── gt0
> > │ │ ├── id
> > │ │ ├── rc6_enable
> > │ │ ├── rc6_residency_ms
> > . . .
> > . . .
> > . .
> > │ └── gtN
> > │ ├── id
> > │ ├── rc6_enable
> > │ ├── rc6_residency_ms
> > │ .
> > │ .
> > │
> > └── power/-+
> >├── rc6_enable|Original interface
> >├── rc6_residency_ms +-> kept as existing ABI;
> >. |it multiplexes over
> >. |the GTs
> > -+
> >
> > The existing interfaces have been kept in their original location
> > to preserve the existing ABI. They act on all the GTs: when
> > reading they provide the average value from all the GTs.
>
> If ABI should be kept forever, depreciation msg should be removed from
> intel_gt_sysfs_get_drvdata.
yes... to be removed!
> > +#ifdef CONFIG_PM
> > +static s64
> > +sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute
> > *attr,
> > + s64 (func)(struct intel_gt *gt))
> > +{
> > + struct intel_gt *gt;
> > + s64 ret = 0;
> > +
> > + if (!is_object_gt(&dev->kobj)) {
> > + int i, num_gt = 0;
> > + struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> > +
> > + for_each_gt(gt, i915, i) {
> > + ret += func(gt);
> > + num_gt++;
> > + }
> > +
> > + ret /= num_gt;
> > + } else {
> > + gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> > + ret = func(gt);
> > + }
> > +
> > + return ret;
> > +}
>
> Return value is always converted to u32 or int, maybe we could use just int
> ?
there is one case when the value to be returned is an 'int' and
that is the "int intel_gpu_freq()". That's why I supposed that
s64 was the right size. But I don't see how it can be negative so
that I will make it u32. Perhaps we need to change intel_gpu_freq
to be u32.
(Didn't want to take it too far, but I was also thinking that in
the future we might need to return error values)
> > +static ssize_t rc6_residency_ms_show(struct device *dev,
> > +struct device_attribute *attr,
> > +char *buff)
> > +{
> > + s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr,
> > + __rc6_residency_ms_show);
> > +
> > + return sysfs_emit(buff, "%u\n", (u32) rc6_residency);
>
> Here and below (where applicable) variable should be just u32, no need to
> conversion in sysfs_emit.
yep! same comment as above.
> > +static int __intel_gt_sysfs_create_group(struct kobject *kobj,
> > +const struct attribute_group *grp)
> > +{
> > + return is_object_gt(kobj) ?
> > + sysfs_create_group(kobj, &grp[0]) :
> > + sysfs_merge_group(kobj, &grp[1]);
> > +}
>
> Merging handling "gt/gt#/*" and "power/*" attributes into the same helpers
> seems unnatural to me, in many functions we have two branches based on value
> of is_object_gt, with the most hacky intel_gt_sysfs_get_drvdata.
> Splitting handling these attributes would allow to drop fragile is_object_gt
> helper and make functions more straightforward, probably at the cost of few
> lines more. On the other side I am not sure if it is worth to change
> everything to just address code purity concerns :)
I the amount of duplicated code would be too high and there have
been already complaints some times in the past (e.g. we have had
same discussion in the case of the debugfs files).
But it's true that is quite hard to find the correct balance
between duplicated code and compact code.
> So with variable type adjustement:
> Reviewed-by: Andrzej Hajda
Thanks!
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
On 17.02.2022 15:41, Andi Shyti wrote:
Now tiles have their own sysfs interfaces under the gt/
directory. Because RC6 is a property that can be configured on a
tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile
case:
/sys/.../card0
├── gt
│ ├── gt0
│ │ ├── id
│ │ ├── rc6_enable
│ │ ├── rc6_residency_ms
. . .
. . .
. .
│ └── gtN
│ ├── id
│ ├── rc6_enable
│ ├── rc6_residency_ms
│ .
│ .
│
└── power/-+
├── rc6_enable|Original interface
├── rc6_residency_ms +-> kept as existing ABI;
. |it multiplexes over
. |the GTs
-+
The existing interfaces have been kept in their original location
to preserve the existing ABI. They act on all the GTs: when
reading they provide the average value from all the GTs.
If ABI should be kept forever, depreciation msg should be removed from
intel_gt_sysfs_get_drvdata.
This patch is not really adding exposing new interfaces (new
ABI) other than adapting the existing one to more tiles. In any
case this new set of interfaces will be a basic tool for system
managers and administrators when using i915.
Signed-off-by: Andi Shyti
Signed-off-by: Lucas De Marchi
Cc: Chris Wilson
Cc: Joonas Lahtinen
Cc: Matt Roper
Cc: Sujaritha Sundaresan
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gt/intel_gt_sysfs.c| 19 ++
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 220
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h | 15 ++
drivers/gpu/drm/i915/i915_sysfs.c | 128
5 files changed, 255 insertions(+), 128 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 277064b00afd..7104558b81d5 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -106,6 +106,7 @@ gt-y += \
gt/intel_gt_pm_irq.o \
gt/intel_gt_requests.o \
gt/intel_gt_sysfs.o \
+ gt/intel_gt_sysfs_pm.o \
gt/intel_gtt.o \
gt/intel_llc.o \
gt/intel_lrc.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
index 0206e9aa4867..132b90247a41 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
@@ -13,6 +13,7 @@
#include "i915_sysfs.h"
#include "intel_gt.h"
#include "intel_gt_sysfs.h"
+#include "intel_gt_sysfs_pm.h"
#include "intel_gt_types.h"
#include "intel_rc6.h"
@@ -54,6 +55,11 @@ struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
return kobj_to_gt(kobj);
}
+static struct kobject *gt_get_parent_obj(struct intel_gt *gt)
+{
+ return >->i915->drm.primary->kdev->kobj;
+}
+
static ssize_t id_show(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -107,6 +113,17 @@ void intel_gt_sysfs_register(struct intel_gt *gt)
struct kobject *dir;
char name[80];
+ /*
+* We need to make things right with the
+* ABI compatibility. The files were originally
+* generated under the parent directory.
+*
+* We generate the files only for gt 0
+* to avoid duplicates.
+*/
+ if (gt_is_root(gt))
+ intel_gt_sysfs_pm_init(gt, gt_get_parent_obj(gt));
+
snprintf(name, sizeof(name), "gt%d", gt->info.id);
dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name);
@@ -115,4 +132,6 @@ void intel_gt_sysfs_register(struct intel_gt *gt)
"failed to initialize %s sysfs root\n", name);
return;
}
+
+ intel_gt_sysfs_pm_init(gt, dir);
}
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
new file mode 100644
index ..27d93a36894a
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include
+#include
+#include
+
+#include "i915_drv.h"
+#include "i915_sysfs.h"
+#include "intel_gt.h"
+#include "intel_gt_regs.h"
+#include "intel_gt_sysfs.h"
+#include "intel_gt_sysfs_pm.h"
+#include "intel_rc6.h"
+
+#ifdef CONFIG_PM
+static s64
+sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr,
+ s64 (func)(struct intel_gt *gt))
+{
+ struct intel_gt *gt;
+ s64 ret = 0;
+
+ if (!is_obj
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
Hi Tvrtko and Joonas, > > > > > Now tiles have their own sysfs interfaces under the gt/ > > > > > directory. Because RC6 is a property that can be configured on a > > > > > tile basis, then each tile should have its own interface > > > > > > > > > > The new sysfs structure will have a similar layout for the 4 tile > > > > > case: > > > > > > > > > > /sys/.../card0 > > > > >\u251c\u2500\u2500 gt > > > > >\u2502 \u251c\u2500\u2500 gt0 > > > > >\u2502 \u2502 \u251c\u2500\u2500 id > > > > >\u2502 \u2502 \u251c\u2500\u2500 rc6_enable > > > > >\u2502 \u2502 \u251c\u2500\u2500 rc6_residency_ms > > > > >. . . > > > > >. . . > > > > >. . > > > > >\u2502 \u2514\u2500\u2500 gtN > > > > >\u2502 \u251c\u2500\u2500 id > > > > >\u2502 \u251c\u2500\u2500 rc6_enable > > > > >\u2502 \u251c\u2500\u2500 rc6_residency_ms > > > > >\u2502 . > > > > >\u2502 . > > > > >\u2502 > > > > >\u2514\u2500\u2500 power/-+ > > > > > \u251c\u2500\u2500 rc6_enable|Original > > > > > interface > > > > > \u251c\u2500\u2500 rc6_residency_ms +-> kept as > > > > > existing ABI; > > > > > . |it multiplexes over > > > > > . |the GTs > > > > > -+ > > > > > > > > > > The existing interfaces have been kept in their original location > > > > > to preserve the existing ABI. They act on all the GTs: when > > > > > reading they provide the average value from all the GTs. > > > > > > > > Average feels very odd to me. I'd ask if we can get away providing an > > > > errno > > > > instead? Or tile zero data? > > > > Tile zero data is always wrong, in my opinion. If we have round-robin > > scaling workloads like some media cases, part of the system load might > > just disappear when it goes to tile 1. > > I was thinking that in conjunction with deprecated log message it wouldn't > be wrong - I mean if the route take was to eventually retire the legacy > files altogether. that's a good point... do we want to treat the legacy interfaces as an error or do we want to make them a feature? As the discussion is turning those interfaces are becoming a feature. But what are we going to do with the coming interfaces? E.g. in the future we will have the rc6_enable/disable that can be a command, so that we will add the "_store" interface per tile. What are we going to do with the above interfaces? Are we going to add a multiplexed command as well? > > When we have frequency readbacks without control, returning MAX() across > > tiles would be the logical thing. The fact that parts of the hardware can > > be clocked lower when one part is fully utilized is the "new feature". > > > > After that we're only really left with the rc6_residency_ms. And that is > > the tough one. I'm inclined that MIN() across tiles would be the right > > answer. If you are fully utilizing a single tile, you should be able to > > see it. > So we have MIN, AVG or SUM, or errno, or remove the file (which is just a > different kind of errno?) to choose from. :) in this case it would just be MIN and MAX. At the end we have here only two types of interface: frequencies and residency_ms. For the first type we would use 'max', for the second 'min'. But the question holds in case we want keep adding interfaces to the above directories. Andi
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
On 18/02/2022 10:46, Joonas Lahtinen wrote: Quoting Andi Shyti (2022-02-17 17:53:58) Hi Tvrtko, Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface The new sysfs structure will have a similar layout for the 4 tile case: /sys/.../card0 \u251c\u2500\u2500 gt \u2502 \u251c\u2500\u2500 gt0 \u2502 \u2502 \u251c\u2500\u2500 id \u2502 \u2502 \u251c\u2500\u2500 rc6_enable \u2502 \u2502 \u251c\u2500\u2500 rc6_residency_ms . . . . . . . . \u2502 \u2514\u2500\u2500 gtN \u2502 \u251c\u2500\u2500 id \u2502 \u251c\u2500\u2500 rc6_enable \u2502 \u251c\u2500\u2500 rc6_residency_ms \u2502 . \u2502 . \u2502 \u2514\u2500\u2500 power/-+ \u251c\u2500\u2500 rc6_enable|Original interface \u251c\u2500\u2500 rc6_residency_ms +-> kept as existing ABI; . |it multiplexes over . |the GTs -+ The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs. Average feels very odd to me. I'd ask if we can get away providing an errno instead? Or tile zero data? Tile zero data is always wrong, in my opinion. If we have round-robin scaling workloads like some media cases, part of the system load might just disappear when it goes to tile 1. I was thinking that in conjunction with deprecated log message it wouldn't be wrong - I mean if the route take was to eventually retire the legacy files altogether. Real multiplexing would be providing something when reading and when writing. The idea of average came while revieweing with Chris the write multiplexing. Indeed it makes sense to provide some common value, but I don't know how useful it can be to the user (still if the user needs any average). I think all read/write controls like min/max/boost_freq should return an error from the global interface if all the tiles don't return same value. Write will always overwrite per-tile values. That would work I think, if the option chosen was not to retire the legacy files. When we have frequency readbacks without control, returning MAX() across tiles would be the logical thing. The fact that parts of the hardware can be clocked lower when one part is fully utilized is the "new feature". After that we're only really left with the rc6_residency_ms. And that is the tough one. I'm inclined that MIN() across tiles would be the right answer. If you are fully utilizing a single tile, you should be able to see it. So we have MIN, AVG or SUM, or errno, or remove the file (which is just a different kind of errno?) to choose from. :) Regards, Tvrtko This all would be what feels natural for an user who has their setup tuned for single-tile device. And would allow simple round-robing balancing across the tiles in somewhat coherent manner.
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
Quoting Andi Shyti (2022-02-17 17:53:58) > Hi Tvrtko, > > > > Now tiles have their own sysfs interfaces under the gt/ > > > directory. Because RC6 is a property that can be configured on a > > > tile basis, then each tile should have its own interface > > > > > > The new sysfs structure will have a similar layout for the 4 tile > > > case: > > > > > > /sys/.../card0 > > > \u251c\u2500\u2500 gt > > > \u2502 \u251c\u2500\u2500 gt0 > > > \u2502 \u2502 \u251c\u2500\u2500 id > > > \u2502 \u2502 \u251c\u2500\u2500 rc6_enable > > > \u2502 \u2502 \u251c\u2500\u2500 rc6_residency_ms > > > . . . > > > . . . > > > . . > > > \u2502 \u2514\u2500\u2500 gtN > > > \u2502 \u251c\u2500\u2500 id > > > \u2502 \u251c\u2500\u2500 rc6_enable > > > \u2502 \u251c\u2500\u2500 rc6_residency_ms > > > \u2502 . > > > \u2502 . > > > \u2502 > > > \u2514\u2500\u2500 power/-+ > > >\u251c\u2500\u2500 rc6_enable|Original > > > interface > > >\u251c\u2500\u2500 rc6_residency_ms +-> kept as existing > > > ABI; > > >. |it multiplexes over > > >. |the GTs > > > -+ > > > > > > The existing interfaces have been kept in their original location > > > to preserve the existing ABI. They act on all the GTs: when > > > reading they provide the average value from all the GTs. > > > > Average feels very odd to me. I'd ask if we can get away providing an errno > > instead? Or tile zero data? Tile zero data is always wrong, in my opinion. If we have round-robin scaling workloads like some media cases, part of the system load might just disappear when it goes to tile 1. > Real multiplexing would be providing something when reading and > when writing. The idea of average came while revieweing with > Chris the write multiplexing. Indeed it makes sense to provide > some common value, but I don't know how useful it can be to the > user (still if the user needs any average). I think all read/write controls like min/max/boost_freq should return an error from the global interface if all the tiles don't return same value. Write will always overwrite per-tile values. When we have frequency readbacks without control, returning MAX() across tiles would be the logical thing. The fact that parts of the hardware can be clocked lower when one part is fully utilized is the "new feature". After that we're only really left with the rc6_residency_ms. And that is the tough one. I'm inclined that MIN() across tiles would be the right answer. If you are fully utilizing a single tile, you should be able to see it. This all would be what feels natural for an user who has their setup tuned for single-tile device. And would allow simple round-robing balancing across the tiles in somewhat coherent manner. Regards, Joonas > > Joonas, Chris... any idea? > > > Case in point, and please correct me if I am wrong, legacy rc6_enable > > returns tile zero, while residency returns average. > > As the interface is done now, the rc6_enable is just returning > whether the gpu (i.e. i915, not gt) supports RC6 or not. I think > there is a patch later. > > > Even the deprecated message gets logged with every access right? > > > > Btw is the deperecated message limited to multi-tile platforms (can't see > > that it is) and what is the plan for that? > > yes, at this point the message would need to be removed and I > forgot to do it. > > > It's a tough problem, no easy answers even after all this time. :D > > yeah! quite hard to get it conceptually right! > > Thanks Tvrtko, > Andi
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
Hi Tvrtko, > > > > Now tiles have their own sysfs interfaces under the gt/ > > > > directory. Because RC6 is a property that can be configured on a > > > > tile basis, then each tile should have its own interface > > > > > > > > The new sysfs structure will have a similar layout for the 4 tile > > > > case: > > > > > > > > /sys/.../card0 > > > >├── gt > > > >│ ├── gt0 > > > >│ │ ├── id > > > >│ │ ├── rc6_enable > > > >│ │ ├── rc6_residency_ms > > > >. . . > > > >. . . > > > >. . > > > >│ └── gtN > > > >│ ├── id > > > >│ ├── rc6_enable > > > >│ ├── rc6_residency_ms > > > >│ . > > > >│ . > > > >│ > > > >└── power/-+ > > > > ├── rc6_enable|Original interface > > > > ├── rc6_residency_ms +-> kept as existing ABI; > > > > . |it multiplexes over > > > > . |the GTs > > > > -+ > > > > > > > > The existing interfaces have been kept in their original location > > > > to preserve the existing ABI. They act on all the GTs: when > > > > reading they provide the average value from all the GTs. > > > > > > Average feels very odd to me. I'd ask if we can get away providing an > > > errno > > > instead? Or tile zero data? > > > > Real multiplexing would be providing something when reading and > > when writing. The idea of average came while revieweing with > > Chris the write multiplexing. Indeed it makes sense to provide > > some common value, but I don't know how useful it can be to the > > user (still if the user needs any average). > > > > Joonas, Chris... any idea? > > > > > Case in point, and please correct me if I am wrong, legacy rc6_enable > > > returns tile zero, while residency returns average. > > > > As the interface is done now, the rc6_enable is just returning > > whether the gpu (i.e. i915, not gt) supports RC6 or not. I think > > there is a patch later. > > > > > Even the deprecated message gets logged with every access right? > > > > > > Btw is the deperecated message limited to multi-tile platforms (can't see > > > that it is) and what is the plan for that? > > > > yes, at this point the message would need to be removed and I > > forgot to do it. > > Maybe it is correct to have it, I don't know at this point. Is the plan to > remove the warning everywhere, or only have it on multi-tile platforms, or > new platforms? And/or remove legacy files after a while on all platforms, or > just new ones? At this point I guess the warning should be removed from everywhere (i.e. only those RC6 and RPS interfaces that are duplicated/multiplexed). We shouldn't be supposed to need more usage of multiplexed interfaces in the future (maybe just rc6 enable, but I don't see it really necessary). Thanks, Andi
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
On 17/02/2022 15:53, Andi Shyti wrote: Hi Tvrtko, Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface The new sysfs structure will have a similar layout for the 4 tile case: /sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms . . . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ . │ . │ └── power/-+ ├── rc6_enable|Original interface ├── rc6_residency_ms +-> kept as existing ABI; . |it multiplexes over . |the GTs -+ The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs. Average feels very odd to me. I'd ask if we can get away providing an errno instead? Or tile zero data? Real multiplexing would be providing something when reading and when writing. The idea of average came while revieweing with Chris the write multiplexing. Indeed it makes sense to provide some common value, but I don't know how useful it can be to the user (still if the user needs any average). Joonas, Chris... any idea? Case in point, and please correct me if I am wrong, legacy rc6_enable returns tile zero, while residency returns average. As the interface is done now, the rc6_enable is just returning whether the gpu (i.e. i915, not gt) supports RC6 or not. I think there is a patch later. Even the deprecated message gets logged with every access right? Btw is the deperecated message limited to multi-tile platforms (can't see that it is) and what is the plan for that? yes, at this point the message would need to be removed and I forgot to do it. Maybe it is correct to have it, I don't know at this point. Is the plan to remove the warning everywhere, or only have it on multi-tile platforms, or new platforms? And/or remove legacy files after a while on all platforms, or just new ones? Regards, Tvrtko
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
Hi Andi, I love your patch! Yet something to improve: [auto build test ERROR on drm-tip/drm-tip] [cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next airlied/drm-next v5.17-rc4 next-20220217] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Andi-Shyti/Introduce-multitile-support/20220217-224547 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: i386-randconfig-a004 (https://download.01.org/0day-ci/archive/20220218/202202180713.xhysmqw4-...@intel.com/config) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/b358d991c154dc27fa4ef2fc99f8819f4f3e97e7 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Andi-Shyti/Introduce-multitile-support/20220217-224547 git checkout b358d991c154dc27fa4ef2fc99f8819f4f3e97e7 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): >> ld.lld: error: undefined symbol: __divdi3 >>> referenced by intel_gt_sysfs_pm.c:35 (drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:35) >>> gpu/drm/i915/gt/intel_gt_sysfs_pm.o:(rc6_residency_ms_show) in archive drivers/built-in.a >>> referenced by intel_gt_sysfs_pm.c:35 (drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:35) >>> gpu/drm/i915/gt/intel_gt_sysfs_pm.o:(rc6p_residency_ms_show) in archive drivers/built-in.a >>> referenced by intel_gt_sysfs_pm.c:35 (drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:35) >>> gpu/drm/i915/gt/intel_gt_sysfs_pm.o:(rc6pp_residency_ms_show) in archive drivers/built-in.a >>> referenced 1 more times --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
Hi Andi, I love your patch! Yet something to improve: [auto build test ERROR on drm-tip/drm-tip] [cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next airlied/drm-next v5.17-rc4 next-20220217] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Andi-Shyti/Introduce-multitile-support/20220217-224547 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: i386-defconfig (https://download.01.org/0day-ci/archive/20220218/202202180400.ppkeh3z4-...@intel.com/config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/b358d991c154dc27fa4ef2fc99f8819f4f3e97e7 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Andi-Shyti/Introduce-multitile-support/20220217-224547 git checkout b358d991c154dc27fa4ef2fc99f8819f4f3e97e7 # save the config file to linux build tree mkdir build_dir make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): ld: drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.o: in function `sysfs_gt_attribute_r_func.isra.0': >> intel_gt_sysfs_pm.c:(.text+0x1b2): undefined reference to `__divdi3' --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
Hi Tvrtko, > > Now tiles have their own sysfs interfaces under the gt/ > > directory. Because RC6 is a property that can be configured on a > > tile basis, then each tile should have its own interface > > > > The new sysfs structure will have a similar layout for the 4 tile > > case: > > > > /sys/.../card0 > > ├── gt > > │ ├── gt0 > > │ │ ├── id > > │ │ ├── rc6_enable > > │ │ ├── rc6_residency_ms > > . . . > > . . . > > . . > > │ └── gtN > > │ ├── id > > │ ├── rc6_enable > > │ ├── rc6_residency_ms > > │ . > > │ . > > │ > > └── power/-+ > >├── rc6_enable|Original interface > >├── rc6_residency_ms +-> kept as existing ABI; > >. |it multiplexes over > >. |the GTs > > -+ > > > > The existing interfaces have been kept in their original location > > to preserve the existing ABI. They act on all the GTs: when > > reading they provide the average value from all the GTs. > > Average feels very odd to me. I'd ask if we can get away providing an errno > instead? Or tile zero data? Real multiplexing would be providing something when reading and when writing. The idea of average came while revieweing with Chris the write multiplexing. Indeed it makes sense to provide some common value, but I don't know how useful it can be to the user (still if the user needs any average). Joonas, Chris... any idea? > Case in point, and please correct me if I am wrong, legacy rc6_enable > returns tile zero, while residency returns average. As the interface is done now, the rc6_enable is just returning whether the gpu (i.e. i915, not gt) supports RC6 or not. I think there is a patch later. > Even the deprecated message gets logged with every access right? > > Btw is the deperecated message limited to multi-tile platforms (can't see > that it is) and what is the plan for that? yes, at this point the message would need to be removed and I forgot to do it. > It's a tough problem, no easy answers even after all this time. :D yeah! quite hard to get it conceptually right! Thanks Tvrtko, Andi
Re: [Intel-gfx] [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
On 17/02/2022 14:41, Andi Shyti wrote:
Now tiles have their own sysfs interfaces under the gt/
directory. Because RC6 is a property that can be configured on a
tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile
case:
/sys/.../card0
├── gt
│ ├── gt0
│ │ ├── id
│ │ ├── rc6_enable
│ │ ├── rc6_residency_ms
. . .
. . .
. .
│ └── gtN
│ ├── id
│ ├── rc6_enable
│ ├── rc6_residency_ms
│ .
│ .
│
└── power/-+
├── rc6_enable|Original interface
├── rc6_residency_ms +-> kept as existing ABI;
. |it multiplexes over
. |the GTs
-+
The existing interfaces have been kept in their original location
to preserve the existing ABI. They act on all the GTs: when
reading they provide the average value from all the GTs.
Average feels very odd to me. I'd ask if we can get away providing an
errno instead? Or tile zero data?
Case in point, and please correct me if I am wrong, legacy rc6_enable
returns tile zero, while residency returns average.
Even the deprecated message gets logged with every access right?
Btw is the deperecated message limited to multi-tile platforms (can't
see that it is) and what is the plan for that?
It's a tough problem, no easy answers even after all this time. :D
Regards,
Tvrtko
This patch is not really adding exposing new interfaces (new
ABI) other than adapting the existing one to more tiles. In any
case this new set of interfaces will be a basic tool for system
managers and administrators when using i915.
Signed-off-by: Andi Shyti
Signed-off-by: Lucas De Marchi
Cc: Chris Wilson
Cc: Joonas Lahtinen
Cc: Matt Roper
Cc: Sujaritha Sundaresan
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gt/intel_gt_sysfs.c| 19 ++
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 220
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h | 15 ++
drivers/gpu/drm/i915/i915_sysfs.c | 128
5 files changed, 255 insertions(+), 128 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 277064b00afd..7104558b81d5 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -106,6 +106,7 @@ gt-y += \
gt/intel_gt_pm_irq.o \
gt/intel_gt_requests.o \
gt/intel_gt_sysfs.o \
+ gt/intel_gt_sysfs_pm.o \
gt/intel_gtt.o \
gt/intel_llc.o \
gt/intel_lrc.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
index 0206e9aa4867..132b90247a41 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
@@ -13,6 +13,7 @@
#include "i915_sysfs.h"
#include "intel_gt.h"
#include "intel_gt_sysfs.h"
+#include "intel_gt_sysfs_pm.h"
#include "intel_gt_types.h"
#include "intel_rc6.h"
@@ -54,6 +55,11 @@ struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
return kobj_to_gt(kobj);
}
+static struct kobject *gt_get_parent_obj(struct intel_gt *gt)
+{
+ return >->i915->drm.primary->kdev->kobj;
+}
+
static ssize_t id_show(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -107,6 +113,17 @@ void intel_gt_sysfs_register(struct intel_gt *gt)
struct kobject *dir;
char name[80];
+ /*
+* We need to make things right with the
+* ABI compatibility. The files were originally
+* generated under the parent directory.
+*
+* We generate the files only for gt 0
+* to avoid duplicates.
+*/
+ if (gt_is_root(gt))
+ intel_gt_sysfs_pm_init(gt, gt_get_parent_obj(gt));
+
snprintf(name, sizeof(name), "gt%d", gt->info.id);
dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name);
@@ -115,4 +132,6 @@ void intel_gt_sysfs_register(struct intel_gt *gt)
"failed to initialize %s sysfs root\n", name);
return;
}
+
+ intel_gt_sysfs_pm_init(gt, dir);
}
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
new file mode 100644
index ..27d93a36894a
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include
+#include
+#include
+
+#include "i915_drv.h"
+#
