| Message ID | 1349318064-22051-1-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ;-) 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 >
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 ;-) 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 > > >
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 > > >
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
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> --- drivers/gpu/drm/i915/intel_ringbuffer.c | 8 -------- 1 file changed, 8 deletions(-)