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

[Laura Abbott]

On 03/14/2018 11:26 AM, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> 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")
> 

Tested-by: Laura Abbott <labbott at redhat.com>

>>> 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
>>