[Intel-gfx] [PATCH] drm/i915: sync dp link status checks against atomic commmits

[Maarten Lankhorst]

Op 08-11-17 om 14:04 schreef Ville Syrjälä:
> On Wed, Nov 08, 2017 at 01:57:50PM +0100, Daniel Vetter wrote:
>> Two bits:
>> - check actual atomic state, the legacy stuff can only be looked at
>>   from within the atomic_commit_tail function, since it's only
>>   protected by ordering and not by any locks.
>>
>> - Make sure we don't wreak the work an ongoing nonblocking commit is
>>   doing.
> I still think we need to move the retraining to the hotplug work.
But lets not block a fix for that. :)

Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> v2: We need the crtc lock too, because a plane update might change it
>> without having to acquire the connection_mutex (Maarten). Use
>> Maarten's changes for this locking, while keeping the logic that uses
>> the connection->commit->hw_done signal for syncing with nonblocking
>> commits.
>>
>> Cc: Manasi Navare <manasi.d.navare at intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=103336
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99272
>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 50 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index d27c0145ac91..7cd7ab4fb431 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4319,6 +4319,8 @@ static void
>>  intel_dp_check_link_status(struct intel_dp *intel_dp)
>>  {
>>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>> +	struct drm_connector_state *conn_state =
>> +		intel_dp->attached_connector->base.state;
>>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>  	u8 link_status[DP_LINK_STATUS_SIZE];
>>  
>> @@ -4329,10 +4331,15 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>  		return;
>>  	}
>>  
>> -	if (!intel_encoder->base.crtc)
>> +	if (!conn_state->crtc)
>>  		return;
>>  
>> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> +	WARN_ON(!drm_modeset_is_locked(&conn_state->crtc->mutex));
>> +
>> +	if (!conn_state->crtc->state->active)
>> +		return;
>> +
>> +	if (!try_wait_for_completion(&conn_state->commit->hw_done))
>>  		return;
>>  
>>  	/*
>> @@ -4368,7 +4375,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>  static bool
>>  intel_dp_short_pulse(struct intel_dp *intel_dp)
>>  {
>> -	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>  	u8 sink_irq_vector = 0;
>>  	u8 old_sink_count = intel_dp->sink_count;
>> @@ -4408,9 +4414,8 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>>  	}
>>  
>> -	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>  	intel_dp_check_link_status(intel_dp);
>> -	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>>  	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
>>  		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
>>  		/* Send a Hotplug Uevent to userspace to start modeset */
>> @@ -4860,8 +4865,19 @@ intel_dp_detect(struct drm_connector *connector,
>>  		      connector->base.id, connector->name);
>>  
>>  	/* If full detect is not performed yet, do a full detect */
>> -	if (!intel_dp->detect_done)
>> +	if (!intel_dp->detect_done) {
>> +		struct drm_crtc *crtc;
>> +		int ret;
>> +
>> +		crtc = connector->state->crtc;
>> +		if (crtc) {
>> +			ret = drm_modeset_lock(&crtc->mutex, ctx);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +
>>  		status = intel_dp_long_pulse(intel_dp->attached_connector);
>> +	}
>>  
>>  	intel_dp->detect_done = false;
>>  
>> @@ -5146,10 +5162,38 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>  	}
>>  
>>  	if (!intel_dp->is_mst) {
>> +		struct drm_modeset_acquire_ctx ctx;
>> +		struct drm_connector *connector = &intel_dp->attached_connector->base;
>> +		struct drm_crtc *crtc;
>> +		int iret;
>> +
>> +		drm_modeset_acquire_init(&ctx, 0);
>> +retry:
>> +		iret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
>> +		if (iret)
>> +			goto err;
>> +
>> +		crtc = connector->state->crtc;
>> +		if (crtc) {
>> +			iret = drm_modeset_lock(&crtc->mutex, &ctx);
>> +			if (iret)
>> +				goto err;
>> +		}
>> +
>>  		if (!intel_dp_short_pulse(intel_dp)) {
>>  			intel_dp->detect_done = false;
>>  			goto put_power;
>>  		}
>> +
>> +err:
>> +		if (iret == -EDEADLK) {
>> +			drm_modeset_backoff(&ctx);
>> +			goto retry;
>> +		}
>> +
>> +		drm_modeset_drop_locks(&ctx);
>> +		drm_modeset_acquire_fini(&ctx);
>> +		WARN(iret, "Acquiring modeset locks failed with %i\n", iret);
>>  	}
>>  
>>  	ret = IRQ_HANDLED;
>> -- 
>> 2.15.0