[v2] drm/i915: Try to sanitize bogus DPLL state left over by broken SNB BIOSen

Submitted by Ville Syrjala on Jan. 11, 2019, 5:49 p.m.

Details

Message ID 20190111174950.10681-1-ville.syrjala@linux.intel.com
State Accepted
Commit 7bed8adcd9f86231bb69bbc02f88ad89330f99e3
Headers show
Series "drm/i915: Try to sanitize bogus DPLL state left over by broken SNB BIOSen" ( rev: 2 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ville Syrjala Jan. 11, 2019, 5:49 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

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.

v2: Fix potential NULL deref (kbuild test robot)
    Constify has_bogus_dpll_config()

Cc: stable@vger.kernel.org # v4.20+
Cc: Daniel Kamil Kozar <dkk089@gmail.com>
Reported-by: Daniel Kamil Kozar <dkk089@gmail.com>
Tested-by: Daniel Kamil Kozar <dkk089@gmail.com>
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ä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5dc0de89c49e..6cd6048b4731 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15438,16 +15438,45 @@  static void intel_sanitize_crtc(struct intel_crtc *crtc,
 	}
 }
 
+static bool has_bogus_dpll_config(const 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->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 (crtc_state && 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) {
@@ -15458,16 +15487,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 = &encoder->base;
+
 			if (encoder->disable)
-				encoder->disable(encoder, to_intel_crtc_state(crtc_state), connector->base.state);
+				encoder->disable(encoder, crtc_state,
+						 connector->base.state);
 			if (encoder->post_disable)
-				encoder->post_disable(encoder, to_intel_crtc_state(crtc_state), connector->base.state);
+				encoder->post_disable(encoder, crtc_state,
+						      connector->base.state);
+
+			connector->base.state->best_encoder = best_encoder;
 		}
 		encoder->base.crtc = NULL;
 

Comments

The patch look ok to me.

On Fri, 2019-01-11 at 19:49 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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.
> 
> v2: Fix potential NULL deref (kbuild test robot)
>     Constify has_bogus_dpll_config()
> 
> Cc: stable@vger.kernel.org # v4.20+
> Cc: Daniel Kamil Kozar <dkk089@gmail.com>
> Reported-by: Daniel Kamil Kozar <dkk089@gmail.com>
> Tested-by: Daniel Kamil Kozar <dkk089@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109245
> Fixes: 516a49cc1946 ("drm/i915: Fix assert_plane() warning on bootup
> with external display")

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++++++++++++
> ----
>  1 file changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 5dc0de89c49e..6cd6048b4731 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15438,16 +15438,45 @@ static void intel_sanitize_crtc(struct
> intel_crtc *crtc,
>  	}
>  }
>  
> +static bool has_bogus_dpll_config(const 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->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 (crtc_state && 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) {
> @@ -15458,16 +15487,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 = &encoder-
> >base;
> +
>  			if (encoder->disable)
> -				encoder->disable(encoder,
> to_intel_crtc_state(crtc_state), connector->base.state);
> +				encoder->disable(encoder, crtc_state,
> +						 connector-
> >base.state);
>  			if (encoder->post_disable)
> -				encoder->post_disable(encoder,
> to_intel_crtc_state(crtc_state), connector->base.state);
> +				encoder->post_disable(encoder,
> crtc_state,
> +						      connector-
> >base.state);
> +
> +			connector->base.state->best_encoder =
> best_encoder;
>  		}
>  		encoder->base.crtc = NULL;
>
On Mon, Jan 28, 2019 at 09:30:35AM +0000, Kahola, Mika wrote:
> The patch look ok to me.
> 
> On Fri, 2019-01-11 at 19:49 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > 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.
> > 
> > v2: Fix potential NULL deref (kbuild test robot)
> >     Constify has_bogus_dpll_config()
> > 
> > Cc: stable@vger.kernel.org # v4.20+
> > Cc: Daniel Kamil Kozar <dkk089@gmail.com>
> > Reported-by: Daniel Kamil Kozar <dkk089@gmail.com>
> > Tested-by: Daniel Kamil Kozar <dkk089@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109245
> > Fixes: 516a49cc1946 ("drm/i915: Fix assert_plane() warning on bootup
> > with external display")
> 
> Reviewed-by: Mika Kahola <mika.kahola@intel.com>

Thanks. Pushed to dinq.

> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++++++++++++
> > ----
> >  1 file changed, 44 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 5dc0de89c49e..6cd6048b4731 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15438,16 +15438,45 @@ static void intel_sanitize_crtc(struct
> > intel_crtc *crtc,
> >  	}
> >  }
> >  
> > +static bool has_bogus_dpll_config(const 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->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 (crtc_state && 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) {
> > @@ -15458,16 +15487,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 = &encoder-
> > >base;
> > +
> >  			if (encoder->disable)
> > -				encoder->disable(encoder,
> > to_intel_crtc_state(crtc_state), connector->base.state);
> > +				encoder->disable(encoder, crtc_state,
> > +						 connector-
> > >base.state);
> >  			if (encoder->post_disable)
> > -				encoder->post_disable(encoder,
> > to_intel_crtc_state(crtc_state), connector->base.state);
> > +				encoder->post_disable(encoder,
> > crtc_state,
> > +						      connector-
> > >base.state);
> > +
> > +			connector->base.state->best_encoder =
> > best_encoder;
> >  		}
> >  		encoder->base.crtc = NULL;
> >