Intel-GFX Archive on lore.kernel.org
 help / color / Atom feed
* [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
@ 2020-02-14 11:03 Andi Shyti
  2020-02-14 12:51 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/gt: make a gt sysfs group and move power management files (rev3) Patchwork
                   ` (5 more replies)
  0 siblings, 6 replies; 22+ messages in thread
From: Andi Shyti @ 2020-02-14 11:03 UTC (permalink / raw)
  To: Intel GFX

The GT has its own properties and in sysfs they should be grouped
in the 'gt/' directory.

Create the 'gt/' directory in sysfs and move the power management
related files.

Signed-off-by: Andi Shyti <andi.shyti@intel.com>
---
Hi,

this version has some more substantial differences, nothing that
changes the wanted behavior, though.

Thanks Chris, Jani and Tvrtko for the reviews,
Andi

Changelog:
==========

v2 -> v3:
 - fix some cleanups that I forgot in the previous patch
 - fix reference pointers to the gt structure
 - and many other small changes here and there.
v1 -> v2:
 - keep the existing files as they are
 - use "intel_gt_*" as prefix instead of "sysfs_*"

 drivers/gpu/drm/i915/Makefile            |   4 +-
 drivers/gpu/drm/i915/gt/intel_gt.c       |   3 +
 drivers/gpu/drm/i915/gt/intel_gt_types.h |   1 +
 drivers/gpu/drm/i915/gt/sysfs_gt.c       |  85 +++++
 drivers/gpu/drm/i915/gt/sysfs_gt.h       |  22 ++
 drivers/gpu/drm/i915/gt/sysfs_gt_pm.c    | 446 +++++++++++++++++++++++
 drivers/gpu/drm/i915/gt/sysfs_gt_pm.h    |  17 +
 drivers/gpu/drm/i915/i915_sysfs.c        | 375 +------------------
 drivers/gpu/drm/i915/i915_sysfs.h        |   3 +
 9 files changed, 581 insertions(+), 375 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt.c
 create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt.h
 create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
 create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt_pm.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 49eed50ef0a4..3b81c8a96c06 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -106,7 +106,9 @@ gt-y += \
 	gt/intel_rps.o \
 	gt/intel_sseu.o \
 	gt/intel_timeline.o \
-	gt/intel_workarounds.o
+	gt/intel_workarounds.o \
+	gt/sysfs_gt.o \
+	gt/sysfs_gt_pm.o
 # autogenerated null render state
 gt-y += \
 	gt/gen6_renderstate.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index f1f1b306e0af..e794d05d69a1 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -15,6 +15,7 @@
 #include "intel_rps.h"
 #include "intel_uncore.h"
 #include "intel_pm.h"
+#include "sysfs_gt.h"
 
 void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915)
 {
@@ -321,6 +322,7 @@ void intel_gt_driver_register(struct intel_gt *gt)
 	intel_rps_driver_register(&gt->rps);
 
 	debugfs_gt_register(gt);
+	intel_gt_sysfs_register(gt);
 }
 
 static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
@@ -641,6 +643,7 @@ void intel_gt_driver_remove(struct intel_gt *gt)
 
 void intel_gt_driver_unregister(struct intel_gt *gt)
 {
+	intel_gt_sysfs_unregister(gt);
 	intel_rps_driver_unregister(&gt->rps);
 }
 
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index 96890dd12b5f..552a27cc0622 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
@@ -32,6 +32,7 @@ struct intel_gt {
 	struct drm_i915_private *i915;
 	struct intel_uncore *uncore;
 	struct i915_ggtt *ggtt;
+	struct kobject kobj;
 
 	struct intel_uc uc;
 
diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt.c b/drivers/gpu/drm/i915/gt/sysfs_gt.c
new file mode 100644
index 000000000000..4ca140fc215f
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: MIT
+
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include <linux/sysfs.h>
+#include <drm/drm_device.h>
+#include <linux/kobject.h>
+#include <linux/printk.h>
+
+#include "../i915_drv.h"
+#include "intel_gt.h"
+#include "intel_gt_types.h"
+#include "intel_rc6.h"
+
+#include "sysfs_gt.h"
+#include "sysfs_gt_pm.h"
+
+static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
+{
+	return kobject_get(&gt->i915->drm.primary->kdev->kobj);
+}
+
+static ssize_t gt_info_show(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buff)
+{
+	return snprintf(buff, PAGE_SIZE, "0\n");
+}
+
+static DEVICE_ATTR_RO(gt_info);
+
+static void sysfs_gt_kobj_release(struct kobject *kobj)
+{
+	struct intel_gt *gt = kobj_to_gt(kobj);
+
+	drm_info(&gt->i915->drm, "releasing interface\n");
+}
+
+static struct kobj_type sysfs_gt_ktype = {
+	.release   = sysfs_gt_kobj_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+};
+
+void intel_gt_sysfs_register(struct intel_gt *gt)
+{
+	struct kobject *kparent = gt_to_parent_obj(gt);
+	int ret;
+
+	ret = kobject_init_and_add(&gt->kobj, &sysfs_gt_ktype, kparent, "gt");
+	if (ret) {
+		drm_err(&gt->i915->drm, "failed to initialize sysfs file\n");
+		kobject_put(&gt->kobj);
+		goto parent_files;
+	}
+
+	ret = sysfs_create_file(&gt->kobj, &dev_attr_gt_info.attr);
+	if (ret)
+		drm_err(&gt->i915->drm, "failed to create sysfs gt info files\n");
+
+	intel_gt_sysfs_pm_init(gt, &gt->kobj);
+
+parent_files:
+	/*
+	 * we need to make things right with the
+	 * ABI compatibility. The files were originally
+	 * generated under the parent directory.
+	 */
+	intel_gt_sysfs_pm_init(gt, kparent);
+}
+
+void intel_gt_sysfs_unregister(struct intel_gt *gt)
+{
+	struct kobject *root = gt_to_parent_obj(gt);
+
+	if (&gt->kobj) {
+		sysfs_remove_file(&gt->kobj, &dev_attr_gt_info.attr);
+		intel_gt_sysfs_pm_remove(gt, &gt->kobj);
+		kobject_put(&gt->kobj);
+	}
+
+	intel_gt_sysfs_pm_remove(gt, root);
+	kobject_put(root);
+}
diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt.h b/drivers/gpu/drm/i915/gt/sysfs_gt.h
new file mode 100644
index 000000000000..efff6b884b18
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef SYSFS_GT_H
+#define SYSFS_GT_H
+
+#include "intel_gt_types.h"
+
+struct intel_gt;
+
+static inline struct intel_gt *kobj_to_gt(struct kobject *kobj)
+{
+	return container_of(kobj, struct intel_gt, kobj);
+}
+
+void intel_gt_sysfs_register(struct intel_gt *gt);
+void intel_gt_sysfs_unregister(struct intel_gt *gt);
+
+#endif /* SYSFS_GT_H */
diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
new file mode 100644
index 000000000000..a3746485569f
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
@@ -0,0 +1,446 @@
+// SPDX-License-Identifier: MIT
+
+/*
+ * Copyright © 2019 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_rc6.h"
+#include "intel_rps.h"
+#include "sysfs_gt.h"
+#include "sysfs_gt_pm.h"
+
+struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
+{
+	struct kobject *kobj = &dev->kobj;
+	/*
+	 * We are interested at knowing from where the interface
+	 * has been called, whether it's called from gt/ or from
+	 * the parent directory.
+	 * From the interface position it depends also the value of
+	 * the private data.
+	 * If the interface is called from gt/ then private data is
+	 * of the "struct intel_gt *" type, otherwise it's * a
+	 * "struct drm_i915_private *" type.
+	 */
+	if (strcmp(dev->kobj.name, "gt")) {
+		struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
+
+		drm_warn(&i915->drm, "the interface is obsolete, use gt/\n");
+		return &i915->gt;
+	}
+
+	return kobj_to_gt(kobj);
+}
+
+#ifdef CONFIG_PM
+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);
+	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 snprintf(buff, PAGE_SIZE, "%x\n", mask);
+}
+
+static ssize_t rc6_residency_ms_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	u32 rc6_residency = get_residency(gt, GEN6_GT_GFX_RC6);
+
+	return snprintf(buff, PAGE_SIZE, "%u\n", rc6_residency);
+}
+
+static ssize_t rc6p_residency_ms_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	u32 rc6p_residency = get_residency(gt, GEN6_GT_GFX_RC6p);
+
+	return snprintf(buff, PAGE_SIZE, "%u\n", rc6p_residency);
+}
+
+static ssize_t rc6pp_residency_ms_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	u32 rc6pp_residency = get_residency(gt, GEN6_GT_GFX_RC6pp);
+
+	return snprintf(buff, PAGE_SIZE, "%u\n", rc6pp_residency);
+}
+
+static ssize_t media_rc6_residency_ms_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	u32 rc6_residency = get_residency(gt, VLV_GT_MEDIA_RC6);
+
+	return snprintf(buff, PAGE_SIZE, "%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 const struct attribute *rc6_attrs[] = {
+	&dev_attr_rc6_enable.attr,
+	&dev_attr_rc6_residency_ms.attr,
+	NULL
+};
+
+static const struct attribute *rc6p_attrs[] = {
+	&dev_attr_rc6p_residency_ms.attr,
+	&dev_attr_rc6pp_residency_ms.attr,
+	NULL
+};
+
+static const struct attribute *media_rc6_attrs[] = {
+	&dev_attr_media_rc6_residency_ms.attr,
+	NULL
+};
+
+static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
+{
+	int ret = 0;
+
+	if (HAS_RC6(gt->i915)) {
+		ret = sysfs_create_files(kobj, rc6_attrs);
+		if (ret)
+			drm_err(&gt->i915->drm,
+				"failed to create RC6 sysfs files\n");
+	}
+
+	if (HAS_RC6p(gt->i915)) {
+		ret = sysfs_create_files(kobj, rc6p_attrs);
+		if (ret)
+			drm_err(&gt->i915->drm,
+				"failed to create RC6p sysfs files\n");
+	}
+
+	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) {
+		ret = sysfs_create_files(kobj, media_rc6_attrs);
+		if (ret)
+			drm_err(&gt->i915->drm,
+				"failed to create media RC6 sysfs files\n");
+	}
+}
+#else
+static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
+{
+	return 0;
+}
+#endif /* CONFIG_PM */
+
+static ssize_t gt_act_freq_mhz_show(struct device *dev,
+				    struct device_attribute *attr, char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_rps_read_actual_frequency(&gt->rps));
+}
+
+static ssize_t gt_cur_freq_mhz_show(struct device *dev,
+				    struct device_attribute *attr, char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(rps, rps->cur_freq));
+}
+
+static ssize_t gt_boost_freq_mhz_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(rps, rps->boost_freq));
+}
+
+static ssize_t gt_boost_freq_mhz_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buff, size_t count)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+	bool boost = false;
+	ssize_t ret;
+	u32 val;
+
+	ret = kstrtou32(buff, 0, &val);
+	if (ret)
+		return ret;
+
+	/* Validate against (static) hardware limits */
+	val = intel_freq_opcode(rps, val);
+	if (val < rps->min_freq || val > rps->max_freq)
+		return -EINVAL;
+
+	mutex_lock(&rps->lock);
+	if (val != rps->boost_freq) {
+		rps->boost_freq = val;
+		boost = atomic_read(&rps->num_waiters);
+	}
+	mutex_unlock(&rps->lock);
+	if (boost)
+		schedule_work(&rps->work);
+
+	return count;
+}
+
+static ssize_t vlv_rpe_freq_mhz_show(struct device *dev,
+				     struct device_attribute *attr, char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(rps, rps->efficient_freq));
+}
+
+static ssize_t gt_max_freq_mhz_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(rps, rps->max_freq_softlimit));
+}
+
+static ssize_t gt_max_freq_mhz_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buff, size_t count)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+	ssize_t ret;
+	u32 val;
+
+	ret = kstrtou32(buff, 0, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&rps->lock);
+
+	val = intel_freq_opcode(rps, val);
+	if (val < rps->min_freq ||
+	    val > rps->max_freq ||
+	    val < rps->min_freq_softlimit) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	if (val > rps->rp0_freq)
+		DRM_DEBUG("User requested overclocking to %d\n",
+			  intel_gpu_freq(rps, val));
+
+	rps->max_freq_softlimit = val;
+
+	val = clamp_t(int, rps->cur_freq,
+		      rps->min_freq_softlimit,
+		      rps->max_freq_softlimit);
+
+	/*
+	 * We still need *_set_rps to process the new max_delay and
+	 * update the interrupt limits and PMINTRMSK even though
+	 * frequency request may be unchanged.
+	 */
+	intel_rps_set(rps, val);
+
+unlock:
+	mutex_unlock(&rps->lock);
+
+	return ret ?: count;
+}
+
+static ssize_t gt_min_freq_mhz_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(rps, rps->min_freq_softlimit));
+}
+
+static ssize_t gt_min_freq_mhz_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buff, size_t count)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+	ssize_t ret;
+	u32 val;
+
+	ret = kstrtou32(buff, 0, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&rps->lock);
+
+	val = intel_freq_opcode(rps, val);
+	if (val < rps->min_freq ||
+	    val > rps->max_freq ||
+	    val > rps->max_freq_softlimit) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	rps->min_freq_softlimit = val;
+
+	val = clamp_t(int, rps->cur_freq,
+		      rps->min_freq_softlimit,
+		      rps->max_freq_softlimit);
+
+	/*
+	 * We still need *_set_rps to process the new min_delay and
+	 * update the interrupt limits and PMINTRMSK even though
+	 * frequency request may be unchanged.
+	 */
+	intel_rps_set(rps, val);
+
+unlock:
+	mutex_unlock(&rps->lock);
+
+	return ret ?: count;
+}
+
+static DEVICE_ATTR_RO(gt_act_freq_mhz);
+static DEVICE_ATTR_RO(gt_cur_freq_mhz);
+static DEVICE_ATTR_RW(gt_boost_freq_mhz);
+static DEVICE_ATTR_RW(gt_max_freq_mhz);
+static DEVICE_ATTR_RW(gt_min_freq_mhz);
+
+static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
+
+static ssize_t gt_rp_mhz_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buff);
+
+static DEVICE_ATTR(gt_RP0_freq_mhz, 0444, gt_rp_mhz_show, NULL);
+static DEVICE_ATTR(gt_RP1_freq_mhz, 0444, gt_rp_mhz_show, NULL);
+static DEVICE_ATTR(gt_RPn_freq_mhz, 0444, gt_rp_mhz_show, NULL);
+
+/* For now we have a static number of RP states */
+static ssize_t gt_rp_mhz_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+	u32 val;
+
+	if (attr == &dev_attr_gt_RP0_freq_mhz)
+		val = intel_gpu_freq(rps, rps->rp0_freq);
+	else if (attr == &dev_attr_gt_RP1_freq_mhz)
+		val = intel_gpu_freq(rps, rps->rp1_freq);
+	else if (attr == &dev_attr_gt_RPn_freq_mhz)
+		val = intel_gpu_freq(rps, rps->min_freq);
+	else
+		BUG();
+
+	return snprintf(buff, PAGE_SIZE, "%d\n", val);
+}
+
+static const struct attribute * const gen6_attrs[] = {
+	&dev_attr_gt_act_freq_mhz.attr,
+	&dev_attr_gt_cur_freq_mhz.attr,
+	&dev_attr_gt_boost_freq_mhz.attr,
+	&dev_attr_gt_max_freq_mhz.attr,
+	&dev_attr_gt_min_freq_mhz.attr,
+	&dev_attr_gt_RP0_freq_mhz.attr,
+	&dev_attr_gt_RP1_freq_mhz.attr,
+	&dev_attr_gt_RPn_freq_mhz.attr,
+	NULL,
+};
+
+static const struct attribute * const vlv_attrs[] = {
+	&dev_attr_gt_act_freq_mhz.attr,
+	&dev_attr_gt_cur_freq_mhz.attr,
+	&dev_attr_gt_boost_freq_mhz.attr,
+	&dev_attr_gt_max_freq_mhz.attr,
+	&dev_attr_gt_min_freq_mhz.attr,
+	&dev_attr_gt_RP0_freq_mhz.attr,
+	&dev_attr_gt_RP1_freq_mhz.attr,
+	&dev_attr_gt_RPn_freq_mhz.attr,
+	&dev_attr_vlv_rpe_freq_mhz.attr,
+	NULL,
+};
+
+static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj)
+{
+	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
+		return sysfs_create_files(kobj, vlv_attrs);
+
+	if (INTEL_GEN(gt->i915) >= 6)
+		return sysfs_create_files(kobj, gen6_attrs);
+
+	return 0;
+}
+
+void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
+{
+	int ret;
+
+	intel_sysfs_rc6_init(gt, kobj);
+
+	ret = intel_sysfs_rps_init(gt, kobj);
+	if (ret)
+		drm_err(&gt->i915->drm, "failed to create RPS sysfs files");
+}
+
+void intel_gt_sysfs_pm_remove(struct intel_gt *gt, struct kobject *kobj)
+{
+#ifdef CONFIG_PM
+	if (HAS_RC6(gt->i915))
+		sysfs_remove_files(kobj, rc6_attrs);
+	if (HAS_RC6p(gt->i915))
+		sysfs_remove_files(kobj, rc6p_attrs);
+	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
+		sysfs_remove_files(kobj, media_rc6_attrs);
+#endif
+
+	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
+		sysfs_remove_files(kobj, vlv_attrs);
+	else
+		sysfs_remove_files(kobj, gen6_attrs);
+}
diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
new file mode 100644
index 000000000000..758d0c3cb998
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef SYSFS_RC6_H
+#define SYSFS_RC6_H
+
+#include <linux/kobject.h>
+
+#include "intel_gt_types.h"
+
+void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj);
+void intel_gt_sysfs_pm_remove(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 c14d762bd652..1298977fc08b 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -38,113 +38,12 @@
 #include "intel_pm.h"
 #include "intel_sideband.h"
 
-static inline struct drm_i915_private *kdev_minor_to_i915(struct device *kdev)
+struct drm_i915_private *kdev_minor_to_i915(struct device *kdev)
 {
 	struct drm_minor *minor = dev_get_drvdata(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(&dev_priv->gt.rc6, reg);
-
-	return DIV_ROUND_CLOSEST_ULL(res, 1000);
-}
-
-static ssize_t
-show_rc6_mask(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 snprintf(buf, PAGE_SIZE, "%x\n", mask);
-}
-
-static ssize_t
-show_rc6_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
-}
-
-static ssize_t
-show_rc6p_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6p_residency);
-}
-
-static ssize_t
-show_rc6pp_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6pp_residency);
-}
-
-static ssize_t
-show_media_rc6_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
-}
-
-static DEVICE_ATTR(rc6_enable, S_IRUGO, show_rc6_mask, NULL);
-static DEVICE_ATTR(rc6_residency_ms, S_IRUGO, show_rc6_ms, NULL);
-static DEVICE_ATTR(rc6p_residency_ms, S_IRUGO, show_rc6p_ms, NULL);
-static DEVICE_ATTR(rc6pp_residency_ms, S_IRUGO, show_rc6pp_ms, NULL);
-static DEVICE_ATTR(media_rc6_residency_ms, S_IRUGO, show_media_rc6_ms, NULL);
-
-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))
@@ -256,239 +155,6 @@ static const struct bin_attribute dpf_attrs_1 = {
 	.private = (void *)1
 };
 
-static ssize_t gt_act_freq_mhz_show(struct device *kdev,
-				    struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &i915->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_rps_read_actual_frequency(rps));
-}
-
-static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
-				    struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &i915->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_gpu_freq(rps, rps->cur_freq));
-}
-
-static ssize_t gt_boost_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &i915->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_gpu_freq(rps, rps->boost_freq));
-}
-
-static ssize_t gt_boost_freq_mhz_store(struct device *kdev,
-				       struct device_attribute *attr,
-				       const char *buf, size_t count)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-	bool boost = false;
-	ssize_t ret;
-	u32 val;
-
-	ret = kstrtou32(buf, 0, &val);
-	if (ret)
-		return ret;
-
-	/* Validate against (static) hardware limits */
-	val = intel_freq_opcode(rps, val);
-	if (val < rps->min_freq || val > rps->max_freq)
-		return -EINVAL;
-
-	mutex_lock(&rps->lock);
-	if (val != rps->boost_freq) {
-		rps->boost_freq = val;
-		boost = atomic_read(&rps->num_waiters);
-	}
-	mutex_unlock(&rps->lock);
-	if (boost)
-		schedule_work(&rps->work);
-
-	return count;
-}
-
-static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
-				     struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_gpu_freq(rps, rps->efficient_freq));
-}
-
-static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_gpu_freq(rps, rps->max_freq_softlimit));
-}
-
-static ssize_t gt_max_freq_mhz_store(struct device *kdev,
-				     struct device_attribute *attr,
-				     const char *buf, size_t count)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-	ssize_t ret;
-	u32 val;
-
-	ret = kstrtou32(buf, 0, &val);
-	if (ret)
-		return ret;
-
-	mutex_lock(&rps->lock);
-
-	val = intel_freq_opcode(rps, val);
-	if (val < rps->min_freq ||
-	    val > rps->max_freq ||
-	    val < rps->min_freq_softlimit) {
-		ret = -EINVAL;
-		goto unlock;
-	}
-
-	if (val > rps->rp0_freq)
-		DRM_DEBUG("User requested overclocking to %d\n",
-			  intel_gpu_freq(rps, val));
-
-	rps->max_freq_softlimit = val;
-
-	val = clamp_t(int, rps->cur_freq,
-		      rps->min_freq_softlimit,
-		      rps->max_freq_softlimit);
-
-	/*
-	 * We still need *_set_rps to process the new max_delay and
-	 * update the interrupt limits and PMINTRMSK even though
-	 * frequency request may be unchanged.
-	 */
-	intel_rps_set(rps, val);
-
-unlock:
-	mutex_unlock(&rps->lock);
-
-	return ret ?: count;
-}
-
-static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_gpu_freq(rps, rps->min_freq_softlimit));
-}
-
-static ssize_t gt_min_freq_mhz_store(struct device *kdev,
-				     struct device_attribute *attr,
-				     const char *buf, size_t count)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-	ssize_t ret;
-	u32 val;
-
-	ret = kstrtou32(buf, 0, &val);
-	if (ret)
-		return ret;
-
-	mutex_lock(&rps->lock);
-
-	val = intel_freq_opcode(rps, val);
-	if (val < rps->min_freq ||
-	    val > rps->max_freq ||
-	    val > rps->max_freq_softlimit) {
-		ret = -EINVAL;
-		goto unlock;
-	}
-
-	rps->min_freq_softlimit = val;
-
-	val = clamp_t(int, rps->cur_freq,
-		      rps->min_freq_softlimit,
-		      rps->max_freq_softlimit);
-
-	/*
-	 * We still need *_set_rps to process the new min_delay and
-	 * update the interrupt limits and PMINTRMSK even though
-	 * frequency request may be unchanged.
-	 */
-	intel_rps_set(rps, val);
-
-unlock:
-	mutex_unlock(&rps->lock);
-
-	return ret ?: count;
-}
-
-static DEVICE_ATTR_RO(gt_act_freq_mhz);
-static DEVICE_ATTR_RO(gt_cur_freq_mhz);
-static DEVICE_ATTR_RW(gt_boost_freq_mhz);
-static DEVICE_ATTR_RW(gt_max_freq_mhz);
-static DEVICE_ATTR_RW(gt_min_freq_mhz);
-
-static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
-
-static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf);
-static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
-static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
-static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
-
-/* For now we have a static number of RP states */
-static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-	u32 val;
-
-	if (attr == &dev_attr_gt_RP0_freq_mhz)
-		val = intel_gpu_freq(rps, rps->rp0_freq);
-	else if (attr == &dev_attr_gt_RP1_freq_mhz)
-		val = intel_gpu_freq(rps, rps->rp1_freq);
-	else if (attr == &dev_attr_gt_RPn_freq_mhz)
-		val = intel_gpu_freq(rps, rps->min_freq);
-	else
-		BUG();
-
-	return snprintf(buf, PAGE_SIZE, "%d\n", val);
-}
-
-static const struct attribute * const gen6_attrs[] = {
-	&dev_attr_gt_act_freq_mhz.attr,
-	&dev_attr_gt_cur_freq_mhz.attr,
-	&dev_attr_gt_boost_freq_mhz.attr,
-	&dev_attr_gt_max_freq_mhz.attr,
-	&dev_attr_gt_min_freq_mhz.attr,
-	&dev_attr_gt_RP0_freq_mhz.attr,
-	&dev_attr_gt_RP1_freq_mhz.attr,
-	&dev_attr_gt_RPn_freq_mhz.attr,
-	NULL,
-};
-
-static const struct attribute * const vlv_attrs[] = {
-	&dev_attr_gt_act_freq_mhz.attr,
-	&dev_attr_gt_cur_freq_mhz.attr,
-	&dev_attr_gt_boost_freq_mhz.attr,
-	&dev_attr_gt_max_freq_mhz.attr,
-	&dev_attr_gt_min_freq_mhz.attr,
-	&dev_attr_gt_RP0_freq_mhz.attr,
-	&dev_attr_gt_RP1_freq_mhz.attr,
-	&dev_attr_gt_RPn_freq_mhz.attr,
-	&dev_attr_vlv_rpe_freq_mhz.attr,
-	NULL,
-};
-
 #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
 
 static ssize_t error_state_read(struct file *filp, struct kobject *kobj,
@@ -559,29 +225,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)
@@ -597,14 +240,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
 		}
 	}
 
-	ret = 0;
-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		ret = sysfs_create_files(&kdev->kobj, vlv_attrs);
-	else if (INTEL_GEN(dev_priv) >= 6)
-		ret = sysfs_create_files(&kdev->kobj, gen6_attrs);
-	if (ret)
-		drm_err(&dev_priv->drm, "RPS sysfs setup failed\n");
-
 	i915_setup_error_capture(kdev);
 }
 
@@ -614,14 +249,6 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
 
 	i915_teardown_error_capture(kdev);
 
-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		sysfs_remove_files(&kdev->kobj, vlv_attrs);
-	else
-		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
 }
diff --git a/drivers/gpu/drm/i915/i915_sysfs.h b/drivers/gpu/drm/i915/i915_sysfs.h
index 41afd4366416..ad6114de81c9 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.h
+++ b/drivers/gpu/drm/i915/i915_sysfs.h
@@ -6,8 +6,11 @@
 #ifndef __I915_SYSFS_H__
 #define __I915_SYSFS_H__
 
+#include <linux/device.h>
+
 struct drm_i915_private;
 
+struct drm_i915_private *kdev_minor_to_i915(struct device *kdev);
 void i915_setup_sysfs(struct drm_i915_private *i915);
 void i915_teardown_sysfs(struct drm_i915_private *i915);
 
-- 
2.25.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/gt: make a gt sysfs group and move power management files (rev3)
  2020-02-14 11:03 [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Andi Shyti
@ 2020-02-14 12:51 ` Patchwork
  2020-02-14 12:52 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2020-02-14 12:51 UTC (permalink / raw)
  To: Andi Shyti; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/gt: make a gt sysfs group and move power management files (rev3)
URL   : https://patchwork.freedesktop.org/series/73190/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
2bb6ef6acc00 drm/i915/gt: make a gt sysfs group and move power management files
-:71: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#71: 
new file mode 100644

-:553: WARNING:DEVICE_ATTR_FUNCTIONS: Consider renaming function(s) 'gt_rp_mhz_show' to 'gt_RP0_freq_mhz_show'
#553: FILE: drivers/gpu/drm/i915/gt/sysfs_gt_pm.c:359:
+static DEVICE_ATTR(gt_RP0_freq_mhz, 0444, gt_rp_mhz_show, NULL);

-:554: WARNING:DEVICE_ATTR_FUNCTIONS: Consider renaming function(s) 'gt_rp_mhz_show' to 'gt_RP1_freq_mhz_show'
#554: FILE: drivers/gpu/drm/i915/gt/sysfs_gt_pm.c:360:
+static DEVICE_ATTR(gt_RP1_freq_mhz, 0444, gt_rp_mhz_show, NULL);

-:555: CHECK:CAMELCASE: Avoid CamelCase: <gt_RPn_freq_mhz>
#555: FILE: drivers/gpu/drm/i915/gt/sysfs_gt_pm.c:361:
+static DEVICE_ATTR(gt_RPn_freq_mhz, 0444, gt_rp_mhz_show, NULL);

-:555: WARNING:DEVICE_ATTR_FUNCTIONS: Consider renaming function(s) 'gt_rp_mhz_show' to 'gt_RPn_freq_mhz_show'
#555: FILE: drivers/gpu/drm/i915/gt/sysfs_gt_pm.c:361:
+static DEVICE_ATTR(gt_RPn_freq_mhz, 0444, gt_rp_mhz_show, NULL);

-:570: CHECK:CAMELCASE: Avoid CamelCase: <dev_attr_gt_RPn_freq_mhz>
#570: FILE: drivers/gpu/drm/i915/gt/sysfs_gt_pm.c:376:
+	else if (attr == &dev_attr_gt_RPn_freq_mhz)

-:573: WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#573: FILE: drivers/gpu/drm/i915/gt/sysfs_gt_pm.c:379:
+		BUG();

total: 0 errors, 5 warnings, 2 checks, 1029 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/gt: make a gt sysfs group and move power management files (rev3)
  2020-02-14 11:03 [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Andi Shyti
  2020-02-14 12:51 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/gt: make a gt sysfs group and move power management files (rev3) Patchwork
@ 2020-02-14 12:52 ` " Patchwork
  2020-02-14 12:54 ` [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Tvrtko Ursulin
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2020-02-14 12:52 UTC (permalink / raw)
  To: Andi Shyti; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/gt: make a gt sysfs group and move power management files (rev3)
URL   : https://patchwork.freedesktop.org/series/73190/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.6.0
Commit: drm/i915/gt: make a gt sysfs group and move power management files
+drivers/gpu/drm/i915/gt/sysfs_gt_pm.c:19:17: warning: symbol 'intel_gt_sysfs_get_drvdata' was not declared. Should it be static?

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-14 11:03 [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Andi Shyti
  2020-02-14 12:51 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/gt: make a gt sysfs group and move power management files (rev3) Patchwork
  2020-02-14 12:52 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
@ 2020-02-14 12:54 ` Tvrtko Ursulin
  2020-02-14 13:16   ` Andi Shyti
  2020-02-14 13:18   ` Chris Wilson
  2020-02-14 13:13 ` [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/gt: make a gt sysfs group and move power management files (rev3) Patchwork
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2020-02-14 12:54 UTC (permalink / raw)
  To: Andi Shyti, Intel GFX


On 14/02/2020 11:03, Andi Shyti wrote:
> The GT has its own properties and in sysfs they should be grouped
> in the 'gt/' directory.
> 
> Create the 'gt/' directory in sysfs and move the power management
> related files.

Can you paste the new and legacy paths in the commit message?

> 
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
> Hi,
> 
> this version has some more substantial differences, nothing that
> changes the wanted behavior, though.
> 
> Thanks Chris, Jani and Tvrtko for the reviews,
> Andi
> 
> Changelog:
> ==========
> 
> v2 -> v3:
>   - fix some cleanups that I forgot in the previous patch
>   - fix reference pointers to the gt structure
>   - and many other small changes here and there.
> v1 -> v2:
>   - keep the existing files as they are
>   - use "intel_gt_*" as prefix instead of "sysfs_*"
> 
>   drivers/gpu/drm/i915/Makefile            |   4 +-
>   drivers/gpu/drm/i915/gt/intel_gt.c       |   3 +
>   drivers/gpu/drm/i915/gt/intel_gt_types.h |   1 +
>   drivers/gpu/drm/i915/gt/sysfs_gt.c       |  85 +++++
>   drivers/gpu/drm/i915/gt/sysfs_gt.h       |  22 ++
>   drivers/gpu/drm/i915/gt/sysfs_gt_pm.c    | 446 +++++++++++++++++++++++
>   drivers/gpu/drm/i915/gt/sysfs_gt_pm.h    |  17 +
>   drivers/gpu/drm/i915/i915_sysfs.c        | 375 +------------------
>   drivers/gpu/drm/i915/i915_sysfs.h        |   3 +
>   9 files changed, 581 insertions(+), 375 deletions(-)
>   create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt.c
>   create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt.h
>   create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
>   create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 49eed50ef0a4..3b81c8a96c06 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -106,7 +106,9 @@ gt-y += \
>   	gt/intel_rps.o \
>   	gt/intel_sseu.o \
>   	gt/intel_timeline.o \
> -	gt/intel_workarounds.o
> +	gt/intel_workarounds.o \
> +	gt/sysfs_gt.o \
> +	gt/sysfs_gt_pm.o
>   # autogenerated null render state
>   gt-y += \
>   	gt/gen6_renderstate.o \
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index f1f1b306e0af..e794d05d69a1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -15,6 +15,7 @@
>   #include "intel_rps.h"
>   #include "intel_uncore.h"
>   #include "intel_pm.h"
> +#include "sysfs_gt.h"
>   
>   void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915)
>   {
> @@ -321,6 +322,7 @@ void intel_gt_driver_register(struct intel_gt *gt)
>   	intel_rps_driver_register(&gt->rps);
>   
>   	debugfs_gt_register(gt);
> +	intel_gt_sysfs_register(gt);
>   }
>   
>   static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
> @@ -641,6 +643,7 @@ void intel_gt_driver_remove(struct intel_gt *gt)
>   
>   void intel_gt_driver_unregister(struct intel_gt *gt)
>   {
> +	intel_gt_sysfs_unregister(gt);
>   	intel_rps_driver_unregister(&gt->rps);
>   }
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> index 96890dd12b5f..552a27cc0622 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -32,6 +32,7 @@ struct intel_gt {
>   	struct drm_i915_private *i915;
>   	struct intel_uncore *uncore;
>   	struct i915_ggtt *ggtt;
> +	struct kobject kobj;

sysfs_root or something like would perhaps be more descriptive?

>   
>   	struct intel_uc uc;
>   
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt.c b/drivers/gpu/drm/i915/gt/sysfs_gt.c
> new file mode 100644
> index 000000000000..4ca140fc215f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: MIT
> +
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include <linux/sysfs.h>
> +#include <drm/drm_device.h>
> +#include <linux/kobject.h>
> +#include <linux/printk.h>
> +
> +#include "../i915_drv.h"
> +#include "intel_gt.h"
> +#include "intel_gt_types.h"
> +#include "intel_rc6.h"
> +
> +#include "sysfs_gt.h"
> +#include "sysfs_gt_pm.h"
> +
> +static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
> +{
> +	return kobject_get(&gt->i915->drm.primary->kdev->kobj);

It's a bit surprising X_to_Y helper get a reference as well, no? 
gt_get_parent_obj perhaps? But where is this released?

> +}
> +
> +static ssize_t gt_info_show(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buff)
> +{
> +	return snprintf(buff, PAGE_SIZE, "0\n");
> +}
> +
> +static DEVICE_ATTR_RO(gt_info);
> +
> +static void sysfs_gt_kobj_release(struct kobject *kobj)
> +{
> +	struct intel_gt *gt = kobj_to_gt(kobj);
> +
> +	drm_info(&gt->i915->drm, "releasing interface\n");

Debugging remnants.

> +}
> +
> +static struct kobj_type sysfs_gt_ktype = {
> +	.release   = sysfs_gt_kobj_release,
> +	.sysfs_ops = &kobj_sysfs_ops,
> +};
> +
> +void intel_gt_sysfs_register(struct intel_gt *gt)
> +{
> +	struct kobject *kparent = gt_to_parent_obj(gt);
> +	int ret;
> +
> +	ret = kobject_init_and_add(&gt->kobj, &sysfs_gt_ktype, kparent, "gt");
> +	if (ret) {
> +		drm_err(&gt->i915->drm, "failed to initialize sysfs file\n");
> +		kobject_put(&gt->kobj);

So you want gt->kobj to be embedded struct and you want to then override 
the release vfunc so it is not freed, but what is the specific reason 
you want it embedded?

> +		goto parent_files;
> +	}
> +
> +	ret = sysfs_create_file(&gt->kobj, &dev_attr_gt_info.attr);
> +	if (ret)
> +		drm_err(&gt->i915->drm, "failed to create sysfs gt info files\n");
> +
> +	intel_gt_sysfs_pm_init(gt, &gt->kobj);
> +
> +parent_files:
> +	/*
> +	 * we need to make things right with the
> +	 * ABI compatibility. The files were originally
> +	 * generated under the parent directory.
> +	 */
> +	intel_gt_sysfs_pm_init(gt, kparent);
> +}
> +
> +void intel_gt_sysfs_unregister(struct intel_gt *gt)
> +{
> +	struct kobject *root = gt_to_parent_obj(gt);
> +
> +	if (&gt->kobj) {

This is always true.

> +		sysfs_remove_file(&gt->kobj, &dev_attr_gt_info.attr);
> +		intel_gt_sysfs_pm_remove(gt, &gt->kobj);
> +		kobject_put(&gt->kobj);

I think kobject_put is enough to tear down the whole hierarchy so you 
could simplify this.

> +	}
> +
> +	intel_gt_sysfs_pm_remove(gt, root);
> +	kobject_put(root);

Maybe stick to the same terminology regarding root and parent.

Get/put on the parent looks unbalanced. Both register and unregister 
take a reference and only unregister releases it. But do you even need a 
reference?

> +}
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt.h b/drivers/gpu/drm/i915/gt/sysfs_gt.h
> new file mode 100644
> index 000000000000..efff6b884b18
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef SYSFS_GT_H
> +#define SYSFS_GT_H
> +
> +#include "intel_gt_types.h"
> +
> +struct intel_gt;
> +
> +static inline struct intel_gt *kobj_to_gt(struct kobject *kobj)
> +{
> +	return container_of(kobj, struct intel_gt, kobj);
> +}
> +
> +void intel_gt_sysfs_register(struct intel_gt *gt);
> +void intel_gt_sysfs_unregister(struct intel_gt *gt);
> +
> +#endif /* SYSFS_GT_H */
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
> new file mode 100644
> index 000000000000..a3746485569f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
> @@ -0,0 +1,446 @@
> +// SPDX-License-Identifier: MIT
> +
> +/*
> + * Copyright © 2019 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_rc6.h"
> +#include "intel_rps.h"
> +#include "sysfs_gt.h"
> +#include "sysfs_gt_pm.h"
> +
> +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
> +{
> +	struct kobject *kobj = &dev->kobj;
> +	/*
> +	 * We are interested at knowing from where the interface
> +	 * has been called, whether it's called from gt/ or from
> +	 * the parent directory.
> +	 * From the interface position it depends also the value of
> +	 * the private data.
> +	 * If the interface is called from gt/ then private data is
> +	 * of the "struct intel_gt *" type, otherwise it's * a
> +	 * "struct drm_i915_private *" type.
> +	 */
> +	if (strcmp(dev->kobj.name, "gt")) {
> +		struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> +
> +		drm_warn(&i915->drm, "the interface is obsolete, use gt/\n");

Can you log current->name & pid?

I am also thinking is a level down from warn would be better. Notice 
sounds intuitively correct to me.

I am also tempted by the _once alternative, but then it makes less sense 
to include name & pid.

Regards,

Tvrtko

> +		return &i915->gt;
> +	}
> +
> +	return kobj_to_gt(kobj);
> +}
> +
> +#ifdef CONFIG_PM
> +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);
> +	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 snprintf(buff, PAGE_SIZE, "%x\n", mask);
> +}
> +
> +static ssize_t rc6_residency_ms_show(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	u32 rc6_residency = get_residency(gt, GEN6_GT_GFX_RC6);
> +
> +	return snprintf(buff, PAGE_SIZE, "%u\n", rc6_residency);
> +}
> +
> +static ssize_t rc6p_residency_ms_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	u32 rc6p_residency = get_residency(gt, GEN6_GT_GFX_RC6p);
> +
> +	return snprintf(buff, PAGE_SIZE, "%u\n", rc6p_residency);
> +}
> +
> +static ssize_t rc6pp_residency_ms_show(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	u32 rc6pp_residency = get_residency(gt, GEN6_GT_GFX_RC6pp);
> +
> +	return snprintf(buff, PAGE_SIZE, "%u\n", rc6pp_residency);
> +}
> +
> +static ssize_t media_rc6_residency_ms_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	u32 rc6_residency = get_residency(gt, VLV_GT_MEDIA_RC6);
> +
> +	return snprintf(buff, PAGE_SIZE, "%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 const struct attribute *rc6_attrs[] = {
> +	&dev_attr_rc6_enable.attr,
> +	&dev_attr_rc6_residency_ms.attr,
> +	NULL
> +};
> +
> +static const struct attribute *rc6p_attrs[] = {
> +	&dev_attr_rc6p_residency_ms.attr,
> +	&dev_attr_rc6pp_residency_ms.attr,
> +	NULL
> +};
> +
> +static const struct attribute *media_rc6_attrs[] = {
> +	&dev_attr_media_rc6_residency_ms.attr,
> +	NULL
> +};
> +
> +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	int ret = 0;
> +
> +	if (HAS_RC6(gt->i915)) {
> +		ret = sysfs_create_files(kobj, rc6_attrs);
> +		if (ret)
> +			drm_err(&gt->i915->drm,
> +				"failed to create RC6 sysfs files\n");
> +	}
> +
> +	if (HAS_RC6p(gt->i915)) {
> +		ret = sysfs_create_files(kobj, rc6p_attrs);
> +		if (ret)
> +			drm_err(&gt->i915->drm,
> +				"failed to create RC6p sysfs files\n");
> +	}
> +
> +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) {
> +		ret = sysfs_create_files(kobj, media_rc6_attrs);
> +		if (ret)
> +			drm_err(&gt->i915->drm,
> +				"failed to create media RC6 sysfs files\n");
> +	}
> +}
> +#else
> +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static ssize_t gt_act_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr, char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_rps_read_actual_frequency(&gt->rps));
> +}
> +
> +static ssize_t gt_cur_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr, char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->cur_freq));
> +}
> +
> +static ssize_t gt_boost_freq_mhz_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->boost_freq));
> +}
> +
> +static ssize_t gt_boost_freq_mhz_store(struct device *dev,
> +				       struct device_attribute *attr,
> +				       const char *buff, size_t count)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	bool boost = false;
> +	ssize_t ret;
> +	u32 val;
> +
> +	ret = kstrtou32(buff, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Validate against (static) hardware limits */
> +	val = intel_freq_opcode(rps, val);
> +	if (val < rps->min_freq || val > rps->max_freq)
> +		return -EINVAL;
> +
> +	mutex_lock(&rps->lock);
> +	if (val != rps->boost_freq) {
> +		rps->boost_freq = val;
> +		boost = atomic_read(&rps->num_waiters);
> +	}
> +	mutex_unlock(&rps->lock);
> +	if (boost)
> +		schedule_work(&rps->work);
> +
> +	return count;
> +}
> +
> +static ssize_t vlv_rpe_freq_mhz_show(struct device *dev,
> +				     struct device_attribute *attr, char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->efficient_freq));
> +}
> +
> +static ssize_t gt_max_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->max_freq_softlimit));
> +}
> +
> +static ssize_t gt_max_freq_mhz_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buff, size_t count)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	ssize_t ret;
> +	u32 val;
> +
> +	ret = kstrtou32(buff, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&rps->lock);
> +
> +	val = intel_freq_opcode(rps, val);
> +	if (val < rps->min_freq ||
> +	    val > rps->max_freq ||
> +	    val < rps->min_freq_softlimit) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	if (val > rps->rp0_freq)
> +		DRM_DEBUG("User requested overclocking to %d\n",
> +			  intel_gpu_freq(rps, val));
> +
> +	rps->max_freq_softlimit = val;
> +
> +	val = clamp_t(int, rps->cur_freq,
> +		      rps->min_freq_softlimit,
> +		      rps->max_freq_softlimit);
> +
> +	/*
> +	 * We still need *_set_rps to process the new max_delay and
> +	 * update the interrupt limits and PMINTRMSK even though
> +	 * frequency request may be unchanged.
> +	 */
> +	intel_rps_set(rps, val);
> +
> +unlock:
> +	mutex_unlock(&rps->lock);
> +
> +	return ret ?: count;
> +}
> +
> +static ssize_t gt_min_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->min_freq_softlimit));
> +}
> +
> +static ssize_t gt_min_freq_mhz_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buff, size_t count)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	ssize_t ret;
> +	u32 val;
> +
> +	ret = kstrtou32(buff, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&rps->lock);
> +
> +	val = intel_freq_opcode(rps, val);
> +	if (val < rps->min_freq ||
> +	    val > rps->max_freq ||
> +	    val > rps->max_freq_softlimit) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	rps->min_freq_softlimit = val;
> +
> +	val = clamp_t(int, rps->cur_freq,
> +		      rps->min_freq_softlimit,
> +		      rps->max_freq_softlimit);
> +
> +	/*
> +	 * We still need *_set_rps to process the new min_delay and
> +	 * update the interrupt limits and PMINTRMSK even though
> +	 * frequency request may be unchanged.
> +	 */
> +	intel_rps_set(rps, val);
> +
> +unlock:
> +	mutex_unlock(&rps->lock);
> +
> +	return ret ?: count;
> +}
> +
> +static DEVICE_ATTR_RO(gt_act_freq_mhz);
> +static DEVICE_ATTR_RO(gt_cur_freq_mhz);
> +static DEVICE_ATTR_RW(gt_boost_freq_mhz);
> +static DEVICE_ATTR_RW(gt_max_freq_mhz);
> +static DEVICE_ATTR_RW(gt_min_freq_mhz);
> +
> +static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
> +
> +static ssize_t gt_rp_mhz_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buff);
> +
> +static DEVICE_ATTR(gt_RP0_freq_mhz, 0444, gt_rp_mhz_show, NULL);
> +static DEVICE_ATTR(gt_RP1_freq_mhz, 0444, gt_rp_mhz_show, NULL);
> +static DEVICE_ATTR(gt_RPn_freq_mhz, 0444, gt_rp_mhz_show, NULL);
> +
> +/* For now we have a static number of RP states */
> +static ssize_t gt_rp_mhz_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	u32 val;
> +
> +	if (attr == &dev_attr_gt_RP0_freq_mhz)
> +		val = intel_gpu_freq(rps, rps->rp0_freq);
> +	else if (attr == &dev_attr_gt_RP1_freq_mhz)
> +		val = intel_gpu_freq(rps, rps->rp1_freq);
> +	else if (attr == &dev_attr_gt_RPn_freq_mhz)
> +		val = intel_gpu_freq(rps, rps->min_freq);
> +	else
> +		BUG();
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n", val);
> +}
> +
> +static const struct attribute * const gen6_attrs[] = {
> +	&dev_attr_gt_act_freq_mhz.attr,
> +	&dev_attr_gt_cur_freq_mhz.attr,
> +	&dev_attr_gt_boost_freq_mhz.attr,
> +	&dev_attr_gt_max_freq_mhz.attr,
> +	&dev_attr_gt_min_freq_mhz.attr,
> +	&dev_attr_gt_RP0_freq_mhz.attr,
> +	&dev_attr_gt_RP1_freq_mhz.attr,
> +	&dev_attr_gt_RPn_freq_mhz.attr,
> +	NULL,
> +};
> +
> +static const struct attribute * const vlv_attrs[] = {
> +	&dev_attr_gt_act_freq_mhz.attr,
> +	&dev_attr_gt_cur_freq_mhz.attr,
> +	&dev_attr_gt_boost_freq_mhz.attr,
> +	&dev_attr_gt_max_freq_mhz.attr,
> +	&dev_attr_gt_min_freq_mhz.attr,
> +	&dev_attr_gt_RP0_freq_mhz.attr,
> +	&dev_attr_gt_RP1_freq_mhz.attr,
> +	&dev_attr_gt_RPn_freq_mhz.attr,
> +	&dev_attr_vlv_rpe_freq_mhz.attr,
> +	NULL,
> +};
> +
> +static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
> +		return sysfs_create_files(kobj, vlv_attrs);
> +
> +	if (INTEL_GEN(gt->i915) >= 6)
> +		return sysfs_create_files(kobj, gen6_attrs);
> +
> +	return 0;
> +}
> +
> +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	int ret;
> +
> +	intel_sysfs_rc6_init(gt, kobj);
> +
> +	ret = intel_sysfs_rps_init(gt, kobj);
> +	if (ret)
> +		drm_err(&gt->i915->drm, "failed to create RPS sysfs files");
> +}
> +
> +void intel_gt_sysfs_pm_remove(struct intel_gt *gt, struct kobject *kobj)
> +{
> +#ifdef CONFIG_PM
> +	if (HAS_RC6(gt->i915))
> +		sysfs_remove_files(kobj, rc6_attrs);
> +	if (HAS_RC6p(gt->i915))
> +		sysfs_remove_files(kobj, rc6p_attrs);
> +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
> +		sysfs_remove_files(kobj, media_rc6_attrs);
> +#endif
> +
> +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
> +		sysfs_remove_files(kobj, vlv_attrs);
> +	else
> +		sysfs_remove_files(kobj, gen6_attrs);
> +}
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
> new file mode 100644
> index 000000000000..758d0c3cb998
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef SYSFS_RC6_H
> +#define SYSFS_RC6_H
> +
> +#include <linux/kobject.h>
> +
> +#include "intel_gt_types.h"
> +
> +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj);
> +void intel_gt_sysfs_pm_remove(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 c14d762bd652..1298977fc08b 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -38,113 +38,12 @@
>   #include "intel_pm.h"
>   #include "intel_sideband.h"
>   
> -static inline struct drm_i915_private *kdev_minor_to_i915(struct device *kdev)
> +struct drm_i915_private *kdev_minor_to_i915(struct device *kdev)
>   {
>   	struct drm_minor *minor = dev_get_drvdata(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(&dev_priv->gt.rc6, reg);
> -
> -	return DIV_ROUND_CLOSEST_ULL(res, 1000);
> -}
> -
> -static ssize_t
> -show_rc6_mask(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 snprintf(buf, PAGE_SIZE, "%x\n", mask);
> -}
> -
> -static ssize_t
> -show_rc6_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
> -}
> -
> -static ssize_t
> -show_rc6p_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6p_residency);
> -}
> -
> -static ssize_t
> -show_rc6pp_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6pp_residency);
> -}
> -
> -static ssize_t
> -show_media_rc6_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
> -}
> -
> -static DEVICE_ATTR(rc6_enable, S_IRUGO, show_rc6_mask, NULL);
> -static DEVICE_ATTR(rc6_residency_ms, S_IRUGO, show_rc6_ms, NULL);
> -static DEVICE_ATTR(rc6p_residency_ms, S_IRUGO, show_rc6p_ms, NULL);
> -static DEVICE_ATTR(rc6pp_residency_ms, S_IRUGO, show_rc6pp_ms, NULL);
> -static DEVICE_ATTR(media_rc6_residency_ms, S_IRUGO, show_media_rc6_ms, NULL);
> -
> -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))
> @@ -256,239 +155,6 @@ static const struct bin_attribute dpf_attrs_1 = {
>   	.private = (void *)1
>   };
>   
> -static ssize_t gt_act_freq_mhz_show(struct device *kdev,
> -				    struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &i915->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_rps_read_actual_frequency(rps));
> -}
> -
> -static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> -				    struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &i915->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->cur_freq));
> -}
> -
> -static ssize_t gt_boost_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &i915->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->boost_freq));
> -}
> -
> -static ssize_t gt_boost_freq_mhz_store(struct device *kdev,
> -				       struct device_attribute *attr,
> -				       const char *buf, size_t count)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	bool boost = false;
> -	ssize_t ret;
> -	u32 val;
> -
> -	ret = kstrtou32(buf, 0, &val);
> -	if (ret)
> -		return ret;
> -
> -	/* Validate against (static) hardware limits */
> -	val = intel_freq_opcode(rps, val);
> -	if (val < rps->min_freq || val > rps->max_freq)
> -		return -EINVAL;
> -
> -	mutex_lock(&rps->lock);
> -	if (val != rps->boost_freq) {
> -		rps->boost_freq = val;
> -		boost = atomic_read(&rps->num_waiters);
> -	}
> -	mutex_unlock(&rps->lock);
> -	if (boost)
> -		schedule_work(&rps->work);
> -
> -	return count;
> -}
> -
> -static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
> -				     struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->efficient_freq));
> -}
> -
> -static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->max_freq_softlimit));
> -}
> -
> -static ssize_t gt_max_freq_mhz_store(struct device *kdev,
> -				     struct device_attribute *attr,
> -				     const char *buf, size_t count)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	ssize_t ret;
> -	u32 val;
> -
> -	ret = kstrtou32(buf, 0, &val);
> -	if (ret)
> -		return ret;
> -
> -	mutex_lock(&rps->lock);
> -
> -	val = intel_freq_opcode(rps, val);
> -	if (val < rps->min_freq ||
> -	    val > rps->max_freq ||
> -	    val < rps->min_freq_softlimit) {
> -		ret = -EINVAL;
> -		goto unlock;
> -	}
> -
> -	if (val > rps->rp0_freq)
> -		DRM_DEBUG("User requested overclocking to %d\n",
> -			  intel_gpu_freq(rps, val));
> -
> -	rps->max_freq_softlimit = val;
> -
> -	val = clamp_t(int, rps->cur_freq,
> -		      rps->min_freq_softlimit,
> -		      rps->max_freq_softlimit);
> -
> -	/*
> -	 * We still need *_set_rps to process the new max_delay and
> -	 * update the interrupt limits and PMINTRMSK even though
> -	 * frequency request may be unchanged.
> -	 */
> -	intel_rps_set(rps, val);
> -
> -unlock:
> -	mutex_unlock(&rps->lock);
> -
> -	return ret ?: count;
> -}
> -
> -static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->min_freq_softlimit));
> -}
> -
> -static ssize_t gt_min_freq_mhz_store(struct device *kdev,
> -				     struct device_attribute *attr,
> -				     const char *buf, size_t count)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	ssize_t ret;
> -	u32 val;
> -
> -	ret = kstrtou32(buf, 0, &val);
> -	if (ret)
> -		return ret;
> -
> -	mutex_lock(&rps->lock);
> -
> -	val = intel_freq_opcode(rps, val);
> -	if (val < rps->min_freq ||
> -	    val > rps->max_freq ||
> -	    val > rps->max_freq_softlimit) {
> -		ret = -EINVAL;
> -		goto unlock;
> -	}
> -
> -	rps->min_freq_softlimit = val;
> -
> -	val = clamp_t(int, rps->cur_freq,
> -		      rps->min_freq_softlimit,
> -		      rps->max_freq_softlimit);
> -
> -	/*
> -	 * We still need *_set_rps to process the new min_delay and
> -	 * update the interrupt limits and PMINTRMSK even though
> -	 * frequency request may be unchanged.
> -	 */
> -	intel_rps_set(rps, val);
> -
> -unlock:
> -	mutex_unlock(&rps->lock);
> -
> -	return ret ?: count;
> -}
> -
> -static DEVICE_ATTR_RO(gt_act_freq_mhz);
> -static DEVICE_ATTR_RO(gt_cur_freq_mhz);
> -static DEVICE_ATTR_RW(gt_boost_freq_mhz);
> -static DEVICE_ATTR_RW(gt_max_freq_mhz);
> -static DEVICE_ATTR_RW(gt_min_freq_mhz);
> -
> -static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
> -
> -static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf);
> -static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> -static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> -static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> -
> -/* For now we have a static number of RP states */
> -static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	u32 val;
> -
> -	if (attr == &dev_attr_gt_RP0_freq_mhz)
> -		val = intel_gpu_freq(rps, rps->rp0_freq);
> -	else if (attr == &dev_attr_gt_RP1_freq_mhz)
> -		val = intel_gpu_freq(rps, rps->rp1_freq);
> -	else if (attr == &dev_attr_gt_RPn_freq_mhz)
> -		val = intel_gpu_freq(rps, rps->min_freq);
> -	else
> -		BUG();
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n", val);
> -}
> -
> -static const struct attribute * const gen6_attrs[] = {
> -	&dev_attr_gt_act_freq_mhz.attr,
> -	&dev_attr_gt_cur_freq_mhz.attr,
> -	&dev_attr_gt_boost_freq_mhz.attr,
> -	&dev_attr_gt_max_freq_mhz.attr,
> -	&dev_attr_gt_min_freq_mhz.attr,
> -	&dev_attr_gt_RP0_freq_mhz.attr,
> -	&dev_attr_gt_RP1_freq_mhz.attr,
> -	&dev_attr_gt_RPn_freq_mhz.attr,
> -	NULL,
> -};
> -
> -static const struct attribute * const vlv_attrs[] = {
> -	&dev_attr_gt_act_freq_mhz.attr,
> -	&dev_attr_gt_cur_freq_mhz.attr,
> -	&dev_attr_gt_boost_freq_mhz.attr,
> -	&dev_attr_gt_max_freq_mhz.attr,
> -	&dev_attr_gt_min_freq_mhz.attr,
> -	&dev_attr_gt_RP0_freq_mhz.attr,
> -	&dev_attr_gt_RP1_freq_mhz.attr,
> -	&dev_attr_gt_RPn_freq_mhz.attr,
> -	&dev_attr_vlv_rpe_freq_mhz.attr,
> -	NULL,
> -};
> -
>   #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
>   
>   static ssize_t error_state_read(struct file *filp, struct kobject *kobj,
> @@ -559,29 +225,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)
> @@ -597,14 +240,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
>   		}
>   	}
>   
> -	ret = 0;
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		ret = sysfs_create_files(&kdev->kobj, vlv_attrs);
> -	else if (INTEL_GEN(dev_priv) >= 6)
> -		ret = sysfs_create_files(&kdev->kobj, gen6_attrs);
> -	if (ret)
> -		drm_err(&dev_priv->drm, "RPS sysfs setup failed\n");
> -
>   	i915_setup_error_capture(kdev);
>   }
>   
> @@ -614,14 +249,6 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
>   
>   	i915_teardown_error_capture(kdev);
>   
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		sysfs_remove_files(&kdev->kobj, vlv_attrs);
> -	else
> -		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
>   }
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.h b/drivers/gpu/drm/i915/i915_sysfs.h
> index 41afd4366416..ad6114de81c9 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.h
> +++ b/drivers/gpu/drm/i915/i915_sysfs.h
> @@ -6,8 +6,11 @@
>   #ifndef __I915_SYSFS_H__
>   #define __I915_SYSFS_H__
>   
> +#include <linux/device.h>
> +
>   struct drm_i915_private;
>   
> +struct drm_i915_private *kdev_minor_to_i915(struct device *kdev);
>   void i915_setup_sysfs(struct drm_i915_private *i915);
>   void i915_teardown_sysfs(struct drm_i915_private *i915);
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/gt: make a gt sysfs group and move power management files (rev3)
  2020-02-14 11:03 [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Andi Shyti
                   ` (2 preceding siblings ...)
  2020-02-14 12:54 ` [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Tvrtko Ursulin
@ 2020-02-14 13:13 ` Patchwork
  2020-02-14 13:24   ` Chris Wilson
  2020-02-14 13:14 ` [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Chris Wilson
  2020-02-17 19:04 ` [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/gt: make a gt sysfs group and move power management files (rev3) Patchwork
  5 siblings, 1 reply; 22+ messages in thread
From: Patchwork @ 2020-02-14 13:13 UTC (permalink / raw)
  To: Andi Shyti; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/gt: make a gt sysfs group and move power management files (rev3)
URL   : https://patchwork.freedesktop.org/series/73190/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7939 -> Patchwork_16569
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/index.html

Known issues
------------

  Here are the changes found in Patchwork_16569 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_gem_contexts:
    - fi-cml-s:           [PASS][1] -> [DMESG-FAIL][2] ([i915#877])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/fi-cml-s/igt@i915_selftest@live_gem_contexts.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/fi-cml-s/igt@i915_selftest@live_gem_contexts.html

  
#### Possible fixes ####

  * igt@gem_exec_parallel@contexts:
    - {fi-ehl-1}:         [INCOMPLETE][3] ([i915#937]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/fi-ehl-1/igt@gem_exec_parallel@contexts.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/fi-ehl-1/igt@gem_exec_parallel@contexts.html

  * igt@i915_selftest@live_execlists:
    - fi-icl-y:           [DMESG-FAIL][5] ([fdo#108569]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/fi-icl-y/igt@i915_selftest@live_execlists.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/fi-icl-y/igt@i915_selftest@live_execlists.html

  
#### Warnings ####

  * igt@gem_close_race@basic-threads:
    - fi-byt-j1900:       [INCOMPLETE][7] ([i915#45]) -> [TIMEOUT][8] ([fdo#112271] / [i915#1084] / [i915#816])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/fi-byt-j1900/igt@gem_close_race@basic-threads.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/fi-byt-j1900/igt@gem_close_race@basic-threads.html

  * igt@i915_pm_rpm@basic-rte:
    - fi-kbl-guc:         [FAIL][9] ([i915#579]) -> [SKIP][10] ([fdo#109271])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/fi-kbl-guc/igt@i915_pm_rpm@basic-rte.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/fi-kbl-guc/igt@i915_pm_rpm@basic-rte.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [i915#1084]: https://gitlab.freedesktop.org/drm/intel/issues/1084
  [i915#45]: https://gitlab.freedesktop.org/drm/intel/issues/45
  [i915#579]: https://gitlab.freedesktop.org/drm/intel/issues/579
  [i915#816]: https://gitlab.freedesktop.org/drm/intel/issues/816
  [i915#877]: https://gitlab.freedesktop.org/drm/intel/issues/877
  [i915#937]: https://gitlab.freedesktop.org/drm/intel/issues/937


Participating hosts (44 -> 43)
------------------------------

  Additional (7): fi-hsw-peppy fi-glk-dsi fi-ivb-3770 fi-icl-u3 fi-bsw-kefka fi-skl-6600u fi-kbl-r 
  Missing    (8): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-ctg-p8600 fi-byt-n2820 fi-byt-clapper fi-bdw-samus fi-snb-2600 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7939 -> Patchwork_16569

  CI-20190529: 20190529
  CI_DRM_7939: cceb0c30a34af6ca96e35211ecdc5ca198d44e7e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5441: 534ca091fe4ffed916752165bc5becd7ff56cd84 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16569: 2bb6ef6acc00a99d609b52064cf9baa76bb79004 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2bb6ef6acc00 drm/i915/gt: make a gt sysfs group and move power management files

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-14 11:03 [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Andi Shyti
                   ` (3 preceding siblings ...)
  2020-02-14 13:13 ` [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/gt: make a gt sysfs group and move power management files (rev3) Patchwork
@ 2020-02-14 13:14 ` Chris Wilson
  2020-02-17 19:04 ` [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/gt: make a gt sysfs group and move power management files (rev3) Patchwork
  5 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2020-02-14 13:14 UTC (permalink / raw)
  To: Andi Shyti, Intel GFX

Quoting Andi Shyti (2020-02-14 11:03:08)
> +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
> +{
> +       struct kobject *kobj = &dev->kobj;
> +       /*
> +        * We are interested at knowing from where the interface
> +        * has been called, whether it's called from gt/ or from
> +        * the parent directory.
> +        * From the interface position it depends also the value of
> +        * the private data.
> +        * If the interface is called from gt/ then private data is
> +        * of the "struct intel_gt *" type, otherwise it's * a
> +        * "struct drm_i915_private *" type.
> +        */
> +       if (strcmp(dev->kobj.name, "gt")) {
> +               struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> +
> +               drm_warn(&i915->drm, "the interface is obsolete, use gt/\n");
> +               return &i915->gt;

I guess it's not possible to flesh this out with path? And we do need it
to be warn_once else the user will be flooded.

One final request, can we also put the old entries under
CONFIG_DRM_I915_SYSFS_OBSOLETE_GT (or somesuch)

As far as the naming goes, the compromise isn't horrendous.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-14 12:54 ` [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Tvrtko Ursulin
@ 2020-02-14 13:16   ` Andi Shyti
  2020-02-14 13:38     ` Tvrtko Ursulin
  2020-02-14 13:18   ` Chris Wilson
  1 sibling, 1 reply; 22+ messages in thread
From: Andi Shyti @ 2020-02-14 13:16 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel GFX

Hi Tvrtko,

> > The GT has its own properties and in sysfs they should be grouped
> > in the 'gt/' directory.
> > 
> > Create the 'gt/' directory in sysfs and move the power management
> > related files.
> 
> Can you paste the new and legacy paths in the commit message?

sure!

> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > index 96890dd12b5f..552a27cc0622 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > @@ -32,6 +32,7 @@ struct intel_gt {
> >   	struct drm_i915_private *i915;
> >   	struct intel_uncore *uncore;
> >   	struct i915_ggtt *ggtt;
> > +	struct kobject kobj;
> 
> sysfs_root or something like would perhaps be more descriptive?

it's a kobj, but yes, I can call it that.

> > +static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
> > +{
> > +	return kobject_get(&gt->i915->drm.primary->kdev->kobj);
> 
> It's a bit surprising X_to_Y helper get a reference as well, no?
> gt_get_parent_obj perhaps? But where is this released?

sure!

the kobject put is handled down, for all the cases, have I missed
any?

> > +}
> > +
> > +static ssize_t gt_info_show(struct device *dev,
> > +			    struct device_attribute *attr,
> > +			    char *buff)
> > +{
> > +	return snprintf(buff, PAGE_SIZE, "0\n");
> > +}
> > +
> > +static DEVICE_ATTR_RO(gt_info);
> > +
> > +static void sysfs_gt_kobj_release(struct kobject *kobj)
> > +{
> > +	struct intel_gt *gt = kobj_to_gt(kobj);
> > +
> > +	drm_info(&gt->i915->drm, "releasing interface\n");
> 
> Debugging remnants.

I wanted to fill this function with a goodbye message :)

> > +void intel_gt_sysfs_register(struct intel_gt *gt)
> > +{
> > +	struct kobject *kparent = gt_to_parent_obj(gt);
> > +	int ret;
> > +
> > +	ret = kobject_init_and_add(&gt->kobj, &sysfs_gt_ktype, kparent, "gt");
> > +	if (ret) {
> > +		drm_err(&gt->i915->drm, "failed to initialize sysfs file\n");
> > +		kobject_put(&gt->kobj);
> 
> So you want gt->kobj to be embedded struct and you want to then override the
> release vfunc so it is not freed, but what is the specific reason you want
> it embedded?

it looked to me like the cleanest way.

There is no real "struct device" that is containing the object I
am creating, sot that the set_drvdata() was producing some
unwanted effects. Embedding it in the gt, I can always get
easily to the gt structure containign the kobject.

> > +void intel_gt_sysfs_unregister(struct intel_gt *gt)
> > +{
> > +	struct kobject *root = gt_to_parent_obj(gt);
> > +
> > +	if (&gt->kobj) {
> 
> This is always true.

remannt from a vim replace command :)

> > +		sysfs_remove_file(&gt->kobj, &dev_attr_gt_info.attr);
> > +		intel_gt_sysfs_pm_remove(gt, &gt->kobj);
> > +		kobject_put(&gt->kobj);
> 
> I think kobject_put is enough to tear down the whole hierarchy so you could
> simplify this.

Uh! forgot that kobject was cleaning up everythign. Thanks!

> > +	}
> > +
> > +	intel_gt_sysfs_pm_remove(gt, root);
> > +	kobject_put(root);
> 
> Maybe stick to the same terminology regarding root and parent.

yes.

> Get/put on the parent looks unbalanced. Both register and unregister take a
> reference and only unregister releases it. But do you even need a reference?

why? I take it here:

static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
{                                                           
	return kobject_get(&gt->i915->drm.primary->kdev->kobj);  
}

at the beginning (when the driver is loaded) and I release it at
the end (when the driver is unloaded). Am I not seeing something?

> > +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
> > +{
> > +	struct kobject *kobj = &dev->kobj;
> > +	/*
> > +	 * We are interested at knowing from where the interface
> > +	 * has been called, whether it's called from gt/ or from
> > +	 * the parent directory.
> > +	 * From the interface position it depends also the value of
> > +	 * the private data.
> > +	 * If the interface is called from gt/ then private data is
> > +	 * of the "struct intel_gt *" type, otherwise it's * a
> > +	 * "struct drm_i915_private *" type.
> > +	 */
> > +	if (strcmp(dev->kobj.name, "gt")) {
> > +		struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> > +
> > +		drm_warn(&i915->drm, "the interface is obsolete, use gt/\n");
> 
> Can you log current->name & pid?
> 
> I am also thinking is a level down from warn would be better. Notice sounds
> intuitively correct to me.

I swear, I thought hard to come up with a meaningful message, but
that's the best I came up with.

> I am also tempted by the _once alternative, but then it makes less sense to
> include name & pid.

It's true, it can be an unrelenting message, and I thought of it,
but if the user is resilient at reading out from the wrong
directory, why shouldn't I :)

Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-14 12:54 ` [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Tvrtko Ursulin
  2020-02-14 13:16   ` Andi Shyti
@ 2020-02-14 13:18   ` Chris Wilson
  2020-02-14 13:41     ` Tvrtko Ursulin
  1 sibling, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2020-02-14 13:18 UTC (permalink / raw)
  To: Andi Shyti, Intel GFX, Tvrtko Ursulin

Quoting Tvrtko Ursulin (2020-02-14 12:54:35)
> 
> On 14/02/2020 11:03, Andi Shyti wrote:
> > +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
> > +{
> > +     struct kobject *kobj = &dev->kobj;
> > +     /*
> > +      * We are interested at knowing from where the interface
> > +      * has been called, whether it's called from gt/ or from
> > +      * the parent directory.
> > +      * From the interface position it depends also the value of
> > +      * the private data.
> > +      * If the interface is called from gt/ then private data is
> > +      * of the "struct intel_gt *" type, otherwise it's * a
> > +      * "struct drm_i915_private *" type.
> > +      */
> > +     if (strcmp(dev->kobj.name, "gt")) {
> > +             struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> > +
> > +             drm_warn(&i915->drm, "the interface is obsolete, use gt/\n");
> 
> Can you log current->name & pid?
> 
> I am also thinking is a level down from warn would be better. Notice 
> sounds intuitively correct to me.

git grep -e 'pr.*obsolete' | grep warn | wc -l
21
git grep -e 'pr.*obsolete' | grep notice | wc -l
1
git grep -e 'pr.*obsolete' | grep info | wc -l
4

Looks like warn's back on the menu, boys.

> I am also tempted by the _once alternative, but then it makes less sense 
> to include name & pid.

I'm more afraid that there are users out there that frequently poke
these files.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx]  ✓ Fi.CI.BAT: success for drm/i915/gt: make a gt sysfs group and move power management files (rev3)
  2020-02-14 13:13 ` [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/gt: make a gt sysfs group and move power management files (rev3) Patchwork
@ 2020-02-14 13:24   ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2020-02-14 13:24 UTC (permalink / raw)
  To: Andi Shyti, Patchwork, intel-gfx; +Cc: intel-gfx

Quoting Patchwork (2020-02-14 13:13:51)
> == Series Details ==
> 
> Series: drm/i915/gt: make a gt sysfs group and move power management files (rev3)
> URL   : https://patchwork.freedesktop.org/series/73190/
> State : success
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_7939 -> Patchwork_16569
> ====================================================
> 
> Summary
> -------
> 
>   **SUCCESS**
> 
>   No regressions found.
> 
>   External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/index.html

Hmm, do we not have the sysfs walker in BAT? Apparently not.

This *should* produce a warning :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-14 13:16   ` Andi Shyti
@ 2020-02-14 13:38     ` Tvrtko Ursulin
  2020-02-14 13:57       ` Andi Shyti
  0 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2020-02-14 13:38 UTC (permalink / raw)
  To: Andi Shyti; +Cc: Intel GFX


On 14/02/2020 13:16, Andi Shyti wrote:
> Hi Tvrtko,
> 
>>> The GT has its own properties and in sysfs they should be grouped
>>> in the 'gt/' directory.
>>>
>>> Create the 'gt/' directory in sysfs and move the power management
>>> related files.
>>
>> Can you paste the new and legacy paths in the commit message?
> 
> sure!
> 
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
>>> index 96890dd12b5f..552a27cc0622 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
>>> @@ -32,6 +32,7 @@ struct intel_gt {
>>>    	struct drm_i915_private *i915;
>>>    	struct intel_uncore *uncore;
>>>    	struct i915_ggtt *ggtt;
>>> +	struct kobject kobj;
>>
>> sysfs_root or something like would perhaps be more descriptive?
> 
> it's a kobj, but yes, I can call it that.
> 
>>> +static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
>>> +{
>>> +	return kobject_get(&gt->i915->drm.primary->kdev->kobj);
>>
>> It's a bit surprising X_to_Y helper get a reference as well, no?
>> gt_get_parent_obj perhaps? But where is this released?
> 
> sure!
> 
> the kobject put is handled down, for all the cases, have I missed
> any?
> 
>>> +}
>>> +
>>> +static ssize_t gt_info_show(struct device *dev,
>>> +			    struct device_attribute *attr,
>>> +			    char *buff)
>>> +{
>>> +	return snprintf(buff, PAGE_SIZE, "0\n");
>>> +}
>>> +
>>> +static DEVICE_ATTR_RO(gt_info);
>>> +
>>> +static void sysfs_gt_kobj_release(struct kobject *kobj)
>>> +{
>>> +	struct intel_gt *gt = kobj_to_gt(kobj);
>>> +
>>> +	drm_info(&gt->i915->drm, "releasing interface\n");
>>
>> Debugging remnants.
> 
> I wanted to fill this function with a goodbye message :)
> 
>>> +void intel_gt_sysfs_register(struct intel_gt *gt)
>>> +{
>>> +	struct kobject *kparent = gt_to_parent_obj(gt);
>>> +	int ret;
>>> +
>>> +	ret = kobject_init_and_add(&gt->kobj, &sysfs_gt_ktype, kparent, "gt");
>>> +	if (ret) {
>>> +		drm_err(&gt->i915->drm, "failed to initialize sysfs file\n");
>>> +		kobject_put(&gt->kobj);
>>
>> So you want gt->kobj to be embedded struct and you want to then override the
>> release vfunc so it is not freed, but what is the specific reason you want
>> it embedded?
> 
> it looked to me like the cleanest way.
> 
> There is no real "struct device" that is containing the object I
> am creating, sot that the set_drvdata() was producing some
> unwanted effects. Embedding it in the gt, I can always get
> easily to the gt structure containign the kobject.

Got it.

> 
>>> +void intel_gt_sysfs_unregister(struct intel_gt *gt)
>>> +{
>>> +	struct kobject *root = gt_to_parent_obj(gt);
>>> +
>>> +	if (&gt->kobj) {
>>
>> This is always true.
> 
> remannt from a vim replace command :)
> 
>>> +		sysfs_remove_file(&gt->kobj, &dev_attr_gt_info.attr);
>>> +		intel_gt_sysfs_pm_remove(gt, &gt->kobj);
>>> +		kobject_put(&gt->kobj);
>>
>> I think kobject_put is enough to tear down the whole hierarchy so you could
>> simplify this.
> 
> Uh! forgot that kobject was cleaning up everythign. Thanks!
> 
>>> +	}
>>> +
>>> +	intel_gt_sysfs_pm_remove(gt, root);
>>> +	kobject_put(root);
>>
>> Maybe stick to the same terminology regarding root and parent.
> 
> yes.
> 
>> Get/put on the parent looks unbalanced. Both register and unregister take a
>> reference and only unregister releases it. But do you even need a reference?
> 
> why? I take it here:
> 
> static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
> {
> 	return kobject_get(&gt->i915->drm.primary->kdev->kobj);
> }
> 
> at the beginning (when the driver is loaded) and I release it at
> the end (when the driver is unloaded). Am I not seeing something?

Gt_to_parent_obj at the top of intel_gt_sysfs_register balances out with 
the put at the end of the same function. What balances out 
gt_to_parent_obj from intel_gt_sysfs_register?

> 
>>> +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
>>> +{
>>> +	struct kobject *kobj = &dev->kobj;
>>> +	/*
>>> +	 * We are interested at knowing from where the interface
>>> +	 * has been called, whether it's called from gt/ or from
>>> +	 * the parent directory.
>>> +	 * From the interface position it depends also the value of
>>> +	 * the private data.
>>> +	 * If the interface is called from gt/ then private data is
>>> +	 * of the "struct intel_gt *" type, otherwise it's * a
>>> +	 * "struct drm_i915_private *" type.
>>> +	 */
>>> +	if (strcmp(dev->kobj.name, "gt")) {
>>> +		struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
>>> +
>>> +		drm_warn(&i915->drm, "the interface is obsolete, use gt/\n");
>>
>> Can you log current->name & pid?
>>
>> I am also thinking is a level down from warn would be better. Notice sounds
>> intuitively correct to me.
> 
> I swear, I thought hard to come up with a meaningful message, but
> that's the best I came up with.

At least we need to mention it is about sysfs, it needs to be helpful 
for the userspace developer/user to know what is being access and from 
where.

I suggested to google for this. This is what I came up with as an example:

[  775.385966] batman_adv: [Deprecated]: batadv-vis (pid 3251) Use of 
sysfs file "iface_status".
[  775.385966] Use batadv genl family instead

I am sure there are more examples, I remember many procfs and sysfs 
deprecated interfaces from the past.

>> I am also tempted by the _once alternative, but then it makes less sense to
>> include name & pid.
> 
> It's true, it can be an unrelenting message, and I thought of it,
> but if the user is resilient at reading out from the wrong
> directory, why shouldn't I :)

Because we always try to avoid emitting spammy logs when they can be 
easily triggered by userspace. Can we do rate limit? I think that could 
work well with logging the process name & pid.

Also, we need an entry in Documentation/ABI/obsolete/.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-14 13:18   ` Chris Wilson
@ 2020-02-14 13:41     ` Tvrtko Ursulin
  0 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2020-02-14 13:41 UTC (permalink / raw)
  To: Chris Wilson, Andi Shyti, Intel GFX


On 14/02/2020 13:18, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-02-14 12:54:35)
>>
>> On 14/02/2020 11:03, Andi Shyti wrote:
>>> +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
>>> +{
>>> +     struct kobject *kobj = &dev->kobj;
>>> +     /*
>>> +      * We are interested at knowing from where the interface
>>> +      * has been called, whether it's called from gt/ or from
>>> +      * the parent directory.
>>> +      * From the interface position it depends also the value of
>>> +      * the private data.
>>> +      * If the interface is called from gt/ then private data is
>>> +      * of the "struct intel_gt *" type, otherwise it's * a
>>> +      * "struct drm_i915_private *" type.
>>> +      */
>>> +     if (strcmp(dev->kobj.name, "gt")) {
>>> +             struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
>>> +
>>> +             drm_warn(&i915->drm, "the interface is obsolete, use gt/\n");
>>
>> Can you log current->name & pid?
>>
>> I am also thinking is a level down from warn would be better. Notice
>> sounds intuitively correct to me.
> 
> git grep -e 'pr.*obsolete' | grep warn | wc -l
> 21
> git grep -e 'pr.*obsolete' | grep notice | wc -l
> 1
> git grep -e 'pr.*obsolete' | grep info | wc -l
> 4
> 
> Looks like warn's back on the menu, boys.

Majority is just wrong. :P

>> I am also tempted by the _once alternative, but then it makes less sense
>> to include name & pid.
> 
> I'm more afraid that there are users out there that frequently poke
> these files.

Agreed, I think best option is to go with ratelimited and name & pid 
logged. And more verbosity about what has been access and what should be 
accessed instead.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-14 13:38     ` Tvrtko Ursulin
@ 2020-02-14 13:57       ` Andi Shyti
  0 siblings, 0 replies; 22+ messages in thread
From: Andi Shyti @ 2020-02-14 13:57 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel GFX

Hi Tvrtko,

> > > > +	}
> > > > +
> > > > +	intel_gt_sysfs_pm_remove(gt, root);
> > > > +	kobject_put(root);
> > > 
> > > Maybe stick to the same terminology regarding root and parent.
> > 
> > yes.
> > 
> > > Get/put on the parent looks unbalanced. Both register and unregister take a
> > > reference and only unregister releases it. But do you even need a reference?
> > 
> > why? I take it here:
> > 
> > static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
> > {
> > 	return kobject_get(&gt->i915->drm.primary->kdev->kobj);
> > }
> > 
> > at the beginning (when the driver is loaded) and I release it at
> > the end (when the driver is unloaded). Am I not seeing something?
> 
> Gt_to_parent_obj at the top of intel_gt_sysfs_register balances out with the
> put at the end of the same function. What balances out gt_to_parent_obj from
> intel_gt_sysfs_register?

And... you are right!
either nothing or too many :)

> > > I am also tempted by the _once alternative, but then it makes less sense to
> > > include name & pid.
> > 
> > It's true, it can be an unrelenting message, and I thought of it,
> > but if the user is resilient at reading out from the wrong
> > directory, why shouldn't I :)
> 
> Because we always try to avoid emitting spammy logs when they can be easily
> triggered by userspace. Can we do rate limit? I think that could work well
> with logging the process name & pid.

yes, if two people suggested the same thing, most probably that's
the right thing to do.

> Also, we need an entry in Documentation/ABI/obsolete/.

I was waiting this patch to get in shape before adding the
interface to obsolete.

I will include it in the next patch.

Thanks a lot for the review,
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/gt: make a gt sysfs group and move power management files (rev3)
  2020-02-14 11:03 [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Andi Shyti
                   ` (4 preceding siblings ...)
  2020-02-14 13:14 ` [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Chris Wilson
@ 2020-02-17 19:04 ` Patchwork
  5 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2020-02-17 19:04 UTC (permalink / raw)
  To: Andi Shyti; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/gt: make a gt sysfs group and move power management files (rev3)
URL   : https://patchwork.freedesktop.org/series/73190/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7939_full -> Patchwork_16569_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_16569_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_16569_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_16569_full:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_pm_rc6_residency@rc6-idle:
    - shard-tglb:         [PASS][1] -> [SKIP][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-tglb3/igt@i915_pm_rc6_residency@rc6-idle.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-tglb3/igt@i915_pm_rc6_residency@rc6-idle.html

  * igt@perf@rc6-disable:
    - shard-iclb:         [PASS][3] -> [SKIP][4] +2 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb1/igt@perf@rc6-disable.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb4/igt@perf@rc6-disable.html

  
#### Warnings ####

  * igt@i915_pm_rc6_residency@media-rc6-accuracy:
    - shard-iclb:         [SKIP][5] ([fdo#109289]) -> [SKIP][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb3/igt@i915_pm_rc6_residency@media-rc6-accuracy.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb1/igt@i915_pm_rc6_residency@media-rc6-accuracy.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@gem_ctx_persistence@close-replace-race}:
    - shard-iclb:         [PASS][7] -> [FAIL][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb5/igt@gem_ctx_persistence@close-replace-race.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb5/igt@gem_ctx_persistence@close-replace-race.html

  
Known issues
------------

  Here are the changes found in Patchwork_16569_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_persistence@processes:
    - shard-skl:          [PASS][9] -> [FAIL][10] ([i915#570] / [i915#679])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-skl1/igt@gem_ctx_persistence@processes.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-skl6/igt@gem_ctx_persistence@processes.html

  * igt@gem_exec_await@wide-contexts:
    - shard-kbl:          [PASS][11] -> [INCOMPLETE][12] ([fdo#103665])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-kbl6/igt@gem_exec_await@wide-contexts.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-kbl3/igt@gem_exec_await@wide-contexts.html

  * igt@gem_exec_schedule@in-order-bsd:
    - shard-iclb:         [PASS][13] -> [SKIP][14] ([fdo#112146]) +6 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb8/igt@gem_exec_schedule@in-order-bsd.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb4/igt@gem_exec_schedule@in-order-bsd.html

  * igt@gem_exec_schedule@pi-shared-iova-bsd:
    - shard-iclb:         [PASS][15] -> [SKIP][16] ([i915#677])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb3/igt@gem_exec_schedule@pi-shared-iova-bsd.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb2/igt@gem_exec_schedule@pi-shared-iova-bsd.html

  * igt@gem_linear_blits@normal:
    - shard-hsw:          [PASS][17] -> [FAIL][18] ([i915#694])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-hsw6/igt@gem_linear_blits@normal.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-hsw1/igt@gem_linear_blits@normal.html

  * igt@gen9_exec_parse@allowed-single:
    - shard-skl:          [PASS][19] -> [DMESG-WARN][20] ([i915#716])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-skl1/igt@gen9_exec_parse@allowed-single.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-skl6/igt@gen9_exec_parse@allowed-single.html

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-tglb:         [PASS][21] -> [SKIP][22] ([fdo#111719])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-tglb3/igt@i915_pm_rc6_residency@rc6-accuracy.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-tglb6/igt@i915_pm_rc6_residency@rc6-accuracy.html

  * igt@i915_pm_rc6_residency@rc6-idle:
    - shard-snb:          [PASS][23] -> [SKIP][24] ([fdo#109271]) +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-snb2/igt@i915_pm_rc6_residency@rc6-idle.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-snb4/igt@i915_pm_rc6_residency@rc6-idle.html
    - shard-skl:          [PASS][25] -> [SKIP][26] ([fdo#109271]) +2 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-skl6/igt@i915_pm_rc6_residency@rc6-idle.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-skl9/igt@i915_pm_rc6_residency@rc6-idle.html
    - shard-apl:          [PASS][27] -> [SKIP][28] ([fdo#109271]) +2 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-apl7/igt@i915_pm_rc6_residency@rc6-idle.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-apl3/igt@i915_pm_rc6_residency@rc6-idle.html
    - shard-glk:          [PASS][29] -> [SKIP][30] ([fdo#109271]) +2 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-glk1/igt@i915_pm_rc6_residency@rc6-idle.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-glk2/igt@i915_pm_rc6_residency@rc6-idle.html

  * igt@i915_suspend@forcewake:
    - shard-kbl:          [PASS][31] -> [DMESG-WARN][32] ([i915#180]) +1 similar issue
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-kbl4/igt@i915_suspend@forcewake.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-kbl2/igt@i915_suspend@forcewake.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [PASS][33] -> [FAIL][34] ([i915#79])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-skl7/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-skl3/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-pwrite:
    - shard-tglb:         [PASS][35] -> [SKIP][36] ([i915#668]) +5 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-tglb8/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-pwrite.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-tglb8/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-pwrite.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
    - shard-apl:          [PASS][37] -> [DMESG-WARN][38] ([i915#180]) +1 similar issue
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-apl6/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-apl1/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html
    - shard-iclb:         [PASS][39] -> [INCOMPLETE][40] ([i915#250])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb8/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][41] -> [FAIL][42] ([fdo#108145] / [i915#265])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-skl2/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-skl4/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr@no_drrs:
    - shard-iclb:         [PASS][43] -> [FAIL][44] ([i915#173])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb6/igt@kms_psr@no_drrs.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb1/igt@kms_psr@no_drrs.html

  * igt@kms_psr@psr2_primary_mmap_cpu:
    - shard-iclb:         [PASS][45] -> [SKIP][46] ([fdo#109441]) +1 similar issue
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb8/igt@kms_psr@psr2_primary_mmap_cpu.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [PASS][47] -> [FAIL][48] ([i915#31])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-kbl7/igt@kms_setmode@basic.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-kbl3/igt@kms_setmode@basic.html

  * igt@perf@rc6-disable:
    - shard-hsw:          [PASS][49] -> [SKIP][50] ([fdo#109271]) +2 similar issues
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-hsw4/igt@perf@rc6-disable.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-hsw5/igt@perf@rc6-disable.html
    - shard-kbl:          [PASS][51] -> [SKIP][52] ([fdo#109271]) +2 similar issues
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-kbl3/igt@perf@rc6-disable.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-kbl4/igt@perf@rc6-disable.html

  * igt@perf_pmu@busy-vcs1:
    - shard-iclb:         [PASS][53] -> [SKIP][54] ([fdo#112080]) +12 similar issues
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb1/igt@perf_pmu@busy-vcs1.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb3/igt@perf_pmu@busy-vcs1.html

  * igt@prime_busy@hang-bsd2:
    - shard-iclb:         [PASS][55] -> [SKIP][56] ([fdo#109276]) +19 similar issues
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb2/igt@prime_busy@hang-bsd2.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb7/igt@prime_busy@hang-bsd2.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-kbl:          [DMESG-WARN][57] ([i915#180]) -> [PASS][58] +7 similar issues
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-kbl6/igt@gem_ctx_isolation@rcs0-s3.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-kbl1/igt@gem_ctx_isolation@rcs0-s3.html

  * {igt@gem_ctx_persistence@close-replace-race}:
    - shard-glk:          [FAIL][59] -> [PASS][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-glk8/igt@gem_ctx_persistence@close-replace-race.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-glk4/igt@gem_ctx_persistence@close-replace-race.html

  * igt@gem_exec_schedule@pi-common-bsd1:
    - shard-iclb:         [SKIP][61] ([fdo#109276]) -> [PASS][62] +20 similar issues
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb3/igt@gem_exec_schedule@pi-common-bsd1.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb1/igt@gem_exec_schedule@pi-common-bsd1.html

  * igt@gem_exec_schedule@preempt-other-chain-bsd:
    - shard-iclb:         [SKIP][63] ([fdo#112146]) -> [PASS][64] +8 similar issues
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb2/igt@gem_exec_schedule@preempt-other-chain-bsd.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb7/igt@gem_exec_schedule@preempt-other-chain-bsd.html

  * igt@gem_workarounds@suspend-resume-context:
    - shard-apl:          [DMESG-WARN][65] ([i915#180]) -> [PASS][66] +2 similar issues
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-apl6/igt@gem_workarounds@suspend-resume-context.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-apl1/igt@gem_workarounds@suspend-resume-context.html

  * igt@kms_flip@plain-flip-fb-recreate:
    - shard-kbl:          [FAIL][67] ([i915#34]) -> [PASS][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-kbl4/igt@kms_flip@plain-flip-fb-recreate.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-kbl2/igt@kms_flip@plain-flip-fb-recreate.html

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-mmap-cpu:
    - shard-tglb:         [SKIP][69] ([i915#668]) -> [PASS][70] +9 similar issues
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-tglb6/igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-mmap-cpu.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-tglb6/igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-mmap-cpu.html

  * igt@kms_psr@psr2_suspend:
    - shard-iclb:         [SKIP][71] ([fdo#109441]) -> [PASS][72] +1 similar issue
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb3/igt@kms_psr@psr2_suspend.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb2/igt@kms_psr@psr2_suspend.html

  * igt@kms_setmode@basic:
    - shard-apl:          [FAIL][73] ([i915#31]) -> [PASS][74]
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-apl1/igt@kms_setmode@basic.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-apl1/igt@kms_setmode@basic.html

  * igt@perf_pmu@busy-check-all-vcs1:
    - shard-iclb:         [SKIP][75] ([fdo#112080]) -> [PASS][76] +12 similar issues
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb6/igt@perf_pmu@busy-check-all-vcs1.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb2/igt@perf_pmu@busy-check-all-vcs1.html

  
#### Warnings ####

  * igt@gem_ctx_isolation@vcs1-nonpriv:
    - shard-iclb:         [SKIP][77] ([fdo#112080]) -> [FAIL][78] ([IGT#28]) +1 similar issue
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-iclb3/igt@gem_ctx_isolation@vcs1-nonpriv.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-iclb1/igt@gem_ctx_isolation@vcs1-nonpriv.html

  * igt@gem_tiled_blits@interruptible:
    - shard-hsw:          [FAIL][79] ([i915#818]) -> [FAIL][80] ([i915#694])
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-hsw8/igt@gem_tiled_blits@interruptible.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-hsw8/igt@gem_tiled_blits@interruptible.html

  * igt@gem_tiled_blits@normal:
    - shard-hsw:          [FAIL][81] ([i915#694]) -> [FAIL][82] ([i915#818])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-hsw1/igt@gem_tiled_blits@normal.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-hsw7/igt@gem_tiled_blits@normal.html

  * igt@i915_pm_dc@dc6-psr:
    - shard-tglb:         [SKIP][83] ([i915#468]) -> [FAIL][84] ([i915#454])
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-tglb2/igt@i915_pm_dc@dc6-psr.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-tglb1/igt@i915_pm_dc@dc6-psr.html

  * igt@i915_pm_rc6_residency@media-rc6-accuracy:
    - shard-tglb:         [SKIP][85] ([fdo#109289] / [fdo#111719]) -> [SKIP][86] ([fdo#111719])
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7939/shard-tglb1/igt@i915_pm_rc6_residency@media-rc6-accuracy.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/shard-tglb7/igt@i915_pm_rc6_residency@media-rc6-accuracy.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [IGT#28]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/28
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#111719]: https://bugs.freedesktop.org/show_bug.cgi?id=111719
  [fdo#112080]: https://bugs.freedesktop.org/show_bug.cgi?id=112080
  [fdo#112146]: https://bugs.freedesktop.org/show_bug.cgi?id=112146
  [i915#173]: https://gitlab.freedesktop.org/drm/intel/issues/173
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#250]: https://gitlab.freedesktop.org/drm/intel/issues/250
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
  [i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#468]: https://gitlab.freedesktop.org/drm/intel/issues/468
  [i915#570]: https://gitlab.freedesktop.org/drm/intel/issues/570
  [i915#668]: https://gitlab.freedesktop.org/drm/intel/issues/668
  [i915#677]: https://gitlab.freedesktop.org/drm/intel/issues/677
  [i915#679]: https://gitlab.freedesktop.org/drm/intel/issues/679
  [i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
  [i915#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#818]: https://gitlab.freedesktop.org/drm/intel/issues/818


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7939 -> Patchwork_16569

  CI-20190529: 20190529
  CI_DRM_7939: cceb0c30a34af6ca96e35211ecdc5ca198d44e7e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5441: 534ca091fe4ffed916752165bc5becd7ff56cd84 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16569: 2bb6ef6acc00a99d609b52064cf9baa76bb79004 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16569/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-09 15:45 ` Jani Nikula
@ 2020-02-09 15:50   ` Andi Shyti
  0 siblings, 0 replies; 22+ messages in thread
From: Andi Shyti @ 2020-02-09 15:50 UTC (permalink / raw)
  To: Jani Nikula; +Cc: Intel GFX

Hi Jani,

> >  void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915)
> >  {
> > @@ -321,6 +322,7 @@ void intel_gt_driver_register(struct intel_gt *gt)
> >  	intel_rps_driver_register(&gt->rps);
> >  
> >  	debugfs_gt_register(gt);
> 
> Yikes, when did this happen? Not good. We don't own the debugfs
> "namespace" prefix.
> 
> > +	sysfs_gt_register(gt);
> 
> Ditto for the sysfs namespace prefix.
> 
> I guess it's not likely debugfs or sysfs would add functions named like
> that, but if they did, they'd be right to call these names a violation
> of their namespace.
> 
> I have been promoting the idea of naming functions somewhat based on the
> name of the file they reside in... so do we want to name the files like
> this either?

Yes, you're right here. I will rename the "debugfs" function and
fix the sysfs in my next patch.

Thanks,
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-08 12:27 [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Andi Shyti
  2020-02-08 16:26 ` Chris Wilson
@ 2020-02-09 15:45 ` Jani Nikula
  2020-02-09 15:50   ` Andi Shyti
  1 sibling, 1 reply; 22+ messages in thread
From: Jani Nikula @ 2020-02-09 15:45 UTC (permalink / raw)
  To: Andi Shyti, Intel GFX

On Sat, 08 Feb 2020, Andi Shyti <andi@etezian.org> wrote:
> From: Andi Shyti <andi.shyti@intel.com>
>
> The GT has its own properties and in sysfs they should be grouped
> in the 'gt/' directory.
>
> Create the 'gt/' directory in sysfs and move the power management
> related files.
>
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile            |   4 +-
>  drivers/gpu/drm/i915/gt/intel_gt.c       |   3 +
>  drivers/gpu/drm/i915/gt/intel_gt_types.h |   1 +
>  drivers/gpu/drm/i915/gt/sysfs_gt.c       |  66 ++++
>  drivers/gpu/drm/i915/gt/sysfs_gt.h       |  15 +
>  drivers/gpu/drm/i915/gt/sysfs_gt_pm.c    | 431 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/gt/sysfs_gt_pm.h    |  17 +
>  drivers/gpu/drm/i915/i915_sysfs.c        | 373 --------------------
>  8 files changed, 536 insertions(+), 374 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt.c
>  create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt.h
>  create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
>  create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 49eed50ef0a4..3b81c8a96c06 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -106,7 +106,9 @@ gt-y += \
>  	gt/intel_rps.o \
>  	gt/intel_sseu.o \
>  	gt/intel_timeline.o \
> -	gt/intel_workarounds.o
> +	gt/intel_workarounds.o \
> +	gt/sysfs_gt.o \
> +	gt/sysfs_gt_pm.o
>  # autogenerated null render state
>  gt-y += \
>  	gt/gen6_renderstate.o \
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index f1f1b306e0af..8c360db14320 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -15,6 +15,7 @@
>  #include "intel_rps.h"
>  #include "intel_uncore.h"
>  #include "intel_pm.h"
> +#include "sysfs_gt.h"
>  
>  void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915)
>  {
> @@ -321,6 +322,7 @@ void intel_gt_driver_register(struct intel_gt *gt)
>  	intel_rps_driver_register(&gt->rps);
>  
>  	debugfs_gt_register(gt);

Yikes, when did this happen? Not good. We don't own the debugfs
"namespace" prefix.

> +	sysfs_gt_register(gt);

Ditto for the sysfs namespace prefix.

I guess it's not likely debugfs or sysfs would add functions named like
that, but if they did, they'd be right to call these names a violation
of their namespace.

I have been promoting the idea of naming functions somewhat based on the
name of the file they reside in... so do we want to name the files like
this either?

BR,
Jani.


>  }
>  
>  static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
> @@ -641,6 +643,7 @@ void intel_gt_driver_remove(struct intel_gt *gt)
>  
>  void intel_gt_driver_unregister(struct intel_gt *gt)
>  {
> +	sysfs_gt_unregister(gt);
>  	intel_rps_driver_unregister(&gt->rps);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> index 96890dd12b5f..cdf659a7c74f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -32,6 +32,7 @@ struct intel_gt {
>  	struct drm_i915_private *i915;
>  	struct intel_uncore *uncore;
>  	struct i915_ggtt *ggtt;
> +	struct kobject *kobj;
>  
>  	struct intel_uc uc;
>  
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt.c b/drivers/gpu/drm/i915/gt/sysfs_gt.c
> new file mode 100644
> index 000000000000..4eda2ae144a0
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: MIT
> +
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include <linux/sysfs.h>
> +#include <drm/drm_device.h>
> +#include <linux/kobject.h>
> +#include <linux/printk.h>
> +
> +#include "../i915_drv.h"
> +#include "intel_gt.h"
> +#include "intel_gt_types.h"
> +#include "intel_rc6.h"
> +
> +#include "sysfs_gt_pm.h"
> +
> +static ssize_t gt_info_show(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buff)
> +{
> +	return snprintf(buff, PAGE_SIZE, "0\n");
> +}
> +
> +static DEVICE_ATTR_RO(gt_info);
> +
> +static struct attribute *gt_attrs[] = {
> +	&dev_attr_gt_info.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group gt_attribute_group = {
> +	.attrs = gt_attrs,
> +};
> +
> +void sysfs_gt_register(struct intel_gt *gt)
> +{
> +	struct device *dev;
> +	int ret;
> +
> +	gt->kobj = kobject_create_and_add("gt",
> +			kobject_get(&gt->i915->drm.primary->kdev->kobj));
> +	if (!gt->kobj) {
> +		pr_err("failed to initialize sysfs file\n");
> +		return;
> +	}
> +
> +	dev = kobj_to_dev(gt->kobj);
> +	dev_set_drvdata(dev, gt);
> +
> +	ret = sysfs_create_group(gt->kobj, &gt_attribute_group);
> +	if (ret)
> +		pr_err("failed to create sysfs gt info files\n");
> +
> +	intel_sysfs_pm_init(gt, gt->kobj);
> +}
> +
> +void sysfs_gt_unregister(struct intel_gt *gt)
> +{
> +	if (!gt->kobj)
> +		return;
> +
> +	intel_sysfs_pm_remove(gt, gt->kobj);
> +	sysfs_remove_group(gt->kobj, &gt_attribute_group);
> +}
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt.h b/drivers/gpu/drm/i915/gt/sysfs_gt.h
> new file mode 100644
> index 000000000000..07638dde6e28
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef SYSFS_GT_H
> +#define SYSFS_GT_H
> +
> +struct intel_gt;
> +
> +void sysfs_gt_register(struct intel_gt *gt);
> +void sysfs_gt_unregister(struct intel_gt *gt);
> +
> +#endif /* SYSFS_GT_H */
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
> new file mode 100644
> index 000000000000..999c100e8239
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
> @@ -0,0 +1,431 @@
> +// SPDX-License-Identifier: MIT
> +
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include <linux/sysfs.h>
> +#include <drm/drm_device.h>
> +#include <linux/printk.h>
> +
> +#include "../i915_drv.h"
> +#include "intel_gt.h"
> +#include "intel_gt_types.h"
> +#include "intel_rc6.h"
> +#include "intel_rps.h"
> +#include "sysfs_gt_pm.h"
> +
> +#ifdef CONFIG_PM
> +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_features_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	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 snprintf(buff, PAGE_SIZE, "%x\n", mask);
> +}
> +
> +static ssize_t rc6_residency_ms_show(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	u32 rc6_residency = get_residency(gt, GEN6_GT_GFX_RC6);
> +
> +	return snprintf(buff, PAGE_SIZE, "%u\n", rc6_residency);
> +}
> +
> +static ssize_t rc6p_residency_ms_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	u32 rc6p_residency = get_residency(gt, GEN6_GT_GFX_RC6p);
> +
> +	return snprintf(buff, PAGE_SIZE, "%u\n", rc6p_residency);
> +}
> +
> +static ssize_t rc6pp_residency_ms_show(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	u32 rc6pp_residency = get_residency(gt, GEN6_GT_GFX_RC6pp);
> +
> +	return snprintf(buff, PAGE_SIZE, "%u\n", rc6pp_residency);
> +}
> +
> +static ssize_t media_rc6_residency_ms_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	u32 rc6_residency = get_residency(gt, VLV_GT_MEDIA_RC6);
> +
> +	return snprintf(buff, PAGE_SIZE, "%u\n", rc6_residency);
> +}
> +
> +static DEVICE_ATTR_RO(rc6_features);
> +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_features.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_attribute_group = {
> +	.attrs = rc6_attrs,
> +};
> +
> +static const struct attribute_group rc6p_attribute_group = {
> +	.attrs = rc6p_attrs,
> +};
> +
> +static const struct attribute_group media_rc6_attribute_group = {
> +	.attrs = media_rc6_attrs,
> +};
> +
> +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	int ret = 0;
> +
> +	if (HAS_RC6(gt->i915)) {
> +		ret = sysfs_create_group(kobj, &rc6_attribute_group);
> +		if (ret)
> +			pr_err("failed to create RC6 sysfs files\n");
> +	}
> +
> +	if (HAS_RC6p(gt->i915)) {
> +		ret = sysfs_merge_group(kobj, &rc6p_attribute_group);
> +		if (ret)
> +			pr_err("failed to create RC6p sysfs files\n");
> +	}
> +
> +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) {
> +		ret = sysfs_merge_group(kobj, &media_rc6_attribute_group);
> +		if (ret)
> +			pr_err("failed to create media RC6 sysfs files\n");
> +	}
> +}
> +#else
> +static int intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static ssize_t gt_act_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr, char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_rps_read_actual_frequency(&gt->rps));
> +}
> +
> +static ssize_t gt_cur_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr, char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->cur_freq));
> +}
> +
> +static ssize_t gt_boost_freq_mhz_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->boost_freq));
> +}
> +
> +static ssize_t gt_boost_freq_mhz_store(struct device *dev,
> +				       struct device_attribute *attr,
> +				       const char *buff, size_t count)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	bool boost = false;
> +	ssize_t ret;
> +	u32 val;
> +
> +	ret = kstrtou32(buff, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Validate against (static) hardware limits */
> +	val = intel_freq_opcode(rps, val);
> +	if (val < rps->min_freq || val > rps->max_freq)
> +		return -EINVAL;
> +
> +	mutex_lock(&rps->lock);
> +	if (val != rps->boost_freq) {
> +		rps->boost_freq = val;
> +		boost = atomic_read(&rps->num_waiters);
> +	}
> +	mutex_unlock(&rps->lock);
> +	if (boost)
> +		schedule_work(&rps->work);
> +
> +	return count;
> +}
> +
> +static ssize_t vlv_rpe_freq_mhz_show(struct device *dev,
> +				     struct device_attribute *attr, char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->efficient_freq));
> +}
> +
> +static ssize_t gt_max_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->max_freq_softlimit));
> +}
> +
> +static ssize_t gt_max_freq_mhz_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buff, size_t count)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	ssize_t ret;
> +	u32 val;
> +
> +	ret = kstrtou32(buff, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&rps->lock);
> +
> +	val = intel_freq_opcode(rps, val);
> +	if (val < rps->min_freq ||
> +	    val > rps->max_freq ||
> +	    val < rps->min_freq_softlimit) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	if (val > rps->rp0_freq)
> +		DRM_DEBUG("User requested overclocking to %d\n",
> +			  intel_gpu_freq(rps, val));
> +
> +	rps->max_freq_softlimit = val;
> +
> +	val = clamp_t(int, rps->cur_freq,
> +		      rps->min_freq_softlimit,
> +		      rps->max_freq_softlimit);
> +
> +	/*
> +	 * We still need *_set_rps to process the new max_delay and
> +	 * update the interrupt limits and PMINTRMSK even though
> +	 * frequency request may be unchanged.
> +	 */
> +	intel_rps_set(rps, val);
> +
> +unlock:
> +	mutex_unlock(&rps->lock);
> +
> +	return ret ?: count;
> +}
> +
> +static ssize_t gt_min_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->min_freq_softlimit));
> +}
> +
> +static ssize_t gt_min_freq_mhz_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buff, size_t count)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	ssize_t ret;
> +	u32 val;
> +
> +	ret = kstrtou32(buff, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&rps->lock);
> +
> +	val = intel_freq_opcode(rps, val);
> +	if (val < rps->min_freq ||
> +	    val > rps->max_freq ||
> +	    val > rps->max_freq_softlimit) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	rps->min_freq_softlimit = val;
> +
> +	val = clamp_t(int, rps->cur_freq,
> +		      rps->min_freq_softlimit,
> +		      rps->max_freq_softlimit);
> +
> +	/*
> +	 * We still need *_set_rps to process the new min_delay and
> +	 * update the interrupt limits and PMINTRMSK even though
> +	 * frequency request may be unchanged.
> +	 */
> +	intel_rps_set(rps, val);
> +
> +unlock:
> +	mutex_unlock(&rps->lock);
> +
> +	return ret ?: count;
> +}
> +
> +static DEVICE_ATTR_RO(gt_act_freq_mhz);
> +static DEVICE_ATTR_RO(gt_cur_freq_mhz);
> +static DEVICE_ATTR_RW(gt_boost_freq_mhz);
> +static DEVICE_ATTR_RW(gt_max_freq_mhz);
> +static DEVICE_ATTR_RW(gt_min_freq_mhz);
> +
> +static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
> +
> +static ssize_t gt_rp_mhz_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buff);
> +
> +static DEVICE_ATTR(gt_RP0_freq_mhz, 0444, gt_rp_mhz_show, NULL);
> +static DEVICE_ATTR(gt_RP1_freq_mhz, 0444, gt_rp_mhz_show, NULL);
> +static DEVICE_ATTR(gt_RPn_freq_mhz, 0444, gt_rp_mhz_show, NULL);
> +
> +/* For now we have a static number of RP states */
> +static ssize_t gt_rp_mhz_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buff)
> +{
> +	struct intel_gt *gt = dev_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	u32 val;
> +
> +	if (attr == &dev_attr_gt_RP0_freq_mhz)
> +		val = intel_gpu_freq(rps, rps->rp0_freq);
> +	else if (attr == &dev_attr_gt_RP1_freq_mhz)
> +		val = intel_gpu_freq(rps, rps->rp1_freq);
> +	else if (attr == &dev_attr_gt_RPn_freq_mhz)
> +		val = intel_gpu_freq(rps, rps->min_freq);
> +	else
> +		BUG();
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n", val);
> +}
> +
> +static const struct attribute * const gen6_attrs[] = {
> +	&dev_attr_gt_act_freq_mhz.attr,
> +	&dev_attr_gt_cur_freq_mhz.attr,
> +	&dev_attr_gt_boost_freq_mhz.attr,
> +	&dev_attr_gt_max_freq_mhz.attr,
> +	&dev_attr_gt_min_freq_mhz.attr,
> +	&dev_attr_gt_RP0_freq_mhz.attr,
> +	&dev_attr_gt_RP1_freq_mhz.attr,
> +	&dev_attr_gt_RPn_freq_mhz.attr,
> +	NULL,
> +};
> +
> +static const struct attribute * const vlv_attrs[] = {
> +	&dev_attr_gt_act_freq_mhz.attr,
> +	&dev_attr_gt_cur_freq_mhz.attr,
> +	&dev_attr_gt_boost_freq_mhz.attr,
> +	&dev_attr_gt_max_freq_mhz.attr,
> +	&dev_attr_gt_min_freq_mhz.attr,
> +	&dev_attr_gt_RP0_freq_mhz.attr,
> +	&dev_attr_gt_RP1_freq_mhz.attr,
> +	&dev_attr_gt_RPn_freq_mhz.attr,
> +	&dev_attr_vlv_rpe_freq_mhz.attr,
> +	NULL,
> +};
> +
> +static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
> +		return sysfs_create_files(kobj, vlv_attrs);
> +
> +	if (INTEL_GEN(gt->i915) >= 6)
> +		return sysfs_create_files(kobj, gen6_attrs);
> +
> +	return 0;
> +}
> +
> +void intel_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	int ret;
> +
> +	intel_sysfs_rc6_init(gt, kobj);
> +
> +	ret = intel_sysfs_rps_init(gt, kobj);
> +	if (ret)
> +		pr_err("failed to create RPS sysfs files");
> +}
> +
> +void intel_sysfs_pm_remove(struct intel_gt *gt, struct kobject *kobj)
> +{
> +#ifdef CONFIG_PM
> +	if (HAS_RC6p(gt->i915))
> +		sysfs_unmerge_group(kobj, &rc6p_attribute_group);
> +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
> +		sysfs_unmerge_group(kobj, &media_rc6_attribute_group);
> +	if (HAS_RC6(gt->i915))
> +		sysfs_remove_group(kobj, &rc6_attribute_group);
> +#endif
> +
> +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
> +		sysfs_remove_files(kobj, vlv_attrs);
> +	else
> +		sysfs_remove_files(kobj, gen6_attrs);
> +}
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
> new file mode 100644
> index 000000000000..daa90fa166c5
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef SYSFS_RC6_H
> +#define SYSFS_RC6_H
> +
> +#include <linux/kobject.h>
> +
> +#include "intel_gt_types.h"
> +
> +void intel_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj);
> +void intel_sysfs_pm_remove(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 c14d762bd652..3214e07460d5 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -44,107 +44,6 @@ static inline 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(&dev_priv->gt.rc6, reg);
> -
> -	return DIV_ROUND_CLOSEST_ULL(res, 1000);
> -}
> -
> -static ssize_t
> -show_rc6_mask(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 snprintf(buf, PAGE_SIZE, "%x\n", mask);
> -}
> -
> -static ssize_t
> -show_rc6_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
> -}
> -
> -static ssize_t
> -show_rc6p_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6p_residency);
> -}
> -
> -static ssize_t
> -show_rc6pp_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6pp_residency);
> -}
> -
> -static ssize_t
> -show_media_rc6_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
> -}
> -
> -static DEVICE_ATTR(rc6_enable, S_IRUGO, show_rc6_mask, NULL);
> -static DEVICE_ATTR(rc6_residency_ms, S_IRUGO, show_rc6_ms, NULL);
> -static DEVICE_ATTR(rc6p_residency_ms, S_IRUGO, show_rc6p_ms, NULL);
> -static DEVICE_ATTR(rc6pp_residency_ms, S_IRUGO, show_rc6pp_ms, NULL);
> -static DEVICE_ATTR(media_rc6_residency_ms, S_IRUGO, show_media_rc6_ms, NULL);
> -
> -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))
> @@ -256,239 +155,6 @@ static const struct bin_attribute dpf_attrs_1 = {
>  	.private = (void *)1
>  };
>  
> -static ssize_t gt_act_freq_mhz_show(struct device *kdev,
> -				    struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &i915->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_rps_read_actual_frequency(rps));
> -}
> -
> -static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> -				    struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &i915->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->cur_freq));
> -}
> -
> -static ssize_t gt_boost_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &i915->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->boost_freq));
> -}
> -
> -static ssize_t gt_boost_freq_mhz_store(struct device *kdev,
> -				       struct device_attribute *attr,
> -				       const char *buf, size_t count)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	bool boost = false;
> -	ssize_t ret;
> -	u32 val;
> -
> -	ret = kstrtou32(buf, 0, &val);
> -	if (ret)
> -		return ret;
> -
> -	/* Validate against (static) hardware limits */
> -	val = intel_freq_opcode(rps, val);
> -	if (val < rps->min_freq || val > rps->max_freq)
> -		return -EINVAL;
> -
> -	mutex_lock(&rps->lock);
> -	if (val != rps->boost_freq) {
> -		rps->boost_freq = val;
> -		boost = atomic_read(&rps->num_waiters);
> -	}
> -	mutex_unlock(&rps->lock);
> -	if (boost)
> -		schedule_work(&rps->work);
> -
> -	return count;
> -}
> -
> -static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
> -				     struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->efficient_freq));
> -}
> -
> -static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->max_freq_softlimit));
> -}
> -
> -static ssize_t gt_max_freq_mhz_store(struct device *kdev,
> -				     struct device_attribute *attr,
> -				     const char *buf, size_t count)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	ssize_t ret;
> -	u32 val;
> -
> -	ret = kstrtou32(buf, 0, &val);
> -	if (ret)
> -		return ret;
> -
> -	mutex_lock(&rps->lock);
> -
> -	val = intel_freq_opcode(rps, val);
> -	if (val < rps->min_freq ||
> -	    val > rps->max_freq ||
> -	    val < rps->min_freq_softlimit) {
> -		ret = -EINVAL;
> -		goto unlock;
> -	}
> -
> -	if (val > rps->rp0_freq)
> -		DRM_DEBUG("User requested overclocking to %d\n",
> -			  intel_gpu_freq(rps, val));
> -
> -	rps->max_freq_softlimit = val;
> -
> -	val = clamp_t(int, rps->cur_freq,
> -		      rps->min_freq_softlimit,
> -		      rps->max_freq_softlimit);
> -
> -	/*
> -	 * We still need *_set_rps to process the new max_delay and
> -	 * update the interrupt limits and PMINTRMSK even though
> -	 * frequency request may be unchanged.
> -	 */
> -	intel_rps_set(rps, val);
> -
> -unlock:
> -	mutex_unlock(&rps->lock);
> -
> -	return ret ?: count;
> -}
> -
> -static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->min_freq_softlimit));
> -}
> -
> -static ssize_t gt_min_freq_mhz_store(struct device *kdev,
> -				     struct device_attribute *attr,
> -				     const char *buf, size_t count)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	ssize_t ret;
> -	u32 val;
> -
> -	ret = kstrtou32(buf, 0, &val);
> -	if (ret)
> -		return ret;
> -
> -	mutex_lock(&rps->lock);
> -
> -	val = intel_freq_opcode(rps, val);
> -	if (val < rps->min_freq ||
> -	    val > rps->max_freq ||
> -	    val > rps->max_freq_softlimit) {
> -		ret = -EINVAL;
> -		goto unlock;
> -	}
> -
> -	rps->min_freq_softlimit = val;
> -
> -	val = clamp_t(int, rps->cur_freq,
> -		      rps->min_freq_softlimit,
> -		      rps->max_freq_softlimit);
> -
> -	/*
> -	 * We still need *_set_rps to process the new min_delay and
> -	 * update the interrupt limits and PMINTRMSK even though
> -	 * frequency request may be unchanged.
> -	 */
> -	intel_rps_set(rps, val);
> -
> -unlock:
> -	mutex_unlock(&rps->lock);
> -
> -	return ret ?: count;
> -}
> -
> -static DEVICE_ATTR_RO(gt_act_freq_mhz);
> -static DEVICE_ATTR_RO(gt_cur_freq_mhz);
> -static DEVICE_ATTR_RW(gt_boost_freq_mhz);
> -static DEVICE_ATTR_RW(gt_max_freq_mhz);
> -static DEVICE_ATTR_RW(gt_min_freq_mhz);
> -
> -static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
> -
> -static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf);
> -static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> -static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> -static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> -
> -/* For now we have a static number of RP states */
> -static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	u32 val;
> -
> -	if (attr == &dev_attr_gt_RP0_freq_mhz)
> -		val = intel_gpu_freq(rps, rps->rp0_freq);
> -	else if (attr == &dev_attr_gt_RP1_freq_mhz)
> -		val = intel_gpu_freq(rps, rps->rp1_freq);
> -	else if (attr == &dev_attr_gt_RPn_freq_mhz)
> -		val = intel_gpu_freq(rps, rps->min_freq);
> -	else
> -		BUG();
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n", val);
> -}
> -
> -static const struct attribute * const gen6_attrs[] = {
> -	&dev_attr_gt_act_freq_mhz.attr,
> -	&dev_attr_gt_cur_freq_mhz.attr,
> -	&dev_attr_gt_boost_freq_mhz.attr,
> -	&dev_attr_gt_max_freq_mhz.attr,
> -	&dev_attr_gt_min_freq_mhz.attr,
> -	&dev_attr_gt_RP0_freq_mhz.attr,
> -	&dev_attr_gt_RP1_freq_mhz.attr,
> -	&dev_attr_gt_RPn_freq_mhz.attr,
> -	NULL,
> -};
> -
> -static const struct attribute * const vlv_attrs[] = {
> -	&dev_attr_gt_act_freq_mhz.attr,
> -	&dev_attr_gt_cur_freq_mhz.attr,
> -	&dev_attr_gt_boost_freq_mhz.attr,
> -	&dev_attr_gt_max_freq_mhz.attr,
> -	&dev_attr_gt_min_freq_mhz.attr,
> -	&dev_attr_gt_RP0_freq_mhz.attr,
> -	&dev_attr_gt_RP1_freq_mhz.attr,
> -	&dev_attr_gt_RPn_freq_mhz.attr,
> -	&dev_attr_vlv_rpe_freq_mhz.attr,
> -	NULL,
> -};
> -
>  #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
>  
>  static ssize_t error_state_read(struct file *filp, struct kobject *kobj,
> @@ -559,29 +225,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)
> @@ -597,14 +240,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
>  		}
>  	}
>  
> -	ret = 0;
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		ret = sysfs_create_files(&kdev->kobj, vlv_attrs);
> -	else if (INTEL_GEN(dev_priv) >= 6)
> -		ret = sysfs_create_files(&kdev->kobj, gen6_attrs);
> -	if (ret)
> -		drm_err(&dev_priv->drm, "RPS sysfs setup failed\n");
> -
>  	i915_setup_error_capture(kdev);
>  }
>  
> @@ -614,14 +249,6 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
>  
>  	i915_teardown_error_capture(kdev);
>  
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		sysfs_remove_files(&kdev->kobj, vlv_attrs);
> -	else
> -		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
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-08 17:06         ` Chris Wilson
@ 2020-02-08 17:23           ` Andi Shyti
  0 siblings, 0 replies; 22+ messages in thread
From: Andi Shyti @ 2020-02-08 17:23 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Intel GFX

> > > > > > The GT has its own properties and in sysfs they should be grouped
> > > > > > in the 'gt/' directory.
> > > > > > 
> > > > > > Create the 'gt/' directory in sysfs and move the power management
> > > > > > related files.
> > > > > 
> > > > > As shown by BAT, we have a conundrum; do we have to preserve the old
> > > > > names forevermore? Or do we just userspace that they have to adapt?
> > > > 
> > > > With this, I guess you are suggesting to change igt accordingly,
> > > > because I'm pretty sure this interface is subject to change,
> > > > sooner or later.
> > > 
> > > Right; but what about powertop? And the bajillion wikis?
> > 
> > mmhhh... right!
> > 
> > > Just feels clumsy, so I'd like to spend a moment to see if we can think
> > > of any options before dropping them. No matter how silly, if userspace
> > > breaks, it's a regression :(
> > > 
> > > Maybe we could do something like keep the old ones and put a deprecation
> > > warning in?
> > 
> > how about linking and declare the old interface obsolete?
> 
> Is a possibility, but I'm not keen on having more and more dangling
> symlinks.
> 
> 1. Do nothing; have redundant files.
> 2. Remove them, expect complaints.
> 3. Mark them as deprecated, remove in 5.9?
> 4. Symlinks forevermore (not clear if we can symlink and emit a warning)

my symlinks suggestions was a mix of the above and it would be:

 1. add symlinks (or redundant files with warning) and mark the
    interfaces as deprecated.
 2. in 5.9 remove the symlinks (or redundant files, I like the
    idea of warning userspace).
 3. expect anyway complaints :-P

Let's give it a few days, perhaps after the weekend someone might
have an opinion or better recommendation.

Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-08 17:01       ` Andi Shyti
@ 2020-02-08 17:06         ` Chris Wilson
  2020-02-08 17:23           ` Andi Shyti
  0 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2020-02-08 17:06 UTC (permalink / raw)
  To: Andi Shyti; +Cc: Intel GFX

Quoting Andi Shyti (2020-02-08 17:01:53)
> Hi Chris,
> 
> > > > > The GT has its own properties and in sysfs they should be grouped
> > > > > in the 'gt/' directory.
> > > > > 
> > > > > Create the 'gt/' directory in sysfs and move the power management
> > > > > related files.
> > > > 
> > > > As shown by BAT, we have a conundrum; do we have to preserve the old
> > > > names forevermore? Or do we just userspace that they have to adapt?
> > > 
> > > With this, I guess you are suggesting to change igt accordingly,
> > > because I'm pretty sure this interface is subject to change,
> > > sooner or later.
> > 
> > Right; but what about powertop? And the bajillion wikis?
> 
> mmhhh... right!
> 
> > Just feels clumsy, so I'd like to spend a moment to see if we can think
> > of any options before dropping them. No matter how silly, if userspace
> > breaks, it's a regression :(
> > 
> > Maybe we could do something like keep the old ones and put a deprecation
> > warning in?
> 
> how about linking and declare the old interface obsolete?

Is a possibility, but I'm not keen on having more and more dangling
symlinks.

1. Do nothing; have redundant files.
2. Remove them, expect complaints.
3. Mark them as deprecated, remove in 5.9?
4. Symlinks forevermore (not clear if we can symlink and emit a warning)
5. Profit?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-08 16:57     ` Chris Wilson
@ 2020-02-08 17:01       ` Andi Shyti
  2020-02-08 17:06         ` Chris Wilson
  0 siblings, 1 reply; 22+ messages in thread
From: Andi Shyti @ 2020-02-08 17:01 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Intel GFX

Hi Chris,

> > > > The GT has its own properties and in sysfs they should be grouped
> > > > in the 'gt/' directory.
> > > > 
> > > > Create the 'gt/' directory in sysfs and move the power management
> > > > related files.
> > > 
> > > As shown by BAT, we have a conundrum; do we have to preserve the old
> > > names forevermore? Or do we just userspace that they have to adapt?
> > 
> > With this, I guess you are suggesting to change igt accordingly,
> > because I'm pretty sure this interface is subject to change,
> > sooner or later.
> 
> Right; but what about powertop? And the bajillion wikis?

mmhhh... right!

> Just feels clumsy, so I'd like to spend a moment to see if we can think
> of any options before dropping them. No matter how silly, if userspace
> breaks, it's a regression :(
> 
> Maybe we could do something like keep the old ones and put a deprecation
> warning in?

how about linking and declare the old interface obsolete?

Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-08 16:51   ` Andi Shyti
@ 2020-02-08 16:57     ` Chris Wilson
  2020-02-08 17:01       ` Andi Shyti
  0 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2020-02-08 16:57 UTC (permalink / raw)
  To: Andi Shyti; +Cc: Intel GFX

Quoting Andi Shyti (2020-02-08 16:51:39)
> On Sat, Feb 08, 2020 at 04:26:18PM +0000, Chris Wilson wrote:
> > Quoting Andi Shyti (2020-02-08 12:27:59)
> > > From: Andi Shyti <andi.shyti@intel.com>
> > > 
> > > The GT has its own properties and in sysfs they should be grouped
> > > in the 'gt/' directory.
> > > 
> > > Create the 'gt/' directory in sysfs and move the power management
> > > related files.
> > 
> > As shown by BAT, we have a conundrum; do we have to preserve the old
> > names forevermore? Or do we just userspace that they have to adapt?
> 
> With this, I guess you are suggesting to change igt accordingly,
> because I'm pretty sure this interface is subject to change,
> sooner or later.

Right; but what about powertop? And the bajillion wikis?

Just feels clumsy, so I'd like to spend a moment to see if we can think
of any options before dropping them. No matter how silly, if userspace
breaks, it's a regression :(

Maybe we could do something like keep the old ones and put a deprecation
warning in?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-08 16:26 ` Chris Wilson
@ 2020-02-08 16:51   ` Andi Shyti
  2020-02-08 16:57     ` Chris Wilson
  0 siblings, 1 reply; 22+ messages in thread
From: Andi Shyti @ 2020-02-08 16:51 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Intel GFX

On Sat, Feb 08, 2020 at 04:26:18PM +0000, Chris Wilson wrote:
> Quoting Andi Shyti (2020-02-08 12:27:59)
> > From: Andi Shyti <andi.shyti@intel.com>
> > 
> > The GT has its own properties and in sysfs they should be grouped
> > in the 'gt/' directory.
> > 
> > Create the 'gt/' directory in sysfs and move the power management
> > related files.
> 
> As shown by BAT, we have a conundrum; do we have to preserve the old
> names forevermore? Or do we just userspace that they have to adapt?

With this, I guess you are suggesting to change igt accordingly,
because I'm pretty sure this interface is subject to change,
sooner or later.

Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
  2020-02-08 12:27 [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Andi Shyti
@ 2020-02-08 16:26 ` Chris Wilson
  2020-02-08 16:51   ` Andi Shyti
  2020-02-09 15:45 ` Jani Nikula
  1 sibling, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2020-02-08 16:26 UTC (permalink / raw)
  To: Andi Shyti, Intel GFX

Quoting Andi Shyti (2020-02-08 12:27:59)
> From: Andi Shyti <andi.shyti@intel.com>
> 
> The GT has its own properties and in sysfs they should be grouped
> in the 'gt/' directory.
> 
> Create the 'gt/' directory in sysfs and move the power management
> related files.

As shown by BAT, we have a conundrum; do we have to preserve the old
names forevermore? Or do we just userspace that they have to adapt?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
@ 2020-02-08 12:27 Andi Shyti
  2020-02-08 16:26 ` Chris Wilson
  2020-02-09 15:45 ` Jani Nikula
  0 siblings, 2 replies; 22+ messages in thread
From: Andi Shyti @ 2020-02-08 12:27 UTC (permalink / raw)
  To: Intel GFX

From: Andi Shyti <andi.shyti@intel.com>

The GT has its own properties and in sysfs they should be grouped
in the 'gt/' directory.

Create the 'gt/' directory in sysfs and move the power management
related files.

Signed-off-by: Andi Shyti <andi.shyti@intel.com>
---
 drivers/gpu/drm/i915/Makefile            |   4 +-
 drivers/gpu/drm/i915/gt/intel_gt.c       |   3 +
 drivers/gpu/drm/i915/gt/intel_gt_types.h |   1 +
 drivers/gpu/drm/i915/gt/sysfs_gt.c       |  66 ++++
 drivers/gpu/drm/i915/gt/sysfs_gt.h       |  15 +
 drivers/gpu/drm/i915/gt/sysfs_gt_pm.c    | 431 +++++++++++++++++++++++
 drivers/gpu/drm/i915/gt/sysfs_gt_pm.h    |  17 +
 drivers/gpu/drm/i915/i915_sysfs.c        | 373 --------------------
 8 files changed, 536 insertions(+), 374 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt.c
 create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt.h
 create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
 create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt_pm.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 49eed50ef0a4..3b81c8a96c06 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -106,7 +106,9 @@ gt-y += \
 	gt/intel_rps.o \
 	gt/intel_sseu.o \
 	gt/intel_timeline.o \
-	gt/intel_workarounds.o
+	gt/intel_workarounds.o \
+	gt/sysfs_gt.o \
+	gt/sysfs_gt_pm.o
 # autogenerated null render state
 gt-y += \
 	gt/gen6_renderstate.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index f1f1b306e0af..8c360db14320 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -15,6 +15,7 @@
 #include "intel_rps.h"
 #include "intel_uncore.h"
 #include "intel_pm.h"
+#include "sysfs_gt.h"
 
 void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915)
 {
@@ -321,6 +322,7 @@ void intel_gt_driver_register(struct intel_gt *gt)
 	intel_rps_driver_register(&gt->rps);
 
 	debugfs_gt_register(gt);
+	sysfs_gt_register(gt);
 }
 
 static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
@@ -641,6 +643,7 @@ void intel_gt_driver_remove(struct intel_gt *gt)
 
 void intel_gt_driver_unregister(struct intel_gt *gt)
 {
+	sysfs_gt_unregister(gt);
 	intel_rps_driver_unregister(&gt->rps);
 }
 
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index 96890dd12b5f..cdf659a7c74f 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
@@ -32,6 +32,7 @@ struct intel_gt {
 	struct drm_i915_private *i915;
 	struct intel_uncore *uncore;
 	struct i915_ggtt *ggtt;
+	struct kobject *kobj;
 
 	struct intel_uc uc;
 
diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt.c b/drivers/gpu/drm/i915/gt/sysfs_gt.c
new file mode 100644
index 000000000000..4eda2ae144a0
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: MIT
+
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include <linux/sysfs.h>
+#include <drm/drm_device.h>
+#include <linux/kobject.h>
+#include <linux/printk.h>
+
+#include "../i915_drv.h"
+#include "intel_gt.h"
+#include "intel_gt_types.h"
+#include "intel_rc6.h"
+
+#include "sysfs_gt_pm.h"
+
+static ssize_t gt_info_show(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buff)
+{
+	return snprintf(buff, PAGE_SIZE, "0\n");
+}
+
+static DEVICE_ATTR_RO(gt_info);
+
+static struct attribute *gt_attrs[] = {
+	&dev_attr_gt_info.attr,
+	NULL
+};
+
+static const struct attribute_group gt_attribute_group = {
+	.attrs = gt_attrs,
+};
+
+void sysfs_gt_register(struct intel_gt *gt)
+{
+	struct device *dev;
+	int ret;
+
+	gt->kobj = kobject_create_and_add("gt",
+			kobject_get(&gt->i915->drm.primary->kdev->kobj));
+	if (!gt->kobj) {
+		pr_err("failed to initialize sysfs file\n");
+		return;
+	}
+
+	dev = kobj_to_dev(gt->kobj);
+	dev_set_drvdata(dev, gt);
+
+	ret = sysfs_create_group(gt->kobj, &gt_attribute_group);
+	if (ret)
+		pr_err("failed to create sysfs gt info files\n");
+
+	intel_sysfs_pm_init(gt, gt->kobj);
+}
+
+void sysfs_gt_unregister(struct intel_gt *gt)
+{
+	if (!gt->kobj)
+		return;
+
+	intel_sysfs_pm_remove(gt, gt->kobj);
+	sysfs_remove_group(gt->kobj, &gt_attribute_group);
+}
diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt.h b/drivers/gpu/drm/i915/gt/sysfs_gt.h
new file mode 100644
index 000000000000..07638dde6e28
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef SYSFS_GT_H
+#define SYSFS_GT_H
+
+struct intel_gt;
+
+void sysfs_gt_register(struct intel_gt *gt);
+void sysfs_gt_unregister(struct intel_gt *gt);
+
+#endif /* SYSFS_GT_H */
diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
new file mode 100644
index 000000000000..999c100e8239
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
@@ -0,0 +1,431 @@
+// SPDX-License-Identifier: MIT
+
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include <linux/sysfs.h>
+#include <drm/drm_device.h>
+#include <linux/printk.h>
+
+#include "../i915_drv.h"
+#include "intel_gt.h"
+#include "intel_gt_types.h"
+#include "intel_rc6.h"
+#include "intel_rps.h"
+#include "sysfs_gt_pm.h"
+
+#ifdef CONFIG_PM
+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_features_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	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 snprintf(buff, PAGE_SIZE, "%x\n", mask);
+}
+
+static ssize_t rc6_residency_ms_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	u32 rc6_residency = get_residency(gt, GEN6_GT_GFX_RC6);
+
+	return snprintf(buff, PAGE_SIZE, "%u\n", rc6_residency);
+}
+
+static ssize_t rc6p_residency_ms_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	u32 rc6p_residency = get_residency(gt, GEN6_GT_GFX_RC6p);
+
+	return snprintf(buff, PAGE_SIZE, "%u\n", rc6p_residency);
+}
+
+static ssize_t rc6pp_residency_ms_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	u32 rc6pp_residency = get_residency(gt, GEN6_GT_GFX_RC6pp);
+
+	return snprintf(buff, PAGE_SIZE, "%u\n", rc6pp_residency);
+}
+
+static ssize_t media_rc6_residency_ms_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	u32 rc6_residency = get_residency(gt, VLV_GT_MEDIA_RC6);
+
+	return snprintf(buff, PAGE_SIZE, "%u\n", rc6_residency);
+}
+
+static DEVICE_ATTR_RO(rc6_features);
+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_features.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_attribute_group = {
+	.attrs = rc6_attrs,
+};
+
+static const struct attribute_group rc6p_attribute_group = {
+	.attrs = rc6p_attrs,
+};
+
+static const struct attribute_group media_rc6_attribute_group = {
+	.attrs = media_rc6_attrs,
+};
+
+static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
+{
+	int ret = 0;
+
+	if (HAS_RC6(gt->i915)) {
+		ret = sysfs_create_group(kobj, &rc6_attribute_group);
+		if (ret)
+			pr_err("failed to create RC6 sysfs files\n");
+	}
+
+	if (HAS_RC6p(gt->i915)) {
+		ret = sysfs_merge_group(kobj, &rc6p_attribute_group);
+		if (ret)
+			pr_err("failed to create RC6p sysfs files\n");
+	}
+
+	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) {
+		ret = sysfs_merge_group(kobj, &media_rc6_attribute_group);
+		if (ret)
+			pr_err("failed to create media RC6 sysfs files\n");
+	}
+}
+#else
+static int intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
+{
+	return 0;
+}
+#endif /* CONFIG_PM */
+
+static ssize_t gt_act_freq_mhz_show(struct device *dev,
+				    struct device_attribute *attr, char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_rps_read_actual_frequency(&gt->rps));
+}
+
+static ssize_t gt_cur_freq_mhz_show(struct device *dev,
+				    struct device_attribute *attr, char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(rps, rps->cur_freq));
+}
+
+static ssize_t gt_boost_freq_mhz_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(rps, rps->boost_freq));
+}
+
+static ssize_t gt_boost_freq_mhz_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buff, size_t count)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+	bool boost = false;
+	ssize_t ret;
+	u32 val;
+
+	ret = kstrtou32(buff, 0, &val);
+	if (ret)
+		return ret;
+
+	/* Validate against (static) hardware limits */
+	val = intel_freq_opcode(rps, val);
+	if (val < rps->min_freq || val > rps->max_freq)
+		return -EINVAL;
+
+	mutex_lock(&rps->lock);
+	if (val != rps->boost_freq) {
+		rps->boost_freq = val;
+		boost = atomic_read(&rps->num_waiters);
+	}
+	mutex_unlock(&rps->lock);
+	if (boost)
+		schedule_work(&rps->work);
+
+	return count;
+}
+
+static ssize_t vlv_rpe_freq_mhz_show(struct device *dev,
+				     struct device_attribute *attr, char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(rps, rps->efficient_freq));
+}
+
+static ssize_t gt_max_freq_mhz_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(rps, rps->max_freq_softlimit));
+}
+
+static ssize_t gt_max_freq_mhz_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buff, size_t count)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+	ssize_t ret;
+	u32 val;
+
+	ret = kstrtou32(buff, 0, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&rps->lock);
+
+	val = intel_freq_opcode(rps, val);
+	if (val < rps->min_freq ||
+	    val > rps->max_freq ||
+	    val < rps->min_freq_softlimit) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	if (val > rps->rp0_freq)
+		DRM_DEBUG("User requested overclocking to %d\n",
+			  intel_gpu_freq(rps, val));
+
+	rps->max_freq_softlimit = val;
+
+	val = clamp_t(int, rps->cur_freq,
+		      rps->min_freq_softlimit,
+		      rps->max_freq_softlimit);
+
+	/*
+	 * We still need *_set_rps to process the new max_delay and
+	 * update the interrupt limits and PMINTRMSK even though
+	 * frequency request may be unchanged.
+	 */
+	intel_rps_set(rps, val);
+
+unlock:
+	mutex_unlock(&rps->lock);
+
+	return ret ?: count;
+}
+
+static ssize_t gt_min_freq_mhz_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+
+	return snprintf(buff, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(rps, rps->min_freq_softlimit));
+}
+
+static ssize_t gt_min_freq_mhz_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buff, size_t count)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+	ssize_t ret;
+	u32 val;
+
+	ret = kstrtou32(buff, 0, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&rps->lock);
+
+	val = intel_freq_opcode(rps, val);
+	if (val < rps->min_freq ||
+	    val > rps->max_freq ||
+	    val > rps->max_freq_softlimit) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	rps->min_freq_softlimit = val;
+
+	val = clamp_t(int, rps->cur_freq,
+		      rps->min_freq_softlimit,
+		      rps->max_freq_softlimit);
+
+	/*
+	 * We still need *_set_rps to process the new min_delay and
+	 * update the interrupt limits and PMINTRMSK even though
+	 * frequency request may be unchanged.
+	 */
+	intel_rps_set(rps, val);
+
+unlock:
+	mutex_unlock(&rps->lock);
+
+	return ret ?: count;
+}
+
+static DEVICE_ATTR_RO(gt_act_freq_mhz);
+static DEVICE_ATTR_RO(gt_cur_freq_mhz);
+static DEVICE_ATTR_RW(gt_boost_freq_mhz);
+static DEVICE_ATTR_RW(gt_max_freq_mhz);
+static DEVICE_ATTR_RW(gt_min_freq_mhz);
+
+static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
+
+static ssize_t gt_rp_mhz_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buff);
+
+static DEVICE_ATTR(gt_RP0_freq_mhz, 0444, gt_rp_mhz_show, NULL);
+static DEVICE_ATTR(gt_RP1_freq_mhz, 0444, gt_rp_mhz_show, NULL);
+static DEVICE_ATTR(gt_RPn_freq_mhz, 0444, gt_rp_mhz_show, NULL);
+
+/* For now we have a static number of RP states */
+static ssize_t gt_rp_mhz_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buff)
+{
+	struct intel_gt *gt = dev_get_drvdata(dev);
+	struct intel_rps *rps = &gt->rps;
+	u32 val;
+
+	if (attr == &dev_attr_gt_RP0_freq_mhz)
+		val = intel_gpu_freq(rps, rps->rp0_freq);
+	else if (attr == &dev_attr_gt_RP1_freq_mhz)
+		val = intel_gpu_freq(rps, rps->rp1_freq);
+	else if (attr == &dev_attr_gt_RPn_freq_mhz)
+		val = intel_gpu_freq(rps, rps->min_freq);
+	else
+		BUG();
+
+	return snprintf(buff, PAGE_SIZE, "%d\n", val);
+}
+
+static const struct attribute * const gen6_attrs[] = {
+	&dev_attr_gt_act_freq_mhz.attr,
+	&dev_attr_gt_cur_freq_mhz.attr,
+	&dev_attr_gt_boost_freq_mhz.attr,
+	&dev_attr_gt_max_freq_mhz.attr,
+	&dev_attr_gt_min_freq_mhz.attr,
+	&dev_attr_gt_RP0_freq_mhz.attr,
+	&dev_attr_gt_RP1_freq_mhz.attr,
+	&dev_attr_gt_RPn_freq_mhz.attr,
+	NULL,
+};
+
+static const struct attribute * const vlv_attrs[] = {
+	&dev_attr_gt_act_freq_mhz.attr,
+	&dev_attr_gt_cur_freq_mhz.attr,
+	&dev_attr_gt_boost_freq_mhz.attr,
+	&dev_attr_gt_max_freq_mhz.attr,
+	&dev_attr_gt_min_freq_mhz.attr,
+	&dev_attr_gt_RP0_freq_mhz.attr,
+	&dev_attr_gt_RP1_freq_mhz.attr,
+	&dev_attr_gt_RPn_freq_mhz.attr,
+	&dev_attr_vlv_rpe_freq_mhz.attr,
+	NULL,
+};
+
+static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj)
+{
+	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
+		return sysfs_create_files(kobj, vlv_attrs);
+
+	if (INTEL_GEN(gt->i915) >= 6)
+		return sysfs_create_files(kobj, gen6_attrs);
+
+	return 0;
+}
+
+void intel_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
+{
+	int ret;
+
+	intel_sysfs_rc6_init(gt, kobj);
+
+	ret = intel_sysfs_rps_init(gt, kobj);
+	if (ret)
+		pr_err("failed to create RPS sysfs files");
+}
+
+void intel_sysfs_pm_remove(struct intel_gt *gt, struct kobject *kobj)
+{
+#ifdef CONFIG_PM
+	if (HAS_RC6p(gt->i915))
+		sysfs_unmerge_group(kobj, &rc6p_attribute_group);
+	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
+		sysfs_unmerge_group(kobj, &media_rc6_attribute_group);
+	if (HAS_RC6(gt->i915))
+		sysfs_remove_group(kobj, &rc6_attribute_group);
+#endif
+
+	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
+		sysfs_remove_files(kobj, vlv_attrs);
+	else
+		sysfs_remove_files(kobj, gen6_attrs);
+}
diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
new file mode 100644
index 000000000000..daa90fa166c5
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef SYSFS_RC6_H
+#define SYSFS_RC6_H
+
+#include <linux/kobject.h>
+
+#include "intel_gt_types.h"
+
+void intel_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj);
+void intel_sysfs_pm_remove(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 c14d762bd652..3214e07460d5 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -44,107 +44,6 @@ static inline 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(&dev_priv->gt.rc6, reg);
-
-	return DIV_ROUND_CLOSEST_ULL(res, 1000);
-}
-
-static ssize_t
-show_rc6_mask(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 snprintf(buf, PAGE_SIZE, "%x\n", mask);
-}
-
-static ssize_t
-show_rc6_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
-}
-
-static ssize_t
-show_rc6p_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6p_residency);
-}
-
-static ssize_t
-show_rc6pp_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6pp_residency);
-}
-
-static ssize_t
-show_media_rc6_ms(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 snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
-}
-
-static DEVICE_ATTR(rc6_enable, S_IRUGO, show_rc6_mask, NULL);
-static DEVICE_ATTR(rc6_residency_ms, S_IRUGO, show_rc6_ms, NULL);
-static DEVICE_ATTR(rc6p_residency_ms, S_IRUGO, show_rc6p_ms, NULL);
-static DEVICE_ATTR(rc6pp_residency_ms, S_IRUGO, show_rc6pp_ms, NULL);
-static DEVICE_ATTR(media_rc6_residency_ms, S_IRUGO, show_media_rc6_ms, NULL);
-
-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))
@@ -256,239 +155,6 @@ static const struct bin_attribute dpf_attrs_1 = {
 	.private = (void *)1
 };
 
-static ssize_t gt_act_freq_mhz_show(struct device *kdev,
-				    struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &i915->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_rps_read_actual_frequency(rps));
-}
-
-static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
-				    struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &i915->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_gpu_freq(rps, rps->cur_freq));
-}
-
-static ssize_t gt_boost_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &i915->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_gpu_freq(rps, rps->boost_freq));
-}
-
-static ssize_t gt_boost_freq_mhz_store(struct device *kdev,
-				       struct device_attribute *attr,
-				       const char *buf, size_t count)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-	bool boost = false;
-	ssize_t ret;
-	u32 val;
-
-	ret = kstrtou32(buf, 0, &val);
-	if (ret)
-		return ret;
-
-	/* Validate against (static) hardware limits */
-	val = intel_freq_opcode(rps, val);
-	if (val < rps->min_freq || val > rps->max_freq)
-		return -EINVAL;
-
-	mutex_lock(&rps->lock);
-	if (val != rps->boost_freq) {
-		rps->boost_freq = val;
-		boost = atomic_read(&rps->num_waiters);
-	}
-	mutex_unlock(&rps->lock);
-	if (boost)
-		schedule_work(&rps->work);
-
-	return count;
-}
-
-static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
-				     struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_gpu_freq(rps, rps->efficient_freq));
-}
-
-static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_gpu_freq(rps, rps->max_freq_softlimit));
-}
-
-static ssize_t gt_max_freq_mhz_store(struct device *kdev,
-				     struct device_attribute *attr,
-				     const char *buf, size_t count)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-	ssize_t ret;
-	u32 val;
-
-	ret = kstrtou32(buf, 0, &val);
-	if (ret)
-		return ret;
-
-	mutex_lock(&rps->lock);
-
-	val = intel_freq_opcode(rps, val);
-	if (val < rps->min_freq ||
-	    val > rps->max_freq ||
-	    val < rps->min_freq_softlimit) {
-		ret = -EINVAL;
-		goto unlock;
-	}
-
-	if (val > rps->rp0_freq)
-		DRM_DEBUG("User requested overclocking to %d\n",
-			  intel_gpu_freq(rps, val));
-
-	rps->max_freq_softlimit = val;
-
-	val = clamp_t(int, rps->cur_freq,
-		      rps->min_freq_softlimit,
-		      rps->max_freq_softlimit);
-
-	/*
-	 * We still need *_set_rps to process the new max_delay and
-	 * update the interrupt limits and PMINTRMSK even though
-	 * frequency request may be unchanged.
-	 */
-	intel_rps_set(rps, val);
-
-unlock:
-	mutex_unlock(&rps->lock);
-
-	return ret ?: count;
-}
-
-static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			intel_gpu_freq(rps, rps->min_freq_softlimit));
-}
-
-static ssize_t gt_min_freq_mhz_store(struct device *kdev,
-				     struct device_attribute *attr,
-				     const char *buf, size_t count)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-	ssize_t ret;
-	u32 val;
-
-	ret = kstrtou32(buf, 0, &val);
-	if (ret)
-		return ret;
-
-	mutex_lock(&rps->lock);
-
-	val = intel_freq_opcode(rps, val);
-	if (val < rps->min_freq ||
-	    val > rps->max_freq ||
-	    val > rps->max_freq_softlimit) {
-		ret = -EINVAL;
-		goto unlock;
-	}
-
-	rps->min_freq_softlimit = val;
-
-	val = clamp_t(int, rps->cur_freq,
-		      rps->min_freq_softlimit,
-		      rps->max_freq_softlimit);
-
-	/*
-	 * We still need *_set_rps to process the new min_delay and
-	 * update the interrupt limits and PMINTRMSK even though
-	 * frequency request may be unchanged.
-	 */
-	intel_rps_set(rps, val);
-
-unlock:
-	mutex_unlock(&rps->lock);
-
-	return ret ?: count;
-}
-
-static DEVICE_ATTR_RO(gt_act_freq_mhz);
-static DEVICE_ATTR_RO(gt_cur_freq_mhz);
-static DEVICE_ATTR_RW(gt_boost_freq_mhz);
-static DEVICE_ATTR_RW(gt_max_freq_mhz);
-static DEVICE_ATTR_RW(gt_min_freq_mhz);
-
-static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
-
-static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf);
-static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
-static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
-static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
-
-/* For now we have a static number of RP states */
-static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
-{
-	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
-	struct intel_rps *rps = &dev_priv->gt.rps;
-	u32 val;
-
-	if (attr == &dev_attr_gt_RP0_freq_mhz)
-		val = intel_gpu_freq(rps, rps->rp0_freq);
-	else if (attr == &dev_attr_gt_RP1_freq_mhz)
-		val = intel_gpu_freq(rps, rps->rp1_freq);
-	else if (attr == &dev_attr_gt_RPn_freq_mhz)
-		val = intel_gpu_freq(rps, rps->min_freq);
-	else
-		BUG();
-
-	return snprintf(buf, PAGE_SIZE, "%d\n", val);
-}
-
-static const struct attribute * const gen6_attrs[] = {
-	&dev_attr_gt_act_freq_mhz.attr,
-	&dev_attr_gt_cur_freq_mhz.attr,
-	&dev_attr_gt_boost_freq_mhz.attr,
-	&dev_attr_gt_max_freq_mhz.attr,
-	&dev_attr_gt_min_freq_mhz.attr,
-	&dev_attr_gt_RP0_freq_mhz.attr,
-	&dev_attr_gt_RP1_freq_mhz.attr,
-	&dev_attr_gt_RPn_freq_mhz.attr,
-	NULL,
-};
-
-static const struct attribute * const vlv_attrs[] = {
-	&dev_attr_gt_act_freq_mhz.attr,
-	&dev_attr_gt_cur_freq_mhz.attr,
-	&dev_attr_gt_boost_freq_mhz.attr,
-	&dev_attr_gt_max_freq_mhz.attr,
-	&dev_attr_gt_min_freq_mhz.attr,
-	&dev_attr_gt_RP0_freq_mhz.attr,
-	&dev_attr_gt_RP1_freq_mhz.attr,
-	&dev_attr_gt_RPn_freq_mhz.attr,
-	&dev_attr_vlv_rpe_freq_mhz.attr,
-	NULL,
-};
-
 #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
 
 static ssize_t error_state_read(struct file *filp, struct kobject *kobj,
@@ -559,29 +225,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)
@@ -597,14 +240,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
 		}
 	}
 
-	ret = 0;
-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		ret = sysfs_create_files(&kdev->kobj, vlv_attrs);
-	else if (INTEL_GEN(dev_priv) >= 6)
-		ret = sysfs_create_files(&kdev->kobj, gen6_attrs);
-	if (ret)
-		drm_err(&dev_priv->drm, "RPS sysfs setup failed\n");
-
 	i915_setup_error_capture(kdev);
 }
 
@@ -614,14 +249,6 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
 
 	i915_teardown_error_capture(kdev);
 
-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		sysfs_remove_files(&kdev->kobj, vlv_attrs);
-	else
-		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
 }
-- 
2.25.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, back to index

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-14 11:03 [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Andi Shyti
2020-02-14 12:51 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/gt: make a gt sysfs group and move power management files (rev3) Patchwork
2020-02-14 12:52 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2020-02-14 12:54 ` [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Tvrtko Ursulin
2020-02-14 13:16   ` Andi Shyti
2020-02-14 13:38     ` Tvrtko Ursulin
2020-02-14 13:57       ` Andi Shyti
2020-02-14 13:18   ` Chris Wilson
2020-02-14 13:41     ` Tvrtko Ursulin
2020-02-14 13:13 ` [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/gt: make a gt sysfs group and move power management files (rev3) Patchwork
2020-02-14 13:24   ` Chris Wilson
2020-02-14 13:14 ` [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Chris Wilson
2020-02-17 19:04 ` [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/gt: make a gt sysfs group and move power management files (rev3) Patchwork
  -- strict thread matches above, loose matches on Subject: below --
2020-02-08 12:27 [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files Andi Shyti
2020-02-08 16:26 ` Chris Wilson
2020-02-08 16:51   ` Andi Shyti
2020-02-08 16:57     ` Chris Wilson
2020-02-08 17:01       ` Andi Shyti
2020-02-08 17:06         ` Chris Wilson
2020-02-08 17:23           ` Andi Shyti
2020-02-09 15:45 ` Jani Nikula
2020-02-09 15:50   ` Andi Shyti

Intel-GFX Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/intel-gfx/0 intel-gfx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 intel-gfx intel-gfx/ https://lore.kernel.org/intel-gfx \
		intel-gfx@lists.freedesktop.org
	public-inbox-index intel-gfx

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.intel-gfx


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git