Re: [PATCH 2/7] drm/i915: Fix computation of last_adjustment for RPS autotuning

[Deepak S <deepak.s@linux.intel.com>]

On Friday 06 March 2015 08:36 PM, Chris Wilson wrote:
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_irq.c | 27 ++++++++++++---------------
>   1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9baecb79de8c..1296ce37e435 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1150,21 +1150,20 @@ static void gen6_pm_rps_work(struct work_struct *work)
>   	mutex_lock(&dev_priv->rps.hw_lock);
>   
>   	adj = dev_priv->rps.last_adj;
> +	new_delay = dev_priv->rps.cur_freq;
>   	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
>   		if (adj > 0)
>   			adj *= 2;
> -		else {
> -			/* CHV needs even encode values */
> -			adj = IS_CHERRYVIEW(dev_priv->dev) ? 2 : 1;
> -		}
> -		new_delay = dev_priv->rps.cur_freq + adj;
> -
> +		else /* CHV needs even encode values */
> +			adj = IS_CHERRYVIEW(dev_priv) ? 2 : 1;
>   		/*
>   		 * For better performance, jump directly
>   		 * to RPe if we're below it.
>   		 */
> -		if (new_delay < dev_priv->rps.efficient_freq)
> +		if (new_delay < dev_priv->rps.efficient_freq - adj) {
>   			new_delay = dev_priv->rps.efficient_freq;
> +			adj = 0;
> +		}
>   	} else if (pm_iir & GEN6_PM_RP_DOWN_TIMEOUT) {
>   		if (dev_priv->rps.cur_freq > dev_priv->rps.efficient_freq)
>   			new_delay = dev_priv->rps.efficient_freq;
> @@ -1176,24 +1175,22 @@ static void gen6_pm_rps_work(struct work_struct *work)

I think we should modify adj in GEN6_PM_RP_UP_EI_EXPIRED?
if not not we might request higher freq since we add adj to new_delay before request freq.
                                                                                                                                                                       
Other than this. Patch looks fine
Reviewed-by: Deepak S <deepak.s@linux.intel.com>

>   	} else if (pm_iir & GEN6_PM_RP_DOWN_THRESHOLD) {
>   		if (adj < 0)
>   			adj *= 2;
> -		else {
> -			/* CHV needs even encode values */
> -			adj = IS_CHERRYVIEW(dev_priv->dev) ? -2 : -1;
> -		}
> -		new_delay = dev_priv->rps.cur_freq + adj;
> +		else /* CHV needs even encode values */
> +			adj = IS_CHERRYVIEW(dev_priv) ? -2 : -1;
>   	} else { /* unknown event */
> -		new_delay = dev_priv->rps.cur_freq;
> +		adj = 0;
>   	}
>   
> +	dev_priv->rps.last_adj = adj;
> +
>   	/* sysfs frequency interfaces may have snuck in while servicing the
>   	 * interrupt
>   	 */
> +	new_delay += adj;
>   	new_delay = clamp_t(int, new_delay,
>   			    dev_priv->rps.min_freq_softlimit,
>   			    dev_priv->rps.max_freq_softlimit);
>   
> -	dev_priv->rps.last_adj = new_delay - dev_priv->rps.cur_freq;
> -
>   	intel_set_rps(dev_priv->dev, new_delay);
>   
>   	mutex_unlock(&dev_priv->rps.hw_lock);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx