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

[Shaohua Li]

On Thu, Jun 18, 2009 at 08:41:04AM +0800, Jesse Barnes wrote:
> On Wed, 10 Jun 2009 14:52:55 +0800
> Shaohua Li <shaohua.li at intel.com> wrote:
> 
> > On Wed, Jun 10, 2009 at 06:05:19AM +0800, Jesse Barnes wrote:
> > > 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.
> > I did some cleanup and try to make it (hopefully) generic. please
> > check how to integrate yours.
> 
> Here's the patch I've been testing...  In looking at integrating the
> code I found some bugs:
>   - intel_crtc->plane wasn't being set properly
>   - the FIFO entry calculation was overflowing
>   - the CXSR code assumes only one pipe is on (is this safe?)
Manual says CxSR can work with one pipe on.

>   - watermark update needs to be called at dpms on/off time as well,
>     since we could go from a one/two pipe config to one/two/zero at
>     that time
That's fine. In previous check, it appears dpms on/off might be called in
disabled pipe, so my patch bypass cxsr set for disabled pipe. Any reason
we shouldn't do this?
Also, in watermark update, my patch doesn't enable CxSR. And I do enable/disable
CxSR in dpms on/off, this seems safer to me. Below patch seems remove the logic
and doesn't enable CxSR.

> I think there's room for integration in my update_watermarks function.
> I have if (...) blocks for the various chip types, we could probably
> put IGD there, and split out the actual register writes into separate
> functions.  I've also got an if (...) block for a single pipe
> configuration, which is where we'd want to update the self-refresh
> anyway, right?  Also, it looks like our entry calculation could be
> mostly unified...  I'd just need to set guards & minimums like you have
> (which is a good idea).
I'm fine with this. 

Thanks,
Shaohua