[Intel-gfx] [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 at 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 at 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 at 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