Re: [PATCH 8/8] drm/i915: enable rc6 on ilk again

[Ben Widawsky <ben@bwidawsk.net>]

On Wed,  8 Aug 2012 23:35:40 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> I have the faint hope that the total absence of any locking for the
> rps code wasn't too good an idea and could very well have caused some
> rc6 related regressions.
> 
> Unfortunately we've never managed to reproduce these issues on any of
> our own machines, so the only way to go about this is to enable it and
> see what happens.
> 
> While at it, kill some stale comments and improve the logging.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Acked-by: Ben Widawsky <ben@bwidawsk.net>

After discussing with Daniel on IRC a bit, I've decided it would take
too much time to actually verify the theory. I am acking on the basis
that I also couldn't find the bug report via google, rc6 has worked for
me on ilk forever, and the savings are too big not to try again.

> ---
>  drivers/gpu/drm/i915/intel_pm.c |   25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index eff0753..a87c625 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2361,31 +2361,26 @@ static void gen6_disable_rps(struct drm_device *dev)
>  
>  int intel_enable_rc6(const struct drm_device *dev)
>  {
> -	/*
> -	 * Respect the kernel parameter if it is set
> -	 */
> +	/* Respect the kernel parameter if it is set */
>  	if (i915_enable_rc6 >= 0)
>  		return i915_enable_rc6;
>  
> -	/*
> -	 * Disable RC6 on Ironlake
> -	 */
> -	if (INTEL_INFO(dev)->gen == 5)
> -		return 0;
> +	if (INTEL_INFO(dev)->gen == 5) {
> +		DRM_DEBUG_DRIVER("Ironlake: only RC6 available\n");
> +		return INTEL_RC6_ENABLE;
> +	}
>  
> -	/* On Haswell, only RC6 is available. So let's enable it by default to
> -	 * provide better testing and coverage since the beginning.
> -	 */
> -	if (IS_HASWELL(dev))
> +	if (IS_HASWELL(dev)) {
> +		DRM_DEBUG_DRIVER("Haswell: only RC6 available\n");
>  		return INTEL_RC6_ENABLE;
> +	}
>  
> -	/*
> -	 * Disable rc6 on Sandybridge
> -	 */
> +	/* snb/ivb have more than one rc6 state. */
>  	if (INTEL_INFO(dev)->gen == 6) {
>  		DRM_DEBUG_DRIVER("Sandybridge: deep RC6 disabled\n");
>  		return INTEL_RC6_ENABLE;
>  	}
> +
>  	DRM_DEBUG_DRIVER("RC6 and deep RC6 enabled\n");
>  	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
>  }



-- 
Ben Widawsky, Intel Open Source Technology Center