Re: [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 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 &gt->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(&gt->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(&gt->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(&gt->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(&gt->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 }