Chris Wilson <ch...@chris-wilson.co.uk> 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 <ch...@chris-wilson.co.uk>
Reviewed-by: Mika Kuoppala <mika.kuopp...@intel.com>
> Cc: Mika Kuoppala <mika.kuopp...@intel.com>
> ---
> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
