[PATCH v3 0/7] drm/atomic: Fix encoder stealing, v3.

[PATCH v3 0/7] drm/atomic: Fix encoder stealing, v3.

[Maarten Lankhorst]

This is a resend of "drm/atomic: Fix encoder stealing" with updated patches.

Maarten Lankhorst (7):
  drm/atomic: Clean up update_output_state.
  drm/atomic: Pass connector and state to update_connector_routing.
  drm/atomic: Always call steal_encoder, v2.
  drm/atomic: Handle encoder stealing from set_config better.
  drm/atomic: Handle encoder assignment conflicts in a separate check,
    v3.
  drm/atomic: Clean up steal_encoder, v2.
  drm/atomic: Clean up update_connector_routing.

 drivers/gpu/drm/drm_atomic_helper.c | 252 +++++++++++++++++++-----------------
 include/drm/drm_crtc.h              |   2 +
 2 files changed, 132 insertions(+), 122 deletions(-)

-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 1/7] drm/atomic: Clean up update_output_state.

[Maarten Lankhorst]

With the addition of crtc_state->connector_mask other connectors from
different crtc's aren't needed any more, so only call
add_affected_connectors on the target crtc. This allows a cleanup
to first remove all current connectors, then add all set->connectors
to the target crtc.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++----------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4da4f2a49078..a0226d4b949a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1761,28 +1761,18 @@ static int update_output_state(struct drm_atomic_state *state,
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
 	struct drm_connector_state *conn_state;
-	int ret, i, j;
+	int ret, i;
 
 	ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
 			       state->acquire_ctx);
 	if (ret)
 		return ret;
 
-	/* First grab all affected connector/crtc states. */
-	for (i = 0; i < set->num_connectors; i++) {
-		conn_state = drm_atomic_get_connector_state(state,
-							    set->connectors[i]);
-		if (IS_ERR(conn_state))
-			return PTR_ERR(conn_state);
-	}
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		ret = drm_atomic_add_affected_connectors(state, crtc);
-		if (ret)
-			return ret;
-	}
+	/* First disable all connectors on the target crtc. */
+	ret = drm_atomic_add_affected_connectors(state, set->crtc);
+	if (ret)
+		return ret;
 
-	/* Then recompute connector->crtc links and crtc enabling state. */
 	for_each_connector_in_state(state, connector, conn_state, i) {
 		if (conn_state->crtc == set->crtc) {
 			ret = drm_atomic_set_crtc_for_connector(conn_state,
@@ -1790,16 +1780,19 @@ static int update_output_state(struct drm_atomic_state *state,
 			if (ret)
 				return ret;
 		}
+	}
 
-		for (j = 0; j < set->num_connectors; j++) {
-			if (set->connectors[j] == connector) {
-				ret = drm_atomic_set_crtc_for_connector(conn_state,
-									set->crtc);
-				if (ret)
-					return ret;
-				break;
-			}
-		}
+	/* Then set all connectors from set->connectors on the target crtc */
+	for (i = 0; i < set->num_connectors; i++) {
+		conn_state = drm_atomic_get_connector_state(state,
+							    set->connectors[i]);
+		if (IS_ERR(conn_state))
+			return PTR_ERR(conn_state);
+
+		ret = drm_atomic_set_crtc_for_connector(conn_state,
+							set->crtc);
+		if (ret)
+			return ret;
 	}
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 2/7] drm/atomic: Pass connector and state to update_connector_routing.

[Maarten Lankhorst]

Minor cleanup, connector and connector_state are always non-NULL here.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a0226d4b949a..3d1f97a832fc 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -215,22 +215,16 @@ steal_encoder(struct drm_atomic_state *state,
 }
 
 static int
-update_connector_routing(struct drm_atomic_state *state, int conn_idx)
+update_connector_routing(struct drm_atomic_state *state,
+			 struct drm_connector *connector,
+			 struct drm_connector_state *connector_state)
 {
 	const struct drm_connector_helper_funcs *funcs;
 	struct drm_encoder *new_encoder;
 	struct drm_crtc *encoder_crtc;
-	struct drm_connector *connector;
-	struct drm_connector_state *connector_state;
 	struct drm_crtc_state *crtc_state;
 	int idx, ret;
 
-	connector = state->connectors[conn_idx];
-	connector_state = state->connector_states[conn_idx];
-
-	if (!connector)
-		return 0;
-
 	DRM_DEBUG_ATOMIC("Updating routing for [CONNECTOR:%d:%s]\n",
 			 connector->base.id,
 			 connector->name);
@@ -494,7 +488,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		 * drivers must set crtc->mode_changed themselves when connector
 		 * properties need to be updated.
 		 */
-		ret = update_connector_routing(state, i);
+		ret = update_connector_routing(state, connector,
+					       connector_state);
 		if (ret)
 			return ret;
 	}
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 3/7] drm/atomic: Always call steal_encoder, v2.

[Maarten Lankhorst]

There's no need to have a separate function to get the crtc
which is stolen, this can already be found when actually
stealing the encoder.

drm_for_each_connector already checks for connection_mutex, so
use that macro now.

Changes since v1:
- Do not check for NULL crtc in connector_state,
  this may happen when a crtc is disabled and its encoder stolen.
- Because of this, use connector->state->crtc instead of conn_state->crtc.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 74 ++++++++++---------------------------
 1 file changed, 20 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3d1f97a832fc..72ca85b32260 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -106,25 +106,6 @@ check_pending_encoder_assignment(struct drm_atomic_state *state,
 	return true;
 }
 
-static struct drm_crtc *
-get_current_crtc_for_encoder(struct drm_device *dev,
-			     struct drm_encoder *encoder)
-{
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_connector *connector;
-
-	WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
-
-	drm_for_each_connector(connector, dev) {
-		if (connector->state->best_encoder != encoder)
-			continue;
-
-		return connector->state->crtc;
-	}
-
-	return NULL;
-}
-
 static void
 set_best_encoder(struct drm_atomic_state *state,
 		 struct drm_connector_state *conn_state,
@@ -168,38 +149,18 @@ set_best_encoder(struct drm_atomic_state *state,
 
 static int
 steal_encoder(struct drm_atomic_state *state,
-	      struct drm_encoder *encoder,
-	      struct drm_crtc *encoder_crtc)
+	      struct drm_encoder *encoder)
 {
-	struct drm_mode_config *config = &state->dev->mode_config;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
 	struct drm_connector_state *connector_state;
 
-	/*
-	 * We can only steal an encoder coming from a connector, which means we
-	 * must already hold the connection_mutex.
-	 */
-	WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
-
-	DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], stealing it\n",
-			 encoder->base.id, encoder->name,
-			 encoder_crtc->base.id, encoder_crtc->name);
+	drm_for_each_connector(connector, state->dev) {
+		struct drm_crtc *encoder_crtc;
 
-	crtc_state = drm_atomic_get_crtc_state(state, encoder_crtc);
-	if (IS_ERR(crtc_state))
-		return PTR_ERR(crtc_state);
-
-	crtc_state->connectors_changed = true;
-
-	list_for_each_entry(connector, &config->connector_list, head) {
 		if (connector->state->best_encoder != encoder)
 			continue;
 
-		DRM_DEBUG_ATOMIC("Stealing encoder from [CONNECTOR:%d:%s]\n",
-				 connector->base.id,
-				 connector->name);
-
 		connector_state = drm_atomic_get_connector_state(state,
 								 connector);
 		if (IS_ERR(connector_state))
@@ -208,7 +169,18 @@ steal_encoder(struct drm_atomic_state *state,
 		if (connector_state->best_encoder != encoder)
 			continue;
 
+		encoder_crtc = connector->state->crtc;
+
+		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], stealing it\n",
+				 encoder->base.id, encoder->name,
+				 encoder_crtc->base.id, encoder_crtc->name);
+
 		set_best_encoder(state, connector_state, NULL);
+
+		crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc);
+		crtc_state->connectors_changed = true;
+
+		return 0;
 	}
 
 	return 0;
@@ -221,7 +193,6 @@ update_connector_routing(struct drm_atomic_state *state,
 {
 	const struct drm_connector_helper_funcs *funcs;
 	struct drm_encoder *new_encoder;
-	struct drm_crtc *encoder_crtc;
 	struct drm_crtc_state *crtc_state;
 	int idx, ret;
 
@@ -299,17 +270,12 @@ update_connector_routing(struct drm_atomic_state *state,
 		return -EINVAL;
 	}
 
-	encoder_crtc = get_current_crtc_for_encoder(state->dev,
-						    new_encoder);
-
-	if (encoder_crtc) {
-		ret = steal_encoder(state, new_encoder, encoder_crtc);
-		if (ret) {
-			DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n",
-					 connector->base.id,
-					 connector->name);
-			return ret;
-		}
+	ret = steal_encoder(state, new_encoder);
+	if (ret) {
+		DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n",
+				 connector->base.id,
+				 connector->name);
+		return ret;
 	}
 
 	if (WARN_ON(!connector_state->crtc))
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 4/7] drm/atomic: Handle encoder stealing from set_config better.

[Maarten Lankhorst]

Instead of failing with -EINVAL when conflicting encoders are found,
the legacy set_config will disable other connectors when encoders
conflict.

With the previous commit this becomes a lot easier to implement.
set_config only adds connectors to the state that are modified,
and because of the previous commit that calls add_affected_connectors
only on set->crtc it means any connector not part of the modeset can
be stolen from. We disable the connector in that case, and possibly
the crtc if required.

Atomic modeset itself still doesn't allow encoder stealing, the results
would be too unpredictable.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 69 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h              |  2 ++
 2 files changed, 71 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 72ca85b32260..db11c2f9b098 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -86,6 +86,68 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 	}
 }
 
+static int disable_conflicting_connectors(struct drm_atomic_state *state)
+{
+	struct drm_connector_state *conn_state;
+	struct drm_connector *connector;
+	struct drm_encoder *encoder;
+	unsigned encoder_mask = 0;
+	int i, ret;
+
+	for_each_connector_in_state(state, connector, conn_state, i) {
+		const struct drm_connector_helper_funcs *funcs = connector->helper_private;
+		struct drm_encoder *new_encoder;
+
+		if (!conn_state->crtc)
+			continue;
+
+		if (funcs->atomic_best_encoder)
+			new_encoder = funcs->atomic_best_encoder(connector, conn_state);
+		else
+			new_encoder = funcs->best_encoder(connector);
+
+		if (new_encoder)
+			encoder_mask |= 1 << drm_encoder_index(new_encoder);
+	}
+
+	drm_for_each_connector(connector, state->dev) {
+		struct drm_crtc_state *crtc_state;
+
+		if (drm_atomic_get_existing_connector_state(state, connector))
+			continue;
+
+		encoder = connector->state->best_encoder;
+		if (!encoder || !(encoder_mask & (1 << drm_encoder_index(encoder))))
+			continue;
+
+		conn_state = drm_atomic_get_connector_state(state, connector);
+		if (IS_ERR(conn_state))
+			return PTR_ERR(conn_state);
+
+		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], disabling [CONNECTOR:%d:%s]\n",
+				 encoder->base.id, encoder->name,
+				 conn_state->crtc->base.id, conn_state->crtc->name,
+				 connector->base.id, connector->name);
+
+		crtc_state = drm_atomic_get_existing_crtc_state(state, conn_state->crtc);
+
+		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+		if (ret)
+			return ret;
+
+		if (!crtc_state->connector_mask) {
+			ret = drm_atomic_set_mode_prop_for_crtc(crtc_state,
+								NULL);
+			if (ret < 0)
+				return ret;
+
+			crtc_state->active = false;
+		}
+	}
+
+	return 0;
+}
+
 static bool
 check_pending_encoder_assignment(struct drm_atomic_state *state,
 				 struct drm_encoder *new_encoder)
@@ -448,6 +510,12 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		}
 	}
 
+	if (state->legacy_set_config) {
+		ret = disable_conflicting_connectors(state);
+		if (ret)
+			return ret;
+	}
+
 	for_each_connector_in_state(state, connector, connector_state, i) {
 		/*
 		 * This only sets crtc->mode_changed for routing changes,
@@ -1796,6 +1864,7 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set)
 	if (!state)
 		return -ENOMEM;
 
+	state->legacy_set_config = true;
 	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
 retry:
 	ret = __drm_atomic_helper_set_config(set, state);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 7fad193dc645..9a946df27f07 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1677,6 +1677,7 @@ struct drm_bridge {
  * @dev: parent DRM device
  * @allow_modeset: allow full modeset
  * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
+ * @legacy_set_config: Disable conflicting encoders instead of failing with -EINVAL.
  * @planes: pointer to array of plane pointers
  * @plane_states: pointer to array of plane states pointers
  * @crtcs: pointer to array of CRTC pointers
@@ -1690,6 +1691,7 @@ struct drm_atomic_state {
 	struct drm_device *dev;
 	bool allow_modeset : 1;
 	bool legacy_cursor_update : 1;
+	bool legacy_set_config : 1;
 	struct drm_plane **planes;
 	struct drm_plane_state **plane_states;
 	struct drm_crtc **crtcs;
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 5/7] drm/atomic: Handle encoder assignment conflicts in a separate check, v3.

[Maarten Lankhorst]

The current check doesn't handle the case where we don't steal an
encoder, but keep it on the current connector. If we repurpose
disable_conflicting_encoders to do the checking, we just have
to reject the ones that conflict.

Changes since v1:
- Return early with empty encoder_mask, drm_for_each_connector
  requires connection_mutex held.
Changes since v2:
- Add comments for the loops.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Testcase: kms_setmode.invalid-clone-single-crtc-stealing
---
 drivers/gpu/drm/drm_atomic_helper.c | 77 +++++++++++++++++++++----------------
 1 file changed, 43 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index db11c2f9b098..bb60148c5c8d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -86,7 +86,8 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 	}
 }
 
-static int disable_conflicting_connectors(struct drm_atomic_state *state)
+static int handle_conflicting_encoders(struct drm_atomic_state *state,
+				       bool disable_conflicting_encoders)
 {
 	struct drm_connector_state *conn_state;
 	struct drm_connector *connector;
@@ -94,6 +95,11 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state)
 	unsigned encoder_mask = 0;
 	int i, ret;
 
+	/*
+	 * First loop, find all newly assigned encoders from the connectors
+	 * part of the state. If the same encoder is assigned to multiple
+	 * connectors bail out.
+	 */
 	for_each_connector_in_state(state, connector, conn_state, i) {
 		const struct drm_connector_helper_funcs *funcs = connector->helper_private;
 		struct drm_encoder *new_encoder;
@@ -106,10 +112,33 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state)
 		else
 			new_encoder = funcs->best_encoder(connector);
 
-		if (new_encoder)
+		if (new_encoder) {
+			if (encoder_mask & (1 << drm_encoder_index(new_encoder))) {
+				DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] on [CONNECTOR:%d:%s] already assigned\n",
+					new_encoder->base.id, new_encoder->name,
+					connector->base.id, connector->name);
+
+				return -EINVAL;
+			}
+
 			encoder_mask |= 1 << drm_encoder_index(new_encoder);
+		}
 	}
 
+	if (!encoder_mask)
+		return 0;
+
+	/*
+	 * Second loop, iterate over all connectors not part of the state.
+	 *
+	 * If a conflicting encoder is found and disable_conflicting_encoders
+	 * is not set, an error is returned. Userspace can provide a solution
+	 * through the atomic ioctl.
+	 *
+	 * If the flag is set conflicting connectors are removed from the crtc
+	 * and the crtc is disabled if no encoder is left. This preserves
+	 * compatibility with the legacy set_config behavior.
+	 */
 	drm_for_each_connector(connector, state->dev) {
 		struct drm_crtc_state *crtc_state;
 
@@ -120,6 +149,15 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state)
 		if (!encoder || !(encoder_mask & (1 << drm_encoder_index(encoder))))
 			continue;
 
+		if (!disable_conflicting_encoders) {
+			DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s] by [CONNECTOR:%d:%s]\n",
+					 encoder->base.id, encoder->name,
+					 connector->state->crtc->base.id,
+					 connector->state->crtc->name,
+					 connector->base.id, connector->name);
+			return -EINVAL;
+		}
+
 		conn_state = drm_atomic_get_connector_state(state, connector);
 		if (IS_ERR(conn_state))
 			return PTR_ERR(conn_state);
@@ -148,26 +186,6 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state)
 	return 0;
 }
 
-static bool
-check_pending_encoder_assignment(struct drm_atomic_state *state,
-				 struct drm_encoder *new_encoder)
-{
-	struct drm_connector *connector;
-	struct drm_connector_state *conn_state;
-	int i;
-
-	for_each_connector_in_state(state, connector, conn_state, i) {
-		if (conn_state->best_encoder != new_encoder)
-			continue;
-
-		/* encoder already assigned and we're trying to re-steal it! */
-		if (connector->state->best_encoder != conn_state->best_encoder)
-			return false;
-	}
-
-	return true;
-}
-
 static void
 set_best_encoder(struct drm_atomic_state *state,
 		 struct drm_connector_state *conn_state,
@@ -325,13 +343,6 @@ update_connector_routing(struct drm_atomic_state *state,
 		return 0;
 	}
 
-	if (!check_pending_encoder_assignment(state, new_encoder)) {
-		DRM_DEBUG_ATOMIC("Encoder for [CONNECTOR:%d:%s] already assigned\n",
-				 connector->base.id,
-				 connector->name);
-		return -EINVAL;
-	}
-
 	ret = steal_encoder(state, new_encoder);
 	if (ret) {
 		DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n",
@@ -510,11 +521,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		}
 	}
 
-	if (state->legacy_set_config) {
-		ret = disable_conflicting_connectors(state);
-		if (ret)
-			return ret;
-	}
+	ret = handle_conflicting_encoders(state, state->legacy_set_config);
+	if (ret)
+		return ret;
 
 	for_each_connector_in_state(state, connector, connector_state, i) {
 		/*
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 6/7] drm/atomic: Clean up steal_encoder, v2.

[Maarten Lankhorst]

Now that only encoders can be stolen that are part of the state
steal_encoder no longer needs to inspect all connectors,
just those that are part of the atomic state.

Changes since v1:
- Change return value to void, can no longer fail.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index bb60148c5c8d..2395201eb7ab 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -227,25 +227,18 @@ set_best_encoder(struct drm_atomic_state *state,
 	conn_state->best_encoder = encoder;
 }
 
-static int
+static void
 steal_encoder(struct drm_atomic_state *state,
 	      struct drm_encoder *encoder)
 {
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
 	struct drm_connector_state *connector_state;
+	int i;
 
-	drm_for_each_connector(connector, state->dev) {
+	for_each_connector_in_state(state, connector, connector_state, i) {
 		struct drm_crtc *encoder_crtc;
 
-		if (connector->state->best_encoder != encoder)
-			continue;
-
-		connector_state = drm_atomic_get_connector_state(state,
-								 connector);
-		if (IS_ERR(connector_state))
-			return PTR_ERR(connector_state);
-
 		if (connector_state->best_encoder != encoder)
 			continue;
 
@@ -260,10 +253,8 @@ steal_encoder(struct drm_atomic_state *state,
 		crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc);
 		crtc_state->connectors_changed = true;
 
-		return 0;
+		return;
 	}
-
-	return 0;
 }
 
 static int
@@ -343,13 +334,7 @@ update_connector_routing(struct drm_atomic_state *state,
 		return 0;
 	}
 
-	ret = steal_encoder(state, new_encoder);
-	if (ret) {
-		DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n",
-				 connector->base.id,
-				 connector->name);
-		return ret;
-	}
+	steal_encoder(state, new_encoder);
 
 	if (WARN_ON(!connector_state->crtc))
 		return -EINVAL;
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 7/7] drm/atomic: Clean up update_connector_routing.

[Maarten Lankhorst]

connector_state->crtc can no longer be unset by accident,
so that check can be removed. The other code open-codes
drm_atomic_get_existing_crtc_state, so use that.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 2395201eb7ab..9f0d16eb04b2 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -265,7 +265,7 @@ update_connector_routing(struct drm_atomic_state *state,
 	const struct drm_connector_helper_funcs *funcs;
 	struct drm_encoder *new_encoder;
 	struct drm_crtc_state *crtc_state;
-	int idx, ret;
+	int ret;
 
 	DRM_DEBUG_ATOMIC("Updating routing for [CONNECTOR:%d:%s]\n",
 			 connector->base.id,
@@ -273,16 +273,12 @@ update_connector_routing(struct drm_atomic_state *state,
 
 	if (connector->state->crtc != connector_state->crtc) {
 		if (connector->state->crtc) {
-			idx = drm_crtc_index(connector->state->crtc);
-
-			crtc_state = state->crtc_states[idx];
+			crtc_state = drm_atomic_get_existing_crtc_state(state, connector->state->crtc);
 			crtc_state->connectors_changed = true;
 		}
 
 		if (connector_state->crtc) {
-			idx = drm_crtc_index(connector_state->crtc);
-
-			crtc_state = state->crtc_states[idx];
+			crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc);
 			crtc_state->connectors_changed = true;
 		}
 	}
@@ -336,14 +332,9 @@ update_connector_routing(struct drm_atomic_state *state,
 
 	steal_encoder(state, new_encoder);
 
-	if (WARN_ON(!connector_state->crtc))
-		return -EINVAL;
-
 	set_best_encoder(state, connector_state, new_encoder);
 
-	idx = drm_crtc_index(connector_state->crtc);
-
-	crtc_state = state->crtc_states[idx];
+	crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc);
 	crtc_state->connectors_changed = true;
 
 	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/atomic: Fix encoder stealing, v3.

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix encoder stealing, v3.
URL   : https://patchwork.freedesktop.org/series/4045/
State : failure

== Summary ==

Series 4045v1 drm/atomic: Fix encoder stealing, v3.
http://patchwork.freedesktop.org/api/1.0/series/4045/revisions/1/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (hsw-brixbox)
                pass       -> DMESG-WARN (bdw-ultra)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> INCOMPLETE (hsw-gt2)

bdw-nuci7        total:169  pass:158  dwarn:0   dfail:0   fail:0   skip:11 
bdw-ultra        total:169  pass:154  dwarn:1   dfail:0   fail:0   skip:14 
bsw-nuc-2        total:169  pass:137  dwarn:1   dfail:0   fail:1   skip:30 
byt-nuc          total:169  pass:144  dwarn:0   dfail:0   fail:0   skip:25 
hsw-brixbox      total:169  pass:153  dwarn:1   dfail:0   fail:0   skip:15 
hsw-gt2          total:31   pass:30   dwarn:0   dfail:0   fail:0   skip:0  
ilk-hp8440p      total:169  pass:119  dwarn:0   dfail:0   fail:0   skip:50 
ivb-t430s        total:169  pass:154  dwarn:0   dfail:0   fail:0   skip:15 
skl-i5k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
skl-i7k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
snb-x220t        total:169  pass:145  dwarn:1   dfail:0   fail:1   skip:22 

Results at /archive/results/CI_IGT_test/Patchwork_1520/

99ba2daf15b76f01916dc645b8d1b03dbbabf13c drm-intel-nightly: 2016y-03m-03d-08h-27m-26s UTC integration manifest
295f18455635ee101a4852f17773809dbce63a00 drm/atomic: Clean up update_connector_routing.
2eb019b3bb0cdaf744306129171cf9a76f2a7228 drm/atomic: Clean up steal_encoder, v2.
08a4219f9e70bc1723915968c5c9c7a4120c4dbf drm/atomic: Handle encoder assignment conflicts in a separate check, v3.
5875bb2f1b4a164f1ea16320faeedafa31a600ee drm/atomic: Handle encoder stealing from set_config better.
44c05a6b31d1602b8858fcd1a1aed40243542b69 drm/atomic: Always call steal_encoder, v2.
ad558b2f2099dfe79fc31a3538aa6ae9f1c1212e drm/atomic: Pass connector and state to update_connector_routing.
c0c9c6cdd42e37e23839bccf01ea6aaa6b388ca2 drm/atomic: Clean up update_output_state.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 6/7] drm/atomic: Clean up steal_encoder, v2.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3137 bytes --]

Hi Maarten,

[auto build test WARNING on drm/drm-next]
[also build test WARNING on next-20160303]
[cannot apply to v4.5-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Maarten-Lankhorst/drm-atomic-Fix-encoder-stealing-v3/20160303-172123
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x014-201609 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/drm_atomic_helper.c: In function 'update_connector_routing':
>> drivers/gpu/drm/drm_atomic_helper.c:268:11: warning: unused variable 'ret' [-Wunused-variable]
     int idx, ret;
              ^

vim +/ret +268 drivers/gpu/drm/drm_atomic_helper.c

c6c869e2 Maarten Lankhorst 2016-03-03  252  
c6c869e2 Maarten Lankhorst 2016-03-03  253  		crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc);
c6c869e2 Maarten Lankhorst 2016-03-03  254  		crtc_state->connectors_changed = true;
c6c869e2 Maarten Lankhorst 2016-03-03  255  
49bf38f8 Maarten Lankhorst 2016-03-03  256  		return;
623369e5 Daniel Vetter     2014-09-16  257  	}
623369e5 Daniel Vetter     2014-09-16  258  }
623369e5 Daniel Vetter     2014-09-16  259  
623369e5 Daniel Vetter     2014-09-16  260  static int
184a4cc1 Maarten Lankhorst 2016-03-03  261  update_connector_routing(struct drm_atomic_state *state,
184a4cc1 Maarten Lankhorst 2016-03-03  262  			 struct drm_connector *connector,
184a4cc1 Maarten Lankhorst 2016-03-03  263  			 struct drm_connector_state *connector_state)
623369e5 Daniel Vetter     2014-09-16  264  {
b5ceff20 Ville Syrjälä     2015-03-10  265  	const struct drm_connector_helper_funcs *funcs;
623369e5 Daniel Vetter     2014-09-16  266  	struct drm_encoder *new_encoder;
623369e5 Daniel Vetter     2014-09-16  267  	struct drm_crtc_state *crtc_state;
623369e5 Daniel Vetter     2014-09-16 @268  	int idx, ret;
623369e5 Daniel Vetter     2014-09-16  269  
17a38d9c Daniel Vetter     2015-02-22  270  	DRM_DEBUG_ATOMIC("Updating routing for [CONNECTOR:%d:%s]\n",
623369e5 Daniel Vetter     2014-09-16  271  			 connector->base.id,
623369e5 Daniel Vetter     2014-09-16  272  			 connector->name);
623369e5 Daniel Vetter     2014-09-16  273  
623369e5 Daniel Vetter     2014-09-16  274  	if (connector->state->crtc != connector_state->crtc) {
623369e5 Daniel Vetter     2014-09-16  275  		if (connector->state->crtc) {
623369e5 Daniel Vetter     2014-09-16  276  			idx = drm_crtc_index(connector->state->crtc);

:::::: The code at line 268 was first introduced by commit
:::::: 623369e533e8a5f85999d605723efa4523554bae drm: Atomic crtc/connector updates using crtc/plane helper interfaces

:::::: TO: Daniel Vetter <daniel.vetter@ffwll.ch>
:::::: CC: Daniel Vetter <daniel.vetter@ffwll.ch>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 21254 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 5/7] drm/atomic: Handle encoder assignment conflicts in a separate check, v3.

[Ville Syrjälä]

On Thu, Mar 03, 2016 at 10:17:40AM +0100, Maarten Lankhorst wrote:
> The current check doesn't handle the case where we don't steal an
> encoder, but keep it on the current connector. If we repurpose
> disable_conflicting_encoders to do the checking, we just have
> to reject the ones that conflict.
> 
> Changes since v1:
> - Return early with empty encoder_mask, drm_for_each_connector
>   requires connection_mutex held.
> Changes since v2:
> - Add comments for the loops.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Testcase: kms_setmode.invalid-clone-single-crtc-stealing

I was a bit lazy on looking at the individual patches 3-5, but the result
made sense to me, so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
for all three.

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 77 +++++++++++++++++++++----------------
>  1 file changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index db11c2f9b098..bb60148c5c8d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -86,7 +86,8 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
>  	}
>  }
>  
> -static int disable_conflicting_connectors(struct drm_atomic_state *state)
> +static int handle_conflicting_encoders(struct drm_atomic_state *state,
> +				       bool disable_conflicting_encoders)
>  {
>  	struct drm_connector_state *conn_state;
>  	struct drm_connector *connector;
> @@ -94,6 +95,11 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state)
>  	unsigned encoder_mask = 0;
>  	int i, ret;
>  
> +	/*
> +	 * First loop, find all newly assigned encoders from the connectors
> +	 * part of the state. If the same encoder is assigned to multiple
> +	 * connectors bail out.
> +	 */
>  	for_each_connector_in_state(state, connector, conn_state, i) {
>  		const struct drm_connector_helper_funcs *funcs = connector->helper_private;
>  		struct drm_encoder *new_encoder;
> @@ -106,10 +112,33 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state)
>  		else
>  			new_encoder = funcs->best_encoder(connector);
>  
> -		if (new_encoder)
> +		if (new_encoder) {
> +			if (encoder_mask & (1 << drm_encoder_index(new_encoder))) {
> +				DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] on [CONNECTOR:%d:%s] already assigned\n",
> +					new_encoder->base.id, new_encoder->name,
> +					connector->base.id, connector->name);
> +
> +				return -EINVAL;
> +			}
> +
>  			encoder_mask |= 1 << drm_encoder_index(new_encoder);
> +		}
>  	}
>  
> +	if (!encoder_mask)
> +		return 0;
> +
> +	/*
> +	 * Second loop, iterate over all connectors not part of the state.
> +	 *
> +	 * If a conflicting encoder is found and disable_conflicting_encoders
> +	 * is not set, an error is returned. Userspace can provide a solution
> +	 * through the atomic ioctl.
> +	 *
> +	 * If the flag is set conflicting connectors are removed from the crtc
> +	 * and the crtc is disabled if no encoder is left. This preserves
> +	 * compatibility with the legacy set_config behavior.
> +	 */
>  	drm_for_each_connector(connector, state->dev) {
>  		struct drm_crtc_state *crtc_state;
>  
> @@ -120,6 +149,15 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state)
>  		if (!encoder || !(encoder_mask & (1 << drm_encoder_index(encoder))))
>  			continue;
>  
> +		if (!disable_conflicting_encoders) {
> +			DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s] by [CONNECTOR:%d:%s]\n",
> +					 encoder->base.id, encoder->name,
> +					 connector->state->crtc->base.id,
> +					 connector->state->crtc->name,
> +					 connector->base.id, connector->name);
> +			return -EINVAL;
> +		}
> +
>  		conn_state = drm_atomic_get_connector_state(state, connector);
>  		if (IS_ERR(conn_state))
>  			return PTR_ERR(conn_state);
> @@ -148,26 +186,6 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> -static bool
> -check_pending_encoder_assignment(struct drm_atomic_state *state,
> -				 struct drm_encoder *new_encoder)
> -{
> -	struct drm_connector *connector;
> -	struct drm_connector_state *conn_state;
> -	int i;
> -
> -	for_each_connector_in_state(state, connector, conn_state, i) {
> -		if (conn_state->best_encoder != new_encoder)
> -			continue;
> -
> -		/* encoder already assigned and we're trying to re-steal it! */
> -		if (connector->state->best_encoder != conn_state->best_encoder)
> -			return false;
> -	}
> -
> -	return true;
> -}
> -
>  static void
>  set_best_encoder(struct drm_atomic_state *state,
>  		 struct drm_connector_state *conn_state,
> @@ -325,13 +343,6 @@ update_connector_routing(struct drm_atomic_state *state,
>  		return 0;
>  	}
>  
> -	if (!check_pending_encoder_assignment(state, new_encoder)) {
> -		DRM_DEBUG_ATOMIC("Encoder for [CONNECTOR:%d:%s] already assigned\n",
> -				 connector->base.id,
> -				 connector->name);
> -		return -EINVAL;
> -	}
> -
>  	ret = steal_encoder(state, new_encoder);
>  	if (ret) {
>  		DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n",
> @@ -510,11 +521,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  		}
>  	}
>  
> -	if (state->legacy_set_config) {
> -		ret = disable_conflicting_connectors(state);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = handle_conflicting_encoders(state, state->legacy_set_config);
> +	if (ret)
> +		return ret;
>  
>  	for_each_connector_in_state(state, connector, connector_state, i) {
>  		/*
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [Intel-gfx] [PATCH v3 6/7] drm/atomic: Clean up steal_encoder, v2.

[Ville Syrjälä]

On Thu, Mar 03, 2016 at 10:17:41AM +0100, Maarten Lankhorst wrote:
> Now that only encoders can be stolen that are part of the state
> steal_encoder no longer needs to inspect all connectors,
> just those that are part of the atomic state.
> 
> Changes since v1:
> - Change return value to void, can no longer fail.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 25 +++++--------------------
>  1 file changed, 5 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index bb60148c5c8d..2395201eb7ab 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -227,25 +227,18 @@ set_best_encoder(struct drm_atomic_state *state,
>  	conn_state->best_encoder = encoder;
>  }
>  
> -static int
> +static void
>  steal_encoder(struct drm_atomic_state *state,
>  	      struct drm_encoder *encoder)
>  {
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_connector *connector;
>  	struct drm_connector_state *connector_state;
> +	int i;
>  
> -	drm_for_each_connector(connector, state->dev) {
> +	for_each_connector_in_state(state, connector, connector_state, i) {
>  		struct drm_crtc *encoder_crtc;
>  
> -		if (connector->state->best_encoder != encoder)
> -			continue;
> -
> -		connector_state = drm_atomic_get_connector_state(state,
> -								 connector);
> -		if (IS_ERR(connector_state))
> -			return PTR_ERR(connector_state);
> -
>  		if (connector_state->best_encoder != encoder)
>  			continue;
>  
> @@ -260,10 +253,8 @@ steal_encoder(struct drm_atomic_state *state,
>  		crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc);
>  		crtc_state->connectors_changed = true;
>  
> -		return 0;
> +		return;

I was wondering if we should add some kind of sanity check here (or
maybe better do it afterwards across the entire device state?) to
make sure the same encoder didn't end up used multiple times?

Anyway, patch is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	}
> -
> -	return 0;
>  }
>  
>  static int
> @@ -343,13 +334,7 @@ update_connector_routing(struct drm_atomic_state *state,
>  		return 0;
>  	}
>  
> -	ret = steal_encoder(state, new_encoder);
> -	if (ret) {
> -		DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n",
> -				 connector->base.id,
> -				 connector->name);
> -		return ret;
> -	}
> +	steal_encoder(state, new_encoder);
>  
>  	if (WARN_ON(!connector_state->crtc))
>  		return -EINVAL;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH v3 7/7] drm/atomic: Clean up update_connector_routing.

[Ville Syrjälä]

On Thu, Mar 03, 2016 at 10:17:42AM +0100, Maarten Lankhorst wrote:
> connector_state->crtc can no longer be unset by accident,
> so that check can be removed. The other code open-codes
> drm_atomic_get_existing_crtc_state, so use that.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 2395201eb7ab..9f0d16eb04b2 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -265,7 +265,7 @@ update_connector_routing(struct drm_atomic_state *state,
>  	const struct drm_connector_helper_funcs *funcs;
>  	struct drm_encoder *new_encoder;
>  	struct drm_crtc_state *crtc_state;
> -	int idx, ret;
> +	int ret;
>  
>  	DRM_DEBUG_ATOMIC("Updating routing for [CONNECTOR:%d:%s]\n",
>  			 connector->base.id,
> @@ -273,16 +273,12 @@ update_connector_routing(struct drm_atomic_state *state,
>  
>  	if (connector->state->crtc != connector_state->crtc) {
>  		if (connector->state->crtc) {
> -			idx = drm_crtc_index(connector->state->crtc);
> -
> -			crtc_state = state->crtc_states[idx];
> +			crtc_state = drm_atomic_get_existing_crtc_state(state, connector->state->crtc);
>  			crtc_state->connectors_changed = true;
>  		}
>  
>  		if (connector_state->crtc) {
> -			idx = drm_crtc_index(connector_state->crtc);
> -
> -			crtc_state = state->crtc_states[idx];
> +			crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc);
>  			crtc_state->connectors_changed = true;
>  		}
>  	}
> @@ -336,14 +332,9 @@ update_connector_routing(struct drm_atomic_state *state,
>  
>  	steal_encoder(state, new_encoder);
>  
> -	if (WARN_ON(!connector_state->crtc))
> -		return -EINVAL;
> -

I was going to suggest just this when I read the code previously.

>  	set_best_encoder(state, connector_state, new_encoder);
>  
> -	idx = drm_crtc_index(connector_state->crtc);
> -
> -	crtc_state = state->crtc_states[idx];
> +	crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc);

These could have been a separate patch, but no biggie

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	crtc_state->connectors_changed = true;
>  
>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v3 7/7] drm/atomic: Clean up update_connector_routing.

[Daniel Vetter]

On Fri, Mar 04, 2016 at 03:29:03PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 03, 2016 at 10:17:42AM +0100, Maarten Lankhorst wrote:
> > connector_state->crtc can no longer be unset by accident,
> > so that check can be removed. The other code open-codes
> > drm_atomic_get_existing_crtc_state, so use that.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 17 ++++-------------
> >  1 file changed, 4 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 2395201eb7ab..9f0d16eb04b2 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -265,7 +265,7 @@ update_connector_routing(struct drm_atomic_state *state,
> >  	const struct drm_connector_helper_funcs *funcs;
> >  	struct drm_encoder *new_encoder;
> >  	struct drm_crtc_state *crtc_state;
> > -	int idx, ret;
> > +	int ret;
> >  
> >  	DRM_DEBUG_ATOMIC("Updating routing for [CONNECTOR:%d:%s]\n",
> >  			 connector->base.id,
> > @@ -273,16 +273,12 @@ update_connector_routing(struct drm_atomic_state *state,
> >  
> >  	if (connector->state->crtc != connector_state->crtc) {
> >  		if (connector->state->crtc) {
> > -			idx = drm_crtc_index(connector->state->crtc);
> > -
> > -			crtc_state = state->crtc_states[idx];
> > +			crtc_state = drm_atomic_get_existing_crtc_state(state, connector->state->crtc);
> >  			crtc_state->connectors_changed = true;
> >  		}
> >  
> >  		if (connector_state->crtc) {
> > -			idx = drm_crtc_index(connector_state->crtc);
> > -
> > -			crtc_state = state->crtc_states[idx];
> > +			crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc);
> >  			crtc_state->connectors_changed = true;
> >  		}
> >  	}
> > @@ -336,14 +332,9 @@ update_connector_routing(struct drm_atomic_state *state,
> >  
> >  	steal_encoder(state, new_encoder);
> >  
> > -	if (WARN_ON(!connector_state->crtc))
> > -		return -EINVAL;
> > -
> 
> I was going to suggest just this when I read the code previously.
> 
> >  	set_best_encoder(state, connector_state, new_encoder);
> >  
> > -	idx = drm_crtc_index(connector_state->crtc);
> > -
> > -	crtc_state = state->crtc_states[idx];
> > +	crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc);
> 
> These could have been a separate patch, but no biggie
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Remaining patches applied to drm-misc, thanks.
-Daniel

> 
> >  	crtc_state->connectors_changed = true;
> >  
> >  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> > -- 
> > 2.1.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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