Re: [PATCH 1/4] drm/i915: Remove duplicate cache workaround

[Ben Widawsky <ben@bwidawsk.net>]

On Thu, 4 Oct 2012 09:01:17 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Oct 03, 2012 at 07:34:21PM -0700, Ben Widawsky wrote:
> > This is already achieved in init_clock gating, and is probably the
> > result of a bad merge from Daniel. I'm too tired to bet on him making a
> > mistake though.
> > 
> > CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Citing from the commit message that added this 2nd copy that you've just
> removed:
> 
> "This cannot be done in gen6_init_clock_gating with most of the other
> workaround bits; the render ring needs to exist.  Otherwise, the
> register write gets dropped on the floor (one printk will show it
> changed, but a second printk immediately following shows the value
> reverts to the old one)."
> 
> Congrats, you've removed the wrong copy ;-)

On further thought, I think we need to investigate the original issue
here more anyway. I've been speaking with Ken, and neither of us really
understand why the later write is required (it seems he authored the
original patch). I think we all agree we'd like to jam these kind of
workarounds into the clock gating functions (even though that is now a
terrible name for it). Ken has tried this patch locally, and it works
fine.

It seems Matt Turner has the original machine, so it'd be great if he
can test this patch. Matt this is the t420s you got from Ken.

> 
> Now I suspect not all of the w/a patches currently floating have seen
> level of testing, and I'd wager a few suffer from the same. So I think we
> need checks in i-g-t. Eric has started this with the intel_reg_checker
> tool, but we lack an aweful lot of recent workarounds. Also, we need to
> integrate a call to this tool in the testsuite, once at least at the
> beginning somewhere (to check boot-up state) but also in ZZ_hangman (in
> case the reset botched things up). We really should add a suspend/resume
> testcase in there, too ...
> 
> Cheers, Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 8 --------
> >  1 file changed, 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 984a0c5..625a348 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -521,14 +521,6 @@ static int init_render_ring(struct intel_ring_buffer *ring)
> >  	}
> >  
> >  	if (IS_GEN6(dev)) {
> > -		/* From the Sandybridge PRM, volume 1 part 3, page 24:
> > -		 * "If this bit is set, STCunit will have LRA as replacement
> > -		 *  policy. [...] This bit must be reset.  LRA replacement
> > -		 *  policy is not supported."
> > -		 */
> > -		I915_WRITE(CACHE_MODE_0,
> > -			   _MASKED_BIT_DISABLE(CM0_STC_EVICT_DISABLE_LRA_SNB));
> > -
> >  		/* This is not explicitly set for GEN6, so read the register.
> >  		 * see intel_ring_mi_set_context() for why we care.
> >  		 * TODO: consider explicitly setting the bit for GEN5
> > -- 
> > 1.7.12.2
> > 
> 



-- 
Ben Widawsky, Intel Open Source Technology Center