| Message ID | 20220217144158.21555-6-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce multitile support | expand |
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 <andi.shyti@linux.intel.com> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > 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 000000000000..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 <drm/drm_device.h> > +#include <linux/sysfs.h> > +#include <linux/printk.h> > + > +#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_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; > +} > + > +static u32 get_residency(struct intel_gt *gt, i915_reg_t reg) > +{ > + intel_wakeref_t wakeref; > + u64 res = 0; > + > + with_intel_runtime_pm(gt->uncore->rpm, wakeref) > + res = intel_rc6_residency_us(>->rc6, reg); > + > + return DIV_ROUND_CLOSEST_ULL(res, 1000); > +} > + > +static ssize_t rc6_enable_show(struct device *dev, > + struct device_attribute *attr, > + char *buff) > +{ > + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); > + u8 mask = 0; > + > + if (HAS_RC6(gt->i915)) > + mask |= BIT(0); > + if (HAS_RC6p(gt->i915)) > + mask |= BIT(1); > + if (HAS_RC6pp(gt->i915)) > + mask |= BIT(2); > + > + return sysfs_emit(buff, "%x\n", mask); > +} > + > +static s64 __rc6_residency_ms_show(struct intel_gt *gt) > +{ > + return get_residency(gt, GEN6_GT_GFX_RC6); > +} > + > +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); > +} > + > +static s64 __rc6p_residency_ms_show(struct intel_gt *gt) > +{ > + return get_residency(gt, GEN6_GT_GFX_RC6p); > +} > + > +static ssize_t rc6p_residency_ms_show(struct device *dev, > + struct device_attribute *attr, > + char *buff) > +{ > + s64 rc6p_residency = sysfs_gt_attribute_r_func(dev, attr, > + __rc6p_residency_ms_show); > + > + return sysfs_emit(buff, "%u\n", (u32) rc6p_residency); > +} > + > +static s64 __rc6pp_residency_ms_show(struct intel_gt *gt) > +{ > + return get_residency(gt, GEN6_GT_GFX_RC6pp); > +} > + > +static ssize_t rc6pp_residency_ms_show(struct device *dev, > + struct device_attribute *attr, > + char *buff) > +{ > + s64 rc6pp_residency = sysfs_gt_attribute_r_func(dev, attr, > + __rc6pp_residency_ms_show); > + > + return sysfs_emit(buff, "%u\n", (u32) rc6pp_residency); > +} > + > +static s64 __media_rc6_residency_ms_show(struct intel_gt *gt) > +{ > + return get_residency(gt, VLV_GT_MEDIA_RC6); > +} > + > +static ssize_t media_rc6_residency_ms_show(struct device *dev, > + struct device_attribute *attr, > + char *buff) > +{ > + s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr, > + __media_rc6_residency_ms_show); > + > + return sysfs_emit(buff, "%u\n", (u32) rc6_residency); > +} > + > +static DEVICE_ATTR_RO(rc6_enable); > +static DEVICE_ATTR_RO(rc6_residency_ms); > +static DEVICE_ATTR_RO(rc6p_residency_ms); > +static DEVICE_ATTR_RO(rc6pp_residency_ms); > +static DEVICE_ATTR_RO(media_rc6_residency_ms); > + > +static struct attribute *rc6_attrs[] = { > + &dev_attr_rc6_enable.attr, > + &dev_attr_rc6_residency_ms.attr, > + NULL > +}; > + > +static struct attribute *rc6p_attrs[] = { > + &dev_attr_rc6p_residency_ms.attr, > + &dev_attr_rc6pp_residency_ms.attr, > + NULL > +}; > + > +static struct attribute *media_rc6_attrs[] = { > + &dev_attr_media_rc6_residency_ms.attr, > + NULL > +}; > + > +static const struct attribute_group rc6_attr_group[] = { > + { .attrs = rc6_attrs, }, > + { .name = power_group_name, .attrs = rc6_attrs, }, > +}; > + > +static const struct attribute_group rc6p_attr_group[] = { > + { .attrs = rc6p_attrs, }, > + { .name = power_group_name, .attrs = rc6p_attrs, }, > +}; > + > +static const struct attribute_group media_rc6_attr_group[] = { > + { .attrs = media_rc6_attrs, }, > + { .name = power_group_name, .attrs = media_rc6_attrs, }, > +}; > + > +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]); > +} > + > +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) > +{ > + int ret; > + > + if (!HAS_RC6(gt->i915)) > + return; > + > + ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group); > + if (ret) > + drm_warn(>->i915->drm, > + "failed to create gt%u RC6 sysfs files\n", > + gt->info.id); > + > + /* > + * cannot use the is_visible() attribute because > + * the upper object inherits from the parent group. > + */ > + if (HAS_RC6p(gt->i915)) { > + ret = __intel_gt_sysfs_create_group(kobj, rc6p_attr_group); > + if (ret) > + drm_warn(>->i915->drm, > + "failed to create gt%u RC6p sysfs files\n", > + gt->info.id); > + } > + > + if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) { > + ret = __intel_gt_sysfs_create_group(kobj, media_rc6_attr_group); > + if (ret) > + drm_warn(>->i915->drm, > + "failed to create media %u RC6 sysfs files\n", > + gt->info.id); > + } > +} > +#else > +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) > +{ > +} > +#endif /* CONFIG_PM */ > + > +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) > +{ > + intel_sysfs_rc6_init(gt, kobj); > +} > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h > new file mode 100644 > index 000000000000..f567105a4a89 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h > @@ -0,0 +1,15 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright © 2022 Intel Corporation > + */ > + > +#ifndef __SYSFS_GT_PM_H__ > +#define __SYSFS_GT_PM_H__ > + > +#include <linux/kobject.h> > + > +#include "intel_gt_types.h" > + > +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj); > + > +#endif /* SYSFS_RC6_H */ > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > index 3643efde52ca..b08a959e5ccc 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -45,107 +45,6 @@ struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) > return to_i915(minor->dev); > } > > -#ifdef CONFIG_PM > -static u32 calc_residency(struct drm_i915_private *dev_priv, > - i915_reg_t reg) > -{ > - intel_wakeref_t wakeref; > - u64 res = 0; > - > - with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref) > - res = intel_rc6_residency_us(&to_gt(dev_priv)->rc6, reg); > - > - return DIV_ROUND_CLOSEST_ULL(res, 1000); > -} > - > -static ssize_t rc6_enable_show(struct device *kdev, > - struct device_attribute *attr, char *buf) > -{ > - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > - unsigned int mask; > - > - mask = 0; > - if (HAS_RC6(dev_priv)) > - mask |= BIT(0); > - if (HAS_RC6p(dev_priv)) > - mask |= BIT(1); > - if (HAS_RC6pp(dev_priv)) > - mask |= BIT(2); > - > - return sysfs_emit(buf, "%x\n", mask); > -} > - > -static ssize_t rc6_residency_ms_show(struct device *kdev, > - struct device_attribute *attr, char *buf) > -{ > - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > - u32 rc6_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6); > - return sysfs_emit(buf, "%u\n", rc6_residency); > -} > - > -static ssize_t rc6p_residency_ms_show(struct device *kdev, > - struct device_attribute *attr, char *buf) > -{ > - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > - u32 rc6p_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6p); > - return sysfs_emit(buf, "%u\n", rc6p_residency); > -} > - > -static ssize_t rc6pp_residency_ms_show(struct device *kdev, > - struct device_attribute *attr, char *buf) > -{ > - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > - u32 rc6pp_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6pp); > - return sysfs_emit(buf, "%u\n", rc6pp_residency); > -} > - > -static ssize_t media_rc6_residency_ms_show(struct device *kdev, > - struct device_attribute *attr, char *buf) > -{ > - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > - u32 rc6_residency = calc_residency(dev_priv, VLV_GT_MEDIA_RC6); > - return sysfs_emit(buf, "%u\n", rc6_residency); > -} > - > -static DEVICE_ATTR_RO(rc6_enable); > -static DEVICE_ATTR_RO(rc6_residency_ms); > -static DEVICE_ATTR_RO(rc6p_residency_ms); > -static DEVICE_ATTR_RO(rc6pp_residency_ms); > -static DEVICE_ATTR_RO(media_rc6_residency_ms); > - > -static struct attribute *rc6_attrs[] = { > - &dev_attr_rc6_enable.attr, > - &dev_attr_rc6_residency_ms.attr, > - NULL > -}; > - > -static const struct attribute_group rc6_attr_group = { > - .name = power_group_name, > - .attrs = rc6_attrs > -}; > - > -static struct attribute *rc6p_attrs[] = { > - &dev_attr_rc6p_residency_ms.attr, > - &dev_attr_rc6pp_residency_ms.attr, > - NULL > -}; > - > -static const struct attribute_group rc6p_attr_group = { > - .name = power_group_name, > - .attrs = rc6p_attrs > -}; > - > -static struct attribute *media_rc6_attrs[] = { > - &dev_attr_media_rc6_residency_ms.attr, > - NULL > -}; > - > -static const struct attribute_group media_rc6_attr_group = { > - .name = power_group_name, > - .attrs = media_rc6_attrs > -}; > -#endif > - > static int l3_access_valid(struct drm_i915_private *i915, loff_t offset) > { > if (!HAS_L3_DPF(i915)) > @@ -497,29 +396,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) > struct device *kdev = dev_priv->drm.primary->kdev; > int ret; > > -#ifdef CONFIG_PM > - if (HAS_RC6(dev_priv)) { > - ret = sysfs_merge_group(&kdev->kobj, > - &rc6_attr_group); > - if (ret) > - drm_err(&dev_priv->drm, > - "RC6 residency sysfs setup failed\n"); > - } > - if (HAS_RC6p(dev_priv)) { > - ret = sysfs_merge_group(&kdev->kobj, > - &rc6p_attr_group); > - if (ret) > - drm_err(&dev_priv->drm, > - "RC6p residency sysfs setup failed\n"); > - } > - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > - ret = sysfs_merge_group(&kdev->kobj, > - &media_rc6_attr_group); > - if (ret) > - drm_err(&dev_priv->drm, > - "Media RC6 residency sysfs setup failed\n"); > - } > -#endif > if (HAS_L3_DPF(dev_priv)) { > ret = device_create_bin_file(kdev, &dpf_attrs); > if (ret) > @@ -565,8 +441,4 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv) > sysfs_remove_files(&kdev->kobj, gen6_attrs); > device_remove_bin_file(kdev, &dpf_attrs_1); > device_remove_bin_file(kdev, &dpf_attrs); > -#ifdef CONFIG_PM > - sysfs_unmerge_group(&kdev->kobj, &rc6_attr_group); > - sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group); > -#endif > }
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
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-lkp@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 <lkp@intel.com>
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-all@lists.01.org
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-lkp@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 <lkp@intel.com>
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-all@lists.01.org
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
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
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
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.
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
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 <andi.shyti@linux.intel.com> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > 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 000000000000..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 <drm/drm_device.h> > +#include <linux/sysfs.h> > +#include <linux/printk.h> > + > +#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_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 ? > + > +static u32 get_residency(struct intel_gt *gt, i915_reg_t reg) > +{ > + intel_wakeref_t wakeref; > + u64 res = 0; > + > + with_intel_runtime_pm(gt->uncore->rpm, wakeref) > + res = intel_rc6_residency_us(>->rc6, reg); > + > + return DIV_ROUND_CLOSEST_ULL(res, 1000); > +} > + > +static ssize_t rc6_enable_show(struct device *dev, > + struct device_attribute *attr, > + char *buff) > +{ > + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); > + u8 mask = 0; > + > + if (HAS_RC6(gt->i915)) > + mask |= BIT(0); > + if (HAS_RC6p(gt->i915)) > + mask |= BIT(1); > + if (HAS_RC6pp(gt->i915)) > + mask |= BIT(2); > + > + return sysfs_emit(buff, "%x\n", mask); > +} > + > +static s64 __rc6_residency_ms_show(struct intel_gt *gt) > +{ > + return get_residency(gt, GEN6_GT_GFX_RC6); > +} > + > +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. > +} > + > +static s64 __rc6p_residency_ms_show(struct intel_gt *gt) > +{ > + return get_residency(gt, GEN6_GT_GFX_RC6p); > +} > + > +static ssize_t rc6p_residency_ms_show(struct device *dev, > + struct device_attribute *attr, > + char *buff) > +{ > + s64 rc6p_residency = sysfs_gt_attribute_r_func(dev, attr, > + __rc6p_residency_ms_show); > + > + return sysfs_emit(buff, "%u\n", (u32) rc6p_residency); > +} > + > +static s64 __rc6pp_residency_ms_show(struct intel_gt *gt) > +{ > + return get_residency(gt, GEN6_GT_GFX_RC6pp); > +} > + > +static ssize_t rc6pp_residency_ms_show(struct device *dev, > + struct device_attribute *attr, > + char *buff) > +{ > + s64 rc6pp_residency = sysfs_gt_attribute_r_func(dev, attr, > + __rc6pp_residency_ms_show); > + > + return sysfs_emit(buff, "%u\n", (u32) rc6pp_residency); > +} > + > +static s64 __media_rc6_residency_ms_show(struct intel_gt *gt) > +{ > + return get_residency(gt, VLV_GT_MEDIA_RC6); > +} > + > +static ssize_t media_rc6_residency_ms_show(struct device *dev, > + struct device_attribute *attr, > + char *buff) > +{ > + s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr, > + __media_rc6_residency_ms_show); > + > + return sysfs_emit(buff, "%u\n", (u32) rc6_residency); > +} > + > +static DEVICE_ATTR_RO(rc6_enable); > +static DEVICE_ATTR_RO(rc6_residency_ms); > +static DEVICE_ATTR_RO(rc6p_residency_ms); > +static DEVICE_ATTR_RO(rc6pp_residency_ms); > +static DEVICE_ATTR_RO(media_rc6_residency_ms); > + > +static struct attribute *rc6_attrs[] = { > + &dev_attr_rc6_enable.attr, > + &dev_attr_rc6_residency_ms.attr, > + NULL > +}; > + > +static struct attribute *rc6p_attrs[] = { > + &dev_attr_rc6p_residency_ms.attr, > + &dev_attr_rc6pp_residency_ms.attr, > + NULL > +}; > + > +static struct attribute *media_rc6_attrs[] = { > + &dev_attr_media_rc6_residency_ms.attr, > + NULL > +}; > + > +static const struct attribute_group rc6_attr_group[] = { > + { .attrs = rc6_attrs, }, > + { .name = power_group_name, .attrs = rc6_attrs, }, > +}; > + > +static const struct attribute_group rc6p_attr_group[] = { > + { .attrs = rc6p_attrs, }, > + { .name = power_group_name, .attrs = rc6p_attrs, }, > +}; > + > +static const struct attribute_group media_rc6_attr_group[] = { > + { .attrs = media_rc6_attrs, }, > + { .name = power_group_name, .attrs = media_rc6_attrs, }, > +}; > + > +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 :) So with variable type adjustement: Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Regards Andrzej Regards Andrzej > + > +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) > +{ > + int ret; > + > + if (!HAS_RC6(gt->i915)) > + return; > + > + ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group); > + if (ret) > + drm_warn(>->i915->drm, > + "failed to create gt%u RC6 sysfs files\n", > + gt->info.id); > + > + /* > + * cannot use the is_visible() attribute because > + * the upper object inherits from the parent group. > + */ > + if (HAS_RC6p(gt->i915)) { > + ret = __intel_gt_sysfs_create_group(kobj, rc6p_attr_group); > + if (ret) > + drm_warn(>->i915->drm, > + "failed to create gt%u RC6p sysfs files\n", > + gt->info.id); > + } > + > + if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) { > + ret = __intel_gt_sysfs_create_group(kobj, media_rc6_attr_group); > + if (ret) > + drm_warn(>->i915->drm, > + "failed to create media %u RC6 sysfs files\n", > + gt->info.id); > + } > +} > +#else > +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) > +{ > +} > +#endif /* CONFIG_PM */ > + > +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) > +{ > + intel_sysfs_rc6_init(gt, kobj); > +} > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h > new file mode 100644 > index 000000000000..f567105a4a89 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h > @@ -0,0 +1,15 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright © 2022 Intel Corporation > + */ > + > +#ifndef __SYSFS_GT_PM_H__ > +#define __SYSFS_GT_PM_H__ > + > +#include <linux/kobject.h> > + > +#include "intel_gt_types.h" > + > +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj); > + > +#endif /* SYSFS_RC6_H */ > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > index 3643efde52ca..b08a959e5ccc 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -45,107 +45,6 @@ struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) > return to_i915(minor->dev); > } > > -#ifdef CONFIG_PM > -static u32 calc_residency(struct drm_i915_private *dev_priv, > - i915_reg_t reg) > -{ > - intel_wakeref_t wakeref; > - u64 res = 0; > - > - with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref) > - res = intel_rc6_residency_us(&to_gt(dev_priv)->rc6, reg); > - > - return DIV_ROUND_CLOSEST_ULL(res, 1000); > -} > - > -static ssize_t rc6_enable_show(struct device *kdev, > - struct device_attribute *attr, char *buf) > -{ > - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > - unsigned int mask; > - > - mask = 0; > - if (HAS_RC6(dev_priv)) > - mask |= BIT(0); > - if (HAS_RC6p(dev_priv)) > - mask |= BIT(1); > - if (HAS_RC6pp(dev_priv)) > - mask |= BIT(2); > - > - return sysfs_emit(buf, "%x\n", mask); > -} > - > -static ssize_t rc6_residency_ms_show(struct device *kdev, > - struct device_attribute *attr, char *buf) > -{ > - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > - u32 rc6_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6); > - return sysfs_emit(buf, "%u\n", rc6_residency); > -} > - > -static ssize_t rc6p_residency_ms_show(struct device *kdev, > - struct device_attribute *attr, char *buf) > -{ > - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > - u32 rc6p_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6p); > - return sysfs_emit(buf, "%u\n", rc6p_residency); > -} > - > -static ssize_t rc6pp_residency_ms_show(struct device *kdev, > - struct device_attribute *attr, char *buf) > -{ > - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > - u32 rc6pp_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6pp); > - return sysfs_emit(buf, "%u\n", rc6pp_residency); > -} > - > -static ssize_t media_rc6_residency_ms_show(struct device *kdev, > - struct device_attribute *attr, char *buf) > -{ > - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > - u32 rc6_residency = calc_residency(dev_priv, VLV_GT_MEDIA_RC6); > - return sysfs_emit(buf, "%u\n", rc6_residency); > -} > - > -static DEVICE_ATTR_RO(rc6_enable); > -static DEVICE_ATTR_RO(rc6_residency_ms); > -static DEVICE_ATTR_RO(rc6p_residency_ms); > -static DEVICE_ATTR_RO(rc6pp_residency_ms); > -static DEVICE_ATTR_RO(media_rc6_residency_ms); > - > -static struct attribute *rc6_attrs[] = { > - &dev_attr_rc6_enable.attr, > - &dev_attr_rc6_residency_ms.attr, > - NULL > -}; > - > -static const struct attribute_group rc6_attr_group = { > - .name = power_group_name, > - .attrs = rc6_attrs > -}; > - > -static struct attribute *rc6p_attrs[] = { > - &dev_attr_rc6p_residency_ms.attr, > - &dev_attr_rc6pp_residency_ms.attr, > - NULL > -}; > - > -static const struct attribute_group rc6p_attr_group = { > - .name = power_group_name, > - .attrs = rc6p_attrs > -}; > - > -static struct attribute *media_rc6_attrs[] = { > - &dev_attr_media_rc6_residency_ms.attr, > - NULL > -}; > - > -static const struct attribute_group media_rc6_attr_group = { > - .name = power_group_name, > - .attrs = media_rc6_attrs > -}; > -#endif > - > static int l3_access_valid(struct drm_i915_private *i915, loff_t offset) > { > if (!HAS_L3_DPF(i915)) > @@ -497,29 +396,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) > struct device *kdev = dev_priv->drm.primary->kdev; > int ret; > > -#ifdef CONFIG_PM > - if (HAS_RC6(dev_priv)) { > - ret = sysfs_merge_group(&kdev->kobj, > - &rc6_attr_group); > - if (ret) > - drm_err(&dev_priv->drm, > - "RC6 residency sysfs setup failed\n"); > - } > - if (HAS_RC6p(dev_priv)) { > - ret = sysfs_merge_group(&kdev->kobj, > - &rc6p_attr_group); > - if (ret) > - drm_err(&dev_priv->drm, > - "RC6p residency sysfs setup failed\n"); > - } > - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > - ret = sysfs_merge_group(&kdev->kobj, > - &media_rc6_attr_group); > - if (ret) > - drm_err(&dev_priv->drm, > - "Media RC6 residency sysfs setup failed\n"); > - } > -#endif > if (HAS_L3_DPF(dev_priv)) { > ret = device_create_bin_file(kdev, &dpf_attrs); > if (ret) > @@ -565,8 +441,4 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv) > sysfs_remove_files(&kdev->kobj, gen6_attrs); > device_remove_bin_file(kdev, &dpf_attrs_1); > device_remove_bin_file(kdev, &dpf_attrs); > -#ifdef CONFIG_PM > - sysfs_unmerge_group(&kdev->kobj, &rc6_attr_group); > - sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group); > -#endif > }
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 <andrzej.hajda@intel.com> Thanks!
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.
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 000000000000..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 <drm/drm_device.h> +#include <linux/sysfs.h> +#include <linux/printk.h> + +#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_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; +} + +static u32 get_residency(struct intel_gt *gt, i915_reg_t reg) +{ + intel_wakeref_t wakeref; + u64 res = 0; + + with_intel_runtime_pm(gt->uncore->rpm, wakeref) + res = intel_rc6_residency_us(>->rc6, reg); + + return DIV_ROUND_CLOSEST_ULL(res, 1000); +} + +static ssize_t rc6_enable_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + u8 mask = 0; + + if (HAS_RC6(gt->i915)) + mask |= BIT(0); + if (HAS_RC6p(gt->i915)) + mask |= BIT(1); + if (HAS_RC6pp(gt->i915)) + mask |= BIT(2); + + return sysfs_emit(buff, "%x\n", mask); +} + +static s64 __rc6_residency_ms_show(struct intel_gt *gt) +{ + return get_residency(gt, GEN6_GT_GFX_RC6); +} + +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); +} + +static s64 __rc6p_residency_ms_show(struct intel_gt *gt) +{ + return get_residency(gt, GEN6_GT_GFX_RC6p); +} + +static ssize_t rc6p_residency_ms_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + s64 rc6p_residency = sysfs_gt_attribute_r_func(dev, attr, + __rc6p_residency_ms_show); + + return sysfs_emit(buff, "%u\n", (u32) rc6p_residency); +} + +static s64 __rc6pp_residency_ms_show(struct intel_gt *gt) +{ + return get_residency(gt, GEN6_GT_GFX_RC6pp); +} + +static ssize_t rc6pp_residency_ms_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + s64 rc6pp_residency = sysfs_gt_attribute_r_func(dev, attr, + __rc6pp_residency_ms_show); + + return sysfs_emit(buff, "%u\n", (u32) rc6pp_residency); +} + +static s64 __media_rc6_residency_ms_show(struct intel_gt *gt) +{ + return get_residency(gt, VLV_GT_MEDIA_RC6); +} + +static ssize_t media_rc6_residency_ms_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr, + __media_rc6_residency_ms_show); + + return sysfs_emit(buff, "%u\n", (u32) rc6_residency); +} + +static DEVICE_ATTR_RO(rc6_enable); +static DEVICE_ATTR_RO(rc6_residency_ms); +static DEVICE_ATTR_RO(rc6p_residency_ms); +static DEVICE_ATTR_RO(rc6pp_residency_ms); +static DEVICE_ATTR_RO(media_rc6_residency_ms); + +static struct attribute *rc6_attrs[] = { + &dev_attr_rc6_enable.attr, + &dev_attr_rc6_residency_ms.attr, + NULL +}; + +static struct attribute *rc6p_attrs[] = { + &dev_attr_rc6p_residency_ms.attr, + &dev_attr_rc6pp_residency_ms.attr, + NULL +}; + +static struct attribute *media_rc6_attrs[] = { + &dev_attr_media_rc6_residency_ms.attr, + NULL +}; + +static const struct attribute_group rc6_attr_group[] = { + { .attrs = rc6_attrs, }, + { .name = power_group_name, .attrs = rc6_attrs, }, +}; + +static const struct attribute_group rc6p_attr_group[] = { + { .attrs = rc6p_attrs, }, + { .name = power_group_name, .attrs = rc6p_attrs, }, +}; + +static const struct attribute_group media_rc6_attr_group[] = { + { .attrs = media_rc6_attrs, }, + { .name = power_group_name, .attrs = media_rc6_attrs, }, +}; + +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]); +} + +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) +{ + int ret; + + if (!HAS_RC6(gt->i915)) + return; + + ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group); + if (ret) + drm_warn(>->i915->drm, + "failed to create gt%u RC6 sysfs files\n", + gt->info.id); + + /* + * cannot use the is_visible() attribute because + * the upper object inherits from the parent group. + */ + if (HAS_RC6p(gt->i915)) { + ret = __intel_gt_sysfs_create_group(kobj, rc6p_attr_group); + if (ret) + drm_warn(>->i915->drm, + "failed to create gt%u RC6p sysfs files\n", + gt->info.id); + } + + if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) { + ret = __intel_gt_sysfs_create_group(kobj, media_rc6_attr_group); + if (ret) + drm_warn(>->i915->drm, + "failed to create media %u RC6 sysfs files\n", + gt->info.id); + } +} +#else +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) +{ +} +#endif /* CONFIG_PM */ + +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) +{ + intel_sysfs_rc6_init(gt, kobj); +} diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h new file mode 100644 index 000000000000..f567105a4a89 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2022 Intel Corporation + */ + +#ifndef __SYSFS_GT_PM_H__ +#define __SYSFS_GT_PM_H__ + +#include <linux/kobject.h> + +#include "intel_gt_types.h" + +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj); + +#endif /* SYSFS_RC6_H */ diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 3643efde52ca..b08a959e5ccc 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -45,107 +45,6 @@ struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) return to_i915(minor->dev); } -#ifdef CONFIG_PM -static u32 calc_residency(struct drm_i915_private *dev_priv, - i915_reg_t reg) -{ - intel_wakeref_t wakeref; - u64 res = 0; - - with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref) - res = intel_rc6_residency_us(&to_gt(dev_priv)->rc6, reg); - - return DIV_ROUND_CLOSEST_ULL(res, 1000); -} - -static ssize_t rc6_enable_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - unsigned int mask; - - mask = 0; - if (HAS_RC6(dev_priv)) - mask |= BIT(0); - if (HAS_RC6p(dev_priv)) - mask |= BIT(1); - if (HAS_RC6pp(dev_priv)) - mask |= BIT(2); - - return sysfs_emit(buf, "%x\n", mask); -} - -static ssize_t rc6_residency_ms_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - u32 rc6_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6); - return sysfs_emit(buf, "%u\n", rc6_residency); -} - -static ssize_t rc6p_residency_ms_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - u32 rc6p_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6p); - return sysfs_emit(buf, "%u\n", rc6p_residency); -} - -static ssize_t rc6pp_residency_ms_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - u32 rc6pp_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6pp); - return sysfs_emit(buf, "%u\n", rc6pp_residency); -} - -static ssize_t media_rc6_residency_ms_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - u32 rc6_residency = calc_residency(dev_priv, VLV_GT_MEDIA_RC6); - return sysfs_emit(buf, "%u\n", rc6_residency); -} - -static DEVICE_ATTR_RO(rc6_enable); -static DEVICE_ATTR_RO(rc6_residency_ms); -static DEVICE_ATTR_RO(rc6p_residency_ms); -static DEVICE_ATTR_RO(rc6pp_residency_ms); -static DEVICE_ATTR_RO(media_rc6_residency_ms); - -static struct attribute *rc6_attrs[] = { - &dev_attr_rc6_enable.attr, - &dev_attr_rc6_residency_ms.attr, - NULL -}; - -static const struct attribute_group rc6_attr_group = { - .name = power_group_name, - .attrs = rc6_attrs -}; - -static struct attribute *rc6p_attrs[] = { - &dev_attr_rc6p_residency_ms.attr, - &dev_attr_rc6pp_residency_ms.attr, - NULL -}; - -static const struct attribute_group rc6p_attr_group = { - .name = power_group_name, - .attrs = rc6p_attrs -}; - -static struct attribute *media_rc6_attrs[] = { - &dev_attr_media_rc6_residency_ms.attr, - NULL -}; - -static const struct attribute_group media_rc6_attr_group = { - .name = power_group_name, - .attrs = media_rc6_attrs -}; -#endif - static int l3_access_valid(struct drm_i915_private *i915, loff_t offset) { if (!HAS_L3_DPF(i915)) @@ -497,29 +396,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) struct device *kdev = dev_priv->drm.primary->kdev; int ret; -#ifdef CONFIG_PM - if (HAS_RC6(dev_priv)) { - ret = sysfs_merge_group(&kdev->kobj, - &rc6_attr_group); - if (ret) - drm_err(&dev_priv->drm, - "RC6 residency sysfs setup failed\n"); - } - if (HAS_RC6p(dev_priv)) { - ret = sysfs_merge_group(&kdev->kobj, - &rc6p_attr_group); - if (ret) - drm_err(&dev_priv->drm, - "RC6p residency sysfs setup failed\n"); - } - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { - ret = sysfs_merge_group(&kdev->kobj, - &media_rc6_attr_group); - if (ret) - drm_err(&dev_priv->drm, - "Media RC6 residency sysfs setup failed\n"); - } -#endif if (HAS_L3_DPF(dev_priv)) { ret = device_create_bin_file(kdev, &dpf_attrs); if (ret) @@ -565,8 +441,4 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv) sysfs_remove_files(&kdev->kobj, gen6_attrs); device_remove_bin_file(kdev, &dpf_attrs_1); device_remove_bin_file(kdev, &dpf_attrs); -#ifdef CONFIG_PM - sysfs_unmerge_group(&kdev->kobj, &rc6_attr_group); - sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group); -#endif }
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. 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 <andi.shyti@linux.intel.com> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- 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