intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / 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; 13+ 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 related	[flat|nested] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ messages in thread

end of thread, other threads:[~2020-02-17 19:04 UTC | newest]

Thread overview: 13+ 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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).