[Intel-gfx] [PATCH] drm/i915: swizzling support for snb/ivb

[Daniel Vetter]

On Sat, Feb 04, 2012 at 09:59:57PM +0100, Eric Anholt wrote:
> On Thu,  2 Feb 2012 09:58:12 +0100, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> > We have to do this manually. Somebody had a Great Idea.
> > 
> > I've measured speed-ups just a few percent above the noise level
> > (below 5% for the best case), but no slowdows. Chris Wilson measured
> > quite a bit more (10-20% above the usual snb variance) on a more
> > recent and better tuned version of sna, but also recorded a few
> > slow-downs on benchmarks know for uglier amounts of snb-induced
> > variance.
> 
> 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 51a2b0c..86fffd2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3681,12 +3681,31 @@ i915_gem_idle(struct drm_device *dev)
> >  	return 0;
> >  }
> >  
> > +void i915_gem_init_swizzling(struct drm_device *dev)
> > +{
> > +	drm_i915_private_t *dev_priv = dev->dev_private;
> > +
> > +	if (INTEL_INFO(dev)->gen < 6 ||
> > +	    dev_priv->mm.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_NONE)
> > +		return;
> > +
> > +	I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) |
> > +				 DISP_TILE_SURFACE_SWIZZLING);
> > +
> > +	I915_WRITE(TILECTL, I915_READ(TILECTL) | TILECTL_SWZCTL);
> > +	if (IS_GEN6(dev))
> > +		I915_WRITE(ARB_MODE, ARB_MODE_ENABLE(ARB_MODE_SWIZZLE_SNB));
> > +	else
> > +		I915_WRITE(ARB_MODE, ARB_MODE_ENABLE(ARB_MODE_SWIZZLE_IVB));
> > +}
> 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > index 861223b..1a93066 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > @@ -93,8 +93,23 @@ i915_gem_detect_bit_6_swizzle(struct drm_device *dev)
> >  	uint32_t swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN;
> >  
> >  	if (INTEL_INFO(dev)->gen >= 6) {
> > -		swizzle_x = I915_BIT_6_SWIZZLE_NONE;
> > -		swizzle_y = I915_BIT_6_SWIZZLE_NONE;
> > +		uint32_t dimm_c0, dimm_c1;
> > +		dimm_c0 = I915_READ(MAD_DIMM_C0);
> > +		dimm_c1 = I915_READ(MAD_DIMM_C1);
> > +		dimm_c0 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
> > +		dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
> > +		/* Enable swizzling when the channels are populated with
> > +		 * identically sized dimms. We don't need to check the 3rd
> > +		 * channel because no cpu with gpu attached ships in that
> > +		 * configuration. Also, swizzling only makes sense for 2
> > +		 * channels anyway. */
> > +		if (dimm_c0 == dimm_c1) {
> > +			swizzle_x = I915_BIT_6_SWIZZLE_9_10;
> > +			swizzle_y = I915_BIT_6_SWIZZLE_9;
> > +		} else {
> > +			swizzle_x = I915_BIT_6_SWIZZLE_NONE;
> > +			swizzle_y = I915_BIT_6_SWIZZLE_NONE;
> > +		}
> 
> It looks to me like you're making the HW always swizzle bit 6 according
> to 9 or 9/10 in i915_gem_init_swizzling, but you're not performing the
> software side of swizzling in the !dual channel case.  My guess would be
> that when you take your other DIMM the swizzling for pread/pwrite/swap
> goes wrong, and that the answer would be to just not look at dimm sizes.

Hm, I'm a bit confused here ... So let me explain how swizzling on gen6+
works with this patch:

In the reset/bootup state the gpu doesn't do any additional swizzling for
tiled surfaces. You have to explicitly frob the 3 registers for gt, cpu
gtt window and scanout. I presume the gpu is pretty oblivious to what the
memory controller does and swizzling could also be enabled on
single-channel or L-shaped layouts (iirc I haven't tried that though).

Now the patch just detects whether we have symmetrically populated
channels and sets the swizzle stuff accordingly. i915_gem_init_swizzling
gets called at init, gpu reset and resume time and the programms the hw
bits accordingly to the swizzling we want. So
i915_gem_detect_bit_6_swizzle is a bit a mistdenomer on gen6+, but I've
thought it fits in better this way with the existing code-flow.

It's been quite a while since I've stitched this together but iirc I've
tested all the different possibilities by yanking dimms (safe for the 2
dimms per channel case, my laptop doesn't have enough slots for that). And
we have a set of testcase in i-g-t that should test pread/pwrite and
swapping, so I'm positive that we should catch any issues with this
easily.

> Other than that, looks reasonable to me.  Might make it clear what's
> going on in the commit message -- "swizzling support for snb/ivb" made
> me think you were fixing up the software side of bit 6 swizzling, when
> it looks like you're actually enabling bit 6 swizzling for the first
> time.

Yeah, I'll improve the commit message to be a bit less terse and explain
what's actually going on.

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