* [PATCH 1/4] drm/i915: Remove duplicate cache workaround
@ 2012-10-04 2:34 Ben Widawsky
2012-10-04 2:34 ` [PATCH 2/4] drm/i915: Disable render depth cache pipeline flush Ben Widawsky
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Ben Widawsky @ 2012-10-04 2:34 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Ben Widawsky
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(-)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/4] drm/i915: Disable render depth cache pipeline flush 2012-10-04 2:34 [PATCH 1/4] drm/i915: Remove duplicate cache workaround Ben Widawsky @ 2012-10-04 2:34 ` Ben Widawsky 2012-10-04 2:34 ` [PATCH 3/4] drm/i915: ILK also needs that last fix Ben Widawsky ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Ben Widawsky @ 2012-10-04 2:34 UTC (permalink / raw) To: intel-gfx; +Cc: Ben Widawsky Required through gen6. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 2 ++ 2 files changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index d17bef7..37e62e0 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -662,6 +662,7 @@ #define MI_ARB_DISPLAY_PRIORITY_B_A (1 << 0) /* display B > display A */ #define CACHE_MODE_0 0x02120 /* 915+ only */ +#define CM0_RC_PIPELINE_FLUSH_DISABLE (1<<8) #define CM0_IZ_OPT_DISABLE (1<<6) #define CM0_ZR_OPT_DISABLE (1<<5) #define CM0_STC_EVICT_DISABLE_LRA_SNB (1<<5) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a3e4f8b..f1800ca 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3385,6 +3385,8 @@ static void gen6_init_clock_gating(struct drm_device *dev) I915_WRITE(CACHE_MODE_0, _MASKED_BIT_DISABLE(CM0_STC_EVICT_DISABLE_LRA_SNB)); + I915_WRITE(CACHE_MODE_0, + _MASKED_BIT_ENABLE(CM0_RC_PIPELINE_FLUSH_DISABLE)); I915_WRITE(GEN6_UCGCTL1, I915_READ(GEN6_UCGCTL1) | -- 1.7.12.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] drm/i915: ILK also needs that last fix 2012-10-04 2:34 [PATCH 1/4] drm/i915: Remove duplicate cache workaround Ben Widawsky 2012-10-04 2:34 ` [PATCH 2/4] drm/i915: Disable render depth cache pipeline flush Ben Widawsky @ 2012-10-04 2:34 ` Ben Widawsky 2012-10-15 18:59 ` Daniel Vetter 2012-10-04 2:34 ` [PATCH 4/4] drm/i915: Fix GT_MODE default value Ben Widawsky 2012-10-04 7:01 ` [PATCH 1/4] drm/i915: Remove duplicate cache workaround Daniel Vetter 3 siblings, 1 reply; 11+ messages in thread From: Ben Widawsky @ 2012-10-04 2:34 UTC (permalink / raw) To: intel-gfx; +Cc: Ben Widawsky That fix was the disable render deptch cache pipeline flush Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/intel_pm.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index f1800ca..8aafa45 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3338,6 +3338,8 @@ static void ironlake_init_clock_gating(struct drm_device *dev) I915_WRITE(WM2_LP_ILK, 0); I915_WRITE(WM1_LP_ILK, 0); + I915_WRITE(CACHE_MODE_0, + _MASKED_BIT_ENABLE(CM0_RC_PIPELINE_FLUSH_DISABLE)); /* * Based on the document from hardware guys the following bits * should be set unconditionally in order to enable FBC. -- 1.7.12.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] drm/i915: ILK also needs that last fix 2012-10-04 2:34 ` [PATCH 3/4] drm/i915: ILK also needs that last fix Ben Widawsky @ 2012-10-15 18:59 ` Daniel Vetter 2012-10-16 3:21 ` Ben Widawsky 0 siblings, 1 reply; 11+ messages in thread From: Daniel Vetter @ 2012-10-15 18:59 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx On Wed, Oct 03, 2012 at 07:34:23PM -0700, Ben Widawsky wrote: > That fix was the disable render deptch cache pipeline flush > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> I've stumbled over the same one, but my docs here suggest i965g/gm45 need it, too: http://cgit.freedesktop.org/~danvet/drm/commit/?h=ilk-wa-pile&id=37c4c82b8cdbcf5adccad97f0b45747ba37ed659 Have you checked whether we don't need this on ivb/vlv/hsw, too? Also, for w/a patches based on the vpg w/a database, please include the vpg w/a name tag both in the commit message and in a code comment somewhere. -Daniel > --- > drivers/gpu/drm/i915/intel_pm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index f1800ca..8aafa45 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3338,6 +3338,8 @@ static void ironlake_init_clock_gating(struct drm_device *dev) > I915_WRITE(WM2_LP_ILK, 0); > I915_WRITE(WM1_LP_ILK, 0); > > + I915_WRITE(CACHE_MODE_0, > + _MASKED_BIT_ENABLE(CM0_RC_PIPELINE_FLUSH_DISABLE)); > /* > * Based on the document from hardware guys the following bits > * should be set unconditionally in order to enable FBC. > -- > 1.7.12.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] drm/i915: ILK also needs that last fix 2012-10-15 18:59 ` Daniel Vetter @ 2012-10-16 3:21 ` Ben Widawsky 2012-10-16 7:16 ` Daniel Vetter 0 siblings, 1 reply; 11+ messages in thread From: Ben Widawsky @ 2012-10-16 3:21 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Mon, 15 Oct 2012 20:59:22 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Oct 03, 2012 at 07:34:23PM -0700, Ben Widawsky wrote: > > That fix was the disable render deptch cache pipeline flush > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > I've stumbled over the same one, but my docs here suggest i965g/gm45 > need it, too: > > http://cgit.freedesktop.org/~danvet/drm/commit/?h=ilk-wa-pile&id=37c4c82b8cdbcf5adccad97f0b45747ba37ed659 > > Have you checked whether we don't need this on ivb/vlv/hsw, too? I did check whether the windows driver does it for those platforms, and the answer is no. So the answer to your question is maybe because who knows what exists in some other doc somewhere in the metaverse. I think this is a good enough start though since it seems SNB was definitely a bit buggier than IVB. > > Also, for w/a patches based on the vpg w/a database, please include > the vpg w/a name tag both in the commit message and in a code comment > somewhere. Good idea. If you're okay with longer commit message subjects, I'd even suggest putting it there to make it even a bit easier to search for. > -Daniel > > --- > > drivers/gpu/drm/i915/intel_pm.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c index f1800ca..8aafa45 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3338,6 +3338,8 @@ static void ironlake_init_clock_gating(struct > > drm_device *dev) I915_WRITE(WM2_LP_ILK, 0); > > I915_WRITE(WM1_LP_ILK, 0); > > > > + I915_WRITE(CACHE_MODE_0, > > + > > _MASKED_BIT_ENABLE(CM0_RC_PIPELINE_FLUSH_DISABLE)); /* > > * Based on the document from hardware guys the following > > bits > > * should be set unconditionally in order to enable FBC. > > -- > > 1.7.12.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] drm/i915: ILK also needs that last fix 2012-10-16 3:21 ` Ben Widawsky @ 2012-10-16 7:16 ` Daniel Vetter 0 siblings, 0 replies; 11+ messages in thread From: Daniel Vetter @ 2012-10-16 7:16 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx On Mon, Oct 15, 2012 at 08:21:41PM -0700, Ben Widawsky wrote: > On Mon, 15 Oct 2012 20:59:22 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Wed, Oct 03, 2012 at 07:34:23PM -0700, Ben Widawsky wrote: > > > That fix was the disable render deptch cache pipeline flush > > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > > I've stumbled over the same one, but my docs here suggest i965g/gm45 > > need it, too: > > > > http://cgit.freedesktop.org/~danvet/drm/commit/?h=ilk-wa-pile&id=37c4c82b8cdbcf5adccad97f0b45747ba37ed659 > > > > Have you checked whether we don't need this on ivb/vlv/hsw, too? > > I did check whether the windows driver does it for those platforms, and > the answer is no. So the answer to your question is maybe because who > knows what exists in some other doc somewhere in the metaverse. I think > this is a good enough start though since it seems SNB was definitely a > bit buggier than IVB. Yeah, I've noticed while checking w/as that they're not consistently named on older platforms. E.g. the above definitely exists on eaglelake, too, but named slightly different. So the w/a db doesn't pick up all uses. Hoooray! > > Also, for w/a patches based on the vpg w/a database, please include > > the vpg w/a name tag both in the commit message and in a code comment > > somewhere. > > Good idea. If you're okay with longer commit message subjects, I'd even > suggest putting it there to make it even a bit easier to search for. Yeah, I'm fine with putting it into the commit head, I've put it there myself. If the w/a only affects one platform we could try to squeeze the platform name into the headline, too. But having to read the commit message for that doesn't really hurt, either. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] drm/i915: Fix GT_MODE default value 2012-10-04 2:34 [PATCH 1/4] drm/i915: Remove duplicate cache workaround Ben Widawsky 2012-10-04 2:34 ` [PATCH 2/4] drm/i915: Disable render depth cache pipeline flush Ben Widawsky 2012-10-04 2:34 ` [PATCH 3/4] drm/i915: ILK also needs that last fix Ben Widawsky @ 2012-10-04 2:34 ` Ben Widawsky 2012-10-04 11:25 ` Daniel Vetter 2012-10-04 7:01 ` [PATCH 1/4] drm/i915: Remove duplicate cache workaround Daniel Vetter 3 siblings, 1 reply; 11+ messages in thread From: Ben Widawsky @ 2012-10-04 2:34 UTC (permalink / raw) To: intel-gfx; +Cc: Ben Widawsky I can't even find how I figured this might be needed anymore. But sure enough, the value I'm reading back on platforms doesn't match what the docs recommends. It seemed to fix Chris' GT1 in limited testing as well. Tested-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_pm.c | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 37e62e0..4c1f461 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -531,6 +531,9 @@ # define VS_TIMER_DISPATCH (1 << 6) # define MI_FLUSH_ENABLE (1 << 12) +#define GEN6_GT_MODE 0x20d0 +#define GEN6_GT_MODE_HI (1 << 9) + #define GFX_MODE 0x02520 #define GFX_MODE_GEN7 0x0229c #define RING_MODE_GEN7(ring) ((ring)->mmio_base+0x29c) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 8aafa45..e0574f9 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3446,6 +3446,11 @@ static void gen6_init_clock_gating(struct drm_device *dev) DISPPLANE_TRICKLE_FEED_DISABLE); intel_flush_display_plane(dev_priv, pipe); } + + /* The default value should be 0x200 according to docs, but the two + * platforms I checked have a 0 for this. (Maybe BIOS overrides?) */ + I915_WRITE(GEN6_GT_MODE, _MASKED_BIT_DISABLE(0xffff)); + I915_WRITE(GEN6_GT_MODE, _MASKED_BIT_ENABLE(GEN6_GT_MODE_HI)); } static void gen7_setup_fixed_func_scheduler(struct drm_i915_private *dev_priv) -- 1.7.12.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] drm/i915: Fix GT_MODE default value 2012-10-04 2:34 ` [PATCH 4/4] drm/i915: Fix GT_MODE default value Ben Widawsky @ 2012-10-04 11:25 ` Daniel Vetter 0 siblings, 0 replies; 11+ messages in thread From: Daniel Vetter @ 2012-10-04 11:25 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx On Wed, Oct 03, 2012 at 07:34:24PM -0700, Ben Widawsky wrote: > I can't even find how I figured this might be needed anymore. But sure > enough, the value I'm reading back on platforms doesn't match what the > docs recommends. > > It seemed to fix Chris' GT1 in limited testing as well. > > Tested-by: Chris Wilson <chris@chris-wilson.co.uk> Merged to -fixes, with the missing sob line forged. -Daniel > --- > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > drivers/gpu/drm/i915/intel_pm.c | 5 +++++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 37e62e0..4c1f461 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -531,6 +531,9 @@ > # define VS_TIMER_DISPATCH (1 << 6) > # define MI_FLUSH_ENABLE (1 << 12) > > +#define GEN6_GT_MODE 0x20d0 > +#define GEN6_GT_MODE_HI (1 << 9) > + > #define GFX_MODE 0x02520 > #define GFX_MODE_GEN7 0x0229c > #define RING_MODE_GEN7(ring) ((ring)->mmio_base+0x29c) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 8aafa45..e0574f9 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3446,6 +3446,11 @@ static void gen6_init_clock_gating(struct drm_device *dev) > DISPPLANE_TRICKLE_FEED_DISABLE); > intel_flush_display_plane(dev_priv, pipe); > } > + > + /* The default value should be 0x200 according to docs, but the two > + * platforms I checked have a 0 for this. (Maybe BIOS overrides?) */ > + I915_WRITE(GEN6_GT_MODE, _MASKED_BIT_DISABLE(0xffff)); > + I915_WRITE(GEN6_GT_MODE, _MASKED_BIT_ENABLE(GEN6_GT_MODE_HI)); > } > > static void gen7_setup_fixed_func_scheduler(struct drm_i915_private *dev_priv) > -- > 1.7.12.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] drm/i915: Remove duplicate cache workaround 2012-10-04 2:34 [PATCH 1/4] drm/i915: Remove duplicate cache workaround Ben Widawsky ` (2 preceding siblings ...) 2012-10-04 2:34 ` [PATCH 4/4] drm/i915: Fix GT_MODE default value Ben Widawsky @ 2012-10-04 7:01 ` Daniel Vetter 2012-10-04 14:55 ` Ben Widawsky 2012-10-05 0:14 ` Ben Widawsky 3 siblings, 2 replies; 11+ messages in thread From: Daniel Vetter @ 2012-10-04 7:01 UTC (permalink / raw) To: Ben Widawsky; +Cc: Daniel Vetter, intel-gfx 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 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] drm/i915: Remove duplicate cache workaround 2012-10-04 7:01 ` [PATCH 1/4] drm/i915: Remove duplicate cache workaround Daniel Vetter @ 2012-10-04 14:55 ` Ben Widawsky 2012-10-05 0:14 ` Ben Widawsky 1 sibling, 0 replies; 11+ messages in thread From: Ben Widawsky @ 2012-10-04 14:55 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx 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 > > > -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] drm/i915: Remove duplicate cache workaround 2012-10-04 7:01 ` [PATCH 1/4] drm/i915: Remove duplicate cache workaround Daniel Vetter 2012-10-04 14:55 ` Ben Widawsky @ 2012-10-05 0:14 ` Ben Widawsky 1 sibling, 0 replies; 11+ messages in thread From: Ben Widawsky @ 2012-10-05 0:14 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, mattst88 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-10-16 7:15 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-04 2:34 [PATCH 1/4] drm/i915: Remove duplicate cache workaround Ben Widawsky 2012-10-04 2:34 ` [PATCH 2/4] drm/i915: Disable render depth cache pipeline flush Ben Widawsky 2012-10-04 2:34 ` [PATCH 3/4] drm/i915: ILK also needs that last fix Ben Widawsky 2012-10-15 18:59 ` Daniel Vetter 2012-10-16 3:21 ` Ben Widawsky 2012-10-16 7:16 ` Daniel Vetter 2012-10-04 2:34 ` [PATCH 4/4] drm/i915: Fix GT_MODE default value Ben Widawsky 2012-10-04 11:25 ` Daniel Vetter 2012-10-04 7:01 ` [PATCH 1/4] drm/i915: Remove duplicate cache workaround Daniel Vetter 2012-10-04 14:55 ` Ben Widawsky 2012-10-05 0:14 ` Ben Widawsky
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.