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@xxxxxxxxxxxxxxxxxx>
---
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@xxxxxxxxx>
/* 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
