[PATCH] drm/i915: Re-enable rc6 w/fix

[PATCH] drm/i915: Re-enable rc6 w/fix

[Ben Widawsky]

This fixes a race condition with MI_SET_CONTEXT and setting of the
PWRCTXA register. If PWRCTXA ends up getting set before MI_SET_CONTEXT
completes, it's possible that the system will enter rc6, and try to
return to the default render context, which if unset, could cause a GPU
hang

Resolve https://bugzilla.kernel.org/show_bug.cgi?id=28582

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c      |    2 +-
 drivers/gpu/drm/i915/intel_display.c |   11 +++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 22ec066..e3c808d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -49,7 +49,7 @@ module_param_named(powersave, i915_powersave, int, 0600);
 unsigned int i915_semaphores = 0;
 module_param_named(semaphores, i915_semaphores, int, 0600);
 
-unsigned int i915_enable_rc6 = 0;
+unsigned int i915_enable_rc6 = 1;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 
 unsigned int i915_lvds_downclock = 0;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 49fb54f..5675610 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6616,7 +6616,7 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	 * GPU can automatically power down the render unit if given a page
 	 * to save state.
 	 */
-	ret = BEGIN_LP_RING(6);
+	ret = BEGIN_LP_RING(10);
 	if (ret) {
 		ironlake_teardown_rc6(dev);
 		return;
@@ -6630,12 +6630,15 @@ void ironlake_enable_rc6(struct drm_device *dev)
 		 MI_RESTORE_EXT_STATE_EN |
 		 MI_RESTORE_INHIBIT);
 	OUT_RING(MI_SUSPEND_FLUSH);
-	OUT_RING(MI_NOOP);
 	OUT_RING(MI_FLUSH);
+	OUT_RING(MI_LOAD_REGISTER_IMM(2));
+	OUT_RING(PWRCTXA);
+	OUT_RING(dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
+	OUT_RING(RSTDBYCTL);
+	OUT_RING(I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
 	ADVANCE_LP_RING();
 
-	I915_WRITE(PWRCTXA, dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
-	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
+	DRM_DEBUG_DRIVER("pwrctx offset: %p", dev_priv->pwrctx->gtt_offset);
 }
 
 
-- 
1.7.3.4

[PATCH] drm/i915: Re-enable rc6 w/fix

[Ben Widawsky]

This fixes a race condition with MI_SET_CONTEXT and setting of the
PWRCTXA register. If PWRCTXA ends up getting set before MI_SET_CONTEXT
completes, it's possible that the system will enter rc6, and try to
return to the default render context, which if unset, could cause a GPU
hang

Resolve https://bugzilla.kernel.org/show_bug.cgi?id=28582

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c      |    2 +-
 drivers/gpu/drm/i915/intel_display.c |   11 +++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 22ec066..e3c808d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -49,7 +49,7 @@ module_param_named(powersave, i915_powersave, int, 0600);
 unsigned int i915_semaphores = 0;
 module_param_named(semaphores, i915_semaphores, int, 0600);
 
-unsigned int i915_enable_rc6 = 0;
+unsigned int i915_enable_rc6 = 1;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 
 unsigned int i915_lvds_downclock = 0;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 49fb54f..5675610 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6616,7 +6616,7 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	 * GPU can automatically power down the render unit if given a page
 	 * to save state.
 	 */
-	ret = BEGIN_LP_RING(6);
+	ret = BEGIN_LP_RING(10);
 	if (ret) {
 		ironlake_teardown_rc6(dev);
 		return;
@@ -6630,12 +6630,15 @@ void ironlake_enable_rc6(struct drm_device *dev)
 		 MI_RESTORE_EXT_STATE_EN |
 		 MI_RESTORE_INHIBIT);
 	OUT_RING(MI_SUSPEND_FLUSH);
-	OUT_RING(MI_NOOP);
 	OUT_RING(MI_FLUSH);
+	OUT_RING(MI_LOAD_REGISTER_IMM(2));
+	OUT_RING(PWRCTXA);
+	OUT_RING(dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
+	OUT_RING(RSTDBYCTL);
+	OUT_RING(I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
 	ADVANCE_LP_RING();
 
-	I915_WRITE(PWRCTXA, dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
-	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
+	DRM_DEBUG_DRIVER("pwrctx offset: %p", dev_priv->pwrctx->gtt_offset);
 }
 
 
-- 
1.7.3.4

Re: [PATCH] drm/i915: Re-enable rc6 w/fix

[Ben Widawsky]

On Mon, Mar 14, 2011 at 09:55:01PM -0700, Ben Widawsky wrote:
> This fixes a race condition with MI_SET_CONTEXT and setting of the
> PWRCTXA register. If PWRCTXA ends up getting set before MI_SET_CONTEXT
> completes, it's possible that the system will enter rc6, and try to
> return to the default render context, which if unset, could cause a GPU
> hang
> 
> Resolve https://bugzilla.kernel.org/show_bug.cgi?id=28582

I'm still waiting for feedback on bugzilla if this patch works like the
previous. Just submitting it here for review while we wait...

Re: [PATCH] drm/i915: Re-enable rc6 w/fix

[Ben Widawsky]

On Mon, Mar 14, 2011 at 10:00:20PM -0700, Ben Widawsky wrote:
> On Mon, Mar 14, 2011 at 09:55:01PM -0700, Ben Widawsky wrote:
> > This fixes a race condition with MI_SET_CONTEXT and setting of the
> > PWRCTXA register. If PWRCTXA ends up getting set before MI_SET_CONTEXT
> > completes, it's possible that the system will enter rc6, and try to
> > return to the default render context, which if unset, could cause a GPU
> > hang
> > 
> > Resolve https://bugzilla.kernel.org/show_bug.cgi?id=28582
> 
> I'm still waiting for feedback on bugzilla if this patch works like the
> previous. Just submitting it here for review while we wait...

It appears that I've jumped the gun on this fix. I can sort of reason
that the LOAD_REGISTER_IMM doesn't work, perhaps because somehow the
MI_SET_CONTEXT is really slow, and the GPU executes the register load
out of order, thus invoking the original potential race.

But I followed up with the i915_gpu_idle(), and that too did not work...

Anyone have any ideas?

Ben

Re: [PATCH] drm/i915: Re-enable rc6 w/fix

[Chris Wilson]

On Mon, 14 Mar 2011 21:55:01 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> This fixes a race condition with MI_SET_CONTEXT and setting of the
> PWRCTXA register. If PWRCTXA ends up getting set before MI_SET_CONTEXT
> completes, it's possible that the system will enter rc6, and try to
> return to the default render context, which if unset, could cause a GPU
> hang

I mentioned this on the bug, but I'll repeat myself here for completeness.
 
> Resolve https://bugzilla.kernel.org/show_bug.cgi?id=28582

I understood the convention (to aide those running external scripts) was
either Bugzilla: or References:
 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |    2 +-
>  drivers/gpu/drm/i915/intel_display.c |   11 +++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 22ec066..e3c808d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -49,7 +49,7 @@ module_param_named(powersave, i915_powersave, int, 0600);
>  unsigned int i915_semaphores = 0;
>  module_param_named(semaphores, i915_semaphores, int, 0600);
>  
> -unsigned int i915_enable_rc6 = 0;
> +unsigned int i915_enable_rc6 = 1;
>  module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
>  
>  unsigned int i915_lvds_downclock = 0;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 49fb54f..5675610 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6616,7 +6616,7 @@ void ironlake_enable_rc6(struct drm_device *dev)
>  	 * GPU can automatically power down the render unit if given a page
>  	 * to save state.
>  	 */
> -	ret = BEGIN_LP_RING(6);
> +	ret = BEGIN_LP_RING(10);
>  	if (ret) {
>  		ironlake_teardown_rc6(dev);
>  		return;
> @@ -6630,12 +6630,15 @@ void ironlake_enable_rc6(struct drm_device *dev)
>  		 MI_RESTORE_EXT_STATE_EN |
>  		 MI_RESTORE_INHIBIT);
>  	OUT_RING(MI_SUSPEND_FLUSH);
> -	OUT_RING(MI_NOOP);
>  	OUT_RING(MI_FLUSH);
> +	OUT_RING(MI_LOAD_REGISTER_IMM(2));

I think I should update the comments to reflect what the spec says about
LOAD_REGISTER_IMM (even though I trust Daniel to have accurately
determined their impact on gen2/3)... The spec implies that the variable
length nature of the command is to handle 32/64 bit writes as opposed to
batch multiple writes into a single (ideally atomic) command.

> +	OUT_RING(PWRCTXA);
> +	OUT_RING(dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
> +	OUT_RING(RSTDBYCTL);
> +	OUT_RING(I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
>  	ADVANCE_LP_RING();
>  
> -	I915_WRITE(PWRCTXA, dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
> -	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
> +	DRM_DEBUG_DRIVER("pwrctx offset: %p", dev_priv->pwrctx->gtt_offset);

gtt_offset is not a pointer. What would be more convenient would be a
const string to name these in the debugfs objects lists.

Having enqueued commands to write the registers via the GPU, we need to
make sure those have been retired before attempting to modify the same
registers directly during teardown.

Having made rc6 the default, we can also use the module parameter to
coarsely control SNB rc6, which may help with potential bug diagnosis and
simply reassuring ourselves that it works.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Re-enable rc6 w/fix

[Chris Wilson]

On Tue, 15 Mar 2011 00:12:51 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Mon, Mar 14, 2011 at 10:00:20PM -0700, Ben Widawsky wrote:
> > On Mon, Mar 14, 2011 at 09:55:01PM -0700, Ben Widawsky wrote:
> > > This fixes a race condition with MI_SET_CONTEXT and setting of the
> > > PWRCTXA register. If PWRCTXA ends up getting set before MI_SET_CONTEXT
> > > completes, it's possible that the system will enter rc6, and try to
> > > return to the default render context, which if unset, could cause a GPU
> > > hang
> > > 
> > > Resolve https://bugzilla.kernel.org/show_bug.cgi?id=28582
> > 
> > I'm still waiting for feedback on bugzilla if this patch works like the
> > previous. Just submitting it here for review while we wait...
> 
> It appears that I've jumped the gun on this fix. I can sort of reason
> that the LOAD_REGISTER_IMM doesn't work, perhaps because somehow the
> MI_SET_CONTEXT is really slow, and the GPU executes the register load
> out of order, thus invoking the original potential race.

If in doubt add a second MI_FLUSH, that is guaranteed to block on the
retirement of the first. But I suspect the actual bug is the misuse of
LOAD_REGISTER_IMM.
 
> But I followed up with the i915_gpu_idle(), and that too did not work...

Yeah, because the rings were idle (no outstanding requests and no active
buffers) it was a no-op. So there is no shortcut, you have to add a
request and then wait (or then idle). The alternative is to add the
HEAD!=TAIL polling to i915_gpu_idle as well, but i915_gpu_idle() is also
used during normal ops via evict_everything, so I'd rather keep it slim.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Re-enable rc6 w/fix

[Daniel Vetter]

On Tue, Mar 15, 2011 at 07:59:48AM +0000, Chris Wilson wrote:
> I think I should update the comments to reflect what the spec says about
> LOAD_REGISTER_IMM (even though I trust Daniel to have accurately
> determined their impact on gen2/3)... The spec implies that the variable
> length nature of the command is to handle 32/64 bit writes as opposed to
> batch multiple writes into a single (ideally atomic) command.

I've derived my knowledge about load_reg_imm from staring at the hw
context layout in the public docs (where this command seems to be used). I
don't recall how extensively I've tested it (iirc only had a i855gm back
then) put it got the job done. On re-reading that stuff I've noticed that
2 MI_NOOP follow immediately, maybe that's part of the secret sauce?

The load_reg_imm docs (at least the public variants) seem to completely
drop the variable length thingy. At least the table looks fixed-length and
the length = Total Length - 2 blurb is standard for all multi-word
commands.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: Re-enable rc6 w/fix

[Florian Mickler]

On Tue, 15 Mar 2011 07:59:48 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> > Resolve https://bugzilla.kernel.org/show_bug.cgi?id=28582
> 
> I understood the convention (to aide those running external scripts) was
> either Bugzilla: or References:

Thx for considering those poor souls that try to follow up on all
those loose ends.... But here it actually doesn't matter all that much,
the 'https://bugzilla.kernel.org/show_bug.cgi?id=' - prefix to the
bug number is enough of a give-away to parse the changelog for. 

Still, I expect there are people who like consistency and so, from my
experience of changelog digging you are indeed correct, that
References: or Bugzilla: seems to be prevalent.

Regards,
Flo