Re: [PATCH 33/36] drm/i915: Pull IPS into RPS

[Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>]

On 3/14/2018 3:07 PM, Chris Wilson wrote:
> IPS was the precursor to RPS on Ironlake. It serves the same function,
> and so should be pulled under the intel_gt_pm umbrella.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Looks good except subject should be "Pull IPS into GT PM". It seems IPS 
and RPS merge is happening in next patch.
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_drv.h    | 37 ++++++++---------
>   drivers/gpu/drm/i915/i915_irq.c    | 21 +++++-----
>   drivers/gpu/drm/i915/intel_gt_pm.c | 83 +++++++++++++++++++++-----------------
>   drivers/gpu/drm/i915/intel_pm.c    |  8 ++--
>   4 files changed, 80 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cd92d0295b63..cfbcaa8556e0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -784,23 +784,10 @@ struct intel_rps {
>   	struct intel_rps_ei ei;
>   };
>   
> -struct intel_rc6 {
> -	u64 prev_hw_residency[4];
> -	u64 cur_residency[4];
> -};
> -
> -struct intel_gt_pm {
> -	struct intel_rc6 rc6;
> -	struct intel_rps rps;
> -
> -	u32 imr;
> -	u32 ier;
> -};
> -
>   /* defined intel_pm.c */
>   extern spinlock_t mchdev_lock;
>   
> -struct intel_ilk_power_mgmt {
> +struct intel_ips {
>   	u8 cur_delay;
>   	u8 min_delay;
>   	u8 max_delay;
> @@ -819,6 +806,24 @@ struct intel_ilk_power_mgmt {
>   	int r_t;
>   };
>   
> +struct intel_rc6 {
> +	u64 prev_hw_residency[4];
> +	u64 cur_residency[4];
> +};
> +
> +struct intel_gt_pm {
> +	struct intel_rc6 rc6;
> +	struct intel_rps rps;
> +	/*
> +	 * ilk-only ips/rps state. Everything in here is protected by the
> +	 * global mchdev_lock in intel_gt_pm.c
> +	 */
> +	struct intel_ips ips;
> +
> +	u32 imr;
> +	u32 ier;
> +};
> +
>   struct drm_i915_private;
>   struct i915_power_well;
>   
> @@ -1780,10 +1785,6 @@ struct drm_i915_private {
>   
>   	struct intel_gt_pm gt_pm;
>   
> -	/* ilk-only ips/rps state. Everything in here is protected by the global
> -	 * mchdev_lock in intel_pm.c */
> -	struct intel_ilk_power_mgmt ips;
> -
>   	struct i915_power_domains power_domains;
>   
>   	struct i915_psr psr;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index dfb711ca4d27..9a52692395f2 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -852,6 +852,7 @@ int intel_get_crtc_scanline(struct intel_crtc *crtc)
>   
>   static void ironlake_rps_change_irq_handler(struct drm_i915_private *dev_priv)
>   {
> +	struct intel_ips *ips = &dev_priv->gt_pm.ips;
>   	u32 busy_up, busy_down, max_avg, min_avg;
>   	u8 new_delay;
>   
> @@ -859,7 +860,7 @@ static void ironlake_rps_change_irq_handler(struct drm_i915_private *dev_priv)
>   
>   	I915_WRITE16(MEMINTRSTS, I915_READ(MEMINTRSTS));
>   
> -	new_delay = dev_priv->ips.cur_delay;
> +	new_delay = ips->cur_delay;
>   
>   	I915_WRITE16(MEMINTRSTS, MEMINT_EVAL_CHG);
>   	busy_up = I915_READ(RCPREVBSYTUPAVG);
> @@ -869,19 +870,19 @@ static void ironlake_rps_change_irq_handler(struct drm_i915_private *dev_priv)
>   
>   	/* Handle RCS change request from hw */
>   	if (busy_up > max_avg) {
> -		if (dev_priv->ips.cur_delay != dev_priv->ips.max_delay)
> -			new_delay = dev_priv->ips.cur_delay - 1;
> -		if (new_delay < dev_priv->ips.max_delay)
> -			new_delay = dev_priv->ips.max_delay;
> +		if (ips->cur_delay != ips->max_delay)
> +			new_delay = ips->cur_delay - 1;
> +		if (new_delay < ips->max_delay)
> +			new_delay = ips->max_delay;
>   	} else if (busy_down < min_avg) {
> -		if (dev_priv->ips.cur_delay != dev_priv->ips.min_delay)
> -			new_delay = dev_priv->ips.cur_delay + 1;
> -		if (new_delay > dev_priv->ips.min_delay)
> -			new_delay = dev_priv->ips.min_delay;
> +		if (ips->cur_delay != ips->min_delay)
> +			new_delay = ips->cur_delay + 1;
> +		if (new_delay > ips->min_delay)
> +			new_delay = ips->min_delay;
>   	}
>   
>   	if (ironlake_set_drps(dev_priv, new_delay))
> -		dev_priv->ips.cur_delay = new_delay;
> +		ips->cur_delay = new_delay;
>   
>   	spin_unlock(&mchdev_lock);
>   
> diff --git a/drivers/gpu/drm/i915/intel_gt_pm.c b/drivers/gpu/drm/i915/intel_gt_pm.c
> index 18ab1b3a2945..def292cfd181 100644
> --- a/drivers/gpu/drm/i915/intel_gt_pm.c
> +++ b/drivers/gpu/drm/i915/intel_gt_pm.c
> @@ -65,6 +65,7 @@ bool ironlake_set_drps(struct drm_i915_private *dev_priv, u8 val)
>   
>   static void ironlake_enable_drps(struct drm_i915_private *dev_priv)
>   {
> +	struct intel_ips *ips = &dev_priv->gt_pm.ips;
>   	u32 rgvmodectl;
>   	u8 fmax, fmin, fstart, vstart;
>   
> @@ -95,12 +96,12 @@ static void ironlake_enable_drps(struct drm_i915_private *dev_priv)
>   	vstart = (I915_READ(PXVFREQ(fstart)) & PXVFREQ_PX_MASK) >>
>   		PXVFREQ_PX_SHIFT;
>   
> -	dev_priv->ips.fmax = fmax; /* IPS callback will increase this */
> -	dev_priv->ips.fstart = fstart;
> +	ips->fmax = fmax; /* IPS callback will increase this */
> +	ips->fstart = fstart;
>   
> -	dev_priv->ips.max_delay = fstart;
> -	dev_priv->ips.min_delay = fmin;
> -	dev_priv->ips.cur_delay = fstart;
> +	ips->max_delay = fstart;
> +	ips->min_delay = fmin;
> +	ips->cur_delay = fstart;
>   
>   	DRM_DEBUG_DRIVER("fmax: %d, fmin: %d, fstart: %d\n",
>   			 fmax, fmin, fstart);
> @@ -123,11 +124,11 @@ static void ironlake_enable_drps(struct drm_i915_private *dev_priv)
>   
>   	ironlake_set_drps(dev_priv, fstart);
>   
> -	dev_priv->ips.last_count1 = I915_READ(DMIEC) +
> -		I915_READ(DDREC) + I915_READ(CSIEC);
> -	dev_priv->ips.last_time1 = jiffies_to_msecs(jiffies);
> -	dev_priv->ips.last_count2 = I915_READ(GFXEC);
> -	dev_priv->ips.last_time2 = ktime_get_raw_ns();
> +	ips->last_count1 =
> +		I915_READ(DMIEC) + I915_READ(DDREC) + I915_READ(CSIEC);
> +	ips->last_time1 = jiffies_to_msecs(jiffies);
> +	ips->last_count2 = I915_READ(GFXEC);
> +	ips->last_time2 = ktime_get_raw_ns();
>   
>   	spin_unlock_irq(&mchdev_lock);
>   }
> @@ -148,7 +149,7 @@ static void ironlake_disable_drps(struct drm_i915_private *dev_priv)
>   	I915_WRITE(DEIMR, I915_READ(DEIMR) | DE_PCU_EVENT);
>   
>   	/* Go back to the starting frequency */
> -	ironlake_set_drps(dev_priv, dev_priv->ips.fstart);
> +	ironlake_set_drps(dev_priv, dev_priv->gt_pm.ips.fstart);
>   	mdelay(1);
>   	rgvswctl |= MEMCTL_CMD_STS;
>   	I915_WRITE(MEMSWCTL, rgvswctl);
> @@ -1857,6 +1858,7 @@ static const struct cparams {
>   
>   static unsigned long __i915_chipset_val(struct drm_i915_private *dev_priv)
>   {
> +	struct intel_ips *ips = &dev_priv->gt_pm.ips;
>   	u64 total_count, diff, ret;
>   	u32 count1, count2, count3, m = 0, c = 0;
>   	unsigned long now = jiffies_to_msecs(jiffies), diff1;
> @@ -1864,7 +1866,7 @@ static unsigned long __i915_chipset_val(struct drm_i915_private *dev_priv)
>   
>   	lockdep_assert_held(&mchdev_lock);
>   
> -	diff1 = now - dev_priv->ips.last_time1;
> +	diff1 = now - ips->last_time1;
>   
>   	/*
>   	 * Prevent division-by-zero if we are asking too fast.
> @@ -1873,7 +1875,7 @@ static unsigned long __i915_chipset_val(struct drm_i915_private *dev_priv)
>   	 * in such cases.
>   	 */
>   	if (diff1 <= 10)
> -		return dev_priv->ips.chipset_power;
> +		return ips->chipset_power;
>   
>   	count1 = I915_READ(DMIEC);
>   	count2 = I915_READ(DDREC);
> @@ -1882,16 +1884,15 @@ static unsigned long __i915_chipset_val(struct drm_i915_private *dev_priv)
>   	total_count = count1 + count2 + count3;
>   
>   	/* FIXME: handle per-counter overflow */
> -	if (total_count < dev_priv->ips.last_count1) {
> -		diff = ~0UL - dev_priv->ips.last_count1;
> +	if (total_count < ips->last_count1) {
> +		diff = ~0UL - ips->last_count1;
>   		diff += total_count;
>   	} else {
> -		diff = total_count - dev_priv->ips.last_count1;
> +		diff = total_count - ips->last_count1;
>   	}
>   
>   	for (i = 0; i < ARRAY_SIZE(cparams); i++) {
> -		if (cparams[i].i == dev_priv->ips.c_m &&
> -		    cparams[i].t == dev_priv->ips.r_t) {
> +		if (cparams[i].i == ips->c_m && cparams[i].t == ips->r_t) {
>   			m = cparams[i].m;
>   			c = cparams[i].c;
>   			break;
> @@ -1902,10 +1903,10 @@ static unsigned long __i915_chipset_val(struct drm_i915_private *dev_priv)
>   	ret = ((m * diff) + c);
>   	ret = div_u64(ret, 10);
>   
> -	dev_priv->ips.last_count1 = total_count;
> -	dev_priv->ips.last_time1 = now;
> +	ips->last_count1 = total_count;
> +	ips->last_time1 = now;
>   
> -	dev_priv->ips.chipset_power = ret;
> +	ips->chipset_power = ret;
>   
>   	return ret;
>   }
> @@ -1967,13 +1968,14 @@ static u32 pvid_to_extvid(struct drm_i915_private *dev_priv, u8 pxvid)
>   
>   static void __i915_update_gfx_val(struct drm_i915_private *dev_priv)
>   {
> +	struct intel_ips *ips = &dev_priv->gt_pm.ips;
>   	u64 now, diff, diffms;
>   	u32 count;
>   
>   	lockdep_assert_held(&mchdev_lock);
>   
>   	now = ktime_get_raw_ns();
> -	diffms = now - dev_priv->ips.last_time2;
> +	diffms = now - ips->last_time2;
>   	do_div(diffms, NSEC_PER_MSEC);
>   
>   	/* Don't divide by 0 */
> @@ -1982,20 +1984,20 @@ static void __i915_update_gfx_val(struct drm_i915_private *dev_priv)
>   
>   	count = I915_READ(GFXEC);
>   
> -	if (count < dev_priv->ips.last_count2) {
> -		diff = ~0UL - dev_priv->ips.last_count2;
> +	if (count < ips->last_count2) {
> +		diff = ~0UL - ips->last_count2;
>   		diff += count;
>   	} else {
> -		diff = count - dev_priv->ips.last_count2;
> +		diff = count - ips->last_count2;
>   	}
>   
> -	dev_priv->ips.last_count2 = count;
> -	dev_priv->ips.last_time2 = now;
> +	ips->last_count2 = count;
> +	ips->last_time2 = now;
>   
>   	/* More magic constants... */
>   	diff = diff * 1181;
>   	diff = div_u64(diff, diffms * 10);
> -	dev_priv->ips.gfx_power = diff;
> +	ips->gfx_power = diff;
>   }
>   
>   void i915_update_gfx_val(struct drm_i915_private *dev_priv)
> @@ -2014,6 +2016,7 @@ void i915_update_gfx_val(struct drm_i915_private *dev_priv)
>   
>   static unsigned long __i915_gfx_val(struct drm_i915_private *dev_priv)
>   {
> +	struct intel_ips *ips = &dev_priv->gt_pm.ips;
>   	unsigned long t, corr, state1, corr2, state2;
>   	u32 pxvid, ext_v;
>   
> @@ -2039,14 +2042,14 @@ static unsigned long __i915_gfx_val(struct drm_i915_private *dev_priv)
>   
>   	corr = corr * ((150142 * state1) / 10000 - 78642);
>   	corr /= 100000;
> -	corr2 = (corr * dev_priv->ips.corr);
> +	corr2 = (corr * ips->corr);
>   
>   	state2 = (corr2 * state1) / 10000;
>   	state2 /= 100; /* convert to mW */
>   
>   	__i915_update_gfx_val(dev_priv);
>   
> -	return dev_priv->ips.gfx_power + state2;
> +	return ips->gfx_power + state2;
>   }
>   
>   unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
> @@ -2117,14 +2120,17 @@ EXPORT_SYMBOL_GPL(i915_read_mch_val);
>   bool i915_gpu_raise(void)
>   {
>   	struct drm_i915_private *i915;
> +	struct intel_ips *ips;
>   
>   	i915 = mchdev_get();
>   	if (!i915)
>   		return false;
>   
> +	ips = &i915->gt_pm.ips;
> +
>   	spin_lock_irq(&mchdev_lock);
> -	if (i915->ips.max_delay > i915->ips.fmax)
> -		i915->ips.max_delay--;
> +	if (ips->max_delay > ips->fmax)
> +		ips->max_delay--;
>   	spin_unlock_irq(&mchdev_lock);
>   
>   	drm_dev_put(&i915->drm);
> @@ -2141,14 +2147,17 @@ EXPORT_SYMBOL_GPL(i915_gpu_raise);
>   bool i915_gpu_lower(void)
>   {
>   	struct drm_i915_private *i915;
> +	struct intel_ips *ips;
>   
>   	i915 = mchdev_get();
>   	if (!i915)
>   		return false;
>   
> +	ips = &i915->gt_pm.ips;
> +
>   	spin_lock_irq(&mchdev_lock);
> -	if (i915->ips.max_delay < i915->ips.min_delay)
> -		i915->ips.max_delay++;
> +	if (ips->max_delay < ips->min_delay)
> +		ips->max_delay++;
>   	spin_unlock_irq(&mchdev_lock);
>   
>   	drm_dev_put(&i915->drm);
> @@ -2193,8 +2202,8 @@ bool i915_gpu_turbo_disable(void)
>   		return false;
>   
>   	spin_lock_irq(&mchdev_lock);
> -	i915->ips.max_delay = i915->ips.fstart;
> -	ret = ironlake_set_drps(i915, i915->ips.fstart);
> +	i915->gt_pm.ips.max_delay = i915->gt_pm.ips.fstart;
> +	ret = ironlake_set_drps(i915, i915->gt_pm.ips.fstart);
>   	spin_unlock_irq(&mchdev_lock);
>   
>   	drm_dev_put(&i915->drm);
> @@ -2305,7 +2314,7 @@ static void intel_init_emon(struct drm_i915_private *dev_priv)
>   
>   	lcfuse = I915_READ(LCFUSE02);
>   
> -	dev_priv->ips.corr = (lcfuse & LCFUSE_HIV_MASK);
> +	dev_priv->gt_pm.ips.corr = (lcfuse & LCFUSE_HIV_MASK);
>   }
>   
>   void intel_gt_pm_sanitize(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0bbee12bee41..1ad86ee668d8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -186,7 +186,7 @@ static void i915_ironlake_get_mem_freq(struct drm_i915_private *dev_priv)
>   		break;
>   	}
>   
> -	dev_priv->ips.r_t = dev_priv->mem_freq;
> +	dev_priv->gt_pm.ips.r_t = dev_priv->mem_freq;
>   
>   	switch (csipll & 0x3ff) {
>   	case 0x00c:
> @@ -218,11 +218,11 @@ static void i915_ironlake_get_mem_freq(struct drm_i915_private *dev_priv)
>   	}
>   
>   	if (dev_priv->fsb_freq == 3200) {
> -		dev_priv->ips.c_m = 0;
> +		dev_priv->gt_pm.ips.c_m = 0;
>   	} else if (dev_priv->fsb_freq > 3200 && dev_priv->fsb_freq <= 4800) {
> -		dev_priv->ips.c_m = 1;
> +		dev_priv->gt_pm.ips.c_m = 1;
>   	} else {
> -		dev_priv->ips.c_m = 2;
> +		dev_priv->gt_pm.ips.c_m = 2;
>   	}
>   }
>   

--
Thanks,
Sagar

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx