Re: [PATCH 4/5] drm/i915: remap l3 on hw init

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, 25 May 2012 10:39:57 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Fri, 27 Apr 2012 17:40:20 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > If any l3 rows have been previously remapped, we must remap them after
> > GPU reset/resume too.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |    3 +++
> >  drivers/gpu/drm/i915/i915_gem.c |   26 ++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_reg.h |    3 +++
> >  3 files changed, 32 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 9505fc0..e9efe17 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -641,6 +641,8 @@ typedef struct drm_i915_private {
> >  		/** PPGTT used for aliasing the PPGTT with the GTT */
> >  		struct i915_hw_ppgtt *aliasing_ppgtt;
> >  
> > +		u32 *l3_remap_info;
> > +
> >  		struct shrinker inactive_shrinker;
> >  
> >  		/**
> > @@ -1290,6 +1292,7 @@ int __must_check i915_gem_object_set_domain(struct drm_i915_gem_object *obj,
> >  					    uint32_t write_domain);
> >  int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
> >  int __must_check i915_gem_init_hw(struct drm_device *dev);
> > +void i915_gem_l3_remap(struct drm_device *dev);
> >  void i915_gem_init_swizzling(struct drm_device *dev);
> >  void i915_gem_init_ppgtt(struct drm_device *dev);
> >  void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 7bc4a40..bb3ef9f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3475,6 +3475,30 @@ i915_gem_idle(struct drm_device *dev)
> >  	return 0;
> >  }
> >  
> > +void i915_gem_l3_remap(struct drm_device *dev)
> > +{
> > +	drm_i915_private_t *dev_priv = dev->dev_private;
> > +	u32 misccpctl;
> > +	int i;
> > +
> > +	if (!dev_priv->mm.l3_remap_info)
> > +		return;
> > +
> > +	WARN_ON(!IS_IVYBRIDGE(dev));
> 
> Should just be a return?

I had this here since it's a public function, and we have a history of
calling functions on the wrong generation. So it just helps us find out
a little sooner we're doing the wrong thing. As you point out below
though, this doesn't give the behavior we want. Thanks for catching it.


> 
> > +
> > +	misccpctl = I915_READ(GEN7_MISCCPCTL);
> > +	I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE);
> > +	POSTING_READ(GEN7_MISCCPCTL);
> > +
> > +	for (i = 0; i < GEN7_L3LOG_SIZE; i += 4)
> > +		I915_WRITE(GEN7_L3LOG_BASE + i, dev_priv->mm.l3_remap_info[i/4]);
> > +
> > +	/* Make sure all the writes land before disabling dop clock gating */
> > +	POSTING_READ(GEN7_L3LOG_BASE);
> 
> Same comment as before on the DOP gating...

I think we're good here now too.

> 
> > +
> > +	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> > +}
> > +
> >  void i915_gem_init_swizzling(struct drm_device *dev)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > @@ -3566,6 +3590,8 @@ i915_gem_init_hw(struct drm_device *dev)
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> >  	int ret;
> >  
> > +	i915_gem_l3_remap(dev);
> 
> Since this looks unconditional, so we'll get the backtrace on every
> non-IVB system?

Yes. That seems like a problem.

> 
> 
> 

Reviewed-by?

-- 
Ben Widawsky, Intel Open Source Technology Center