Patchwork drm/i915: sync dp link status checks against atomic commmits

login
register
mail settings
Submitter Daniel Vetter
Date Nov. 8, 2017, 12:57 p.m.
Message ID <20171108125750.1663-1-daniel.vetter@ffwll.ch>
Download mbox | patch
Permalink /patch/10048463/
State New
Headers show

Comments

Daniel Vetter - Nov. 8, 2017, 12:57 p.m.
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.

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@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@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@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 6 deletions(-)
Ville Syrjälä - Nov. 8, 2017, 1:04 p.m.
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.

> 
> 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@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@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@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
Daniel Vetter - Nov. 8, 2017, 1:11 p.m.
On Wed, Nov 08, 2017 at 03:04:58PM +0200, Ville Syrjälä wrote:
> 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.

Why? One of the patch series Maarten mentioned says there's a deadlock
with dp aux, but it seems to be all just fine when CI retrains.

And even if there is a deadlock, it's there already, so not sure why we
need to block this bugfix on a different bugfix. Which seems to be what
you're suggesting here (but it's a bit sparse).
-Daniel

> > 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@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@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@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
> 
> -- 
> Ville Syrjälä
> Intel OTC
Maarten Lankhorst - Nov. 8, 2017, 1:13 p.m.
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@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@intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@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@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
Ville Syrjälä - Nov. 8, 2017, 1:26 p.m.
On Wed, Nov 08, 2017 at 02:11:46PM +0100, Daniel Vetter wrote:
> On Wed, Nov 08, 2017 at 03:04:58PM +0200, Ville Syrjälä wrote:
> > 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.
> 
> Why? One of the patch series Maarten mentioned says there's a deadlock
> with dp aux, but it seems to be all just fine when CI retrains.

I guess the deadlock is possible only with MST, maybe. Currently MST
has it's own copy of the retraining code without the locks.

At one point I started to rewrite the entire sink irq handling into a
much nicer shape, also unifying the approach between MST and SST. IIRC
I did still make the mistake of having some kind of higher level MST
vs. SST split, but I think the proper solution is to combine the two
almost entirely. I think we should even be using the ESI registers
with SST for DPCD 1.2+. Currently we use ESI for MST and non-ESI for
SST. Sadly I've not found the time to continue that work (same story
with the "shutdown displays prior to rebooting to keep Dell MST
monitors working" thing).

> 
> And even if there is a deadlock, it's there already, so not sure why we
> need to block this bugfix on a different bugfix. Which seems to be what
> you're suggesting here (but it's a bit sparse).

I guess what I'm really saying is that we shouldn't stop here.

> -Daniel
> 
> > > 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@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@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@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
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter - Nov. 8, 2017, 1:28 p.m.
On Wed, Nov 08, 2017 at 03:26:15PM +0200, Ville Syrjälä wrote:
> On Wed, Nov 08, 2017 at 02:11:46PM +0100, Daniel Vetter wrote:
> > On Wed, Nov 08, 2017 at 03:04:58PM +0200, Ville Syrjälä wrote:
> > > 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.
> > 
> > Why? One of the patch series Maarten mentioned says there's a deadlock
> > with dp aux, but it seems to be all just fine when CI retrains.
> 
> I guess the deadlock is possible only with MST, maybe. Currently MST
> has it's own copy of the retraining code without the locks.
> 
> At one point I started to rewrite the entire sink irq handling into a
> much nicer shape, also unifying the approach between MST and SST. IIRC
> I did still make the mistake of having some kind of higher level MST
> vs. SST split, but I think the proper solution is to combine the two
> almost entirely. I think we should even be using the ESI registers
> with SST for DPCD 1.2+. Currently we use ESI for MST and non-ESI for
> SST. Sadly I've not found the time to continue that work (same story
> with the "shutdown displays prior to rebooting to keep Dell MST
> monitors working" thing).

Yeah, merging sst and mst more definitely sounds like a good idea. I've
also been toying with it since forever.

> > And even if there is a deadlock, it's there already, so not sure why we
> > need to block this bugfix on a different bugfix. Which seems to be what
> > you're suggesting here (but it's a bit sparse).
> 
> I guess what I'm really saying is that we shouldn't stop here.

Fully agreed.
-Daniel

> 
> > -Daniel
> > 
> > > > 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@intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@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@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
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC
Navare, Manasi D - Nov. 8, 2017, 6:09 p.m.
On Wed, Nov 08, 2017 at 02:28:23PM +0100, Daniel Vetter wrote:
> On Wed, Nov 08, 2017 at 03:26:15PM +0200, Ville Syrjälä wrote:
> > On Wed, Nov 08, 2017 at 02:11:46PM +0100, Daniel Vetter wrote:
> > > On Wed, Nov 08, 2017 at 03:04:58PM +0200, Ville Syrjälä wrote:
> > > > 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.
> > > 
> > > Why? One of the patch series Maarten mentioned says there's a deadlock
> > > with dp aux, but it seems to be all just fine when CI retrains.

This retraining here would be not because of the training failing in the commit but
when we get a short pulse right so when the link was already up and running?

> > 
> > I guess the deadlock is possible only with MST, maybe. Currently MST
> > has it's own copy of the retraining code without the locks.
> > 
> > At one point I started to rewrite the entire sink irq handling into a
> > much nicer shape, also unifying the approach between MST and SST. IIRC
> > I did still make the mistake of having some kind of higher level MST
> > vs. SST split, but I think the proper solution is to combine the two
> > almost entirely. I think we should even be using the ESI registers
> > with SST for DPCD 1.2+. Currently we use ESI for MST and non-ESI for
> > SST. Sadly I've not found the time to continue that work (same story
> > with the "shutdown displays prior to rebooting to keep Dell MST
> > monitors working" thing).
> 
> Yeah, merging sst and mst more definitely sounds like a good idea. I've
> also been toying with it since forever.
> 
> > > And even if there is a deadlock, it's there already, so not sure why we
> > > need to block this bugfix on a different bugfix. Which seems to be what
> > > you're suggesting here (but it's a bit sparse).
> > 
> > I guess what I'm really saying is that we shouldn't stop here.
> 
> Fully agreed.
> -Daniel
> 
> > 
> > > -Daniel
> > > 
> > > > > 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@intel.com>
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@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@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;

I dont think we need intel_encoder, can we remove this?

Manasi

> > > > > +	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
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel OTC
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Navare, Manasi D - Nov. 8, 2017, 8:52 p.m.
On Wed, Nov 08, 2017 at 10:09:32AM -0800, Manasi Navare wrote:
> On Wed, Nov 08, 2017 at 02:28:23PM +0100, Daniel Vetter wrote:
> > On Wed, Nov 08, 2017 at 03:26:15PM +0200, Ville Syrjälä wrote:
> > > On Wed, Nov 08, 2017 at 02:11:46PM +0100, Daniel Vetter wrote:
> > > > On Wed, Nov 08, 2017 at 03:04:58PM +0200, Ville Syrjälä wrote:
> > > > > 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.
> > > > 
> > > > Why? One of the patch series Maarten mentioned says there's a deadlock
> > > > with dp aux, but it seems to be all just fine when CI retrains.
> 
> This retraining here would be not because of the training failing in the commit but
> when we get a short pulse right so when the link was already up and running?
> 
> > > 
> > > I guess the deadlock is possible only with MST, maybe. Currently MST
> > > has it's own copy of the retraining code without the locks.
> > > 
> > > At one point I started to rewrite the entire sink irq handling into a
> > > much nicer shape, also unifying the approach between MST and SST. IIRC
> > > I did still make the mistake of having some kind of higher level MST
> > > vs. SST split, but I think the proper solution is to combine the two
> > > almost entirely. I think we should even be using the ESI registers
> > > with SST for DPCD 1.2+. Currently we use ESI for MST and non-ESI for
> > > SST. Sadly I've not found the time to continue that work (same story
> > > with the "shutdown displays prior to rebooting to keep Dell MST
> > > monitors working" thing).
> > 
> > Yeah, merging sst and mst more definitely sounds like a good idea. I've
> > also been toying with it since forever.
> > 
> > > > And even if there is a deadlock, it's there already, so not sure why we
> > > > need to block this bugfix on a different bugfix. Which seems to be what
> > > > you're suggesting here (but it's a bit sparse).
> > > 
> > > I guess what I'm really saying is that we shouldn't stop here.
> > 
> > Fully agreed.
> > -Daniel
> > 
> > > 
> > > > -Daniel
> > > > 
> > > > > > 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@intel.com>
> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@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@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;
> 
> I dont think we need intel_encoder, can we remove this?
> 
> Manasi
>

Sorry didnt realize we still need to use intel_encoder in the DRM_DEBUG_KMS.

Manasi
 
> > > > > > +	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
> > > > > 
> > > > > -- 
> > > > > Ville Syrjälä
> > > > > Intel OTC
> > > > 
> > > > -- 
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

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;