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

[Chris Wilson <chris@chris-wilson.co.uk>]

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