Re: [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxxxxxx>
Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
Cc: Sujaritha Sundaresan <sujaritha.sundaresan@xxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
  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@xxxxxxxxx>

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
  }




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux