* Keep package awake during punit access
@ 2018-01-10 12:55 Chris Wilson
2018-01-10 12:55 ` [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband Chris Wilson
` (7 more replies)
0 siblings, 8 replies; 19+ messages in thread
From: Chris Wilson @ 2018-01-10 12:55 UTC (permalink / raw)
To: intel-gfx
If we use the punit often enough, we eventually see a machine hang on
Valleyview and perhaps it may explain a few Braswell incompletes as
well. The cause seems to be the package sleeping while the punit is
active, so we simply have to wake everything up before we start talking.
This is very expensive...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-10 12:55 Keep package awake during punit access Chris Wilson
@ 2018-01-10 12:55 ` Chris Wilson
2018-01-10 13:18 ` Hans de Goede
2018-01-11 20:10 ` Ville Syrjälä
2018-01-10 12:55 ` [PATCH 2/7] drm/i915: Lift acquiring the vlv punit to the callers Chris Wilson
` (6 subsequent siblings)
7 siblings, 2 replies; 19+ messages in thread
From: Chris Wilson @ 2018-01-10 12:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Hans de Goede
While we talk to the punit over its sideband, we need to prevent the cpu
from sleeping in order to prevent a potential machine hang.
Note that by itself, it appears that pm_qos_update_request (via
intel_idle) doesn't provide a sufficient barrier to ensure that all core
are indeed awake (out of Cstate) and that the package is awake. To do so,
we need to supplement the pm_qos with a manual ping on_each_cpu.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
drivers/gpu/drm/i915/i915_drv.c | 6 ++++
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_sideband.c | 61 ++++++++++++++++++++++++-----------
3 files changed, 50 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c8da9d20c33..d4b90cc0130b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -902,6 +902,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
spin_lock_init(&dev_priv->uncore.lock);
mutex_init(&dev_priv->sb_lock);
+ pm_qos_add_request(&dev_priv->sb_qos,
+ PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
+
mutex_init(&dev_priv->modeset_restore_lock);
mutex_init(&dev_priv->av_mutex);
mutex_init(&dev_priv->wm.wm_mutex);
@@ -953,6 +956,9 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
intel_irq_fini(dev_priv);
i915_workqueues_cleanup(dev_priv);
i915_engines_cleanup(dev_priv);
+
+ pm_qos_remove_request(&dev_priv->sb_qos);
+ mutex_destroy(&dev_priv->sb_lock);
}
static int i915_mmio_setup(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a689396d0ff6..ff3f9effc0bb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1887,6 +1887,7 @@ struct drm_i915_private {
/* Sideband mailbox protection */
struct mutex sb_lock;
+ struct pm_qos_request sb_qos;
/** Cached value of IMR to avoid reads in updating the bitfield */
union {
diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
index 75c872bb8cc9..02bdd2e2cef6 100644
--- a/drivers/gpu/drm/i915/intel_sideband.c
+++ b/drivers/gpu/drm/i915/intel_sideband.c
@@ -22,6 +22,8 @@
*
*/
+#include <asm/iosf_mbi.h>
+
#include "i915_drv.h"
#include "intel_drv.h"
@@ -39,18 +41,20 @@
/* Private register write, double-word addressing, non-posted */
#define SB_CRWRDA_NP 0x07
-static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
- u32 port, u32 opcode, u32 addr, u32 *val)
+static void ping(void *info)
{
- u32 cmd, be = 0xf, bar = 0;
- bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
+}
- cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
- (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
- (bar << IOSF_BAR_SHIFT);
+static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
+ u32 devfn, u32 port, u32 opcode,
+ u32 addr, u32 *val)
+{
+ const bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
+ int err;
- WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
+ lockdep_assert_held(&dev_priv->sb_lock);
+ /* Flush the previous comms, just in case it failed last time. */
if (intel_wait_for_register(dev_priv,
VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
5)) {
@@ -59,22 +63,43 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
return -EAGAIN;
}
- I915_WRITE(VLV_IOSF_ADDR, addr);
- I915_WRITE(VLV_IOSF_DATA, is_read ? 0 : *val);
- I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
+ iosf_mbi_punit_acquire();
- if (intel_wait_for_register(dev_priv,
- VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
- 5)) {
+ /*
+ * Prevent the cpu from sleeping while we use this sideband, otherwise
+ * the punit may cause a machine hang.
+ */
+ pm_qos_update_request(&dev_priv->sb_qos, 0);
+ on_each_cpu(ping, NULL, 1);
+ preempt_disable();
+
+ I915_WRITE_FW(VLV_IOSF_ADDR, addr);
+ I915_WRITE_FW(VLV_IOSF_DATA, is_read ? 0 : *val);
+ I915_WRITE_FW(VLV_IOSF_DOORBELL_REQ,
+ (devfn << IOSF_DEVFN_SHIFT) |
+ (opcode << IOSF_OPCODE_SHIFT) |
+ (port << IOSF_PORT_SHIFT) |
+ (0xf << IOSF_BYTE_ENABLES_SHIFT) |
+ (0 << IOSF_BAR_SHIFT) |
+ IOSF_SB_BUSY);
+
+ if (__intel_wait_for_register_fw(dev_priv,
+ VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
+ 10000, 0, NULL) == 0) {
+ if (is_read)
+ *val = I915_READ_FW(VLV_IOSF_DATA);
+ err = 0;
+ } else {
DRM_DEBUG_DRIVER("IOSF sideband finish wait (%s) timed out\n",
is_read ? "read" : "write");
- return -ETIMEDOUT;
+ err = -ETIMEDOUT;
}
- if (is_read)
- *val = I915_READ(VLV_IOSF_DATA);
+ preempt_enable();
+ pm_qos_update_request(&dev_priv->sb_qos, PM_QOS_DEFAULT_VALUE);
+ iosf_mbi_punit_release();
- return 0;
+ return err;
}
u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
--
2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/7] drm/i915: Lift acquiring the vlv punit to the callers
2018-01-10 12:55 Keep package awake during punit access Chris Wilson
2018-01-10 12:55 ` [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband Chris Wilson
@ 2018-01-10 12:55 ` Chris Wilson
2018-01-10 12:55 ` [PATCH 3/7] drm/i915: Lift sideband locking for vlv_punit_(read|write) Chris Wilson
` (5 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2018-01-10 12:55 UTC (permalink / raw)
To: intel-gfx
As we now employ a very heavy pm_qos around the punit access, we want to
minimise the number of synchronous requests by performing one for the
whole punit sequence rather than around individual accesses. The
sideband lock is used for this, so push the pm_qos into the sideband
lock acquisition and release, moving it from the lowlevel punit rw
routine to the callers.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 10 ++++++
drivers/gpu/drm/i915/intel_cdclk.c | 4 +--
drivers/gpu/drm/i915/intel_display.c | 36 ++++++++++-----------
drivers/gpu/drm/i915/intel_dp.c | 4 +--
drivers/gpu/drm/i915/intel_dpio_phy.c | 33 ++++++++++----------
drivers/gpu/drm/i915/intel_dsi.c | 8 ++---
drivers/gpu/drm/i915/intel_dsi_pll.c | 14 ++++-----
drivers/gpu/drm/i915/intel_dsi_vbt.c | 8 ++---
drivers/gpu/drm/i915/intel_hdmi.c | 4 +--
drivers/gpu/drm/i915/intel_pm.c | 4 +--
drivers/gpu/drm/i915/intel_runtime_pm.c | 8 ++---
drivers/gpu/drm/i915/intel_sideband.c | 55 ++++++++++++++++++++-------------
12 files changed, 104 insertions(+), 84 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ff3f9effc0bb..40a61fd13bef 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3724,19 +3724,29 @@ int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
u32 reply_mask, u32 reply, int timeout_base_ms);
/* intel_sideband.c */
+void vlv_sideband_get(struct drm_i915_private *dev_priv);
+void vlv_sideband_put(struct drm_i915_private *dev_priv);
+
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);
+
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);
+
u32 vlv_cck_read(struct drm_i915_private *dev_priv, u32 reg);
void vlv_cck_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
+
u32 vlv_ccu_read(struct drm_i915_private *dev_priv, u32 reg);
void vlv_ccu_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
+
u32 vlv_bunit_read(struct drm_i915_private *dev_priv, u32 reg);
void vlv_bunit_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
+
u32 vlv_dpio_read(struct drm_i915_private *dev_priv, enum pipe pipe, int reg);
void vlv_dpio_write(struct drm_i915_private *dev_priv, enum pipe pipe, int reg, u32 val);
+
u32 intel_sbi_read(struct drm_i915_private *dev_priv, u16 reg,
enum intel_sbi_destination destination);
void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index ca36321eafac..20189cad0eab 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -552,7 +552,7 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
}
mutex_unlock(&dev_priv->pcu_lock);
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
if (cdclk == 400000) {
u32 divider;
@@ -586,7 +586,7 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
val |= 3000 / 250; /* 3.0 usec */
vlv_bunit_write(dev_priv, BUNIT_REG_BISOC, val);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
intel_update_cdclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 221e3a183d36..345cccaba6d9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -165,10 +165,10 @@ int vlv_get_hpll_vco(struct drm_i915_private *dev_priv)
int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
/* Obtain SKU information */
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) &
CCK_FUSE_HPLL_FREQ_MASK;
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
return vco_freq[hpll_freq] * 1000;
}
@@ -179,9 +179,9 @@ int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
u32 val;
int divider;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
val = vlv_cck_read(dev_priv, reg);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
divider = val & CCK_FREQUENCY_VALUES;
@@ -1078,9 +1078,9 @@ void assert_dsi_pll(struct drm_i915_private *dev_priv, bool state)
u32 val;
bool cur_state;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
val = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_CONTROL);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
cur_state = val & DSI_PLL_VCO_EN;
I915_STATE_WARN(cur_state != state,
@@ -1428,14 +1428,14 @@ static void _chv_enable_pll(struct intel_crtc *crtc,
enum dpio_channel port = vlv_pipe_to_channel(pipe);
u32 tmp;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* Enable back the 10bit clock to display controller */
tmp = vlv_dpio_read(dev_priv, pipe, CHV_CMN_DW14(port));
tmp |= DPIO_DCLKP_EN;
vlv_dpio_write(dev_priv, pipe, CHV_CMN_DW14(port), tmp);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
/*
* Need to wait > 100ns between dclkp clock enable bit and PLL enable.
@@ -1620,14 +1620,14 @@ static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
I915_WRITE(DPLL(pipe), val);
POSTING_READ(DPLL(pipe));
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* Disable 10bit clock to display controller */
val = vlv_dpio_read(dev_priv, pipe, CHV_CMN_DW14(port));
val &= ~DPIO_DCLKP_EN;
vlv_dpio_write(dev_priv, pipe, CHV_CMN_DW14(port), val);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
@@ -6638,7 +6638,7 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
if ((pipe_config->dpll_hw_state.dpll & DPLL_VCO_ENABLE) == 0)
return;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
bestn = pipe_config->dpll.n;
bestm1 = pipe_config->dpll.m1;
@@ -6715,7 +6715,7 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW7(pipe), coreclk);
vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW11(pipe), 0x87871000);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
static void chv_prepare_pll(struct intel_crtc *crtc,
@@ -6748,7 +6748,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
dpio_val = 0;
loopfilter = 0;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* p1 and p2 divider */
vlv_dpio_write(dev_priv, pipe, CHV_CMN_DW13(port),
@@ -6820,7 +6820,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
vlv_dpio_read(dev_priv, pipe, CHV_CMN_DW14(port)) |
DPIO_AFC_RECAL);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
/**
@@ -7422,9 +7422,9 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
if ((pipe_config->dpll_hw_state.dpll & DPLL_VCO_ENABLE) == 0)
return;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
mdiv = vlv_dpio_read(dev_priv, pipe, VLV_PLL_DW3(pipe));
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
clock.m1 = (mdiv >> DPIO_M1DIV_SHIFT) & 7;
clock.m2 = mdiv & DPIO_M2DIV_MASK;
@@ -7524,13 +7524,13 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc,
if ((pipe_config->dpll_hw_state.dpll & DPLL_VCO_ENABLE) == 0)
return;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
cmn_dw13 = vlv_dpio_read(dev_priv, pipe, CHV_CMN_DW13(port));
pll_dw0 = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW0(port));
pll_dw1 = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW1(port));
pll_dw2 = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW2(port));
pll_dw3 = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW3(port));
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
clock.m1 = (pll_dw1 & 0x7) == DPIO_CHV_M1_DIV_BY_2 ? 2 : 0;
clock.m2 = (pll_dw0 & 0xff) << 22;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 68229f53d5b8..0e9fee9cf76a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2771,12 +2771,12 @@ static void chv_post_disable_dp(struct intel_encoder *encoder,
intel_dp_link_down(encoder, old_crtc_state);
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* Assert data lane reset */
chv_data_lane_soft_reset(encoder, old_crtc_state, true);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
static void
diff --git a/drivers/gpu/drm/i915/intel_dpio_phy.c b/drivers/gpu/drm/i915/intel_dpio_phy.c
index 76473e9836c6..cd06fc206610 100644
--- a/drivers/gpu/drm/i915/intel_dpio_phy.c
+++ b/drivers/gpu/drm/i915/intel_dpio_phy.c
@@ -645,7 +645,7 @@ void chv_set_phy_signal_level(struct intel_encoder *encoder,
u32 val;
int i;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* Clear calc init */
val = vlv_dpio_read(dev_priv, pipe, VLV_PCS01_DW10(ch));
@@ -726,8 +726,7 @@ void chv_set_phy_signal_level(struct intel_encoder *encoder,
vlv_dpio_write(dev_priv, pipe, VLV_PCS23_DW10(ch), val);
}
- mutex_unlock(&dev_priv->sb_lock);
-
+ vlv_sideband_put(dev_priv);
}
void chv_data_lane_soft_reset(struct intel_encoder *encoder,
@@ -797,7 +796,7 @@ void chv_phy_pre_pll_enable(struct intel_encoder *encoder,
chv_phy_powergate_lanes(encoder, true, lane_mask);
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* Assert data lane reset */
chv_data_lane_soft_reset(encoder, crtc_state, true);
@@ -852,7 +851,7 @@ void chv_phy_pre_pll_enable(struct intel_encoder *encoder,
val |= CHV_CMN_USEDCLKCHANNEL;
vlv_dpio_write(dev_priv, pipe, CHV_CMN_DW19(ch), val);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
void chv_phy_pre_encoder_enable(struct intel_encoder *encoder,
@@ -867,7 +866,7 @@ void chv_phy_pre_encoder_enable(struct intel_encoder *encoder,
int data, i, stagger;
u32 val;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* allow hardware to manage TX FIFO reset source */
val = vlv_dpio_read(dev_priv, pipe, VLV_PCS01_DW11(ch));
@@ -932,7 +931,7 @@ void chv_phy_pre_encoder_enable(struct intel_encoder *encoder,
/* Deassert data lane reset */
chv_data_lane_soft_reset(encoder, crtc_state, false);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
void chv_phy_release_cl2_override(struct intel_encoder *encoder)
@@ -953,7 +952,7 @@ void chv_phy_post_pll_disable(struct intel_encoder *encoder,
enum pipe pipe = to_intel_crtc(old_crtc_state->base.crtc)->pipe;
u32 val;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* disable left/right clock distribution */
if (pipe != PIPE_B) {
@@ -966,7 +965,7 @@ void chv_phy_post_pll_disable(struct intel_encoder *encoder,
vlv_dpio_write(dev_priv, pipe, _CHV_CMN_DW1_CH1, val);
}
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
/*
* Leave the power down bit cleared for at least one
@@ -990,7 +989,7 @@ void vlv_set_phy_signal_level(struct intel_encoder *encoder,
enum dpio_channel port = vlv_dport_to_channel(dport);
enum pipe pipe = intel_crtc->pipe;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
vlv_dpio_write(dev_priv, pipe, VLV_TX_DW5(port), 0x00000000);
vlv_dpio_write(dev_priv, pipe, VLV_TX_DW4(port), demph_reg_value);
vlv_dpio_write(dev_priv, pipe, VLV_TX_DW2(port),
@@ -1003,7 +1002,7 @@ void vlv_set_phy_signal_level(struct intel_encoder *encoder,
vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW11(port), 0x00030000);
vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW9(port), preemph_reg_value);
vlv_dpio_write(dev_priv, pipe, VLV_TX_DW5(port), DPIO_TX_OCALINIT_EN);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
void vlv_phy_pre_pll_enable(struct intel_encoder *encoder,
@@ -1016,7 +1015,7 @@ void vlv_phy_pre_pll_enable(struct intel_encoder *encoder,
enum pipe pipe = crtc->pipe;
/* Program Tx lane resets to default */
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW0(port),
DPIO_PCS_TX_LANE2_RESET |
DPIO_PCS_TX_LANE1_RESET);
@@ -1030,7 +1029,7 @@ void vlv_phy_pre_pll_enable(struct intel_encoder *encoder,
vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW12(port), 0x00750f00);
vlv_dpio_write(dev_priv, pipe, VLV_TX_DW11(port), 0x00001500);
vlv_dpio_write(dev_priv, pipe, VLV_TX_DW14(port), 0x40400000);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
void vlv_phy_pre_encoder_enable(struct intel_encoder *encoder,
@@ -1044,7 +1043,7 @@ void vlv_phy_pre_encoder_enable(struct intel_encoder *encoder,
enum pipe pipe = crtc->pipe;
u32 val;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* Enable clock channels for this port */
val = vlv_dpio_read(dev_priv, pipe, VLV_PCS01_DW8(port));
@@ -1060,7 +1059,7 @@ void vlv_phy_pre_encoder_enable(struct intel_encoder *encoder,
vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW14(port), 0x00760018);
vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW23(port), 0x00400888);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
void vlv_phy_reset_lanes(struct intel_encoder *encoder,
@@ -1072,8 +1071,8 @@ void vlv_phy_reset_lanes(struct intel_encoder *encoder,
enum dpio_channel port = vlv_dport_to_channel(dport);
enum pipe pipe = crtc->pipe;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW0(port), 0x00000000);
vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW1(port), 0x00e00060);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index f67d321376e4..b78617722249 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -278,7 +278,7 @@ static int dpi_send_cmd(struct intel_dsi *intel_dsi, u32 cmd, bool hs,
static void band_gap_reset(struct drm_i915_private *dev_priv)
{
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
vlv_flisdsi_write(dev_priv, 0x08, 0x0001);
vlv_flisdsi_write(dev_priv, 0x0F, 0x0005);
@@ -287,7 +287,7 @@ static void band_gap_reset(struct drm_i915_private *dev_priv)
vlv_flisdsi_write(dev_priv, 0x0F, 0x0000);
vlv_flisdsi_write(dev_priv, 0x08, 0x0000);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
static inline bool is_vid_mode(struct intel_dsi *intel_dsi)
@@ -509,11 +509,11 @@ static void vlv_dsi_device_ready(struct intel_encoder *encoder)
DRM_DEBUG_KMS("\n");
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* program rcomp for compliance, reduce from 50 ohms to 45 ohms
* needed everytime after power gate */
vlv_flisdsi_write(dev_priv, 0x04, 0x0004);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
/* bandgap reset is needed after everytime we do power gate */
band_gap_reset(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
index 2ff2ee7f3b78..bcbecad7f8f9 100644
--- a/drivers/gpu/drm/i915/intel_dsi_pll.c
+++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
@@ -149,7 +149,7 @@ static void vlv_enable_dsi_pll(struct intel_encoder *encoder,
DRM_DEBUG_KMS("\n");
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
vlv_cck_write(dev_priv, CCK_REG_DSI_PLL_CONTROL, 0);
vlv_cck_write(dev_priv, CCK_REG_DSI_PLL_DIVIDER, config->dsi_pll.div);
@@ -166,11 +166,11 @@ static void vlv_enable_dsi_pll(struct intel_encoder *encoder,
if (wait_for(vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_CONTROL) &
DSI_PLL_LOCK, 20)) {
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
DRM_ERROR("DSI PLL lock failed\n");
return;
}
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
DRM_DEBUG_KMS("DSI PLL locked\n");
}
@@ -182,14 +182,14 @@ static void vlv_disable_dsi_pll(struct intel_encoder *encoder)
DRM_DEBUG_KMS("\n");
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
tmp = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_CONTROL);
tmp &= ~DSI_PLL_VCO_EN;
tmp |= DSI_PLL_LDO_GATE;
vlv_cck_write(dev_priv, CCK_REG_DSI_PLL_CONTROL, tmp);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
static bool bxt_dsi_pll_is_enabled(struct drm_i915_private *dev_priv)
@@ -274,10 +274,10 @@ static u32 vlv_dsi_get_pclk(struct intel_encoder *encoder, int pipe_bpp,
DRM_DEBUG_KMS("\n");
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
pll_ctl = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_CONTROL);
pll_div = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_DIVIDER);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
config->dsi_pll.ctrl = pll_ctl & ~DSI_PLL_LOCK;
config->dsi_pll.div = pll_div;
diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
index 91c07b0c8db9..72ddc215b7a9 100644
--- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
@@ -234,7 +234,7 @@ static void vlv_exec_gpio(struct drm_i915_private *dev_priv,
pconf0 = VLV_GPIO_PCONF0(map->base_offset);
padval = VLV_GPIO_PAD_VAL(map->base_offset);
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
if (!map->init) {
/* FIXME: remove constant below */
vlv_iosf_sb_write(dev_priv, port, pconf0, 0x2000CC00);
@@ -243,7 +243,7 @@ static void vlv_exec_gpio(struct drm_i915_private *dev_priv,
tmp = 0x4 | value;
vlv_iosf_sb_write(dev_priv, port, padval, tmp);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
static void chv_exec_gpio(struct drm_i915_private *dev_priv,
@@ -289,12 +289,12 @@ static void chv_exec_gpio(struct drm_i915_private *dev_priv,
cfg0 = CHV_GPIO_PAD_CFG0(family_num, gpio_index);
cfg1 = CHV_GPIO_PAD_CFG1(family_num, gpio_index);
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
vlv_iosf_sb_write(dev_priv, port, cfg0,
CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
CHV_GPIO_GPIOTXSTATE(value));
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
static void bxt_exec_gpio(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 691f15b59124..fbcd2848acdf 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1995,12 +1995,12 @@ static void chv_hdmi_post_disable(struct intel_encoder *encoder,
struct drm_device *dev = encoder->base.dev;
struct drm_i915_private *dev_priv = to_i915(dev);
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* Assert data lane reset */
chv_data_lane_soft_reset(encoder, old_crtc_state, true);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
}
static void chv_hdmi_pre_enable(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1db79a860b96..ee6a87afcfe0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7173,9 +7173,9 @@ static void cherryview_init_gt_powersave(struct drm_i915_private *dev_priv)
vlv_init_gpll_ref_freq(dev_priv);
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
val = vlv_cck_read(dev_priv, CCK_FUSE_REG);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
switch ((val >> 2) & 0x7) {
case 3:
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index d758da6156a8..aab595f5065f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1182,7 +1182,7 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
1))
DRM_ERROR("Display PHY %d is not power up\n", phy);
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
/* Enable dynamic power down */
tmp = vlv_dpio_read(dev_priv, pipe, CHV_CMN_DW28);
@@ -1205,7 +1205,7 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
vlv_dpio_write(dev_priv, pipe, CHV_CMN_DW30, tmp);
}
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
@@ -1268,9 +1268,9 @@ static void assert_chv_phy_powergate(struct drm_i915_private *dev_priv, enum dpi
else
reg = _CHV_CMN_DW6_CH1;
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
val = vlv_dpio_read(dev_priv, pipe, reg);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
/*
* This assumes !override is only used when the port is disabled.
diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
index 02bdd2e2cef6..8d1dc38764d0 100644
--- a/drivers/gpu/drm/i915/intel_sideband.c
+++ b/drivers/gpu/drm/i915/intel_sideband.c
@@ -45,6 +45,27 @@ static void ping(void *info)
{
}
+void vlv_sideband_get(struct drm_i915_private *dev_priv)
+{
+ /*
+ * Prevent the cpu from sleeping while we use this sideband, otherwise
+ * the punit may cause a machine hang.
+ */
+ pm_qos_update_request(&dev_priv->sb_qos, 0);
+ on_each_cpu(ping, NULL, 1);
+
+ iosf_mbi_punit_acquire();
+ mutex_lock(&dev_priv->sb_lock);
+}
+
+void vlv_sideband_put(struct drm_i915_private *dev_priv)
+{
+ mutex_unlock(&dev_priv->sb_lock);
+ iosf_mbi_punit_release();
+
+ pm_qos_update_request(&dev_priv->sb_qos, PM_QOS_DEFAULT_VALUE);
+}
+
static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
u32 devfn, u32 port, u32 opcode,
u32 addr, u32 *val)
@@ -53,6 +74,7 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
int err;
lockdep_assert_held(&dev_priv->sb_lock);
+ iosf_mbi_assert_punit_acquired();
/* Flush the previous comms, just in case it failed last time. */
if (intel_wait_for_register(dev_priv,
@@ -63,14 +85,6 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
return -EAGAIN;
}
- iosf_mbi_punit_acquire();
-
- /*
- * Prevent the cpu from sleeping while we use this sideband, otherwise
- * the punit may cause a machine hang.
- */
- pm_qos_update_request(&dev_priv->sb_qos, 0);
- on_each_cpu(ping, NULL, 1);
preempt_disable();
I915_WRITE_FW(VLV_IOSF_ADDR, addr);
@@ -96,8 +110,6 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
}
preempt_enable();
- pm_qos_update_request(&dev_priv->sb_qos, PM_QOS_DEFAULT_VALUE);
- iosf_mbi_punit_release();
return err;
}
@@ -106,12 +118,12 @@ u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
{
u32 val = 0;
- WARN_ON(!mutex_is_locked(&dev_priv->pcu_lock));
+ lockdep_assert_held(&dev_priv->pcu_lock);
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
SB_CRRDDA_NP, addr, &val);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
return val;
}
@@ -120,12 +132,12 @@ int vlv_punit_write(struct drm_i915_private *dev_priv, u32 addr, u32 val)
{
int err;
- WARN_ON(!mutex_is_locked(&dev_priv->pcu_lock));
+ lockdep_assert_held(&dev_priv->pcu_lock);
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
err = vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
SB_CRWRDA_NP, addr, &val);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
return err;
}
@@ -150,12 +162,10 @@ u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr)
{
u32 val = 0;
- WARN_ON(!mutex_is_locked(&dev_priv->pcu_lock));
-
- mutex_lock(&dev_priv->sb_lock);
+ vlv_sideband_get(dev_priv);
vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_NC,
SB_CRRDDA_NP, addr, &val);
- mutex_unlock(&dev_priv->sb_lock);
+ vlv_sideband_put(dev_priv);
return val;
}
@@ -231,7 +241,8 @@ u32 intel_sbi_read(struct drm_i915_private *dev_priv, u16 reg,
enum intel_sbi_destination destination)
{
u32 value = 0;
- WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
+
+ lockdep_assert_held(&dev_priv->sb_lock);
if (intel_wait_for_register(dev_priv,
SBI_CTL_STAT, SBI_BUSY, 0,
@@ -271,7 +282,7 @@ void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
{
u32 tmp;
- WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
+ lockdep_assert_held(&dev_priv->sb_lock);
if (intel_wait_for_register(dev_priv,
SBI_CTL_STAT, SBI_BUSY, 0,
--
2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/7] drm/i915: Lift sideband locking for vlv_punit_(read|write)
2018-01-10 12:55 Keep package awake during punit access Chris Wilson
2018-01-10 12:55 ` [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband Chris Wilson
2018-01-10 12:55 ` [PATCH 2/7] drm/i915: Lift acquiring the vlv punit to the callers Chris Wilson
@ 2018-01-10 12:55 ` Chris Wilson
2018-01-10 12:55 ` [PATCH 4/7] drm/i915: Reduce RPS update frequency on Valleyview/Cherryview Chris Wilson
` (4 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2018-01-10 12:55 UTC (permalink / raw)
To: intel-gfx
Lift the sideband acquisition for vlv_punit_read and vlv_punit_write
into their callers, so that we can lock the sideband once for a sequence
of operations, rather than perform the heavyweight acquisition on each
request.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_debugfs.c | 3 +++
drivers/gpu/drm/i915/i915_sysfs.c | 15 +++++++-------
drivers/gpu/drm/i915/intel_cdclk.c | 14 ++++++++++---
drivers/gpu/drm/i915/intel_display.c | 16 ++++++++-------
drivers/gpu/drm/i915/intel_pm.c | 35 +++++++++++++++++++++++++--------
drivers/gpu/drm/i915/intel_runtime_pm.c | 8 ++++++++
drivers/gpu/drm/i915/intel_sideband.c | 14 ++-----------
7 files changed, 68 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2bb63073d73f..7174da799efb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1033,7 +1033,10 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
yesno((rpmodectl & GEN6_RP_MEDIA_MODE_MASK) ==
GEN6_RP_MEDIA_SW_MODE));
+ vlv_sideband_get(dev_priv);
freq_sts = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
+ vlv_sideband_put(dev_priv);
+
seq_printf(m, "PUNIT_REG_GPU_FREQ_STS: 0x%08x\n", freq_sts);
seq_printf(m, "DDR freq: %d MHz\n", dev_priv->mem_freq);
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index c74a20b80182..6dad9d3ce731 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -258,25 +258,26 @@ static ssize_t gt_act_freq_mhz_show(struct device *kdev,
struct device_attribute *attr, char *buf)
{
struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- int ret;
+ u32 freq;
intel_runtime_pm_get(dev_priv);
mutex_lock(&dev_priv->pcu_lock);
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
- u32 freq;
+
+ vlv_sideband_get(dev_priv);
freq = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
- ret = intel_gpu_freq(dev_priv, (freq >> 8) & 0xff);
+ vlv_sideband_put(dev_priv);
+
+ freq = (freq >> 8) & 0xff;
} else {
- ret = intel_gpu_freq(dev_priv,
- intel_get_cagf(dev_priv,
- I915_READ(GEN6_RPSTAT1)));
+ freq = intel_get_cagf(dev_priv, I915_READ(GEN6_RPSTAT1));
}
mutex_unlock(&dev_priv->pcu_lock);
intel_runtime_pm_put(dev_priv);
- return snprintf(buf, PAGE_SIZE, "%d\n", ret);
+ return snprintf(buf, PAGE_SIZE, "%d\n", intel_gpu_freq(dev_priv, freq));
}
static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 20189cad0eab..bb8f7c163351 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -461,13 +461,17 @@ static void vlv_get_cdclk(struct drm_i915_private *dev_priv,
{
u32 val;
+ mutex_lock(&dev_priv->pcu_lock);
+ vlv_sideband_get(dev_priv);
+
cdclk_state->vco = vlv_get_hpll_vco(dev_priv);
cdclk_state->cdclk = vlv_get_cck_clock(dev_priv, "cdclk",
CCK_DISPLAY_CLOCK_CONTROL,
cdclk_state->vco);
- mutex_lock(&dev_priv->pcu_lock);
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
+
+ vlv_sideband_put(dev_priv);
mutex_unlock(&dev_priv->pcu_lock);
if (IS_VALLEYVIEW(dev_priv))
@@ -540,6 +544,8 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
*/
intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE_A);
+ vlv_sideband_get(dev_priv);
+
mutex_lock(&dev_priv->pcu_lock);
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
val &= ~DSPFREQGUAR_MASK;
@@ -552,8 +558,6 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
}
mutex_unlock(&dev_priv->pcu_lock);
- vlv_sideband_get(dev_priv);
-
if (cdclk == 400000) {
u32 divider;
@@ -621,6 +625,8 @@ static void chv_set_cdclk(struct drm_i915_private *dev_priv,
intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE_A);
mutex_lock(&dev_priv->pcu_lock);
+ vlv_sideband_get(dev_priv);
+
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
val &= ~DSPFREQGUAR_MASK_CHV;
val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
@@ -630,6 +636,8 @@ static void chv_set_cdclk(struct drm_i915_private *dev_priv,
50)) {
DRM_ERROR("timed out waiting for CDclk change\n");
}
+
+ vlv_sideband_put(dev_priv);
mutex_unlock(&dev_priv->pcu_lock);
intel_update_cdclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 345cccaba6d9..46f5d11be60c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -165,10 +165,8 @@ int vlv_get_hpll_vco(struct drm_i915_private *dev_priv)
int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
/* Obtain SKU information */
- vlv_sideband_get(dev_priv);
hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) &
CCK_FUSE_HPLL_FREQ_MASK;
- vlv_sideband_put(dev_priv);
return vco_freq[hpll_freq] * 1000;
}
@@ -179,10 +177,7 @@ int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
u32 val;
int divider;
- vlv_sideband_get(dev_priv);
val = vlv_cck_read(dev_priv, reg);
- vlv_sideband_put(dev_priv);
-
divider = val & CCK_FREQUENCY_VALUES;
WARN((val & CCK_FREQUENCY_STATUS) !=
@@ -195,11 +190,18 @@ int vlv_get_cck_clock(struct drm_i915_private *dev_priv,
int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
const char *name, u32 reg)
{
+ int hpll;
+
+ vlv_sideband_get(dev_priv);
+
if (dev_priv->hpll_freq == 0)
dev_priv->hpll_freq = vlv_get_hpll_vco(dev_priv);
- return vlv_get_cck_clock(dev_priv, name, reg,
- dev_priv->hpll_freq);
+ hpll = vlv_get_cck_clock(dev_priv, name, reg, dev_priv->hpll_freq);
+
+ vlv_sideband_put(dev_priv);
+
+ return hpll;
}
static void intel_update_czclk(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 ee6a87afcfe0..fcde1dc6b2ed 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -311,6 +311,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
u32 val;
mutex_lock(&dev_priv->pcu_lock);
+ vlv_sideband_get(dev_priv);
val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
if (enable)
@@ -325,6 +326,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
+ vlv_sideband_put(dev_priv);
mutex_unlock(&dev_priv->pcu_lock);
}
@@ -333,6 +335,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
u32 val;
mutex_lock(&dev_priv->pcu_lock);
+ vlv_sideband_get(dev_priv);
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
if (enable)
@@ -341,6 +344,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
val &= ~DSP_MAXFIFO_PM5_ENABLE;
vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
+ vlv_sideband_put(dev_priv);
mutex_unlock(&dev_priv->pcu_lock);
}
@@ -5636,6 +5640,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
if (IS_CHERRYVIEW(dev_priv)) {
mutex_lock(&dev_priv->pcu_lock);
+ vlv_sideband_get(dev_priv);
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
if (val & DSP_MAXFIFO_PM5_ENABLE)
@@ -5665,6 +5670,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
wm->level = VLV_WM_LEVEL_DDR_DVFS;
}
+ vlv_sideband_put(dev_priv);
mutex_unlock(&dev_priv->pcu_lock);
}
@@ -6211,7 +6217,9 @@ static int 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->gt_pm.rps.cur_freq) {
+ vlv_sideband_get(dev_priv);
err = vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
+ vlv_sideband_put(dev_priv);
if (err)
return err;
@@ -7125,9 +7133,12 @@ static void valleyview_init_gt_powersave(struct drm_i915_private *dev_priv)
valleyview_setup_pctx(dev_priv);
+ vlv_sideband_get(dev_priv);
+
vlv_init_gpll_ref_freq(dev_priv);
val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
+
switch ((val >> 6) & 3) {
case 0:
case 1:
@@ -7162,6 +7173,8 @@ static void valleyview_init_gt_powersave(struct drm_i915_private *dev_priv)
DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
intel_gpu_freq(dev_priv, rps->min_freq),
rps->min_freq);
+
+ vlv_sideband_put(dev_priv);
}
static void cherryview_init_gt_powersave(struct drm_i915_private *dev_priv)
@@ -7171,11 +7184,11 @@ static void cherryview_init_gt_powersave(struct drm_i915_private *dev_priv)
cherryview_setup_pctx(dev_priv);
+ vlv_sideband_get(dev_priv);
+
vlv_init_gpll_ref_freq(dev_priv);
- vlv_sideband_get(dev_priv);
val = vlv_cck_read(dev_priv, CCK_FUSE_REG);
- vlv_sideband_put(dev_priv);
switch ((val >> 2) & 0x7) {
case 3:
@@ -7208,6 +7221,8 @@ static void cherryview_init_gt_powersave(struct drm_i915_private *dev_priv)
intel_gpu_freq(dev_priv, rps->min_freq),
rps->min_freq);
+ vlv_sideband_put(dev_priv);
+
WARN_ONCE((rps->max_freq | rps->efficient_freq | rps->rp1_freq |
rps->min_freq) & 1,
"Odd GPU freq values\n");
@@ -7295,13 +7310,15 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv)
GEN6_RP_DOWN_IDLE_AVG);
/* Setting Fixed Bias */
- val = VLV_OVERRIDE_EN |
- VLV_SOC_TDP_EN |
- CHV_BIAS_CPU_50_SOC_50;
+ vlv_sideband_get(dev_priv);
+
+ val = VLV_OVERRIDE_EN | VLV_SOC_TDP_EN | CHV_BIAS_CPU_50_SOC_50;
vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
+ vlv_sideband_put(dev_priv);
+
/* RPS code assumes GPLL is used */
WARN_ONCE((val & GPLLENABLE) == 0, "GPLL not enabled\n");
@@ -7378,14 +7395,16 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
GEN6_RP_UP_BUSY_AVG |
GEN6_RP_DOWN_IDLE_CONT);
+ vlv_sideband_get(dev_priv);
+
/* Setting Fixed Bias */
- val = VLV_OVERRIDE_EN |
- VLV_SOC_TDP_EN |
- VLV_BIAS_CPU_125_SOC_875;
+ val = VLV_OVERRIDE_EN | VLV_SOC_TDP_EN | VLV_BIAS_CPU_125_SOC_875;
vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
+ vlv_sideband_put(dev_priv);
+
/* RPS code assumes GPLL is used */
WARN_ONCE((val & GPLLENABLE) == 0, "GPLL not enabled\n");
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index aab595f5065f..c434a750e9a3 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -799,6 +799,7 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
PUNIT_PWRGT_PWR_GATE(power_well_id);
mutex_lock(&dev_priv->pcu_lock);
+ vlv_sideband_get(dev_priv);
#define COND \
((vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask) == state)
@@ -819,6 +820,7 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
#undef COND
out:
+ vlv_sideband_put(dev_priv);
mutex_unlock(&dev_priv->pcu_lock);
}
@@ -847,6 +849,7 @@ static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv,
ctrl = PUNIT_PWRGT_PWR_ON(power_well_id);
mutex_lock(&dev_priv->pcu_lock);
+ vlv_sideband_get(dev_priv);
state = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask;
/*
@@ -865,6 +868,7 @@ static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv,
ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL) & mask;
WARN_ON(ctrl != state);
+ vlv_sideband_put(dev_priv);
mutex_unlock(&dev_priv->pcu_lock);
return enabled;
@@ -1378,6 +1382,7 @@ static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
u32 state, ctrl;
mutex_lock(&dev_priv->pcu_lock);
+ vlv_sideband_get(dev_priv);
state = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) & DP_SSS_MASK(pipe);
/*
@@ -1394,6 +1399,7 @@ static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) & DP_SSC_MASK(pipe);
WARN_ON(ctrl << 16 != state);
+ vlv_sideband_put(dev_priv);
mutex_unlock(&dev_priv->pcu_lock);
return enabled;
@@ -1410,6 +1416,7 @@ 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_sideband_get(dev_priv);
#define COND \
((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) & DP_SSS_MASK(pipe)) == state)
@@ -1430,6 +1437,7 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
#undef COND
out:
+ vlv_sideband_put(dev_priv);
mutex_unlock(&dev_priv->pcu_lock);
}
diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
index 8d1dc38764d0..6d1674d479e0 100644
--- a/drivers/gpu/drm/i915/intel_sideband.c
+++ b/drivers/gpu/drm/i915/intel_sideband.c
@@ -120,26 +120,18 @@ u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
lockdep_assert_held(&dev_priv->pcu_lock);
- vlv_sideband_get(dev_priv);
vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
SB_CRRDDA_NP, addr, &val);
- vlv_sideband_put(dev_priv);
return val;
}
int vlv_punit_write(struct drm_i915_private *dev_priv, u32 addr, u32 val)
{
- int err;
-
lockdep_assert_held(&dev_priv->pcu_lock);
- vlv_sideband_get(dev_priv);
- err = vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
- SB_CRWRDA_NP, addr, &val);
- vlv_sideband_put(dev_priv);
-
- return err;
+ return vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
+ SB_CRWRDA_NP, addr, &val);
}
u32 vlv_bunit_read(struct drm_i915_private *dev_priv, u32 reg)
@@ -162,10 +154,8 @@ u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr)
{
u32 val = 0;
- vlv_sideband_get(dev_priv);
vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_NC,
SB_CRRDDA_NP, addr, &val);
- vlv_sideband_put(dev_priv);
return val;
}
--
2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/7] drm/i915: Reduce RPS update frequency on Valleyview/Cherryview
2018-01-10 12:55 Keep package awake during punit access Chris Wilson
` (2 preceding siblings ...)
2018-01-10 12:55 ` [PATCH 3/7] drm/i915: Lift sideband locking for vlv_punit_(read|write) Chris Wilson
@ 2018-01-10 12:55 ` Chris Wilson
2018-01-10 12:55 ` [PATCH 5/7] Revert "drm/i915: Avoid tweaking evaluation thresholds on Baytrail v3" Chris Wilson
` (3 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2018-01-10 12:55 UTC (permalink / raw)
To: intel-gfx
Valleyview and Cherryview update the GPU frequency via the punit, which
is very expensive as we have to ensure the cores do not sleep during the
comms. If we perform frequent RPS evaluations, the frequent punit
requests cause measurable system overhead for little benefit, so
increase the evaluation intervals to reduce the number of times we try
and change frequency.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_pm.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fcde1dc6b2ed..1d5d2a12eca1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6119,6 +6119,19 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
break;
}
+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+ /*
+ * Baytrail and Braswell control the gpu frequency via the
+ * punit, which is very slow and expensive to communicate with,
+ * as we synchronously force the package to C0. If we try and
+ * update the gpufreq too often we cause measurable system
+ * load for little benefit (effectively stealing CPU time for
+ * the GPU, negatively impacting overall throughput).
+ */
+ ei_up <<= 2;
+ ei_down <<= 2;
+ }
+
/* When byt can survive without system hang with dynamic
* sw freq adjustments, this restriction can be lifted.
*/
--
2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 5/7] Revert "drm/i915: Avoid tweaking evaluation thresholds on Baytrail v3"
2018-01-10 12:55 Keep package awake during punit access Chris Wilson
` (3 preceding siblings ...)
2018-01-10 12:55 ` [PATCH 4/7] drm/i915: Reduce RPS update frequency on Valleyview/Cherryview Chris Wilson
@ 2018-01-10 12:55 ` Chris Wilson
2018-01-10 12:55 ` [PATCH 6/7] drm/i915: Replace pcu_lock with sb_lock Chris Wilson
` (2 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2018-01-10 12:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Len Brown, Jani Nikula, Daniel Vetter, fritsch
With the vlv sideband fixed to avoid sleeping while we talk to the
punit, the system should be much more stable and be able to utilise the
punit without risk.
This reverts commit 6067a27d1f01 ("drm/i915: Avoid tweaking evaluation
thresholds on Baytrail v3")
References: 6067a27d1f01 ("drm/i915: Avoid tweaking evaluation thresholds on Baytrail v3")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: fritsch@xbmc.org
---
drivers/gpu/drm/i915/intel_pm.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1d5d2a12eca1..5dfba4739328 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6132,12 +6132,6 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
ei_down <<= 2;
}
- /* When byt can survive without system hang with dynamic
- * sw freq adjustments, this restriction can be lifted.
- */
- if (IS_VALLEYVIEW(dev_priv))
- goto skip_hw_write;
-
I915_WRITE(GEN6_RP_UP_EI,
GT_INTERVAL_FROM_US(dev_priv, ei_up));
I915_WRITE(GEN6_RP_UP_THRESHOLD,
@@ -6158,7 +6152,6 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
GEN6_RP_UP_BUSY_AVG |
GEN6_RP_DOWN_IDLE_AVG);
-skip_hw_write:
rps->power = new_power;
rps->up_threshold = threshold_up;
rps->down_threshold = threshold_down;
--
2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 6/7] drm/i915: Replace pcu_lock with sb_lock
2018-01-10 12:55 Keep package awake during punit access Chris Wilson
` (4 preceding siblings ...)
2018-01-10 12:55 ` [PATCH 5/7] Revert "drm/i915: Avoid tweaking evaluation thresholds on Baytrail v3" Chris Wilson
@ 2018-01-10 12:55 ` Chris Wilson
2018-01-10 12:55 ` [PATCH 7/7] drm/i915: Merge sandybride_pcode_(read|write) Chris Wilson
2018-01-10 13:41 ` ✓ Fi.CI.BAT: success for series starting with [1/7] drm/i915: Disable preemption and sleeping while using the punit sideband Patchwork
7 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2018-01-10 12:55 UTC (permalink / raw)
To: intel-gfx
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).
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_debugfs.c | 52 +++++++------------
drivers/gpu/drm/i915/i915_drv.h | 10 +---
drivers/gpu/drm/i915/i915_irq.c | 4 +-
drivers/gpu/drm/i915/i915_sysfs.c | 36 ++++++-------
drivers/gpu/drm/i915/intel_cdclk.c | 24 ---------
drivers/gpu/drm/i915/intel_display.c | 6 ---
drivers/gpu/drm/i915/intel_hdcp.c | 2 -
drivers/gpu/drm/i915/intel_pm.c | 92 +++++++++++++++++----------------
drivers/gpu/drm/i915/intel_runtime_pm.c | 8 ---
drivers/gpu/drm/i915/intel_sideband.c | 4 --
10 files changed, 86 insertions(+), 152 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7174da799efb..413752f214cc 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1022,8 +1022,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));
@@ -1058,7 +1056,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;
@@ -1486,9 +1483,7 @@ static int gen6_drpc_info(struct seq_file *m)
gen9_powergate_status = I915_READ(GEN9_PWRGT_DOMAIN_STATUS);
}
- mutex_lock(&dev_priv->pcu_lock);
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));
@@ -1754,30 +1749,24 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
{
struct drm_i915_private *dev_priv = node_to_i915(m->private);
struct intel_rps *rps = &dev_priv->gt_pm.rps;
- int ret = 0;
int gpu_freq, ia_freq;
unsigned int max_gpu_freq, min_gpu_freq;
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)) {
/* Convert GT frequency to 50 HZ units */
- min_gpu_freq = rps->min_freq_softlimit / GEN9_FREQ_SCALER;
- max_gpu_freq = rps->max_freq_softlimit / GEN9_FREQ_SCALER;
- } else {
- min_gpu_freq = rps->min_freq_softlimit;
- max_gpu_freq = rps->max_freq_softlimit;
+ min_gpu_freq /= GEN9_FREQ_SCALER;
+ max_gpu_freq /= GEN9_FREQ_SCALER;
}
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,
@@ -1791,12 +1780,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)
@@ -4131,7 +4117,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;
@@ -4144,8 +4130,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;
@@ -4153,9 +4139,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,
@@ -4187,7 +4173,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;
@@ -4201,8 +4187,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;
@@ -4210,9 +4196,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 40a61fd13bef..b311fed33580 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -892,6 +892,8 @@ struct intel_rps_ei {
};
struct intel_rps {
+ struct mutex lock;
+
/*
* work, interrupts_enabled and pm_iir are protected by
* dev_priv->irq_lock
@@ -2034,14 +2036,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;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3517c6548e2c..2aee867e5cfa 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1191,7 +1191,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
if ((pm_iir & dev_priv->pm_rps_events) == 0 && !client_boost)
goto out;
- mutex_lock(&dev_priv->pcu_lock);
+ mutex_lock(&rps->lock);
pm_iir |= vlv_wa_c0_ei(dev_priv, pm_iir);
@@ -1245,7 +1245,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
rps->last_adj = 0;
}
- mutex_unlock(&dev_priv->pcu_lock);
+ mutex_unlock(&rps->lock);
out:
/* Make sure not to corrupt PMIMR state used by ringbuffer on GEN6 */
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 6dad9d3ce731..1187586b4d98 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -262,7 +262,6 @@ static ssize_t gt_act_freq_mhz_show(struct device *kdev,
intel_runtime_pm_get(dev_priv);
- mutex_lock(&dev_priv->pcu_lock);
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
vlv_sideband_get(dev_priv);
@@ -273,7 +272,6 @@ static ssize_t gt_act_freq_mhz_show(struct device *kdev,
} else {
freq = intel_get_cagf(dev_priv, I915_READ(GEN6_RPSTAT1));
}
- mutex_unlock(&dev_priv->pcu_lock);
intel_runtime_pm_put(dev_priv);
@@ -317,9 +315,11 @@ static ssize_t gt_boost_freq_mhz_store(struct device *kdev,
if (val < rps->min_freq || val > rps->max_freq)
return -EINVAL;
- mutex_lock(&dev_priv->pcu_lock);
- rps->boost_freq = val;
- mutex_unlock(&dev_priv->pcu_lock);
+ if (val != READ_ONCE(rps->boost_freq)) {
+ WRITE_ONCE(rps->boost_freq, val);
+ if (atomic_read(&rps->num_waiters))
+ schedule_work(&rps->work);
+ }
return count;
}
@@ -357,17 +357,14 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev,
return ret;
intel_runtime_pm_get(dev_priv);
-
- mutex_lock(&dev_priv->pcu_lock);
+ mutex_lock(&rps->lock);
val = intel_freq_opcode(dev_priv, val);
-
if (val < rps->min_freq ||
val > rps->max_freq ||
val < rps->min_freq_softlimit) {
- mutex_unlock(&dev_priv->pcu_lock);
- intel_runtime_pm_put(dev_priv);
- return -EINVAL;
+ ret = -EINVAL;
+ goto unlock;
}
if (val > rps->rp0_freq)
@@ -385,8 +382,8 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev,
* frequency request may be unchanged. */
ret = intel_set_rps(dev_priv, val);
- mutex_unlock(&dev_priv->pcu_lock);
-
+unlock:
+ mutex_unlock(&rps->lock);
intel_runtime_pm_put(dev_priv);
return ret ?: count;
@@ -415,17 +412,14 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
return ret;
intel_runtime_pm_get(dev_priv);
-
- mutex_lock(&dev_priv->pcu_lock);
+ mutex_lock(&rps->lock);
val = intel_freq_opcode(dev_priv, val);
-
if (val < rps->min_freq ||
val > rps->max_freq ||
val > rps->max_freq_softlimit) {
- mutex_unlock(&dev_priv->pcu_lock);
- intel_runtime_pm_put(dev_priv);
- return -EINVAL;
+ ret = -EINVAL;
+ goto unlock;
}
rps->min_freq_softlimit = val;
@@ -439,8 +433,8 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
* frequency request may be unchanged. */
ret = intel_set_rps(dev_priv, val);
- mutex_unlock(&dev_priv->pcu_lock);
-
+unlock:
+ mutex_unlock(&rps->lock);
intel_runtime_pm_put(dev_priv);
return ret ?: count;
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index bb8f7c163351..8f2e35ae1865 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -461,7 +461,6 @@ static void vlv_get_cdclk(struct drm_i915_private *dev_priv,
{
u32 val;
- mutex_lock(&dev_priv->pcu_lock);
vlv_sideband_get(dev_priv);
cdclk_state->vco = vlv_get_hpll_vco(dev_priv);
@@ -472,7 +471,6 @@ static void vlv_get_cdclk(struct drm_i915_private *dev_priv,
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
vlv_sideband_put(dev_priv);
- mutex_unlock(&dev_priv->pcu_lock);
if (IS_VALLEYVIEW(dev_priv))
cdclk_state->voltage_level = (val & DSPFREQGUAR_MASK) >>
@@ -546,7 +544,6 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
vlv_sideband_get(dev_priv);
- mutex_lock(&dev_priv->pcu_lock);
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
val &= ~DSPFREQGUAR_MASK;
val |= (cmd << DSPFREQGUAR_SHIFT);
@@ -556,7 +553,6 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
50)) {
DRM_ERROR("timed out waiting for CDclk change\n");
}
- mutex_unlock(&dev_priv->pcu_lock);
if (cdclk == 400000) {
u32 divider;
@@ -624,7 +620,6 @@ static void chv_set_cdclk(struct drm_i915_private *dev_priv,
*/
intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE_A);
- mutex_lock(&dev_priv->pcu_lock);
vlv_sideband_get(dev_priv);
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
@@ -638,7 +633,6 @@ static void chv_set_cdclk(struct drm_i915_private *dev_priv,
}
vlv_sideband_put(dev_priv);
- mutex_unlock(&dev_priv->pcu_lock);
intel_update_cdclk(dev_priv);
@@ -716,10 +710,8 @@ static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
"trying to change cdclk frequency with cdclk not enabled\n"))
return;
- mutex_lock(&dev_priv->pcu_lock);
ret = sandybridge_pcode_write(dev_priv,
BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ, 0x0);
- mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
DRM_ERROR("failed to inform pcode about cdclk change\n");
return;
@@ -768,10 +760,8 @@ static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
DRM_ERROR("Switching back to LCPLL failed\n");
- mutex_lock(&dev_priv->pcu_lock);
sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
cdclk_state->voltage_level);
- mutex_unlock(&dev_priv->pcu_lock);
I915_WRITE(CDCLK_FREQ, DIV_ROUND_CLOSEST(cdclk, 1000) - 1);
@@ -999,12 +989,10 @@ static void skl_set_cdclk(struct drm_i915_private *dev_priv,
u32 freq_select, cdclk_ctl;
int ret;
- mutex_lock(&dev_priv->pcu_lock);
ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
SKL_CDCLK_PREPARE_FOR_CHANGE,
SKL_CDCLK_READY_FOR_CHANGE,
SKL_CDCLK_READY_FOR_CHANGE, 3);
- mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
ret);
@@ -1068,10 +1056,8 @@ static void skl_set_cdclk(struct drm_i915_private *dev_priv,
POSTING_READ(CDCLK_CTL);
/* inform PCU of the change */
- mutex_lock(&dev_priv->pcu_lock);
sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
cdclk_state->voltage_level);
- mutex_unlock(&dev_priv->pcu_lock);
intel_update_cdclk(dev_priv);
}
@@ -1379,11 +1365,8 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
}
/* Inform power controller of upcoming frequency change */
- mutex_lock(&dev_priv->pcu_lock);
ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
0x80000000);
- mutex_unlock(&dev_priv->pcu_lock);
-
if (ret) {
DRM_ERROR("PCode CDCLK freq change notify failed (err %d, freq %d)\n",
ret, cdclk);
@@ -1411,11 +1394,8 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
I915_WRITE(CDCLK_CTL, val);
- mutex_lock(&dev_priv->pcu_lock);
ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
cdclk_state->voltage_level);
- mutex_unlock(&dev_priv->pcu_lock);
-
if (ret) {
DRM_ERROR("PCode CDCLK freq set failed, (err %d, freq %d)\n",
ret, cdclk);
@@ -1653,12 +1633,10 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
u32 val, divider;
int ret;
- mutex_lock(&dev_priv->pcu_lock);
ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
SKL_CDCLK_PREPARE_FOR_CHANGE,
SKL_CDCLK_READY_FOR_CHANGE,
SKL_CDCLK_READY_FOR_CHANGE, 3);
- mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
ret);
@@ -1695,10 +1673,8 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
I915_WRITE(CDCLK_CTL, val);
/* inform PCU of the change */
- mutex_lock(&dev_priv->pcu_lock);
sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
cdclk_state->voltage_level);
- mutex_unlock(&dev_priv->pcu_lock);
intel_update_cdclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 46f5d11be60c..f4c1fa0990cf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4858,10 +4858,8 @@ void hsw_enable_ips(const struct intel_crtc_state *crtc_state)
WARN_ON(!(crtc_state->active_planes & ~BIT(PLANE_CURSOR)));
if (IS_BROADWELL(dev_priv)) {
- mutex_lock(&dev_priv->pcu_lock);
WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL,
IPS_ENABLE | IPS_PCODE_CONTROL));
- mutex_unlock(&dev_priv->pcu_lock);
/* Quoting Art Runyan: "its not safe to expect any particular
* value in IPS_CTL bit 31 after enabling IPS through the
* mailbox." Moreover, the mailbox may return a bogus state,
@@ -4891,9 +4889,7 @@ void hsw_disable_ips(const struct intel_crtc_state *crtc_state)
return;
if (IS_BROADWELL(dev_priv)) {
- mutex_lock(&dev_priv->pcu_lock);
WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0));
- mutex_unlock(&dev_priv->pcu_lock);
/* wait for pcode to finish disabling IPS, which may take up to 42ms */
if (intel_wait_for_register(dev_priv,
IPS_CTL, IPS_ENABLE, 0,
@@ -8732,11 +8728,9 @@ static uint32_t hsw_read_dcomp(struct drm_i915_private *dev_priv)
static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
{
if (IS_HASWELL(dev_priv)) {
- mutex_lock(&dev_priv->pcu_lock);
if (sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_D_COMP,
val))
DRM_DEBUG_KMS("Failed to write to D_COMP\n");
- mutex_unlock(&dev_priv->pcu_lock);
} else {
I915_WRITE(D_COMP_BDW, val);
POSTING_READ(D_COMP_BDW);
diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index 827cab22f191..d0e97172b8ed 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -50,9 +50,7 @@ static int intel_hdcp_load_keys(struct drm_i915_private *dev_priv)
u32 val;
/* Initiate loading the HDCP key from fuses */
- mutex_lock(&dev_priv->pcu_lock);
ret = sandybridge_pcode_write(dev_priv, SKL_PCODE_LOAD_HDCP_KEYS, 1);
- mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
DRM_ERROR("Failed to initiate HDCP key load (%d)\n", ret);
return ret;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5dfba4739328..cf8b4c28f1e1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -310,7 +310,6 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
{
u32 val;
- mutex_lock(&dev_priv->pcu_lock);
vlv_sideband_get(dev_priv);
val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
@@ -327,14 +326,12 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
vlv_sideband_put(dev_priv);
- mutex_unlock(&dev_priv->pcu_lock);
}
static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
{
u32 val;
- mutex_lock(&dev_priv->pcu_lock);
vlv_sideband_get(dev_priv);
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
@@ -345,7 +342,6 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
vlv_sideband_put(dev_priv);
- mutex_unlock(&dev_priv->pcu_lock);
}
#define FW_WM(value, plane) \
@@ -2809,11 +2805,9 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv,
/* read the first set of memory latencies[0:3] */
val = 0; /* data0 to be programmed to 0 for first set */
- mutex_lock(&dev_priv->pcu_lock);
ret = sandybridge_pcode_read(dev_priv,
GEN9_PCODE_READ_MEM_LATENCY,
&val);
- mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
DRM_ERROR("SKL Mailbox read error = %d\n", ret);
@@ -2830,11 +2824,9 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv,
/* read the second set of memory latencies[4:7] */
val = 1; /* data0 to be programmed to 1 for second set */
- mutex_lock(&dev_priv->pcu_lock);
ret = sandybridge_pcode_read(dev_priv,
GEN9_PCODE_READ_MEM_LATENCY,
&val);
- mutex_unlock(&dev_priv->pcu_lock);
if (ret) {
DRM_ERROR("SKL Mailbox read error = %d\n", ret);
return;
@@ -3625,13 +3617,10 @@ intel_enable_sagv(struct drm_i915_private *dev_priv)
return 0;
DRM_DEBUG_KMS("Enabling the SAGV\n");
- mutex_lock(&dev_priv->pcu_lock);
-
ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
GEN9_SAGV_ENABLE);
/* We don't need to wait for the SAGV when enabling */
- mutex_unlock(&dev_priv->pcu_lock);
/*
* Some skl systems, pre-release machines in particular,
@@ -3662,15 +3651,11 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
return 0;
DRM_DEBUG_KMS("Disabling the SAGV\n");
- mutex_lock(&dev_priv->pcu_lock);
-
/* bspec says to keep retrying for at least 1 ms */
ret = skl_pcode_request(dev_priv, GEN9_PCODE_SAGV_CONTROL,
GEN9_SAGV_DISABLE,
GEN9_SAGV_IS_DISABLED, GEN9_SAGV_IS_DISABLED,
1);
- mutex_unlock(&dev_priv->pcu_lock);
-
/*
* Some skl systems, pre-release machines in particular,
* don't actually have an SAGV.
@@ -5639,7 +5624,6 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
wm->level = VLV_WM_LEVEL_PM2;
if (IS_CHERRYVIEW(dev_priv)) {
- mutex_lock(&dev_priv->pcu_lock);
vlv_sideband_get(dev_priv);
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
@@ -5671,7 +5655,6 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
}
vlv_sideband_put(dev_priv);
- mutex_unlock(&dev_priv->pcu_lock);
}
for_each_intel_crtc(dev, crtc) {
@@ -6278,7 +6261,7 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
{
struct intel_rps *rps = &dev_priv->gt_pm.rps;
- mutex_lock(&dev_priv->pcu_lock);
+ mutex_lock(&rps->lock);
if (rps->enabled) {
u8 freq;
@@ -6301,7 +6284,7 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
rps->max_freq_softlimit)))
DRM_DEBUG_DRIVER("Failed to set idle frequency\n");
}
- mutex_unlock(&dev_priv->pcu_lock);
+ mutex_unlock(&rps->lock);
}
void gen6_rps_idle(struct drm_i915_private *dev_priv)
@@ -6315,7 +6298,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
*/
gen6_disable_rps_interrupts(dev_priv);
- mutex_lock(&dev_priv->pcu_lock);
+ mutex_lock(&rps->lock);
if (rps->enabled) {
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
vlv_set_rps_idle(dev_priv);
@@ -6325,7 +6308,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
I915_WRITE(GEN6_PMINTRMSK,
gen6_sanitize_rps_pm_mask(dev_priv, ~0));
}
- mutex_unlock(&dev_priv->pcu_lock);
+ mutex_unlock(&rps->lock);
}
void gen6_rps_boost(struct drm_i915_gem_request *rq,
@@ -6363,7 +6346,7 @@ int intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
struct intel_rps *rps = &dev_priv->gt_pm.rps;
int err;
- lockdep_assert_held(&dev_priv->pcu_lock);
+ lockdep_assert_held(&rps->lock);
GEM_BUG_ON(val > rps->max_freq);
GEM_BUG_ON(val < rps->min_freq);
@@ -6842,7 +6825,7 @@ static void gen6_update_ring_freq(struct drm_i915_private *dev_priv)
int scaling_factor = 180;
struct cpufreq_policy *policy;
- WARN_ON(!mutex_is_locked(&dev_priv->pcu_lock));
+ lockdep_assert_held(&rps->lock);
policy = cpufreq_cpu_get(0);
if (policy) {
@@ -7908,7 +7891,7 @@ void intel_init_gt_powersave(struct drm_i915_private *dev_priv)
intel_runtime_pm_get(dev_priv);
}
- mutex_lock(&dev_priv->pcu_lock);
+ mutex_lock(&rps->lock);
/* Initialize RPS limits (for userspace) */
if (IS_CHERRYVIEW(dev_priv))
@@ -7948,7 +7931,7 @@ void intel_init_gt_powersave(struct drm_i915_private *dev_priv)
/* Finally allow us to boost to max by default */
rps->boost_freq = rps->max_freq;
- mutex_unlock(&dev_priv->pcu_lock);
+ mutex_unlock(&rps->lock);
}
void intel_cleanup_gt_powersave(struct drm_i915_private *dev_priv)
@@ -7987,7 +7970,7 @@ void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv)
static inline void intel_disable_llc_pstate(struct drm_i915_private *i915)
{
- lockdep_assert_held(&i915->pcu_lock);
+ lockdep_assert_held(&i915->gt_pm.rps.lock);
if (!i915->gt_pm.llc_pstate.enabled)
return;
@@ -7999,7 +7982,7 @@ static inline void intel_disable_llc_pstate(struct drm_i915_private *i915)
static void intel_disable_rc6(struct drm_i915_private *dev_priv)
{
- lockdep_assert_held(&dev_priv->pcu_lock);
+ lockdep_assert_held(&dev_priv->gt_pm.rps.lock);
if (!dev_priv->gt_pm.rc6.enabled)
return;
@@ -8018,7 +8001,7 @@ static void intel_disable_rc6(struct drm_i915_private *dev_priv)
static void intel_disable_rps(struct drm_i915_private *dev_priv)
{
- lockdep_assert_held(&dev_priv->pcu_lock);
+ lockdep_assert_held(&dev_priv->gt_pm.rps.lock);
if (!dev_priv->gt_pm.rps.enabled)
return;
@@ -8039,19 +8022,19 @@ static void intel_disable_rps(struct drm_i915_private *dev_priv)
void intel_disable_gt_powersave(struct drm_i915_private *dev_priv)
{
- mutex_lock(&dev_priv->pcu_lock);
+ mutex_lock(&dev_priv->gt_pm.rps.lock);
intel_disable_rc6(dev_priv);
intel_disable_rps(dev_priv);
if (HAS_LLC(dev_priv))
intel_disable_llc_pstate(dev_priv);
- mutex_unlock(&dev_priv->pcu_lock);
+ mutex_unlock(&dev_priv->gt_pm.rps.lock);
}
static inline void intel_enable_llc_pstate(struct drm_i915_private *i915)
{
- lockdep_assert_held(&i915->pcu_lock);
+ lockdep_assert_held(&i915->gt_pm.rps.lock);
if (i915->gt_pm.llc_pstate.enabled)
return;
@@ -8063,7 +8046,7 @@ static inline void intel_enable_llc_pstate(struct drm_i915_private *i915)
static void intel_enable_rc6(struct drm_i915_private *dev_priv)
{
- lockdep_assert_held(&dev_priv->pcu_lock);
+ lockdep_assert_held(&dev_priv->gt_pm.rps.lock);
if (dev_priv->gt_pm.rc6.enabled)
return;
@@ -8086,7 +8069,7 @@ static void intel_enable_rps(struct drm_i915_private *dev_priv)
{
struct intel_rps *rps = &dev_priv->gt_pm.rps;
- lockdep_assert_held(&dev_priv->pcu_lock);
+ lockdep_assert_held(&rps->lock);
if (rps->enabled)
return;
@@ -8121,7 +8104,7 @@ void intel_enable_gt_powersave(struct drm_i915_private *dev_priv)
if (intel_vgpu_active(dev_priv))
return;
- mutex_lock(&dev_priv->pcu_lock);
+ mutex_lock(&dev_priv->gt_pm.rps.lock);
if (HAS_RC6(dev_priv))
intel_enable_rc6(dev_priv);
@@ -8129,7 +8112,7 @@ void intel_enable_gt_powersave(struct drm_i915_private *dev_priv)
if (HAS_LLC(dev_priv))
intel_enable_llc_pstate(dev_priv);
- mutex_unlock(&dev_priv->pcu_lock);
+ mutex_unlock(&dev_priv->gt_pm.rps.lock);
}
static void ibx_init_clock_gating(struct drm_i915_private *dev_priv)
@@ -9128,11 +9111,11 @@ static inline int gen7_check_mailbox_status(struct drm_i915_private *dev_priv)
}
}
-int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val)
+static int __sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val)
{
int status;
- WARN_ON(!mutex_is_locked(&dev_priv->pcu_lock));
+ lockdep_assert_held(&dev_priv->sb_lock);
/* GEN6_PCODE_* are outside of the forcewake domain, we can
* use te fw I915_READ variants to reduce the amount of work
@@ -9174,12 +9157,21 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
return 0;
}
-int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
- u32 mbox, u32 val)
+int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val)
{
int status;
- WARN_ON(!mutex_is_locked(&dev_priv->pcu_lock));
+ mutex_lock(&dev_priv->sb_lock);
+ status = __sandybridge_pcode_read(dev_priv, mbox, val);
+ mutex_unlock(&dev_priv->sb_lock);
+
+ return status;
+}
+
+static int __sandybridge_pcode_write(struct drm_i915_private *dev_priv,
+ u32 mbox, u32 val)
+{
+ int status;
/* GEN6_PCODE_* are outside of the forcewake domain, we can
* use te fw I915_READ variants to reduce the amount of work
@@ -9220,13 +9212,25 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
return 0;
}
+int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
+ u32 mbox, u32 val)
+{
+ int status;
+
+ mutex_lock(&dev_priv->sb_lock);
+ status = __sandybridge_pcode_write(dev_priv, mbox, val);
+ mutex_unlock(&dev_priv->sb_lock);
+
+ return status;
+}
+
static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox,
u32 request, u32 reply_mask, u32 reply,
u32 *status)
{
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);
}
@@ -9256,7 +9260,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)
@@ -9292,6 +9296,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
}
@@ -9361,8 +9366,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 c434a750e9a3..54aca7fd40ad 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -798,7 +798,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_sideband_get(dev_priv);
#define COND \
@@ -821,7 +820,6 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
out:
vlv_sideband_put(dev_priv);
- mutex_unlock(&dev_priv->pcu_lock);
}
static void vlv_power_well_enable(struct drm_i915_private *dev_priv,
@@ -848,7 +846,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_sideband_get(dev_priv);
state = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask;
@@ -869,7 +866,6 @@ static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv,
WARN_ON(ctrl != state);
vlv_sideband_put(dev_priv);
- mutex_unlock(&dev_priv->pcu_lock);
return enabled;
}
@@ -1381,7 +1377,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_sideband_get(dev_priv);
state = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) & DP_SSS_MASK(pipe);
@@ -1400,7 +1395,6 @@ static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
WARN_ON(ctrl << 16 != state);
vlv_sideband_put(dev_priv);
- mutex_unlock(&dev_priv->pcu_lock);
return enabled;
}
@@ -1415,7 +1409,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_sideband_get(dev_priv);
#define COND \
@@ -1438,7 +1431,6 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
out:
vlv_sideband_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 6d1674d479e0..7a58bd924fc3 100644
--- a/drivers/gpu/drm/i915/intel_sideband.c
+++ b/drivers/gpu/drm/i915/intel_sideband.c
@@ -118,8 +118,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);
@@ -128,8 +126,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);
}
--
2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 7/7] drm/i915: Merge sandybride_pcode_(read|write)
2018-01-10 12:55 Keep package awake during punit access Chris Wilson
` (5 preceding siblings ...)
2018-01-10 12:55 ` [PATCH 6/7] drm/i915: Replace pcu_lock with sb_lock Chris Wilson
@ 2018-01-10 12:55 ` Chris Wilson
2018-01-10 13:41 ` ✓ Fi.CI.BAT: success for series starting with [1/7] drm/i915: Disable preemption and sleeping while using the punit sideband Patchwork
7 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2018-01-10 12:55 UTC (permalink / raw)
To: intel-gfx
These routines are identical except in the nature of the value parameter.
For writes it is a pure in-param, but for a read, we need an out-param.
Since they differ in a single line, merge the two routines into one.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_pm.c | 63 +++++++----------------------------------
1 file changed, 10 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index cf8b4c28f1e1..c9636c684f65 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -9111,13 +9111,15 @@ static inline int gen7_check_mailbox_status(struct drm_i915_private *dev_priv)
}
}
-static int __sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val)
+static int __sandybridge_pcode_rw(struct drm_i915_private *dev_priv,
+ u32 mbox, u32 *val, bool is_read)
{
int status;
lockdep_assert_held(&dev_priv->sb_lock);
- /* GEN6_PCODE_* are outside of the forcewake domain, we can
+ /*
+ * 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.
*/
@@ -9140,7 +9142,8 @@ static int __sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox,
return -ETIMEDOUT;
}
- *val = I915_READ_FW(GEN6_PCODE_DATA);
+ if (is_read)
+ *val = I915_READ_FW(GEN6_PCODE_DATA);
I915_WRITE_FW(GEN6_PCODE_DATA, 0);
if (INTEL_GEN(dev_priv) > 6)
@@ -9162,63 +9165,19 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
int status;
mutex_lock(&dev_priv->sb_lock);
- status = __sandybridge_pcode_read(dev_priv, mbox, val);
+ status = __sandybridge_pcode_rw(dev_priv, mbox, val, true);
mutex_unlock(&dev_priv->sb_lock);
return status;
}
-static int __sandybridge_pcode_write(struct drm_i915_private *dev_priv,
- u32 mbox, u32 val)
-{
- int status;
-
- /* 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));
- return -EAGAIN;
- }
-
- I915_WRITE_FW(GEN6_PCODE_DATA, val);
- I915_WRITE_FW(GEN6_PCODE_DATA1, 0);
- I915_WRITE_FW(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
-
- if (__intel_wait_for_register_fw(dev_priv,
- GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
- 500, 0, 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));
- return -ETIMEDOUT;
- }
-
- I915_WRITE_FW(GEN6_PCODE_DATA, 0);
-
- if (INTEL_GEN(dev_priv) > 6)
- status = gen7_check_mailbox_status(dev_priv);
- else
- status = gen6_check_mailbox_status(dev_priv);
-
- 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;
-}
-
int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
u32 mbox, u32 val)
{
int status;
mutex_lock(&dev_priv->sb_lock);
- status = __sandybridge_pcode_write(dev_priv, mbox, val);
+ status = __sandybridge_pcode_rw(dev_priv, mbox, &val, false);
mutex_unlock(&dev_priv->sb_lock);
return status;
@@ -9228,11 +9187,9 @@ static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox,
u32 request, u32 reply_mask, u32 reply,
u32 *status)
{
- u32 val = request;
-
- *status = __sandybridge_pcode_read(dev_priv, mbox, &val);
+ *status = __sandybridge_pcode_rw(dev_priv, mbox, &request, true);
- return *status || ((val & reply_mask) == reply);
+ return *status || ((request & reply_mask) == reply);
}
/**
--
2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-10 12:55 ` [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband Chris Wilson
@ 2018-01-10 13:18 ` Hans de Goede
2018-01-10 13:45 ` Mika Kuoppala
2018-01-11 19:43 ` Hans de Goede
2018-01-11 20:10 ` Ville Syrjälä
1 sibling, 2 replies; 19+ messages in thread
From: Hans de Goede @ 2018-01-10 13:18 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Hi,
On 10-01-18 13:55, Chris Wilson wrote:
> While we talk to the punit over its sideband, we need to prevent the cpu
> from sleeping in order to prevent a potential machine hang.
>
> Note that by itself, it appears that pm_qos_update_request (via
> intel_idle) doesn't provide a sufficient barrier to ensure that all core
> are indeed awake (out of Cstate) and that the package is awake. To do so,
> we need to supplement the pm_qos with a manual ping on_each_cpu.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
> References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
> References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
Interesting, I've added similar pm_qos code in
drivers/i2c/busses/i2c-designware-baytrail.c quite a while ago because
the CPU transitioning to higher C-state while accessing the i2c bus to
the pmic (if it is shared) also causes the SoC to hang.
I could reproduce this quite easily by doing "i2cdump" on the pmic,
usually the system would hang in one or 2 i2cdump calls.
Note IIRC this was on CHT.
I see that you also block any pmic-i2c bus accesses while doing
punit access by calling iosf_mbi_punit_acquire();
Maybe we need to move the pm_qos stuff out of
drivers/i2c/busses/i2c-designware-baytrail.c
And into iosf_mbi_punit_acquire? The i2c-designware-baytrail.c
does its own pm_qos dance directly after calling
iosf_mbi_punit_acquire / before calling iosf_mbi_punit_release();
Note the i2c-designware-baytrail.c version lacks the ping, but it
should, probably have it too.
Regards,
Hans
> ---
> drivers/gpu/drm/i915/i915_drv.c | 6 ++++
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_sideband.c | 61 ++++++++++++++++++++++++-----------
> 3 files changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6c8da9d20c33..d4b90cc0130b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -902,6 +902,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> spin_lock_init(&dev_priv->uncore.lock);
>
> mutex_init(&dev_priv->sb_lock);
> + pm_qos_add_request(&dev_priv->sb_qos,
> + PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
> +
> mutex_init(&dev_priv->modeset_restore_lock);
> mutex_init(&dev_priv->av_mutex);
> mutex_init(&dev_priv->wm.wm_mutex);
> @@ -953,6 +956,9 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
> intel_irq_fini(dev_priv);
> i915_workqueues_cleanup(dev_priv);
> i915_engines_cleanup(dev_priv);
> +
> + pm_qos_remove_request(&dev_priv->sb_qos);
> + mutex_destroy(&dev_priv->sb_lock);
> }
>
> static int i915_mmio_setup(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a689396d0ff6..ff3f9effc0bb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1887,6 +1887,7 @@ struct drm_i915_private {
>
> /* Sideband mailbox protection */
> struct mutex sb_lock;
> + struct pm_qos_request sb_qos;
>
> /** Cached value of IMR to avoid reads in updating the bitfield */
> union {
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> index 75c872bb8cc9..02bdd2e2cef6 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.c
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -22,6 +22,8 @@
> *
> */
>
> +#include <asm/iosf_mbi.h>
> +
> #include "i915_drv.h"
> #include "intel_drv.h"
>
> @@ -39,18 +41,20 @@
> /* Private register write, double-word addressing, non-posted */
> #define SB_CRWRDA_NP 0x07
>
> -static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
> - u32 port, u32 opcode, u32 addr, u32 *val)
> +static void ping(void *info)
> {
> - u32 cmd, be = 0xf, bar = 0;
> - bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
> +}
>
> - cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
> - (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
> - (bar << IOSF_BAR_SHIFT);
> +static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
> + u32 devfn, u32 port, u32 opcode,
> + u32 addr, u32 *val)
> +{
> + const bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
> + int err;
>
> - WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
> + lockdep_assert_held(&dev_priv->sb_lock);
>
> + /* Flush the previous comms, just in case it failed last time. */
> if (intel_wait_for_register(dev_priv,
> VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
> 5)) {
> @@ -59,22 +63,43 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
> return -EAGAIN;
> }
>
> - I915_WRITE(VLV_IOSF_ADDR, addr);
> - I915_WRITE(VLV_IOSF_DATA, is_read ? 0 : *val);
> - I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
> + iosf_mbi_punit_acquire();
>
> - if (intel_wait_for_register(dev_priv,
> - VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
> - 5)) {
> + /*
> + * Prevent the cpu from sleeping while we use this sideband, otherwise
> + * the punit may cause a machine hang.
> + */
> + pm_qos_update_request(&dev_priv->sb_qos, 0);
> + on_each_cpu(ping, NULL, 1);
> + preempt_disable();
> +
> + I915_WRITE_FW(VLV_IOSF_ADDR, addr);
> + I915_WRITE_FW(VLV_IOSF_DATA, is_read ? 0 : *val);
> + I915_WRITE_FW(VLV_IOSF_DOORBELL_REQ,
> + (devfn << IOSF_DEVFN_SHIFT) |
> + (opcode << IOSF_OPCODE_SHIFT) |
> + (port << IOSF_PORT_SHIFT) |
> + (0xf << IOSF_BYTE_ENABLES_SHIFT) |
> + (0 << IOSF_BAR_SHIFT) |
> + IOSF_SB_BUSY);
> +
> + if (__intel_wait_for_register_fw(dev_priv,
> + VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
> + 10000, 0, NULL) == 0) {
> + if (is_read)
> + *val = I915_READ_FW(VLV_IOSF_DATA);
> + err = 0;
> + } else {
> DRM_DEBUG_DRIVER("IOSF sideband finish wait (%s) timed out\n",
> is_read ? "read" : "write");
> - return -ETIMEDOUT;
> + err = -ETIMEDOUT;
> }
>
> - if (is_read)
> - *val = I915_READ(VLV_IOSF_DATA);
> + preempt_enable();
> + pm_qos_update_request(&dev_priv->sb_qos, PM_QOS_DEFAULT_VALUE);
> + iosf_mbi_punit_release();
>
> - return 0;
> + return err;
> }
>
> u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-10 12:55 Keep package awake during punit access Chris Wilson
` (6 preceding siblings ...)
2018-01-10 12:55 ` [PATCH 7/7] drm/i915: Merge sandybride_pcode_(read|write) Chris Wilson
@ 2018-01-10 13:41 ` Patchwork
7 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2018-01-10 13:41 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
URL : https://patchwork.freedesktop.org/series/36275/
State : success
== Summary ==
Series 36275v1 series starting with [1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
https://patchwork.freedesktop.org/api/1.0/series/36275/revisions/1/mbox/
Test debugfs_test:
Subgroup read_all_entries:
pass -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
fail -> PASS (fi-gdg-551) fdo#102575
Test kms_frontbuffer_tracking:
Subgroup basic:
incomplete -> PASS (fi-bsw-n3050) fdo#104571
pass -> FAIL (fi-glk-1) fdo#103167
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
dmesg-warn -> PASS (fi-kbl-r) fdo#104172 +1
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#104571 https://bugs.freedesktop.org/show_bug.cgi?id=104571
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:426s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:426s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:372s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:492s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:280s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:481s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:489s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:473s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:458s
fi-elk-e7500 total:224 pass:168 dwarn:10 dfail:0 fail:0 skip:45
fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:276s
fi-glk-1 total:288 pass:259 dwarn:0 dfail:0 fail:1 skip:28 time:507s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:394s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:401s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:416s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:449s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:413s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:467s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:507s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:452s
fi-kbl-r total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:503s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:581s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:437s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:507s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:529s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:495s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:482s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:430s
fi-snb-2520m total:3 pass:2 dwarn:0 dfail:0 fail:0 skip:0
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:401s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:567s
fi-cnl-y2 total:19 pass:18 dwarn:0 dfail:0 fail:0 skip:0
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:469s
17fd16225c4df784ff06467c8bc528bee156a274 drm-tip: 2018y-01m-10d-09h-37m-51s UTC integration manifest
a0eac435cf3e drm/i915: Merge sandybride_pcode_(read|write)
21994513c8e4 drm/i915: Replace pcu_lock with sb_lock
eda3e5d41347 Revert "drm/i915: Avoid tweaking evaluation thresholds on Baytrail v3"
860edcaff331 drm/i915: Reduce RPS update frequency on Valleyview/Cherryview
de1ff6c12fe3 drm/i915: Lift sideband locking for vlv_punit_(read|write)
1c967bb4f34c drm/i915: Lift acquiring the vlv punit to the callers
b1cfc63c8642 drm/i915: Disable preemption and sleeping while using the punit sideband
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7643/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-10 13:18 ` Hans de Goede
@ 2018-01-10 13:45 ` Mika Kuoppala
2018-01-10 14:02 ` Chris Wilson
2018-01-11 19:43 ` Hans de Goede
1 sibling, 1 reply; 19+ messages in thread
From: Mika Kuoppala @ 2018-01-10 13:45 UTC (permalink / raw)
To: Hans de Goede, Chris Wilson, intel-gfx
Hans de Goede <hdegoede@redhat.com> writes:
> Hi,
>
> On 10-01-18 13:55, Chris Wilson wrote:
>> While we talk to the punit over its sideband, we need to prevent the cpu
>> from sleeping in order to prevent a potential machine hang.
>>
>> Note that by itself, it appears that pm_qos_update_request (via
>> intel_idle) doesn't provide a sufficient barrier to ensure that all core
>> are indeed awake (out of Cstate) and that the package is awake. To do so,
>> we need to supplement the pm_qos with a manual ping on_each_cpu.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
>> References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>
> Interesting, I've added similar pm_qos code in
> drivers/i2c/busses/i2c-designware-baytrail.c quite a while ago because
> the CPU transitioning to higher C-state while accessing the i2c bus to
> the pmic (if it is shared) also causes the SoC to hang.
>
> I could reproduce this quite easily by doing "i2cdump" on the pmic,
> usually the system would hang in one or 2 i2cdump calls.
>
> Note IIRC this was on CHT.
>
> I see that you also block any pmic-i2c bus accesses while doing
> punit access by calling iosf_mbi_punit_acquire();
>
> Maybe we need to move the pm_qos stuff out of
> drivers/i2c/busses/i2c-designware-baytrail.c
>
> And into iosf_mbi_punit_acquire? The i2c-designware-baytrail.c
> does its own pm_qos dance directly after calling
> iosf_mbi_punit_acquire / before calling iosf_mbi_punit_release();
>
> Note the i2c-designware-baytrail.c version lacks the ping, but it
> should, probably have it too.
The ping, if it deemed worthy, should find its way into intel_idle
parts.
-Mika
>
> Regards,
>
> Hans
>
>
>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 6 ++++
>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>> drivers/gpu/drm/i915/intel_sideband.c | 61 ++++++++++++++++++++++++-----------
>> 3 files changed, 50 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 6c8da9d20c33..d4b90cc0130b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -902,6 +902,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>> spin_lock_init(&dev_priv->uncore.lock);
>>
>> mutex_init(&dev_priv->sb_lock);
>> + pm_qos_add_request(&dev_priv->sb_qos,
>> + PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>> +
>> mutex_init(&dev_priv->modeset_restore_lock);
>> mutex_init(&dev_priv->av_mutex);
>> mutex_init(&dev_priv->wm.wm_mutex);
>> @@ -953,6 +956,9 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
>> intel_irq_fini(dev_priv);
>> i915_workqueues_cleanup(dev_priv);
>> i915_engines_cleanup(dev_priv);
>> +
>> + pm_qos_remove_request(&dev_priv->sb_qos);
>> + mutex_destroy(&dev_priv->sb_lock);
>> }
>>
>> static int i915_mmio_setup(struct drm_i915_private *dev_priv)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index a689396d0ff6..ff3f9effc0bb 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1887,6 +1887,7 @@ struct drm_i915_private {
>>
>> /* Sideband mailbox protection */
>> struct mutex sb_lock;
>> + struct pm_qos_request sb_qos;
>>
>> /** Cached value of IMR to avoid reads in updating the bitfield */
>> union {
>> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
>> index 75c872bb8cc9..02bdd2e2cef6 100644
>> --- a/drivers/gpu/drm/i915/intel_sideband.c
>> +++ b/drivers/gpu/drm/i915/intel_sideband.c
>> @@ -22,6 +22,8 @@
>> *
>> */
>>
>> +#include <asm/iosf_mbi.h>
>> +
>> #include "i915_drv.h"
>> #include "intel_drv.h"
>>
>> @@ -39,18 +41,20 @@
>> /* Private register write, double-word addressing, non-posted */
>> #define SB_CRWRDA_NP 0x07
>>
>> -static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
>> - u32 port, u32 opcode, u32 addr, u32 *val)
>> +static void ping(void *info)
>> {
>> - u32 cmd, be = 0xf, bar = 0;
>> - bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
>> +}
>>
>> - cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
>> - (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
>> - (bar << IOSF_BAR_SHIFT);
>> +static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
>> + u32 devfn, u32 port, u32 opcode,
>> + u32 addr, u32 *val)
>> +{
>> + const bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
>> + int err;
>>
>> - WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
>> + lockdep_assert_held(&dev_priv->sb_lock);
>>
>> + /* Flush the previous comms, just in case it failed last time. */
>> if (intel_wait_for_register(dev_priv,
>> VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>> 5)) {
>> @@ -59,22 +63,43 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
>> return -EAGAIN;
>> }
>>
>> - I915_WRITE(VLV_IOSF_ADDR, addr);
>> - I915_WRITE(VLV_IOSF_DATA, is_read ? 0 : *val);
>> - I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
>> + iosf_mbi_punit_acquire();
>>
>> - if (intel_wait_for_register(dev_priv,
>> - VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>> - 5)) {
>> + /*
>> + * Prevent the cpu from sleeping while we use this sideband, otherwise
>> + * the punit may cause a machine hang.
>> + */
>> + pm_qos_update_request(&dev_priv->sb_qos, 0);
>> + on_each_cpu(ping, NULL, 1);
>> + preempt_disable();
>> +
>> + I915_WRITE_FW(VLV_IOSF_ADDR, addr);
>> + I915_WRITE_FW(VLV_IOSF_DATA, is_read ? 0 : *val);
>> + I915_WRITE_FW(VLV_IOSF_DOORBELL_REQ,
>> + (devfn << IOSF_DEVFN_SHIFT) |
>> + (opcode << IOSF_OPCODE_SHIFT) |
>> + (port << IOSF_PORT_SHIFT) |
>> + (0xf << IOSF_BYTE_ENABLES_SHIFT) |
>> + (0 << IOSF_BAR_SHIFT) |
>> + IOSF_SB_BUSY);
>> +
>> + if (__intel_wait_for_register_fw(dev_priv,
>> + VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>> + 10000, 0, NULL) == 0) {
>> + if (is_read)
>> + *val = I915_READ_FW(VLV_IOSF_DATA);
>> + err = 0;
>> + } else {
>> DRM_DEBUG_DRIVER("IOSF sideband finish wait (%s) timed out\n",
>> is_read ? "read" : "write");
>> - return -ETIMEDOUT;
>> + err = -ETIMEDOUT;
>> }
>>
>> - if (is_read)
>> - *val = I915_READ(VLV_IOSF_DATA);
>> + preempt_enable();
>> + pm_qos_update_request(&dev_priv->sb_qos, PM_QOS_DEFAULT_VALUE);
>> + iosf_mbi_punit_release();
>>
>> - return 0;
>> + return err;
>> }
>>
>> u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-10 13:45 ` Mika Kuoppala
@ 2018-01-10 14:02 ` Chris Wilson
0 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2018-01-10 14:02 UTC (permalink / raw)
To: Mika Kuoppala, Hans de Goede, intel-gfx
Quoting Mika Kuoppala (2018-01-10 13:45:27)
> Hans de Goede <hdegoede@redhat.com> writes:
>
> > Hi,
> >
> > On 10-01-18 13:55, Chris Wilson wrote:
> >> While we talk to the punit over its sideband, we need to prevent the cpu
> >> from sleeping in order to prevent a potential machine hang.
> >>
> >> Note that by itself, it appears that pm_qos_update_request (via
> >> intel_idle) doesn't provide a sufficient barrier to ensure that all core
> >> are indeed awake (out of Cstate) and that the package is awake. To do so,
> >> we need to supplement the pm_qos with a manual ping on_each_cpu.
> >>
> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
> >> References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >> Cc: Hans de Goede <hdegoede@redhat.com>
> >
> > Interesting, I've added similar pm_qos code in
> > drivers/i2c/busses/i2c-designware-baytrail.c quite a while ago because
> > the CPU transitioning to higher C-state while accessing the i2c bus to
> > the pmic (if it is shared) also causes the SoC to hang.
> >
> > I could reproduce this quite easily by doing "i2cdump" on the pmic,
> > usually the system would hang in one or 2 i2cdump calls.
> >
> > Note IIRC this was on CHT.
> >
> > I see that you also block any pmic-i2c bus accesses while doing
> > punit access by calling iosf_mbi_punit_acquire();
> >
> > Maybe we need to move the pm_qos stuff out of
> > drivers/i2c/busses/i2c-designware-baytrail.c
> >
> > And into iosf_mbi_punit_acquire? The i2c-designware-baytrail.c
> > does its own pm_qos dance directly after calling
> > iosf_mbi_punit_acquire / before calling iosf_mbi_punit_release();
> >
> > Note the i2c-designware-baytrail.c version lacks the ping, but it
> > should, probably have it too.
>
> The ping, if it deemed worthy, should find its way into intel_idle
> parts.
It's definitely the missing piece in the puzzle. Without the
on_each_cpu(ping), the machine hangs remain a reoccuring nightmare.
However, the max test runtime so far with these patches has been 12h on
one j1900, not enough to be conclusive.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-10 13:18 ` Hans de Goede
2018-01-10 13:45 ` Mika Kuoppala
@ 2018-01-11 19:43 ` Hans de Goede
1 sibling, 0 replies; 19+ messages in thread
From: Hans de Goede @ 2018-01-11 19:43 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Hi,
On 10-01-18 14:18, Hans de Goede wrote:
> Hi,
>
> On 10-01-18 13:55, Chris Wilson wrote:
>> While we talk to the punit over its sideband, we need to prevent the cpu
>> from sleeping in order to prevent a potential machine hang.
>>
>> Note that by itself, it appears that pm_qos_update_request (via
>> intel_idle) doesn't provide a sufficient barrier to ensure that all core
>> are indeed awake (out of Cstate) and that the package is awake. To do so,
>> we need to supplement the pm_qos with a manual ping on_each_cpu.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
>> References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>
> Interesting, I've added similar pm_qos code in
> drivers/i2c/busses/i2c-designware-baytrail.c quite a while ago because
> the CPU transitioning to higher C-state while accessing the i2c bus to
> the pmic (if it is shared) also causes the SoC to hang.
>
> I could reproduce this quite easily by doing "i2cdump" on the pmic,
> usually the system would hang in one or 2 i2cdump calls.
>
> Note IIRC this was on CHT.
>
> I see that you also block any pmic-i2c bus accesses while doing
> punit access by calling iosf_mbi_punit_acquire();
>
> Maybe we need to move the pm_qos stuff out of
> drivers/i2c/busses/i2c-designware-baytrail.c
>
> And into iosf_mbi_punit_acquire? The i2c-designware-baytrail.c
> does its own pm_qos dance directly after calling
> iosf_mbi_punit_acquire / before calling iosf_mbi_punit_release();
I just remembered that the i915 code also call iosf_mbi_punit_acquire()
from drivers/gpu/drm/i915/intel_uncore.c and does it for non VLV/CHT
boards there too. Currently that is not an issue because
iosf_mbi_punit_acquire() currently is a nop for non VLV/CHT, but if
we move the pm_qos stuff there then it no longer is a nop, so it is
probably best to keep separate pm_qos code in intel_sideband.c and
i2c-designware-baytrail.c, iow move ahead with this patch as as,
except for maybe moving the ping stuff to the intel_idle.c code.
Regards,
Hans
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 6 ++++
>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>> drivers/gpu/drm/i915/intel_sideband.c | 61 ++++++++++++++++++++++++-----------
>> 3 files changed, 50 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 6c8da9d20c33..d4b90cc0130b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -902,6 +902,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>> spin_lock_init(&dev_priv->uncore.lock);
>> mutex_init(&dev_priv->sb_lock);
>> + pm_qos_add_request(&dev_priv->sb_qos,
>> + PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>> +
>> mutex_init(&dev_priv->modeset_restore_lock);
>> mutex_init(&dev_priv->av_mutex);
>> mutex_init(&dev_priv->wm.wm_mutex);
>> @@ -953,6 +956,9 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
>> intel_irq_fini(dev_priv);
>> i915_workqueues_cleanup(dev_priv);
>> i915_engines_cleanup(dev_priv);
>> +
>> + pm_qos_remove_request(&dev_priv->sb_qos);
>> + mutex_destroy(&dev_priv->sb_lock);
>> }
>> static int i915_mmio_setup(struct drm_i915_private *dev_priv)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index a689396d0ff6..ff3f9effc0bb 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1887,6 +1887,7 @@ struct drm_i915_private {
>> /* Sideband mailbox protection */
>> struct mutex sb_lock;
>> + struct pm_qos_request sb_qos;
>> /** Cached value of IMR to avoid reads in updating the bitfield */
>> union {
>> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
>> index 75c872bb8cc9..02bdd2e2cef6 100644
>> --- a/drivers/gpu/drm/i915/intel_sideband.c
>> +++ b/drivers/gpu/drm/i915/intel_sideband.c
>> @@ -22,6 +22,8 @@
>> *
>> */
>> +#include <asm/iosf_mbi.h>
>> +
>> #include "i915_drv.h"
>> #include "intel_drv.h"
>> @@ -39,18 +41,20 @@
>> /* Private register write, double-word addressing, non-posted */
>> #define SB_CRWRDA_NP 0x07
>> -static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
>> - u32 port, u32 opcode, u32 addr, u32 *val)
>> +static void ping(void *info)
>> {
>> - u32 cmd, be = 0xf, bar = 0;
>> - bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
>> +}
>> - cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
>> - (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
>> - (bar << IOSF_BAR_SHIFT);
>> +static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
>> + u32 devfn, u32 port, u32 opcode,
>> + u32 addr, u32 *val)
>> +{
>> + const bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
>> + int err;
>> - WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
>> + lockdep_assert_held(&dev_priv->sb_lock);
>> + /* Flush the previous comms, just in case it failed last time. */
>> if (intel_wait_for_register(dev_priv,
>> VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>> 5)) {
>> @@ -59,22 +63,43 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
>> return -EAGAIN;
>> }
>> - I915_WRITE(VLV_IOSF_ADDR, addr);
>> - I915_WRITE(VLV_IOSF_DATA, is_read ? 0 : *val);
>> - I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
>> + iosf_mbi_punit_acquire();
>> - if (intel_wait_for_register(dev_priv,
>> - VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>> - 5)) {
>> + /*
>> + * Prevent the cpu from sleeping while we use this sideband, otherwise
>> + * the punit may cause a machine hang.
>> + */
>> + pm_qos_update_request(&dev_priv->sb_qos, 0);
>> + on_each_cpu(ping, NULL, 1);
>> + preempt_disable();
>> +
>> + I915_WRITE_FW(VLV_IOSF_ADDR, addr);
>> + I915_WRITE_FW(VLV_IOSF_DATA, is_read ? 0 : *val);
>> + I915_WRITE_FW(VLV_IOSF_DOORBELL_REQ,
>> + (devfn << IOSF_DEVFN_SHIFT) |
>> + (opcode << IOSF_OPCODE_SHIFT) |
>> + (port << IOSF_PORT_SHIFT) |
>> + (0xf << IOSF_BYTE_ENABLES_SHIFT) |
>> + (0 << IOSF_BAR_SHIFT) |
>> + IOSF_SB_BUSY);
>> +
>> + if (__intel_wait_for_register_fw(dev_priv,
>> + VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>> + 10000, 0, NULL) == 0) {
>> + if (is_read)
>> + *val = I915_READ_FW(VLV_IOSF_DATA);
>> + err = 0;
>> + } else {
>> DRM_DEBUG_DRIVER("IOSF sideband finish wait (%s) timed out\n",
>> is_read ? "read" : "write");
>> - return -ETIMEDOUT;
>> + err = -ETIMEDOUT;
>> }
>> - if (is_read)
>> - *val = I915_READ(VLV_IOSF_DATA);
>> + preempt_enable();
>> + pm_qos_update_request(&dev_priv->sb_qos, PM_QOS_DEFAULT_VALUE);
>> + iosf_mbi_punit_release();
>> - return 0;
>> + return err;
>> }
>> u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-10 12:55 ` [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband Chris Wilson
2018-01-10 13:18 ` Hans de Goede
@ 2018-01-11 20:10 ` Ville Syrjälä
2018-01-11 20:53 ` Chris Wilson
1 sibling, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2018-01-11 20:10 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, Hans de Goede
On Wed, Jan 10, 2018 at 12:55:05PM +0000, Chris Wilson wrote:
> While we talk to the punit over its sideband, we need to prevent the cpu
> from sleeping in order to prevent a potential machine hang.
>
> Note that by itself, it appears that pm_qos_update_request (via
> intel_idle) doesn't provide a sufficient barrier to ensure that all core
> are indeed awake (out of Cstate) and that the package is awake. To do so,
> we need to supplement the pm_qos with a manual ping on_each_cpu.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
> References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
> References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 6 ++++
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_sideband.c | 61 ++++++++++++++++++++++++-----------
> 3 files changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6c8da9d20c33..d4b90cc0130b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -902,6 +902,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> spin_lock_init(&dev_priv->uncore.lock);
>
> mutex_init(&dev_priv->sb_lock);
> + pm_qos_add_request(&dev_priv->sb_qos,
> + PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
> +
> mutex_init(&dev_priv->modeset_restore_lock);
> mutex_init(&dev_priv->av_mutex);
> mutex_init(&dev_priv->wm.wm_mutex);
> @@ -953,6 +956,9 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
> intel_irq_fini(dev_priv);
> i915_workqueues_cleanup(dev_priv);
> i915_engines_cleanup(dev_priv);
> +
> + pm_qos_remove_request(&dev_priv->sb_qos);
> + mutex_destroy(&dev_priv->sb_lock);
> }
>
> static int i915_mmio_setup(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a689396d0ff6..ff3f9effc0bb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1887,6 +1887,7 @@ struct drm_i915_private {
>
> /* Sideband mailbox protection */
> struct mutex sb_lock;
> + struct pm_qos_request sb_qos;
>
> /** Cached value of IMR to avoid reads in updating the bitfield */
> union {
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> index 75c872bb8cc9..02bdd2e2cef6 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.c
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -22,6 +22,8 @@
> *
> */
>
> +#include <asm/iosf_mbi.h>
> +
> #include "i915_drv.h"
> #include "intel_drv.h"
>
> @@ -39,18 +41,20 @@
> /* Private register write, double-word addressing, non-posted */
> #define SB_CRWRDA_NP 0x07
>
> -static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
> - u32 port, u32 opcode, u32 addr, u32 *val)
> +static void ping(void *info)
> {
> - u32 cmd, be = 0xf, bar = 0;
> - bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
> +}
>
> - cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
> - (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
> - (bar << IOSF_BAR_SHIFT);
> +static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
> + u32 devfn, u32 port, u32 opcode,
> + u32 addr, u32 *val)
> +{
> + const bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
> + int err;
>
> - WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
> + lockdep_assert_held(&dev_priv->sb_lock);
>
> + /* Flush the previous comms, just in case it failed last time. */
> if (intel_wait_for_register(dev_priv,
> VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
> 5)) {
> @@ -59,22 +63,43 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
> return -EAGAIN;
> }
>
> - I915_WRITE(VLV_IOSF_ADDR, addr);
> - I915_WRITE(VLV_IOSF_DATA, is_read ? 0 : *val);
> - I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
> + iosf_mbi_punit_acquire();
>
> - if (intel_wait_for_register(dev_priv,
> - VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
> - 5)) {
> + /*
> + * Prevent the cpu from sleeping while we use this sideband, otherwise
> + * the punit may cause a machine hang.
> + */
> + pm_qos_update_request(&dev_priv->sb_qos, 0);
> + on_each_cpu(ping, NULL, 1);
pm_qos_update_request() doesn't wake up the cpus on its own? I wonder
kind of latency guarantees it can actually give without doing that.
Shouldn't we check if we're actually be talking to the punit and not
some other unit before we do all this extra work?
> + preempt_disable();
> +
> + I915_WRITE_FW(VLV_IOSF_ADDR, addr);
> + I915_WRITE_FW(VLV_IOSF_DATA, is_read ? 0 : *val);
> + I915_WRITE_FW(VLV_IOSF_DOORBELL_REQ,
> + (devfn << IOSF_DEVFN_SHIFT) |
> + (opcode << IOSF_OPCODE_SHIFT) |
> + (port << IOSF_PORT_SHIFT) |
> + (0xf << IOSF_BYTE_ENABLES_SHIFT) |
> + (0 << IOSF_BAR_SHIFT) |
> + IOSF_SB_BUSY);
> +
> + if (__intel_wait_for_register_fw(dev_priv,
> + VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
> + 10000, 0, NULL) == 0) {
> + if (is_read)
> + *val = I915_READ_FW(VLV_IOSF_DATA);
> + err = 0;
> + } else {
> DRM_DEBUG_DRIVER("IOSF sideband finish wait (%s) timed out\n",
> is_read ? "read" : "write");
> - return -ETIMEDOUT;
> + err = -ETIMEDOUT;
> }
>
> - if (is_read)
> - *val = I915_READ(VLV_IOSF_DATA);
> + preempt_enable();
> + pm_qos_update_request(&dev_priv->sb_qos, PM_QOS_DEFAULT_VALUE);
> + iosf_mbi_punit_release();
>
> - return 0;
> + return err;
> }
>
> u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
> --
> 2.15.1
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-11 20:10 ` Ville Syrjälä
@ 2018-01-11 20:53 ` Chris Wilson
2018-01-11 21:17 ` Ville Syrjälä
0 siblings, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2018-01-11 20:53 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Hans de Goede
Quoting Ville Syrjälä (2018-01-11 20:10:45)
> On Wed, Jan 10, 2018 at 12:55:05PM +0000, Chris Wilson wrote:
> > While we talk to the punit over its sideband, we need to prevent the cpu
> > from sleeping in order to prevent a potential machine hang.
> >
> > Note that by itself, it appears that pm_qos_update_request (via
> > intel_idle) doesn't provide a sufficient barrier to ensure that all core
> > are indeed awake (out of Cstate) and that the package is awake. To do so,
> > we need to supplement the pm_qos with a manual ping on_each_cpu.
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
> > References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 6 ++++
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_sideband.c | 61 ++++++++++++++++++++++++-----------
> > 3 files changed, 50 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 6c8da9d20c33..d4b90cc0130b 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -902,6 +902,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> > spin_lock_init(&dev_priv->uncore.lock);
> >
> > mutex_init(&dev_priv->sb_lock);
> > + pm_qos_add_request(&dev_priv->sb_qos,
> > + PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
> > +
> > mutex_init(&dev_priv->modeset_restore_lock);
> > mutex_init(&dev_priv->av_mutex);
> > mutex_init(&dev_priv->wm.wm_mutex);
> > @@ -953,6 +956,9 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
> > intel_irq_fini(dev_priv);
> > i915_workqueues_cleanup(dev_priv);
> > i915_engines_cleanup(dev_priv);
> > +
> > + pm_qos_remove_request(&dev_priv->sb_qos);
> > + mutex_destroy(&dev_priv->sb_lock);
> > }
> >
> > static int i915_mmio_setup(struct drm_i915_private *dev_priv)
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index a689396d0ff6..ff3f9effc0bb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1887,6 +1887,7 @@ struct drm_i915_private {
> >
> > /* Sideband mailbox protection */
> > struct mutex sb_lock;
> > + struct pm_qos_request sb_qos;
> >
> > /** Cached value of IMR to avoid reads in updating the bitfield */
> > union {
> > diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> > index 75c872bb8cc9..02bdd2e2cef6 100644
> > --- a/drivers/gpu/drm/i915/intel_sideband.c
> > +++ b/drivers/gpu/drm/i915/intel_sideband.c
> > @@ -22,6 +22,8 @@
> > *
> > */
> >
> > +#include <asm/iosf_mbi.h>
> > +
> > #include "i915_drv.h"
> > #include "intel_drv.h"
> >
> > @@ -39,18 +41,20 @@
> > /* Private register write, double-word addressing, non-posted */
> > #define SB_CRWRDA_NP 0x07
> >
> > -static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
> > - u32 port, u32 opcode, u32 addr, u32 *val)
> > +static void ping(void *info)
> > {
> > - u32 cmd, be = 0xf, bar = 0;
> > - bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
> > +}
> >
> > - cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
> > - (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
> > - (bar << IOSF_BAR_SHIFT);
> > +static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
> > + u32 devfn, u32 port, u32 opcode,
> > + u32 addr, u32 *val)
> > +{
> > + const bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
> > + int err;
> >
> > - WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
> > + lockdep_assert_held(&dev_priv->sb_lock);
> >
> > + /* Flush the previous comms, just in case it failed last time. */
> > if (intel_wait_for_register(dev_priv,
> > VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
> > 5)) {
> > @@ -59,22 +63,43 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
> > return -EAGAIN;
> > }
> >
> > - I915_WRITE(VLV_IOSF_ADDR, addr);
> > - I915_WRITE(VLV_IOSF_DATA, is_read ? 0 : *val);
> > - I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
> > + iosf_mbi_punit_acquire();
> >
> > - if (intel_wait_for_register(dev_priv,
> > - VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
> > - 5)) {
> > + /*
> > + * Prevent the cpu from sleeping while we use this sideband, otherwise
> > + * the punit may cause a machine hang.
> > + */
> > + pm_qos_update_request(&dev_priv->sb_qos, 0);
> > + on_each_cpu(ping, NULL, 1);
>
> pm_qos_update_request() doesn't wake up the cpus on its own? I wonder
> kind of latency guarantees it can actually give without doing that.
intel_idle plugs into the update to wake up idle cpus, but it is not
synchronous. If we don't have the on_each_cpu() here, the hangs reoccur;
the easiest answer seems to be that the on_each_cpu() is causing a
schedule of the RT migration thread which is enough to ensure that the
wakeup has occurred.
> Shouldn't we check if we're actually be talking to the punit and not
> some other unit before we do all this extra work?
Checking port? I am suspicious of the whole iosf mechanism...
Also debating whether to only apply it to j1900 or all. Is it just pure
fluke that we can easily reproduce it on the quad core Baytrail as
opposed to the dual core vlv or chv?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-11 20:53 ` Chris Wilson
@ 2018-01-11 21:17 ` Ville Syrjälä
2018-01-11 21:42 ` Hans de Goede
0 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2018-01-11 21:17 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, Hans de Goede
On Thu, Jan 11, 2018 at 08:53:42PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjälä (2018-01-11 20:10:45)
> > On Wed, Jan 10, 2018 at 12:55:05PM +0000, Chris Wilson wrote:
> > > While we talk to the punit over its sideband, we need to prevent the cpu
> > > from sleeping in order to prevent a potential machine hang.
> > >
> > > Note that by itself, it appears that pm_qos_update_request (via
> > > intel_idle) doesn't provide a sufficient barrier to ensure that all core
> > > are indeed awake (out of Cstate) and that the package is awake. To do so,
> > > we need to supplement the pm_qos with a manual ping on_each_cpu.
> > >
> > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
> > > References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.c | 6 ++++
> > > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > > drivers/gpu/drm/i915/intel_sideband.c | 61 ++++++++++++++++++++++++-----------
> > > 3 files changed, 50 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 6c8da9d20c33..d4b90cc0130b 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -902,6 +902,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> > > spin_lock_init(&dev_priv->uncore.lock);
> > >
> > > mutex_init(&dev_priv->sb_lock);
> > > + pm_qos_add_request(&dev_priv->sb_qos,
> > > + PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
> > > +
> > > mutex_init(&dev_priv->modeset_restore_lock);
> > > mutex_init(&dev_priv->av_mutex);
> > > mutex_init(&dev_priv->wm.wm_mutex);
> > > @@ -953,6 +956,9 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
> > > intel_irq_fini(dev_priv);
> > > i915_workqueues_cleanup(dev_priv);
> > > i915_engines_cleanup(dev_priv);
> > > +
> > > + pm_qos_remove_request(&dev_priv->sb_qos);
> > > + mutex_destroy(&dev_priv->sb_lock);
> > > }
> > >
> > > static int i915_mmio_setup(struct drm_i915_private *dev_priv)
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index a689396d0ff6..ff3f9effc0bb 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1887,6 +1887,7 @@ struct drm_i915_private {
> > >
> > > /* Sideband mailbox protection */
> > > struct mutex sb_lock;
> > > + struct pm_qos_request sb_qos;
> > >
> > > /** Cached value of IMR to avoid reads in updating the bitfield */
> > > union {
> > > diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> > > index 75c872bb8cc9..02bdd2e2cef6 100644
> > > --- a/drivers/gpu/drm/i915/intel_sideband.c
> > > +++ b/drivers/gpu/drm/i915/intel_sideband.c
> > > @@ -22,6 +22,8 @@
> > > *
> > > */
> > >
> > > +#include <asm/iosf_mbi.h>
> > > +
> > > #include "i915_drv.h"
> > > #include "intel_drv.h"
> > >
> > > @@ -39,18 +41,20 @@
> > > /* Private register write, double-word addressing, non-posted */
> > > #define SB_CRWRDA_NP 0x07
> > >
> > > -static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
> > > - u32 port, u32 opcode, u32 addr, u32 *val)
> > > +static void ping(void *info)
> > > {
> > > - u32 cmd, be = 0xf, bar = 0;
> > > - bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
> > > +}
> > >
> > > - cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
> > > - (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
> > > - (bar << IOSF_BAR_SHIFT);
> > > +static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
> > > + u32 devfn, u32 port, u32 opcode,
> > > + u32 addr, u32 *val)
> > > +{
> > > + const bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
> > > + int err;
> > >
> > > - WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
> > > + lockdep_assert_held(&dev_priv->sb_lock);
> > >
> > > + /* Flush the previous comms, just in case it failed last time. */
> > > if (intel_wait_for_register(dev_priv,
> > > VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
> > > 5)) {
> > > @@ -59,22 +63,43 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
> > > return -EAGAIN;
> > > }
> > >
> > > - I915_WRITE(VLV_IOSF_ADDR, addr);
> > > - I915_WRITE(VLV_IOSF_DATA, is_read ? 0 : *val);
> > > - I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
> > > + iosf_mbi_punit_acquire();
> > >
> > > - if (intel_wait_for_register(dev_priv,
> > > - VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
> > > - 5)) {
> > > + /*
> > > + * Prevent the cpu from sleeping while we use this sideband, otherwise
> > > + * the punit may cause a machine hang.
> > > + */
> > > + pm_qos_update_request(&dev_priv->sb_qos, 0);
> > > + on_each_cpu(ping, NULL, 1);
> >
> > pm_qos_update_request() doesn't wake up the cpus on its own? I wonder
> > kind of latency guarantees it can actually give without doing that.
>
> intel_idle plugs into the update to wake up idle cpus, but it is not
> synchronous. If we don't have the on_each_cpu() here, the hangs reoccur;
> the easiest answer seems to be that the on_each_cpu() is causing a
> schedule of the RT migration thread which is enough to ensure that the
> wakeup has occurred.
>
> > Shouldn't we check if we're actually be talking to the punit and not
> > some other unit before we do all this extra work?
>
> Checking port? I am suspicious of the whole iosf mechanism...
I doubt iosf sb itself is busted. That would probably make the entire
soc pretty much dead. I suspect the problem is either in the punit
firmware, or something fishy is going on with power delivery and
reducing the rate at which we change the voltages and whatnot helps
keep things a bit more stable.
So yeah, I would limit this to punit only by checking the port.
>
> Also debating whether to only apply it to j1900 or all. Is it just pure
> fluke that we can easily reproduce it on the quad core Baytrail as
> opposed to the dual core vlv or chv?
Not sure. I guess if the problem is around power delivery it might even
be board specific to some degree.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-11 21:17 ` Ville Syrjälä
@ 2018-01-11 21:42 ` Hans de Goede
2018-01-11 22:04 ` Hans de Goede
0 siblings, 1 reply; 19+ messages in thread
From: Hans de Goede @ 2018-01-11 21:42 UTC (permalink / raw)
To: Ville Syrjälä, Chris Wilson; +Cc: intel-gfx
Hi,
On 11-01-18 22:17, Ville Syrjälä wrote:
> On Thu, Jan 11, 2018 at 08:53:42PM +0000, Chris Wilson wrote:
>> Quoting Ville Syrjälä (2018-01-11 20:10:45)
>>> On Wed, Jan 10, 2018 at 12:55:05PM +0000, Chris Wilson wrote:
>>>> While we talk to the punit over its sideband, we need to prevent the cpu
>>>> from sleeping in order to prevent a potential machine hang.
>>>>
>>>> Note that by itself, it appears that pm_qos_update_request (via
>>>> intel_idle) doesn't provide a sufficient barrier to ensure that all core
>>>> are indeed awake (out of Cstate) and that the package is awake. To do so,
>>>> we need to supplement the pm_qos with a manual ping on_each_cpu.
>>>>
>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
>>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
>>>> References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_drv.c | 6 ++++
>>>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>>>> drivers/gpu/drm/i915/intel_sideband.c | 61 ++++++++++++++++++++++++-----------
>>>> 3 files changed, 50 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>>> index 6c8da9d20c33..d4b90cc0130b 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -902,6 +902,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>>>> spin_lock_init(&dev_priv->uncore.lock);
>>>>
>>>> mutex_init(&dev_priv->sb_lock);
>>>> + pm_qos_add_request(&dev_priv->sb_qos,
>>>> + PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>>>> +
>>>> mutex_init(&dev_priv->modeset_restore_lock);
>>>> mutex_init(&dev_priv->av_mutex);
>>>> mutex_init(&dev_priv->wm.wm_mutex);
>>>> @@ -953,6 +956,9 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
>>>> intel_irq_fini(dev_priv);
>>>> i915_workqueues_cleanup(dev_priv);
>>>> i915_engines_cleanup(dev_priv);
>>>> +
>>>> + pm_qos_remove_request(&dev_priv->sb_qos);
>>>> + mutex_destroy(&dev_priv->sb_lock);
>>>> }
>>>>
>>>> static int i915_mmio_setup(struct drm_i915_private *dev_priv)
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index a689396d0ff6..ff3f9effc0bb 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1887,6 +1887,7 @@ struct drm_i915_private {
>>>>
>>>> /* Sideband mailbox protection */
>>>> struct mutex sb_lock;
>>>> + struct pm_qos_request sb_qos;
>>>>
>>>> /** Cached value of IMR to avoid reads in updating the bitfield */
>>>> union {
>>>> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
>>>> index 75c872bb8cc9..02bdd2e2cef6 100644
>>>> --- a/drivers/gpu/drm/i915/intel_sideband.c
>>>> +++ b/drivers/gpu/drm/i915/intel_sideband.c
>>>> @@ -22,6 +22,8 @@
>>>> *
>>>> */
>>>>
>>>> +#include <asm/iosf_mbi.h>
>>>> +
>>>> #include "i915_drv.h"
>>>> #include "intel_drv.h"
>>>>
>>>> @@ -39,18 +41,20 @@
>>>> /* Private register write, double-word addressing, non-posted */
>>>> #define SB_CRWRDA_NP 0x07
>>>>
>>>> -static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
>>>> - u32 port, u32 opcode, u32 addr, u32 *val)
>>>> +static void ping(void *info)
>>>> {
>>>> - u32 cmd, be = 0xf, bar = 0;
>>>> - bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
>>>> +}
>>>>
>>>> - cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
>>>> - (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
>>>> - (bar << IOSF_BAR_SHIFT);
>>>> +static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
>>>> + u32 devfn, u32 port, u32 opcode,
>>>> + u32 addr, u32 *val)
>>>> +{
>>>> + const bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
>>>> + int err;
>>>>
>>>> - WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
>>>> + lockdep_assert_held(&dev_priv->sb_lock);
>>>>
>>>> + /* Flush the previous comms, just in case it failed last time. */
>>>> if (intel_wait_for_register(dev_priv,
>>>> VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>>>> 5)) {
>>>> @@ -59,22 +63,43 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
>>>> return -EAGAIN;
>>>> }
>>>>
>>>> - I915_WRITE(VLV_IOSF_ADDR, addr);
>>>> - I915_WRITE(VLV_IOSF_DATA, is_read ? 0 : *val);
>>>> - I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
>>>> + iosf_mbi_punit_acquire();
>>>>
>>>> - if (intel_wait_for_register(dev_priv,
>>>> - VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>>>> - 5)) {
>>>> + /*
>>>> + * Prevent the cpu from sleeping while we use this sideband, otherwise
>>>> + * the punit may cause a machine hang.
>>>> + */
>>>> + pm_qos_update_request(&dev_priv->sb_qos, 0);
>>>> + on_each_cpu(ping, NULL, 1);
>>>
>>> pm_qos_update_request() doesn't wake up the cpus on its own? I wonder
>>> kind of latency guarantees it can actually give without doing that.
>>
>> intel_idle plugs into the update to wake up idle cpus, but it is not
>> synchronous. If we don't have the on_each_cpu() here, the hangs reoccur;
>> the easiest answer seems to be that the on_each_cpu() is causing a
>> schedule of the RT migration thread which is enough to ensure that the
>> wakeup has occurred.
>>
>>> Shouldn't we check if we're actually be talking to the punit and not
>>> some other unit before we do all this extra work?
>>
>> Checking port? I am suspicious of the whole iosf mechanism...
>
> I doubt iosf sb itself is busted. That would probably make the entire
> soc pretty much dead. I suspect the problem is either in the punit
> firmware, or something fishy is going on with power delivery and
> reducing the rate at which we change the voltages and whatnot helps
> keep things a bit more stable.
>
> So yeah, I would limit this to punit only by checking the port.
>
>>
>> Also debating whether to only apply it to j1900 or all. Is it just pure
>> fluke that we can easily reproduce it on the quad core Baytrail as
>> opposed to the dual core vlv or chv?
>
> Not sure. I guess if the problem is around power delivery it might even
> be board specific to some degree.
The wordpress (on azure.com) link here:
https://bugzilla.kernel.org/show_bug.cgi?id=109051#c860
Certainly seems to hint at a power delivery problem (with the SoC not
with specific boards) and all models reported as needing
intel_idle.max_cstate=1 there are quad-core Bay Trails.
I guess it the problem happens on all quad-core Bay Trail variants when
the GPU and CPU both increase there power-demand (by up-clocking, resp.
leaving C6) at the same time and Chris' fix makes sure all CPU cores
leave C6 before doing the GPU up-clocking avoiding this.
Note this is all speculation / educated guessing.
Regards,
Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-11 21:42 ` Hans de Goede
@ 2018-01-11 22:04 ` Hans de Goede
2018-01-12 10:02 ` Mika Kuoppala
0 siblings, 1 reply; 19+ messages in thread
From: Hans de Goede @ 2018-01-11 22:04 UTC (permalink / raw)
To: Ville Syrjälä, Chris Wilson; +Cc: intel-gfx
Hi,
On 11-01-18 22:42, Hans de Goede wrote:
> Hi,
>
> On 11-01-18 22:17, Ville Syrjälä wrote:
>> On Thu, Jan 11, 2018 at 08:53:42PM +0000, Chris Wilson wrote:
>>> Quoting Ville Syrjälä (2018-01-11 20:10:45)
>>>> On Wed, Jan 10, 2018 at 12:55:05PM +0000, Chris Wilson wrote:
>>>>> While we talk to the punit over its sideband, we need to prevent the cpu
>>>>> from sleeping in order to prevent a potential machine hang.
>>>>>
>>>>> Note that by itself, it appears that pm_qos_update_request (via
>>>>> intel_idle) doesn't provide a sufficient barrier to ensure that all core
>>>>> are indeed awake (out of Cstate) and that the package is awake. To do so,
>>>>> we need to supplement the pm_qos with a manual ping on_each_cpu.
>>>>>
>>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
>>>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
>>>>> References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> drivers/gpu/drm/i915/i915_drv.c | 6 ++++
>>>>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>>>>> drivers/gpu/drm/i915/intel_sideband.c | 61 ++++++++++++++++++++++++-----------
>>>>> 3 files changed, 50 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>>>> index 6c8da9d20c33..d4b90cc0130b 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>>> @@ -902,6 +902,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>>>>> spin_lock_init(&dev_priv->uncore.lock);
>>>>> mutex_init(&dev_priv->sb_lock);
>>>>> + pm_qos_add_request(&dev_priv->sb_qos,
>>>>> + PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>>>>> +
>>>>> mutex_init(&dev_priv->modeset_restore_lock);
>>>>> mutex_init(&dev_priv->av_mutex);
>>>>> mutex_init(&dev_priv->wm.wm_mutex);
>>>>> @@ -953,6 +956,9 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
>>>>> intel_irq_fini(dev_priv);
>>>>> i915_workqueues_cleanup(dev_priv);
>>>>> i915_engines_cleanup(dev_priv);
>>>>> +
>>>>> + pm_qos_remove_request(&dev_priv->sb_qos);
>>>>> + mutex_destroy(&dev_priv->sb_lock);
>>>>> }
>>>>> static int i915_mmio_setup(struct drm_i915_private *dev_priv)
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index a689396d0ff6..ff3f9effc0bb 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -1887,6 +1887,7 @@ struct drm_i915_private {
>>>>> /* Sideband mailbox protection */
>>>>> struct mutex sb_lock;
>>>>> + struct pm_qos_request sb_qos;
>>>>> /** Cached value of IMR to avoid reads in updating the bitfield */
>>>>> union {
>>>>> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
>>>>> index 75c872bb8cc9..02bdd2e2cef6 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_sideband.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_sideband.c
>>>>> @@ -22,6 +22,8 @@
>>>>> *
>>>>> */
>>>>> +#include <asm/iosf_mbi.h>
>>>>> +
>>>>> #include "i915_drv.h"
>>>>> #include "intel_drv.h"
>>>>> @@ -39,18 +41,20 @@
>>>>> /* Private register write, double-word addressing, non-posted */
>>>>> #define SB_CRWRDA_NP 0x07
>>>>> -static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
>>>>> - u32 port, u32 opcode, u32 addr, u32 *val)
>>>>> +static void ping(void *info)
>>>>> {
>>>>> - u32 cmd, be = 0xf, bar = 0;
>>>>> - bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
>>>>> +}
>>>>> - cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
>>>>> - (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
>>>>> - (bar << IOSF_BAR_SHIFT);
>>>>> +static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
>>>>> + u32 devfn, u32 port, u32 opcode,
>>>>> + u32 addr, u32 *val)
>>>>> +{
>>>>> + const bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
>>>>> + int err;
>>>>> - WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
>>>>> + lockdep_assert_held(&dev_priv->sb_lock);
>>>>> + /* Flush the previous comms, just in case it failed last time. */
>>>>> if (intel_wait_for_register(dev_priv,
>>>>> VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>>>>> 5)) {
>>>>> @@ -59,22 +63,43 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
>>>>> return -EAGAIN;
>>>>> }
>>>>> - I915_WRITE(VLV_IOSF_ADDR, addr);
>>>>> - I915_WRITE(VLV_IOSF_DATA, is_read ? 0 : *val);
>>>>> - I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
>>>>> + iosf_mbi_punit_acquire();
>>>>> - if (intel_wait_for_register(dev_priv,
>>>>> - VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>>>>> - 5)) {
>>>>> + /*
>>>>> + * Prevent the cpu from sleeping while we use this sideband, otherwise
>>>>> + * the punit may cause a machine hang.
>>>>> + */
>>>>> + pm_qos_update_request(&dev_priv->sb_qos, 0);
>>>>> + on_each_cpu(ping, NULL, 1);
>>>>
>>>> pm_qos_update_request() doesn't wake up the cpus on its own? I wonder
>>>> kind of latency guarantees it can actually give without doing that.
>>>
>>> intel_idle plugs into the update to wake up idle cpus, but it is not
>>> synchronous. If we don't have the on_each_cpu() here, the hangs reoccur;
>>> the easiest answer seems to be that the on_each_cpu() is causing a
>>> schedule of the RT migration thread which is enough to ensure that the
>>> wakeup has occurred.
>>>> Shouldn't we check if we're actually be talking to the punit and not
>>>> some other unit before we do all this extra work?
>>>
>>> Checking port? I am suspicious of the whole iosf mechanism...
>>
>> I doubt iosf sb itself is busted. That would probably make the entire
>> soc pretty much dead. I suspect the problem is either in the punit
>> firmware, or something fishy is going on with power delivery and
>> reducing the rate at which we change the voltages and whatnot helps
>> keep things a bit more stable.
>>
>> So yeah, I would limit this to punit only by checking the port.
>>
>>>
>>> Also debating whether to only apply it to j1900 or all. Is it just pure
>>> fluke that we can easily reproduce it on the quad core Baytrail as
>>> opposed to the dual core vlv or chv?
>>
>> Not sure. I guess if the problem is around power delivery it might even
>> be board specific to some degree.
>
> The wordpress (on azure.com) link here:
> https://bugzilla.kernel.org/show_bug.cgi?id=109051#c860
>
> Certainly seems to hint at a power delivery problem (with the SoC not
> with specific boards) and all models reported as needing
> intel_idle.max_cstate=1 there are quad-core Bay Trails.
>
> I guess it the problem happens on all quad-core Bay Trail variants when
> the GPU and CPU both increase there power-demand (by up-clocking, resp.
> leaving C6) at the same time and Chris' fix makes sure all CPU cores
> leave C6 before doing the GPU up-clocking avoiding this.
>
> Note this is all speculation / educated guessing.
Also interesting wrt this is this comment:
https://bugzilla.kernel.org/show_bug.cgi?id=109051#c752
Which speculates along the same line and the commenter has done
extensive testing (with 2 boards) coming to the conclusion that
the dual core part does not have whatever problem he is seeing.
Regards,
Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband
2018-01-11 22:04 ` Hans de Goede
@ 2018-01-12 10:02 ` Mika Kuoppala
0 siblings, 0 replies; 19+ messages in thread
From: Mika Kuoppala @ 2018-01-12 10:02 UTC (permalink / raw)
To: Hans de Goede, Ville Syrjälä, Chris Wilson; +Cc: intel-gfx
Hans de Goede <hdegoede@redhat.com> writes:
> Hi,
>
> On 11-01-18 22:42, Hans de Goede wrote:
>> Hi,
>>
>> On 11-01-18 22:17, Ville Syrjälä wrote:
>>> On Thu, Jan 11, 2018 at 08:53:42PM +0000, Chris Wilson wrote:
>>>> Quoting Ville Syrjälä (2018-01-11 20:10:45)
>>>>> On Wed, Jan 10, 2018 at 12:55:05PM +0000, Chris Wilson wrote:
>>>>>> While we talk to the punit over its sideband, we need to prevent the cpu
>>>>>> from sleeping in order to prevent a potential machine hang.
>>>>>>
>>>>>> Note that by itself, it appears that pm_qos_update_request (via
>>>>>> intel_idle) doesn't provide a sufficient barrier to ensure that all core
>>>>>> are indeed awake (out of Cstate) and that the package is awake. To do so,
>>>>>> we need to supplement the pm_qos with a manual ping on_each_cpu.
>>>>>>
>>>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109051
>>>>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=102657
>>>>>> References: https://bugzilla.kernel.org/show_bug.cgi?id=195255
>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/i915/i915_drv.c | 6 ++++
>>>>>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>>>>>> drivers/gpu/drm/i915/intel_sideband.c | 61 ++++++++++++++++++++++++-----------
>>>>>> 3 files changed, 50 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>>>>> index 6c8da9d20c33..d4b90cc0130b 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>>>> @@ -902,6 +902,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>>>>>> spin_lock_init(&dev_priv->uncore.lock);
>>>>>> mutex_init(&dev_priv->sb_lock);
>>>>>> + pm_qos_add_request(&dev_priv->sb_qos,
>>>>>> + PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>>>>>> +
>>>>>> mutex_init(&dev_priv->modeset_restore_lock);
>>>>>> mutex_init(&dev_priv->av_mutex);
>>>>>> mutex_init(&dev_priv->wm.wm_mutex);
>>>>>> @@ -953,6 +956,9 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
>>>>>> intel_irq_fini(dev_priv);
>>>>>> i915_workqueues_cleanup(dev_priv);
>>>>>> i915_engines_cleanup(dev_priv);
>>>>>> +
>>>>>> + pm_qos_remove_request(&dev_priv->sb_qos);
>>>>>> + mutex_destroy(&dev_priv->sb_lock);
>>>>>> }
>>>>>> static int i915_mmio_setup(struct drm_i915_private *dev_priv)
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>>> index a689396d0ff6..ff3f9effc0bb 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>>> @@ -1887,6 +1887,7 @@ struct drm_i915_private {
>>>>>> /* Sideband mailbox protection */
>>>>>> struct mutex sb_lock;
>>>>>> + struct pm_qos_request sb_qos;
>>>>>> /** Cached value of IMR to avoid reads in updating the bitfield */
>>>>>> union {
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
>>>>>> index 75c872bb8cc9..02bdd2e2cef6 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_sideband.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_sideband.c
>>>>>> @@ -22,6 +22,8 @@
>>>>>> *
>>>>>> */
>>>>>> +#include <asm/iosf_mbi.h>
>>>>>> +
>>>>>> #include "i915_drv.h"
>>>>>> #include "intel_drv.h"
>>>>>> @@ -39,18 +41,20 @@
>>>>>> /* Private register write, double-word addressing, non-posted */
>>>>>> #define SB_CRWRDA_NP 0x07
>>>>>> -static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
>>>>>> - u32 port, u32 opcode, u32 addr, u32 *val)
>>>>>> +static void ping(void *info)
>>>>>> {
>>>>>> - u32 cmd, be = 0xf, bar = 0;
>>>>>> - bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
>>>>>> +}
>>>>>> - cmd = (devfn << IOSF_DEVFN_SHIFT) | (opcode << IOSF_OPCODE_SHIFT) |
>>>>>> - (port << IOSF_PORT_SHIFT) | (be << IOSF_BYTE_ENABLES_SHIFT) |
>>>>>> - (bar << IOSF_BAR_SHIFT);
>>>>>> +static int vlv_sideband_rw(struct drm_i915_private *dev_priv,
>>>>>> + u32 devfn, u32 port, u32 opcode,
>>>>>> + u32 addr, u32 *val)
>>>>>> +{
>>>>>> + const bool is_read = (opcode == SB_MRD_NP || opcode == SB_CRRDDA_NP);
>>>>>> + int err;
>>>>>> - WARN_ON(!mutex_is_locked(&dev_priv->sb_lock));
>>>>>> + lockdep_assert_held(&dev_priv->sb_lock);
>>>>>> + /* Flush the previous comms, just in case it failed last time. */
>>>>>> if (intel_wait_for_register(dev_priv,
>>>>>> VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>>>>>> 5)) {
>>>>>> @@ -59,22 +63,43 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
>>>>>> return -EAGAIN;
>>>>>> }
>>>>>> - I915_WRITE(VLV_IOSF_ADDR, addr);
>>>>>> - I915_WRITE(VLV_IOSF_DATA, is_read ? 0 : *val);
>>>>>> - I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
>>>>>> + iosf_mbi_punit_acquire();
>>>>>> - if (intel_wait_for_register(dev_priv,
>>>>>> - VLV_IOSF_DOORBELL_REQ, IOSF_SB_BUSY, 0,
>>>>>> - 5)) {
>>>>>> + /*
>>>>>> + * Prevent the cpu from sleeping while we use this sideband, otherwise
>>>>>> + * the punit may cause a machine hang.
>>>>>> + */
>>>>>> + pm_qos_update_request(&dev_priv->sb_qos, 0);
>>>>>> + on_each_cpu(ping, NULL, 1);
>>>>>
>>>>> pm_qos_update_request() doesn't wake up the cpus on its own? I wonder
>>>>> kind of latency guarantees it can actually give without doing that.
>>>>
>>>> intel_idle plugs into the update to wake up idle cpus, but it is not
>>>> synchronous. If we don't have the on_each_cpu() here, the hangs reoccur;
>>>> the easiest answer seems to be that the on_each_cpu() is causing a
>>>> schedule of the RT migration thread which is enough to ensure that the
>>>> wakeup has occurred.
>>>>> Shouldn't we check if we're actually be talking to the punit and not
>>>>> some other unit before we do all this extra work?
>>>>
>>>> Checking port? I am suspicious of the whole iosf mechanism...
>>>
>>> I doubt iosf sb itself is busted. That would probably make the entire
>>> soc pretty much dead. I suspect the problem is either in the punit
>>> firmware, or something fishy is going on with power delivery and
>>> reducing the rate at which we change the voltages and whatnot helps
>>> keep things a bit more stable.
>>>
>>> So yeah, I would limit this to punit only by checking the port.
>>>
>>>>
>>>> Also debating whether to only apply it to j1900 or all. Is it just pure
>>>> fluke that we can easily reproduce it on the quad core Baytrail as
>>>> opposed to the dual core vlv or chv?
>>>
>>> Not sure. I guess if the problem is around power delivery it might even
>>> be board specific to some degree.
>>
>> The wordpress (on azure.com) link here:
>> https://bugzilla.kernel.org/show_bug.cgi?id=109051#c860
>>
>> Certainly seems to hint at a power delivery problem (with the SoC not
>> with specific boards) and all models reported as needing
>> intel_idle.max_cstate=1 there are quad-core Bay Trails.
>>
>> I guess it the problem happens on all quad-core Bay Trail variants when
>> the GPU and CPU both increase there power-demand (by up-clocking, resp.
>> leaving C6) at the same time and Chris' fix makes sure all CPU cores
>> leave C6 before doing the GPU up-clocking avoiding this.
>>
>> Note this is all speculation / educated guessing.
>
> Also interesting wrt this is this comment:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=109051#c752
>
> Which speculates along the same line and the commenter has done
> extensive testing (with 2 boards) coming to the conclusion that
> the dual core part does not have whatever problem he is seeing.
>
This has been my understanding that 2 core parts are
not affected. Further, limiting cores to 2 with 4 core j1900 using
maxcpus=2 is an effective workaround.
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-01-12 13:26 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-10 12:55 Keep package awake during punit access Chris Wilson
2018-01-10 12:55 ` [PATCH 1/7] drm/i915: Disable preemption and sleeping while using the punit sideband Chris Wilson
2018-01-10 13:18 ` Hans de Goede
2018-01-10 13:45 ` Mika Kuoppala
2018-01-10 14:02 ` Chris Wilson
2018-01-11 19:43 ` Hans de Goede
2018-01-11 20:10 ` Ville Syrjälä
2018-01-11 20:53 ` Chris Wilson
2018-01-11 21:17 ` Ville Syrjälä
2018-01-11 21:42 ` Hans de Goede
2018-01-11 22:04 ` Hans de Goede
2018-01-12 10:02 ` Mika Kuoppala
2018-01-10 12:55 ` [PATCH 2/7] drm/i915: Lift acquiring the vlv punit to the callers Chris Wilson
2018-01-10 12:55 ` [PATCH 3/7] drm/i915: Lift sideband locking for vlv_punit_(read|write) Chris Wilson
2018-01-10 12:55 ` [PATCH 4/7] drm/i915: Reduce RPS update frequency on Valleyview/Cherryview Chris Wilson
2018-01-10 12:55 ` [PATCH 5/7] Revert "drm/i915: Avoid tweaking evaluation thresholds on Baytrail v3" Chris Wilson
2018-01-10 12:55 ` [PATCH 6/7] drm/i915: Replace pcu_lock with sb_lock Chris Wilson
2018-01-10 12:55 ` [PATCH 7/7] drm/i915: Merge sandybride_pcode_(read|write) Chris Wilson
2018-01-10 13:41 ` ✓ Fi.CI.BAT: success for series starting with [1/7] drm/i915: Disable preemption and sleeping while using the punit sideband Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.