Re: [PATCH] drm/i915: Fix offset page-flips on i965+

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Fix offset page-flips on i965+
Date: Sun, 8 Aug 2010 14:24:11 +0200	[thread overview]
Message-ID: <20100808122410.GA15846@viiv.ffwll.ch> (raw)
In-Reply-To: <89k83a$974h4e@azsmga001.ch.intel.com>

On Sun, Aug 08, 2010 at 12:39:00PM +0100, Chris Wilson wrote:
> On Sun,  8 Aug 2010 10:20:25 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >  		OUT_RING(MI_DISPLAY_FLIP |
> >  			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> > -		OUT_RING(fb->pitch);
> > -		OUT_RING(offset | obj_priv->tiling_mode);
> > -		pipesrc = I915_READ(pipesrc_reg); 
> > -		OUT_RING(pipesrc & 0x0fff0fff);
> > +		OUT_RING(fb->pitch | obj_priv->tiling_mode);
> > +		/* i965+ uses the linear or tiled offsets from the
> > +		 * Display Registers (which do not change across a page-flip)
> > +		 * so we need only reprogram the base address.
> > +		 */
> > +		OUT_RING(obj_priv->gtt_offset);
> 
> There's a discrepancy here between our internal docs and the publish PDFs
> for MI_DISPLAY_FLIP. In our docs, we have pitch | tiling. In the
> PDF, it's address | tiling. There are other inconsistencies in the
> bitfields surround this in the PDF, so it is worth a second set of eyes
> checking this.

Just fyi, here's what I've distilled from the published docs. In short,
it's mess.

i965/G45 documentation:
dword 0, bit 22:
"This field specifies whether the flip operation should be performed
asynchronously to vertical retrace."

But then goes on to define:
0 = Asynchronous flip, synchronize with vblank.
1 = Syncrhronous flip, do "as soon as possible"
This seems to be the definition actually used (synchronous wrt command
execution), not the one in the intro (synchronous wrt vblank).

Whatever, current code sets this bit to 0. Docs then mention that for
asynchrouns flips, pitch and tiling mode get ignored (but should be set to
the same values for all flips). Pitch is in dword 1, tiling mode dword 2,
bit 0.

HD documentation:
dword0, bit 22: Should always be set to 1 due to hw limitations. Current
code is not doing this.

Sync/async seem to mean the same as in the i965/G45 docs, save for
dword 2, bit 0: (This is the old tiling mode bit that get's ignored)
DevSNB+, Flip type
0 = Sync Flip (synced to vblank)
1 = Async Flip (immediatly)
It also mentions that this only works for X-Tiled buffers (probably only
the immediate flip, for the rest tiling is most likely ignored, anyway).
So the tiling_mode be seems unnecessary and could hence be recycled.

Current code doesn't special-case ilk, so I think this only applies to
Sandybridge and later (the set dword 0, bit 22 always to 1, too). After
all, my i5 can do pageflips ;)

In conclusion I think we need an if (IS_SNB(dev)) that sets dword 0, bit
22 to 1 and ensures that dword 2, bit 0 is zero. For the rest of of the
IS_I965G branch we might as well write 0 instead of pitch and tiling_mode.

Whatever, current code seems to work.

-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48