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

On Thu, 14 Jun 2018, Jani Nikula jani.nikula@linux.intel.com wrote:

On Wed, 13 Jun 2018, Ville Syrjala ville.syrjala@linux.intel.com wrote:

...

From: Ville Syrjälä ville.syrjala@linux.intel.com

Looks like interlaced DP output doesn't work on g4x either. Not all that surprising considering we already established that interlaced DP output is busted on VLV/CHV.

Cc: stable@vger.kernel.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

Reviewed-by: Jani Nikula jani.nikula@intel.com

Oh, did you have a bug reference?

...

---

drivers/gpu/drm/i915/intel_dp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 40ffd9163175..6068986fd985 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1869,7 +1869,7 @@ intel_dp_compute_config(struct intel_encoder *encoder, conn_state->scaling_mode); }

• if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&

• if (HAS_GMCH_DISPLAY(dev_priv) && adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) return false;

@@ -6351,7 +6351,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, drm_connector_init(dev, connector, &intel_dp_connector_funcs, type); drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs);

• if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))

• if (!HAS_GMCH_DISPLAY(dev_priv)) connector->interlace_allowed = true; connector->doublescan_allowed = 0;