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

[Jesse Barnes]

On Wed, 6 Jan 2010 21:13:10 +0000
Matthew Garrett <mjg59 at srcf.ucam.org> wrote:
> > -/** GM965 GM45 render standby register */
> > -#define MCHBAR_RENDER_STANDBY	0x111B8
> 
> You've changed this nice readable register name into something that 
> makes me cry inside. Are we able to get human readable definitions?

This one could be RSTDBYCTL, would that stop the tears?

> 
> > +#define PXVFREQ_BASE		0x11110 /* P[0-15]VIDFREQ
> > (0x1114c) (ILK) */
> 
> Ironlake?
> 
> > +#define VIDFREQ1		0x11110 /* VIDFREQ1-4 (0x1111c)
> > (CTG) */
> 
> Cantiga? Ajax went to some efforts to clean up codename use, it'd be 
> nice to keep that consistent.

Sure, I can change those.

> > +#define INTTOEXT_BASE_ILK	0x11300
> > +#define INTTOEXT_BASE		0x11120 /* INTTOEXT1-8
> > (0x1113c) */
> 
> Should there be separate INTTOEXT[1-8] for Iron Lake?

Nah, in fact I can remove the non-Ironlake ones.

> 
> > +	if (IS_IRONLAKE_M(dev))
> > +		ironlake_disable_drps(dev);
> > +
> 
> I know that the rest of the code is pretty much a mess in this
> respect as well, but if this can be extended to 965 support should
> there just be an intel_disable/enable_drps call that's a noop on
> unsupported hardware? Otherwise the conditionals here are going to be
> a nightmare.

Yeah, I just figured adding a hook for it would be premature with just
one chipset supported.

> 
> >  	/* Cache mode state */
> >  	dev_priv->saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);
> 
> Heh. Not sure that comment helps all that much...

True that. :)

> 
> > +	fstart = (rgvmodectl & MEMMODE_FSTART_MASK) >>
> > +		MEMMODE_FSTART_SHIFT;
> > +	vstart = (I915_READ(PXVFREQ_BASE + (fstart * 4)) &
> > PXVFREQ_PX_MASK) >>
> > +		PXVFREQ_PX_SHIFT;
> > +	vstart = 127 - ((I915_READ(MEMSTAT_ILK) &
> > MEMSTAT_VID_MASK) >>
> > +			MEMSTAT_VID_SHIFT);
> > +	vstart = 0x66;
> 
> Pretty sure that's not right.

The second two assignments are dropped in the last patch I sent.

> 
> > +	while (I915_READ(MEMSWCTL) & MEMCTL_CMD_STS)
> > +		;
> 
> Should this be bounded, maybe with a BUG()?

Ah, yes it should.  I'll add a counter.

> > +	DRM_DEBUG_DRIVER("GCFGC: 0x%04x\n", dev_priv->orig_clock);
> > +	if ((dev_priv->orig_clock & 0xf) == 0)
> > +		clock = 167;
> > +	if ((dev_priv->orig_clock & 0xf) == 0)
> > +		clock = 200;
> > +	if ((dev_priv->orig_clock & 0xf) == 0)
> > +		clock = 250;
> > +	if ((dev_priv->orig_clock & 0xf) == 0)
> > +		clock = 333;
> > +	if ((dev_priv->orig_clock & 0xf) == 0)
> > +		clock = 400;
> > +	if ((dev_priv->orig_clock & 0xf) == 0)
> > +		clock = 500;
> > +	if ((dev_priv->orig_clock & 0xf) == 0)
> > +		clock = 667;
> 
> *Really* not convinced by this bit. Should maybe be a switch
> statement, in any case?
> 
> > +	pci_read_config_word(dev->pdev, GCFGC2, &gcfgc2);
> > +	if ((gcfgc2 & 0xf) == 0)
> > +		max_clock = 167;
> > +	if ((gcfgc2 & 0xf) == 0)
> > +		max_clock = 200;
> > +	if ((gcfgc2 & 0xf) == 0)
> > +		max_clock = 250;
> > +	if ((gcfgc2 & 0xf) == 0)
> > +		max_clock = 333;
> > +	if ((gcfgc2 & 0xf) == 0)
> > +		max_clock = 400;
> > +	if ((gcfgc2 & 0xf) == 0)
> > +		max_clock = 500;
> > +	if ((gcfgc2 & 0xf) == 0)
> > +		max_clock = 667;
> 
> Or this. Is gcfgc ignored entirely on Iron Lake?
> 
> I'll hold off trying this until I'm sure it's not going to overclock
> my board horribly :)

All this should probably be removed entirely and added when needed.

It's highly unlikely to do any harm to your system.  If you're running
a really heavy graphics load, the GPU may start taking more power, but
at worst, throttling is likely to kick in to cool things off (if your
system even gets to that point).

You can monitor changes using the debugfs files.

-- 
Jesse Barnes, Intel Open Source Technology Center