Re: [PATCH 20/43] drm/i915: swizzling support for snb/ivb

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jan 30, 2012 at 11:44:35PM -0800, Ben Widawsky wrote:
> On Wed, Dec 14, 2011 at 01:57:17PM +0100, Daniel Vetter wrote:
> > We have to do this manually. Somebody had a Great Idea.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Okay, my configdb access isn't working, so please forgive me if my
> inline comments are not correct... I'll try to review it again when my
> access works again.
> 
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c        |    2 +-
> >  drivers/gpu/drm/i915/i915_drv.c        |    4 ++-
> >  drivers/gpu/drm/i915/i915_drv.h        |    3 +-
> >  drivers/gpu/drm/i915/i915_gem.c        |   23 ++++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_gem_tiling.c |   16 +++++++++++++-
> >  drivers/gpu/drm/i915/i915_reg.h        |   33 ++++++++++++++++++++++++++++++++
> >  6 files changed, 74 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 448d5b1..4c21c67 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1202,7 +1202,7 @@ static int i915_load_gem_init(struct drm_device *dev)
> >  	i915_gem_do_init(dev, 0, mappable_size, gtt_size - PAGE_SIZE);
> >  
> >  	mutex_lock(&dev->struct_mutex);
> > -	ret = i915_gem_init_ringbuffer(dev);
> > +	ret = i915_gem_init_hw(dev);
> >  	mutex_unlock(&dev->struct_mutex);
> >  	if (ret)
> >  		return ret;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 6dd219b..f12b43e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -494,7 +494,7 @@ static int i915_drm_thaw(struct drm_device *dev)
> >  		mutex_lock(&dev->struct_mutex);
> >  		dev_priv->mm.suspended = 0;
> >  
> > -		error = i915_gem_init_ringbuffer(dev);
> > +		error = i915_gem_init_hw(dev);
> >  		mutex_unlock(&dev->struct_mutex);
> >  
> >  		if (HAS_PCH_SPLIT(dev))
> > @@ -690,6 +690,8 @@ int i915_reset(struct drm_device *dev, u8 flags)
> >  			!dev_priv->mm.suspended) {
> >  		dev_priv->mm.suspended = 0;
> >  
> > +		i915_gem_init_swizzling(dev);
> > +
> >  		dev_priv->ring[RCS].init(&dev_priv->ring[RCS]);
> >  		if (HAS_BSD(dev))
> >  		    dev_priv->ring[VCS].init(&dev_priv->ring[VCS]);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index b46fac5..311a4e1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1206,7 +1206,8 @@ int __must_check i915_gem_object_set_domain(struct drm_i915_gem_object *obj,
> >  					    uint32_t read_domains,
> >  					    uint32_t write_domain);
> >  int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
> > -int __must_check i915_gem_init_ringbuffer(struct drm_device *dev);
> > +int __must_check i915_gem_init_hw(struct drm_device *dev);
> > +void i915_gem_init_swizzling(struct drm_device *dev);
> >  void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
> >  void i915_gem_do_init(struct drm_device *dev,
> >  		      unsigned long start,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index e995248..39459d2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3744,12 +3744,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(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));
> > +	I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) |
> > +				 DISP_TILE_SURFACE_SWIZZLING);
> > +
> > +}
> >  int
> > -i915_gem_init_ringbuffer(struct drm_device *dev)
> > +i915_gem_init_hw(struct drm_device *dev)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> >  	int ret;
> >  
> > +	i915_gem_init_swizzling(dev);
> > +
> >  	ret = intel_init_render_ring_buffer(dev);
> >  	if (ret)
> >  		return ret;
> > @@ -3805,7 +3824,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
> >  	mutex_lock(&dev->struct_mutex);
> >  	dev_priv->mm.suspended = 0;
> >  
> > -	ret = i915_gem_init_ringbuffer(dev);
> > +	ret = i915_gem_init_hw(dev);
> >  	if (ret != 0) {
> >  		mutex_unlock(&dev->struct_mutex);
> >  		return ret;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > index 861223b..af0a2fc 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > @@ -93,8 +93,20 @@ 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_A_SIZE_MASK;
> > +		dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_A_SIZE_MASK;
> 
> That doesn't look right to me. Presumably you meant
> MAD_DIMM_B_SIZE_MASK. Is it safe to ignore the others bits too, like
> dual rank, and width? Also without configdb, I'm somewhat confused why
> you don't read c2.

Yep to dimm_B instead of A. For the other bits I'm under the impression
that they're just used to select the best swizzling/interleaving within a
dimm, so shouldn't really matter for our purpose of selecting swizzling
between channels. For that I think we just want equally-sized channels.

I don't check for channel 2 because that only works on high-end 3-channel
cpus that don't ship with a gpu. All the swizzling patterns presume that
we have 2 channels only, anyway.

> 
> > +		/* Enable swizzling when the channels are populated with
> > +		 * identically sized dimms. */
> > +		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;
> > +		}
> >  	} else if (IS_GEN5(dev)) {
> >  		/* On Ironlake whatever DRAM config, GPU always do
> >  		 * same swizzling setup.
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8a9f113..e810723 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -295,6 +295,12 @@
> >  #define FENCE_REG_SANDYBRIDGE_0		0x100000
> >  #define   SANDYBRIDGE_FENCE_PITCH_SHIFT	32
> >  
> > +/* control register for cpu gtt access */
> > +#define TILECTL				0x101000
> > +#define   TILECTL_SWZCTL			(1 << 0)
> > +#define   TILECTL_TLB_PREFETCH_DIS	(1 << 2)
> > +#define   TILECTL_BACKSNOOP_DIS		(1 << 3)
> > +
> >  /*
> >   * Instruction and interrupt control regs
> >   */
> > @@ -318,6 +324,11 @@
> >  #define RING_MAX_IDLE(base)	((base)+0x54)
> >  #define RING_HWS_PGA(base)	((base)+0x80)
> >  #define RING_HWS_PGA_GEN6(base)	((base)+0x2080)
> > +#define ARB_MODE		0x04030
> > +#define   ARB_MODE_SWIZZLE_SNB	(1<<4)
> > +#define   ARB_MODE_SWIZZLE_IVB	(1<<5)
> > +#define   ARB_MODE_ENABLE(x)	GFX_MODE_ENABLE(x)
> > +#define   ARB_MODE_DISABLE(x)	GFX_MODE_DISABLE(x)
> 
> Hurray for the designers reusing bits again!
> 
> >  #define RENDER_HWS_PGA_GEN7	(0x04080)
> >  #define BSD_HWS_PGA_GEN7	(0x04180)
> >  #define BLT_HWS_PGA_GEN7	(0x04280)
> > @@ -1034,6 +1045,28 @@
> >  #define C0DRB3			0x10206
> >  #define C1DRB3			0x10606
> >  
> > +/** snb MCH registers for reading the DRAM channel configuration */
> > +#define MAD_DIMM_C0			(MCHBAR_MIRROR_BASE_SNB + 0x5004)
> > +#define   MAD_DIMM_C1			(MCHBAR_MIRROR_BASE_SNB + 0x5008)
> > +#define   MAD_DIMM_C2			(MCHBAR_MIRROR_BASE_SNB + 0x500C)
> > +#define   MAD_DIMM_ECC_MASK		(0x3 << 24)
> > +#define   MAD_DIMM_ECC_OFF		(0x0 << 24)
> > +#define   MAD_DIMM_ECC_IO_ON_LOGIC_OFF	(0x1 << 24)
> > +#define   MAD_DIMM_ECC_IO_OFF_LOGIC_ON	(0x2 << 24)
> > +#define   MAD_DIMM_ECC_ON		(0x3 << 24)
> > +#define   MAD_DIMM_ENH_INTERLEAVE	(0x1 << 22)
> > +#define   MAD_DIMM_RANK_INTERLEAVE	(0x1 << 21)
> > +#define   MAD_DIMM_B_WIDTH_X16		(0x1 << 20) /* X8 chips if unset */
> > +#define   MAD_DIMM_A_WIDTH_X16		(0x1 << 19) /* X8 chips if unset */
> > +#define   MAD_DIMM_B_DUAL_RANK		(0x1 << 18)
> > +#define   MAD_DIMM_A_DUAL_RANK		(0x1 << 17)
> > +#define   MAD_DIMM_A_SELECT		(0x1 << 16)
> > +#define   MAD_DIMM_B_SIZE_MASK		(0xff << 8) /* in multiples of 256mb */
> > +#define   MAD_DIMM_B_SIZE_SHIFT		8
> > +#define   MAD_DIMM_A_SIZE_MASK		(0xff << 0) /* in multiples of 256mb */
> > +#define   MAD_DIMM_A_SIZE_SHIFT		8
> > +
> > +
> 
> This also looks less than correct. Seems like MAD_DIMM_A_SIZE_SHIFT
> should be 0, and you should use those defines in the MASK definitions.

Agreed, I'll fix this up.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48