| Message ID | 20180613160553.11664-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> but I really don't know if this is the right thing to do. Your call. (And your responsibility to fix the regressions! ;) > 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;
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
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{ 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;