[Intel-gfx] [PATCH 30/36] drm/i915: Refactor frequency bounds computation

[Sagar Arun Kamble]

On 3/14/2018 3:07 PM, Chris Wilson wrote:
> When choosing the initial frequency in intel_gt_pm_busy() we also need
> to calculate the current min/max bounds. As this calculation is going to
> become more complex with the intersection of several different limits,
> refactor it to a common function. The alternative wold be to feed the
typo
> initial reclocking through the RPS worker, but the latency in this case
> is undesirable.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/intel_gt_pm.c | 58 +++++++++++++++-----------------------
>   1 file changed, 22 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_gt_pm.c b/drivers/gpu/drm/i915/intel_gt_pm.c
> index 8630c30a7e48..f8e029b4a8a7 100644
> --- a/drivers/gpu/drm/i915/intel_gt_pm.c
> +++ b/drivers/gpu/drm/i915/intel_gt_pm.c
> @@ -382,15 +382,25 @@ static int __intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
>   		return 0;
>   }
>   
> -static int intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> +static int adjust_rps(struct drm_i915_private *dev_priv, int freq, int adj)
>   {
>   	struct intel_rps *rps = &dev_priv->gt_pm.rps;
> +	int min, max, val;
Can we move to u8 type in this patch itself
>   	int err;
>   
>   	lockdep_assert_held(&rps->lock);
>   	GEM_BUG_ON(!rps->active);
> -	GEM_BUG_ON(val > rps->max_freq);
> -	GEM_BUG_ON(val < rps->min_freq);
> +
> +	min = rps->min_freq_softlimit;
> +	max = rps->max_freq_softlimit;
> +	if (atomic_read(&rps->num_waiters) && max < rps->boost_freq)
> +		max = rps->boost_freq;
> +
> +	GEM_BUG_ON(min < rps->min_freq);
> +	GEM_BUG_ON(max > rps->max_freq);
> +	GEM_BUG_ON(max < min);
> +
> +	val = clamp(freq + adj, min, max);
>   
>   	err = __intel_set_rps(dev_priv, val);
>   	if (err)
> @@ -401,6 +411,8 @@ static int intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
>   		rps->cur_freq = val;
>   	}
>   
> +	rps->last_adj = val == freq ? adj : 0;
> +
I think this should be:
rps->last_adj = val == freq ? 0 : adj;
and this update can be done in previous if/(else) condition.
>   	return 0;
>   }
>   
> @@ -562,8 +574,8 @@ static void intel_rps_work(struct work_struct *work)
>   	struct drm_i915_private *i915 =
>   		container_of(work, struct drm_i915_private, gt_pm.rps.work);
>   	struct intel_rps *rps = &i915->gt_pm.rps;
> -	int freq, adj, min, max;
>   	bool client_boost;
> +	int freq, adj;
>   	u32 pm_iir;
>   
>   	pm_iir = xchg(&rps->pm_iir, 0) & ~rps->pm_events;
> @@ -576,15 +588,6 @@ static void intel_rps_work(struct work_struct *work)
>   	if (!rps->active)
>   		goto unlock;
>   
> -	min = rps->min_freq_softlimit;
> -	max = rps->max_freq_softlimit;
> -	if (client_boost && max < rps->boost_freq)
> -		max = rps->boost_freq;
> -
> -	GEM_BUG_ON(min < rps->min_freq);
> -	GEM_BUG_ON(max > rps->max_freq);
> -	GEM_BUG_ON(max < min);
> -
>   	adj = rps->last_adj;
>   	freq = rps->cur_freq;
>   	if (client_boost && freq < rps->boost_freq) {
> @@ -595,16 +598,13 @@ static void intel_rps_work(struct work_struct *work)
>   			adj *= 2;
>   		else /* CHV needs even encode values */
>   			adj = IS_CHERRYVIEW(i915) ? 2 : 1;
> -
> -		if (freq >= max)
> -			adj = 0;
>   	} else if (client_boost) {
>   		adj = 0;
>   	} else if (pm_iir & GEN6_PM_RP_DOWN_TIMEOUT) {
> -		if (freq > max_t(int, rps->efficient_freq, min))
> -			freq = max_t(int, rps->efficient_freq, min);
> -		else if (freq > min_t(int, rps->efficient_freq, min))
> -			freq = min_t(int, rps->efficient_freq, min);
> +		if (freq > rps->efficient_freq)
> +			freq = rps->efficient_freq;
> +		else if (freq > rps->idle_freq)
> +			freq = rps->idle_freq;
>   
>   		 adj = 0;
>   	} else if (pm_iir & GEN6_PM_RP_DOWN_THRESHOLD) {
> @@ -612,23 +612,17 @@ static void intel_rps_work(struct work_struct *work)
>   			adj *= 2;
>   		else /* CHV needs even encode values */
>   			adj = IS_CHERRYVIEW(i915) ? -2 : -1;
> -
> -		if (freq <= min)
> -			adj = 0;
>   	} else { /* unknown/external event */
>   		adj = 0;
>   	}
>   
> -	if (intel_set_rps(i915, clamp_t(int, freq + adj, min, max))) {
> +	if (adjust_rps(i915, freq, adj))
>   		DRM_DEBUG_DRIVER("Failed to set new GPU frequency\n");
> -		adj = 0;
> -	}
>   
>   	if (pm_iir) {
>   		spin_lock_irq(&i915->irq_lock);
>   		gen6_unmask_pm_irq(i915, rps->pm_events);
>   		spin_unlock_irq(&i915->irq_lock);
> -		rps->last_adj = adj;
>   	}
>   
>   unlock:
> @@ -652,7 +646,6 @@ void intel_gt_pm_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>   void intel_gt_pm_busy(struct drm_i915_private *dev_priv)
>   {
>   	struct intel_rps *rps = &dev_priv->gt_pm.rps;
> -	u8 freq;
>   
>   	if (!HAS_RPS(dev_priv))
>   		return;
> @@ -667,14 +660,7 @@ void intel_gt_pm_busy(struct drm_i915_private *dev_priv)
>   	 * Use the user's desired frequency as a guide, but for better
>   	 * performance, jump directly to RPe as our starting frequency.
>   	 */
> -	freq = max(rps->cur_freq, rps->efficient_freq);
> -	if (intel_set_rps(dev_priv,
> -			  clamp(freq,
> -				rps->min_freq_softlimit,
> -				rps->max_freq_softlimit)))
> -		DRM_DEBUG_DRIVER("Failed to set busy frequency\n");
> -
> -	rps->last_adj = 0;
> +	adjust_rps(dev_priv, max(rps->cur_freq, rps->efficient_freq), 0);
>   
>   	if (INTEL_GEN(dev_priv) >= 6) {
>   		memset(&rps->ei, 0, sizeof(rps->ei));

-- 
Thanks,
Sagar