On Thu, 4 Oct 2012 09:01:17 +0200
Daniel Vetter <daniel at 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 at ffwll.ch>
> > Signed-off-by: Ben Widawsky <ben at 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 ;-)
Would you be willing to verify this again? There is some interesting
information in the bspec that leads me to believe you've jumped to the
wrong conclusion regarding whether or not the render ring needs to
exist (I think there are other preconditions that I can workaround a
bit better).
>
> 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.
Just to clarify a bit here: I don't have an SNB here with me, so the
patches themselves were only booted with an IVB. I didn't test the ILK
patch at all because I don't have one to test with also. I did test
patch 2, and 4 on Jesse's machine (1 is obviously not testable without
the kernel actually running)
> 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 ...
>
I'll leave this discussion to the mail thread started on patch 10.
> 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
