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

From: Jesse Barnes <jbarnes@virtuousgeek.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3
Date: Tue, 10 Jun 2014 10:27:11 -0700	[thread overview]
Message-ID: <20140610102711.171104dc@jbarnes-desktop> (raw)
In-Reply-To: <20140610140251.GP5821@phenom.ffwll.local>

On Tue, 10 Jun 2014 16:02:51 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jun 05, 2014 at 11:24:28AM -0700, Jesse Barnes wrote:
> > Some machines (like MBAs) might use a tiled framebuffer but not enable
> > display swizzling at boot time.  We want to preserve that configuration
> > if possible to prevent a boot time mode set.  On IVB+ it shouldn't
> > affect performance anyway since the memory controller does internal
> > swizzling anyway.
> > 
> > For most other configs we'll be able to enable swizzling at boot time,
> > since the initial framebuffer won't be tiled, thus we won't see any
> > corruption when we enable it.
> > 
> > v2: preserve swizzling if BIOS had it set (Daniel)
> > v3: preserve swizzling only if we inherited a tiled framebuffer (Daniel)
> >     check display swizzle setting in detect_bit_6_swizzle (Daniel)
> >     use gen6 as cutoff point (Daniel)
> > 
> > Reported-by: Kristian Høgsberg <hoegsberg@gmail.com>
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h        |  1 +
> >  drivers/gpu/drm/i915/i915_gem.c        |  3 +++
> >  drivers/gpu/drm/i915/i915_gem_tiling.c | 38 +++++++++++++++++++---------------
> >  drivers/gpu/drm/i915/intel_display.c   |  3 +++
> >  4 files changed, 28 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f57b752..f49fdcb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1405,6 +1405,7 @@ struct drm_i915_private {
> >  	struct intel_vbt_data vbt;
> >  
> >  	bool bios_ssc; /* BIOS had SSC enabled at boot? */
> > +	bool preserve_bios_swizzle;
> >  
> >  	/* overlay */
> >  	struct intel_overlay *overlay;
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index bfc7af0..0b168fb 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4580,6 +4580,9 @@ void i915_gem_init_swizzling(struct drm_device *dev)
> >  	    dev_priv->mm.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_NONE)
> >  		return;
> >  
> > +	if (INTEL_INFO(dev)->gen >= 6 && dev_priv->preserve_bios_swizzle)
> > +		return;
> > +
> 
> Above two hunks shouldnt be needed if the setup in
> i915_gem_detect_bit_6_swizzle works correctly.
> 
> >  	I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) |
> >  				 DISP_TILE_SURFACE_SWIZZLING);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > index cb150e8..73005ad 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > @@ -91,26 +91,30 @@ i915_gem_detect_bit_6_swizzle(struct drm_device *dev)
> >  	uint32_t swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN;
> >  	uint32_t swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN;
> >  
> > -	if (IS_VALLEYVIEW(dev)) {
> > -		swizzle_x = I915_BIT_6_SWIZZLE_NONE;
> > -		swizzle_y = I915_BIT_6_SWIZZLE_NONE;
> > -	} else if (INTEL_INFO(dev)->gen >= 6) {
> > +	if (INTEL_INFO(dev)->gen >= 6) {
> >  		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 {
> > +
> > +		/* Make sure to honor boot time display configuration */
> > +		if (!(I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING)) {
> 
> Not quite what I had in mind. Imo we need to check for whether any
> inherited fbs are tiled and if so also inherit the swizzle setting
> unconditionally, whether it is swizzled or unswizzled. See
> 
> http://patchwork.freedesktop.org/patch/22204/

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.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel