Re: [Intel-gfx] [PATCH] drm/i915: Try to sanitize bogus DPLL state left over by broken SNB BIOSen
Hi, [This is an automated email] This commit has been processed because it contains a "Fixes:" tag, fixing commit: 516a49cc1946 drm/i915: Fix assert_plane() warning on bootup with external display. The bot has tested the following trees: v4.20.2. v4.20.2: Failed to apply! Possible dependencies: c84c6fe30302 ("drm/i915: make encoder enable and disable hooks optional") How should we proceed with this patch? -- Thanks, Sasha ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Try to sanitize bogus DPLL state left over by broken SNB BIOSen
Hi Ville, Thank you for the patch! Perhaps something to improve: url: https://github.com/0day-ci/linux/commits/Ville-Syrjala/drm-i915-Try-to-sanitize-bogus-DPLL-state-left-over-by-broken-SNB-BIOSen/20190109-222838 base: git://anongit.freedesktop.org/drm-intel for-linux-next New smatch warnings: drivers/gpu/drm/i915/intel_display.c:15452 has_bogus_dpll_config() warn: variable dereferenced before check 'crtc_state' (see line 15439) drivers/gpu/drm/i915/intel_display.c:15472 intel_sanitize_encoder() error: we previously assumed 'crtc_state' could be null (see line 15469) drivers/gpu/drm/i915/intel_display.c:15487 intel_sanitize_encoder() warn: variable dereferenced before check 'crtc_state' (see line 15472) # https://github.com/0day-ci/linux/commit/5a0056f774727561e522ccde5fe7fe78d343d25f git remote add linux-review https://github.com/0day-ci/linux git remote update linux-review git checkout 5a0056f774727561e522ccde5fe7fe78d343d25f vim +/crtc_state +15452 drivers/gpu/drm/i915/intel_display.c 249293524 Daniel Vetter 2012-07-02 15436 5a0056f77 Ville Syrjälä 2019-01-09 15437 static bool has_bogus_dpll_config(struct intel_crtc_state *crtc_state) 5a0056f77 Ville Syrjälä 2019-01-09 15438 { 5a0056f77 Ville Syrjälä 2019-01-09 @15439 struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); 5a0056f77 Ville Syrjälä 2019-01-09 15440 5a0056f77 Ville Syrjälä 2019-01-09 15441 /* 5a0056f77 Ville Syrjälä 2019-01-09 15442* Some SNB BIOSen (eg. ASUS K53SV) are known to misprogram 5a0056f77 Ville Syrjälä 2019-01-09 15443* the hardware when a high res displays plugged in. DPLL P 5a0056f77 Ville Syrjälä 2019-01-09 15444* divider is zero, and the pipe timings are bonkers. We'll 5a0056f77 Ville Syrjälä 2019-01-09 15445* try to disable everything in that case. 5a0056f77 Ville Syrjälä 2019-01-09 15446* 5a0056f77 Ville Syrjälä 2019-01-09 15447* FIXME would be nice to be able to sanitize this state 5a0056f77 Ville Syrjälä 2019-01-09 15448* without several WARNs, but for now let's take the easy 5a0056f77 Ville Syrjälä 2019-01-09 15449* road. 5a0056f77 Ville Syrjälä 2019-01-09 15450*/ 5a0056f77 Ville Syrjälä 2019-01-09 15451 return IS_GEN(dev_priv, 6) && 5a0056f77 Ville Syrjälä 2019-01-09 @15452 crtc_state && 5a0056f77 Ville Syrjälä 2019-01-09 15453 crtc_state->base.active && 5a0056f77 Ville Syrjälä 2019-01-09 15454 crtc_state->shared_dpll && 5a0056f77 Ville Syrjälä 2019-01-09 15455 crtc_state->port_clock == 0; 5a0056f77 Ville Syrjälä 2019-01-09 15456 } 5a0056f77 Ville Syrjälä 2019-01-09 15457 249293524 Daniel Vetter 2012-07-02 15458 static void intel_sanitize_encoder(struct intel_encoder *encoder) 249293524 Daniel Vetter 2012-07-02 15459 { 70332ac53 Imre Deak 2018-11-01 15460 struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); 249293524 Daniel Vetter 2012-07-02 15461 struct intel_connector *connector; 5a0056f77 Ville Syrjälä 2019-01-09 15462 struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); 5a0056f77 Ville Syrjälä 2019-01-09 15463 struct intel_crtc_state *crtc_state = crtc ? 5a0056f77 Ville Syrjälä 2019-01-09 15464 to_intel_crtc_state(crtc->base.state) : NULL; 249293524 Daniel Vetter 2012-07-02 15465 249293524 Daniel Vetter 2012-07-02 15466 /* We need to check both for a crtc link (meaning that the 249293524 Daniel Vetter 2012-07-02 15467* encoder is active and trying to read from a pipe) and the 249293524 Daniel Vetter 2012-07-02 15468* pipe itself being active. */ 5a0056f77 Ville Syrjälä 2019-01-09 @15469 bool has_active_crtc = crtc_state && 5a0056f77 Ville Syrjälä 2019-01-09 15470 crtc_state->base.active; 5a0056f77 Ville Syrjälä 2019-01-09 15471 5a0056f77 Ville Syrjälä 2019-01-09 @15472 if (has_bogus_dpll_config(crtc_state)) { 5a0056f77 Ville Syrjälä 2019-01-09 15473 DRM_DEBUG_KMS("BIOS has misprogrammed the hardware. Disabling pipe %c\n", 5a0056f77 Ville Syrjälä 2019-01-09 15474 pipe_name(crtc->pipe)); 5a0056f77 Ville Syrjälä 2019-01-09 15475 has_active_crtc = false; 5a0056f77 Ville Syrjälä 2019-01-09 15476 } 249293524 Daniel Vetter 2012-07-02 15477 496b0fc37 Maarten Lankhorst 2016-08-23 15478 connector = intel_encoder_find_connector(encoder); 496b0fc37 Maarten Lankhorst 2016-08-23 15479 if (connector && !has_active_crtc) { 249293524 Daniel Vetter 2012-07-02 15480 DRM_DEBUG_KMS("[ENCODER:%d:%s] has active connectors but no active pipe!\n", 249293524 Daniel Vetter 2012-07-02 15481 encoder->base.base.id, 8e329a039 Jani Nikula 2014-06-03 15482 encoder->base.name); 249293524 Daniel
[Intel-gfx] [PATCH] drm/i915: Try to sanitize bogus DPLL state left over by broken SNB BIOSen
From: Ville Syrjälä Certain SNB machines (eg. ASUS K53SV) seem to have a broken BIOS which misprograms the hardware badly when encountering a suitably high resolution display. The programmed pipe timings are somewhat bonkers and the DPLL is totally misprogrammed (P divider == 0). That will result in atomic commit timeouts as apparently the pipe is sufficiently stuck to not signal vblank interrupts. IIRC something like this was also observed on some other SNB machine years ago (might have been a Dell XPS 8300) but a BIOS update cured it. Sadly looks like this was never fixed for the ASUS K53SV as the latest BIOS (K53SV.320 11/11/2011) is still broken. The quickest way to deal with this seems to be to shut down the pipe+ports+DPLL. Unfortunately doing this during the normal sanitization phase isn't quite soon enough as we already spew several WARNs about the bogus hardware state. But it's better than hanging the boot for a few dozen seconds. Since this is limited to a few old machines it doesn't seem entirely worthwile to try and rework the readout+sanitization code to handle it more gracefully. Cc: sta...@vger.kernel.org # v4.20+ Cc: Daniel Kamil Kozar Reported-by: Daniel Kamil Kozar Tested-by: Daniel Kamil Kozar Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109245 Fixes: 516a49cc1946 ("drm/i915: Fix assert_plane() warning on bootup with external display") Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 51 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 696e6f5680df..ea2e09d36b4d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15435,16 +15435,46 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc, } } +static bool has_bogus_dpll_config(struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); + + /* +* Some SNB BIOSen (eg. ASUS K53SV) are known to misprogram +* the hardware when a high res displays plugged in. DPLL P +* divider is zero, and the pipe timings are bonkers. We'll +* try to disable everything in that case. +* +* FIXME would be nice to be able to sanitize this state +* without several WARNs, but for now let's take the easy +* road. +*/ + return IS_GEN(dev_priv, 6) && + crtc_state && + crtc_state->base.active && + crtc_state->shared_dpll && + crtc_state->port_clock == 0; +} + static void intel_sanitize_encoder(struct intel_encoder *encoder) { struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct intel_connector *connector; + struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); + struct intel_crtc_state *crtc_state = crtc ? + to_intel_crtc_state(crtc->base.state) : NULL; /* We need to check both for a crtc link (meaning that the * encoder is active and trying to read from a pipe) and the * pipe itself being active. */ - bool has_active_crtc = encoder->base.crtc && - to_intel_crtc(encoder->base.crtc)->active; + bool has_active_crtc = crtc_state && + crtc_state->base.active; + + if (has_bogus_dpll_config(crtc_state)) { + DRM_DEBUG_KMS("BIOS has misprogrammed the hardware. Disabling pipe %c\n", + pipe_name(crtc->pipe)); + has_active_crtc = false; + } connector = intel_encoder_find_connector(encoder); if (connector && !has_active_crtc) { @@ -15455,16 +15485,25 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) /* Connector is active, but has no active pipe. This is * fallout from our resume register restoring. Disable * the encoder manually again. */ - if (encoder->base.crtc) { - struct drm_crtc_state *crtc_state = encoder->base.crtc->state; + if (crtc_state) { + struct drm_encoder *best_encoder; DRM_DEBUG_KMS("[ENCODER:%d:%s] manually disabled\n", encoder->base.base.id, encoder->base.name); + + /* avoid oopsing in case the hooks consult best_encoder */ + best_encoder = connector->base.state->best_encoder; + connector->base.state->best_encoder = >base; + if (encoder->disable) - encoder->disable(encoder, to_intel_crtc_state(crtc_state), connector->base.state); + encoder->disable(encoder, crtc_state, +