[Intel-gfx] [PATCH 10/36] drm/i915: Replace pcu_lock with sb_lock
Sagar Arun Kamble
sagar.a.kamble at intel.com
Thu Mar 15 12:06:57 UTC 2018
On 3/14/2018 3:07 PM, Chris Wilson wrote:
> We now have two locks for sideband access. The general one covering
> sideband access across all generation, sb_lock, and a specific one
> covering sideband access via the punit on vlv/chv. After lifting the
> sb_lock around the punit into the callers, the pcu_lock is now redudant
> and can be separated from its other use to regulate RPS (essentially
> giving RPS a lock all of its own).
>
> v2: Extract a couple of minor bug fixes.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 47 ++++--------
> drivers/gpu/drm/i915/i915_drv.h | 10 +--
> drivers/gpu/drm/i915/i915_irq.c | 4 +-
> drivers/gpu/drm/i915/i915_sysfs.c | 32 +++-----
> drivers/gpu/drm/i915/intel_cdclk.c | 28 -------
> drivers/gpu/drm/i915/intel_display.c | 6 --
> drivers/gpu/drm/i915/intel_hdcp.c | 2 -
> drivers/gpu/drm/i915/intel_pm.c | 127 +++++++++++++++-----------------
> drivers/gpu/drm/i915/intel_runtime_pm.c | 8 --
> drivers/gpu/drm/i915/intel_sideband.c | 4 -
> 10 files changed, 93 insertions(+), 175 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index ebce80f29087..0db75e8ce494 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1074,8 +1074,6 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> u32 rpmodectl, freq_sts;
>
> - mutex_lock(&dev_priv->pcu_lock);
> -
> rpmodectl = I915_READ(GEN6_RP_CONTROL);
> seq_printf(m, "Video Turbo Mode: %s\n",
> yesno(rpmodectl & GEN6_RP_MEDIA_TURBO));
> @@ -1110,7 +1108,6 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> seq_printf(m,
> "efficient (RPe) frequency: %d MHz\n",
> intel_gpu_freq(dev_priv, rps->efficient_freq));
> - mutex_unlock(&dev_priv->pcu_lock);
> } else if (INTEL_GEN(dev_priv) >= 6) {
> u32 rp_state_limits;
> u32 gt_perf_status;
> @@ -1525,12 +1522,9 @@ static int gen6_drpc_info(struct seq_file *m)
> gen9_powergate_status = I915_READ(GEN9_PWRGT_DOMAIN_STATUS);
> }
>
> - if (INTEL_GEN(dev_priv) <= 7) {
> - mutex_lock(&dev_priv->pcu_lock);
> + if (INTEL_GEN(dev_priv) <= 7)
> sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS,
> &rc6vids);
> - mutex_unlock(&dev_priv->pcu_lock);
> - }
>
> seq_printf(m, "RC1e Enabled: %s\n",
> yesno(rcctl1 & GEN6_RC_CTL_RC1e_ENABLE));
> @@ -1801,17 +1795,10 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
> struct intel_rps *rps = &dev_priv->gt_pm.rps;
> unsigned int max_gpu_freq, min_gpu_freq;
> int gpu_freq, ia_freq;
> - int ret;
>
> if (!HAS_LLC(dev_priv))
> return -ENODEV;
>
> - intel_runtime_pm_get(dev_priv);
> -
> - ret = mutex_lock_interruptible(&dev_priv->pcu_lock);
> - if (ret)
> - goto out;
> -
> min_gpu_freq = rps->min_freq;
> max_gpu_freq = rps->max_freq;
> if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> @@ -1822,6 +1809,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>
> seq_puts(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n");
>
> + intel_runtime_pm_get(dev_priv);
> for (gpu_freq = min_gpu_freq; gpu_freq <= max_gpu_freq; gpu_freq++) {
> ia_freq = gpu_freq;
> sandybridge_pcode_read(dev_priv,
> @@ -1835,12 +1823,9 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
> ((ia_freq >> 0) & 0xff) * 100,
> ((ia_freq >> 8) & 0xff) * 100);
> }
> -
> - mutex_unlock(&dev_priv->pcu_lock);
> -
> -out:
> intel_runtime_pm_put(dev_priv);
> - return ret;
> +
> + return 0;
> }
>
> static int i915_opregion(struct seq_file *m, void *unused)
> @@ -4174,7 +4159,7 @@ i915_max_freq_set(void *data, u64 val)
>
> DRM_DEBUG_DRIVER("Manually setting max freq to %llu\n", val);
>
> - ret = mutex_lock_interruptible(&dev_priv->pcu_lock);
> + ret = mutex_lock_interruptible(&rps->lock);
> if (ret)
> return ret;
>
> @@ -4187,8 +4172,8 @@ i915_max_freq_set(void *data, u64 val)
> hw_min = rps->min_freq;
>
> if (val < hw_min || val > hw_max || val < rps->min_freq_softlimit) {
> - mutex_unlock(&dev_priv->pcu_lock);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto unlock;
> }
>
> rps->max_freq_softlimit = val;
> @@ -4196,9 +4181,9 @@ i915_max_freq_set(void *data, u64 val)
> if (intel_set_rps(dev_priv, val))
> DRM_DEBUG_DRIVER("failed to update RPS to new softlimit\n");
>
> - mutex_unlock(&dev_priv->pcu_lock);
> -
> - return 0;
> +unlock:
> + mutex_unlock(&rps->lock);
> + return ret;
> }
>
> DEFINE_SIMPLE_ATTRIBUTE(i915_max_freq_fops,
> @@ -4230,7 +4215,7 @@ i915_min_freq_set(void *data, u64 val)
>
> DRM_DEBUG_DRIVER("Manually setting min freq to %llu\n", val);
>
> - ret = mutex_lock_interruptible(&dev_priv->pcu_lock);
> + ret = mutex_lock_interruptible(&rps->lock);
> if (ret)
> return ret;
>
> @@ -4244,8 +4229,8 @@ i915_min_freq_set(void *data, u64 val)
>
> if (val < hw_min ||
> val > hw_max || val > rps->max_freq_softlimit) {
> - mutex_unlock(&dev_priv->pcu_lock);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto unlock;
> }
>
> rps->min_freq_softlimit = val;
> @@ -4253,9 +4238,9 @@ i915_min_freq_set(void *data, u64 val)
> if (intel_set_rps(dev_priv, val))
> DRM_DEBUG_DRIVER("failed to update RPS to new softlimit\n");
>
> - mutex_unlock(&dev_priv->pcu_lock);
> -
> - return 0;
> +unlock:
> + mutex_unlock(&rps->lock);
> + return ret;
> }
>
> DEFINE_SIMPLE_ATTRIBUTE(i915_min_freq_fops,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 67cf0fe533f8..1f246d2a4e84 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -735,6 +735,8 @@ struct intel_rps_ei {
> };
>
> struct intel_rps {
> + struct mutex lock;
> +
I think this lock can now become part of struct intel_gt_pm.
> /*
> * work, interrupts_enabled and pm_iir are protected by
> * dev_priv->irq_lock
> @@ -1783,14 +1785,6 @@ struct drm_i915_private {
> /* Cannot be determined by PCIID. You must always read a register. */
> u32 edram_cap;
>
> - /*
> - * Protects RPS/RC6 register access and PCU communication.
> - * Must be taken after struct_mutex if nested. Note that
> - * this lock may be held for long periods of time when
> - * talking to hw - so only take it when talking to hw!
> - */
> - struct mutex pcu_lock;
> -
> /* gen6+ GT PM state */
> struct intel_gen6_power_mgmt gt_pm;
>
...
> -int sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv,
> - u32 mbox, u32 val,
> - int fast_timeout_us, int slow_timeout_ms)
> +static int __sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv,
> + u32 mbox, u32 val,
> + int fast_timeout_us,
> + int slow_timeout_ms)
> {
> int status;
>
> - WARN_ON(!mutex_is_locked(&dev_priv->pcu_lock));
> -
lockdep_assert is missed here.
With this change, patch looks good to me.
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> /* GEN6_PCODE_* are outside of the forcewake domain, we can
> * use te fw I915_READ variants to reduce the amount of work
> * required when reading/writing.
> */
>
> - if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) {
> - DRM_DEBUG_DRIVER("warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps\n",
> - val, mbox, __builtin_return_address(0));
> + if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY)
> return -EAGAIN;
> - }
>
> I915_WRITE_FW(GEN6_PCODE_DATA, val);
> I915_WRITE_FW(GEN6_PCODE_DATA1, 0);
> @@ -9290,11 +9273,8 @@ int sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv,
> if (__intel_wait_for_register_fw(dev_priv,
> GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> fast_timeout_us, slow_timeout_ms,
> - NULL)) {
> - DRM_ERROR("timeout waiting for pcode write of 0x%08x to mbox %x to finish for %ps\n",
> - val, mbox, __builtin_return_address(0));
> + NULL))
> return -ETIMEDOUT;
> - }
>
> I915_WRITE_FW(GEN6_PCODE_DATA, 0);
>
> @@ -9303,13 +9283,28 @@ int sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv,
> else
> status = gen6_check_mailbox_status(dev_priv);
>
> + return status;
> +}
> +
> +int sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv,
> + u32 mbox, u32 val,
> + int fast_timeout_us,
> + int slow_timeout_ms)
> +{
> + int status;
> +
> + mutex_lock(&dev_priv->sb_lock);
> + status = __sandybridge_pcode_write_timeout(dev_priv, mbox, val,
> + fast_timeout_us,
> + slow_timeout_ms);
> + mutex_unlock(&dev_priv->sb_lock);
> +
> if (status) {
> DRM_DEBUG_DRIVER("warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps: %d\n",
> val, mbox, __builtin_return_address(0), status);
> - return status;
> }
>
> - return 0;
> + return status;
> }
>
> static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox,
> @@ -9318,7 +9313,7 @@ static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox,
> {
> u32 val = request;
>
> - *status = sandybridge_pcode_read(dev_priv, mbox, &val);
> + *status = __sandybridge_pcode_read(dev_priv, mbox, &val);
>
> return *status || ((val & reply_mask) == reply);
> }
> @@ -9348,7 +9343,7 @@ int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
> u32 status;
> int ret;
>
> - WARN_ON(!mutex_is_locked(&dev_priv->pcu_lock));
> + mutex_lock(&dev_priv->sb_lock);
>
> #define COND skl_pcode_try_request(dev_priv, mbox, request, reply_mask, reply, \
> &status)
> @@ -9384,6 +9379,7 @@ int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
> preempt_enable();
>
> out:
> + mutex_unlock(&dev_priv->sb_lock);
> return ret ? ret : status;
> #undef COND
> }
> @@ -9453,8 +9449,7 @@ int intel_freq_opcode(struct drm_i915_private *dev_priv, int val)
>
> void intel_pm_setup(struct drm_i915_private *dev_priv)
> {
> - mutex_init(&dev_priv->pcu_lock);
> -
> + mutex_init(&dev_priv->gt_pm.rps.lock);
> atomic_set(&dev_priv->gt_pm.rps.num_waiters, 0);
>
> dev_priv->runtime_pm.suspended = false;
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 069b6a30468f..2cc64f0fda57 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -815,7 +815,6 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> state = enable ? PUNIT_PWRGT_PWR_ON(power_well_id) :
> PUNIT_PWRGT_PWR_GATE(power_well_id);
>
> - mutex_lock(&dev_priv->pcu_lock);
> vlv_punit_get(dev_priv);
>
> #define COND \
> @@ -838,7 +837,6 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>
> out:
> vlv_punit_put(dev_priv);
> - mutex_unlock(&dev_priv->pcu_lock);
> }
>
> static void vlv_power_well_enable(struct drm_i915_private *dev_priv,
> @@ -865,7 +863,6 @@ static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv,
> mask = PUNIT_PWRGT_MASK(power_well_id);
> ctrl = PUNIT_PWRGT_PWR_ON(power_well_id);
>
> - mutex_lock(&dev_priv->pcu_lock);
> vlv_punit_get(dev_priv);
>
> state = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask;
> @@ -886,7 +883,6 @@ static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv,
> WARN_ON(ctrl != state);
>
> vlv_punit_put(dev_priv);
> - mutex_unlock(&dev_priv->pcu_lock);
>
> return enabled;
> }
> @@ -1398,7 +1394,6 @@ static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
> bool enabled;
> u32 state, ctrl;
>
> - mutex_lock(&dev_priv->pcu_lock);
> vlv_punit_get(dev_priv);
>
> state = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) & DP_SSS_MASK(pipe);
> @@ -1417,7 +1412,6 @@ static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
> WARN_ON(ctrl << 16 != state);
>
> vlv_punit_put(dev_priv);
> - mutex_unlock(&dev_priv->pcu_lock);
>
> return enabled;
> }
> @@ -1432,7 +1426,6 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>
> state = enable ? DP_SSS_PWR_ON(pipe) : DP_SSS_PWR_GATE(pipe);
>
> - mutex_lock(&dev_priv->pcu_lock);
> vlv_punit_get(dev_priv);
>
> #define COND \
> @@ -1455,7 +1448,6 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>
> out:
> vlv_punit_put(dev_priv);
> - mutex_unlock(&dev_priv->pcu_lock);
> }
>
> static void chv_pipe_power_well_enable(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> index dc3b491b4d00..2d4e48e9e1d5 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.c
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -142,8 +142,6 @@ u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
> {
> u32 val = 0;
>
> - lockdep_assert_held(&dev_priv->pcu_lock);
> -
> vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
> SB_CRRDDA_NP, addr, &val);
>
> @@ -152,8 +150,6 @@ u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
>
> int vlv_punit_write(struct drm_i915_private *dev_priv, u32 addr, u32 val)
> {
> - lockdep_assert_held(&dev_priv->pcu_lock);
> -
> return vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
> SB_CRWRDA_NP, addr, &val);
> }
--
Thanks,
Sagar
More information about the Intel-gfx
mailing list