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

Submitted by Maarten Lankhorst on March 3, 2016, 9:17 a.m.

Details

Message ID 1456996662-8704-6-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show
Series "drm/atomic: Fix encoder stealing, v3." ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Maarten Lankhorst March 3, 2016, 9:17 a.m.
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(-)

Patch hide | download patch | download mbox

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) {
 		/*

Comments

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