Re: [PATCH 2/2] drm/i915: Turn off g4x DP port in .post_disable()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jun 14, 2018 at 03:42:42PM +0300, Jani Nikula wrote:
> On Wed, 13 Jun 2018, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > While Bspec doesn't list a specific sequence for turning off the DP port
> > on g4x we are getting an underrun if the port is disabled in the
> > .disable() hook. Looks like the pipe stops when the port stops, and by
> > that time the plane disable may not have completed yet. Also the plane(s)
> > seem to end up in some wonky state when this happens as they also signal
> > another underrun immediately after we turn them back on during the next
> > enable sequence.
> >
> > We could add a vblank wait in .disable() to avoid wedging the planes,
> > but I assume we're still tripping up the pipe in some way. So it seems
> > better to me to just follow the ILK+ sequence and turn off the DP port
> > in .post_disable() instead. This sequence doesn't seem to suffer from
> > this problem. Could be it was always the intended sequence for DP and
> > the gen4 bspec was just never updated to include it.
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The change for PCH was
> 
> commit 08aff3fe26ae7a0d6f302ac2e1b7e2eb9933cd42
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Mon Aug 18 22:16:09 2014 +0300
> 
>     drm/i915: Move DP port disable to post_disable for pch platforms
> 
> where you explicitly left out g4x per modeset sequence. I guess you
> could reference that in the commit message.
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 26 +++++++++-----------------
> >  1 file changed, 9 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 6068986fd985..5f09a8015c89 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2803,16 +2803,6 @@ static void intel_disable_dp(struct intel_encoder *encoder,
> >  static void g4x_disable_dp(struct intel_encoder *encoder,
> >  			   const struct intel_crtc_state *old_crtc_state,
> >  			   const struct drm_connector_state *old_conn_state)
> > -{
> > -	intel_disable_dp(encoder, old_crtc_state, old_conn_state);
> > -
> > -	/* disable the port before the pipe on g4x */
> > -	intel_dp_link_down(encoder, old_crtc_state);
> > -}
> > -
> > -static void ilk_disable_dp(struct intel_encoder *encoder,
> > -			   const struct intel_crtc_state *old_crtc_state,
> > -			   const struct drm_connector_state *old_conn_state)
> >  {
> >  	intel_disable_dp(encoder, old_crtc_state, old_conn_state);
> >  }
> > @@ -2828,13 +2818,19 @@ static void vlv_disable_dp(struct intel_encoder *encoder,
> >  	intel_disable_dp(encoder, old_crtc_state, old_conn_state);
> >  }
> >  
> > -static void ilk_post_disable_dp(struct intel_encoder *encoder,
> > +static void g4x_post_disable_dp(struct intel_encoder *encoder,
> >  				const struct intel_crtc_state *old_crtc_state,
> >  				const struct drm_connector_state *old_conn_state)
> >  {
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >  	enum port port = encoder->port;
> >  
> > +	/*
> > +	 * Bspec does not list a specific disable sequence for g4x DP.
> > +	 * Follow the ilk+ sequence (disable pipe before the port) for
> > +	 * g4x DP as it does not suffer from underruns like the normal
> > +	 * g4x modeset sequence (disable pipe after the port).
> > +	 */
> >  	intel_dp_link_down(encoder, old_crtc_state);
> >  
> >  	/* Only ilk+ has port A */
> > @@ -6450,15 +6446,11 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
> >  		intel_encoder->enable = vlv_enable_dp;
> >  		intel_encoder->disable = vlv_disable_dp;
> >  		intel_encoder->post_disable = vlv_post_disable_dp;
> > -	} else if (INTEL_GEN(dev_priv) >= 5) {
> > -		intel_encoder->pre_enable = g4x_pre_enable_dp;
> > -		intel_encoder->enable = g4x_enable_dp;
> > -		intel_encoder->disable = ilk_disable_dp;
> > -		intel_encoder->post_disable = ilk_post_disable_dp;
> > -	} else {
> > +	} else{
> 
> Space missing before {.
> 
> The code matches the commit message, so strictly in that sense,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Fixed up the whitespace, amended the commit message a bit with the pch
commit details, and pushed the series to dinq. Thanks for the review.

> 
> but I really don't know if this is the right thing to do. Your
> call. (And your responsibility to fix the regressions! ;)

I'm feeling unusually confident about this one :)

> 
> 
> >  		intel_encoder->pre_enable = g4x_pre_enable_dp;
> >  		intel_encoder->enable = g4x_enable_dp;
> >  		intel_encoder->disable = g4x_disable_dp;
> > +		intel_encoder->post_disable = g4x_post_disable_dp;
> >  	}
> >  
> >  	intel_dig_port->dp.output_reg = output_reg;
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx