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

[Matthew Garrett]

On Wed, Jan 06, 2010 at 12:36:26PM -0800, Jesse Barnes wrote:

> +	u32 slow_up, slow_down, max_avg, min_avg;

"slow_up" and "slow_down" *really* suck as variable names.

> +		DRM_ERROR("gpu slow, RCS change rejected\n");
> +		return; /* still slow with another command */

Not really clear on what this means...

> -/** 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?

> +#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.

> +#define INTTOEXT_BASE_ILK	0x11300
> +#define INTTOEXT_BASE		0x11120 /* INTTOEXT1-8 (0x1113c) */

Should there be separate INTTOEXT[1-8] for Iron Lake?

> +	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.

>  	/* Cache mode state */
>  	dev_priv->saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);

Heh. Not sure that comment helps all that much...

> +	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.

> +	while (I915_READ(MEMSWCTL) & MEMCTL_CMD_STS)
> +		;

Should this be bounded, maybe with a BUG()?

> +	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 :)

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