drm/atomic-helper: always track connector commits, too

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 <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +50 -6

index d27c0145ac91..7cd7ab4fb431 100644
@@ -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;

Signature · 1 line

2.15.0

Quoted 7 lines · Daniel Vetter

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.

Quoted 128 lines

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 <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +50 -6

index d27c0145ac91..7cd7ab4fb431 100644
@@ -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;

Signature · 1 line

2.15.0

Signature · 2 lines

Ville Syrjälä
Intel OTC

Quoted 6 lines · Ville Syrjälä

Quoted 5 lines · Daniel Vetter

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.

Quoted 4 lines

Quoted 2 lines

- 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

Quoted 132 lines

Quoted 128 lines

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 <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +50 -6

index d27c0145ac91..7cd7ab4fb431 100644
@@ -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;

Signature · 1 line

2.15.0

Signature · 2 lines

Ville Syrjälä
Intel OTC

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 7 lines · Daniel Vetter

Quoted 6 lines · Ville Syrjälä

Quoted 5 lines · Daniel Vetter

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.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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

Quoted 3 lines

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.

Quoted 140 lines

-Daniel

Quoted 132 lines

Quoted 128 lines

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 <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +50 -6

index d27c0145ac91..7cd7ab4fb431 100644
@@ -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;

Signature · 1 line

2.15.0

Signature · 2 lines

Ville Syrjälä
Intel OTC

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Signature · 2 lines

Ville Syrjälä
Intel OTC

Quoted 8 lines · Ville Syrjälä

Quoted 7 lines · Daniel Vetter

Quoted 6 lines · Ville Syrjälä

Quoted 5 lines · Daniel Vetter

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.

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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

Quoted 4 lines

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

Quoted 9 lines

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.

Quoted 5 lines

Quoted 3 lines

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

Quoted 19 lines

Quoted 19 lines

-Daniel

Quoted 16 lines

Quoted 16 lines

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 <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 6 deletions(-)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c static void · line 4321

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c +0 -0

index d27c0145ac91..7cd7ab4fb431 100644
@@ -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;

Quoted 109 lines

Quoted 109 lines

Quoted 108 lines

Quoted 104 lines

drivers/gpu/drm/i915/intel_dp.c +50 -6

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

Signature · 1 line

2.15.0

Signature · 2 lines

Ville Syrjälä
Intel OTC

Quoted 8 lines

Quoted 4 lines

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Signature · 2 lines

Ville Syrjälä
Intel OTC

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 9 lines · Daniel Vetter

Quoted 8 lines · Ville Syrjälä

Quoted 7 lines · Daniel Vetter

Quoted 6 lines · Ville Syrjälä

Quoted 5 lines · Daniel Vetter

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.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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

Quoted 45 lines

Quoted 4 lines

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

Quoted 9 lines

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.

Quoted 5 lines

Quoted 3 lines

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

Quoted 19 lines

Quoted 19 lines

-Daniel

Quoted 16 lines

Quoted 16 lines

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 <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 6 deletions(-)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c static void · line 4321

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c +0 -0

index d27c0145ac91..7cd7ab4fb431 100644
@@ -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

Quoted 118 lines

Quoted 117 lines

Quoted 113 lines

Quoted 108 lines

Quoted 104 lines

drivers/gpu/drm/i915/intel_dp.c +50 -6

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

Signature · 1 line

2.15.0

Signature · 2 lines

Ville Syrjälä
Intel OTC

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Signature · 2 lines

Ville Syrjälä
Intel OTC

Quoted 4 lines

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 7 lines

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.

Quoted 125 lines

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 <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +50 -6

index d27c0145ac91..7cd7ab4fb431 100644
@@ -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);
 	}
 

Quoted 3 lines

drivers/gpu/drm/i915/intel_dp.c +0 -0

 	ret = IRQ_HANDLED;

Signature · 1 line

2.15.0

It's useful for syncing async connector work like link retraining.

Cc Manasi Navare <[email protected]>

Cc Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/drm_atomic_helper.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +0 -0

index 71d712f1b56a..ea781d55f2c1 100644

drivers/gpu/drm/drm_atomic_helper.c +3 -5

-		commit = crtc_or_fake_commit(state, old_conn_state->crtc);
+		/* Always track connectors explicitly for e.g. link retraining. */
+		commit = crtc_or_fake_commit(state, new_conn_state->crtc ?: old_conn_state->crtc);
 		if (!commit)
 			return -ENOMEM;
 

@@ -1805,10 +1806,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
 			return -EBUSY;
 
-		/*
-		 * Unlike connectors, always track planes explicitly for
-		 * async pageflip support.
-		 */
+		/* Always track planes explicitly for async pageflip support. */
 		commit = crtc_or_fake_commit(state, new_plane_state->crtc ?: old_plane_state->crtc);
 		if (!commit)
 			return -ENOMEM;

Signature · 1 line

2.15.0

Quoted 15 lines · Daniel Vetter

It's useful for syncing async connector work like link retraining.

Cc Manasi Navare <[email protected]>

Cc Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/drm_atomic_helper.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +0 -0

index 71d712f1b56a..ea781d55f2c1 100644

drivers/gpu/drm/drm_atomic_helper.c int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, · line 1792

Quoted 2 lines

drivers/gpu/drm/drm_atomic_helper.c +0 -0

@@ -1791,7 +1791,8 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		if (new_conn_state->crtc)
 			continue;

This guy says we will never have new_conn_state->crtc below.

Quoted 1 line

drivers/gpu/drm/drm_atomic_helper.c int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, · line 1795

Quoted 3 lines

drivers/gpu/drm/drm_atomic_helper.c +2 -1

-		commit = crtc_or_fake_commit(state, old_conn_state->crtc);
+		/* Always track connectors explicitly for e.g. link retraining. */
+		commit = crtc_or_fake_commit(state, new_conn_state->crtc ?: old_conn_state->crtc);

Quoted 7 lines

drivers/gpu/drm/drm_atomic_helper.c +0 -0

 		if (!commit)
 			return -ENOMEM;

@@ -1805,10 +1806,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
 			return -EBUSY;
 

Quoted 10 lines

drivers/gpu/drm/drm_atomic_helper.c +1 -4

-		/*
-		 * Unlike connectors, always track planes explicitly for
-		 * async pageflip support.
-		 */
+		/* Always track planes explicitly for async pageflip support. */
 		commit = crtc_or_fake_commit(state, new_plane_state->crtc ?: old_plane_state->crtc);
 		if (!commit)
 			return -ENOMEM;

Signature · 1 line

2.15.0

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel