Re: [PATCH 10/10] drm/i915/chv: Freq(opcode) request value for CHV.

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, Apr 21, 2014 at 01:34:14PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> On CHV, All the freq request should be even. S0, we need to make sure we
> request the opcode accordingly.
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++
>  drivers/gpu/drm/i915/i915_irq.c | 4 ++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ead2714..5435d87 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2641,6 +2641,15 @@ timespec_to_jiffies_timeout(const struct timespec *value)
>  	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>  }
>  
> +/* rps/turbo related */
> +static inline int i915_rps_freq_change(struct drm_device *dev)
> +{
> +	if (IS_CHERRYVIEW(dev))
> +		return 2;
> +
> +	return 1;
> +}
> +
>  /*
>   * If you need to wait X milliseconds between events A and B, but event B
>   * doesn't happen exactly after event A, you record the timestamp (jiffies) of
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index cf29668..11538fe 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1190,7 +1190,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  		if (adj > 0)
>  			adj *= 2;
>  		else
> -			adj = 1;
> +			adj = i915_rps_freq_change(dev_priv->dev);
>  		new_delay = dev_priv->rps.cur_freq + adj;
>  
>  		/*
> @@ -1209,7 +1209,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  		if (adj < 0)
>  			adj *= 2;
>  		else
> -			adj = -1;
> +			adj = -1 * i915_rps_freq_change(dev_priv->dev);
>  		new_delay = dev_priv->rps.cur_freq + adj;
>  	} else { /* unknown event */
>  		new_delay = dev_priv->rps.cur_freq;

splitting hairs a bit, but adding a new function that isn't named well
doesn't really improve readability. Since we only ever do it for one
case, I think this logic is better stuffed in Cherryview specific
_set_rps() function.

I don't see anything incorrect though. I'd prefer my recommendation, but
it's 
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center