[Intel-gfx] [PATCH] drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.

[Pandiyan, Dhinakaran]

On Wed, 2018-03-14 at 15:35 +0200, Ville Syrjälä wrote:
> On Tue, Mar 13, 2018 at 10:48:25PM -0700, Dhinakaran Pandiyan wrote:
> > If bios sets up an MST output and hardware state readout code sees this is
> > an SST configuration, when disabling the encoder we end up calling
> > ->post_disable_dp() hook instead of the MST version. Consequently, we write
> > to the DP_SET_POWER dpcd to set it D3 state. Further along when we try
> > enable the encoder in MST mode, POWER_UP_PHY transaction fails to power up
> > the MST hub. This results in continuous link training failures which keep
> > the system busy delaying boot. We could identify bios MST boot discrepancy
> > and handle it accordingly but a simple way to solve this is to write to the
> > DP_SET_POWER dpcd for MST too.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105470
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Jani Nikula <jani.nikula at intel.com>

Reported-by: Laura Abbott <labbott at redhat.com>
Cc: stable at vger.kernel.org
Fixes: 5ea2355a100a ("drm/i915/mst: Use MST sideband message
transactions for dpms control")

> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index dbcf1a0586f9..8c2d778560f0 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2205,8 +2205,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >  		intel_prepare_dp_ddi_buffers(encoder, crtc_state);
> >  
> >  	intel_ddi_init_dp_buf_reg(encoder);
> > -	if (!is_mst)
> > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > +	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> 
> The spec is perhaps a bit unclear on this. 
> 
> "If the message is sent as a path request, all DP nodes from the source
>  immediate downstream device and the targeted DP node will be placed in
>  the D0 power state."
> 
> Doesn't quite tell me whether the immediate downstream device is
> included in that list of nodes.
> 
> However the spec goes on to say
> "Each nodes immediate upstream device will use Native AUX writes to the
>  SET_POWER & SET_DP_PWR_VOLTAGE register (DPCD Address 00600h) to set
>  the power state of the downstream node."
> 
> and since we are the immediate upstream for that device I guess it makes
> sense that we should still do the DP_SET_POWER manually.
> 
> But I have to wonder what the original issue was before we started to use
> POWER_UP/DOWN_PHY. I suppose somehow some further downstream device
> wasn't in D0 when we needed it.

Correct, sinks farther downstream weren't lighting up.

>  But that is a bit weird as I believe all
> devices should really start up in D0.
> 
> Anyways based on my reading of the spec I can justify this so
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 

Thanks for the review.

> I presume we want cc:stable + fixes: on this?

Yeah, I suppose we should wait for the reporter to confirm that this
indeed fixes the bug. It does fix the problem on the Thinkpad laptop +
dock I tested it on.


> 
> >  	intel_dp_start_link_train(intel_dp);
> >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >  		intel_dp_stop_link_train(intel_dp);
> > @@ -2304,14 +2303,12 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> >  	struct intel_dp *intel_dp = &dig_port->dp;
> > -	bool is_mst = intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST);
> >  
> >  	/*
> >  	 * Power down sink before disabling the port, otherwise we end
> >  	 * up getting interrupts from the sink on detecting link loss.
> >  	 */
> > -	if (!is_mst)
> > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > +	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >  
> >  	intel_disable_ddi_buf(encoder);
> >  
> > -- 
> > 2.14.1
>