[PATCH 1/3] drm/i915: add rc6 residency times to debugfs

[PATCH 1/3] drm/i915: add rc6 residency times to debugfs

[Ben Widawsky]

RC6 residency should be in intervals of 1.28us, and the counter wraps.
Here is an example using awk to get the various RC6 and RC6+ residency
times in seconds, since boot.

cat /sys/kernel/debug/dri/0/i915_drpc_info  | grep residency | awk -F':' -F' '  '{print $5 * 1.28 / 1000000}'

This is primarily for QA, but has other applications as well. An
upcoming patch to add interfaces should be more interesting to
application developers.

CC: Ouping Zhang <ouping.zhang@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   11 +++++++++++
 drivers/gpu/drm/i915/i915_reg.h     |    5 +++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 66c90d4..72457ff 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1133,6 +1133,17 @@ static int gen6_drpc_info(struct seq_file *m)
 
 	seq_printf(m, "Core Power Down: %s\n",
 		   yesno(gt_core_status & GEN6_CORE_CPD_STATE_MASK));
+
+	seq_printf(m, "RC6 \"Locked to RPn\" residency since boot: %d\n",
+		   I915_READ(GEN6_GT_GFX_RC6_LOCKED));
+	/* Not exactly sure what this is */
+	seq_printf(m, "RC6 residency since boot: %d\n",
+		   I915_READ(GEN6_GT_GFX_RC6));
+	seq_printf(m, "RC6+ residency since boot: %d\n",
+		   I915_READ(GEN6_GT_GFX_RC6p));
+	seq_printf(m, "RC6++ residency since boot: %d\n",
+		   I915_READ(GEN6_GT_GFX_RC6pp));
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f3609f2..b1c3d35 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3799,6 +3799,11 @@
 						 GEN6_PM_RP_DOWN_THRESHOLD | \
 						 GEN6_PM_RP_DOWN_TIMEOUT)
 
+#define GEN6_GT_GFX_RC6_LOCKED			0x138104
+#define GEN6_GT_GFX_RC6				0x138108
+#define GEN6_GT_GFX_RC6p			0x13810C
+#define GEN6_GT_GFX_RC6pp			0x138110
+
 #define GEN6_PCODE_MAILBOX			0x138124
 #define   GEN6_PCODE_READY			(1<<31)
 #define   GEN6_READ_OC_PARAMS			0xc
-- 
1.7.9.4

[PATCH 2/3] drm/i915: extract intel_enable_rc6()

[Ben Widawsky]

nice to have elsewhere

CC: Eugeni Dodonov <eugeni.dodonov@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bbbc1a4..22ab4db 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1414,6 +1414,7 @@ extern void ironlake_enable_rc6(struct drm_device *dev);
 extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void intel_detect_pch(struct drm_device *dev);
 extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
+extern int intel_enable_rc6(const struct drm_device *dev);
 
 extern void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
 extern void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ab62c96..efbf709 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8235,7 +8235,7 @@ void intel_init_emon(struct drm_device *dev)
 	dev_priv->corr = (lcfuse & LCFUSE_HIV_MASK);
 }
 
-static int intel_enable_rc6(struct drm_device *dev)
+int intel_enable_rc6(const struct drm_device *dev)
 {
 	/*
 	 * Respect the kernel parameter if it is set
-- 
1.7.9.4

[PATCH 3/3] drm/i915: rc6 in sysfs

[Ben Widawsky]

Merge rc6 information into the power group for our device. Until now the
i915 driver has not had any sysfs entries (aside from the connector
stuff enabled by libdrm). Since it seems like we're likely to have more
in the future I created a new file for sysfs stubs, as well as the rc6
sysfs functions which don't really belong elsewhere (perhaps
i915_suspend, but most of the stuff is in intel_display,c).

displays #ms GPU has been in rc6 since boot:
cat /sys/class/drm/card0/power/rc6

displays #ms GPU has been in deep rc6 since boot:
cat /sys/class/drm/card0/power/rc6p

displays #ms GPU has been in deepest rc6 since boot:
cat /sys/class/drm/card0/power/rc6pp

Important note: I've seen on SNB that even when RC6 is *not* enabled the
rc6 register seems to have a random value in it. I cannot explain a
reason for this. Those writing tools that utilize this value need to be
careful and probably want to scrutinize the value very carefully.

Please see intel-gpu-tools patches for sample code.

CC: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/Makefile       |    1 +
 drivers/gpu/drm/i915/i915_debugfs.c |    2 +-
 drivers/gpu/drm/i915/i915_dma.c     |    4 ++
 drivers/gpu/drm/i915/i915_drv.h     |    4 ++
 drivers/gpu/drm/i915/i915_sysfs.c   |  106 +++++++++++++++++++++++++++++++++++
 5 files changed, 116 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/i915_sysfs.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index ce7fc77..f801330 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -12,6 +12,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
 	  i915_gem_execbuffer.o \
 	  i915_gem_gtt.o \
 	  i915_gem_tiling.o \
+	  i915_sysfs.o \
 	  i915_trace_points.o \
 	  intel_display.o \
 	  intel_crt.o \
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 72457ff..4257151 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1134,9 +1134,9 @@ static int gen6_drpc_info(struct seq_file *m)
 	seq_printf(m, "Core Power Down: %s\n",
 		   yesno(gt_core_status & GEN6_CORE_CPD_STATE_MASK));
 
+	/* Not exactly sure what this is */
 	seq_printf(m, "RC6 \"Locked to RPn\" residency since boot: %d\n",
 		   I915_READ(GEN6_GT_GFX_RC6_LOCKED));
-	/* Not exactly sure what this is */
 	seq_printf(m, "RC6 residency since boot: %d\n",
 		   I915_READ(GEN6_GT_GFX_RC6));
 	seq_printf(m, "RC6+ residency since boot: %d\n",
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index fdff009..64dfbb8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2113,6 +2113,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		}
 	}
 
+	i915_setup_sysfs(dev);
+
 	/* Must be done after probing outputs */
 	intel_opregion_init(dev);
 	acpi_video_register();
@@ -2164,6 +2166,8 @@ int i915_driver_unload(struct drm_device *dev)
 	i915_mch_dev = NULL;
 	spin_unlock(&mchdev_lock);
 
+	i915_teardown_sysfs(dev);
+
 	if (dev_priv->mm.inactive_shrinker.shrink)
 		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 22ab4db..e267774 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1365,6 +1365,10 @@ extern int i915_restore_state(struct drm_device *dev);
 extern int i915_save_state(struct drm_device *dev);
 extern int i915_restore_state(struct drm_device *dev);
 
+/* i915_sysfs.c */
+void i915_setup_sysfs(struct drm_device *dev_priv);
+void i915_teardown_sysfs(struct drm_device *dev_priv);
+
 /* intel_i2c.c */
 extern int intel_setup_gmbus(struct drm_device *dev);
 extern void intel_teardown_gmbus(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
new file mode 100644
index 0000000..a18a955f
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -0,0 +1,106 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/stat.h>
+#include <linux/sysfs.h>
+#include "i915_drv.h"
+
+static u32 calc_residency(struct drm_device *dev, const u32 reg)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u64 raw_time;
+	u32 residency;
+
+	if (!intel_enable_rc6(dev))
+		return 0;
+
+	raw_time = I915_READ(reg) * 128ULL;
+	residency = DIV_ROUND_CLOSEST(raw_time, 1000) / 100;
+	return residency;
+}
+
+static ssize_t
+show_rc6pp_ms(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+	u32 rc6pp_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6pp);
+	return snprintf(buf, PAGE_SIZE, "%u", rc6pp_residency);
+}
+
+static ssize_t
+show_rc6_ms(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+	struct drm_i915_private *dev_priv = dminor->dev->dev_private;
+	/* 32b value may overflow during fixed point math */
+	u64 time = (I915_READ(GEN6_GT_GFX_RC6) * 128);
+	u32 rc6_residency = DIV_ROUND_CLOSEST(time, 1000) / 100;
+	return snprintf(buf, PAGE_SIZE, "%u", rc6_residency);
+}
+
+static ssize_t
+show_rc6p_ms(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+	struct drm_i915_private *dev_priv = dminor->dev->dev_private;
+	/* 32b value may overflow during fixed point math */
+	u64 time = (I915_READ(GEN6_GT_GFX_RC6p) * 128);
+	u32 rc6p_residency = DIV_ROUND_CLOSEST(time, 1000) / 100;
+	return snprintf(buf, PAGE_SIZE, "%u", rc6p_residency);
+}
+
+static DEVICE_ATTR(rc6, S_IRUGO, show_rc6_ms, NULL);
+static DEVICE_ATTR(rc6p, S_IRUGO, show_rc6p_ms, NULL);
+static DEVICE_ATTR(rc6pp, S_IRUGO, show_rc6pp_ms, NULL);
+
+static struct attribute *rc6_attrs[] = {
+	&dev_attr_rc6.attr,
+	&dev_attr_rc6p.attr,
+	&dev_attr_rc6pp.attr,
+	NULL
+};
+
+static struct attribute_group rc6_attr_group = {
+	.name = power_group_name,
+	.attrs =  rc6_attrs
+};
+
+void i915_setup_sysfs(struct drm_device *dev)
+{
+	int ret;
+
+	ret = sysfs_merge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
+	if (ret)
+		DRM_ERROR("sysfs setup failed\n");
+}
+
+void i915_teardown_sysfs(struct drm_device *dev)
+{
+	sysfs_remove_group(&dev->primary->kdev.kobj, &rc6_attr_group);
+}
-- 
1.7.9.4

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

[PATCH 1/2] build: make sure we have asprintf

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 configure.ac |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/configure.ac b/configure.ac
index f778a9a..567f10d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -32,6 +32,7 @@ AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_AUX_DIR([build-aux])
 AC_USE_SYSTEM_EXTENSIONS
 AC_SYS_LARGEFILE
+AC_GNU_SOURCE
 
 AM_INIT_AUTOMAKE([foreign dist-bzip2])
 AM_PATH_PYTHON([3],, [:])
@@ -43,6 +44,7 @@ AC_CHECK_MEMBERS([struct sysinfo.totalram],[],[],[AC_INCLUDES_DEFAULT
 #include <sys/sysinfo.h>
 ])
 AC_CHECK_FUNCS([swapctl])
+AC_CHECK_FUNCS([asprintf])
 
 # Initialize libtool
 AC_DISABLE_STATIC
-- 
1.7.9.4

[PATCH 2/2] tests: rc6 residency test

[Ben Widawsky]

This is meant to test the sysfs entry for showing rc6 residency in
milliseconds. Remember, sysfs is a permanent interface.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 tests/Makefile.am     |    1 +
 tests/rc6_residency.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)
 create mode 100644 tests/rc6_residency.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 6544ec7..a8eed88 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -56,6 +56,7 @@ TESTS_progs = \
 	drm_vma_limiter_cpu \
 	drm_vma_limiter_gtt \
 	drm_vma_limiter_cached \
+	rc6_residency \
 	$(NULL)
 
 # IMPORTANT: The ZZ_ tests need to be run last!
diff --git a/tests/rc6_residency.c b/tests/rc6_residency.c
new file mode 100644
index 0000000..8ae6aa4
--- /dev/null
+++ b/tests/rc6_residency.c
@@ -0,0 +1,80 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *
+ */
+
+#include <stdlib.h>
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include "drmtest.h"
+
+static unsigned int readit(const char *path)
+{
+	unsigned int ret;
+
+	FILE *file;
+	file = fopen(path, "r");
+	if (file == NULL) {
+		fprintf(stderr, "Couldn't open %s (%d)\n", path, errno);
+		abort();
+	}
+	fscanf(file, "%u", &ret);
+	fclose(file);
+
+	return ret;
+}
+
+int main(int argc, char *argv[])
+{
+	/* TODO: don't always assume 0 */
+	const int device = 0;
+	char *path;
+	int fd, ret;
+	unsigned int value, value2;
+
+	/* Use drm_open_any to verify device existence */
+	fd = drm_open_any();
+	close(fd);
+
+	ret = asprintf(&path, "/sys/class/drm/card%d/power/rc6", device);
+	if (ret == -1 || ret < strlen("/sys/class/drm/card0/power/rc6"))
+		abort();
+
+	value = readit(path);
+
+	// Sleep for 3 seconds and compare
+	sleep(3);
+	value2 = readit(path);
+	free(path);
+
+	/* Plenty of fudge */
+	if (((value2 - value) > 3900) ||
+	    ((value2 - value) < 2100))
+		exit(EXIT_FAILURE);
+
+	exit(EXIT_SUCCESS);
+}
-- 
1.7.9.4

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

Re: [PATCH 3/3] drm/i915: rc6 in sysfs

[Ben Widawsky]

I sent stale patches, but this is the only one with a big effect. See
inline for what's fixed, and feel free to review the rest.

On Sat, 24 Mar 2012 19:09:46 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> Merge rc6 information into the power group for our device. Until now
> the i915 driver has not had any sysfs entries (aside from the
> connector stuff enabled by libdrm). Since it seems like we're likely
> to have more in the future I created a new file for sysfs stubs, as
> well as the rc6 sysfs functions which don't really belong elsewhere
> (perhaps i915_suspend, but most of the stuff is in intel_display,c).
> 
> displays #ms GPU has been in rc6 since boot:
> cat /sys/class/drm/card0/power/rc6
> 
> displays #ms GPU has been in deep rc6 since boot:
> cat /sys/class/drm/card0/power/rc6p
> 
> displays #ms GPU has been in deepest rc6 since boot:
> cat /sys/class/drm/card0/power/rc6pp
> 
> Important note: I've seen on SNB that even when RC6 is *not* enabled
> the rc6 register seems to have a random value in it. I cannot explain
> a reason for this. Those writing tools that utilize this value need
> to be careful and probably want to scrutinize the value very
> carefully.
> 
> Please see intel-gpu-tools patches for sample code.
> 
> CC: Arjan van de Ven <arjan@linux.intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/Makefile       |    1 +
>  drivers/gpu/drm/i915/i915_debugfs.c |    2 +-
>  drivers/gpu/drm/i915/i915_dma.c     |    4 ++
>  drivers/gpu/drm/i915/i915_drv.h     |    4 ++
>  drivers/gpu/drm/i915/i915_sysfs.c   |  106
> +++++++++++++++++++++++++++++++++++ 5 files changed, 116
> insertions(+), 1 deletion(-) create mode 100644
> drivers/gpu/drm/i915/i915_sysfs.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile
> b/drivers/gpu/drm/i915/Makefile index ce7fc77..f801330 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -12,6 +12,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
>  	  i915_gem_execbuffer.o \
>  	  i915_gem_gtt.o \
>  	  i915_gem_tiling.o \
> +	  i915_sysfs.o \
>  	  i915_trace_points.o \
>  	  intel_display.o \
>  	  intel_crt.o \
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c index 72457ff..4257151 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1134,9 +1134,9 @@ static int gen6_drpc_info(struct seq_file *m)
>  	seq_printf(m, "Core Power Down: %s\n",
>  		   yesno(gt_core_status & GEN6_CORE_CPD_STATE_MASK));
>  
> +	/* Not exactly sure what this is */
>  	seq_printf(m, "RC6 \"Locked to RPn\" residency since boot:
> %d\n", I915_READ(GEN6_GT_GFX_RC6_LOCKED));
> -	/* Not exactly sure what this is */
>  	seq_printf(m, "RC6 residency since boot: %d\n",
>  		   I915_READ(GEN6_GT_GFX_RC6));
>  	seq_printf(m, "RC6+ residency since boot: %d\n",

This hunk should be rebased on patch 1... my bad. I'll fix after
waiting on some review.

> diff --git a/drivers/gpu/drm/i915/i915_dma.c
> b/drivers/gpu/drm/i915/i915_dma.c index fdff009..64dfbb8 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -2113,6 +2113,8 @@ int i915_driver_load(struct drm_device *dev,
> unsigned long flags) }
>  	}
>  
> +	i915_setup_sysfs(dev);
> +
>  	/* Must be done after probing outputs */
>  	intel_opregion_init(dev);
>  	acpi_video_register();
> @@ -2164,6 +2166,8 @@ int i915_driver_unload(struct drm_device *dev)
>  	i915_mch_dev = NULL;
>  	spin_unlock(&mchdev_lock);
>  
> +	i915_teardown_sysfs(dev);
> +
>  	if (dev_priv->mm.inactive_shrinker.shrink)
>  		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h index 22ab4db..e267774 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1365,6 +1365,10 @@ extern int i915_restore_state(struct
> drm_device *dev); extern int i915_save_state(struct drm_device *dev);
>  extern int i915_restore_state(struct drm_device *dev);
>  
> +/* i915_sysfs.c */
> +void i915_setup_sysfs(struct drm_device *dev_priv);
> +void i915_teardown_sysfs(struct drm_device *dev_priv);
> +
>  /* intel_i2c.c */
>  extern int intel_setup_gmbus(struct drm_device *dev);
>  extern void intel_teardown_gmbus(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
> b/drivers/gpu/drm/i915/i915_sysfs.c new file mode 100644
> index 0000000..a18a955f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -0,0 +1,106 @@
> +/*
> + * Copyright © 2012 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> the
> + * Software is furnished to do so, subject to the following
> conditions:
> + *
> + * The above copyright notice and this permission notice (including
> the next
> + * paragraph) shall be included in all copies or substantial
> portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Ben Widawsky <ben@bwidawsk.net>
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/stat.h>
> +#include <linux/sysfs.h>
> +#include "i915_drv.h"
> +
> +static u32 calc_residency(struct drm_device *dev, const u32 reg)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u64 raw_time;
> +	u32 residency;
> +
> +	if (!intel_enable_rc6(dev))
> +		return 0;
> +
> +	raw_time = I915_READ(reg) * 128ULL;
> +	residency = DIV_ROUND_CLOSEST(raw_time, 1000) / 100;
> +	return residency;
> +}
> +
> +static ssize_t
> +show_rc6pp_ms(struct device *dev, struct device_attribute *attr,
> char *buf) +{
> +	struct drm_minor *dminor = container_of(dev, struct
> drm_minor, kdev);
> +	u32 rc6pp_residency = calc_residency(dminor->dev,
> GEN6_GT_GFX_RC6pp);
> +	return snprintf(buf, PAGE_SIZE, "%u", rc6pp_residency);
> +}
> +
> +static ssize_t
> +show_rc6_ms(struct device *dev, struct device_attribute *attr, char
> *buf) +{
> +	struct drm_minor *dminor = container_of(dev, struct
> drm_minor, kdev);
> +	struct drm_i915_private *dev_priv = dminor->dev->dev_private;
> +	/* 32b value may overflow during fixed point math */
> +	u64 time = (I915_READ(GEN6_GT_GFX_RC6) * 128);
> +	u32 rc6_residency = DIV_ROUND_CLOSEST(time, 1000) / 100;
> +	return snprintf(buf, PAGE_SIZE, "%u", rc6_residency);
> +}
> +
> +static ssize_t
> +show_rc6p_ms(struct device *dev, struct device_attribute *attr, char
> *buf) +{
> +	struct drm_minor *dminor = container_of(dev, struct
> drm_minor, kdev);
> +	struct drm_i915_private *dev_priv = dminor->dev->dev_private;
> +	/* 32b value may overflow during fixed point math */
> +	u64 time = (I915_READ(GEN6_GT_GFX_RC6p) * 128);
> +	u32 rc6p_residency = DIV_ROUND_CLOSEST(time, 1000) / 100;
> +	return snprintf(buf, PAGE_SIZE, "%u", rc6p_residency);
> +}

These all use calc_residenc() in the newer patch. Again will wait on
some review before fixing.

> +
> +static DEVICE_ATTR(rc6, S_IRUGO, show_rc6_ms, NULL);
> +static DEVICE_ATTR(rc6p, S_IRUGO, show_rc6p_ms, NULL);
> +static DEVICE_ATTR(rc6pp, S_IRUGO, show_rc6pp_ms, NULL);
> +
> +static struct attribute *rc6_attrs[] = {
> +	&dev_attr_rc6.attr,
> +	&dev_attr_rc6p.attr,
> +	&dev_attr_rc6pp.attr,
> +	NULL
> +};
> +
> +static struct attribute_group rc6_attr_group = {
> +	.name = power_group_name,
> +	.attrs =  rc6_attrs
> +};
> +
> +void i915_setup_sysfs(struct drm_device *dev)
> +{
> +	int ret;
> +
> +	ret = sysfs_merge_group(&dev->primary->kdev.kobj,
> &rc6_attr_group);
> +	if (ret)
> +		DRM_ERROR("sysfs setup failed\n");
> +}
> +
> +void i915_teardown_sysfs(struct drm_device *dev)
> +{
> +	sysfs_remove_group(&dev->primary->kdev.kobj,
> &rc6_attr_group); +}

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

Re: [PATCH 1/3] drm/i915: add rc6 residency times to debugfs

[Ben Widawsky]

On Sat, 24 Mar 2012 19:09:44 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> RC6 residency should be in intervals of 1.28us, and the counter wraps.
> Here is an example using awk to get the various RC6 and RC6+ residency
> times in seconds, since boot.
> 
> cat /sys/kernel/debug/dri/0/i915_drpc_info  | grep residency | awk
> -F':' -F' '  '{print $5 * 1.28 / 1000000}'
> 
> This is primarily for QA, but has other applications as well. An
> upcoming patch to add interfaces should be more interesting to
> application developers.
> 
> CC: Ouping Zhang <ouping.zhang@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |   11 +++++++++++
>  drivers/gpu/drm/i915/i915_reg.h     |    5 +++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c index 66c90d4..72457ff 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1133,6 +1133,17 @@ static int gen6_drpc_info(struct seq_file *m)
>  
>  	seq_printf(m, "Core Power Down: %s\n",
>  		   yesno(gt_core_status & GEN6_CORE_CPD_STATE_MASK));
> +
> +	seq_printf(m, "RC6 \"Locked to RPn\" residency since boot:
> %d\n",
> +		   I915_READ(GEN6_GT_GFX_RC6_LOCKED));
> +	/* Not exactly sure what this is */
> +	seq_printf(m, "RC6 residency since boot: %d\n",
> +		   I915_READ(GEN6_GT_GFX_RC6));
> +	seq_printf(m, "RC6+ residency since boot: %d\n",
> +		   I915_READ(GEN6_GT_GFX_RC6p));
> +	seq_printf(m, "RC6++ residency since boot: %d\n",
> +		   I915_READ(GEN6_GT_GFX_RC6pp));
> +
>  	return 0;
>  }

As noted in patch 3, this was a stale patch, and the comment belongs
for the Locked to RPn.

>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h index f3609f2..b1c3d35 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3799,6 +3799,11 @@
>  						 GEN6_PM_RP_DOWN_THRESHOLD
> | \ GEN6_PM_RP_DOWN_TIMEOUT)
>  
> +#define GEN6_GT_GFX_RC6_LOCKED			0x138104
> +#define GEN6_GT_GFX_RC6				0x138108
> +#define GEN6_GT_GFX_RC6p			0x13810C
> +#define GEN6_GT_GFX_RC6pp			0x138110
> +
>  #define GEN6_PCODE_MAILBOX			0x138124
>  #define   GEN6_PCODE_READY			(1<<31)
>  #define   GEN6_READ_OC_PARAMS			0xc

Re: [PATCH 2/3] drm/i915: extract intel_enable_rc6()

[Daniel Vetter]

On Sat, Mar 24, 2012 at 07:09:45PM -0700, Ben Widawsky wrote:
> nice to have elsewhere
> 
> CC: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I was momentarily confused with this commit message, until I've noticed
that you need this in the next patch (I've assumed Eugeni needs to for
something). So please slightly elaborate on the reasons for this.

Also I've wondered whether we shouldn't filter the individual rc6 levels?
Or are all counters sane once you enable at least one of the rc6 levels?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bbbc1a4..22ab4db 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1414,6 +1414,7 @@ extern void ironlake_enable_rc6(struct drm_device *dev);
>  extern void gen6_set_rps(struct drm_device *dev, u8 val);
>  extern void intel_detect_pch(struct drm_device *dev);
>  extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
> +extern int intel_enable_rc6(const struct drm_device *dev);
>  
>  extern void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
>  extern void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ab62c96..efbf709 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8235,7 +8235,7 @@ void intel_init_emon(struct drm_device *dev)
>  	dev_priv->corr = (lcfuse & LCFUSE_HIV_MASK);
>  }
>  
> -static int intel_enable_rc6(struct drm_device *dev)
> +int intel_enable_rc6(const struct drm_device *dev)
>  {
>  	/*
>  	 * Respect the kernel parameter if it is set
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/3] drm/i915: rc6 in sysfs

[Daniel Vetter]

On Sat, Mar 24, 2012 at 07:09:46PM -0700, Ben Widawsky wrote:
> Merge rc6 information into the power group for our device. Until now the
> i915 driver has not had any sysfs entries (aside from the connector
> stuff enabled by libdrm). Since it seems like we're likely to have more
> in the future I created a new file for sysfs stubs, as well as the rc6
> sysfs functions which don't really belong elsewhere (perhaps
> i915_suspend, but most of the stuff is in intel_display,c).
> 
> displays #ms GPU has been in rc6 since boot:
> cat /sys/class/drm/card0/power/rc6
> 
> displays #ms GPU has been in deep rc6 since boot:
> cat /sys/class/drm/card0/power/rc6p
> 
> displays #ms GPU has been in deepest rc6 since boot:
> cat /sys/class/drm/card0/power/rc6pp
> 
> Important note: I've seen on SNB that even when RC6 is *not* enabled the
> rc6 register seems to have a random value in it. I cannot explain a
> reason for this. Those writing tools that utilize this value need to be
> careful and probably want to scrutinize the value very carefully.
> 
> Please see intel-gpu-tools patches for sample code.
> 
> CC: Arjan van de Ven <arjan@linux.intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I like this (with the cleanups you've mentioned). While you dig around in
sysfs, can I volunteer you to add another patch to create an official
interface for i915_max_freq? Maybe call it max_gpu_freq_MHz (if MHz is an
acceptable unit for sysfs files). We have a bug report which might get
partially happy with this (there seem to be other issues):

https://bugzilla.kernel.org/show_bug.cgi?id=41392a

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/Makefile       |    1 +
>  drivers/gpu/drm/i915/i915_debugfs.c |    2 +-
>  drivers/gpu/drm/i915/i915_dma.c     |    4 ++
>  drivers/gpu/drm/i915/i915_drv.h     |    4 ++
>  drivers/gpu/drm/i915/i915_sysfs.c   |  106 +++++++++++++++++++++++++++++++++++
>  5 files changed, 116 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_sysfs.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index ce7fc77..f801330 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -12,6 +12,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
>  	  i915_gem_execbuffer.o \
>  	  i915_gem_gtt.o \
>  	  i915_gem_tiling.o \
> +	  i915_sysfs.o \
>  	  i915_trace_points.o \
>  	  intel_display.o \
>  	  intel_crt.o \
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 72457ff..4257151 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1134,9 +1134,9 @@ static int gen6_drpc_info(struct seq_file *m)
>  	seq_printf(m, "Core Power Down: %s\n",
>  		   yesno(gt_core_status & GEN6_CORE_CPD_STATE_MASK));
>  
> +	/* Not exactly sure what this is */
>  	seq_printf(m, "RC6 \"Locked to RPn\" residency since boot: %d\n",
>  		   I915_READ(GEN6_GT_GFX_RC6_LOCKED));
> -	/* Not exactly sure what this is */
>  	seq_printf(m, "RC6 residency since boot: %d\n",
>  		   I915_READ(GEN6_GT_GFX_RC6));
>  	seq_printf(m, "RC6+ residency since boot: %d\n",
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index fdff009..64dfbb8 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -2113,6 +2113,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		}
>  	}
>  
> +	i915_setup_sysfs(dev);
> +
>  	/* Must be done after probing outputs */
>  	intel_opregion_init(dev);
>  	acpi_video_register();
> @@ -2164,6 +2166,8 @@ int i915_driver_unload(struct drm_device *dev)
>  	i915_mch_dev = NULL;
>  	spin_unlock(&mchdev_lock);
>  
> +	i915_teardown_sysfs(dev);
> +
>  	if (dev_priv->mm.inactive_shrinker.shrink)
>  		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 22ab4db..e267774 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1365,6 +1365,10 @@ extern int i915_restore_state(struct drm_device *dev);
>  extern int i915_save_state(struct drm_device *dev);
>  extern int i915_restore_state(struct drm_device *dev);
>  
> +/* i915_sysfs.c */
> +void i915_setup_sysfs(struct drm_device *dev_priv);
> +void i915_teardown_sysfs(struct drm_device *dev_priv);
> +
>  /* intel_i2c.c */
>  extern int intel_setup_gmbus(struct drm_device *dev);
>  extern void intel_teardown_gmbus(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> new file mode 100644
> index 0000000..a18a955f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -0,0 +1,106 @@
> +/*
> + * Copyright © 2012 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Ben Widawsky <ben@bwidawsk.net>
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/stat.h>
> +#include <linux/sysfs.h>
> +#include "i915_drv.h"
> +
> +static u32 calc_residency(struct drm_device *dev, const u32 reg)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u64 raw_time;
> +	u32 residency;
> +
> +	if (!intel_enable_rc6(dev))
> +		return 0;
> +
> +	raw_time = I915_READ(reg) * 128ULL;
> +	residency = DIV_ROUND_CLOSEST(raw_time, 1000) / 100;
> +	return residency;
> +}
> +
> +static ssize_t
> +show_rc6pp_ms(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> +	u32 rc6pp_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6pp);
> +	return snprintf(buf, PAGE_SIZE, "%u", rc6pp_residency);
> +}
> +
> +static ssize_t
> +show_rc6_ms(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> +	struct drm_i915_private *dev_priv = dminor->dev->dev_private;
> +	/* 32b value may overflow during fixed point math */
> +	u64 time = (I915_READ(GEN6_GT_GFX_RC6) * 128);
> +	u32 rc6_residency = DIV_ROUND_CLOSEST(time, 1000) / 100;
> +	return snprintf(buf, PAGE_SIZE, "%u", rc6_residency);
> +}
> +
> +static ssize_t
> +show_rc6p_ms(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> +	struct drm_i915_private *dev_priv = dminor->dev->dev_private;
> +	/* 32b value may overflow during fixed point math */
> +	u64 time = (I915_READ(GEN6_GT_GFX_RC6p) * 128);
> +	u32 rc6p_residency = DIV_ROUND_CLOSEST(time, 1000) / 100;
> +	return snprintf(buf, PAGE_SIZE, "%u", rc6p_residency);
> +}
> +
> +static DEVICE_ATTR(rc6, S_IRUGO, show_rc6_ms, NULL);
> +static DEVICE_ATTR(rc6p, S_IRUGO, show_rc6p_ms, NULL);
> +static DEVICE_ATTR(rc6pp, S_IRUGO, show_rc6pp_ms, NULL);
> +
> +static struct attribute *rc6_attrs[] = {
> +	&dev_attr_rc6.attr,
> +	&dev_attr_rc6p.attr,
> +	&dev_attr_rc6pp.attr,
> +	NULL
> +};
> +
> +static struct attribute_group rc6_attr_group = {
> +	.name = power_group_name,
> +	.attrs =  rc6_attrs
> +};
> +
> +void i915_setup_sysfs(struct drm_device *dev)
> +{
> +	int ret;
> +
> +	ret = sysfs_merge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
> +	if (ret)
> +		DRM_ERROR("sysfs setup failed\n");
> +}
> +
> +void i915_teardown_sysfs(struct drm_device *dev)
> +{
> +	sysfs_remove_group(&dev->primary->kdev.kobj, &rc6_attr_group);
> +}
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] tests: rc6 residency test

[Daniel Vetter]

On Sat, Mar 24, 2012 at 07:09:48PM -0700, Ben Widawsky wrote:
> This is meant to test the sysfs entry for showing rc6 residency in
> milliseconds. Remember, sysfs is a permanent interface.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  tests/Makefile.am     |    1 +
>  tests/rc6_residency.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+)
>  create mode 100644 tests/rc6_residency.c
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 6544ec7..a8eed88 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -56,6 +56,7 @@ TESTS_progs = \
>  	drm_vma_limiter_cpu \
>  	drm_vma_limiter_gtt \
>  	drm_vma_limiter_cached \
> +	rc6_residency \
>  	$(NULL)
>  
>  # IMPORTANT: The ZZ_ tests need to be run last!
> diff --git a/tests/rc6_residency.c b/tests/rc6_residency.c
> new file mode 100644
> index 0000000..8ae6aa4
> --- /dev/null
> +++ b/tests/rc6_residency.c
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright © 2012 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Ben Widawsky <ben@bwidawsk.net>
> + *
> + */
> +
> +#include <stdlib.h>
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include "drmtest.h"
> +
> +static unsigned int readit(const char *path)
> +{
> +	unsigned int ret;
> +
> +	FILE *file;
> +	file = fopen(path, "r");
> +	if (file == NULL) {
> +		fprintf(stderr, "Couldn't open %s (%d)\n", path, errno);
> +		abort();
> +	}
> +	fscanf(file, "%u", &ret);
> +	fclose(file);
> +
> +	return ret;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	/* TODO: don't always assume 0 */
> +	const int device = 0;

Yeah, I'd like to have this, because I have machines with other gpus
besides the intel one ;-)

> +	char *path;
> +	int fd, ret;
> +	unsigned int value, value2;
> +
> +	/* Use drm_open_any to verify device existence */
> +	fd = drm_open_any();
> +	close(fd);

I think we want a gem_quiescent_gpu here to ensure things are _really_
idle.

> +
> +	ret = asprintf(&path, "/sys/class/drm/card%d/power/rc6", device);
> +	if (ret == -1 || ret < strlen("/sys/class/drm/card0/power/rc6"))
> +		abort();
> +
> +	value = readit(path);
> +
> +	// Sleep for 3 seconds and compare
> +	sleep(3);
> +	value2 = readit(path);
> +	free(path);
> +
> +	/* Plenty of fudge */
> +	if (((value2 - value) > 3900) ||
> +	    ((value2 - value) < 2100))
> +		exit(EXIT_FAILURE);

Does that mean that rc6 residency is inclusive of lower-level
rc6-resdidency? I.e. on ivb where we enable rc6p, does that include rc6p
residency? Othwerwise I think we need to add rc6p and rc6pp in, too.

Cheers, Daniel

> +
> +	exit(EXIT_SUCCESS);
> +}
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 2/3] drm/i915: extract intel_enable_rc6()

[Ben Widawsky]

This function tells us the state of RC6. In the future perhaps it will
even give something like a mask back telling which RC6 types are
enabled.

In order to return sane values to userspace for an upcoming RC6 sysfs
interface (since register behavior seems to be somewhat random), we will
query this value, and therefore it needs to be available to the rest of
the driver.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bbbc1a4..22ab4db 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1414,6 +1414,7 @@ extern void ironlake_enable_rc6(struct drm_device *dev);
 extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void intel_detect_pch(struct drm_device *dev);
 extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
+extern int intel_enable_rc6(const struct drm_device *dev);
 
 extern void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
 extern void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ab62c96..efbf709 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8235,7 +8235,7 @@ void intel_init_emon(struct drm_device *dev)
 	dev_priv->corr = (lcfuse & LCFUSE_HIV_MASK);
 }
 
-static int intel_enable_rc6(struct drm_device *dev)
+int intel_enable_rc6(const struct drm_device *dev)
 {
 	/*
 	 * Respect the kernel parameter if it is set
-- 
1.7.9.4

Re: [PATCH 2/3] drm/i915: extract intel_enable_rc6()

[Daniel Vetter]

On Sun, Mar 25, 2012 at 05:33:28PM -0700, Ben Widawsky wrote:
> This function tells us the state of RC6. In the future perhaps it will
> even give something like a mask back telling which RC6 types are
> enabled.

This comment somehow sounds like you expect to use this when Eugeni's
patches are merged. But actually your patch doesn't apply without Eugeni's
patches, because they change the return type from bool to int.

So can you be slightly less fuzzy in your comments, please?

/rant
-Daniel
> 
> In order to return sane values to userspace for an upcoming RC6 sysfs
> interface (since register behavior seems to be somewhat random), we will
> query this value, and therefore it needs to be available to the rest of
> the driver.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bbbc1a4..22ab4db 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1414,6 +1414,7 @@ extern void ironlake_enable_rc6(struct drm_device *dev);
>  extern void gen6_set_rps(struct drm_device *dev, u8 val);
>  extern void intel_detect_pch(struct drm_device *dev);
>  extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
> +extern int intel_enable_rc6(const struct drm_device *dev);
>  
>  extern void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
>  extern void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ab62c96..efbf709 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8235,7 +8235,7 @@ void intel_init_emon(struct drm_device *dev)
>  	dev_priv->corr = (lcfuse & LCFUSE_HIV_MASK);
>  }
>  
> -static int intel_enable_rc6(struct drm_device *dev)
> +int intel_enable_rc6(const struct drm_device *dev)
>  {
>  	/*
>  	 * Respect the kernel parameter if it is set
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/3] drm/i915: extract intel_enable_rc6()

[Ben Widawsky]

On Sun, 25 Mar 2012 14:10:59 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sat, Mar 24, 2012 at 07:09:45PM -0700, Ben Widawsky wrote:
> > nice to have elsewhere
> > 
> > CC: Eugeni Dodonov <eugeni.dodonov@intel.com>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> I was momentarily confused with this commit message, until I've
> noticed that you need this in the next patch (I've assumed Eugeni
> needs to for something). So please slightly elaborate on the reasons
> for this.
> 
> Also I've wondered whether we shouldn't filter the individual rc6
> levels? Or are all counters sane once you enable at least one of the
> rc6 levels? -Daniel

The only insane counter is rc6 (p and pp work as I would expect). Rc6
seems to always have some random value in it. Eugeni's theory is the
BIOS initializes RC6 always, and that's where that value comes from.

The register is read only, unfortunately.

Re: [PATCH 3/3] drm/i915: rc6 in sysfs

[Ben Widawsky]

On Sun, 25 Mar 2012 14:15:56 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sat, Mar 24, 2012 at 07:09:46PM -0700, Ben Widawsky wrote:
> > Merge rc6 information into the power group for our device. Until
> > now the i915 driver has not had any sysfs entries (aside from the
> > connector stuff enabled by libdrm). Since it seems like we're
> > likely to have more in the future I created a new file for sysfs
> > stubs, as well as the rc6 sysfs functions which don't really belong
> > elsewhere (perhaps i915_suspend, but most of the stuff is in
> > intel_display,c).
> > 
> > displays #ms GPU has been in rc6 since boot:
> > cat /sys/class/drm/card0/power/rc6
> > 
> > displays #ms GPU has been in deep rc6 since boot:
> > cat /sys/class/drm/card0/power/rc6p
> > 
> > displays #ms GPU has been in deepest rc6 since boot:
> > cat /sys/class/drm/card0/power/rc6pp
> > 
> > Important note: I've seen on SNB that even when RC6 is *not*
> > enabled the rc6 register seems to have a random value in it. I
> > cannot explain a reason for this. Those writing tools that utilize
> > this value need to be careful and probably want to scrutinize the
> > value very carefully.
> > 
> > Please see intel-gpu-tools patches for sample code.
> > 
> > CC: Arjan van de Ven <arjan@linux.intel.com>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> I like this (with the cleanups you've mentioned). While you dig
> around in sysfs, can I volunteer you to add another patch to create
> an official interface for i915_max_freq? Maybe call it
> max_gpu_freq_MHz (if MHz is an acceptable unit for sysfs files). We
> have a bug report which might get partially happy with this (there
> seem to be other issues):
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=41392a
> 
> Cheers, Daniel
> 

Yes. From looking at cpureq, it seems MHz is not standard. Simply
writing the frequency as a number in the [m|b]illions is the way to go.

I'll get on this once I get an ack from you that you pick up the later
versions of my sysfs stuff.