Many distro's have mechanism in place to collect and automatically file bugs for failed WARN()s. And since every third line in i915 is a WARN() it generates quite a lot of noise which is somewhat disconcerting to the end user.
Separate out the internal hw-is-in-the-state-I-expected checks into I915_WARN()s and allow configuration via i915.verbose_checks module param about whether this will generate a full blown stacktrace or just DRM_ERROR().
Signed-off-by: Rob Clark robdclark@gmail.com --- drivers/gpu/drm/i915/i915_drv.h | 29 +++++++ drivers/gpu/drm/i915/i915_params.c | 5 ++ drivers/gpu/drm/i915/intel_display.c | 134 ++++++++++++++++---------------- drivers/gpu/drm/i915/intel_dp.c | 4 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- 5 files changed, 104 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index bb1892d..9fabaff 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -60,6 +60,34 @@ #undef WARN_ON #define WARN_ON(x) WARN(x, "WARN_ON(" #x ")")
+/* Use I915_WARN(x) and I915_WARN_ON() (rather than WARN() and WARN_ON()) + * to check for unexpected conditions which may not necessarily be a user + * visible problem. This will either WARN() or DRM_ERROR() depending on + * the verbose_checks moduleparam, enabling distros and users to tailor + * their preferred amount of i915 abrt spam. + */ +#define I915_WARN(condition, format...) ({ \ + int __ret_warn_on = !!(condition); \ + if (unlikely(__ret_warn_on)) { \ + if (i915.verbose_checks) \ + __WARN_printf(format); \ + else \ + DRM_ERROR(format); \ + } \ + unlikely(__ret_warn_on); \ +}) + +#define I915_WARN_ON(condition) ({ \ + int __ret_warn_on = !!(condition); \ + if (unlikely(__ret_warn_on)) { \ + if (i915.verbose_checks) \ + __WARN_printf("WARN_ON(" #condition ")\n"); \ + else \ + DRM_ERROR("WARN_ON(" #condition ")\n"); \ + } \ + unlikely(__ret_warn_on); \ +}) + enum pipe { INVALID_PIPE = -1, PIPE_A = 0, @@ -2311,6 +2339,7 @@ struct i915_params { bool disable_vtd_wa; int use_mmio_flip; bool mmio_debug; + bool verbose_checks; }; extern struct i915_params i915 __read_mostly;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index c91cb20..72777da 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -51,6 +51,7 @@ struct i915_params i915 __read_mostly = { .disable_vtd_wa = 0, .use_mmio_flip = 0, .mmio_debug = 0, + .verbose_checks = 1, };
module_param_named(modeset, i915.modeset, int, 0400); @@ -173,3 +174,7 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600); MODULE_PARM_DESC(mmio_debug, "Enable the MMIO debug code (default: false). This may negatively " "affect performance."); + +module_param_named(verbose_checks, i915.verbose_checks, bool, 0600); +MODULE_PARM_DESC(verbose_checks, + "Enable verbose logs (ie. WARN_ON()) in case of unexpected conditions."); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 63247c6..7b4ed18 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1024,7 +1024,7 @@ void assert_pll(struct drm_i915_private *dev_priv, reg = DPLL(pipe); val = I915_READ(reg); cur_state = !!(val & DPLL_VCO_ENABLE); - WARN(cur_state != state, + I915_WARN(cur_state != state, "PLL state assertion failure (expected %s, current %s)\n", state_string(state), state_string(cur_state)); } @@ -1040,7 +1040,7 @@ static void assert_dsi_pll(struct drm_i915_private *dev_priv, bool state) mutex_unlock(&dev_priv->dpio_lock);
cur_state = val & DSI_PLL_VCO_EN; - WARN(cur_state != state, + I915_WARN(cur_state != state, "DSI PLL state assertion failure (expected %s, current %s)\n", state_string(state), state_string(cur_state)); } @@ -1071,7 +1071,7 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv, return;
cur_state = pll->get_hw_state(dev_priv, pll, &hw_state); - WARN(cur_state != state, + I915_WARN(cur_state != state, "%s assertion failure (expected %s, current %s)\n", pll->name, state_string(state), state_string(cur_state)); } @@ -1095,7 +1095,7 @@ static void assert_fdi_tx(struct drm_i915_private *dev_priv, val = I915_READ(reg); cur_state = !!(val & FDI_TX_ENABLE); } - WARN(cur_state != state, + I915_WARN(cur_state != state, "FDI TX state assertion failure (expected %s, current %s)\n", state_string(state), state_string(cur_state)); } @@ -1112,7 +1112,7 @@ static void assert_fdi_rx(struct drm_i915_private *dev_priv, reg = FDI_RX_CTL(pipe); val = I915_READ(reg); cur_state = !!(val & FDI_RX_ENABLE); - WARN(cur_state != state, + I915_WARN(cur_state != state, "FDI RX state assertion failure (expected %s, current %s)\n", state_string(state), state_string(cur_state)); } @@ -1135,7 +1135,7 @@ static void assert_fdi_tx_pll_enabled(struct drm_i915_private *dev_priv,
reg = FDI_TX_CTL(pipe); val = I915_READ(reg); - WARN(!(val & FDI_TX_PLL_ENABLE), "FDI TX PLL assertion failure, should be active but is disabled\n"); + I915_WARN(!(val & FDI_TX_PLL_ENABLE), "FDI TX PLL assertion failure, should be active but is disabled\n"); }
void assert_fdi_rx_pll(struct drm_i915_private *dev_priv, @@ -1148,7 +1148,7 @@ void assert_fdi_rx_pll(struct drm_i915_private *dev_priv, reg = FDI_RX_CTL(pipe); val = I915_READ(reg); cur_state = !!(val & FDI_RX_PLL_ENABLE); - WARN(cur_state != state, + I915_WARN(cur_state != state, "FDI RX PLL assertion failure (expected %s, current %s)\n", state_string(state), state_string(cur_state)); } @@ -1190,7 +1190,7 @@ void assert_panel_unlocked(struct drm_i915_private *dev_priv, ((val & PANEL_UNLOCK_MASK) == PANEL_UNLOCK_REGS)) locked = false;
- WARN(panel_pipe == pipe && locked, + I915_WARN(panel_pipe == pipe && locked, "panel assertion failure, pipe %c regs locked\n", pipe_name(pipe)); } @@ -1206,7 +1206,7 @@ static void assert_cursor(struct drm_i915_private *dev_priv, else cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
- WARN(cur_state != state, + I915_WARN(cur_state != state, "cursor on pipe %c assertion failure (expected %s, current %s)\n", pipe_name(pipe), state_string(state), state_string(cur_state)); } @@ -1236,7 +1236,7 @@ void assert_pipe(struct drm_i915_private *dev_priv, cur_state = !!(val & PIPECONF_ENABLE); }
- WARN(cur_state != state, + I915_WARN(cur_state != state, "pipe %c assertion failure (expected %s, current %s)\n", pipe_name(pipe), state_string(state), state_string(cur_state)); } @@ -1251,7 +1251,7 @@ static void assert_plane(struct drm_i915_private *dev_priv, reg = DSPCNTR(plane); val = I915_READ(reg); cur_state = !!(val & DISPLAY_PLANE_ENABLE); - WARN(cur_state != state, + I915_WARN(cur_state != state, "plane %c assertion failure (expected %s, current %s)\n", plane_name(plane), state_string(state), state_string(cur_state)); } @@ -1271,7 +1271,7 @@ static void assert_planes_disabled(struct drm_i915_private *dev_priv, if (INTEL_INFO(dev)->gen >= 4) { reg = DSPCNTR(pipe); val = I915_READ(reg); - WARN(val & DISPLAY_PLANE_ENABLE, + I915_WARN(val & DISPLAY_PLANE_ENABLE, "plane %c assertion failure, should be disabled but not\n", plane_name(pipe)); return; @@ -1283,7 +1283,7 @@ static void assert_planes_disabled(struct drm_i915_private *dev_priv, val = I915_READ(reg); cur_pipe = (val & DISPPLANE_SEL_PIPE_MASK) >> DISPPLANE_SEL_PIPE_SHIFT; - WARN((val & DISPLAY_PLANE_ENABLE) && pipe == cur_pipe, + I915_WARN((val & DISPLAY_PLANE_ENABLE) && pipe == cur_pipe, "plane %c assertion failure, should be off on pipe %c but is still active\n", plane_name(i), pipe_name(pipe)); } @@ -1299,7 +1299,7 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv, if (INTEL_INFO(dev)->gen >= 9) { for_each_sprite(pipe, sprite) { val = I915_READ(PLANE_CTL(pipe, sprite)); - WARN(val & PLANE_CTL_ENABLE, + I915_WARN(val & PLANE_CTL_ENABLE, "plane %d assertion failure, should be off on pipe %c but is still active\n", sprite, pipe_name(pipe)); } @@ -1307,20 +1307,20 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv, for_each_sprite(pipe, sprite) { reg = SPCNTR(pipe, sprite); val = I915_READ(reg); - WARN(val & SP_ENABLE, + I915_WARN(val & SP_ENABLE, "sprite %c assertion failure, should be off on pipe %c but is still active\n", sprite_name(pipe, sprite), pipe_name(pipe)); } } else if (INTEL_INFO(dev)->gen >= 7) { reg = SPRCTL(pipe); val = I915_READ(reg); - WARN(val & SPRITE_ENABLE, + I915_WARN(val & SPRITE_ENABLE, "sprite %c assertion failure, should be off on pipe %c but is still active\n", plane_name(pipe), pipe_name(pipe)); } else if (INTEL_INFO(dev)->gen >= 5) { reg = DVSCNTR(pipe); val = I915_READ(reg); - WARN(val & DVS_ENABLE, + I915_WARN(val & DVS_ENABLE, "sprite %c assertion failure, should be off on pipe %c but is still active\n", plane_name(pipe), pipe_name(pipe)); } @@ -1328,7 +1328,7 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
static void assert_vblank_disabled(struct drm_crtc *crtc) { - if (WARN_ON(drm_crtc_vblank_get(crtc) == 0)) + if (I915_WARN_ON(drm_crtc_vblank_get(crtc) == 0)) drm_crtc_vblank_put(crtc); }
@@ -1337,12 +1337,12 @@ static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv) u32 val; bool enabled;
- WARN_ON(!(HAS_PCH_IBX(dev_priv->dev) || HAS_PCH_CPT(dev_priv->dev))); + I915_WARN_ON(!(HAS_PCH_IBX(dev_priv->dev) || HAS_PCH_CPT(dev_priv->dev)));
val = I915_READ(PCH_DREF_CONTROL); enabled = !!(val & (DREF_SSC_SOURCE_MASK | DREF_NONSPREAD_SOURCE_MASK | DREF_SUPERSPREAD_SOURCE_MASK)); - WARN(!enabled, "PCH refclk assertion failure, should be active but is disabled\n"); + I915_WARN(!enabled, "PCH refclk assertion failure, should be active but is disabled\n"); }
static void assert_pch_transcoder_disabled(struct drm_i915_private *dev_priv, @@ -1355,7 +1355,7 @@ static void assert_pch_transcoder_disabled(struct drm_i915_private *dev_priv, reg = PCH_TRANSCONF(pipe); val = I915_READ(reg); enabled = !!(val & TRANS_ENABLE); - WARN(enabled, + I915_WARN(enabled, "transcoder assertion failed, should be off on pipe %c but is still active\n", pipe_name(pipe)); } @@ -1435,11 +1435,11 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv, enum pipe pipe, int reg, u32 port_sel) { u32 val = I915_READ(reg); - WARN(dp_pipe_enabled(dev_priv, pipe, port_sel, val), + I915_WARN(dp_pipe_enabled(dev_priv, pipe, port_sel, val), "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n", reg, pipe_name(pipe));
- WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0 + I915_WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0 && (val & DP_PIPEB_SELECT), "IBX PCH dp port still using transcoder B\n"); } @@ -1448,11 +1448,11 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv, enum pipe pipe, int reg) { u32 val = I915_READ(reg); - WARN(hdmi_pipe_enabled(dev_priv, pipe, val), + I915_WARN(hdmi_pipe_enabled(dev_priv, pipe, val), "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n", reg, pipe_name(pipe));
- WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_ENABLE) == 0 + I915_WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_ENABLE) == 0 && (val & SDVO_PIPE_B_SELECT), "IBX PCH hdmi port still using transcoder B\n"); } @@ -1469,13 +1469,13 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
reg = PCH_ADPA; val = I915_READ(reg); - WARN(adpa_pipe_enabled(dev_priv, pipe, val), + I915_WARN(adpa_pipe_enabled(dev_priv, pipe, val), "PCH VGA enabled on transcoder %c, should be disabled\n", pipe_name(pipe));
reg = PCH_LVDS; val = I915_READ(reg); - WARN(lvds_pipe_enabled(dev_priv, pipe, val), + I915_WARN(lvds_pipe_enabled(dev_priv, pipe, val), "PCH LVDS enabled on transcoder %c, should be disabled\n", pipe_name(pipe));
@@ -5382,25 +5382,25 @@ static void intel_connector_check_state(struct intel_connector *connector) if (connector->mst_port) return;
- WARN(connector->base.dpms == DRM_MODE_DPMS_OFF, + I915_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF, "wrong connector dpms state\n"); - WARN(connector->base.encoder != &encoder->base, + I915_WARN(connector->base.encoder != &encoder->base, "active connector not linked to encoder\n");
if (encoder) { - WARN(!encoder->connectors_active, + I915_WARN(!encoder->connectors_active, "encoder->connectors_active not set\n");
encoder_enabled = encoder->get_hw_state(encoder, &pipe); - WARN(!encoder_enabled, "encoder not enabled\n"); - if (WARN_ON(!encoder->base.crtc)) + I915_WARN(!encoder_enabled, "encoder not enabled\n"); + if (I915_WARN_ON(!encoder->base.crtc)) return;
crtc = encoder->base.crtc;
- WARN(!crtc->enabled, "crtc not enabled\n"); - WARN(!to_intel_crtc(crtc)->active, "crtc not active\n"); - WARN(pipe != to_intel_crtc(crtc)->pipe, + I915_WARN(!crtc->enabled, "crtc not enabled\n"); + I915_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n"); + I915_WARN(pipe != to_intel_crtc(crtc)->pipe, "encoder active on the wrong pipe\n"); } } @@ -7810,24 +7810,24 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv) struct intel_crtc *crtc;
for_each_intel_crtc(dev, crtc) - WARN(crtc->active, "CRTC for pipe %c enabled\n", + I915_WARN(crtc->active, "CRTC for pipe %c enabled\n", pipe_name(crtc->pipe));
- WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n"); - WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL enabled\n"); - WARN(I915_READ(WRPLL_CTL1) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n"); - WARN(I915_READ(WRPLL_CTL2) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n"); - WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n"); - WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE, + I915_WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n"); + I915_WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL enabled\n"); + I915_WARN(I915_READ(WRPLL_CTL1) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n"); + I915_WARN(I915_READ(WRPLL_CTL2) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n"); + I915_WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n"); + I915_WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE, "CPU PWM1 enabled\n"); if (IS_HASWELL(dev)) - WARN(I915_READ(HSW_BLC_PWM2_CTL) & BLM_PWM_ENABLE, + I915_WARN(I915_READ(HSW_BLC_PWM2_CTL) & BLM_PWM_ENABLE, "CPU PWM2 enabled\n"); - WARN(I915_READ(BLC_PWM_PCH_CTL1) & BLM_PCH_PWM_ENABLE, + I915_WARN(I915_READ(BLC_PWM_PCH_CTL1) & BLM_PCH_PWM_ENABLE, "PCH PWM1 enabled\n"); - WARN(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE, + I915_WARN(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE, "Utility pin enabled\n"); - WARN(I915_READ(PCH_GTC_CTL) & PCH_GTC_ENABLE, "PCH GTC enabled\n"); + I915_WARN(I915_READ(PCH_GTC_CTL) & PCH_GTC_ENABLE, "PCH GTC enabled\n");
/* * In theory we can still leave IRQs enabled, as long as only the HPD @@ -7835,7 +7835,7 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv) * gen-specific and since we only disable LCPLL after we fully disable * the interrupts, the check below should be enough. */ - WARN(intel_irqs_enabled(dev_priv), "IRQs enabled\n"); + I915_WARN(intel_irqs_enabled(dev_priv), "IRQs enabled\n"); }
static uint32_t hsw_read_dcomp(struct drm_i915_private *dev_priv) @@ -10742,7 +10742,7 @@ check_connector_state(struct drm_device *dev) * ->get_hw_state callbacks. */ intel_connector_check_state(connector);
- WARN(&connector->new_encoder->base != connector->base.encoder, + I915_WARN(&connector->new_encoder->base != connector->base.encoder, "connector's staged encoder doesn't match current encoder\n"); } } @@ -10762,9 +10762,9 @@ check_encoder_state(struct drm_device *dev) encoder->base.base.id, encoder->base.name);
- WARN(&encoder->new_crtc->base != encoder->base.crtc, + I915_WARN(&encoder->new_crtc->base != encoder->base.crtc, "encoder's stage crtc doesn't match current crtc\n"); - WARN(encoder->connectors_active && !encoder->base.crtc, + I915_WARN(encoder->connectors_active && !encoder->base.crtc, "encoder's active_connectors set, but no crtc\n");
list_for_each_entry(connector, &dev->mode_config.connector_list, @@ -10783,19 +10783,19 @@ check_encoder_state(struct drm_device *dev) if (!enabled && encoder->base.encoder_type == DRM_MODE_ENCODER_DPMST) continue;
- WARN(!!encoder->base.crtc != enabled, + I915_WARN(!!encoder->base.crtc != enabled, "encoder's enabled state mismatch " "(expected %i, found %i)\n", !!encoder->base.crtc, enabled); - WARN(active && !encoder->base.crtc, + I915_WARN(active && !encoder->base.crtc, "active encoder with no crtc\n");
- WARN(encoder->connectors_active != active, + I915_WARN(encoder->connectors_active != active, "encoder's computed active state doesn't match tracked active state " "(expected %i, found %i)\n", active, encoder->connectors_active);
active = encoder->get_hw_state(encoder, &pipe); - WARN(active != encoder->connectors_active, + I915_WARN(active != encoder->connectors_active, "encoder's hw state doesn't match sw tracking " "(expected %i, found %i)\n", encoder->connectors_active, active); @@ -10804,7 +10804,7 @@ check_encoder_state(struct drm_device *dev) continue;
tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe; - WARN(active && pipe != tracked_pipe, + I915_WARN(active && pipe != tracked_pipe, "active encoder's pipe doesn't match" "(expected %i, found %i)\n", tracked_pipe, pipe); @@ -10829,7 +10829,7 @@ check_crtc_state(struct drm_device *dev) DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.base.id);
- WARN(crtc->active && !crtc->base.enabled, + I915_WARN(crtc->active && !crtc->base.enabled, "active crtc, but not enabled in sw tracking\n");
for_each_intel_encoder(dev, encoder) { @@ -10840,10 +10840,10 @@ check_crtc_state(struct drm_device *dev) active = true; }
- WARN(active != crtc->active, + I915_WARN(active != crtc->active, "crtc's computed active state doesn't match tracked active state " "(expected %i, found %i)\n", active, crtc->active); - WARN(enabled != crtc->base.enabled, + I915_WARN(enabled != crtc->base.enabled, "crtc's computed enabled state doesn't match tracked enabled state " "(expected %i, found %i)\n", enabled, crtc->base.enabled);
@@ -10863,13 +10863,13 @@ check_crtc_state(struct drm_device *dev) encoder->get_config(encoder, &pipe_config); }
- WARN(crtc->active != active, + I915_WARN(crtc->active != active, "crtc active state doesn't match with hw state " "(expected %i, found %i)\n", crtc->active, active);
if (active && !intel_pipe_config_compare(dev, &crtc->config, &pipe_config)) { - WARN(1, "pipe state doesn't match!\n"); + I915_WARN(1, "pipe state doesn't match!\n"); intel_dump_pipe_config(crtc, &pipe_config, "[hw state]"); intel_dump_pipe_config(crtc, &crtc->config, @@ -10897,14 +10897,14 @@ check_shared_dpll_state(struct drm_device *dev)
active = pll->get_hw_state(dev_priv, pll, &dpll_hw_state);
- WARN(pll->active > hweight32(pll->config.crtc_mask), + I915_WARN(pll->active > hweight32(pll->config.crtc_mask), "more active pll users than references: %i vs %i\n", pll->active, hweight32(pll->config.crtc_mask)); - WARN(pll->active && !pll->on, + I915_WARN(pll->active && !pll->on, "pll in active use but not on in sw tracking\n"); - WARN(pll->on && !pll->active, + I915_WARN(pll->on && !pll->active, "pll in on but not on in use in sw tracking\n"); - WARN(pll->on != active, + I915_WARN(pll->on != active, "pll on state mismatch (expected %i, found %i)\n", pll->on, active);
@@ -10914,14 +10914,14 @@ check_shared_dpll_state(struct drm_device *dev) if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) active_crtcs++; } - WARN(pll->active != active_crtcs, + I915_WARN(pll->active != active_crtcs, "pll active crtcs mismatch (expected %i, found %i)\n", pll->active, active_crtcs); - WARN(hweight32(pll->config.crtc_mask) != enabled_crtcs, + I915_WARN(hweight32(pll->config.crtc_mask) != enabled_crtcs, "pll enabled crtcs mismatch (expected %i, found %i)\n", hweight32(pll->config.crtc_mask), enabled_crtcs);
- WARN(pll->on && memcmp(&pll->config.hw_state, &dpll_hw_state, + I915_WARN(pll->on && memcmp(&pll->config.hw_state, &dpll_hw_state, sizeof(dpll_hw_state)), "pll hw state mismatch\n"); } diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 5cecc20..6d5bf6e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1558,7 +1558,7 @@ void intel_edp_panel_vdd_on(struct intel_dp *intel_dp) vdd = edp_panel_vdd_on(intel_dp); pps_unlock(intel_dp);
- WARN(!vdd, "eDP port %c VDD already requested on\n", + I915_WARN(!vdd, "eDP port %c VDD already requested on\n", port_name(dp_to_dig_port(intel_dp)->port)); }
@@ -1642,7 +1642,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) if (!is_edp(intel_dp)) return;
- WARN(!intel_dp->want_panel_vdd, "eDP port %c VDD not forced on", + I915_WARN(!intel_dp->want_panel_vdd, "eDP port %c VDD not forced on", port_name(dp_to_dig_port(intel_dp)->port));
intel_dp->want_panel_vdd = false; diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index f5a78d5..770c5f4 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -633,7 +633,7 @@ static void check_power_well_state(struct drm_i915_private *dev_priv, return;
mismatch: - WARN(1, "state mismatch for '%s' (always_on %d hw state %d use-count %d disable_power_well %d\n", + I915_WARN(1, "state mismatch for '%s' (always_on %d hw state %d use-count %d disable_power_well %d\n", power_well->name, power_well->always_on, enabled, power_well->count, i915.disable_power_well); }
On Thu, Dec 11, 2014 at 06:18:12PM -0500, Rob Clark wrote:
Many distro's have mechanism in place to collect and automatically file bugs for failed WARN()s. And since every third line in i915 is a WARN() it generates quite a lot of noise which is somewhat disconcerting to the end user.
Separate out the internal hw-is-in-the-state-I-expected checks into I915_WARN()s and allow configuration via i915.verbose_checks module param about whether this will generate a full blown stacktrace or just DRM_ERROR().
Signed-off-by: Rob Clark robdclark@gmail.com
Yeah I guess makes sense, although I still claim that these are as much "we've lost track of shit" bugs as when a refcount underflows or a pointer is NULL when it shouldn't. But I also agree that we've done a stellar job this year at not locking at these bugs, so meh.
Since this is all about hw cross checking, ack if I do an s/I915_WARN/I915_HW_WARN/ on top? -Daniel
drivers/gpu/drm/i915/i915_drv.h | 29 +++++++ drivers/gpu/drm/i915/i915_params.c | 5 ++ drivers/gpu/drm/i915/intel_display.c | 134 ++++++++++++++++---------------- drivers/gpu/drm/i915/intel_dp.c | 4 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- 5 files changed, 104 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index bb1892d..9fabaff 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -60,6 +60,34 @@ #undef WARN_ON #define WARN_ON(x) WARN(x, "WARN_ON(" #x ")")
+/* Use I915_WARN(x) and I915_WARN_ON() (rather than WARN() and WARN_ON())
- to check for unexpected conditions which may not necessarily be a user
- visible problem. This will either WARN() or DRM_ERROR() depending on
- the verbose_checks moduleparam, enabling distros and users to tailor
- their preferred amount of i915 abrt spam.
- */
+#define I915_WARN(condition, format...) ({ \
- int __ret_warn_on = !!(condition); \
- if (unlikely(__ret_warn_on)) { \
if (i915.verbose_checks) \__WARN_printf(format); \else \DRM_ERROR(format); \- } \
- unlikely(__ret_warn_on); \
+})
+#define I915_WARN_ON(condition) ({ \
- int __ret_warn_on = !!(condition); \
- if (unlikely(__ret_warn_on)) { \
if (i915.verbose_checks) \__WARN_printf("WARN_ON(" #condition ")\n"); \else \DRM_ERROR("WARN_ON(" #condition ")\n"); \- } \
- unlikely(__ret_warn_on); \
+})
enum pipe { INVALID_PIPE = -1, PIPE_A = 0, @@ -2311,6 +2339,7 @@ struct i915_params { bool disable_vtd_wa; int use_mmio_flip; bool mmio_debug;
- bool verbose_checks;
}; extern struct i915_params i915 __read_mostly;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index c91cb20..72777da 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -51,6 +51,7 @@ struct i915_params i915 __read_mostly = { .disable_vtd_wa = 0, .use_mmio_flip = 0, .mmio_debug = 0,
- .verbose_checks = 1,
};
module_param_named(modeset, i915.modeset, int, 0400); @@ -173,3 +174,7 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600); MODULE_PARM_DESC(mmio_debug, "Enable the MMIO debug code (default: false). This may negatively " "affect performance.");
+module_param_named(verbose_checks, i915.verbose_checks, bool, 0600); +MODULE_PARM_DESC(verbose_checks,
- "Enable verbose logs (ie. WARN_ON()) in case of unexpected conditions.");
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 63247c6..7b4ed18 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1024,7 +1024,7 @@ void assert_pll(struct drm_i915_private *dev_priv, reg = DPLL(pipe); val = I915_READ(reg); cur_state = !!(val & DPLL_VCO_ENABLE);
- WARN(cur_state != state,
- I915_WARN(cur_state != state, "PLL state assertion failure (expected %s, current %s)\n", state_string(state), state_string(cur_state));
} @@ -1040,7 +1040,7 @@ static void assert_dsi_pll(struct drm_i915_private *dev_priv, bool state) mutex_unlock(&dev_priv->dpio_lock);
cur_state = val & DSI_PLL_VCO_EN;
- WARN(cur_state != state,
- I915_WARN(cur_state != state, "DSI PLL state assertion failure (expected %s, current %s)\n", state_string(state), state_string(cur_state));
} @@ -1071,7 +1071,7 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv, return;
cur_state = pll->get_hw_state(dev_priv, pll, &hw_state);
- WARN(cur_state != state,
- I915_WARN(cur_state != state, "%s assertion failure (expected %s, current %s)\n", pll->name, state_string(state), state_string(cur_state));
} @@ -1095,7 +1095,7 @@ static void assert_fdi_tx(struct drm_i915_private *dev_priv, val = I915_READ(reg); cur_state = !!(val & FDI_TX_ENABLE); }
- WARN(cur_state != state,
- I915_WARN(cur_state != state, "FDI TX state assertion failure (expected %s, current %s)\n", state_string(state), state_string(cur_state));
} @@ -1112,7 +1112,7 @@ static void assert_fdi_rx(struct drm_i915_private *dev_priv, reg = FDI_RX_CTL(pipe); val = I915_READ(reg); cur_state = !!(val & FDI_RX_ENABLE);
- WARN(cur_state != state,
- I915_WARN(cur_state != state, "FDI RX state assertion failure (expected %s, current %s)\n", state_string(state), state_string(cur_state));
} @@ -1135,7 +1135,7 @@ static void assert_fdi_tx_pll_enabled(struct drm_i915_private *dev_priv,
reg = FDI_TX_CTL(pipe); val = I915_READ(reg);
- WARN(!(val & FDI_TX_PLL_ENABLE), "FDI TX PLL assertion failure, should be active but is disabled\n");
- I915_WARN(!(val & FDI_TX_PLL_ENABLE), "FDI TX PLL assertion failure, should be active but is disabled\n");
}
void assert_fdi_rx_pll(struct drm_i915_private *dev_priv, @@ -1148,7 +1148,7 @@ void assert_fdi_rx_pll(struct drm_i915_private *dev_priv, reg = FDI_RX_CTL(pipe); val = I915_READ(reg); cur_state = !!(val & FDI_RX_PLL_ENABLE);
- WARN(cur_state != state,
- I915_WARN(cur_state != state, "FDI RX PLL assertion failure (expected %s, current %s)\n", state_string(state), state_string(cur_state));
} @@ -1190,7 +1190,7 @@ void assert_panel_unlocked(struct drm_i915_private *dev_priv, ((val & PANEL_UNLOCK_MASK) == PANEL_UNLOCK_REGS)) locked = false;
- WARN(panel_pipe == pipe && locked,
- I915_WARN(panel_pipe == pipe && locked, "panel assertion failure, pipe %c regs locked\n", pipe_name(pipe));
} @@ -1206,7 +1206,7 @@ static void assert_cursor(struct drm_i915_private *dev_priv, else cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
- WARN(cur_state != state,
- I915_WARN(cur_state != state, "cursor on pipe %c assertion failure (expected %s, current %s)\n", pipe_name(pipe), state_string(state), state_string(cur_state));
} @@ -1236,7 +1236,7 @@ void assert_pipe(struct drm_i915_private *dev_priv, cur_state = !!(val & PIPECONF_ENABLE); }
- WARN(cur_state != state,
- I915_WARN(cur_state != state, "pipe %c assertion failure (expected %s, current %s)\n", pipe_name(pipe), state_string(state), state_string(cur_state));
} @@ -1251,7 +1251,7 @@ static void assert_plane(struct drm_i915_private *dev_priv, reg = DSPCNTR(plane); val = I915_READ(reg); cur_state = !!(val & DISPLAY_PLANE_ENABLE);
- WARN(cur_state != state,
- I915_WARN(cur_state != state, "plane %c assertion failure (expected %s, current %s)\n", plane_name(plane), state_string(state), state_string(cur_state));
} @@ -1271,7 +1271,7 @@ static void assert_planes_disabled(struct drm_i915_private *dev_priv, if (INTEL_INFO(dev)->gen >= 4) { reg = DSPCNTR(pipe); val = I915_READ(reg);
WARN(val & DISPLAY_PLANE_ENABLE,
I915_WARN(val & DISPLAY_PLANE_ENABLE, "plane %c assertion failure, should be disabled but not\n", plane_name(pipe));@@ -1283,7 +1283,7 @@ static void assert_planes_disabled(struct drm_i915_private *dev_priv, val = I915_READ(reg); cur_pipe = (val & DISPPLANE_SEL_PIPE_MASK) >> DISPPLANE_SEL_PIPE_SHIFT;
WARN((val & DISPLAY_PLANE_ENABLE) && pipe == cur_pipe,
I915_WARN((val & DISPLAY_PLANE_ENABLE) && pipe == cur_pipe, "plane %c assertion failure, should be off on pipe %c but is still active\n", plane_name(i), pipe_name(pipe));@@ -1299,7 +1299,7 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv, if (INTEL_INFO(dev)->gen >= 9) { for_each_sprite(pipe, sprite) { val = I915_READ(PLANE_CTL(pipe, sprite));
WARN(val & PLANE_CTL_ENABLE,
I915_WARN(val & PLANE_CTL_ENABLE, "plane %d assertion failure, should be off on pipe %c but is still active\n", sprite, pipe_name(pipe));@@ -1307,20 +1307,20 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv, for_each_sprite(pipe, sprite) { reg = SPCNTR(pipe, sprite); val = I915_READ(reg);
WARN(val & SP_ENABLE,
I915_WARN(val & SP_ENABLE, "sprite %c assertion failure, should be off on pipe %c but is still active\n", sprite_name(pipe, sprite), pipe_name(pipe));
WARN(val & SPRITE_ENABLE,
I915_WARN(val & SPRITE_ENABLE, "sprite %c assertion failure, should be off on pipe %c but is still active\n", plane_name(pipe), pipe_name(pipe));
WARN(val & DVS_ENABLE,
I915_WARN(val & DVS_ENABLE, "sprite %c assertion failure, should be off on pipe %c but is still active\n", plane_name(pipe), pipe_name(pipe));@@ -1328,7 +1328,7 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
static void assert_vblank_disabled(struct drm_crtc *crtc) {
- if (WARN_ON(drm_crtc_vblank_get(crtc) == 0))
- if (I915_WARN_ON(drm_crtc_vblank_get(crtc) == 0)) drm_crtc_vblank_put(crtc);
}
@@ -1337,12 +1337,12 @@ static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv) u32 val; bool enabled;
- WARN_ON(!(HAS_PCH_IBX(dev_priv->dev) || HAS_PCH_CPT(dev_priv->dev)));
I915_WARN_ON(!(HAS_PCH_IBX(dev_priv->dev) || HAS_PCH_CPT(dev_priv->dev)));
val = I915_READ(PCH_DREF_CONTROL); enabled = !!(val & (DREF_SSC_SOURCE_MASK | DREF_NONSPREAD_SOURCE_MASK | DREF_SUPERSPREAD_SOURCE_MASK));
- WARN(!enabled, "PCH refclk assertion failure, should be active but is disabled\n");
- I915_WARN(!enabled, "PCH refclk assertion failure, should be active but is disabled\n");
}
static void assert_pch_transcoder_disabled(struct drm_i915_private *dev_priv, @@ -1355,7 +1355,7 @@ static void assert_pch_transcoder_disabled(struct drm_i915_private *dev_priv, reg = PCH_TRANSCONF(pipe); val = I915_READ(reg); enabled = !!(val & TRANS_ENABLE);
- WARN(enabled,
- I915_WARN(enabled, "transcoder assertion failed, should be off on pipe %c but is still active\n", pipe_name(pipe));
} @@ -1435,11 +1435,11 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv, enum pipe pipe, int reg, u32 port_sel) { u32 val = I915_READ(reg);
- WARN(dp_pipe_enabled(dev_priv, pipe, port_sel, val),
- I915_WARN(dp_pipe_enabled(dev_priv, pipe, port_sel, val), "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n", reg, pipe_name(pipe));
- WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0
- I915_WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0 && (val & DP_PIPEB_SELECT), "IBX PCH dp port still using transcoder B\n");
} @@ -1448,11 +1448,11 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv, enum pipe pipe, int reg) { u32 val = I915_READ(reg);
- WARN(hdmi_pipe_enabled(dev_priv, pipe, val),
- I915_WARN(hdmi_pipe_enabled(dev_priv, pipe, val), "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n", reg, pipe_name(pipe));
- WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_ENABLE) == 0
- I915_WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_ENABLE) == 0 && (val & SDVO_PIPE_B_SELECT), "IBX PCH hdmi port still using transcoder B\n");
} @@ -1469,13 +1469,13 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
reg = PCH_ADPA; val = I915_READ(reg);
- WARN(adpa_pipe_enabled(dev_priv, pipe, val),
I915_WARN(adpa_pipe_enabled(dev_priv, pipe, val), "PCH VGA enabled on transcoder %c, should be disabled\n", pipe_name(pipe));
reg = PCH_LVDS; val = I915_READ(reg);
- WARN(lvds_pipe_enabled(dev_priv, pipe, val),
- I915_WARN(lvds_pipe_enabled(dev_priv, pipe, val), "PCH LVDS enabled on transcoder %c, should be disabled\n", pipe_name(pipe));
@@ -5382,25 +5382,25 @@ static void intel_connector_check_state(struct intel_connector *connector) if (connector->mst_port) return;
WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
I915_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF, "wrong connector dpms state\n");
WARN(connector->base.encoder != &encoder->base,
I915_WARN(connector->base.encoder != &encoder->base, "active connector not linked to encoder\n");if (encoder) {
WARN(!encoder->connectors_active,
I915_WARN(!encoder->connectors_active, "encoder->connectors_active not set\n"); encoder_enabled = encoder->get_hw_state(encoder, &pipe);
WARN(!encoder_enabled, "encoder not enabled\n");if (WARN_ON(!encoder->base.crtc))
I915_WARN(!encoder_enabled, "encoder not enabled\n");if (I915_WARN_ON(!encoder->base.crtc)) return; crtc = encoder->base.crtc;
WARN(!crtc->enabled, "crtc not enabled\n");WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");WARN(pipe != to_intel_crtc(crtc)->pipe,
I915_WARN(!crtc->enabled, "crtc not enabled\n");I915_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");I915_WARN(pipe != to_intel_crtc(crtc)->pipe, "encoder active on the wrong pipe\n");@@ -7810,24 +7810,24 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv) struct intel_crtc *crtc;
for_each_intel_crtc(dev, crtc)
WARN(crtc->active, "CRTC for pipe %c enabled\n",
I915_WARN(crtc->active, "CRTC for pipe %c enabled\n", pipe_name(crtc->pipe));
- WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
- WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL enabled\n");
- WARN(I915_READ(WRPLL_CTL1) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n");
- WARN(I915_READ(WRPLL_CTL2) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n");
- WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n");
- WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE,
- I915_WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
- I915_WARN(I915_READ(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL enabled\n");
- I915_WARN(I915_READ(WRPLL_CTL1) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n");
- I915_WARN(I915_READ(WRPLL_CTL2) & WRPLL_PLL_ENABLE, "WRPLL2 enabled\n");
- I915_WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n");
- I915_WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE, "CPU PWM1 enabled\n"); if (IS_HASWELL(dev))
WARN(I915_READ(HSW_BLC_PWM2_CTL) & BLM_PWM_ENABLE,
I915_WARN(I915_READ(HSW_BLC_PWM2_CTL) & BLM_PWM_ENABLE, "CPU PWM2 enabled\n");
- WARN(I915_READ(BLC_PWM_PCH_CTL1) & BLM_PCH_PWM_ENABLE,
- I915_WARN(I915_READ(BLC_PWM_PCH_CTL1) & BLM_PCH_PWM_ENABLE, "PCH PWM1 enabled\n");
- WARN(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE,
- I915_WARN(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE, "Utility pin enabled\n");
- WARN(I915_READ(PCH_GTC_CTL) & PCH_GTC_ENABLE, "PCH GTC enabled\n");
I915_WARN(I915_READ(PCH_GTC_CTL) & PCH_GTC_ENABLE, "PCH GTC enabled\n");
/*
- In theory we can still leave IRQs enabled, as long as only the HPD
@@ -7835,7 +7835,7 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv) * gen-specific and since we only disable LCPLL after we fully disable * the interrupts, the check below should be enough. */
- WARN(intel_irqs_enabled(dev_priv), "IRQs enabled\n");
- I915_WARN(intel_irqs_enabled(dev_priv), "IRQs enabled\n");
}
static uint32_t hsw_read_dcomp(struct drm_i915_private *dev_priv) @@ -10742,7 +10742,7 @@ check_connector_state(struct drm_device *dev) * ->get_hw_state callbacks. */ intel_connector_check_state(connector);
WARN(&connector->new_encoder->base != connector->base.encoder,
I915_WARN(&connector->new_encoder->base != connector->base.encoder, "connector's staged encoder doesn't match current encoder\n");} @@ -10762,9 +10762,9 @@ check_encoder_state(struct drm_device *dev) encoder->base.base.id, encoder->base.name);
WARN(&encoder->new_crtc->base != encoder->base.crtc,
I915_WARN(&encoder->new_crtc->base != encoder->base.crtc, "encoder's stage crtc doesn't match current crtc\n");
WARN(encoder->connectors_active && !encoder->base.crtc,
I915_WARN(encoder->connectors_active && !encoder->base.crtc, "encoder's active_connectors set, but no crtc\n");list_for_each_entry(connector, &dev->mode_config.connector_list,
@@ -10783,19 +10783,19 @@ check_encoder_state(struct drm_device *dev) if (!enabled && encoder->base.encoder_type == DRM_MODE_ENCODER_DPMST) continue;
WARN(!!encoder->base.crtc != enabled,
I915_WARN(!!encoder->base.crtc != enabled, "encoder's enabled state mismatch " "(expected %i, found %i)\n", !!encoder->base.crtc, enabled);
WARN(active && !encoder->base.crtc,
I915_WARN(active && !encoder->base.crtc, "active encoder with no crtc\n");
WARN(encoder->connectors_active != active,
I915_WARN(encoder->connectors_active != active, "encoder's computed active state doesn't match tracked active state " "(expected %i, found %i)\n", active, encoder->connectors_active);active = encoder->get_hw_state(encoder, &pipe);
WARN(active != encoder->connectors_active,
I915_WARN(active != encoder->connectors_active, "encoder's hw state doesn't match sw tracking " "(expected %i, found %i)\n", encoder->connectors_active, active);@@ -10804,7 +10804,7 @@ check_encoder_state(struct drm_device *dev) continue;
tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe;
WARN(active && pipe != tracked_pipe,
I915_WARN(active && pipe != tracked_pipe, "active encoder's pipe doesn't match" "(expected %i, found %i)\n", tracked_pipe, pipe);@@ -10829,7 +10829,7 @@ check_crtc_state(struct drm_device *dev) DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.base.id);
WARN(crtc->active && !crtc->base.enabled,
I915_WARN(crtc->active && !crtc->base.enabled, "active crtc, but not enabled in sw tracking\n");for_each_intel_encoder(dev, encoder) {
@@ -10840,10 +10840,10 @@ check_crtc_state(struct drm_device *dev) active = true; }
WARN(active != crtc->active,
I915_WARN(active != crtc->active, "crtc's computed active state doesn't match tracked active state " "(expected %i, found %i)\n", active, crtc->active);
WARN(enabled != crtc->base.enabled,
I915_WARN(enabled != crtc->base.enabled, "crtc's computed enabled state doesn't match tracked enabled state " "(expected %i, found %i)\n", enabled, crtc->base.enabled);@@ -10863,13 +10863,13 @@ check_crtc_state(struct drm_device *dev) encoder->get_config(encoder, &pipe_config); }
WARN(crtc->active != active,
I915_WARN(crtc->active != active, "crtc active state doesn't match with hw state " "(expected %i, found %i)\n", crtc->active, active);if (active && !intel_pipe_config_compare(dev, &crtc->config, &pipe_config)) {
WARN(1, "pipe state doesn't match!\n");
I915_WARN(1, "pipe state doesn't match!\n"); intel_dump_pipe_config(crtc, &pipe_config, "[hw state]"); intel_dump_pipe_config(crtc, &crtc->config,@@ -10897,14 +10897,14 @@ check_shared_dpll_state(struct drm_device *dev)
active = pll->get_hw_state(dev_priv, pll, &dpll_hw_state);
WARN(pll->active > hweight32(pll->config.crtc_mask),
I915_WARN(pll->active > hweight32(pll->config.crtc_mask), "more active pll users than references: %i vs %i\n", pll->active, hweight32(pll->config.crtc_mask));
WARN(pll->active && !pll->on,
I915_WARN(pll->active && !pll->on, "pll in active use but not on in sw tracking\n");
WARN(pll->on && !pll->active,
I915_WARN(pll->on && !pll->active, "pll in on but not on in use in sw tracking\n");
WARN(pll->on != active,
I915_WARN(pll->on != active, "pll on state mismatch (expected %i, found %i)\n", pll->on, active);@@ -10914,14 +10914,14 @@ check_shared_dpll_state(struct drm_device *dev) if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) active_crtcs++; }
WARN(pll->active != active_crtcs,
I915_WARN(pll->active != active_crtcs, "pll active crtcs mismatch (expected %i, found %i)\n", pll->active, active_crtcs);
WARN(hweight32(pll->config.crtc_mask) != enabled_crtcs,
I915_WARN(hweight32(pll->config.crtc_mask) != enabled_crtcs, "pll enabled crtcs mismatch (expected %i, found %i)\n", hweight32(pll->config.crtc_mask), enabled_crtcs);
WARN(pll->on && memcmp(&pll->config.hw_state, &dpll_hw_state,
I915_WARN(pll->on && memcmp(&pll->config.hw_state, &dpll_hw_state, sizeof(dpll_hw_state)), "pll hw state mismatch\n");diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 5cecc20..6d5bf6e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1558,7 +1558,7 @@ void intel_edp_panel_vdd_on(struct intel_dp *intel_dp) vdd = edp_panel_vdd_on(intel_dp); pps_unlock(intel_dp);
- WARN(!vdd, "eDP port %c VDD already requested on\n",
- I915_WARN(!vdd, "eDP port %c VDD already requested on\n", port_name(dp_to_dig_port(intel_dp)->port));
}
@@ -1642,7 +1642,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) if (!is_edp(intel_dp)) return;
- WARN(!intel_dp->want_panel_vdd, "eDP port %c VDD not forced on",
I915_WARN(!intel_dp->want_panel_vdd, "eDP port %c VDD not forced on", port_name(dp_to_dig_port(intel_dp)->port));
intel_dp->want_panel_vdd = false;
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index f5a78d5..770c5f4 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -633,7 +633,7 @@ static void check_power_well_state(struct drm_i915_private *dev_priv, return;
mismatch:
- WARN(1, "state mismatch for '%s' (always_on %d hw state %d use-count %d disable_power_well %d\n",
- I915_WARN(1, "state mismatch for '%s' (always_on %d hw state %d use-count %d disable_power_well %d\n", power_well->name, power_well->always_on, enabled, power_well->count, i915.disable_power_well);
}
2.1.0
On Fri, Dec 12, 2014 at 08:17:23AM +0100, Daniel Vetter wrote:
On Thu, Dec 11, 2014 at 06:18:12PM -0500, Rob Clark wrote:
Many distro's have mechanism in place to collect and automatically file bugs for failed WARN()s. And since every third line in i915 is a WARN() it generates quite a lot of noise which is somewhat disconcerting to the end user.
Separate out the internal hw-is-in-the-state-I-expected checks into I915_WARN()s and allow configuration via i915.verbose_checks module param about whether this will generate a full blown stacktrace or just DRM_ERROR().
Signed-off-by: Rob Clark robdclark@gmail.com
Yeah I guess makes sense, although I still claim that these are as much "we've lost track of shit" bugs as when a refcount underflows or a pointer is NULL when it shouldn't. But I also agree that we've done a stellar job this year at not locking at these bugs, so meh.
How about making the state checker WARNs conditional on drm.debug&2? The premise is that they are often tantalising unhelpful without the debug log, so only show them when we have both? -Chris
On Fri, 12 Dec 2014, Chris Wilson chris@chris-wilson.co.uk wrote:
On Fri, Dec 12, 2014 at 08:17:23AM +0100, Daniel Vetter wrote:
On Thu, Dec 11, 2014 at 06:18:12PM -0500, Rob Clark wrote:
Many distro's have mechanism in place to collect and automatically file bugs for failed WARN()s. And since every third line in i915 is a WARN()
Come on Rob, that's not necessary.
it generates quite a lot of noise which is somewhat disconcerting to the end user.
Separate out the internal hw-is-in-the-state-I-expected checks into I915_WARN()s and allow configuration via i915.verbose_checks module param about whether this will generate a full blown stacktrace or just DRM_ERROR().
Signed-off-by: Rob Clark robdclark@gmail.com
Yeah I guess makes sense, although I still claim that these are as much "we've lost track of shit" bugs as when a refcount underflows or a pointer is NULL when it shouldn't. But I also agree that we've done a stellar job this year at not locking at these bugs, so meh.
How about making the state checker WARNs conditional on drm.debug&2? The premise is that they are often tantalising unhelpful without the debug log, so only show them when we have both?
As I suggested in an internal mail just days ago, make the checks emit a DRM_ERROR normally (but do log something!), and a WARN if drm.debug & DRM_UT_KMS (or DRM_UT_DRIVER like Chris suggests) holds.
I object to adding new kernel parameters for this. We have enough, and our bug fixing efforts really don't need another round of "oh please *also* enable this new parameter we added".
BR, Jani.
-Chris
-- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Dec 12, 2014 at 5:57 AM, Jani Nikula jani.nikula@linux.intel.com wrote:
On Fri, 12 Dec 2014, Chris Wilson chris@chris-wilson.co.uk wrote:
On Fri, Dec 12, 2014 at 08:17:23AM +0100, Daniel Vetter wrote:
On Thu, Dec 11, 2014 at 06:18:12PM -0500, Rob Clark wrote:
Many distro's have mechanism in place to collect and automatically file bugs for failed WARN()s. And since every third line in i915 is a WARN()
Come on Rob, that's not necessary.
sorry, I didn't intend that to be mean, so much as a bit tongue-in-cheek :-P
i915 currently leads the rhel7 and fedora abrt leaderboard, but I realized if you divide the # of abrts by the # of WARN's in the driver code, i915 looks a lot better :-P
it generates quite a lot of noise which is somewhat disconcerting to the end user.
Separate out the internal hw-is-in-the-state-I-expected checks into I915_WARN()s and allow configuration via i915.verbose_checks module param about whether this will generate a full blown stacktrace or just DRM_ERROR().
Signed-off-by: Rob Clark robdclark@gmail.com
Yeah I guess makes sense, although I still claim that these are as much "we've lost track of shit" bugs as when a refcount underflows or a pointer is NULL when it shouldn't. But I also agree that we've done a stellar job this year at not locking at these bugs, so meh.
How about making the state checker WARNs conditional on drm.debug&2? The premise is that they are often tantalising unhelpful without the debug log, so only show them when we have both?
As I suggested in an internal mail just days ago, make the checks emit a DRM_ERROR normally (but do log something!), and a WARN if drm.debug & DRM_UT_KMS (or DRM_UT_DRIVER like Chris suggests) holds.
I object to adding new kernel parameters for this. We have enough, and our bug fixing efforts really don't need another round of "oh please *also* enable this new parameter we added".
I did kinda want to keep it as a separate param (or at least a separate drm.debug bit, but I preferred a param as we could more easily keep the default to 'true'), since for rawhide and people running their own kernels I do still want to get this "things-are-not-quite-in-the-state-I-expected" feedback to you. But at any rate, as long as we separate out the hw-state-check WARNs from the actually something-is-about-to-catch-fire WARNs, it is easy enough to patch the macro definitions in a distro kernel.
I had some half-baked idea that in the DRM_ERROR case, the message dumped out should contain some string we can search for, so that we could eventually have some "enable sending anonymous driver feedback" type option in the distro, which would still scan the kernel logs for this string and upload feedback via some non-abrt mechanism. (And so, in case the user is actually seeing a problem, when we ask them to attach logs, we still easily see this information. I think in most cases the full callstack doesn't add too much value, so much as knowing what assumptions were broken.) No real idea how that would work from the infrastructure side, so I didn't try to add anything yet, but seems like it could be useful.
BR, -R
BR, Jani.
-Chris
-- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Jani Nikula, Intel Open Source Technology Center
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Jani Nikula -
Rob Clark