[PATCH] BDW swizzling

[PATCH] BDW swizzling

[Damien Lespiau]

While cruising through the specs, I noticed a note about swizzling changes on
BDW. My understanding is that we don't need to either initialize swizzling on
the GPU side nor swizzle the address ourselves on the CPU side.

That could be totally wrong though, and I unfortunately don't have a machine to
test this theory on.

-- 
Damien

Damien Lespiau (1):
  drm/i915/bdw: BDW swizzling in done by the memory controller

 drivers/gpu/drm/i915/i915_gem.c        |  2 --
 drivers/gpu/drm/i915/i915_gem_tiling.c | 10 +++++++++-
 drivers/gpu/drm/i915/i915_reg.h        |  1 -
 3 files changed, 9 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH] drm/i915/bdw: BDW swizzling in done by the memory controller

[Damien Lespiau]

Instead of needing to configure swizzling in 3 units (GAM, GT, DE), the
memory controller is in charge of doing them on BDW. As a consequence
all those swizzling bits are reserved. As specs put it:

  Before Gen8, there was a historical configuration control field to
  swizzle address bit[6] for in X/Y tiling modes. This was set in three
  different places: TILECTL[1:0], ARB_MODE[5:4], and
  DISP_ARB_CTL[14:13]"

  For Gen8 the swizzle fields are all reserved, and the CPU's memory
  controller performs all address swizzling modifications.

This also means that user space doesn't have to manually swizzle when
accessing tiled buffers from the CPU, and so we always return
I915_BIT_6_SWIZZLE_NONE from i915_gem_detect_bit_6_swizzle(), which
short-circuits the initialization of the registers mentionned above in
i915_gem_init_swizzling().

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c        |  2 --
 drivers/gpu/drm/i915/i915_gem_tiling.c | 10 +++++++++-
 drivers/gpu/drm/i915/i915_reg.h        |  1 -
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 85c9cf0..9032c1b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4325,8 +4325,6 @@ void i915_gem_init_swizzling(struct drm_device *dev)
 		I915_WRITE(ARB_MODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_SNB));
 	else if (IS_GEN7(dev))
 		I915_WRITE(ARB_MODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_IVB));
-	else if (IS_GEN8(dev))
-		I915_WRITE(GAMTARBMODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_BDW));
 	else
 		BUG();
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index cb150e8..a5ddf12 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -91,7 +91,15 @@ 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)) {
+	if (INTEL_INFO(dev)->gen >= 8) {
+		/*
+		 * On BDW+, the CPU memory controller performs all address
+		 * swizzling modifications. This condition also catches CHV,
+		 * where swizzling is not supported.
+		 */
+		swizzle_x = I915_BIT_6_SWIZZLE_NONE;
+		swizzle_y = I915_BIT_6_SWIZZLE_NONE;
+	} else 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) {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 01c05af..faba21b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -781,7 +781,6 @@ enum punit_power_well {
 #define   ARB_MODE_SWIZZLE_IVB	(1<<5)
 #define GAMTARBMODE		0x04a08
 #define   ARB_MODE_BWGTLB_DISABLE (1<<9)
-#define   ARB_MODE_SWIZZLE_BDW	(1<<1)
 #define RENDER_HWS_PGA_GEN7	(0x04080)
 #define RING_FAULT_REG(ring)	(0x4094 + 0x100*(ring)->id)
 #define   RING_FAULT_GTTSEL_MASK (1<<11)
-- 
1.8.3.1

Re: [PATCH] BDW swizzling

[Ben Widawsky]

On Thu, Apr 10, 2014 at 05:24:07PM +0100, Damien Lespiau wrote:
> While cruising through the specs, I noticed a note about swizzling changes on
> BDW. My understanding is that we don't need to either initialize swizzling on
> the GPU side nor swizzle the address ourselves on the CPU side.
> 
> That could be totally wrong though, and I unfortunately don't have a machine to
> test this theory on.

I fought with this too. My resolution was we can either set all the
swizzling bits, or set none. There is no motivation to do either, and
the spec simply is telling us what they do for windows. That was well
over a year ago, so it all can be different now.

I honestly don't care what we do though, so long as the patches get
tested both in simulation and silicon, and there is no measurable perf
drop. I suppose my mild preference is to, "don't touch it if it ain't
broke."

Sorry, but at the moment, I don't have time to test this for you. Maybe
someone else can, or remind me in a couple of weeks.

> 
> -- 
> Damien
> 
> Damien Lespiau (1):
>   drm/i915/bdw: BDW swizzling in done by the memory controller
> 
>  drivers/gpu/drm/i915/i915_gem.c        |  2 --
>  drivers/gpu/drm/i915/i915_gem_tiling.c | 10 +++++++++-
>  drivers/gpu/drm/i915/i915_reg.h        |  1 -
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> -- 
> 1.8.3.1
> 

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] BDW swizzling

[Damien Lespiau]

On Thu, Apr 10, 2014 at 10:32:46AM -0700, Ben Widawsky wrote:
> On Thu, Apr 10, 2014 at 05:24:07PM +0100, Damien Lespiau wrote:
> > While cruising through the specs, I noticed a note about swizzling changes on
> > BDW. My understanding is that we don't need to either initialize swizzling on
> > the GPU side nor swizzle the address ourselves on the CPU side.
> > 
> > That could be totally wrong though, and I unfortunately don't have a machine to
> > test this theory on.
> 
> I fought with this too. My resolution was we can either set all the
> swizzling bits, or set none. There is no motivation to do either, and
> the spec simply is telling us what they do for windows. That was well
> over a year ago, so it all can be different now.

My (limited) understanding is telling me that if we don't return
I915_BIT_6_SWIZZLE_NONE, user space is going to swizzle the address and
the controller is going to swizzle it again, even from the CPU, so we
end up at the wrong place.

> I honestly don't care what we do though, so long as the patches get
> tested both in simulation and silicon, and there is no measurable perf
> drop. I suppose my mild preference is to, "don't touch it if it ain't
> broke."
> 
> Sorry, but at the moment, I don't have time to test this for you. Maybe
> someone else can, or remind me in a couple of weeks.

Do you know if you have a configuration where we try to swizzle? If yes
and tests/gem_tiled_pread is passing that would give us a nice bit of
information. (which of course can be tried by the next person with time
to do so).

-- 
Damien

Re: [PATCH] BDW swizzling

[Ben Widawsky]

On Thu, Apr 10, 2014 at 06:51:50PM +0100, Damien Lespiau wrote:
> On Thu, Apr 10, 2014 at 10:32:46AM -0700, Ben Widawsky wrote:
> > On Thu, Apr 10, 2014 at 05:24:07PM +0100, Damien Lespiau wrote:
> > > While cruising through the specs, I noticed a note about swizzling changes on
> > > BDW. My understanding is that we don't need to either initialize swizzling on
> > > the GPU side nor swizzle the address ourselves on the CPU side.
> > > 
> > > That could be totally wrong though, and I unfortunately don't have a machine to
> > > test this theory on.
> > 
> > I fought with this too. My resolution was we can either set all the
> > swizzling bits, or set none. There is no motivation to do either, and
> > the spec simply is telling us what they do for windows. That was well
> > over a year ago, so it all can be different now.
> 
> My (limited) understanding is telling me that if we don't return
> I915_BIT_6_SWIZZLE_NONE, user space is going to swizzle the address and
> the controller is going to swizzle it again, even from the CPU, so we
> end up at the wrong place.

I thought this type of swizzling was usually determined by our code
i915_gem_detect_bit_6_swizzle(). I am also unfamiliar with whether or
not userspace controls it, and whether or not it should. I never really
looked at that aspect of the code. Maybe Chris can answer this part.

> 
> > I honestly don't care what we do though, so long as the patches get
> > tested both in simulation and silicon, and there is no measurable perf
> > drop. I suppose my mild preference is to, "don't touch it if it ain't
> > broke."
> > 
> > Sorry, but at the moment, I don't have time to test this for you. Maybe
> > someone else can, or remind me in a couple of weeks.
> 
> Do you know if you have a configuration where we try to swizzle? If yes
> and tests/gem_tiled_pread is passing that would give us a nice bit of
> information. (which of course can be tried by the next person with time
> to do so).
> 

If you get it wrong, it looks really obvious. Swizzling is *supposed* to
be one of those transparent things (I thought). What follows can be
entirely wrong, it's mostly from memory and a brief conversation with
Art.

There are 3 places that care about swizzling:
1. The memory/DRAM controller
2. The displayer interface to memory
3. The GAM arbiter (generic interface to memory)

It may or may not be talking about the same type of swizzling (bit) in
all cases. The important thing, and what I have observed, is that the
GAM and DE match on how things are swizzled. Otherwise we render/blit to
a surface and it gets [de]swizzled when it's displayed. I never measured
performance for setting both to 0, instead of 1.

The part that's confused me has always been why we are supposed to
program it based on #1. The way the DRAM controller decides to lay out
the physical rows/banks etc. shouldn't matter as long as everyone goes
through the same DRAM controller. It should just be transparent linear
RAM. In other words, the comment about how we need to program the
swizzle based on the DRAM controller never quite made sense to me. It's
also possible if you enable one, you shouldn't/should enable another
since compounding swizzling may be self-defeating. Dunno - so maybe your
patch helps, maybe it hurts.

Art suggested that the swizzling in GAM and DE predate the DRAM
swizzler.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] BDW swizzling

[Chris Wilson]

On Thu, Apr 10, 2014 at 03:50:35PM -0700, Ben Widawsky wrote:
> On Thu, Apr 10, 2014 at 06:51:50PM +0100, Damien Lespiau wrote:
> > On Thu, Apr 10, 2014 at 10:32:46AM -0700, Ben Widawsky wrote:
> > > On Thu, Apr 10, 2014 at 05:24:07PM +0100, Damien Lespiau wrote:
> > > > While cruising through the specs, I noticed a note about swizzling changes on
> > > > BDW. My understanding is that we don't need to either initialize swizzling on
> > > > the GPU side nor swizzle the address ourselves on the CPU side.
> > > > 
> > > > That could be totally wrong though, and I unfortunately don't have a machine to
> > > > test this theory on.
> > > 
> > > I fought with this too. My resolution was we can either set all the
> > > swizzling bits, or set none. There is no motivation to do either, and
> > > the spec simply is telling us what they do for windows. That was well
> > > over a year ago, so it all can be different now.
> > 
> > My (limited) understanding is telling me that if we don't return
> > I915_BIT_6_SWIZZLE_NONE, user space is going to swizzle the address and
> > the controller is going to swizzle it again, even from the CPU, so we
> > end up at the wrong place.
> 
> I thought this type of swizzling was usually determined by our code
> i915_gem_detect_bit_6_swizzle(). I am also unfamiliar with whether or
> not userspace controls it, and whether or not it should. I never really
> looked at that aspect of the code. Maybe Chris can answer this part.

Userspace only queries swizzling.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] BDW swizzling

[Ben Widawsky]

On Thu, Apr 10, 2014 at 03:50:35PM -0700, Ben Widawsky wrote:
> On Thu, Apr 10, 2014 at 06:51:50PM +0100, Damien Lespiau wrote:
[snip]

> > 
> > Do you know if you have a configuration where we try to swizzle? If yes
> > and tests/gem_tiled_pread is passing that would give us a nice bit of
> > information. (which of course can be tried by the next person with time
> > to do so).
> > 
> 
> If you get it wrong, it looks really obvious. Swizzling is *supposed* to
> be one of those transparent things (I thought). What follows can be
> entirely wrong, it's mostly from memory and a brief conversation with
> Art.
> 
> There are 3 places that care about swizzling:
> 1. The memory/DRAM controller
> 2. The displayer interface to memory
> 3. The GAM arbiter (generic interface to memory)
> 
> It may or may not be talking about the same type of swizzling (bit) in
> all cases. The important thing, and what I have observed, is that the
> GAM and DE match on how things are swizzled. Otherwise we render/blit to
> a surface and it gets [de]swizzled when it's displayed. I never measured
> performance for setting both to 0, instead of 1.
> 
> The part that's confused me has always been why we are supposed to
> program it based on #1. The way the DRAM controller decides to lay out
> the physical rows/banks etc. shouldn't matter as long as everyone goes
> through the same DRAM controller. It should just be transparent linear
> RAM. In other words, the comment about how we need to program the
> swizzle based on the DRAM controller never quite made sense to me. It's
> also possible if you enable one, you shouldn't/should enable another
> since compounding swizzling may be self-defeating. Dunno - so maybe your
> patch helps, maybe it hurts.
> 
> Art suggested that the swizzling in GAM and DE predate the DRAM
> swizzler.

My bad. I just read the actual patch's commit message. Seems like you
knew all this already. Feel free to ignore me. I'll try to read both
before responding, next time.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915/bdw: BDW swizzling in done by the memory controller

[Daniel Vetter]

On Thu, Apr 10, 2014 at 05:24:08PM +0100, Damien Lespiau wrote:
> Instead of needing to configure swizzling in 3 units (GAM, GT, DE), the
> memory controller is in charge of doing them on BDW. As a consequence
> all those swizzling bits are reserved. As specs put it:
> 
>   Before Gen8, there was a historical configuration control field to
>   swizzle address bit[6] for in X/Y tiling modes. This was set in three
>   different places: TILECTL[1:0], ARB_MODE[5:4], and
>   DISP_ARB_CTL[14:13]"
> 
>   For Gen8 the swizzle fields are all reserved, and the CPU's memory
>   controller performs all address swizzling modifications.
> 
> This also means that user space doesn't have to manually swizzle when
> accessing tiled buffers from the CPU, and so we always return
> I915_BIT_6_SWIZZLE_NONE from i915_gem_detect_bit_6_swizzle(), which
> short-circuits the initialization of the registers mentionned above in
> i915_gem_init_swizzling().
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Afaik the memory controller has always done the swizzling. What this pile
of bits controls is the _additional_ swizzling done to improve the access
pattern for 2d data, i.e. everything X and Y tiled. The theory of
operation is that the additional swizzling improves access patterns when
walking in Y direction on a surface.

And when we've last looked at this (well Chris) it seemed to indeed have
improved sampler performace. The downside is that it's a bit a pain for
userspace since essentially userspace has to deal with 4 tiling formats
instead of just 2, but we have all the code for that.

So not yet sold on the story here, at least for gen8/bdw. Apparently
people haven't screamed about this yet, so it probably works.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c        |  2 --
>  drivers/gpu/drm/i915/i915_gem_tiling.c | 10 +++++++++-
>  drivers/gpu/drm/i915/i915_reg.h        |  1 -
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 85c9cf0..9032c1b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4325,8 +4325,6 @@ void i915_gem_init_swizzling(struct drm_device *dev)
>  		I915_WRITE(ARB_MODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_SNB));
>  	else if (IS_GEN7(dev))
>  		I915_WRITE(ARB_MODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_IVB));
> -	else if (IS_GEN8(dev))
> -		I915_WRITE(GAMTARBMODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_BDW));
>  	else
>  		BUG();
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index cb150e8..a5ddf12 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -91,7 +91,15 @@ 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)) {
> +	if (INTEL_INFO(dev)->gen >= 8) {
> +		/*
> +		 * On BDW+, the CPU memory controller performs all address
> +		 * swizzling modifications. This condition also catches CHV,
> +		 * where swizzling is not supported.
> +		 */
> +		swizzle_x = I915_BIT_6_SWIZZLE_NONE;
> +		swizzle_y = I915_BIT_6_SWIZZLE_NONE;
> +	} else 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) {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 01c05af..faba21b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -781,7 +781,6 @@ enum punit_power_well {
>  #define   ARB_MODE_SWIZZLE_IVB	(1<<5)
>  #define GAMTARBMODE		0x04a08
>  #define   ARB_MODE_BWGTLB_DISABLE (1<<9)
> -#define   ARB_MODE_SWIZZLE_BDW	(1<<1)
>  #define RENDER_HWS_PGA_GEN7	(0x04080)
>  #define RING_FAULT_REG(ring)	(0x4094 + 0x100*(ring)->id)
>  #define   RING_FAULT_GTTSEL_MASK (1<<11)
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915/bdw: BDW swizzling in done by the memory controller

[Chris Wilson]

On Fri, Apr 11, 2014 at 11:09:03AM +0200, Daniel Vetter wrote:
> Afaik the memory controller has always done the swizzling. What this pile
> of bits controls is the _additional_ swizzling done to improve the access
> pattern for 2d data, i.e. everything X and Y tiled. The theory of
> operation is that the additional swizzling improves access patterns when
> walking in Y direction on a surface.
> 
> And when we've last looked at this (well Chris) it seemed to indeed have
> improved sampler performace.

And recall this was around SNB time. Jesse keeps claiming that IVB+
doesn't benefit, and I'm inclined to believe him. At some point, we
should test again.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/bdw: BDW swizzling in done by the memory controller

[Damien Lespiau]

On Fri, Apr 11, 2014 at 11:09:03AM +0200, Daniel Vetter wrote:
> So not yet sold on the story here, at least for gen8/bdw. Apparently
> people haven't screamed about this yet, so it probably works.

Having thought about it a bit more, I don't see how the CPU side would
know about the tiling layout of the surfaces it accesses, so the
remaining options I see:

  - we still need to swizzle the address on the CPU side
  - bit 6 swizzling for X/Y tiling is just gone and the optimal use of
    the RAM is left to the memory controller. If that's the case, we
    should see things failing soon enough

So, until any failure case, meh. Just one thing to remember is that the
swizzling bits we set are possibly reserved and may be no-ops.

-- 
Damien

Re: [PATCH] drm/i915/bdw: BDW swizzling in done by the memory controller

[Daniel Vetter]

On Fri, Apr 11, 2014 at 1:10 PM, Damien Lespiau
<damien.lespiau@intel.com> wrote:
> On Fri, Apr 11, 2014 at 11:09:03AM +0200, Daniel Vetter wrote:
>> So not yet sold on the story here, at least for gen8/bdw. Apparently
>> people haven't screamed about this yet, so it probably works.
>
> Having thought about it a bit more, I don't see how the CPU side would
> know about the tiling layout of the surfaces it accesses, so the
> remaining options I see:
>
>   - we still need to swizzle the address on the CPU side
>   - bit 6 swizzling for X/Y tiling is just gone and the optimal use of
>     the RAM is left to the memory controller. If that's the case, we
>     should see things failing soon enough
>
> So, until any failure case, meh. Just one thing to remember is that the
> swizzling bits we set are possibly reserved and may be no-ops.

That's very easy to figure out. Set the bit in TILE_CTL and
GAMT_ARBMODE differently and see what happens. blt vs gtt mmap access
would be different. igt has testcases which will catch this.

At least on simulation this blew up rather badly ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch