[PATCH] drm/i915: Fix pte updates in ggtt clear range

[PATCH] drm/i915: Fix pte updates in ggtt clear range

[Ben Widawsky]

This bug was introduced by me:
commit e76e9aebcdbfebae8f4cd147e3c0f800d36e97f3
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Sun Nov 4 09:21:27 2012 -0800

    drm/i915: Stop using AGP layer for GEN6+

The existing code uses memset_io which follows memset semantics in only
guaranteeing a write of individual bytes. Since a PTE entry is 4 bytes,
this can only be correct if the scratch page address is 0.

This caused unsightly errors when we clear the range at load time,
though I'm not really sure what the heck is referencing that memory
anyway. I caught this is because I believe we have some other bug where
the display is doing reads of memory we feel should be cleared (or we
are relying on scratch pages to be a specific value).

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

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 51f79bb..f7ac61e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -367,8 +367,9 @@ static void i915_ggtt_clear_range(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	gtt_pte_t scratch_pte;
-	volatile void __iomem *gtt_base = dev_priv->mm.gtt->gtt + first_entry;
+	gtt_pte_t __iomem *gtt_base = dev_priv->mm.gtt->gtt + first_entry;
 	const int max_entries = dev_priv->mm.gtt->gtt_total_entries - first_entry;
+	int i;
 
 	if (INTEL_INFO(dev)->gen < 6) {
 		intel_gtt_clear_range(first_entry, num_entries);
@@ -381,7 +382,8 @@ static void i915_ggtt_clear_range(struct drm_device *dev,
 		num_entries = max_entries;
 
 	scratch_pte = pte_encode(dev, dev_priv->mm.gtt->scratch_page_dma, I915_CACHE_LLC);
-	memset_io(gtt_base, scratch_pte, num_entries * sizeof(scratch_pte));
+	for (i = 0; i < num_entries; i++)
+		iowrite32(scratch_pte, &gtt_base[i]);
 	readl(gtt_base);
 }
 
-- 
1.8.0.1

Re: [PATCH] drm/i915: Fix pte updates in ggtt clear range

[Chris Wilson]

On Mon, 26 Nov 2012 21:52:54 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> This bug was introduced by me:
> commit e76e9aebcdbfebae8f4cd147e3c0f800d36e97f3
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Sun Nov 4 09:21:27 2012 -0800
> 
>     drm/i915: Stop using AGP layer for GEN6+
> 
> The existing code uses memset_io which follows memset semantics in only
> guaranteeing a write of individual bytes. Since a PTE entry is 4 bytes,
> this can only be correct if the scratch page address is 0.

Gah. Wasn't there an iowrite32_rep?
 
> This caused unsightly errors when we clear the range at load time,
> though I'm not really sure what the heck is referencing that memory
> anyway. I caught this is because I believe we have some other bug where
> the display is doing reads of memory we feel should be cleared (or we
> are relying on scratch pages to be a specific value).

That's just because we are no longer disabling outputs before updating
the GTT and hence continue to scanout from the BIOS fb during module
load. It's a regression that we'll be able to finally fix properly with
fastboot - though that will not be without its downsides either.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix pte updates in ggtt clear range

[Ben Widawsky]

On Tue, 27 Nov 2012 08:22:01 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, 26 Nov 2012 21:52:54 -0800, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > This bug was introduced by me:
> > commit e76e9aebcdbfebae8f4cd147e3c0f800d36e97f3
> > Author: Ben Widawsky <ben@bwidawsk.net>
> > Date:   Sun Nov 4 09:21:27 2012 -0800
> > 
> >     drm/i915: Stop using AGP layer for GEN6+
> > 
> > The existing code uses memset_io which follows memset semantics in
> > only guaranteeing a write of individual bytes. Since a PTE entry is
> > 4 bytes, this can only be correct if the scratch page address is 0.
> 
> Gah. Wasn't there an iowrite32_rep?

And you would hope it does what you want... but it seems like just a
memcpy of dword sized chunks.

My first thought was to write my own memset which does what you want,
but this seemed like a path of less resistance.

>  
> > This caused unsightly errors when we clear the range at load time,
> > though I'm not really sure what the heck is referencing that memory
> > anyway. I caught this is because I believe we have some other bug
> > where the display is doing reads of memory we feel should be
> > cleared (or we are relying on scratch pages to be a specific value).
> 
> That's just because we are no longer disabling outputs before updating
> the GTT and hence continue to scanout from the BIOS fb during module
> load. It's a regression that we'll be able to finally fix properly
> with fastboot - though that will not be without its downsides either.
> -Chris
> 

Re: [PATCH] drm/i915: Fix pte updates in ggtt clear range

[Chris Wilson]

On Tue, 27 Nov 2012 08:32:57 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Tue, 27 Nov 2012 08:22:01 +0000
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Mon, 26 Nov 2012 21:52:54 -0800, Ben Widawsky <ben@bwidawsk.net>
> > wrote:
> > > This bug was introduced by me:
> > > commit e76e9aebcdbfebae8f4cd147e3c0f800d36e97f3
> > > Author: Ben Widawsky <ben@bwidawsk.net>
> > > Date:   Sun Nov 4 09:21:27 2012 -0800
> > > 
> > >     drm/i915: Stop using AGP layer for GEN6+
> > > 
> > > The existing code uses memset_io which follows memset semantics in
> > > only guaranteeing a write of individual bytes. Since a PTE entry is
> > > 4 bytes, this can only be correct if the scratch page address is 0.
> > 
> > Gah. Wasn't there an iowrite32_rep?
> 
> And you would hope it does what you want... but it seems like just a
> memcpy of dword sized chunks.

I feel suitably embarassed at missing it the first time, especially as
we've had similar conversations in the past.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix pte updates in ggtt clear range

[Ben Widawsky]

On Mon, 26 Nov 2012 21:52:54 -0800
Ben Widawsky <ben@bwidawsk.net> wrote:

> This bug was introduced by me:
> commit e76e9aebcdbfebae8f4cd147e3c0f800d36e97f3
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Sun Nov 4 09:21:27 2012 -0800
> 
>     drm/i915: Stop using AGP layer for GEN6+
> 
> The existing code uses memset_io which follows memset semantics in only
> guaranteeing a write of individual bytes. Since a PTE entry is 4 bytes,
> this can only be correct if the scratch page address is 0.
> 
> This caused unsightly errors when we clear the range at load time,
> though I'm not really sure what the heck is referencing that memory
> anyway. I caught this is because I believe we have some other bug where
> the display is doing reads of memory we feel should be cleared (or we
> are relying on scratch pages to be a specific value).
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 51f79bb..f7ac61e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -367,8 +367,9 @@ static void i915_ggtt_clear_range(struct drm_device *dev,
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	gtt_pte_t scratch_pte;
> -	volatile void __iomem *gtt_base = dev_priv->mm.gtt->gtt + first_entry;
> +	gtt_pte_t __iomem *gtt_base = dev_priv->mm.gtt->gtt + first_entry;
>  	const int max_entries = dev_priv->mm.gtt->gtt_total_entries - first_entry;
> +	int i;
>  
>  	if (INTEL_INFO(dev)->gen < 6) {
>  		intel_gtt_clear_range(first_entry, num_entries);
> @@ -381,7 +382,8 @@ static void i915_ggtt_clear_range(struct drm_device *dev,
>  		num_entries = max_entries;
>  
>  	scratch_pte = pte_encode(dev, dev_priv->mm.gtt->scratch_page_dma, I915_CACHE_LLC);
> -	memset_io(gtt_base, scratch_pte, num_entries * sizeof(scratch_pte));
> +	for (i = 0; i < num_entries; i++)
> +		iowrite32(scratch_pte, &gtt_base[i]);
>  	readl(gtt_base);
>  }
>  

"This has been slurped into -queued. Thanks for the patch" - or
something like that.

-- 
Ben Widawsky, Intel Open Source Technology Center