[Intel-gfx] [PATCH] drm/i915: add dynamic performance control support for Ironlake

Matthew Garrett mjg59 at srcf.ucam.org
Tue Feb 2 19:30:47 CET 2010


On Wed, Jan 27, 2010 at 12:10:37PM -0800, Jesse Barnes wrote:

> v2 no longer hangs my Calpella when RPS interrupts are enabled.  I was
> missing a bit in the register write in the handle_rps_change routine.
> Now I just have to figure out why X hangs on my Calpella with the
> current for-linus tree (with or without this patch).

My Calpella now also boots with this code enabled, so it's certainly no 
longer clearly harmful. Quick comments:

> +static int i915_cur_delayinfo(struct seq_file *m, void *unused)
> +{
> +	struct drm_info_node *node = (struct drm_info_node *) m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	drm_i915_private_t *dev_priv = dev->dev_private;
> +	u16 rgvswctl = I915_READ16(MEMSWCTL);
> +
> +	seq_printf(m, "Last command: 0x%01x\n", (rgvswctl >> 13) & 0x3);
> +	seq_printf(m, "Command status: %d\n", (rgvswctl >> 12) & 1);
> +	seq_printf(m, "P%d DELAY 0x%02x\n", (rgvswctl >> 8) & 0xf,
> +		   rgvswctl & 0x3f);
> +
> +	return 0;
> +}

Some info on what the debugfs files actually mean would be nice.

> @@ -549,6 +549,11 @@ static int __init i915_init(void)
>  		driver.driver_features &= ~DRIVER_MODESET;
>  #endif
>  
> +	if (!(driver.driver_features & DRIVER_MODESET)) {
> +		driver.suspend = i915_suspend;
> +		driver.resume = i915_resume;
> +	}
> +

Is this directly related?

> +static void i915_handle_rps_change(struct drm_device *dev)
> +{
> +	drm_i915_private_t *dev_priv = dev->dev_private;
> +	u32 slow_up, slow_down, max_avg, min_avg;
> +	u16 rgvswctl;
> +	u8 new_delay = dev_priv->cur_delay;
> +
> +	I915_WRITE(MEMINTRSTS, I915_READ(MEMINTRSTS) & ~MEMINT_EVAL_CHG);
> +	slow_up = I915_READ(RCPREVBSYTUPAVG);
> +	slow_down = I915_READ(RCPREVBSYTDNAVG);

No, really. This is dumb. You've clearly done s/busy/slow/ on this code, 
and I have no idea why. The variable names make *no* sense as a result, 
and the comments and printk are worse than useless. It just looks like 
some kind of pointless obfuscation.

> +	if (de_iir & DE_PCU_EVENT) {
> +		I915_WRITE(MEMINTRSTS, I915_READ(MEMINTRSTS));
> +		i915_handle_rps_change(dev);

You modify MEMINTRSTS here and at the start of handle_rps_change. Are 
both writes necessary? Should the call be conditional on one of these 
bits being set? 

> +#define GCFGC2	0xda

Doesn't seem to be used.

> -#define   I915_GMCH_THERMAL_SENSOR_EVENT_INTERRUPT	(1<<14)
> +#define   I915_GMCH_THERMAL_SENSOR_EVENT_INTERRUPT	(1<<14) /* p-state */

Is this only used for p-states? It also doesn't seem to be used in this 
patch, so I'm guessing this is 965?

>  			I915_WRITE(PWRCTXA, obj_priv->gtt_offset | PWRCTX_EN);
> -			I915_WRITE(MCHBAR_RENDER_STANDBY,
> -				   I915_READ(MCHBAR_RENDER_STANDBY) & ~RCX_SW_EXIT);
> +			I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) &
> +				   ~RCX_SW_EXIT);

Why are we renaming RENDER_STANDBY to RSTDBYCTL?

> -	if (IS_I85X(dev))
> -		pci_read_config_word(dev->pdev, HPLLCC, &dev_priv->orig_clock);
> -	else if (IS_I9XX(dev) || IS_G4X(dev))
> -		pci_read_config_word(dev->pdev, GCFGC, &dev_priv->orig_clock);
> -

Seems unrelated, but since we don't use it for anything now anyway... If 
you're going to do this, can you blow away the orig_clock struct member 
as well?

Other than the stylistic issues, the following patch on top of this 
would leave me happy:

commit a91ea04c100e8de21d0cd35b0f8b3992ec54a268
Author: Matthew Garrett <mjg at redhat.com>
Date:   Tue Feb 2 13:26:17 2010 -0500

i915: Deobfuscate the render p-state obfuscation

The ironlake render p-state support includes some rather odd variable
names. Clean them up in order to improve the readability of the code.

Signed-off-by: Matthew Garrett <mjg at redhat.com>

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 301d77c..8532aea 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -452,7 +452,7 @@ typedef struct drm_i915_private {
 	u32 savePIPEB_DATA_N1;
 	u32 savePIPEB_LINK_M1;
 	u32 savePIPEB_LINK_N1;
-	u32 saveRSTDBYCTL;
+	u32 saveMCHBAR_RENDER_STANDBY;
 
 	struct {
 		struct drm_mm gtt_space;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 18370ac..3559afa 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -272,23 +272,23 @@ static void i915_hotplug_work_func(struct work_struct *work)
 static void i915_handle_rps_change(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	u32 slow_up, slow_down, max_avg, min_avg;
+	u32 busy_up, busy_down, max_avg, min_avg;
 	u16 rgvswctl;
 	u8 new_delay = dev_priv->cur_delay;
 
 	I915_WRITE(MEMINTRSTS, I915_READ(MEMINTRSTS) & ~MEMINT_EVAL_CHG);
-	slow_up = I915_READ(RCPREVBSYTUPAVG);
-	slow_down = I915_READ(RCPREVBSYTDNAVG);
+	busy_up = I915_READ(RCPREVBSYTUPAVG);
+	busy_down = I915_READ(RCPREVBSYTDNAVG);
 	max_avg = I915_READ(RCBMAXAVG);
 	min_avg = I915_READ(RCBMINAVG);
 
 	/* Handle RCS change request from hw */
-	if (slow_up > max_avg) {
+	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;
-	} else if (slow_down < min_avg) {
+	} 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)
@@ -300,8 +300,8 @@ static void i915_handle_rps_change(struct drm_device *dev)
 
 	rgvswctl = I915_READ(MEMSWCTL);
 	if (rgvswctl & MEMCTL_CMD_STS) {
-		DRM_ERROR("gpu slow, RCS change rejected\n");
-		return; /* still slow with another command */
+		DRM_ERROR("gpu busy, RCS change rejected\n");
+		return; /* still busy with another command */
 	}
 
 	/* Program the new state */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 487b818..30cc9c9 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -903,7 +903,7 @@
 #define RCBMINAVG		0x111a0
 #define RCUPEI			0x111b0
 #define RCDNEI			0x111b4
-#define RSTDBYCTL		0x111b8
+#define MCHBAR_RENDER_STANDBY		0x111b8
 #define   RCX_SW_EXIT		(1<<23)
 #define   RSX_STATUS_MASK	0x00700000
 #define VIDCTL			0x111c0
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 2c34664..ac0d1a7 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -682,7 +682,8 @@ void i915_restore_display(struct drm_device *dev)
 		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->savePP_OFF_DELAYS);
 		I915_WRITE(PCH_PP_DIVISOR, dev_priv->savePP_DIVISOR);
 		I915_WRITE(PCH_PP_CONTROL, dev_priv->savePP_CONTROL);
-		I915_WRITE(RSTDBYCTL, dev_priv->saveRSTDBYCTL);
+		I915_WRITE(MCHBAR_RENDER_STANDBY,
+			   dev_priv->saveMCHBAR_RENDER_STANDBY);
 	} else {
 		I915_WRITE(PFIT_PGM_RATIOS, dev_priv->savePFIT_PGM_RATIOS);
 		I915_WRITE(BLC_PWM_CTL, dev_priv->saveBLC_PWM_CTL);
@@ -746,7 +747,8 @@ int i915_save_state(struct drm_device *dev)
 		dev_priv->saveGTIMR = I915_READ(GTIMR);
 		dev_priv->saveFDI_RXA_IMR = I915_READ(FDI_RXA_IMR);
 		dev_priv->saveFDI_RXB_IMR = I915_READ(FDI_RXB_IMR);
-		dev_priv->saveRSTDBYCTL = I915_READ(RSTDBYCTL);
+		dev_priv->saveMCHBAR_RENDER_STANDBY =
+			I915_READ(MCHBAR_RENDER_STANDBY);
 	} else {
 		dev_priv->saveIER = I915_READ(IER);
 		dev_priv->saveIMR = I915_READ(IMR);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fd36dfe..d014132 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4591,8 +4591,8 @@ void intel_init_clock_gating(struct drm_device *dev)
 
 		if (obj_priv) {
 			I915_WRITE(PWRCTXA, obj_priv->gtt_offset | PWRCTX_EN);
-			I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) &
-				   ~RCX_SW_EXIT);
+			I915_WRITE(MCHBAR_RENDER_STANDBY,
+				   I915_READ(MCHBAR_RENDER_STANDBY) & ~RCX_SW_EXIT);
 		}
 	}
 }

-- 
Matthew Garrett | mjg59 at srcf.ucam.org



More information about the Intel-gfx mailing list