[PATCH 0/8 v2] Moar sysfs stuff

[PATCH 0/8 v2] Moar sysfs stuff

[Ben Widawsky]

This series is being resubmitted in full because I added two new patches
which changes the orderings. In addition I found a bug using the newly
included intel-gpu-tool test, and I've dropped Jesse's patch since there
was some negative hooplah. I also dropped the previous patch 1, which
Daniel has already taken.

Ben Widawsky (7):
  drm/i915: variable renames
  drm/i915: #define gpu freq multipler
  drm/i915: Add current/max/min GPU freq to sysfs
  drm/i915: POSTING_READ the new rps value
  drm/i915: Error checks in gen6_set_rps
  drm/i915: Add setters for min/max frequency
  drm/i915: Show render P state thresholds in sysfs

 drivers/gpu/drm/i915/i915_debugfs.c |  22 ++--
 drivers/gpu/drm/i915/i915_drv.h     |   2 +
 drivers/gpu/drm/i915/i915_irq.c     |   8 +-
 drivers/gpu/drm/i915/i915_sysfs.c   | 206 ++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_pm.c     |   4 +
 5 files changed, 222 insertions(+), 20 deletions(-)

Ben Widawsky (1):
  sysfs rps test added

 tests/Makefile.am |   1 +
 tests/sysfs_rps.c | 179 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 180 insertions(+)
 create mode 100644 tests/sysfs_rps.c


-- 
1.7.12

[REPOST PATCH 1/7] drm/i915: variable renames

[Ben Widawsky]

Name variables a bit better for copy-pasters. This got turned up as part
of review for upcoming sysfs patches.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_sysfs.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index da733a3..c701a17 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -46,32 +46,32 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
 }
 
 static ssize_t
-show_rc6_mask(struct device *dev, struct device_attribute *attr, char *buf)
+show_rc6_mask(struct device *kdev, struct device_attribute *attr, char *buf)
 {
-	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+	struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev);
 	return snprintf(buf, PAGE_SIZE, "%x", intel_enable_rc6(dminor->dev));
 }
 
 static ssize_t
-show_rc6_ms(struct device *dev, struct device_attribute *attr, char *buf)
+show_rc6_ms(struct device *kdev, struct device_attribute *attr, char *buf)
 {
-	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+	struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev);
 	u32 rc6_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6);
 	return snprintf(buf, PAGE_SIZE, "%u", rc6_residency);
 }
 
 static ssize_t
-show_rc6p_ms(struct device *dev, struct device_attribute *attr, char *buf)
+show_rc6p_ms(struct device *kdev, struct device_attribute *attr, char *buf)
 {
-	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+	struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev);
 	u32 rc6p_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6p);
 	return snprintf(buf, PAGE_SIZE, "%u", rc6p_residency);
 }
 
 static ssize_t
-show_rc6pp_ms(struct device *dev, struct device_attribute *attr, char *buf)
+show_rc6pp_ms(struct device *kdev, struct device_attribute *attr, char *buf)
 {
-	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+	struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev);
 	u32 rc6pp_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6pp);
 	return snprintf(buf, PAGE_SIZE, "%u", rc6pp_residency);
 }
-- 
1.7.12

[REPOST PATCH 2/7] drm/i915: #define gpu freq multipler

[Ben Widawsky]

Magic numbers are bad mmmkay. In this case in particular the value is
especially weird because the docs say multiple things. We'll need this
value for sysfs, so extracting it is useful for that as well.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 22 +++++++++++-----------
 drivers/gpu/drm/i915/i915_drv.h     |  2 ++
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 274a328..7a4b3f3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -914,7 +914,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
 		seq_printf(m, "Render p-state limit: %d\n",
 			   rp_state_limits & 0xff);
 		seq_printf(m, "CAGF: %dMHz\n", ((rpstat & GEN6_CAGF_MASK) >>
-						GEN6_CAGF_SHIFT) * 50);
+						GEN6_CAGF_SHIFT) * GT_FREQUENCY_MULTIPLIER);
 		seq_printf(m, "RP CUR UP EI: %dus\n", rpupei &
 			   GEN6_CURICONT_MASK);
 		seq_printf(m, "RP CUR UP: %dus\n", rpcurup &
@@ -930,15 +930,15 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
 
 		max_freq = (rp_state_cap & 0xff0000) >> 16;
 		seq_printf(m, "Lowest (RPN) frequency: %dMHz\n",
-			   max_freq * 50);
+			   max_freq * GT_FREQUENCY_MULTIPLIER);
 
 		max_freq = (rp_state_cap & 0xff00) >> 8;
 		seq_printf(m, "Nominal (RP1) frequency: %dMHz\n",
-			   max_freq * 50);
+			   max_freq * GT_FREQUENCY_MULTIPLIER);
 
 		max_freq = rp_state_cap & 0xff;
 		seq_printf(m, "Max non-overclocked (RP0) frequency: %dMHz\n",
-			   max_freq * 50);
+			   max_freq * GT_FREQUENCY_MULTIPLIER);
 	} else {
 		seq_printf(m, "no P-state info available\n");
 	}
@@ -1292,7 +1292,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
 			continue;
 		}
 		ia_freq = I915_READ(GEN6_PCODE_DATA);
-		seq_printf(m, "%d\t\t%d\n", gpu_freq * 50, ia_freq * 100);
+		seq_printf(m, "%d\t\t%d\n", gpu_freq * GT_FREQUENCY_MULTIPLIER, ia_freq * 100);
 	}
 
 	mutex_unlock(&dev->struct_mutex);
@@ -1717,7 +1717,7 @@ i915_max_freq_read(struct file *filp,
 		return ret;
 
 	len = snprintf(buf, sizeof(buf),
-		       "max freq: %d\n", dev_priv->rps.max_delay * 50);
+		       "max freq: %d\n", dev_priv->rps.max_delay * GT_FREQUENCY_MULTIPLIER);
 	mutex_unlock(&dev->struct_mutex);
 
 	if (len > sizeof(buf))
@@ -1760,9 +1760,9 @@ i915_max_freq_write(struct file *filp,
 	/*
 	 * Turbo will still be enabled, but won't go above the set value.
 	 */
-	dev_priv->rps.max_delay = val / 50;
+	dev_priv->rps.max_delay = val / GT_FREQUENCY_MULTIPLIER;
 
-	gen6_set_rps(dev, val / 50);
+	gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER);
 	mutex_unlock(&dev->struct_mutex);
 
 	return cnt;
@@ -1793,7 +1793,7 @@ i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max,
 		return ret;
 
 	len = snprintf(buf, sizeof(buf),
-		       "min freq: %d\n", dev_priv->rps.min_delay * 50);
+		       "min freq: %d\n", dev_priv->rps.min_delay * GT_FREQUENCY_MULTIPLIER);
 	mutex_unlock(&dev->struct_mutex);
 
 	if (len > sizeof(buf))
@@ -1834,9 +1834,9 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	/*
 	 * Turbo will still be enabled, but won't go below the set value.
 	 */
-	dev_priv->rps.min_delay = val / 50;
+	dev_priv->rps.min_delay = val / GT_FREQUENCY_MULTIPLIER;
 
-	gen6_set_rps(dev, val / 50);
+	gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER);
 	mutex_unlock(&dev->struct_mutex);
 
 	return cnt;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 26c6959..0eb18be 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1163,6 +1163,8 @@ struct drm_i915_file_private {
 
 #define HAS_L3_GPU_CACHE(dev) (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
 
+#define GT_FREQUENCY_MULTIPLIER 50
+
 #include "i915_trace.h"
 
 /**
-- 
1.7.12

[REPOST PATCH 3/7 v2] drm/i915: Add current/max/min GPU freq to sysfs

[Ben Widawsky]

Userspace applications such as PowerTOP are interesting in being able to
read the current GPU frequency. The patch itself sets up a generic array
for gen6 attributes so we can easily add other items in the future (and
it also happens to be just about the cleanest way to do this).

The patch is a nice addition to
commit 1ac02185dff3afac146d745ba220dc6672d1d162
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Aug 30 13:26:48 2012 +0200

    drm/i915: add a tracepoint for gpu frequency changes

Reading the GPU frequncy can be done by reading a file like:
/sys/class/drm/card0/render_frequency_mhz

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
v2: rename the sysfs file to gt_cur_freq_mhz (Daniel)
Use kdev naming (Chris)
Use DEVICE_ATTR over __ATTR (Ben)
Add min/max freq (Ben/Daniel)
user the new #define for frequency multiplier
---
 drivers/gpu/drm/i915/i915_sysfs.c | 70 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index c701a17..1da07e3 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -203,6 +203,69 @@ static struct bin_attribute dpf_attrs = {
 	.mmap = NULL
 };
 
+static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
+	struct drm_device *dev = minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
+
+	ret = dev_priv->rps.cur_delay * GT_FREQUENCY_MULTIPLIER;
+	mutex_unlock(&dev->struct_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%d", ret);
+}
+
+static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
+{
+	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
+	struct drm_device *dev = minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
+
+	ret = dev_priv->rps.max_delay * GT_FREQUENCY_MULTIPLIER;
+	mutex_unlock(&dev->struct_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%d", ret);
+}
+
+static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
+{
+	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
+	struct drm_device *dev = minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
+
+	ret = dev_priv->rps.min_delay * GT_FREQUENCY_MULTIPLIER;
+	mutex_unlock(&dev->struct_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%d", ret);
+}
+
+static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL);
+static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, NULL);
+static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, NULL);
+
+static const struct attribute *gen6_attrs[] = {
+	&dev_attr_gt_cur_freq_mhz.attr,
+	&dev_attr_gt_max_freq_mhz.attr,
+	&dev_attr_gt_min_freq_mhz.attr,
+	NULL,
+};
+
 void i915_setup_sysfs(struct drm_device *dev)
 {
 	int ret;
@@ -220,10 +283,17 @@ void i915_setup_sysfs(struct drm_device *dev)
 		if (ret)
 			DRM_ERROR("l3 parity sysfs setup failed\n");
 	}
+
+	if (INTEL_INFO(dev)->gen >= 6) {
+		ret = sysfs_create_files(&dev->primary->kdev.kobj, gen6_attrs);
+		if (ret)
+			DRM_ERROR("gen6 sysfs setup failed\n");
+	}
 }
 
 void i915_teardown_sysfs(struct drm_device *dev)
 {
+	sysfs_remove_files(&dev->primary->kdev.kobj, gen6_attrs);
 	device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
 	sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
 }
-- 
1.7.12

[PATCH 4/7] drm/i915: POSTING_READ the new rps value

[Ben Widawsky]

In order to keep our cached values in sync with the hardware, we need a
posting read here.

CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 36c6409..4e86037 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2338,6 +2338,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	 */
 	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
 
+	POSTING_READ(GEN6_RPNSWREQ);
+
 	dev_priv->rps.cur_delay = val;
 
 	trace_intel_gpu_freq_change(val * 50);
-- 
1.7.12

[PATCH 5/7] drm/i915: Error checks in gen6_set_rps

[Ben Widawsky]

With the new "standardized" sysfs interfaces we need to be a bit more
careful about setting the RPS values.

Because the sysfs code and the rps workqueue can run at the same time,
if the sysfs setter wins the race to the mutex, the workqueue can come
in and set a value which is out of range (ie. we're no longer protecting
by RPINTLIM).

I was not able to actually make this error occur in testing.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d601013..e34b7d4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -382,7 +382,13 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	else
 		new_delay = dev_priv->rps.cur_delay - 1;
 
-	gen6_set_rps(dev_priv->dev, new_delay);
+	/* sysfs frequency interfaces may have snuck in while servicing the
+	 * interrupt
+	 */
+	if (!(new_delay > dev_priv->rps.max_delay ||
+	      new_delay < dev_priv->rps.min_delay)) {
+		gen6_set_rps(dev_priv->dev, new_delay);
+	}
 
 	mutex_unlock(&dev_priv->dev->struct_mutex);
 }
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4e86037..82ca172 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2324,6 +2324,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	u32 limits = gen6_rps_limits(dev_priv, &val);
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+	WARN_ON(val > dev_priv->rps.max_delay);
+	WARN_ON(val < dev_priv->rps.min_delay);
 
 	if (val == dev_priv->rps.cur_delay)
 		return;
-- 
1.7.12

[PATCH 6/7 v3] drm/i915: Add setters for min/max frequency

[Ben Widawsky]

Provide a standardized sysfs interface for setting min, and max
frequencies.  The code which reads the limits were lifted from the
debugfs files. As a brief explanation, the limits are similar to the CPU
p-states. We have 3 states:

RP0 - ie. max frequency
RP1 - ie. "preferred min" frequency
RPn - seriously lowest frequency

Initially Daniel asked me to clamp the writes to supported values, but
in conforming to the way the cpufreq drivers seem to work, instead
return -EINVAL (noticed by Jesse in discussion).
The values can be used by userspace wishing to control the limits of the
GPU (see the CC list for people who care).

v2 Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
v3: bug fix (Ben) -  was passing the MHz value to gen6_set_rps instead of
the step value. To fix, deal only with step values by doing the divide
at the top.

v2: add the dropped mutex_unlock in error cases (Chris)
EINVAL on both too min, or too max (Daniel)
---
 drivers/gpu/drm/i915/i915_sysfs.c | 88 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 1da07e3..b67f5d7 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -238,6 +238,48 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute
 	return snprintf(buf, PAGE_SIZE, "%d", ret);
 }
 
+static ssize_t gt_max_freq_mhz_store(struct device *kdev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
+	struct drm_device *dev = minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 val, rp_state_cap, hw_max, hw_min;
+	ssize_t ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	val /= GT_FREQUENCY_MULTIPLIER;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
+	hw_max = (rp_state_cap & 0xff);
+	hw_min = ((rp_state_cap & 0xff0000) >> 16);
+
+	if (val < hw_min || val > hw_max) {
+		mutex_unlock(&dev->struct_mutex);
+		return -EINVAL;
+	}
+
+	if (val < dev_priv->rps.min_delay)
+		val = dev_priv->rps.min_delay;
+
+	if (dev_priv->rps.cur_delay > val)
+		gen6_set_rps(dev_priv->dev, val);
+
+	dev_priv->rps.max_delay = val;
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return count;
+}
+
 static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
 {
 	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
@@ -255,9 +297,51 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute
 	return snprintf(buf, PAGE_SIZE, "%d", ret);
 }
 
+static ssize_t gt_min_freq_mhz_store(struct device *kdev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
+	struct drm_device *dev = minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 val, rp_state_cap, hw_max, hw_min;
+	ssize_t ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	val /= GT_FREQUENCY_MULTIPLIER;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
+	hw_max = (rp_state_cap & 0xff);
+	hw_min = ((rp_state_cap & 0xff0000) >> 16);
+
+	if (val < hw_min || val > hw_max) {
+		mutex_unlock(&dev->struct_mutex);
+		return -EINVAL;
+	}
+
+	if (val > dev_priv->rps.max_delay)
+		val = dev_priv->rps.max_delay;
+
+	if (dev_priv->rps.cur_delay < val)
+		gen6_set_rps(dev_priv->dev, val);
+
+	dev_priv->rps.min_delay = val;
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return count;
+}
+
 static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL);
-static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, NULL);
-static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, NULL);
+static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, gt_max_freq_mhz_store);
+static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, gt_min_freq_mhz_store);
 
 static const struct attribute *gen6_attrs[] = {
 	&dev_attr_gt_cur_freq_mhz.attr,
-- 
1.7.12

[REPOST PATCH 7/7] drm/i915: Show render P state thresholds in sysfs

[Ben Widawsky]

This is useful for userspace utilities which wish to use the previous
interface, specifically for micromanaging the increase/decrease steps by
setting min == max.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index b67f5d7..11091a9 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -343,10 +343,46 @@ static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL);
 static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, gt_max_freq_mhz_store);
 static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, gt_min_freq_mhz_store);
 
+
+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_minor *minor = container_of(kdev, struct drm_minor, kdev);
+	struct drm_device *dev = minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 val, rp_state_cap;
+	ssize_t ret;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+	rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
+	mutex_unlock(&dev->struct_mutex);
+
+	if (attr == &dev_attr_gt_RP0_freq_mhz) {
+		val = ((rp_state_cap & 0x0000ff) >> 0) * GT_FREQUENCY_MULTIPLIER;
+	} else if (attr == &dev_attr_gt_RP1_freq_mhz) {
+		val = ((rp_state_cap & 0x00ff00) >> 8) * GT_FREQUENCY_MULTIPLIER;
+	} else if (attr == &dev_attr_gt_RPn_freq_mhz) {
+		val = ((rp_state_cap & 0xff0000) >> 16) * GT_FREQUENCY_MULTIPLIER;
+	} else {
+		BUG();
+	}
+	return snprintf(buf, PAGE_SIZE, "%d", val);
+}
+
 static const struct attribute *gen6_attrs[] = {
 	&dev_attr_gt_cur_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,
 };
 
-- 
1.7.12

[PATCH] sysfs rps test added

[Ben Widawsky]

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

diff --git a/tests/Makefile.am b/tests/Makefile.am
index d4c7c46..d88a83a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -71,6 +71,7 @@ TESTS_progs = \
 	drm_vma_limiter_gtt \
 	drm_vma_limiter_cached \
 	sysfs_rc6_residency \
+	sysfs_rps \
 	flip_test \
 	gem_wait_render_timeout \
 	gem_ctx_create \
diff --git a/tests/sysfs_rps.c b/tests/sysfs_rps.c
new file mode 100644
index 0000000..b830797
--- /dev/null
+++ b/tests/sysfs_rps.c
@@ -0,0 +1,179 @@
+/*
+ * 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>
+ *
+ */
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include "drmtest.h"
+
+static bool verbose = false;
+static int origmin, origmax;
+
+#define restore_assert(COND) do { \
+	if (!(COND)) { \
+		writeval(stuff[MIN].filp, origmin); \
+		writeval(stuff[MAX].filp, origmax); \
+		assert(0); \
+	} \
+} while (0);
+
+static const char sysfs_base_path[] = "/sys/class/drm/card%d/gt_%s_freq_mhz";
+enum {
+	CUR,
+	MIN,
+	MAX,
+	RP0,
+	RP1,
+	RPn
+};
+
+struct junk {
+	const char *name;
+	const char *mode;
+	FILE *filp;
+} stuff[] = {
+	{ "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { NULL, NULL, NULL }
+};
+
+static int readval(FILE *filp)
+{
+	int val;
+	fflush(filp);
+	rewind(filp);
+	fscanf(filp, "%d", &val);
+	return val;
+}
+
+static void writeval(FILE *filp, int val)
+{
+	rewind(filp);
+	fprintf(filp, "%d", val);
+	fflush(filp);
+}
+
+#define fcur (readval(stuff[CUR].filp))
+#define fmin (readval(stuff[MIN].filp))
+#define fmax (readval(stuff[MAX].filp))
+#define frp0 (readval(stuff[RP0].filp))
+#define frp1 (readval(stuff[RP1].filp))
+#define frpn (readval(stuff[RPn].filp))
+
+static void setfreq(int val)
+{
+	writeval(stuff[MIN].filp, val);
+	writeval(stuff[MAX].filp, val);
+}
+
+static void checkit(void)
+{
+	restore_assert(fmin <= fmax);
+	restore_assert(fcur <= fmax);
+	restore_assert(fmin <= fcur);
+	restore_assert(frpn <= fmin);
+	restore_assert(fmax <= frp0);
+	restore_assert(frp1 <= frp0);
+	restore_assert(frpn <= frp1);
+	restore_assert(frp0 != 0);
+	restore_assert(frp1 != 0);
+}
+
+static void dumpit(void)
+{
+	struct junk *junk = stuff;
+	do {
+		printf("gt frequency %s (MHz):  %d\n", junk->name, readval(junk->filp));
+		junk++;
+	} while(junk->name != NULL);
+
+	printf("\n");
+}
+
+
+int main(int argc, char *argv[])
+{
+	const int device = drm_get_card(0);
+	struct junk *junk = stuff;
+	int fd, ret;
+
+	if (argc > 1)
+		verbose++;
+
+	/* Use drm_open_any to verify device existence */
+	fd = drm_open_any();
+	close(fd);
+
+	do {
+		int val = -1;
+		char *path;
+		ret = asprintf(&path, sysfs_base_path, device, junk->name);
+		assert(ret != -1);
+		junk->filp = fopen(path, junk->mode);
+		if (junk->filp == NULL) {
+			fprintf(stderr, "Kernel is too old. GTFO\n");
+			exit(77);
+		}
+		val = readval(junk->filp);
+		assert(val >= 0);
+		junk++;
+	} while(junk->name != NULL);
+
+	origmin = fmin;
+	origmax = fmax;
+
+	if (verbose)
+		printf("Original min = %d\nOriginal max = %d\n", origmin, origmax);
+
+	if (verbose)
+		dumpit();
+
+	checkit();
+	setfreq(fmin);
+	if (verbose)
+		dumpit();
+	restore_assert(fcur == fmin);
+	setfreq(fmax);
+	if (verbose)
+		dumpit();
+	restore_assert(fcur == fmax);
+	checkit();
+
+	/* And some errors */
+	writeval(stuff[MIN].filp, frpn - 1);
+	writeval(stuff[MAX].filp, frp0 + 1000);
+	checkit();
+
+	writeval(stuff[MIN].filp, fmax + 1000);
+	writeval(stuff[MAX].filp, fmin - 1);
+	checkit();
+
+	writeval(stuff[MIN].filp, origmin);
+	writeval(stuff[MAX].filp, origmax);
+
+	exit(EXIT_SUCCESS);
+}
-- 
1.7.12

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

Re: [PATCH 5/7] drm/i915: Error checks in gen6_set_rps

[Chris Wilson]

On Fri,  7 Sep 2012 19:43:42 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> With the new "standardized" sysfs interfaces we need to be a bit more
> careful about setting the RPS values.
> 
> Because the sysfs code and the rps workqueue can run at the same time,
> if the sysfs setter wins the race to the mutex, the workqueue can come
> in and set a value which is out of range (ie. we're no longer protecting
> by RPINTLIM).
> 
> I was not able to actually make this error occur in testing.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Good catch, care to squeeze the comment into a single line ;-)
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/7] drm/i915: POSTING_READ the new rps value

[Chris Wilson]

On Fri,  7 Sep 2012 19:43:41 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> In order to keep our cached values in sync with the hardware, we need a
> posting read here.
> 
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Yes, a POSTING_READ does look required here.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/7 v3] drm/i915: Add setters for min/max frequency

[Daniel Vetter]

On Fri, Sep 07, 2012 at 07:43:43PM -0700, Ben Widawsky wrote:
> Provide a standardized sysfs interface for setting min, and max
> frequencies.  The code which reads the limits were lifted from the
> debugfs files. As a brief explanation, the limits are similar to the CPU
> p-states. We have 3 states:
> 
> RP0 - ie. max frequency
> RP1 - ie. "preferred min" frequency
> RPn - seriously lowest frequency
> 
> Initially Daniel asked me to clamp the writes to supported values, but
> in conforming to the way the cpufreq drivers seem to work, instead
> return -EINVAL (noticed by Jesse in discussion).
> The values can be used by userspace wishing to control the limits of the
> GPU (see the CC list for people who care).
> 
> v2 Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> v3: bug fix (Ben) -  was passing the MHz value to gen6_set_rps instead of
> the step value. To fix, deal only with step values by doing the divide
> at the top.
> 
> v2: add the dropped mutex_unlock in error cases (Chris)
> EINVAL on both too min, or too max (Daniel)
> ---
>  drivers/gpu/drm/i915/i915_sysfs.c | 88 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 1da07e3..b67f5d7 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -238,6 +238,48 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute
>  	return snprintf(buf, PAGE_SIZE, "%d", ret);
>  }
>  
> +static ssize_t gt_max_freq_mhz_store(struct device *kdev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
> +	struct drm_device *dev = minor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 val, rp_state_cap, hw_max, hw_min;
> +	ssize_t ret;
> +
> +	ret = kstrtou32(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	val /= GT_FREQUENCY_MULTIPLIER;
> +
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
> +	rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> +	hw_max = (rp_state_cap & 0xff);
> +	hw_min = ((rp_state_cap & 0xff0000) >> 16);
> +
> +	if (val < hw_min || val > hw_max) {
> +		mutex_unlock(&dev->struct_mutex);
> +		return -EINVAL;
> +	}
> +
> +	if (val < dev_priv->rps.min_delay)
> +		val = dev_priv->rps.min_delay;

Just forwarding an irc discussion so I don't forget about it:
I think we need return -EINVAL here, too. Otherwise userspace gets an
-EINVAL for hw limits, but if it tries to set up an otherwise illegal
min/max pair we silently fix it up. Same applies for the min delay below.
-Daniel

> +
> +	if (dev_priv->rps.cur_delay > val)
> +		gen6_set_rps(dev_priv->dev, val);
> +
> +	dev_priv->rps.max_delay = val;
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return count;
> +}
> +
>  static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
>  {
>  	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
> @@ -255,9 +297,51 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute
>  	return snprintf(buf, PAGE_SIZE, "%d", ret);
>  }
>  
> +static ssize_t gt_min_freq_mhz_store(struct device *kdev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
> +	struct drm_device *dev = minor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 val, rp_state_cap, hw_max, hw_min;
> +	ssize_t ret;
> +
> +	ret = kstrtou32(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	val /= GT_FREQUENCY_MULTIPLIER;
> +
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
> +	rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> +	hw_max = (rp_state_cap & 0xff);
> +	hw_min = ((rp_state_cap & 0xff0000) >> 16);
> +
> +	if (val < hw_min || val > hw_max) {
> +		mutex_unlock(&dev->struct_mutex);
> +		return -EINVAL;
> +	}
> +
> +	if (val > dev_priv->rps.max_delay)
> +		val = dev_priv->rps.max_delay;
> +
> +	if (dev_priv->rps.cur_delay < val)
> +		gen6_set_rps(dev_priv->dev, val);
> +
> +	dev_priv->rps.min_delay = val;
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return count;
> +}
> +
>  static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL);
> -static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, NULL);
> -static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, NULL);
> +static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, gt_max_freq_mhz_store);
> +static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, gt_min_freq_mhz_store);
>  
>  static const struct attribute *gen6_attrs[] = {
>  	&dev_attr_gt_cur_freq_mhz.attr,
> -- 
> 1.7.12
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/7] drm/i915: Error checks in gen6_set_rps

[Daniel Vetter]

On Sun, Sep 09, 2012 at 07:25:40PM +0100, Chris Wilson wrote:
> On Fri,  7 Sep 2012 19:43:42 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > With the new "standardized" sysfs interfaces we need to be a bit more
> > careful about setting the RPS values.
> > 
> > Because the sysfs code and the rps workqueue can run at the same time,
> > if the sysfs setter wins the race to the mutex, the workqueue can come
> > in and set a value which is out of range (ie. we're no longer protecting
> > by RPINTLIM).
> > 
> > I was not able to actually make this error occur in testing.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Good catch, care to squeeze the comment into a single line ;-)
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Applied all patches to dinq up to this one here, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 6/7 v4] drm/i915: Add setters for min/max frequency

[Ben Widawsky]

Provide a standardized sysfs interface for setting min, and max
frequencies.  The code which reads the limits were lifted from the
debugfs files. As a brief explanation, the limits are similar to the CPU
p-states. We have 3 states:

RP0 - ie. max frequency
RP1 - ie. "preferred min" frequency
RPn - seriously lowest frequency

Initially Daniel asked me to clamp the writes to supported values, but
in conforming to the way the cpufreq drivers seem to work, instead
return -EINVAL (noticed by Jesse in discussion).
The values can be used by userspace wishing to control the limits of the
GPU (see the CC list for people who care).

v4: Make exceeding the soft limits return -EINVAL as well (Daniel)

v3: bug fix (Ben) -  was passing the MHz value to gen6_set_rps instead of
the step value. To fix, deal only with step values by doing the divide
at the top.

v2: add the dropped mutex_unlock in error cases (Chris)
EINVAL on both too min, or too max (Daniel)

v2 Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
CC: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_sysfs.c | 83 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 1da07e3..1ffb988 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -238,6 +238,45 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute
 	return snprintf(buf, PAGE_SIZE, "%d", ret);
 }
 
+static ssize_t gt_max_freq_mhz_store(struct device *kdev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
+	struct drm_device *dev = minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 val, rp_state_cap, hw_max, hw_min;
+	ssize_t ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	val /= GT_FREQUENCY_MULTIPLIER;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
+	hw_max = (rp_state_cap & 0xff);
+	hw_min = ((rp_state_cap & 0xff0000) >> 16);
+
+	if (val < hw_min || val > hw_max || val < dev_priv->rps.min_delay) {
+		mutex_unlock(&dev->struct_mutex);
+		return -EINVAL;
+	}
+
+	if (dev_priv->rps.cur_delay > val)
+		gen6_set_rps(dev_priv->dev, val);
+
+	dev_priv->rps.max_delay = val;
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return count;
+}
+
 static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
 {
 	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
@@ -255,9 +294,49 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute
 	return snprintf(buf, PAGE_SIZE, "%d", ret);
 }
 
+static ssize_t gt_min_freq_mhz_store(struct device *kdev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
+	struct drm_device *dev = minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 val, rp_state_cap, hw_max, hw_min;
+	ssize_t ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	val /= GT_FREQUENCY_MULTIPLIER;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
+	hw_max = (rp_state_cap & 0xff);
+	hw_min = ((rp_state_cap & 0xff0000) >> 16);
+
+	if (val < hw_min || val > hw_max || val > dev_priv->rps.max_delay) {
+		mutex_unlock(&dev->struct_mutex);
+		return -EINVAL;
+	}
+
+	if (dev_priv->rps.cur_delay < val)
+		gen6_set_rps(dev_priv->dev, val);
+
+	dev_priv->rps.min_delay = val;
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return count;
+
+}
+
 static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL);
-static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, NULL);
-static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, NULL);
+static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, gt_max_freq_mhz_store);
+static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, gt_min_freq_mhz_store);
 
 static const struct attribute *gen6_attrs[] = {
 	&dev_attr_gt_cur_freq_mhz.attr,
-- 
1.7.12

Re: [REPOST PATCH 7/7] drm/i915: Show render P state thresholds in sysfs

[Daniel Vetter]

On Fri, Sep 07, 2012 at 07:43:44PM -0700, Ben Widawsky wrote:
> This is useful for userspace utilities which wish to use the previous
> interface, specifically for micromanaging the increase/decrease steps by
> setting min == max.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I've now also queued this and the latest iteration of patch 6, thanks a
lot.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] sysfs rps test added

[Daniel Vetter]

On Fri, Sep 07, 2012 at 07:43:45PM -0700, Ben Widawsky wrote:
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---

One small improvement would be nice:

[snip]

> +static void writeval(FILE *filp, int val)
> +{
> +	rewind(filp);
> +	fprintf(filp, "%d", val);
> +	fflush(filp);
> +}

Adding some assert here to check whether a write we expect to succeed
indeed worked, and that a write we expect to return -EINVAL indeed fails
with that error.
-Daniel

> +
> +#define fcur (readval(stuff[CUR].filp))
> +#define fmin (readval(stuff[MIN].filp))
> +#define fmax (readval(stuff[MAX].filp))
> +#define frp0 (readval(stuff[RP0].filp))
> +#define frp1 (readval(stuff[RP1].filp))
> +#define frpn (readval(stuff[RPn].filp))
> +
> +static void setfreq(int val)
> +{
> +	writeval(stuff[MIN].filp, val);
> +	writeval(stuff[MAX].filp, val);
> +}
> +
> +static void checkit(void)
> +{
> +	restore_assert(fmin <= fmax);
> +	restore_assert(fcur <= fmax);
> +	restore_assert(fmin <= fcur);
> +	restore_assert(frpn <= fmin);
> +	restore_assert(fmax <= frp0);
> +	restore_assert(frp1 <= frp0);
> +	restore_assert(frpn <= frp1);
> +	restore_assert(frp0 != 0);
> +	restore_assert(frp1 != 0);
> +}
> +
> +static void dumpit(void)
> +{
> +	struct junk *junk = stuff;
> +	do {
> +		printf("gt frequency %s (MHz):  %d\n", junk->name, readval(junk->filp));
> +		junk++;
> +	} while(junk->name != NULL);
> +
> +	printf("\n");
> +}
> +
> +
> +int main(int argc, char *argv[])
> +{
> +	const int device = drm_get_card(0);
> +	struct junk *junk = stuff;
> +	int fd, ret;
> +
> +	if (argc > 1)
> +		verbose++;
> +
> +	/* Use drm_open_any to verify device existence */
> +	fd = drm_open_any();
> +	close(fd);
> +
> +	do {
> +		int val = -1;
> +		char *path;
> +		ret = asprintf(&path, sysfs_base_path, device, junk->name);
> +		assert(ret != -1);
> +		junk->filp = fopen(path, junk->mode);
> +		if (junk->filp == NULL) {
> +			fprintf(stderr, "Kernel is too old. GTFO\n");
> +			exit(77);
> +		}
> +		val = readval(junk->filp);
> +		assert(val >= 0);
> +		junk++;
> +	} while(junk->name != NULL);
> +
> +	origmin = fmin;
> +	origmax = fmax;
> +
> +	if (verbose)
> +		printf("Original min = %d\nOriginal max = %d\n", origmin, origmax);
> +
> +	if (verbose)
> +		dumpit();
> +
> +	checkit();
> +	setfreq(fmin);
> +	if (verbose)
> +		dumpit();
> +	restore_assert(fcur == fmin);
> +	setfreq(fmax);
> +	if (verbose)
> +		dumpit();
> +	restore_assert(fcur == fmax);
> +	checkit();
> +
> +	/* And some errors */
> +	writeval(stuff[MIN].filp, frpn - 1);
> +	writeval(stuff[MAX].filp, frp0 + 1000);
> +	checkit();
> +
> +	writeval(stuff[MIN].filp, fmax + 1000);
> +	writeval(stuff[MAX].filp, fmin - 1);
> +	checkit();
> +
> +	writeval(stuff[MIN].filp, origmin);
> +	writeval(stuff[MAX].filp, origmax);
> +
> +	exit(EXIT_SUCCESS);
> +}
> -- 
> 1.7.12
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch