[Intel-gfx] [PATCH]DRM i915: IGD big FIFO support

[Jesse Barnes]

On Tue, 2 Jun 2009 15:49:27 +0800
Shaohua Li <shaohua.li at intel.com> wrote:

> On Mon, Jun 01, 2009 at 05:26:01PM +0800, Jesse Barnes wrote:
> > On Mon, 25 May 2009 10:28:36 +0800
> > Shaohua Li <shaohua.li at intel.com> wrote:
> > 
> > > On Sat, May 23, 2009 at 03:39:51AM +0800, Eric Anholt wrote:
> > > > On Mon, 2009-05-18 at 10:44 +0800, Shaohua Li wrote:
> > > > > +	addr = ioremap(mch_bar, HOSTBAR_SIZE);
> > > > > +	if (!addr)
> > > > > +		return -ENOMEM;
> > > > > +	tmp = *(u32 *)(addr + 0xc00);
> > > > > +	iounmap(addr);
> > > > 
> > > > MCHBAR mapping is tricky.  I think we'll need to use the same
> > > > code here as i915_gem_tiling.c, and that's changing in 2.6.31 to
> > > > successfully enable it (safely) more often.
> > > Makes sense, I fixed the issues you pointed out.
> > > 
> > > 
> > > Big FIFO is a feature to put memory into self-refresh mode when
> > > CPU enters C3+ state. Gfx has a FIFO to buffer memory access,
> > > when the watermark of the FIFO is under threshold, Gfx doesn't
> > > need access memory, so at that time memory can be put into
> > > self-refresh mode.
> > > 
> > > The watermark calculation is based on CPU C3 state exit latency.
> > > If watermark is wrong, when CPU enters C3+, display will be
> > > broken or flicker.
> > > 
> > > I had a power measurement about the feature:
> > > environment: 1920x1400 display, Atom CPU with C4 enabled, system
> > > FSB is 667 and memory is DDR2 667. Launch X, and gives system
> > > several minutes to settle down, then test the power of the whole
> > > system in idle time: without big fifo, idle power is 19.8w
> > > with it, idle power is 18.6w
> > > 
> > > The patch doesn't enable HPLL off for CxSR. Last time I heard it's
> > > broken in current IGD chip, if it works, then I'll add it later.
> > 
> > Shaohua, can you take a look at this patch and see if it makes
> > sense to include in yours?  I think we could probably share the
> > latency tables & math (haven't checked mine in this patch yet, it's
> > untested on platforms needing FIFO adjustment) at the very least...
> 
> the latency talbe might be ok. the math looks different. My patch is
> using clock*pixel_size*latency/cachline_size for SR WM, but looks
> yours takes a different approach for SR WM. does the math depends on
> chipset? The register write is different. Most fields of FW_BLC and
> FW_BLC_SELF is reserved in IGD and my patch is using FW1-FW3.

No, the SR WM should probably be shared; you have both docs right?
It's quite possible I have the wrong formula.

A few comments to help integrate things:
  - the DPMS callout should be like mine, just call it
    update_watermarks or something instead
  - the new IGD_* constants should be in i915_reg.h probably
  - the read frequency function should use new defines for all the
    registers read/written
  - see anholt's comment about MCHBAR; we probably want to put this in
    the tiling.c file and run it at load time rather than mode set time
  - the update_watermarks should either be a function pointer to set
    the chip specific watermark regs or should handle all cases along
    the lines of my patch

I'd like to get both bits of code into 2.6.31 though, so we should get
them integrated soon.

Thanks,
Jesse