Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 11 Jun 2014 11:23:26 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Jun 10, 2014 at 12:45:38PM -0700, Jesse Barnes wrote:
> > On Tue, 10 Jun 2014 21:33:27 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > > On Tue, Jun 10, 2014 at 7:27 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > > Yes, that's what I do below... I even added it to the changelog:
> > > > http://patchwork.freedesktop.org/patch/27223/
> > > >
> > > > Did you miss the later hunk in intel_display.c?
> > > >
> > > > What we try to do here is enable swizzling if possible, which we can do
> > > > if no inherited fbs are tiled.
> > > >
> > > > So I think I've done exactly what you repeated above, and documented
> > > > it.  So you're going to need to repeat it with different words so I can
> > > > understand, if I'm still missing something.
> > >
> > > In swizzle_detect:
> > >
> > > ...
> > >
> > > if (GEN6+) {
> > > if (preserve_bios_swizzle) {
> > > if (I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING) {
> > > swizzle_x = I915_BIT_6_SWIZZLE_9_10;
> > > ...
> > > } else {
> > > swizzle_x = I915_BIT_6_SWIZZLE_NONE;
> > > ...
> > > }
> > > } else {
> > > /* existing/old logic to decide about swizzling */
> > > }
> > > }
> > >
> > > ...
> > >
> > > Plus no shortcut in i915_gem_init_swizzling. Personally I'd also just use
> > > a small helper function to compute preserve_bios_swizzle instead of
> > > storing it in dev_priv (since we will only use it at exactly one place),
> > > but that's a pure style preference.
> >
> > Doesn't this amount to the same thing?  I.e. we enable it if possible,
> > otherwise just report it as unswizzled?  So you're just wanting a style
> > change here?
> 
> So presuming I read your code correctly there's two issues:
> 
> - The first thing you check in swizzle_detect is the hw swizzle bit in
>   DSP_ARB. If that's not set you force unswizzled. Since most BIOS don't
>   bother to set this (they use an untiled buffer after all) this means
>   you've effectively disabled swizzling on almost all machines. The goal
>   of keeping the old logic was that we actually want to enable swizzling
>   in some situations.

Ah damn, I had been thinking that bit_6_swizzle was the runtime call,
and that the init_swizzle call would go ahead and set it up properly.
I see that's too late though.

> - If you have a machine which uses tiled framebuffers and enables
>   swizzling in the BIOS your code will a) drop the swizzle setup in
>   gem_init_hw, breaking resume b) not set the swizzle settings correctly
>   in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such
>   a machine exists, but still.

This would affect krh's MBA, which is why I wanted testing here...
anyway I'll spin a new one and ask krh to test again.

-- 
Jesse Barnes, Intel Open Source Technology Center