[6/6] drm/i915: Fix up verify_encoder_state

Commit Message

Daniel Vetter March 1, 2017, 9:52 a.m. UTC

The trouble here is that looking at all connector->state in the
verifier isn't good, because that's run from the commit work, which
doesn't hold the connection_mutex. Which means we're only allowed to
look at states in our atomic update.

The simple fix for future proofing would be to switch over to
drm_for_each_connector_in_state, but that has the problem that the
verification then fails if not all connectors are in the state. And we
also need to be careful to check both old and new encoders, and not
screw things up when an encoder gets reassigned.

Note that this isn't the full fix, since we still look at
connector->state. To fix that, we need Maarten's patch series to
switch over to state pointers within drm_atomic_state, but that's a
different series.

v2: Use oldnew iterator (Maarten).

v3: Rebase onto the iter_get/put->iter_begin/end rename.

Cc: Thierry Reding <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
---
 drivers/gpu/drm/i915/intel_display.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

Comments

Maarten Lankhorst March 8, 2017, 9:37 a.m. UTC | #1

Op 01-03-17 om 10:52 schreef Daniel Vetter:
> The trouble here is that looking at all connector->state in the
> verifier isn't good, because that's run from the commit work, which
> doesn't hold the connection_mutex. Which means we're only allowed to
> look at states in our atomic update.
>
> The simple fix for future proofing would be to switch over to
> drm_for_each_connector_in_state, but that has the problem that the
> verification then fails if not all connectors are in the state. And we
> also need to be careful to check both old and new encoders, and not
> screw things up when an encoder gets reassigned.
>
> Note that this isn't the full fix, since we still look at
> connector->state. To fix that, we need Maarten's patch series to
> switch over to state pointers within drm_atomic_state, but that's a
> different series.
>
> v2: Use oldnew iterator (Maarten).
>
> v3: Rebase onto the iter_get/put->iter_begin/end rename.
>
> Cc: Thierry Reding <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>

Reviewed-by: Maarten Lankhorst <[e-mail]>

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f6560ab3531d..958e68430ca3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11898,31 +11898,37 @@  verify_connector_state(struct drm_device *dev,
 }
 
 static void
-verify_encoder_state(struct drm_device *dev)
+verify_encoder_state(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-	struct drm_connector_list_iter conn_iter;
+	struct drm_connector *connector;
+	struct drm_connector_state *old_conn_state, *new_conn_state;
+	int i;
 
 	for_each_intel_encoder(dev, encoder) {
-		bool enabled = false;
+		bool enabled = false, found = false;
 		enum pipe pipe;
 
 		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
 			      encoder->base.base.id,
 			      encoder->base.name);
 
-		drm_connector_list_iter_begin(dev, &conn_iter);
-		for_each_intel_connector_iter(connector, &conn_iter) {
-			if (connector->base.state->best_encoder != &encoder->base)
+		for_each_oldnew_connector_in_state(state, connector, old_conn_state,
+						   new_conn_state, i) {
+			if (old_conn_state->best_encoder == &encoder->base)
+				found = true;
+
+			if (new_conn_state->best_encoder != &encoder->base)
 				continue;
-			enabled = true;
+			found = enabled = true;
 
-			I915_STATE_WARN(connector->base.state->crtc !=
+			I915_STATE_WARN(new_conn_state->crtc !=
 					encoder->base.crtc,
 			     "connector's crtc doesn't match encoder crtc\n");
 		}
-		drm_connector_list_iter_end(&conn_iter);
+
+		if (!found)
+			continue;
 
 		I915_STATE_WARN(!!encoder->base.crtc != enabled,
 		     "encoder's enabled state mismatch "
@@ -12124,7 +12130,7 @@  static void
 intel_modeset_verify_disabled(struct drm_device *dev,
 			      struct drm_atomic_state *state)
 {
-	verify_encoder_state(dev);
+	verify_encoder_state(dev, state);
 	verify_connector_state(dev, state, NULL);
 	verify_disabled_dpll_state(dev);
 }