diff mbox

[8/9] drm/ips: move drps/ips/ilk related variables into dev_priv->ips

Message ID 1343165630-21604-9-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter July 24, 2012, 9:33 p.m. UTC 
Like with the equivalent change for gen6+ rps state, this helps in
clarifying the code (and in fixing a few places that have fallen through
the cracks in the locking review).

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h |   36 +++++++++-------
 drivers/gpu/drm/i915/i915_irq.c |   20 ++++-----
 drivers/gpu/drm/i915/intel_pm.c |   90 ++++++++++++++++++---------------------
 3 files changed, 72 insertions(+), 74 deletions(-)

Comments

Ben Widawsky July 25, 2012, 9:25 p.m. UTC | #1 
On Tue, 24 Jul 2012 23:33:49 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Like with the equivalent change for gen6+ rps state, this helps in
> clarifying the code (and in fixing a few places that have fallen through
> the cracks in the locking review).
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I don't think this patch is necessary, and doesn't belong in the series.
The series was about fixing a locking problem. If you want to submit
this as a separate patch, I'd prefer it.

If you're really determined to keep it, I'd roll it into the earlier
patches that did the rps renaming.
Daniel Vetter July 25, 2012, 9:32 p.m. UTC | #2 
On Wed, Jul 25, 2012 at 02:25:20PM -0700, Ben Widawsky wrote:
> On Tue, 24 Jul 2012 23:33:49 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > Like with the equivalent change for gen6+ rps state, this helps in
> > clarifying the code (and in fixing a few places that have fallen through
> > the cracks in the locking review).
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I don't think this patch is necessary, and doesn't belong in the series.
> The series was about fixing a locking problem. If you want to submit
> this as a separate patch, I'd prefer it.
> 
> If you're really determined to keep it, I'd roll it into the earlier
> patches that did the rps renaming.

Well, you've already smashed your r-b onto the equivalent patch for the
gen6+ rps code. But the real reason this belongs to this series is that
I've used this rename (and the rps one) to actually figure out (with the
help of the compiler) what is actually touched where and which parts
belong together. 'Cause the current code is a rather decent mess.

-Daniel
Ben Widawsky July 25, 2012, 11:52 p.m. UTC | #3 
On Wed, 25 Jul 2012 23:32:16 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Jul 25, 2012 at 02:25:20PM -0700, Ben Widawsky wrote:
> > On Tue, 24 Jul 2012 23:33:49 +0200
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > 
> > > Like with the equivalent change for gen6+ rps state, this helps in
> > > clarifying the code (and in fixing a few places that have fallen through
> > > the cracks in the locking review).
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > I don't think this patch is necessary, and doesn't belong in the series.
> > The series was about fixing a locking problem. If you want to submit
> > this as a separate patch, I'd prefer it.
> > 
> > If you're really determined to keep it, I'd roll it into the earlier
> > patches that did the rps renaming.
> 
> Well, you've already smashed your r-b onto the equivalent patch for the
> gen6+ rps code. But the real reason this belongs to this series is that
> I've used this rename (and the rps one) to actually figure out (with the
> help of the compiler) what is actually touched where and which parts
> belong together. 'Cause the current code is a rather decent mess.
> 
> -Daniel

You've shot down quite a few patches of mine (usually assertions) which
I've used for similar, 'this helped me track down an issue' purposes.

In any case, the r-b on the other one is because you're restructuring
the code you want to fix, before you fix it. That is fine. As I said,
if you want to put this as a beautifier, I don't think it belongs in
the series.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b104b7f..2a1f861 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -808,22 +808,26 @@  typedef struct drm_i915_private {
 		u8 max_delay;
 	} rps;
 
-
-	u8 cur_delay;
-	u8 min_delay;
-	u8 max_delay;
-	u8 fmax;
-	u8 fstart;
-
-	u64 last_count1;
-	unsigned long last_time1;
-	unsigned long chipset_power;
-	u64 last_count2;
-	struct timespec last_time2;
-	unsigned long gfx_power;
-	int c_m;
-	int r_t;
-	u8 corr;
+	/* ilk-only ips/rps state. Everything in here is protected by the global
+	 * mchdev_lock in intel_pm.c */
+	struct {
+		u8 cur_delay;
+		u8 min_delay;
+		u8 max_delay;
+		u8 fmax;
+		u8 fstart;
+
+		u64 last_count1;
+		unsigned long last_time1;
+		unsigned long chipset_power;
+		u64 last_count2;
+		struct timespec last_time2;
+		unsigned long gfx_power;
+		u8 corr;
+
+		int c_m;
+		int r_t;
+	} ips;
 
 	enum no_fbc_reason no_fbc_reason;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 75c631d..0b75d22 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -310,7 +310,7 @@  static void ironlake_handle_rps_change(struct drm_device *dev)
 
 	I915_WRITE16(MEMINTRSTS, I915_READ(MEMINTRSTS));
 
-	new_delay = dev_priv->cur_delay;
+	new_delay = dev_priv->ips.cur_delay;
 
 	I915_WRITE16(MEMINTRSTS, MEMINT_EVAL_CHG);
 	busy_up = I915_READ(RCPREVBSYTUPAVG);
@@ -320,19 +320,19 @@  static void ironlake_handle_rps_change(struct drm_device *dev)
 
 	/* Handle RCS change request from hw */
 	if (busy_up > max_avg) {
-		if (dev_priv->cur_delay != dev_priv->max_delay)
-			new_delay = dev_priv->cur_delay - 1;
-		if (new_delay < dev_priv->max_delay)
-			new_delay = dev_priv->max_delay;
+		if (dev_priv->ips.cur_delay != dev_priv->ips.max_delay)
+			new_delay = dev_priv->ips.cur_delay - 1;
+		if (new_delay < dev_priv->ips.max_delay)
+			new_delay = dev_priv->ips.max_delay;
 	} else if (busy_down < min_avg) {
-		if (dev_priv->cur_delay != dev_priv->min_delay)
-			new_delay = dev_priv->cur_delay + 1;
-		if (new_delay > dev_priv->min_delay)
-			new_delay = dev_priv->min_delay;
+		if (dev_priv->ips.cur_delay != dev_priv->ips.min_delay)
+			new_delay = dev_priv->ips.cur_delay + 1;
+		if (new_delay > dev_priv->ips.min_delay)
+			new_delay = dev_priv->ips.min_delay;
 	}
 
 	if (ironlake_set_drps(dev, new_delay))
-		dev_priv->cur_delay = new_delay;
+		dev_priv->ips.cur_delay = new_delay;
 
 	spin_unlock_irqrestore(&mchdev_lock, flags);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e582b50..0056362 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -593,7 +593,7 @@  static void i915_ironlake_get_mem_freq(struct drm_device *dev)
 		break;
 	}
 
-	dev_priv->r_t = dev_priv->mem_freq;
+	dev_priv->ips.r_t = dev_priv->mem_freq;
 
 	switch (csipll & 0x3ff) {
 	case 0x00c:
@@ -625,11 +625,11 @@  static void i915_ironlake_get_mem_freq(struct drm_device *dev)
 	}
 
 	if (dev_priv->fsb_freq == 3200) {
-		dev_priv->c_m = 0;
+		dev_priv->ips.c_m = 0;
 	} else if (dev_priv->fsb_freq > 3200 && dev_priv->fsb_freq <= 4800) {
-		dev_priv->c_m = 1;
+		dev_priv->ips.c_m = 1;
 	} else {
-		dev_priv->c_m = 2;
+		dev_priv->ips.c_m = 2;
 	}
 }
 
@@ -2162,12 +2162,6 @@  err_unref:
 
 /**
  * Lock protecting IPS related data structures
- *   - i915_mch_dev
- *   - dev_priv->max_delay
- *   - dev_priv->min_delay
- *   - dev_priv->fmax
- *   - dev_priv->gpu_busy
- *   - dev_priv->gfx_power
  */
 DEFINE_SPINLOCK(mchdev_lock);
 
@@ -2230,12 +2224,12 @@  static void ironlake_enable_drps(struct drm_device *dev)
 	vstart = (I915_READ(PXVFREQ_BASE + (fstart * 4)) & PXVFREQ_PX_MASK) >>
 		PXVFREQ_PX_SHIFT;
 
-	dev_priv->fmax = fmax; /* IPS callback will increase this */
-	dev_priv->fstart = fstart;
+	dev_priv->ips.fmax = fmax; /* IPS callback will increase this */
+	dev_priv->ips.fstart = fstart;
 
-	dev_priv->max_delay = fstart;
-	dev_priv->min_delay = fmin;
-	dev_priv->cur_delay = fstart;
+	dev_priv->ips.max_delay = fstart;
+	dev_priv->ips.min_delay = fmin;
+	dev_priv->ips.cur_delay = fstart;
 
 	DRM_DEBUG_DRIVER("fmax: %d, fmin: %d, fstart: %d\n",
 			 fmax, fmin, fstart);
@@ -2258,11 +2252,11 @@  static void ironlake_enable_drps(struct drm_device *dev)
 
 	ironlake_set_drps(dev, fstart);
 
-	dev_priv->last_count1 = I915_READ(0x112e4) + I915_READ(0x112e8) +
+	dev_priv->ips.last_count1 = I915_READ(0x112e4) + I915_READ(0x112e8) +
 		I915_READ(0x112e0);
-	dev_priv->last_time1 = jiffies_to_msecs(jiffies);
-	dev_priv->last_count2 = I915_READ(0x112f4);
-	getrawmonotonic(&dev_priv->last_time2);
+	dev_priv->ips.last_time1 = jiffies_to_msecs(jiffies);
+	dev_priv->ips.last_count2 = I915_READ(0x112f4);
+	getrawmonotonic(&dev_priv->ips.last_time2);
 
 	spin_unlock_irq(&mchdev_lock);
 }
@@ -2284,7 +2278,7 @@  static void ironlake_disable_drps(struct drm_device *dev)
 	I915_WRITE(DEIMR, I915_READ(DEIMR) | DE_PCU_EVENT);
 
 	/* Go back to the starting frequency */
-	ironlake_set_drps(dev, dev_priv->fstart);
+	ironlake_set_drps(dev, dev_priv->ips.fstart);
 	mdelay(1);
 	rgvswctl |= MEMCTL_CMD_STS;
 	I915_WRITE(MEMSWCTL, rgvswctl);
@@ -2730,7 +2724,7 @@  unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 
 	assert_spin_locked(&mchdev_lock);
 
-	diff1 = now - dev_priv->last_time1;
+	diff1 = now - dev_priv->ips.last_time1;
 
 	/* Prevent division-by-zero if we are asking too fast.
 	 * Also, we don't get interesting results if we are polling
@@ -2738,7 +2732,7 @@  unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 	 * in such cases.
 	 */
 	if (diff1 <= 10)
-		return dev_priv->chipset_power;
+		return dev_priv->ips.chipset_power;
 
 	count1 = I915_READ(DMIEC);
 	count2 = I915_READ(DDREC);
@@ -2747,16 +2741,16 @@  unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 	total_count = count1 + count2 + count3;
 
 	/* FIXME: handle per-counter overflow */
-	if (total_count < dev_priv->last_count1) {
-		diff = ~0UL - dev_priv->last_count1;
+	if (total_count < dev_priv->ips.last_count1) {
+		diff = ~0UL - dev_priv->ips.last_count1;
 		diff += total_count;
 	} else {
-		diff = total_count - dev_priv->last_count1;
+		diff = total_count - dev_priv->ips.last_count1;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(cparams); i++) {
-		if (cparams[i].i == dev_priv->c_m &&
-		    cparams[i].t == dev_priv->r_t) {
+		if (cparams[i].i == dev_priv->ips.c_m &&
+		    cparams[i].t == dev_priv->ips.r_t) {
 			m = cparams[i].m;
 			c = cparams[i].c;
 			break;
@@ -2767,10 +2761,10 @@  unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 	ret = ((m * diff) + c);
 	ret = div_u64(ret, 10);
 
-	dev_priv->last_count1 = total_count;
-	dev_priv->last_time1 = now;
+	dev_priv->ips.last_count1 = total_count;
+	dev_priv->ips.last_time1 = now;
 
-	dev_priv->chipset_power = ret;
+	dev_priv->ips.chipset_power = ret;
 
 	return ret;
 }
@@ -2941,7 +2935,7 @@  void __i915_update_gfx_val(struct drm_i915_private *dev_priv)
 	assert_spin_locked(&mchdev_lock);
 
 	getrawmonotonic(&now);
-	diff1 = timespec_sub(now, dev_priv->last_time2);
+	diff1 = timespec_sub(now, dev_priv->ips.last_time2);
 
 	/* Don't divide by 0 */
 	diffms = diff1.tv_sec * 1000 + diff1.tv_nsec / 1000000;
@@ -2950,20 +2944,20 @@  void __i915_update_gfx_val(struct drm_i915_private *dev_priv)
 
 	count = I915_READ(GFXEC);
 
-	if (count < dev_priv->last_count2) {
-		diff = ~0UL - dev_priv->last_count2;
+	if (count < dev_priv->ips.last_count2) {
+		diff = ~0UL - dev_priv->ips.last_count2;
 		diff += count;
 	} else {
-		diff = count - dev_priv->last_count2;
+		diff = count - dev_priv->ips.last_count2;
 	}
 
-	dev_priv->last_count2 = count;
-	dev_priv->last_time2 = now;
+	dev_priv->ips.last_count2 = count;
+	dev_priv->ips.last_time2 = now;
 
 	/* More magic constants... */
 	diff = diff * 1181;
 	diff = div_u64(diff, diffms * 10);
-	dev_priv->gfx_power = diff;
+	dev_priv->ips.gfx_power = diff;
 }
 
 void i915_update_gfx_val(struct drm_i915_private *dev_priv)
@@ -3005,14 +2999,14 @@  unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
 
 	corr = corr * ((150142 * state1) / 10000 - 78642);
 	corr /= 100000;
-	corr2 = (corr * dev_priv->corr);
+	corr2 = (corr * dev_priv->ips.corr);
 
 	state2 = (corr2 * state1) / 10000;
 	state2 /= 100; /* convert to mW */
 
 	__i915_update_gfx_val(dev_priv);
 
-	return dev_priv->gfx_power + state2;
+	return dev_priv->ips.gfx_power + state2;
 }
 
 /**
@@ -3060,8 +3054,8 @@  bool i915_gpu_raise(void)
 	}
 	dev_priv = i915_mch_dev;
 
-	if (dev_priv->max_delay > dev_priv->fmax)
-		dev_priv->max_delay--;
+	if (dev_priv->ips.max_delay > dev_priv->ips.fmax)
+		dev_priv->ips.max_delay--;
 
 out_unlock:
 	spin_unlock_irq(&mchdev_lock);
@@ -3088,8 +3082,8 @@  bool i915_gpu_lower(void)
 	}
 	dev_priv = i915_mch_dev;
 
-	if (dev_priv->max_delay < dev_priv->min_delay)
-		dev_priv->max_delay++;
+	if (dev_priv->ips.max_delay < dev_priv->ips.min_delay)
+		dev_priv->ips.max_delay++;
 
 out_unlock:
 	spin_unlock_irq(&mchdev_lock);
@@ -3143,9 +3137,9 @@  bool i915_gpu_turbo_disable(void)
 	}
 	dev_priv = i915_mch_dev;
 
-	dev_priv->max_delay = dev_priv->fstart;
+	dev_priv->ips.max_delay = dev_priv->ips.fstart;
 
-	if (!ironlake_set_drps(dev_priv->dev, dev_priv->fstart))
+	if (!ironlake_set_drps(dev_priv->dev, dev_priv->ips.fstart))
 		ret = false;
 
 out_unlock:
@@ -3258,7 +3252,7 @@  static void intel_init_emon(struct drm_device *dev)
 
 	lcfuse = I915_READ(LCFUSE02);
 
-	dev_priv->corr = (lcfuse & LCFUSE_HIV_MASK);
+	dev_priv->ips.corr = (lcfuse & LCFUSE_HIV_MASK);
 }
 
 void intel_disable_gt_powersave(struct drm_device *dev)
@@ -3783,8 +3777,8 @@  static void gen6_sanitize_pm(struct drm_device *dev)
 	 */
 	limits &= ~(0x3f << 16 | 0x3f << 24);
 	delay = dev_priv->rps.cur_delay;
-	if (delay < dev_priv->max_delay)
-		limits |= (dev_priv->max_delay & 0x3f) << 24;
+	if (delay < dev_priv->ips.max_delay)
+		limits |= (dev_priv->ips.max_delay & 0x3f) << 24;
 	if (delay > dev_priv->rps.min_delay)
 		limits |= (dev_priv->rps.min_delay & 0x3f) << 16;