[PATCH 2/3] drm/i915: Wait for pending modesets to complete before trying link training

[Maarten Lankhorst]

Op 09-02-17 om 13:34 schreef Ville Syrjälä:
> On Thu, Feb 09, 2017 at 12:27:12PM +0100, Maarten Lankhorst wrote:
>> Op 26-01-17 om 19:31 schreef Ville Syrjälä:
>>> On Mon, Jan 09, 2017 at 06:06:57PM +0100, Maarten Lankhorst wrote:
>>>> We're protected by the connection_mutex lock against blocking modesets,
>>>> but nonblocking modesets are performed without any locking. This is
>>>> causing WARN in drm_wait_for_vblank and in general it's better to wait
>>>> before modeset completes before trying anything.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_dp.c | 51 +++++++++++++++++++++++++++++++----------
>>>>  1 file changed, 39 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index fb12896bafee..71882d887393 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -4080,24 +4080,38 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>>>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>>>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>  	u8 link_status[DP_LINK_STATUS_SIZE];
>>>> +	struct drm_modeset_acquire_ctx ctx;
>>>> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>>>> +	int ret;
>>>> +	struct drm_crtc *crtc;
>>>>  
>>>> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>>> +	drm_modeset_acquire_init(&ctx, 0);
>>>> +
>>>> +retry:
>>>> +	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
>>>> +	if (ret)
>>>> +		goto out;
>>>>  
>>>>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>>>  		DRM_ERROR("Failed to get link status\n");
>>>> -		return;
>>>> +		goto out;
>>>>  	}
>>>>  
>>>> -	if (!intel_encoder->base.crtc)
>>>> -		return;
>>>> +	crtc = intel_connector->base.state->crtc;
>>>> +	if (!crtc)
>>>> +		goto out;
>>>>  
>>>> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>>>> -		return;
>>>> +	ret = drm_modeset_lock(&crtc->mutex, &ctx);
>>>> +	if (ret)
>>>> +		goto out;
>>>> +
>>>> +	if (!crtc->state->active)
>>>> +		goto out;
>>>>  
>>>>  	/* FIXME: we need to synchronize this sort of stuff with hardware
>>>>  	 * readout. Currently fast link training doesn't work on boot-up. */
>>>>  	if (!intel_dp->lane_count)
>>>> -		return;
>>>> +		goto out;
>>>>  
>>>>  	/* if link training is requested we should perform it always */
>>>>  	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
>>>> @@ -4105,8 +4119,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>>>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>>>>  			      intel_encoder->base.name);
>>>>  
>>>> +		if (drm_atomic_crtc_needs_modeset(crtc->state)) {
>>>> +			/* wait for atomic modeset to complete */
>>>> +			ret = drm_atomic_helper_wait_for_hw_done(crtc);
>>>> +			if (ret < 0)
>>>> +				DRM_ERROR("Waiting for hw_done timed out\n");
>>> I wonder if we should just skip the hw frobbing in this case. I guess it
>>> might still restore the link if the commit just somehow got stuck
>>> somewhere non-critical.
>> Hm,  we could check if the link training is still required after the wait.
>> Maybe even do the wait unlocked, since it might take an arbitrary amount of time
>> and then retry?
> I'm not sure there's much point in trying to make this fancy. Link
> problems should not happen with any regularity.
Indeed, lets just do the simple thing then.