diff mbox

drm/i915: Removed the read of RP_STATE_CAP from sysfs/debugfs functions

Message ID 1424937549-5440-1-git-send-email-akash.goel@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

akash.goel@intel.com Feb. 26, 2015, 7:59 a.m. UTC 
From: Akash Goel <akash.goel@intel.com>

The frequency values(Rp0, Rp1, Rpn) reported by RP_STATE_CAP register
are stored, initially by the Driver, inside the dev_priv->rps structure.
Since these values are expected to remain same throughout, there is no real
need to read this register, on dynamic basis, from debugfs/sysfs functions
and the values can be instead retrieved from the dev_priv->rps structure
when needed.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 43 ++++++++++---------------------------
 drivers/gpu/drm/i915/i915_sysfs.c   | 39 ++++++++-------------------------
 2 files changed, 20 insertions(+), 62 deletions(-)

Comments

Chris Wilson Feb. 26, 2015, 8:04 a.m. UTC | #1 
On Thu, Feb 26, 2015 at 01:29:09PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> The frequency values(Rp0, Rp1, Rpn) reported by RP_STATE_CAP register
> are stored, initially by the Driver, inside the dev_priv->rps structure.
> Since these values are expected to remain same throughout, there is no real
> need to read this register, on dynamic basis, from debugfs/sysfs functions
> and the values can be instead retrieved from the dev_priv->rps structure
> when needed.

Inside debugfs, you want both as we want to compare what the hardware
thinks and what we believe it should be. Everywhere else we should
indeed be trusting our bookkeeping.
-Chris
akash.goel@intel.com Feb. 26, 2015, 8:40 a.m. UTC | #2 
On Thu, 2015-02-26 at 08:04 +0000, Chris Wilson wrote:
> On Thu, Feb 26, 2015 at 01:29:09PM +0530, akash.goel@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> > 
> > The frequency values(Rp0, Rp1, Rpn) reported by RP_STATE_CAP register
> > are stored, initially by the Driver, inside the dev_priv->rps structure.
> > Since these values are expected to remain same throughout, there is no real
> > need to read this register, on dynamic basis, from debugfs/sysfs functions
> > and the values can be instead retrieved from the dev_priv->rps structure
> > when needed.
> 
> Inside debugfs, you want both as we want to compare what the hardware
> thinks and what we believe it should be. Everywhere else we should
> indeed be trusting our bookkeeping.

Agree, ideally we should throw an error, if the stored values do not
match the ones reported by Hw and report the values read from the
registers to User.

Actually currently there is a bit of discrepancy. For VLV/CHV, directly
the stored values only are being returned to the User.

Even for non VLV also, there is inconsistency in the way RP_STATE_CAP
register is being used inside i915_max_freq_set/i915_min_freq_set pair
of debugfs functions.

So tried to bring uniformity by using the stored values everywhere. 

Best regards
Akash

> -Chris
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 26e9c7b..4736b84 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1092,13 +1092,11 @@  static int i915_frequency_info(struct seq_file *m, void *unused)
 		   IS_BROADWELL(dev)) {
 		u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
 		u32 rp_state_limits = I915_READ(GEN6_RP_STATE_LIMITS);
-		u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
 		u32 rpmodectl, rpinclimit, rpdeclimit;
 		u32 rpstat, cagf, reqf;
 		u32 rpupei, rpcurup, rpprevup;
 		u32 rpdownei, rpcurdown, rpprevdown;
 		u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
-		int max_freq;
 
 		/* RPSTAT1 is in the GT power well */
 		ret = mutex_lock_interruptible(&dev->struct_mutex);
@@ -1176,17 +1174,14 @@  static int i915_frequency_info(struct seq_file *m, void *unused)
 		seq_printf(m, "RP PREV DOWN: %dus\n", rpprevdown &
 			   GEN6_CURBSYTAVG_MASK);
 
-		max_freq = (rp_state_cap & 0xff0000) >> 16;
 		seq_printf(m, "Lowest (RPN) frequency: %dMHz\n",
-			   intel_gpu_freq(dev_priv, max_freq));
+			   intel_gpu_freq(dev_priv, dev_priv->rps.min_freq));
 
-		max_freq = (rp_state_cap & 0xff00) >> 8;
 		seq_printf(m, "Nominal (RP1) frequency: %dMHz\n",
-			   intel_gpu_freq(dev_priv, max_freq));
+			   intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq));
 
-		max_freq = rp_state_cap & 0xff;
 		seq_printf(m, "Max non-overclocked (RP0) frequency: %dMHz\n",
-			   intel_gpu_freq(dev_priv, max_freq));
+			   intel_gpu_freq(dev_priv, dev_priv->rps.rp0_freq));
 
 		seq_printf(m, "Max overclocked frequency: %dMHz\n",
 			   intel_gpu_freq(dev_priv, dev_priv->rps.max_freq));
@@ -4191,7 +4186,7 @@  i915_max_freq_set(void *data, u64 val)
 {
 	struct drm_device *dev = data;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 rp_state_cap, hw_max, hw_min;
+	u32 hw_max, hw_min;
 	int ret;
 
 	if (INTEL_INFO(dev)->gen < 6)
@@ -4208,18 +4203,10 @@  i915_max_freq_set(void *data, u64 val)
 	/*
 	 * Turbo will still be enabled, but won't go above the set value.
 	 */
-	if (IS_VALLEYVIEW(dev)) {
-		val = intel_freq_opcode(dev_priv, val);
-
-		hw_max = dev_priv->rps.max_freq;
-		hw_min = dev_priv->rps.min_freq;
-	} else {
-		val = intel_freq_opcode(dev_priv, val);
+	val = intel_freq_opcode(dev_priv, val);
 
-		rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
-		hw_max = dev_priv->rps.max_freq;
-		hw_min = (rp_state_cap >> 16) & 0xff;
-	}
+	hw_max = dev_priv->rps.max_freq;
+	hw_min = dev_priv->rps.min_freq;
 
 	if (val < hw_min || val > hw_max || val < dev_priv->rps.min_freq_softlimit) {
 		mutex_unlock(&dev_priv->rps.hw_lock);
@@ -4266,7 +4253,7 @@  i915_min_freq_set(void *data, u64 val)
 {
 	struct drm_device *dev = data;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 rp_state_cap, hw_max, hw_min;
+	u32 hw_max, hw_min;
 	int ret;
 
 	if (INTEL_INFO(dev)->gen < 6)
@@ -4283,18 +4270,10 @@  i915_min_freq_set(void *data, u64 val)
 	/*
 	 * Turbo will still be enabled, but won't go below the set value.
 	 */
-	if (IS_VALLEYVIEW(dev)) {
-		val = intel_freq_opcode(dev_priv, val);
+	val = intel_freq_opcode(dev_priv, val);
 
-		hw_max = dev_priv->rps.max_freq;
-		hw_min = dev_priv->rps.min_freq;
-	} else {
-		val = intel_freq_opcode(dev_priv, val);
-
-		rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
-		hw_max = dev_priv->rps.max_freq;
-		hw_min = (rp_state_cap >> 16) & 0xff;
-	}
+	hw_max = dev_priv->rps.max_freq;
+	hw_min = dev_priv->rps.min_freq;
 
 	if (val < hw_min || val > hw_max || val > dev_priv->rps.max_freq_softlimit) {
 		mutex_unlock(&dev_priv->rps.hw_lock);
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index cdc9da0..186ab95 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -487,38 +487,17 @@  static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr
 	struct drm_minor *minor = dev_to_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;
-	intel_runtime_pm_get(dev_priv);
-	rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
-	intel_runtime_pm_put(dev_priv);
-	mutex_unlock(&dev->struct_mutex);
+	u32 val;
 
-	if (attr == &dev_attr_gt_RP0_freq_mhz) {
-		if (IS_VALLEYVIEW(dev))
-			val = intel_gpu_freq(dev_priv, dev_priv->rps.rp0_freq);
-		else
-			val = intel_gpu_freq(dev_priv,
-					     ((rp_state_cap & 0x0000ff) >> 0));
-	} else if (attr == &dev_attr_gt_RP1_freq_mhz) {
-		if (IS_VALLEYVIEW(dev))
-			val = intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq);
-		else
-			val = intel_gpu_freq(dev_priv,
-					     ((rp_state_cap & 0x00ff00) >> 8));
-	} else if (attr == &dev_attr_gt_RPn_freq_mhz) {
-		if (IS_VALLEYVIEW(dev))
-			val = intel_gpu_freq(dev_priv, dev_priv->rps.min_freq);
-		else
-			val = intel_gpu_freq(dev_priv,
-					     ((rp_state_cap & 0xff0000) >> 16));
-	} else {
+	if (attr == &dev_attr_gt_RP0_freq_mhz)
+		val = intel_gpu_freq(dev_priv, dev_priv->rps.rp0_freq);
+	else if (attr == &dev_attr_gt_RP1_freq_mhz)
+		val = intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq);
+	else if (attr == &dev_attr_gt_RPn_freq_mhz)
+		val = intel_gpu_freq(dev_priv, dev_priv->rps.min_freq);
+	else
 		BUG();
-	}
+
 	return snprintf(buf, PAGE_SIZE, "%d\n", val);
 }