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
