[PATCH v2 0/6] drm/atomic: Fix encoder stealing, v2.

[PATCH v2 0/6] drm/atomic: Fix encoder stealing, v2.

[Maarten Lankhorst]

After trying out various ways to handle encoder stealing better
I came up with a cleaner way.

The first patch cleans up update_output_state and only adds affected connectors.
This is required to determine which connectors are not part of the atomic state,
and allow disabling them if they have conflicting encoders.

After that 2 cleanups, then a fix to make encoder stealing explicit,
followed by using the newly created function to prevent encoder duplication
and finally another cleanup.

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

 drivers/gpu/drm/drm_atomic_helper.c | 215 +++++++++++++++++++-----------------
 include/drm/drm_crtc.h              |   2 +
 2 files changed, 113 insertions(+), 104 deletions(-)

-- 
2.1.0

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

[PATCH v2 1/6] 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 to determine if a crtc has
connectors, 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>
---
 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

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

Re: [PATCH v2 1/6] drm/atomic: Clean up update_output_state.

[Ville Syrjälä]

On Wed, Feb 24, 2016 at 09:37:28AM +0100, Maarten Lankhorst wrote:
> With the addition of crtc_state->connector_mask other connectors from
> different crtc's aren't needed any more to determine if a crtc has
> connectors, 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.

The new order of things make sense to me.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

One thing I did notice is that we're not clearing the primary plane
crtc and fb when a crtc gets disabled due to its connector(s) getting
stolen. We do clear them when the crtc is explicitly disabled. Not sure
if that matters for anything, but it does seem a bit inconsistent.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH v2 2/6] 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>
---
 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

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

Re: [PATCH v2 2/6] drm/atomic: Pass connector and state to update_connector_routing.

[Ville Syrjälä]

On Wed, Feb 24, 2016 at 09:37:29AM +0100, Maarten Lankhorst wrote:
> 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

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

Re: [Intel-gfx] [PATCH v2 2/6] drm/atomic: Pass connector and state to update_connector_routing.

[Daniel Vetter]

On Tue, Mar 01, 2016 at 05:19:53PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 24, 2016 at 09:37:29AM +0100, Maarten Lankhorst wrote:
> > 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>

Merged the first 2 patches to drm-misc, thanks.
-Daniel
> 
> > ---
> >  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
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/6] drm/atomic: Always call steal_encoder.

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

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3d1f97a832fc..e89a5da27463 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,47 +149,39 @@ 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))
 			return PTR_ERR(connector_state);
 
-		if (connector_state->best_encoder != encoder)
+		if (connector_state->best_encoder != encoder ||
+		    WARN_ON(!connector_state->crtc))
 			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 +194,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 +271,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 v2.1 3/6] 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>
---
Oops, this patch had a WARN_ON(!conn_state->crtc); in v1,
this broke DP MST which had a legitimate reason to do so.

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

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

Re: [PATCH v2.1 3/6] drm/atomic: Always call steal_encoder, v2.

[Daniel Vetter]

On Tue, Mar 01, 2016 at 02:52:05PM +0100, Maarten Lankhorst wrote:
> 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>

Also I think we should have a testcase of some sorts for this. Probably
simplest to write unit tests for the atomic helper library.
-Daniel

> ---
> Oops, this patch had a WARN_ON(!conn_state->crtc); in v1,
> this broke DP MST which had a legitimate reason to do so.
> 
> 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))
> 
> _______________________________________________
> 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

[PATCH v2 4/6] 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 cleanup to update_output_state this is 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 no connectors are left.

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 e89a5da27463..3543c7fcd072 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)
@@ -449,6 +511,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,
@@ -1797,6 +1865,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

Re: [PATCH v2 4/6] drm/atomic: Handle encoder stealing from set_config better.

[Daniel Vetter]

On Wed, Feb 24, 2016 at 09:37:31AM +0100, Maarten Lankhorst wrote:
> Instead of failing with -EINVAL when conflicting encoders are found,
> the legacy set_config will disable other connectors when encoders
> conflict.
> 
> With the cleanup to update_output_state this is 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 no connectors are left.
> 
> Atomic modeset itself still doesn't allow encoder stealing, the
> results would be too unpredictable.

Shouldn't this be reworked to say something like "With this we can rework
the atomic core to not allow stealing"?
> 
> 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 e89a5da27463..3543c7fcd072 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)
> @@ -449,6 +511,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,
> @@ -1797,6 +1865,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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/6] drm/atomic: Handle encoder assignment conflicts in a separate check.

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

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 | 58 +++++++++++++++----------------------
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3543c7fcd072..32bd5bebef0b 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;
@@ -106,8 +107,17 @@ 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);
+		}
 	}
 
 	drm_for_each_connector(connector, state->dev) {
@@ -120,6 +130,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 +167,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,
@@ -326,13 +325,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",
@@ -511,11 +503,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

Re: [PATCH v2.1 5/6] drm/atomic: Handle encoder assignment conflicts in a separate check, v2.

[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 when encoder_mask is empty, drm_for_each_connector
  requires connection_mutex held.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Testcase: kms_setmode.invalid-clone-single-crtc-stealing
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3543c7fcd072..f44c043596d0 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;
@@ -106,10 +107,22 @@ 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;
+
 	drm_for_each_connector(connector, state->dev) {
 		struct drm_crtc_state *crtc_state;
 
@@ -120,6 +133,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 +170,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,
@@ -326,13 +328,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",
@@ -511,11 +506,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) {
 		/*

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

Re: [PATCH v2 5/6] drm/atomic: Handle encoder assignment conflicts in a separate check.

[Ville Syrjälä]

On Wed, Feb 24, 2016 at 09:37:32AM +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.
> 
> 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 | 58 +++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 3543c7fcd072..32bd5bebef0b 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;
> @@ -106,8 +107,17 @@ 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);
> +		}
>  	}
>  
>  	drm_for_each_connector(connector, state->dev) {
> @@ -120,6 +130,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;
> +		}
> +

Hmm. This can't possibly work can it? If I'm reding things correctly
this would already fail if we have crtc0->enc0->conn0 and then try to
change it to crtc1->enc0->conn0. But perhaps I'm missing some subtle
thing (there are a lot of those in our atomic framework due to thing
automagically getting added to the state).

The idea I had for checking things was something like:
for legacy setcrtc:
 for_new_connstate()
 	if (!state->crtc)
		continue;
	enc = ->best_encoder();
	if (encoder_mask & 1 << enc.index)
		fail;
 	encoder_mask |= 1 << enc.index;
 for_all_connectors
 	if (new_state)
		continue;
	if (!old_state->crtc)
		continue;
	enc = conn->best_encoder;
	if (encoder_mask & 1 << enc.index)
		disable_connector;

for atomic: 
 for_all_connectors
 	if (new_state)
		state = new_state;
	else
		state = old_state;
	if (!state->crtc)
		continue;
	if (new_state)
		enc = ->best_encoder()
	else
		enc = conn->best_encoder;
	if (encoder_mask & 1 << enc.index)
		fail;
	encoder_mask |= 1 << enc.index;

Though I'm not entirely sure the legacy variant would work correctly due
to something maybe adding the connector to the state even though it's
not part of the set that the user requested. I might need to think more
on this.


>  		conn_state = drm_atomic_get_connector_state(state, connector);
>  		if (IS_ERR(conn_state))
>  			return PTR_ERR(conn_state);
> @@ -148,26 +167,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,
> @@ -326,13 +325,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",
> @@ -511,11 +503,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

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

Re: [Intel-gfx] [PATCH v2 5/6] drm/atomic: Handle encoder assignment conflicts in a separate check.

[Maarten Lankhorst]

Op 01-03-16 om 18:21 schreef Ville Syrjälä:
> On Wed, Feb 24, 2016 at 09:37:32AM +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.
>>
>> 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 | 58 +++++++++++++++----------------------
>>  1 file changed, 24 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 3543c7fcd072..32bd5bebef0b 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;
>> @@ -106,8 +107,17 @@ 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);
>> +		}
>>  	}
>>  
>>  	drm_for_each_connector(connector, state->dev) {
>> @@ -120,6 +130,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;
>> +		}
>> +
> Hmm. This can't possibly work can it? If I'm reding things correctly
> this would already fail if we have crtc0->enc0->conn0 and then try to
> change it to crtc1->enc0->conn0. But perhaps I'm missing some subtle
> thing (there are a lot of those in our atomic framework due to thing
> automagically getting added to the state).
>
No, in that case the connector is part of the state.

This boils down to:

if (stealing encoder from existing connector not part of state)
if (atomic) return -EINVAL;
else
// disable connector and possibly crtc

~Maarten

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

Re: [PATCH v2 5/6] drm/atomic: Handle encoder assignment conflicts in a separate check.

[Ville Syrjälä]

On Tue, Mar 01, 2016 at 06:45:53PM +0100, Maarten Lankhorst wrote:
> Op 01-03-16 om 18:21 schreef Ville Syrjälä:
> > On Wed, Feb 24, 2016 at 09:37:32AM +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.
> >>
> >> 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 | 58 +++++++++++++++----------------------
> >>  1 file changed, 24 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 3543c7fcd072..32bd5bebef0b 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;
> >> @@ -106,8 +107,17 @@ 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);
> >> +		}
> >>  	}
> >>  
> >>  	drm_for_each_connector(connector, state->dev) {
> >> @@ -120,6 +130,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;
> >> +		}
> >> +
> > Hmm. This can't possibly work can it? If I'm reding things correctly
> > this would already fail if we have crtc0->enc0->conn0 and then try to
> > change it to crtc1->enc0->conn0. But perhaps I'm missing some subtle
> > thing (there are a lot of those in our atomic framework due to thing
> > automagically getting added to the state).
> >
> No, in that case the connector is part of the state.

Doh, I misread the condition for the existing state check.

OK, so I guess this is more or less what I had in mind as well, except
it uses the two loops version for both cases. The one difference between
my idea and this is that you don't include the old encoders in
encoder_mask for the atomic case, but that should be perfectly fine
since we can assume the old state itself had no conflicting encoder
assignments.

I think it could use a few high levelish comments around the loops to
explain what they are supposed to do, somewhat like you had in
update_output_state().

> 
> This boils down to:
> 
> if (stealing encoder from existing connector not part of state)
> if (atomic) return -EINVAL;
> else
> // disable connector and possibly crtc
> 
> ~Maarten

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

[PATCH v2.1 5/6] 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 to the loops.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Testcase: kms_setmode.invalid-clone-single-crtc-stealing
---
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) {
 		/*

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

[PATCH v2 6/6] drm/atomic: Clean up steal_encoder

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

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 32bd5bebef0b..0fc56339001d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -215,18 +215,11 @@ steal_encoder(struct drm_atomic_state *state,
 	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 ||
 		    WARN_ON(!connector_state->crtc))
 			continue;
-- 
2.1.0

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

Re: [PATCH v2 6/6] drm/atomic: Clean up steal_encoder

[Ville Syrjälä]

On Wed, Feb 24, 2016 at 09:37:33AM +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.

steal_encoder() can no longer fail after this, so should clean up the
caller a bit too I think.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 32bd5bebef0b..0fc56339001d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -215,18 +215,11 @@ steal_encoder(struct drm_atomic_state *state,
>  	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 ||
>  		    WARN_ON(!connector_state->crtc))
>  			continue;
> -- 
> 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: [PATCH v2 6/6] drm/atomic: Clean up steal_encoder

[Ville Syrjälä]

On Wed, Mar 02, 2016 at 07:32:30PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 24, 2016 at 09:37:33AM +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.
> 
> steal_encoder() can no longer fail after this, so should clean up the
> caller a bit too I think.

Oh and maybe it should be called something different? We've sort of done
the stealing part already, so I think the only thing we have left to do
here is clean up the stale best_encoder pointers.

> 
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 11 ++---------
> >  1 file changed, 2 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 32bd5bebef0b..0fc56339001d 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -215,18 +215,11 @@ steal_encoder(struct drm_atomic_state *state,
> >  	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 ||
> >  		    WARN_ON(!connector_state->crtc))
> >  			continue;
> > -- 
> > 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
> _______________________________________________
> 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