[Intel-gfx] [PATCH] v2 drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Jan-Marek Glogowski]

Am August 7, 2018 7:33:12 PM UTC schrieb Lyude Paul <lyude at redhat.com>:
>On Mon, 2018-08-06 at 12:25 +0200, Jan-Marek Glogowski wrote:
>> This re-applies the workaround for "some DP sinks, [which] are a
>> little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
>> quality check unconditionally during long pulse").
>> It makes the secondary AOC E2460P monitor connected via DP to an
>> acer Veriton N4640G usable again.
>> 
>> This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
>> DP link retraining into the ->post_hotplug() hook")
>> 
>> Signed-off-by: Jan-Marek Glogowski <glogow at fbihome.de>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 46
>++++++++++++++++++++++++------------
>> -----
>>  1 file changed, 27 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index 8e0e14b..7e6f8a5 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4333,18 +4333,6 @@ intel_dp_needs_link_retrain(struct intel_dp
>> *intel_dp)
>>  	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
>>  }
>> 
>> -/*
>> - * If display is now connected check links status,
>> - * there has been known issues of link loss triggering
>> - * long pulse.
>> - *
>> - * Some sinks (eg. ASUS PB287Q) seem to perform some
>> - * weird HPD ping pong during modesets. So we can apparently
>> - * end up with HPD going low during a modeset, and then
>> - * going back up soon after. And once that happens we must
>> - * retrain the link to get a picture. That's in case no
>> - * userspace component reacted to intermittent HPD dip.
>> - */
>>  int intel_dp_retrain_link(struct intel_encoder *encoder,
>>  			  struct drm_modeset_acquire_ctx *ctx)
>>  {
>> @@ -4361,10 +4349,12 @@ int intel_dp_retrain_link(struct
>intel_encoder
>> *encoder,
>>  	if (!connector || connector->base.status !=
>> connector_status_connected)
>>  		return 0;
>> 
>> -	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex,
>> -			       ctx);
>> -	if (ret)
>> -		return ret;
>> +	if (ctx) {
>> +		ret = drm_modeset_lock
>> +			(&dev_priv->drm.mode_config.connection_mutex, ctx);
>> +		if (ret)
>> +			return ret;
>> +	}
>...why are we skipping locking anything if there isn't a
>drm_modeset_acquire_ctx passed to us? And additionally, why wouldn't
>there be
>an acquisition context passed to us? We always need to be holding the
>connection mutex and the respective CRTC lock whenever we're
>retraining.

The original patch added all the locking to intel_dp_retrain_link. intel_dp_long_pulse is just called from intel_dp_detect, which takes the drm_modeset_lock depending on a valid crtc. I just assumed this is still sufficient, so for this case we don't need to acquire the lock again. If this is not sufficient, we can forward the ctx, state etc. from intel_dp_detect. I didn't check, if these values are the same values that intel_dp_retrain_link collects using different calls.

There is also the "WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));" in intel_dp_long_pulse, which suggests we should also already have the connection_mutex when calling intel_dp_retrain_link later.

All my assumptions are based on reading the original patch and a little bit of the surrounding code.

This patch is just a minimal change, which "works for me". Not sure if all the locking should be moved back to the retrain call sites, to make it more explicit?

>> 
>>  	conn_state = connector->base.state;
>> 
>> @@ -4372,9 +4362,11 @@ int intel_dp_retrain_link(struct intel_encoder
>> *encoder,
>>  	if (!crtc)
>>  		return 0;
>> 
>> -	ret = drm_modeset_lock(&crtc->base.mutex, ctx);
>> -	if (ret)
>> -		return ret;
>> +	if (ctx) {
>> +		ret = drm_modeset_lock(&crtc->base.mutex, ctx);
>> +		if (ret)
>> +			return ret;
>> +	}
>> 
>>  	crtc_state = to_intel_crtc_state(crtc->base.state);
>> 
>> @@ -4982,6 +4974,22 @@ intel_dp_long_pulse(struct intel_connector
>> *connector)
>>  		 */
>>  		status = connector_status_disconnected;
>>  		goto out;
>> +	} else {
>> +		/*
>> +		 * If display is now connected check links status,
>> +		 * there has been known issues of link loss triggering
>> +		 * long pulse.
>> +		 *
>> +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
>> +		 * weird HPD ping pong during modesets. So we can apparently
>> +		 * end up with HPD going low during a modeset, and then
>> +		 * going back up soon after. And once that happens we must
>> +		 * retrain the link to get a picture. That's in case no
>> +		 * userspace component reacted to intermittent HPD dip.
>> +		 */
>> +		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)-
>> >base;
>> +
>> +		intel_dp_retrain_link(encoder, NULL);
>>  	}
>> 
>>  	/*