[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