[PATCH 0/5] Add power feature debugfs disabling

[PATCH 0/5] Add power feature debugfs disabling

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

This series has recently been accepted into the Haswell Android kernel and
helps with debugging and profiling these power features. I would like it
to be considered for upstream incorporation. The patches here have been
rebased (minimal changes required) and compile-tested only.

Broad device support is provided, accept for RPS and RC6 with Broadwell
and Valleyview. Both of these were somewhat of a moving target and I
didn't have devices to work with. Support can of course be added with
help from appropriate folks.

The hooks introduce some amount of overhead as an additional check is
often needed to determine whether the feature is on or off - similar to
the module parameters that already exist. I felt that the overhead was
minimal enough and didn't want to ugly up the code with CONFIG_DEBUG_FS
compile conditionals. But I'm open to the list's thoughts on this.

IGT tests of these new interfaces can certainly be added. I wanted to
make sure there was sufficient interest in having these interfaces before
starting on the tests. So please provide feedback.

Thanks,
Jeff

Jeff McGee (5):
  drm/i915: Add RPS debugfs manual mode
  drm/i915: Add RC6 debugfs disabling
  drm/i915: Add IPS debugfs disabling
  drm/i915: Add FBC debugfs disabling
  drm/i915: Add CxSR debugfs disabling

 drivers/gpu/drm/i915/i915_debugfs.c  | 300 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h      |  14 ++
 drivers/gpu/drm/i915/i915_irq.c      |   6 +
 drivers/gpu/drm/i915/intel_display.c |   4 +
 drivers/gpu/drm/i915/intel_pm.c      | 125 +++++++++++----
 5 files changed, 420 insertions(+), 29 deletions(-)

-- 
1.8.5.2

[PATCH 1/5] drm/i915: Add RPS debugfs manual mode

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

RPS manual mode disables/ignores load-based inputs and allows render
performance state to be controlled externally. The enabling of manual
mode and setting of desired frequency is done through debugfs.

i915_rps_manual:
'0' - RPS controlled normally using load metrics.
'1' - RPS controlled manually via i915_cur_freq writes.

i915_cur_freq:
u64 - Value is the current gpu frequency request in MHz. Writes
      accepted only if i915_rps_manual = 1.

Supports Gen6+ except Valleyview and Broadwell.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 111 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |   2 +
 drivers/gpu/drm/i915/i915_irq.c     |   6 ++
 drivers/gpu/drm/i915/intel_pm.c     |  42 ++++++++++++--
 4 files changed, 156 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index bc8707f..c6d4da0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3322,6 +3322,115 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_min_freq_fops,
 			i915_min_freq_get, i915_min_freq_set,
 			"%llu\n");
 
+static int i915_cur_freq_get(void *data, u64 *val)
+{
+	struct drm_device *dev = data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	if ((INTEL_INFO(dev)->gen < 6) ||
+	     IS_VALLEYVIEW(dev) ||
+	     IS_BROADWELL(dev))
+		return -ENODEV;
+
+	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+
+	*val = dev_priv->rps.cur_delay * GT_FREQUENCY_MULTIPLIER;
+
+	return 0;
+}
+
+static int i915_cur_freq_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u64 freq = val;
+	int ret;
+
+	if ((INTEL_INFO(dev)->gen < 6) ||
+	     IS_VALLEYVIEW(dev) ||
+	     IS_BROADWELL(dev))
+		return -ENODEV;
+
+	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+
+	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
+	if (ret)
+		return ret;
+
+	/* Must be in manual mode to guarantee setting will persist. */
+	if (!dev_priv->rps.manual_mode) {
+		mutex_unlock(&dev_priv->rps.hw_lock);
+		return -EINVAL;
+	}
+
+	do_div(val, GT_FREQUENCY_MULTIPLIER);
+
+	if (val < dev_priv->rps.min_delay || val > dev_priv->rps.max_delay) {
+		mutex_unlock(&dev_priv->rps.hw_lock);
+		return -EINVAL;
+	}
+
+	DRM_DEBUG_DRIVER("Setting current freq to %llu\n", freq);
+
+	gen6_set_rps(dev, val);
+
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_cur_freq_fops,
+			i915_cur_freq_get, i915_cur_freq_set,
+			"%llu\n");
+
+static int i915_rps_manual_get(void *data, u64 *val)
+{
+	struct drm_device *dev = data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	if ((INTEL_INFO(dev)->gen < 6) ||
+	     IS_VALLEYVIEW(dev) ||
+	     IS_BROADWELL(dev))
+		return -ENODEV;
+
+	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+
+	*val = dev_priv->rps.manual_mode;
+
+	return 0;
+}
+
+static int i915_rps_manual_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	if ((INTEL_INFO(dev)->gen < 6) ||
+	     IS_VALLEYVIEW(dev) ||
+	     IS_BROADWELL(dev))
+		return -ENODEV;
+
+	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+
+	DRM_DEBUG_DRIVER("Setting RPS mode to %s\n",
+			 val ? "manual" : "normal");
+
+	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
+	if (ret)
+		return ret;
+
+	gen6_set_rps_mode(dev, val);
+
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_rps_manual_fops,
+			i915_rps_manual_get, i915_rps_manual_set,
+			"%llu\n");
+
 static int
 i915_cache_sharing_get(void *data, u64 *val)
 {
@@ -3496,6 +3605,8 @@ static const struct i915_debugfs_files {
 	{"i915_wedged", &i915_wedged_fops},
 	{"i915_max_freq", &i915_max_freq_fops},
 	{"i915_min_freq", &i915_min_freq_fops},
+	{"i915_cur_freq", &i915_cur_freq_fops},
+	{"i915_rps_manual", &i915_rps_manual_fops},
 	{"i915_cache_sharing", &i915_cache_sharing_fops},
 	{"i915_ring_stop", &i915_ring_stop_fops},
 	{"i915_ring_missed_irq", &i915_ring_missed_irq_fops},
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fa37dfd..73fd646 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -968,6 +968,7 @@ struct intel_gen6_power_mgmt {
 	int last_adj;
 	enum { LOW_POWER, BETWEEN, HIGH_POWER } power;
 
+	bool manual_mode;
 	bool enabled;
 	struct delayed_work delayed_resume_work;
 
@@ -2536,6 +2537,7 @@ extern bool intel_fbc_enabled(struct drm_device *dev);
 extern void intel_disable_fbc(struct drm_device *dev);
 extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
 extern void intel_init_pch_refclk(struct drm_device *dev);
+extern void gen6_set_rps_mode(struct drm_device *dev, bool manual);
 extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void valleyview_set_rps(struct drm_device *dev, u8 val);
 extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b226ae6..7b04949 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1045,6 +1045,12 @@ static void gen6_pm_rps_work(struct work_struct *work)
 
 	mutex_lock(&dev_priv->rps.hw_lock);
 
+	/* May have just entered manual mode. */
+	if (dev_priv->rps.manual_mode) {
+		mutex_unlock(&dev_priv->rps.hw_lock);
+		return;
+	}
+
 	adj = dev_priv->rps.last_adj;
 	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
 		if (adj > 0)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3c79b63..cfdf5f0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3014,7 +3014,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	if (val == dev_priv->rps.cur_delay)
 		return;
 
-	gen6_set_rps_thresholds(dev_priv, val);
+	if (!dev_priv->rps.manual_mode)
+		gen6_set_rps_thresholds(dev_priv, val);
 
 	if (IS_HASWELL(dev))
 		I915_WRITE(GEN6_RPNSWREQ,
@@ -3038,12 +3039,44 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(val * 50);
 }
 
+void gen6_set_rps_mode(struct drm_device *dev, bool manual)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u8 delay;
+
+	if ((INTEL_INFO(dev)->gen < 6) ||
+	     IS_VALLEYVIEW(dev) ||
+	     IS_BROADWELL(dev)) {
+		DRM_DEBUG_DRIVER("RPS mode change not supported\n");
+		return;
+	}
+
+	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+	dev_priv->rps.manual_mode = manual;
+
+	/* Manual mode disables/ignores load-based inputs and allows render
+	 * performance state to be controlled externally. */
+	if (manual) {
+		I915_WRITE(GEN6_RP_CONTROL,
+			   GEN6_RP_MEDIA_HW_NORMAL_MODE);
+		delay = (I915_READ(GEN6_GT_PERF_STATUS) & 0xff00) >> 8;
+	} else {
+		/* Force a reset */
+		dev_priv->rps.power = HIGH_POWER;
+		dev_priv->rps.cur_delay = 0;
+		delay = dev_priv->rps.min_delay;
+	}
+
+	gen6_set_rps(dev, delay);
+}
+
 void gen6_rps_idle(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
-	if (dev_priv->rps.enabled) {
+	if (dev_priv->rps.enabled && !dev_priv->rps.manual_mode) {
 		if (IS_VALLEYVIEW(dev))
 			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
 		else
@@ -3058,7 +3091,7 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
-	if (dev_priv->rps.enabled) {
+	if (dev_priv->rps.enabled && !dev_priv->rps.manual_mode) {
 		if (IS_VALLEYVIEW(dev))
 			valleyview_set_rps(dev_priv->dev, dev_priv->rps.max_delay);
 		else
@@ -3366,8 +3399,7 @@ static void gen6_enable_rps(struct drm_device *dev)
 		DRM_DEBUG_DRIVER("Failed to set the min frequency\n");
 	}
 
-	dev_priv->rps.power = HIGH_POWER; /* force a reset */
-	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
+	gen6_set_rps_mode(dev, dev_priv->rps.manual_mode);
 
 	gen6_enable_rps_interrupts(dev);
 
-- 
1.8.5.2

Re: [PATCH 1/5] drm/i915: Add RPS debugfs manual mode

[Daniel Vetter]

On Fri, Jan 31, 2014 at 03:42:48PM -0600, jeff.mcgee@intel.com wrote:
> From: Jeff McGee <jeff.mcgee@intel.com>
> 
> RPS manual mode disables/ignores load-based inputs and allows render
> performance state to be controlled externally. The enabling of manual
> mode and setting of desired frequency is done through debugfs.
> 
> i915_rps_manual:
> '0' - RPS controlled normally using load metrics.
> '1' - RPS controlled manually via i915_cur_freq writes.
> 
> i915_cur_freq:
> u64 - Value is the current gpu frequency request in MHz. Writes
>       accepted only if i915_rps_manual = 1.
> 
> Supports Gen6+ except Valleyview and Broadwell.
> 
> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>

Hm, can't we fake this by setting min/max to the same values?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/5] drm/i915: Add RPS debugfs manual mode

[Chris Wilson]

On Tue, Feb 04, 2014 at 12:31:37PM +0100, Daniel Vetter wrote:
> On Fri, Jan 31, 2014 at 03:42:48PM -0600, jeff.mcgee@intel.com wrote:
> > From: Jeff McGee <jeff.mcgee@intel.com>
> > 
> > RPS manual mode disables/ignores load-based inputs and allows render
> > performance state to be controlled externally. The enabling of manual
> > mode and setting of desired frequency is done through debugfs.
> > 
> > i915_rps_manual:
> > '0' - RPS controlled normally using load metrics.
> > '1' - RPS controlled manually via i915_cur_freq writes.
> > 
> > i915_cur_freq:
> > u64 - Value is the current gpu frequency request in MHz. Writes
> >       accepted only if i915_rps_manual = 1.
> > 
> > Supports Gen6+ except Valleyview and Broadwell.
> > 
> > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> 
> Hm, can't we fake this by setting min/max to the same values?

This is one where we can reconfigure the hardware if explicitly set to
disabled. Or maybe we should just put the smarts in to detect min == max
and disable the interrupt generation (as opposed to just masking at the
various points along the path).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/5] drm/i915: Add RPS debugfs manual mode

[Jeff McGee]

On Tue, Feb 04, 2014 at 11:40:20AM +0000, Chris Wilson wrote:
> On Tue, Feb 04, 2014 at 12:31:37PM +0100, Daniel Vetter wrote:
> > On Fri, Jan 31, 2014 at 03:42:48PM -0600, jeff.mcgee@intel.com wrote:
> > > From: Jeff McGee <jeff.mcgee@intel.com>
> > > 
> > > RPS manual mode disables/ignores load-based inputs and allows render
> > > performance state to be controlled externally. The enabling of manual
> > > mode and setting of desired frequency is done through debugfs.
> > > 
> > > i915_rps_manual:
> > > '0' - RPS controlled normally using load metrics.
> > > '1' - RPS controlled manually via i915_cur_freq writes.
> > > 
> > > i915_cur_freq:
> > > u64 - Value is the current gpu frequency request in MHz. Writes
> > >       accepted only if i915_rps_manual = 1.
> > > 
> > > Supports Gen6+ except Valleyview and Broadwell.
> > > 
> > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> > 
> > Hm, can't we fake this by setting min/max to the same values?
> 
> This is one where we can reconfigure the hardware if explicitly set to
> disabled. Or maybe we should just put the smarts in to detect min == max
> and disable the interrupt generation (as opposed to just masking at the
> various points along the path).
> -Chris

Yes, it is possible to achieve this functionality by coercing the frequency
with min and max. That seemed less clean to me and adds extra steps to test
code. Also the hardware will still be processing RPS logic unless we add the
min==max smarts as Chris mentions.
-Jeff

[PATCH 2/5] drm/i915: Add RC6 debugfs disabling

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

i915_rc6_disable:
'0' - RC6 states used normally per device and settings.
'1' - RC6 states explicitly disabled.

Supports Gen6+ except Valleyview and Broadwell.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 49 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |  5 ++++
 drivers/gpu/drm/i915/intel_pm.c     | 31 ++++++++++++++++++++---
 3 files changed, 81 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c6d4da0..a51c357 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3431,6 +3431,54 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_rps_manual_fops,
 			i915_rps_manual_get, i915_rps_manual_set,
 			"%llu\n");
 
+static int i915_rc6_disable_get(void *data, u64 *val)
+{
+	struct drm_device *dev = data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	if ((INTEL_INFO(dev)->gen < 6) ||
+	     IS_VALLEYVIEW(dev) ||
+	     IS_BROADWELL(dev))
+		return -ENODEV;
+
+	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+
+	*val = dev_priv->rps.rc6_disable;
+
+	return 0;
+}
+
+static int i915_rc6_disable_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	if ((INTEL_INFO(dev)->gen < 6) ||
+	     IS_VALLEYVIEW(dev) ||
+	     IS_BROADWELL(dev))
+		return -ENODEV;
+
+	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+
+	DRM_DEBUG_DRIVER("Setting RC6 disable %s\n",
+			 val ? "true" : "false");
+
+	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
+	if (ret)
+		return ret;
+
+	gen6_set_rc6_mode(dev, val);
+
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_rc6_disable_fops,
+			i915_rc6_disable_get, i915_rc6_disable_set,
+			"%llu\n");
+
 static int
 i915_cache_sharing_get(void *data, u64 *val)
 {
@@ -3607,6 +3655,7 @@ static const struct i915_debugfs_files {
 	{"i915_min_freq", &i915_min_freq_fops},
 	{"i915_cur_freq", &i915_cur_freq_fops},
 	{"i915_rps_manual", &i915_rps_manual_fops},
+	{"i915_rc6_disable", &i915_rc6_disable_fops},
 	{"i915_cache_sharing", &i915_cache_sharing_fops},
 	{"i915_ring_stop", &i915_ring_stop_fops},
 	{"i915_ring_missed_irq", &i915_ring_missed_irq_fops},
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 73fd646..9893451 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -970,6 +970,10 @@ struct intel_gen6_power_mgmt {
 
 	bool manual_mode;
 	bool enabled;
+
+	u32 rc6_mask;
+	bool rc6_disable;
+
 	struct delayed_work delayed_resume_work;
 
 	/*
@@ -2537,6 +2541,7 @@ extern bool intel_fbc_enabled(struct drm_device *dev);
 extern void intel_disable_fbc(struct drm_device *dev);
 extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
 extern void intel_init_pch_refclk(struct drm_device *dev);
+extern void gen6_set_rc6_mode(struct drm_device *dev, bool disable);
 extern void gen6_set_rps_mode(struct drm_device *dev, bool manual);
 extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void valleyview_set_rps(struct drm_device *dev, u8 val);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index cfdf5f0..6478116 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3071,6 +3071,30 @@ void gen6_set_rps_mode(struct drm_device *dev, bool manual)
 	gen6_set_rps(dev, delay);
 }
 
+void gen6_set_rc6_mode(struct drm_device *dev, bool disable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if ((INTEL_INFO(dev)->gen < 6) ||
+	     IS_VALLEYVIEW(dev) ||
+	     IS_BROADWELL(dev)) {
+		DRM_DEBUG_DRIVER("RC6 disable not supported\n");
+		return;
+	}
+
+	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+	dev_priv->rps.rc6_disable = disable;
+
+	if (disable)
+		I915_WRITE(GEN6_RC_CONTROL, 0);
+	else
+		I915_WRITE(GEN6_RC_CONTROL,
+			   dev_priv->rps.rc6_mask |
+			   GEN6_RC_CTL_EI_MODE(1) |
+			   GEN6_RC_CTL_HW_ENABLE);
+}
+
 void gen6_rps_idle(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -3376,10 +3400,9 @@ static void gen6_enable_rps(struct drm_device *dev)
 
 	intel_print_rc6_info(dev, rc6_mask);
 
-	I915_WRITE(GEN6_RC_CONTROL,
-		   rc6_mask |
-		   GEN6_RC_CTL_EI_MODE(1) |
-		   GEN6_RC_CTL_HW_ENABLE);
+	dev_priv->rps.rc6_mask = rc6_mask;
+
+	gen6_set_rc6_mode(dev, dev_priv->rps.rc6_disable);
 
 	/* Power down if completely idle for over 50ms */
 	I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 50000);
-- 
1.8.5.2

[PATCH 3/5] drm/i915: Add IPS debugfs disabling

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

i915_ips_disable:
'0' - IPS enabled normally per device and settings.
'1' - IPS explicitly disabled.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 47 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/intel_display.c |  4 +++
 3 files changed, 53 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a51c357..949c6a4 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1413,6 +1413,52 @@ static int i915_ips_status(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static int i915_ips_disable_get(void *data, u64 *val)
+{
+	struct drm_device *dev = data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	if (!HAS_IPS(dev))
+		return -ENODEV;
+
+	*val = dev_priv->ips_disable;
+
+	return 0;
+}
+
+static int i915_ips_disable_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+
+	if (!HAS_IPS(dev))
+		return -ENODEV;
+
+	if (dev_priv->ips_disable == (bool)val)
+		return 0;
+
+	drm_modeset_lock_all(dev);
+
+	DRM_DEBUG_DRIVER("Setting IPS disable %s\n",
+			 val ? "true" : "false");
+
+	dev_priv->ips_disable = (bool)val;
+
+	/* Reset enabled crtc to force IPS state update */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		if (crtc->enabled)
+			intel_crtc_restore_mode(crtc);
+
+	drm_modeset_unlock_all(dev);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_ips_disable_fops,
+			i915_ips_disable_get, i915_ips_disable_set,
+			"%llu\n");
+
 static int i915_sr_status(struct seq_file *m, void *unused)
 {
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
@@ -3656,6 +3702,7 @@ static const struct i915_debugfs_files {
 	{"i915_cur_freq", &i915_cur_freq_fops},
 	{"i915_rps_manual", &i915_rps_manual_fops},
 	{"i915_rc6_disable", &i915_rc6_disable_fops},
+	{"i915_ips_disable", &i915_ips_disable_fops},
 	{"i915_cache_sharing", &i915_cache_sharing_fops},
 	{"i915_ring_stop", &i915_ring_stop_fops},
 	{"i915_ring_missed_irq", &i915_ring_missed_irq_fops},
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9893451..93e1547 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1532,6 +1532,8 @@ typedef struct drm_i915_private {
 	 * mchdev_lock in intel_pm.c */
 	struct intel_ilk_power_mgmt ips;
 
+	bool ips_disable;
+
 	struct i915_power_domains power_domains;
 
 	struct i915_psr psr;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4d4a0d9..5af41ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4580,7 +4580,11 @@ retry:
 static void hsw_compute_ips_config(struct intel_crtc *crtc,
 				   struct intel_crtc_config *pipe_config)
 {
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
 	pipe_config->ips_enabled = i915.enable_ips &&
+				   !dev_priv->ips_disable &&
 				   hsw_crtc_supports_ips(crtc) &&
 				   pipe_config->pipe_bpp <= 24;
 }
-- 
1.8.5.2

[PATCH 4/5] drm/i915: Add FBC debugfs disabling

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

i915_fbc_disable:
'0' - FBC enabled normally per device and settings.
'1' - FBC explicitly disabled.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 50 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |  3 +++
 drivers/gpu/drm/i915/intel_pm.c     |  5 ++++
 3 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 949c6a4..92f6213 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1386,6 +1386,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 		case FBC_CHIP_DEFAULT:
 			seq_puts(m, "disabled per chip default");
 			break;
+		case FBC_DEBUG_FS:
+			seq_puts(m, "disabled per debugfs");
+			break;
 		default:
 			seq_puts(m, "unknown reason");
 		}
@@ -1394,6 +1397,52 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static int i915_fbc_disable_get(void *data, u64 *val)
+{
+	struct drm_device *dev = data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	if (!HAS_FBC(dev))
+		return -ENODEV;
+
+	*val = dev_priv->fbc.disable;
+
+	return 0;
+}
+
+static int i915_fbc_disable_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+
+	if (!HAS_FBC(dev))
+		return -ENODEV;
+
+	if (dev_priv->fbc.disable == (bool)val)
+		return 0;
+
+	drm_modeset_lock_all(dev);
+
+	DRM_DEBUG_DRIVER("Setting FBC disable %s\n",
+			 val ? "true" : "false");
+
+	dev_priv->fbc.disable = (bool)val;
+
+	/* Reset enabled crtc to force FBC state update */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		if (crtc->enabled)
+			intel_crtc_restore_mode(crtc);
+
+	drm_modeset_unlock_all(dev);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_fbc_disable_fops,
+			i915_fbc_disable_get, i915_fbc_disable_set,
+			"%llu\n");
+
 static int i915_ips_status(struct seq_file *m, void *unused)
 {
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
@@ -3703,6 +3752,7 @@ static const struct i915_debugfs_files {
 	{"i915_rps_manual", &i915_rps_manual_fops},
 	{"i915_rc6_disable", &i915_rc6_disable_fops},
 	{"i915_ips_disable", &i915_ips_disable_fops},
+	{"i915_fbc_disable", &i915_fbc_disable_fops},
 	{"i915_cache_sharing", &i915_cache_sharing_fops},
 	{"i915_ring_stop", &i915_ring_stop_fops},
 	{"i915_ring_missed_irq", &i915_ring_missed_irq_fops},
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 93e1547..18b2849 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -763,7 +763,10 @@ struct i915_fbc {
 		FBC_MULTIPLE_PIPES, /* more than one pipe active */
 		FBC_MODULE_PARAM,
 		FBC_CHIP_DEFAULT, /* disabled by default on this chip */
+		FBC_DEBUG_FS, /* user requests disabling through debugfs */
 	} no_fbc_reason;
+
+	bool disable;
 };
 
 struct i915_psr {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6478116..a8605fc 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -519,6 +519,11 @@ void intel_update_fbc(struct drm_device *dev)
 			DRM_DEBUG_KMS("fbc disabled per module param\n");
 		goto out_disable;
 	}
+	if (dev_priv->fbc.disable) {
+		if (set_no_fbc_reason(dev_priv, FBC_DEBUG_FS))
+			DRM_DEBUG_KMS("fbc disabled per debugfs\n");
+		goto out_disable;
+	}
 	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||
 	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {
 		if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE))
-- 
1.8.5.2

[PATCH 5/5] drm/i915: Add CxSR debugfs disabling

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

i915_sr_disable:
'0' - CxSR enabled normally per device and settings.
'1' - CxSR explicitly disabled.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 43 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_pm.c     | 47 +++++++++++++++++++++----------------
 3 files changed, 72 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 92f6213..cccb1bf 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1523,6 +1523,8 @@ static int i915_sr_status(struct seq_file *m, void *unused)
 		sr_enabled = I915_READ(INSTPM) & INSTPM_SELF_EN;
 	else if (IS_PINEVIEW(dev))
 		sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN;
+	else if (IS_VALLEYVIEW(dev))
+		sr_enabled = I915_READ(FW_BLC_SELF_VLV) & FW_CSPWRDWNEN;
 
 	seq_printf(m, "self-refresh: %s\n",
 		   sr_enabled ? "enabled" : "disabled");
@@ -1530,6 +1532,46 @@ static int i915_sr_status(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static int i915_sr_disable_get(void *data, u64 *val)
+{
+	struct drm_device *dev = data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	*val = dev_priv->sr_disable;
+
+	return 0;
+}
+
+static int i915_sr_disable_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+
+	if (dev_priv->sr_disable == (bool)val)
+		return 0;
+
+	drm_modeset_lock_all(dev);
+
+	DRM_DEBUG_DRIVER("Setting CxSR disable %s\n",
+			 val ? "true" : "false");
+
+	dev_priv->sr_disable = (bool)val;
+
+	/* Reset enabled crtc to force CxSR state update */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		if (crtc->enabled)
+			intel_crtc_restore_mode(crtc);
+
+	drm_modeset_unlock_all(dev);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_sr_disable_fops,
+			i915_sr_disable_get, i915_sr_disable_set,
+			"%llu\n");
+
 static int i915_emon_status(struct seq_file *m, void *unused)
 {
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
@@ -3753,6 +3795,7 @@ static const struct i915_debugfs_files {
 	{"i915_rc6_disable", &i915_rc6_disable_fops},
 	{"i915_ips_disable", &i915_ips_disable_fops},
 	{"i915_fbc_disable", &i915_fbc_disable_fops},
+	{"i915_sr_disable", &i915_sr_disable_fops},
 	{"i915_cache_sharing", &i915_cache_sharing_fops},
 	{"i915_ring_stop", &i915_ring_stop_fops},
 	{"i915_ring_missed_irq", &i915_ring_missed_irq_fops},
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 18b2849..6d7dae2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1537,6 +1537,8 @@ typedef struct drm_i915_private {
 
 	bool ips_disable;
 
+	bool sr_disable;
+
 	struct i915_power_domains power_domains;
 
 	struct i915_psr psr;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a8605fc..93e1c60 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1041,7 +1041,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
 	}
 
 	crtc = single_enabled_crtc(dev);
-	if (crtc) {
+	if (crtc && !dev_priv->sr_disable) {
 		const struct drm_display_mode *adjusted_mode;
 		int pixel_size = crtc->fb->bits_per_pixel / 8;
 		int clock;
@@ -1335,6 +1335,7 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
 		enabled |= 1 << PIPE_B;
 
 	if (single_plane_enabled(enabled) &&
+	    !dev_priv->sr_disable &&
 	    g4x_compute_srwm(dev, ffs(enabled) - 1,
 			     sr_latency_ns,
 			     &valleyview_wm_info,
@@ -1392,6 +1393,7 @@ static void g4x_update_wm(struct drm_crtc *crtc)
 		enabled |= 1 << PIPE_B;
 
 	if (single_plane_enabled(enabled) &&
+	    !dev_priv->sr_disable &&
 	    g4x_compute_srwm(dev, ffs(enabled) - 1,
 			     sr_latency_ns,
 			     &g4x_wm_info,
@@ -1433,7 +1435,7 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
 
 	/* Calc sr entries for one plane configs */
 	crtc = single_enabled_crtc(dev);
-	if (crtc) {
+	if (crtc && !dev_priv->sr_disable) {
 		/* self-refresh has much higher latency */
 		static const int sr_latency_ns = 12000;
 		const struct drm_display_mode *adjusted_mode =
@@ -1603,7 +1605,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	I915_WRITE(FW_BLC2, fwater_hi);
 
 	if (HAS_FW_BLC(dev)) {
-		if (enabled) {
+		if (enabled && !dev_priv->sr_disable) {
 			if (IS_I945G(dev) || IS_I945GM(dev))
 				I915_WRITE(FW_BLC_SELF,
 					   FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
@@ -2279,12 +2281,34 @@ static void ilk_compute_wm_results(struct drm_device *dev,
 				   enum intel_ddb_partitioning partitioning,
 				   struct ilk_wm_values *results)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
 	int level, wm_lp;
 
 	results->enable_fbc_wm = merged->fbc_wm_enabled;
 	results->partitioning = partitioning;
 
+	/* LP0 register values */
+	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
+		enum pipe pipe = intel_crtc->pipe;
+		const struct intel_wm_level *r =
+			&intel_crtc->wm.active.wm[0];
+
+		if (WARN_ON(!r->enable))
+			continue;
+
+		results->wm_linetime[pipe] = intel_crtc->wm.active.linetime;
+
+		results->wm_pipe[pipe] =
+			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
+			(r->spr_val << WM0_PIPE_SPRITE_SHIFT) |
+			r->cur_val;
+	}
+
+	/* Leave LP1+ registers zeroed if self-refresh is not to be used */
+	if (dev_priv->sr_disable)
+		return;
+
 	/* LP1+ register values */
 	for (wm_lp = 1; wm_lp <= 3; wm_lp++) {
 		const struct intel_wm_level *r;
@@ -2313,23 +2337,6 @@ static void ilk_compute_wm_results(struct drm_device *dev,
 		} else
 			results->wm_lp_spr[wm_lp - 1] = r->spr_val;
 	}
-
-	/* LP0 register values */
-	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
-		enum pipe pipe = intel_crtc->pipe;
-		const struct intel_wm_level *r =
-			&intel_crtc->wm.active.wm[0];
-
-		if (WARN_ON(!r->enable))
-			continue;
-
-		results->wm_linetime[pipe] = intel_crtc->wm.active.linetime;
-
-		results->wm_pipe[pipe] =
-			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
-			(r->spr_val << WM0_PIPE_SPRITE_SHIFT) |
-			r->cur_val;
-	}
 }
 
 /* Find the result with the highest level enabled. Check for enable_fbc_wm in
-- 
1.8.5.2

Re: [PATCH 0/5] Add power feature debugfs disabling

[Chris Wilson]

On Fri, Jan 31, 2014 at 03:42:47PM -0600, jeff.mcgee@intel.com wrote:
> From: Jeff McGee <jeff.mcgee@intel.com>
> 
> This series has recently been accepted into the Haswell Android kernel and
> helps with debugging and profiling these power features. I would like it
> to be considered for upstream incorporation. The patches here have been
> rebased (minimal changes required) and compile-tested only.
> 
> Broad device support is provided, accept for RPS and RC6 with Broadwell
> and Valleyview. Both of these were somewhat of a moving target and I
> didn't have devices to work with. Support can of course be added with
> help from appropriate folks.
> 
> The hooks introduce some amount of overhead as an additional check is
> often needed to determine whether the feature is on or off - similar to
> the module parameters that already exist. I felt that the overhead was
> minimal enough and didn't want to ugly up the code with CONFIG_DEBUG_FS
> compile conditionals. But I'm open to the list's thoughts on this.
> 
> IGT tests of these new interfaces can certainly be added. I wanted to
> make sure there was sufficient interest in having these interfaces before
> starting on the tests. So please provide feedback.

I can see the value of adding this for power testing and the code looks
quite neat and self-contained. (The one bikeshed I have is that I would
like the parameter check and debug.disable check combined into a single
function call, similar to intel_enable_rc6() so that all the similar
logic is together, well commented and easy to verify, and hard for
callers to get wrong.) Longer term, should we not consider this for
our /sys/drm/card0/power API? i.e. do we see value beyond debugging and
testing?

In the RPS autoadjust disabling code, it looks like you could go much
further and shutdown the interrupts entirely saving wakeups and power.
And put the restart logic into a single function (it requires too much
inner knowledge of the RPS logic to spread around).

I think the rest looked ok - but I did have a moment of puzzlement that
IPS did not turn off intel_ips ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/5] Add power feature debugfs disabling

[Daniel Vetter]

On Sat, Feb 01, 2014 at 05:14:22PM +0000, Chris Wilson wrote:
> On Fri, Jan 31, 2014 at 03:42:47PM -0600, jeff.mcgee@intel.com wrote:
> > From: Jeff McGee <jeff.mcgee@intel.com>
> > 
> > This series has recently been accepted into the Haswell Android kernel and
> > helps with debugging and profiling these power features. I would like it
> > to be considered for upstream incorporation. The patches here have been
> > rebased (minimal changes required) and compile-tested only.
> > 
> > Broad device support is provided, accept for RPS and RC6 with Broadwell
> > and Valleyview. Both of these were somewhat of a moving target and I
> > didn't have devices to work with. Support can of course be added with
> > help from appropriate folks.
> > 
> > The hooks introduce some amount of overhead as an additional check is
> > often needed to determine whether the feature is on or off - similar to
> > the module parameters that already exist. I felt that the overhead was
> > minimal enough and didn't want to ugly up the code with CONFIG_DEBUG_FS
> > compile conditionals. But I'm open to the list's thoughts on this.
> > 
> > IGT tests of these new interfaces can certainly be added. I wanted to
> > make sure there was sufficient interest in having these interfaces before
> > starting on the tests. So please provide feedback.
> 
> I can see the value of adding this for power testing and the code looks
> quite neat and self-contained. (The one bikeshed I have is that I would
> like the parameter check and debug.disable check combined into a single
> function call, similar to intel_enable_rc6() so that all the similar
> logic is together, well commented and easy to verify, and hard for
> callers to get wrong.) Longer term, should we not consider this for
> our /sys/drm/card0/power API? i.e. do we see value beyond debugging and
> testing?

For fbc/psr/ips I'd even use a connector/crtc property if we really need
to frob this. Would be a good excuse for some better kms cmdline
utilities ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/5] Add power feature debugfs disabling

[Daniel Vetter]

On Fri, Jan 31, 2014 at 03:42:47PM -0600, jeff.mcgee@intel.com wrote:
> From: Jeff McGee <jeff.mcgee@intel.com>
> 
> This series has recently been accepted into the Haswell Android kernel and
> helps with debugging and profiling these power features. I would like it
> to be considered for upstream incorporation. The patches here have been
> rebased (minimal changes required) and compile-tested only.
> 
> Broad device support is provided, accept for RPS and RC6 with Broadwell
> and Valleyview. Both of these were somewhat of a moving target and I
> didn't have devices to work with. Support can of course be added with
> help from appropriate folks.
> 
> The hooks introduce some amount of overhead as an additional check is
> often needed to determine whether the feature is on or off - similar to
> the module parameters that already exist. I felt that the overhead was
> minimal enough and didn't want to ugly up the code with CONFIG_DEBUG_FS
> compile conditionals. But I'm open to the list's thoughts on this.
> 
> IGT tests of these new interfaces can certainly be added. I wanted to
> make sure there was sufficient interest in having these interfaces before
> starting on the tests. So please provide feedback.

debugfs doesn't have any abi guarantees and hence requirements are much
lower. Generally I only want a testcase for new debugfs if it is complex
infrastructure and other testcases rely on its functionality, like the CRC
stuff. Simple on/off knobs imo don't need testcases really.

Still even debugfs code has a bit of cost, so I'll hold off until someone
says that "yep, this is useful for developing stuff, I'll review it" or
some i-g-ts show up.

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

Re: [PATCH 0/5] Add power feature debugfs disabling

[Jeff McGee]

On Tue, Feb 04, 2014 at 12:30:00PM +0100, Daniel Vetter wrote:
> On Fri, Jan 31, 2014 at 03:42:47PM -0600, jeff.mcgee@intel.com wrote:
> > From: Jeff McGee <jeff.mcgee@intel.com>
> > 
> > This series has recently been accepted into the Haswell Android kernel and
> > helps with debugging and profiling these power features. I would like it
> > to be considered for upstream incorporation. The patches here have been
> > rebased (minimal changes required) and compile-tested only.
> > 
> > Broad device support is provided, accept for RPS and RC6 with Broadwell
> > and Valleyview. Both of these were somewhat of a moving target and I
> > didn't have devices to work with. Support can of course be added with
> > help from appropriate folks.
> > 
> > The hooks introduce some amount of overhead as an additional check is
> > often needed to determine whether the feature is on or off - similar to
> > the module parameters that already exist. I felt that the overhead was
> > minimal enough and didn't want to ugly up the code with CONFIG_DEBUG_FS
> > compile conditionals. But I'm open to the list's thoughts on this.
> > 
> > IGT tests of these new interfaces can certainly be added. I wanted to
> > make sure there was sufficient interest in having these interfaces before
> > starting on the tests. So please provide feedback.
> 
> debugfs doesn't have any abi guarantees and hence requirements are much
> lower. Generally I only want a testcase for new debugfs if it is complex
> infrastructure and other testcases rely on its functionality, like the CRC
> stuff. Simple on/off knobs imo don't need testcases really.
> 
> Still even debugfs code has a bit of cost, so I'll hold off until someone
> says that "yep, this is useful for developing stuff, I'll review it" or
> some i-g-ts show up.

Our Android system validation tests are expecting these interfaces. That's
not igt, I know, but is supporting downstream test suites a priority? I can
get our val guys on the list to +1 the need for these patches. Likewise I
can request a developer from my team to review these patches. Or are you
looking specifically for someone outside our downstream product to 2nd the
need-for and quality of the patches?
-Jeff

Re: [PATCH 0/5] Add power feature debugfs disabling

[Daniel Vetter]

On Thu, Feb 6, 2014 at 4:44 PM, Jeff McGee <jeff.mcgee@intel.com> wrote:
> Our Android system validation tests are expecting these interfaces. That's
> not igt, I know, but is supporting downstream test suites a priority? I can
> get our val guys on the list to +1 the need for these patches. Likewise I
> can request a developer from my team to review these patches. Or are you
> looking specifically for someone outside our downstream product to 2nd the
> need-for and quality of the patches?

I don't mind if something is only used in one product - if it's useful
sooner or later other product teams will pick up on in. So upstreaming
is the right thing. But if you add a debugfs for tests I also want to
have the tests upstreamed for the following reasons:

- If a product team deems it useful to test something, our upstream QA
should probably do the same.

- Without testcases actually using these on upstream there's a good
chance that we'll break them. Which means forward-rolling to new
forklifts will be more of a pain than strictly needed. We already have
our issues with upstream collaboration, no need to make it more ugly.

- For interfaces used in tests/scripts I also want to do a bit of api
review, which is best done by looking at the actual users. Ofc debugfs
doesn't have strict abi guarantees imposed by the linux community like
ioctls/sysfs which we're essentially never allowed to break. But
change the interfaces still has its costs.

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

Re: [PATCH 0/5] Add power feature debugfs disabling

[Jeff McGee]

On Thu, Feb 06, 2014 at 05:37:29PM +0100, Daniel Vetter wrote:
> On Thu, Feb 6, 2014 at 4:44 PM, Jeff McGee <jeff.mcgee@intel.com> wrote:
> > Our Android system validation tests are expecting these interfaces. That's
> > not igt, I know, but is supporting downstream test suites a priority? I can
> > get our val guys on the list to +1 the need for these patches. Likewise I
> > can request a developer from my team to review these patches. Or are you
> > looking specifically for someone outside our downstream product to 2nd the
> > need-for and quality of the patches?
> 
> I don't mind if something is only used in one product - if it's useful
> sooner or later other product teams will pick up on in. So upstreaming
> is the right thing. But if you add a debugfs for tests I also want to
> have the tests upstreamed for the following reasons:
> 
> - If a product team deems it useful to test something, our upstream QA
> should probably do the same.
> 
> - Without testcases actually using these on upstream there's a good
> chance that we'll break them. Which means forward-rolling to new
> forklifts will be more of a pain than strictly needed. We already have
> our issues with upstream collaboration, no need to make it more ugly.
> 
> - For interfaces used in tests/scripts I also want to do a bit of api
> review, which is best done by looking at the actual users. Ofc debugfs
> doesn't have strict abi guarantees imposed by the linux community like
> ioctls/sysfs which we're essentially never allowed to break. But
> change the interfaces still has its costs.
> 
> Cheers, Daniel

Thanks for the clarification. I think there is a willingness from our
Android teams, both dev and val, to invest in igt as we've just started to
do. I don't know the history well, but we obvious have a lot invested in
our own test suites, and so bridging those with igt will take time and
may occur on the back burner while the focus has to be on Android product.
-Jeff