[PATCH 1/2] drm/i915: Only print information for filing bug reports once

[PATCH 1/2] drm/i915: Only print information for filing bug reports once

[Chris Wilson]

Repeating the same information multiple times is just annoying.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 8e6d8f744e7e..6d5ab945132c 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -980,6 +980,7 @@ static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
  */
 void i915_capture_error_state(struct drm_device *dev)
 {
+	static bool warned;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_error_state *error;
 	unsigned long flags;
@@ -1000,10 +1001,13 @@ void i915_capture_error_state(struct drm_device *dev)
 
 	DRM_INFO("GPU crash dump saved to /sys/class/drm/card%d/error\n",
 		 dev->primary->index);
-	DRM_INFO("GPU hangs can indicate a bug anywhere in the entire gfx stack, including userspace.\n");
-	DRM_INFO("Please file a _new_ bug report on bugs.freedesktop.org against DRI -> DRM/Intel\n");
-	DRM_INFO("drm/i915 developers can then reassign to the right component if it's not a kernel issue.\n");
-	DRM_INFO("The gpu crash dump is required to analyze gpu hangs, so please always attach it.\n");
+	if (!warned) {
+		DRM_INFO("GPU hangs can indicate a bug anywhere in the entire gfx stack, including userspace.\n");
+		DRM_INFO("Please file a _new_ bug report on bugs.freedesktop.org against DRI -> DRM/Intel\n");
+		DRM_INFO("drm/i915 developers can then reassign to the right component if it's not a kernel issue.\n");
+		DRM_INFO("The gpu crash dump is required to analyze gpu hangs, so please always attach it.\n");
+		warned = true;
+	}
 
 	kref_init(&error->ref);
 	error->eir = I915_READ(EIR);
-- 
1.9.rc1

[PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)

[Chris Wilson]

We want to use the GTT for reading back objects upon an error so that we
have exactly the information that the GPU saw. However, it is verboten
to access snoopable pages through the GTT and causes my PineView GPU to
throw a page fault instead.

This has not been a problem in the past as we only dumped ringbuffers
and batchbuffers, both of which must be uncached. However, the
introduction of HWS page dumping leads to a read of a snooped object
through the GTT. This was introduced by

commit f3ce3821393e31a3f1a8ca6c24eb2d735a428445
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Jan 23 22:40:36 2014 +0000

    drm/i915: Include HW status page in error capture

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 6d5ab945132c..3b18c2dff3b8 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -527,7 +527,8 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
 			goto unwind;
 
 		local_irq_save(flags);
-		if (reloc_offset < dev_priv->gtt.mappable_end &&
+		if (src->cache_level == I915_CACHE_NONE &&
+		    reloc_offset < dev_priv->gtt.mappable_end &&
 		    src->has_global_gtt_mapping &&
 		    i915_is_ggtt(vm)) {
 			void __iomem *s;
-- 
1.9.rc1

Re: [PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)

[Chris Wilson]

On Thu, Jan 30, 2014 at 02:38:16PM +0000, Chris Wilson wrote:
> We want to use the GTT for reading back objects upon an error so that we
> have exactly the information that the GPU saw. However, it is verboten
> to access snoopable pages through the GTT and causes my PineView GPU to
> throw a page fault instead.
> 
> This has not been a problem in the past as we only dumped ringbuffers
> and batchbuffers, both of which must be uncached. However, the

s/uncached/not snooped/ to avoid confusion on SNB and friends.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)

[Ville Syrjälä]

On Thu, Jan 30, 2014 at 02:38:16PM +0000, Chris Wilson wrote:
> We want to use the GTT for reading back objects upon an error so that we
> have exactly the information that the GPU saw. However, it is verboten
> to access snoopable pages through the GTT and causes my PineView GPU to
> throw a page fault instead.
> 
> This has not been a problem in the past as we only dumped ringbuffers
> and batchbuffers, both of which must be uncached. However, the
> introduction of HWS page dumping leads to a read of a snooped object
> through the GTT. This was introduced by
> 
> commit f3ce3821393e31a3f1a8ca6c24eb2d735a428445
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu Jan 23 22:40:36 2014 +0000
> 
>     drm/i915: Include HW status page in error capture
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 6d5ab945132c..3b18c2dff3b8 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -527,7 +527,8 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
>  			goto unwind;
>  
>  		local_irq_save(flags);
> -		if (reloc_offset < dev_priv->gtt.mappable_end &&
> +		if (src->cache_level == I915_CACHE_NONE &&
> +		    reloc_offset < dev_priv->gtt.mappable_end &&
>  		    src->has_global_gtt_mapping &&
>  		    i915_is_ggtt(vm)) {
>  			void __iomem *s;

So you don't want to make it (cache_level == NONE || HAS_LLC)?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)

[Chris Wilson]

On Thu, Jan 30, 2014 at 05:08:09PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 30, 2014 at 02:38:16PM +0000, Chris Wilson wrote:
> > We want to use the GTT for reading back objects upon an error so that we
> > have exactly the information that the GPU saw. However, it is verboten
> > to access snoopable pages through the GTT and causes my PineView GPU to
> > throw a page fault instead.
> > 
> > This has not been a problem in the past as we only dumped ringbuffers
> > and batchbuffers, both of which must be uncached. However, the
> > introduction of HWS page dumping leads to a read of a snooped object
> > through the GTT. This was introduced by
> > 
> > commit f3ce3821393e31a3f1a8ca6c24eb2d735a428445
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Thu Jan 23 22:40:36 2014 +0000
> > 
> >     drm/i915: Include HW status page in error capture
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 6d5ab945132c..3b18c2dff3b8 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -527,7 +527,8 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
> >  			goto unwind;
> >  
> >  		local_irq_save(flags);
> > -		if (reloc_offset < dev_priv->gtt.mappable_end &&
> > +		if (src->cache_level == I915_CACHE_NONE &&
> > +		    reloc_offset < dev_priv->gtt.mappable_end &&
> >  		    src->has_global_gtt_mapping &&
> >  		    i915_is_ggtt(vm)) {
> >  			void __iomem *s;
> 
> So you don't want to make it (cache_level == NONE || HAS_LLC)?

That llc is coherent between the physical pages and GTT is deep rooted
in our code by now, and Ben is working hard to make sure we never take
that path on llc anyway. Hence I let it go.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)

[Ville Syrjälä]

On Thu, Jan 30, 2014 at 03:11:08PM +0000, Chris Wilson wrote:
> On Thu, Jan 30, 2014 at 05:08:09PM +0200, Ville Syrjälä wrote:
> > On Thu, Jan 30, 2014 at 02:38:16PM +0000, Chris Wilson wrote:
> > > We want to use the GTT for reading back objects upon an error so that we
> > > have exactly the information that the GPU saw. However, it is verboten
> > > to access snoopable pages through the GTT and causes my PineView GPU to
> > > throw a page fault instead.
> > > 
> > > This has not been a problem in the past as we only dumped ringbuffers
> > > and batchbuffers, both of which must be uncached. However, the
> > > introduction of HWS page dumping leads to a read of a snooped object
> > > through the GTT. This was introduced by
> > > 
> > > commit f3ce3821393e31a3f1a8ca6c24eb2d735a428445
> > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > Date:   Thu Jan 23 22:40:36 2014 +0000
> > > 
> > >     drm/i915: Include HW status page in error capture
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > index 6d5ab945132c..3b18c2dff3b8 100644
> > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > @@ -527,7 +527,8 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
> > >  			goto unwind;
> > >  
> > >  		local_irq_save(flags);
> > > -		if (reloc_offset < dev_priv->gtt.mappable_end &&
> > > +		if (src->cache_level == I915_CACHE_NONE &&
> > > +		    reloc_offset < dev_priv->gtt.mappable_end &&
> > >  		    src->has_global_gtt_mapping &&
> > >  		    i915_is_ggtt(vm)) {
> > >  			void __iomem *s;
> > 
> > So you don't want to make it (cache_level == NONE || HAS_LLC)?
> 
> That llc is coherent between the physical pages and GTT is deep rooted
> in our code by now, and Ben is working hard to make sure we never take
> that path on llc anyway. Hence I let it go.

Fine by me. For the series:

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)

[Daniel Vetter]

On Thu, Jan 30, 2014 at 05:32:10PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 30, 2014 at 03:11:08PM +0000, Chris Wilson wrote:
> > On Thu, Jan 30, 2014 at 05:08:09PM +0200, Ville Syrjälä wrote:
> > > On Thu, Jan 30, 2014 at 02:38:16PM +0000, Chris Wilson wrote:
> > > > We want to use the GTT for reading back objects upon an error so that we
> > > > have exactly the information that the GPU saw. However, it is verboten
> > > > to access snoopable pages through the GTT and causes my PineView GPU to
> > > > throw a page fault instead.
> > > > 
> > > > This has not been a problem in the past as we only dumped ringbuffers
> > > > and batchbuffers, both of which must be uncached. However, the
> > > > introduction of HWS page dumping leads to a read of a snooped object
> > > > through the GTT. This was introduced by
> > > > 
> > > > commit f3ce3821393e31a3f1a8ca6c24eb2d735a428445
> > > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Date:   Thu Jan 23 22:40:36 2014 +0000
> > > > 
> > > >     drm/i915: Include HW status page in error capture
> > > > 
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > index 6d5ab945132c..3b18c2dff3b8 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > @@ -527,7 +527,8 @@ i915_error_object_create_sized(struct drm_i915_private *dev_priv,
> > > >  			goto unwind;
> > > >  
> > > >  		local_irq_save(flags);
> > > > -		if (reloc_offset < dev_priv->gtt.mappable_end &&
> > > > +		if (src->cache_level == I915_CACHE_NONE &&
> > > > +		    reloc_offset < dev_priv->gtt.mappable_end &&
> > > >  		    src->has_global_gtt_mapping &&
> > > >  		    i915_is_ggtt(vm)) {
> > > >  			void __iomem *s;
> > > 
> > > So you don't want to make it (cache_level == NONE || HAS_LLC)?
> > 
> > That llc is coherent between the physical pages and GTT is deep rooted
> > in our code by now, and Ben is working hard to make sure we never take
> > that path on llc anyway. Hence I let it go.
> 
> Fine by me. For the series:
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Both merge, with uncached rectified.

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