Re: [PATCH v2] drm/i915: Report the failure to write to the punit

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:

> The write to the punit may fail, so propagate the error code back to its
> callers. Of particular interest are the RPS writes, so add appropriate
> user error codes and logging.
>
> v2: Add DEBUG for failed frequency changes during RPS.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>

Reviewed-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>

> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c   |  6 +++--
>  drivers/gpu/drm/i915/i915_drv.h       |  4 ++--
>  drivers/gpu/drm/i915/i915_irq.c       |  5 +++-
>  drivers/gpu/drm/i915/i915_sysfs.c     |  9 ++++---
>  drivers/gpu/drm/i915/intel_pm.c       | 45 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_sideband.c | 10 +++++---
>  6 files changed, 53 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index c041a2ae9af0..aebd456edec1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4272,7 +4272,8 @@ i915_max_freq_set(void *data, u64 val)
>  
>  	dev_priv->rps.max_freq_softlimit = val;
>  
> -	intel_set_rps(dev_priv, val);
> +	if (intel_set_rps(dev_priv, val))
> +		DRM_DEBUG_DRIVER("failed to update RPS to new softlimit\n");
>  
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
> @@ -4327,7 +4328,8 @@ i915_min_freq_set(void *data, u64 val)
>  
>  	dev_priv->rps.min_freq_softlimit = val;
>  
> -	intel_set_rps(dev_priv, val);
> +	if (intel_set_rps(dev_priv, val))
> +		DRM_DEBUG_DRIVER("failed to update RPS to new softlimit\n");
>  
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ef5af3c7a14a..1c5145d0e53d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3766,7 +3766,7 @@ extern void i915_redisable_vga(struct drm_i915_private *dev_priv);
>  extern void i915_redisable_vga_power_on(struct drm_i915_private *dev_priv);
>  extern bool ironlake_set_drps(struct drm_i915_private *dev_priv, u8 val);
>  extern void intel_init_pch_refclk(struct drm_i915_private *dev_priv);
> -extern void intel_set_rps(struct drm_i915_private *dev_priv, u8 val);
> +extern int intel_set_rps(struct drm_i915_private *dev_priv, u8 val);
>  extern bool intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
>  				  bool enable);
>  
> @@ -3792,7 +3792,7 @@ int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
>  
>  /* intel_sideband.c */
>  u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
> -void vlv_punit_write(struct drm_i915_private *dev_priv, u32 addr, u32 val);
> +int vlv_punit_write(struct drm_i915_private *dev_priv, u32 addr, u32 val);
>  u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr);
>  u32 vlv_iosf_sb_read(struct drm_i915_private *dev_priv, u8 port, u32 reg);
>  void vlv_iosf_sb_write(struct drm_i915_private *dev_priv, u8 port, u32 reg, u32 val);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 59865654f552..145bee0aa57d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1234,7 +1234,10 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  	new_delay += adj;
>  	new_delay = clamp_t(int, new_delay, min, max);
>  
> -	intel_set_rps(dev_priv, new_delay);
> +	if (intel_set_rps(dev_priv, new_delay)) {
> +		DRM_DEBUG_DRIVER("Failed to set new GPU frequency\n");
> +		dev_priv->rps.last_adj = 0;
> +	}
>  
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 376ac957cd1c..a721ff116101 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -395,13 +395,13 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev,
>  	/* We still need *_set_rps to process the new max_delay and
>  	 * update the interrupt limits and PMINTRMSK even though
>  	 * frequency request may be unchanged. */
> -	intel_set_rps(dev_priv, val);
> +	ret = intel_set_rps(dev_priv, val);
>  
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
>  	intel_runtime_pm_put(dev_priv);
>  
> -	return count;
> +	return ret ?: count;
>  }
>  
>  static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> @@ -448,14 +448,13 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
>  	/* We still need *_set_rps to process the new min_delay and
>  	 * update the interrupt limits and PMINTRMSK even though
>  	 * frequency request may be unchanged. */
> -	intel_set_rps(dev_priv, val);
> +	ret = intel_set_rps(dev_priv, val);
>  
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
>  	intel_runtime_pm_put(dev_priv);
>  
> -	return count;
> -
> +	return ret ?: count;
>  }
>  
>  static DEVICE_ATTR(gt_act_freq_mhz, S_IRUGO, gt_act_freq_mhz_show, NULL);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 25c2652e1e11..798787c5df9e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4933,7 +4933,7 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
>  /* gen6_set_rps is called to update the frequency request, but should also be
>   * called when the range (min_delay and max_delay) is modified so that we can
>   * update the GEN6_RP_INTERRUPT_LIMITS register accordingly. */
> -static void gen6_set_rps(struct drm_i915_private *dev_priv, u8 val)
> +static int gen6_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  {
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  	WARN_ON(val > dev_priv->rps.max_freq);
> @@ -4968,10 +4968,14 @@ static void gen6_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  
>  	dev_priv->rps.cur_freq = val;
>  	trace_intel_gpu_freq_change(intel_gpu_freq(dev_priv, val));
> +
> +	return 0;
>  }
>  
> -static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
> +static int valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  {
> +	int err;
> +
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  	WARN_ON(val > dev_priv->rps.max_freq);
>  	WARN_ON(val < dev_priv->rps.min_freq);
> @@ -4983,13 +4987,18 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>  
>  	if (val != dev_priv->rps.cur_freq) {
> -		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> +		err = vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> +		if (err)
> +			return err;
> +
>  		if (!IS_CHERRYVIEW(dev_priv))
>  			gen6_set_rps_thresholds(dev_priv, val);
>  	}
>  
>  	dev_priv->rps.cur_freq = val;
>  	trace_intel_gpu_freq_change(intel_gpu_freq(dev_priv, val));
> +
> +	return 0;
>  }
>  
>  /* vlv_set_rps_idle: Set the frequency to idle, if Gfx clocks are down
> @@ -5002,6 +5011,7 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>  {
>  	u32 val = dev_priv->rps.idle_freq;
> +	int err;
>  
>  	if (dev_priv->rps.cur_freq <= val)
>  		return;
> @@ -5019,8 +5029,11 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>  	 * power than the render powerwell.
>  	 */
>  	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> -	valleyview_set_rps(dev_priv, val);
> +	err = valleyview_set_rps(dev_priv, val);
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
> +
> +	if (err)
> +		DRM_ERROR("Failed to set RPS for idle\n");
>  }
>  
>  void gen6_rps_busy(struct drm_i915_private *dev_priv)
> @@ -5035,10 +5048,11 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
>  		gen6_enable_rps_interrupts(dev_priv);
>  
>  		/* Ensure we start at the user's desired frequency */
> -		intel_set_rps(dev_priv,
> -			      clamp(dev_priv->rps.cur_freq,
> -				    dev_priv->rps.min_freq_softlimit,
> -				    dev_priv->rps.max_freq_softlimit));
> +		if (intel_set_rps(dev_priv,
> +				  clamp(dev_priv->rps.cur_freq,
> +					dev_priv->rps.min_freq_softlimit,
> +					dev_priv->rps.max_freq_softlimit)))
> +			DRM_DEBUG_DRIVER("Failed to set idle frequency\n");
>  	}
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
> @@ -5106,12 +5120,16 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
>  	spin_unlock(&dev_priv->rps.client_lock);
>  }
>  
> -void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> +int intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  {
> +	int err;
> +
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		valleyview_set_rps(dev_priv, val);
> +		err = valleyview_set_rps(dev_priv, val);
>  	else
> -		gen6_set_rps(dev_priv, val);
> +		err = gen6_set_rps(dev_priv, val);
> +
> +	return err;
>  }
>  
>  static void gen9_disable_rc6(struct drm_i915_private *dev_priv)
> @@ -5315,7 +5333,7 @@ static void gen6_init_rps_frequencies(struct drm_i915_private *dev_priv)
>  }
>  
>  static void reset_rps(struct drm_i915_private *dev_priv,
> -		      void (*set)(struct drm_i915_private *, u8))
> +		      int (*set)(struct drm_i915_private *, u8))
>  {
>  	u8 freq = dev_priv->rps.cur_freq;
>  
> @@ -5323,7 +5341,8 @@ static void reset_rps(struct drm_i915_private *dev_priv,
>  	dev_priv->rps.power = -1;
>  	dev_priv->rps.cur_freq = -1;
>  
> -	set(dev_priv, freq);
> +	if (set(dev_priv, freq))
> +		DRM_ERROR("Failed to reset RPS to initial values\n");
>  }
>  
>  /* See the Gen9_GT_PM_Programming_Guide doc for the below */
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> index 1a840bf92eea..d3d49a09c919 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.c
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -93,14 +93,18 @@ u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
>  	return val;
>  }
>  
> -void vlv_punit_write(struct drm_i915_private *dev_priv, u32 addr, u32 val)
> +int vlv_punit_write(struct drm_i915_private *dev_priv, u32 addr, u32 val)
>  {
> +	int err;
> +
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  
>  	mutex_lock(&dev_priv->sb_lock);
> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
> -			SB_CRWRDA_NP, addr, &val);
> +	err = vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
> +			      SB_CRWRDA_NP, addr, &val);
>  	mutex_unlock(&dev_priv->sb_lock);
> +
> +	return err;
>  }
>  
>  u32 vlv_bunit_read(struct drm_i915_private *dev_priv, u32 reg)
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux