Re: [PATCH] drm/i915: PPGTT dump for debug

Re: [PATCH] drm/i915: PPGTT dump for debug

[Ben Widawsky]

On Mon, Jun 24, 2013 at 10:01:33AM -0700, Ben Widawsky wrote:
> No users yet
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I didn't mean to send this yet. Please ignore.

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH] drm/i915: Fix context sizes on HSW

[Ben Widawsky]

With updates to the spec, we can actually see the context layout, and
how many dwords are allocated. That table suggests we need 70720 bytes
per HW context. Rounded up, this is 18 pages. Looking at what lives
after the current 4 pages we use, I can't see too much important (mostly
it's d3d related), but there are a couple of things which look scary. I
am hopeful this can explain some of our odd HSW failures.

NOTE: There is some discrepancy in our internal docs about whether or
not this should be 17, or 18 pages. Since for stable, and currently we
don't use many contexts, just use 18 to be safe. We can trim it later.

Reported-by: "Azad, Vinit" <vinit.azad@intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
 drivers/gpu/drm/i915/i915_reg.h         | 9 +--------
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index ff47145..51b7a21 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -113,7 +113,7 @@ static int get_context_size(struct drm_device *dev)
 	case 7:
 		reg = I915_READ(GEN7_CXT_SIZE);
 		if (IS_HASWELL(dev))
-			ret = HSW_CXT_TOTAL_SIZE(reg) * 64;
+			ret = HSW_CXT_TOTAL_SIZE;
 		else
 			ret = GEN7_CXT_TOTAL_SIZE(reg) * 64;
 		break;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2102ff3..4344b77 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1718,14 +1718,7 @@
 					 GEN7_CXT_EXTENDED_SIZE(ctx_reg) + \
 					 GEN7_CXT_GT1_SIZE(ctx_reg) + \
 					 GEN7_CXT_VFSTATE_SIZE(ctx_reg))
-#define HSW_CXT_POWER_SIZE(ctx_reg)	((ctx_reg >> 26) & 0x3f)
-#define HSW_CXT_RING_SIZE(ctx_reg)	((ctx_reg >> 23) & 0x7)
-#define HSW_CXT_RENDER_SIZE(ctx_reg)	((ctx_reg >> 15) & 0xff)
-#define HSW_CXT_TOTAL_SIZE(ctx_reg)	(HSW_CXT_POWER_SIZE(ctx_reg) + \
-					 HSW_CXT_RING_SIZE(ctx_reg) + \
-					 HSW_CXT_RENDER_SIZE(ctx_reg) + \
-					 GEN7_CXT_VFSTATE_SIZE(ctx_reg))
-
+#define HSW_CXT_TOTAL_SIZE		(18 * PAGE_SIZE)
 
 /*
  * Overlay regs
-- 
1.8.3

[PATCH] drm/i915: PPGTT dump for debug

[Ben Widawsky]

No users yet

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 893007a..005358f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -99,6 +99,37 @@ static gen6_gtt_pte_t hsw_pte_encode(dma_addr_t addr,
 
 	return pte;
 }
+void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt)
+{
+	struct drm_i915_private *dev_priv = ppgtt->base.dev->dev_private;
+	gen6_gtt_pte_t __iomem *pd_addr;
+	uint32_t pd_entry;
+	int i, j;
+
+	pd_addr = (gen6_gtt_pte_t __iomem*)dev_priv->gtt.gsm +
+		ppgtt->pd_offset / sizeof(gen6_gtt_pte_t);
+
+	for (i = 0; i < ppgtt->num_pd_entries; i++) {
+		gen6_gtt_pte_t *pt_vaddr;
+		dma_addr_t pt_addr = ppgtt->pt_dma_addr[i];
+		pd_entry = readl(pd_addr + i);
+
+		if (pd_entry != (u32)(GEN6_PDE_ADDR_ENCODE(pt_addr) | GEN6_PDE_VALID))
+			DRM_ERROR("Actual PDE: %x Expected PDE: %x",
+				  pd_entry,
+				  (u32)(GEN6_PDE_ADDR_ENCODE(pt_addr) | GEN6_PDE_VALID));
+
+		pt_vaddr = kmap_atomic(ppgtt->pt_pages[i]);
+		for (j = 0; j < I915_PPGTT_PT_ENTRIES; j++) {
+			gen6_gtt_pte_t scratch_pte =
+				ppgtt->base.pte_encode(ppgtt->base.scratch.addr,
+						       I915_CACHE_LLC);
+			if (pt_vaddr[j] != scratch_pte)
+				DRM_ERROR("\tPTE = %x (%d %d)", pt_vaddr[j], i,j);
+		}
+		kunmap_atomic(pt_vaddr);
+	}
+}
 
 static void gen6_write_pdes(struct i915_hw_ppgtt *ppgtt)
 {
-- 
1.8.3.1

[PATCH] [v2] drm/i915: Fix context sizes on HSW

[Ben Widawsky]

With updates to the spec, we can actually see the context layout, and
how many dwords are allocated. That table suggests we need 70720 bytes
per HW context. Rounded up, this is 18 pages. Looking at what lives
after the current 4 pages we use, I can't see too much important (mostly
it's d3d related), but there are a couple of things which look scary. I
am hopeful this can explain some of our odd HSW failures.

v2: Make the context only 17 pages. The power context space isn't used
ever, and execlists aren't used in our driver, making the actual total
66944 bytes.

Reported-by: "Azad, Vinit" <vinit.azad@intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
 drivers/gpu/drm/i915/i915_reg.h         | 9 +--------
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index ff47145..51b7a21 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -113,7 +113,7 @@ static int get_context_size(struct drm_device *dev)
 	case 7:
 		reg = I915_READ(GEN7_CXT_SIZE);
 		if (IS_HASWELL(dev))
-			ret = HSW_CXT_TOTAL_SIZE(reg) * 64;
+			ret = HSW_CXT_TOTAL_SIZE;
 		else
 			ret = GEN7_CXT_TOTAL_SIZE(reg) * 64;
 		break;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2102ff3..f061c67 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1718,14 +1718,7 @@
 					 GEN7_CXT_EXTENDED_SIZE(ctx_reg) + \
 					 GEN7_CXT_GT1_SIZE(ctx_reg) + \
 					 GEN7_CXT_VFSTATE_SIZE(ctx_reg))
-#define HSW_CXT_POWER_SIZE(ctx_reg)	((ctx_reg >> 26) & 0x3f)
-#define HSW_CXT_RING_SIZE(ctx_reg)	((ctx_reg >> 23) & 0x7)
-#define HSW_CXT_RENDER_SIZE(ctx_reg)	((ctx_reg >> 15) & 0xff)
-#define HSW_CXT_TOTAL_SIZE(ctx_reg)	(HSW_CXT_POWER_SIZE(ctx_reg) + \
-					 HSW_CXT_RING_SIZE(ctx_reg) + \
-					 HSW_CXT_RENDER_SIZE(ctx_reg) + \
-					 GEN7_CXT_VFSTATE_SIZE(ctx_reg))
-
+#define HSW_CXT_TOTAL_SIZE		(17 * PAGE_SIZE)
 
 /*
  * Overlay regs
-- 
1.8.3

Re: [Intel-gfx] [PATCH] [v2] drm/i915: Fix context sizes on HSW

[Jesse Barnes]

On Mon, 24 Jun 2013 15:17:09 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> With updates to the spec, we can actually see the context layout, and
> how many dwords are allocated. That table suggests we need 70720 bytes
> per HW context. Rounded up, this is 18 pages. Looking at what lives
> after the current 4 pages we use, I can't see too much important (mostly
> it's d3d related), but there are a couple of things which look scary. I
> am hopeful this can explain some of our odd HSW failures.
> 
> v2: Make the context only 17 pages. The power context space isn't used
> ever, and execlists aren't used in our driver, making the actual total
> 66944 bytes.
> 
> Reported-by: "Azad, Vinit" <vinit.azad@intel.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
>  drivers/gpu/drm/i915/i915_reg.h         | 9 +--------
>  2 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index ff47145..51b7a21 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -113,7 +113,7 @@ static int get_context_size(struct drm_device *dev)
>  	case 7:
>  		reg = I915_READ(GEN7_CXT_SIZE);
>  		if (IS_HASWELL(dev))
> -			ret = HSW_CXT_TOTAL_SIZE(reg) * 64;
> +			ret = HSW_CXT_TOTAL_SIZE;
>  		else
>  			ret = GEN7_CXT_TOTAL_SIZE(reg) * 64;
>  		break;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2102ff3..f061c67 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1718,14 +1718,7 @@
>  					 GEN7_CXT_EXTENDED_SIZE(ctx_reg) + \
>  					 GEN7_CXT_GT1_SIZE(ctx_reg) + \
>  					 GEN7_CXT_VFSTATE_SIZE(ctx_reg))
> -#define HSW_CXT_POWER_SIZE(ctx_reg)	((ctx_reg >> 26) & 0x3f)
> -#define HSW_CXT_RING_SIZE(ctx_reg)	((ctx_reg >> 23) & 0x7)
> -#define HSW_CXT_RENDER_SIZE(ctx_reg)	((ctx_reg >> 15) & 0xff)
> -#define HSW_CXT_TOTAL_SIZE(ctx_reg)	(HSW_CXT_POWER_SIZE(ctx_reg) + \
> -					 HSW_CXT_RING_SIZE(ctx_reg) + \
> -					 HSW_CXT_RENDER_SIZE(ctx_reg) + \
> -					 GEN7_CXT_VFSTATE_SIZE(ctx_reg))
> -
> +#define HSW_CXT_TOTAL_SIZE		(17 * PAGE_SIZE)
>  
>  /*
>   * Overlay regs

Yeah, we can't rely on the reg I guess, and this matches the docs
(though they don't make it easy to figure it out)...  I'd like to see a
reference to the bspec where this information is kept, but other than
that:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] [v3] drm/i915: Fix context sizes on HSW

[Ben Widawsky]

With updates to the spec, we can actually see the context layout, and
how many dwords are allocated. That table suggests we need 70720 bytes
per HW context. Rounded up, this is 18 pages. Looking at what lives
after the current 4 pages we use, I can't see too much important (mostly
it's d3d related), but there are a couple of things which look scary. I
am hopeful this can explain some of our odd HSW failures.

v2: Make the context only 17 pages. The power context space isn't used
ever, and execlists aren't used in our driver, making the actual total
66944 bytes.

v3: Add a comment to the code. (Jesse & Paulo)

Reported-by: "Azad, Vinit" <vinit.azad@intel.com>
Cc: stable@vger.kernel.org
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
 drivers/gpu/drm/i915/i915_reg.h         | 15 +++++++--------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index ff47145..51b7a21 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -113,7 +113,7 @@ static int get_context_size(struct drm_device *dev)
 	case 7:
 		reg = I915_READ(GEN7_CXT_SIZE);
 		if (IS_HASWELL(dev))
-			ret = HSW_CXT_TOTAL_SIZE(reg) * 64;
+			ret = HSW_CXT_TOTAL_SIZE;
 		else
 			ret = GEN7_CXT_TOTAL_SIZE(reg) * 64;
 		break;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2102ff3..a426168 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1718,14 +1718,13 @@
 					 GEN7_CXT_EXTENDED_SIZE(ctx_reg) + \
 					 GEN7_CXT_GT1_SIZE(ctx_reg) + \
 					 GEN7_CXT_VFSTATE_SIZE(ctx_reg))
-#define HSW_CXT_POWER_SIZE(ctx_reg)	((ctx_reg >> 26) & 0x3f)
-#define HSW_CXT_RING_SIZE(ctx_reg)	((ctx_reg >> 23) & 0x7)
-#define HSW_CXT_RENDER_SIZE(ctx_reg)	((ctx_reg >> 15) & 0xff)
-#define HSW_CXT_TOTAL_SIZE(ctx_reg)	(HSW_CXT_POWER_SIZE(ctx_reg) + \
-					 HSW_CXT_RING_SIZE(ctx_reg) + \
-					 HSW_CXT_RENDER_SIZE(ctx_reg) + \
-					 GEN7_CXT_VFSTATE_SIZE(ctx_reg))
-
+/* Haswell does have the CXT_SIZE register however it does not appear to be
+ * valid. Now, docs explain in dwords what is in the context object. The full
+ * size is 70720 bytes, however, the power context and execlist context will
+ * never be saved (power context is stored elsewhere, and execlists don't work
+ * on HSW) - so the final size is 66944 bytes, which rounds to 17 pages.
+ */
+#define HSW_CXT_TOTAL_SIZE		(17 * PAGE_SIZE)
 
 /*
  * Overlay regs
-- 
1.8.3.1

Re: [Intel-gfx] [PATCH] [v3] drm/i915: Fix context sizes on HSW

[Daniel Vetter]

On Tue, Jun 25, 2013 at 09:53:40PM -0700, Ben Widawsky wrote:
> With updates to the spec, we can actually see the context layout, and
> how many dwords are allocated. That table suggests we need 70720 bytes
> per HW context. Rounded up, this is 18 pages. Looking at what lives
> after the current 4 pages we use, I can't see too much important (mostly
> it's d3d related), but there are a couple of things which look scary. I
> am hopeful this can explain some of our odd HSW failures.
> 
> v2: Make the context only 17 pages. The power context space isn't used
> ever, and execlists aren't used in our driver, making the actual total
> 66944 bytes.
> 
> v3: Add a comment to the code. (Jesse & Paulo)
> 
> Reported-by: "Azad, Vinit" <vinit.azad@intel.com>
> Cc: stable@vger.kernel.org
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

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