[PATCH 0/3] drm/atomic: Fix encoder stealing.

[PATCH 0/3] drm/atomic: Fix encoder stealing.

[Maarten Lankhorst]

This is a small change to make encoder stealing work more as expected.
First patch is a small cleanup, second patch is a small fix
for a crash I was hitting on my skylake with MST. The third patch
refuses to steal encoders from connectors not part of the state.
I had an alternate version that would restart in that case, but I feel
that in those cases the connector <-> encoder mapping would stay the same
regardless, so it would be easier to understand if we would instead fail
right away.

Maarten Lankhorst (3):
  drm/atomic: Always call steal_encoder.
  drm/atomic: Refuse to steal encoders with index < conn_idx.
  drm/atomic: Refuse to steal encoders from connectors not part of the
    state.

 drivers/gpu/drm/drm_atomic_helper.c | 121 +++++++++++++-----------------------
 1 file changed, 44 insertions(+), 77 deletions(-)

-- 
2.1.0

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

[PATCH 1/3] 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.

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4da4f2a49078..fc1c06b1d8bb 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,8 +149,7 @@ 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;
@@ -182,55 +162,49 @@ steal_encoder(struct drm_atomic_state *state,
 	 */
 	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);
-
-	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) {
+		struct drm_crtc *encoder_crtc;
+
 		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;
 }
 
 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,
+			 int conn_idx)
 {
 	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);
@@ -305,17 +279,12 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
 		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))
@@ -494,7 +463,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, i);
 		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 2/3] drm/atomic: Refuse to steal encoders with index < conn_idx.

[Maarten Lankhorst]

There was a potential to crash in the following case:
[   49.985270] [drm:update_connector_routing] Updating routing for [CONNECTOR:48:DP-3]
[   49.985273] [drm:update_connector_routing] [CONNECTOR:48:DP-3] keeps [ENCODER:33:DP MST-33], now on [CRTC:21:crtc-0]
[   49.985275] [drm:update_connector_routing] Updating routing for [CONNECTOR:51:DP-4]
[   49.985278] [drm:steal_encoder] [ENCODER:33:DP MST-33] in use on [CRTC:21:crtc-0], stealing it
[   49.985281] [drm:update_connector_routing] [CONNECTOR:51:DP-4] using [ENCODER:33:DP MST-33] on [CRTC:21:crtc-0]

This case is not allowed, similar to the previous case of 2 connectors newly assigned to the same encoder.

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fc1c06b1d8bb..fa708559542c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -88,18 +88,18 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 
 static bool
 check_pending_encoder_assignment(struct drm_atomic_state *state,
-				 struct drm_encoder *new_encoder)
+				 struct drm_encoder *new_encoder,
+				 int conn_idx)
 {
 	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;
+		if (i >= conn_idx)
+			break;
 
-		/* encoder already assigned and we're trying to re-steal it! */
-		if (connector->state->best_encoder != conn_state->best_encoder)
+		if (conn_state->best_encoder == new_encoder)
 			return false;
 	}
 
@@ -272,7 +272,7 @@ update_connector_routing(struct drm_atomic_state *state,
 		return 0;
 	}
 
-	if (!check_pending_encoder_assignment(state, new_encoder)) {
+	if (!check_pending_encoder_assignment(state, new_encoder, conn_idx)) {
 		DRM_DEBUG_ATOMIC("Encoder for [CONNECTOR:%d:%s] already assigned\n",
 				 connector->base.id,
 				 connector->name);
-- 
2.1.0

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

[PATCH 3/3] drm/atomic: Refuse to steal encoders from connectors not part of the state.

[Maarten Lankhorst]

Because encoder <-> connector mapping is fixed when not moving to
another crtc we can just reject connectors trying to steal an encoder
from a connector not part of the state. This won't break MST on i915
because in that case connectors will be part of the state if you switch
them between crtc's. If they're not they stay on the same crtc, and
encoder stealing would have failed anyway.

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fa708559542c..b2028d592483 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -92,14 +92,21 @@ check_pending_encoder_assignment(struct drm_atomic_state *state,
 				 int conn_idx)
 {
 	struct drm_connector *connector;
-	struct drm_connector_state *conn_state;
-	int i;
 
-	for_each_connector_in_state(state, connector, conn_state, i) {
-		if (i >= conn_idx)
-			break;
+	drm_for_each_connector(connector, state->dev) {
+		struct drm_connector_state *conn_state =
+			drm_atomic_get_existing_connector_state(state,
+								connector);
 
-		if (conn_state->best_encoder == new_encoder)
+		if (!conn_state) {
+			if (connector->state->best_encoder == new_encoder)
+				return false;
+
+			continue;
+		}
+
+		if (drm_connector_index(connector) < conn_idx &&
+		    conn_state->best_encoder == new_encoder)
 			return false;
 	}
 
@@ -149,29 +156,19 @@ set_best_encoder(struct drm_atomic_state *state,
 
 static int
 steal_encoder(struct drm_atomic_state *state,
-	      struct drm_encoder *encoder)
+	      struct drm_encoder *encoder,
+	      int conn_idx)
 {
-	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;
+	int i;
 
-	/*
-	 * 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));
-
-	list_for_each_entry(connector, &config->connector_list, head) {
+	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 (i >= conn_idx)
+			break;
 
 		if (connector_state->best_encoder != encoder ||
 		    WARN_ON(!connector_state->crtc))
@@ -279,7 +276,7 @@ update_connector_routing(struct drm_atomic_state *state,
 		return -EINVAL;
 	}
 
-	ret = steal_encoder(state, new_encoder);
+	ret = steal_encoder(state, new_encoder, conn_idx);
 	if (ret) {
 		DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n",
 				 connector->base.id,
-- 
2.1.0

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

Re: [PATCH 3/3] drm/atomic: Refuse to steal encoders from connectors not part of the state.

[Daniel Vetter]

On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote:
> Because encoder <-> connector mapping is fixed when not moving to
> another crtc we can just reject connectors trying to steal an encoder
> from a connector not part of the state. This won't break MST on i915
> because in that case connectors will be part of the state if you switch
> them between crtc's. If they're not they stay on the same crtc, and
> encoder stealing would have failed anyway.

We must do this for backwards compat. setCrtc on a connector that needs an
encoder already used on some other crtc is supposed to disable that
encoder (and the entire pipe if it's all unused) if we need it.
-Daniel

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 43 +++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index fa708559542c..b2028d592483 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -92,14 +92,21 @@ check_pending_encoder_assignment(struct drm_atomic_state *state,
>  				 int conn_idx)
>  {
>  	struct drm_connector *connector;
> -	struct drm_connector_state *conn_state;
> -	int i;
>  
> -	for_each_connector_in_state(state, connector, conn_state, i) {
> -		if (i >= conn_idx)
> -			break;
> +	drm_for_each_connector(connector, state->dev) {
> +		struct drm_connector_state *conn_state =
> +			drm_atomic_get_existing_connector_state(state,
> +								connector);
>  
> -		if (conn_state->best_encoder == new_encoder)
> +		if (!conn_state) {
> +			if (connector->state->best_encoder == new_encoder)
> +				return false;
> +
> +			continue;
> +		}
> +
> +		if (drm_connector_index(connector) < conn_idx &&
> +		    conn_state->best_encoder == new_encoder)
>  			return false;
>  	}
>  
> @@ -149,29 +156,19 @@ set_best_encoder(struct drm_atomic_state *state,
>  
>  static int
>  steal_encoder(struct drm_atomic_state *state,
> -	      struct drm_encoder *encoder)
> +	      struct drm_encoder *encoder,
> +	      int conn_idx)
>  {
> -	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;
> +	int i;
>  
> -	/*
> -	 * 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));
> -
> -	list_for_each_entry(connector, &config->connector_list, head) {
> +	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 (i >= conn_idx)
> +			break;
>  
>  		if (connector_state->best_encoder != encoder ||
>  		    WARN_ON(!connector_state->crtc))
> @@ -279,7 +276,7 @@ update_connector_routing(struct drm_atomic_state *state,
>  		return -EINVAL;
>  	}
>  
> -	ret = steal_encoder(state, new_encoder);
> +	ret = steal_encoder(state, new_encoder, conn_idx);
>  	if (ret) {
>  		DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n",
>  				 connector->base.id,
> -- 
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/atomic: Refuse to steal encoders with index < conn_idx.

[Daniel Vetter]

On Thu, Feb 18, 2016 at 09:54:42AM +0100, Maarten Lankhorst wrote:
> There was a potential to crash in the following case:
> [   49.985270] [drm:update_connector_routing] Updating routing for [CONNECTOR:48:DP-3]
> [   49.985273] [drm:update_connector_routing] [CONNECTOR:48:DP-3] keeps [ENCODER:33:DP MST-33], now on [CRTC:21:crtc-0]
> [   49.985275] [drm:update_connector_routing] Updating routing for [CONNECTOR:51:DP-4]
> [   49.985278] [drm:steal_encoder] [ENCODER:33:DP MST-33] in use on [CRTC:21:crtc-0], stealing it
> [   49.985281] [drm:update_connector_routing] [CONNECTOR:51:DP-4] using [ENCODER:33:DP MST-33] on [CRTC:21:crtc-0]
> 
> This case is not allowed, similar to the previous case of 2 connectors newly assigned to the same encoder.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

What index something has is pretty arbitrary, i.e. I don't understand at
all what exactly you're trying to fix here, and what the problem is. Note
that this patch here seems to break the stealing-prevention logic: We're
supposed to steal the first time around, but not move an already stolen
encoder further (to make sure that all connectors in the current set can
be lit up).
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index fc1c06b1d8bb..fa708559542c 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -88,18 +88,18 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
>  
>  static bool
>  check_pending_encoder_assignment(struct drm_atomic_state *state,
> -				 struct drm_encoder *new_encoder)
> +				 struct drm_encoder *new_encoder,
> +				 int conn_idx)
>  {
>  	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;
> +		if (i >= conn_idx)
> +			break;
>  
> -		/* encoder already assigned and we're trying to re-steal it! */
> -		if (connector->state->best_encoder != conn_state->best_encoder)
> +		if (conn_state->best_encoder == new_encoder)
>  			return false;
>  	}
>  
> @@ -272,7 +272,7 @@ update_connector_routing(struct drm_atomic_state *state,
>  		return 0;
>  	}
>  
> -	if (!check_pending_encoder_assignment(state, new_encoder)) {
> +	if (!check_pending_encoder_assignment(state, new_encoder, conn_idx)) {
>  		DRM_DEBUG_ATOMIC("Encoder for [CONNECTOR:%d:%s] already assigned\n",
>  				 connector->base.id,
>  				 connector->name);
> -- 
> 2.1.0
> 
> _______________________________________________
> 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

Re: [Intel-gfx] [PATCH 0/3] drm/atomic: Fix encoder stealing.

[Daniel Vetter]

On Thu, Feb 18, 2016 at 09:54:40AM +0100, Maarten Lankhorst wrote:
> This is a small change to make encoder stealing work more as expected.

I think the first step here is to type in unit tests with specific cases
so that we can actually have a reasonable discussion about what's
"expected". Currently there's lots of shuffling going on in this code.

I think a simple drm_atomic_tests.ko driver which sets up a mostly-faked
drm_device instance would be the fastest way to get there. Later on we
could move some of that into userspace perhaps, but as long as it's a
separate module reloading to re-run tests is pretty fast.
-Daniel

> First patch is a small cleanup, second patch is a small fix
> for a crash I was hitting on my skylake with MST. The third patch
> refuses to steal encoders from connectors not part of the state.
> I had an alternate version that would restart in that case, but I feel
> that in those cases the connector <-> encoder mapping would stay the same
> regardless, so it would be easier to understand if we would instead fail
> right away.
> 
> Maarten Lankhorst (3):
>   drm/atomic: Always call steal_encoder.
>   drm/atomic: Refuse to steal encoders with index < conn_idx.
>   drm/atomic: Refuse to steal encoders from connectors not part of the
>     state.
> 
>  drivers/gpu/drm/drm_atomic_helper.c | 121 +++++++++++++-----------------------
>  1 file changed, 44 insertions(+), 77 deletions(-)
> 
> -- 
> 2.1.0
> 
> _______________________________________________
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/atomic: Refuse to steal encoders from connectors not part of the state.

[Maarten Lankhorst]

Op 18-02-16 om 12:07 schreef Daniel Vetter:
> On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote:
>> Because encoder <-> connector mapping is fixed when not moving to
>> another crtc we can just reject connectors trying to steal an encoder
>> from a connector not part of the state. This won't break MST on i915
>> because in that case connectors will be part of the state if you switch
>> them between crtc's. If they're not they stay on the same crtc, and
>> encoder stealing would have failed anyway.
> We must do this for backwards compat. setCrtc on a connector that needs an
> encoder already used on some other crtc is supposed to disable that
> encoder (and the entire pipe if it's all unused) if we need it.
> -Daniel
>
Could this be done from the setcrtc helper? Seems with atomic that wouldn't be desired behavior.

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

Re: [PATCH 2/3] drm/atomic: Refuse to steal encoders with index < conn_idx.

[Maarten Lankhorst]

Op 18-02-16 om 12:09 schreef Daniel Vetter:
> On Thu, Feb 18, 2016 at 09:54:42AM +0100, Maarten Lankhorst wrote:
>> There was a potential to crash in the following case:
>> [   49.985270] [drm:update_connector_routing] Updating routing for [CONNECTOR:48:DP-3]
>> [   49.985273] [drm:update_connector_routing] [CONNECTOR:48:DP-3] keeps [ENCODER:33:DP MST-33], now on [CRTC:21:crtc-0]
>> [   49.985275] [drm:update_connector_routing] Updating routing for [CONNECTOR:51:DP-4]
>> [   49.985278] [drm:steal_encoder] [ENCODER:33:DP MST-33] in use on [CRTC:21:crtc-0], stealing it
>> [   49.985281] [drm:update_connector_routing] [CONNECTOR:51:DP-4] using [ENCODER:33:DP MST-33] on [CRTC:21:crtc-0]
>>
>> This case is not allowed, similar to the previous case of 2 connectors newly assigned to the same encoder.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> What index something has is pretty arbitrary, i.e. I don't understand at
> all what exactly you're trying to fix here, and what the problem is. Note
> that this patch here seems to break the stealing-prevention logic: We're
> supposed to steal the first time around, but not move an already stolen
> encoder further (to make sure that all connectors in the current set can
> be lit up).
Well update_connector_routing runs over the for_each_connector_in_state, and connector_index linearly increases.

This means that 0...conn_idx have already been assigned, so if you see encoder with those indexes
you can't steal them. With conn_idx+1...n you can still steal it and be assured that the state is sane,
and a new encoder will be assigned by the next call to update_connector_routing.

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

Re: [PATCH 3/3] drm/atomic: Refuse to steal encoders from connectors not part of the state.

[Maarten Lankhorst]

Op 18-02-16 om 12:07 schreef Daniel Vetter:
> On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote:
>> Because encoder <-> connector mapping is fixed when not moving to
>> another crtc we can just reject connectors trying to steal an encoder
>> from a connector not part of the state. This won't break MST on i915
>> because in that case connectors will be part of the state if you switch
>> them between crtc's. If they're not they stay on the same crtc, and
>> encoder stealing would have failed anyway.
> We must do this for backwards compat. setCrtc on a connector that needs an
> encoder already used on some other crtc is supposed to disable that
> encoder (and the entire pipe if it's all unused) if we need it.
Well that won't do what you expect it to do in certain cases..

crtc1 has MST_DP1

setcrtc(crtc1, { MST_DP1, MST_DP2 });

MST_DP1 gets disabled.

or

setcrtc fails with -EINVAL

Seems to me that if you steal an encoder, failing with an error would be better.

But for backward compat we could add some code to allow encoder stealing in the legacy setcrtc helper?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/atomic: Refuse to steal encoders from connectors not part of the state.

[Daniel Vetter]

On Thu, Feb 18, 2016 at 12:18:53PM +0100, Maarten Lankhorst wrote:
> Op 18-02-16 om 12:07 schreef Daniel Vetter:
> > On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote:
> >> Because encoder <-> connector mapping is fixed when not moving to
> >> another crtc we can just reject connectors trying to steal an encoder
> >> from a connector not part of the state. This won't break MST on i915
> >> because in that case connectors will be part of the state if you switch
> >> them between crtc's. If they're not they stay on the same crtc, and
> >> encoder stealing would have failed anyway.
> > We must do this for backwards compat. setCrtc on a connector that needs an
> > encoder already used on some other crtc is supposed to disable that
> > encoder (and the entire pipe if it's all unused) if we need it.
> > -Daniel
> >
> Could this be done from the setcrtc helper? Seems with atomic that wouldn't be desired behavior.

If you want to avoid stealing with atomic, supply _all_ the
connectors/crtcs when doing an atomic modeset. After all the point of
atomic is to do global updates. I don't think it makes sense to have a
special case just for setcrtc, since it makes compat/transition
unecesserily complicated. And we do this kind of stealing in other places
too with public api objects, e.g. if you move a plane.
-Daniel
-- 
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

Re: [PATCH 3/3] drm/atomic: Refuse to steal encoders from connectors not part of the state.

[Ville Syrjälä]

On Thu, Feb 18, 2016 at 01:43:11PM +0100, Daniel Vetter wrote:
> On Thu, Feb 18, 2016 at 12:18:53PM +0100, Maarten Lankhorst wrote:
> > Op 18-02-16 om 12:07 schreef Daniel Vetter:
> > > On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote:
> > >> Because encoder <-> connector mapping is fixed when not moving to
> > >> another crtc we can just reject connectors trying to steal an encoder
> > >> from a connector not part of the state. This won't break MST on i915
> > >> because in that case connectors will be part of the state if you switch
> > >> them between crtc's. If they're not they stay on the same crtc, and
> > >> encoder stealing would have failed anyway.
> > > We must do this for backwards compat. setCrtc on a connector that needs an
> > > encoder already used on some other crtc is supposed to disable that
> > > encoder (and the entire pipe if it's all unused) if we need it.
> > > -Daniel
> > >
> > Could this be done from the setcrtc helper? Seems with atomic that wouldn't be desired behavior.
> 
> If you want to avoid stealing with atomic, supply _all_ the
> connectors/crtcs when doing an atomic modeset. After all the point of
> atomic is to do global updates. I don't think it makes sense to have a
> special case just for setcrtc, since it makes compat/transition
> unecesserily complicated.

I disagree. Having properties change magically is just a bad idea IMO.
As far as checking for conflicts, IIRC I did that with a few bitmasks
in my original atomic code, and it was pretty trivial. The current
stealing  code we have is way too complicated for what it does IMO.

> And we do this kind of stealing in other places
> too with public api objects, e.g. if you move a plane.

Mm. What exactly do we steal with planes?

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

✓ Fi.CI.BAT: success for drm/atomic: Fix encoder stealing.

[Patchwork]

== Summary ==

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

Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (skl-i5k-2) UNSTABLE
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (skl-i5k-2) UNSTABLE
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (byt-nuc) UNSTABLE

bdw-nuci7        total:164  pass:153  dwarn:0   dfail:0   fail:0   skip:11 
bsw-nuc-2        total:167  pass:136  dwarn:1   dfail:0   fail:0   skip:30 
byt-nuc          total:167  pass:142  dwarn:0   dfail:0   fail:0   skip:25 
hsw-gt2          total:167  pass:156  dwarn:0   dfail:0   fail:1   skip:10 
ilk-hp8440p      total:167  pass:117  dwarn:1   dfail:0   fail:1   skip:48 
ivb-t430s        total:167  pass:152  dwarn:0   dfail:0   fail:1   skip:14 
skl-i5k-2        total:167  pass:150  dwarn:1   dfail:0   fail:0   skip:16 
snb-dellxps      total:167  pass:144  dwarn:0   dfail:0   fail:1   skip:22 

Results at /archive/results/CI_IGT_test/Patchwork_1431/

631053ba117c294f0cdfe704718a040b4406a240 drm-intel-nightly: 2016y-02m-17d-21h-05m-38s UTC integration manifest
4eae4c19ce067c1dc15c27a84e32507c66a22d57 drm/atomic: Refuse to steal encoders from connectors not part of the state.
edc4b79064c40a321efbbc16d252f89f086c737a drm/atomic: Refuse to steal encoders with index < conn_idx.
db8c02c5a5bc29223c29fe9f33ead31f56dbf535 drm/atomic: Always call steal_encoder.

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

Re: [PATCH 3/3] drm/atomic: Refuse to steal encoders from connectors not part of the state.

[Maarten Lankhorst]

Hey,

Op 18-02-16 om 13:59 schreef Ville Syrjälä:
> On Thu, Feb 18, 2016 at 01:43:11PM +0100, Daniel Vetter wrote:
>> On Thu, Feb 18, 2016 at 12:18:53PM +0100, Maarten Lankhorst wrote:
>>> Op 18-02-16 om 12:07 schreef Daniel Vetter:
>>>> On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote:
>>>>> Because encoder <-> connector mapping is fixed when not moving to
>>>>> another crtc we can just reject connectors trying to steal an encoder
>>>>> from a connector not part of the state. This won't break MST on i915
>>>>> because in that case connectors will be part of the state if you switch
>>>>> them between crtc's. If they're not they stay on the same crtc, and
>>>>> encoder stealing would have failed anyway.
>>>> We must do this for backwards compat. setCrtc on a connector that needs an
>>>> encoder already used on some other crtc is supposed to disable that
>>>> encoder (and the entire pipe if it's all unused) if we need it.
>>>> -Daniel
>>>>
>>> Could this be done from the setcrtc helper? Seems with atomic that wouldn't be desired behavior.
>> If you want to avoid stealing with atomic, supply _all_ the
>> connectors/crtcs when doing an atomic modeset. After all the point of
>> atomic is to do global updates. I don't think it makes sense to have a
>> special case just for setcrtc, since it makes compat/transition
>> unecesserily complicated.
> I disagree. Having properties change magically is just a bad idea IMO.
> As far as checking for conflicts, IIRC I did that with a few bitmasks
> in my original atomic code, and it was pretty trivial. The current
> stealing  code we have is way too complicated for what it does IMO.
>
>> And we do this kind of stealing in other places
>> too with public api objects, e.g. if you move a plane.
> Mm. What exactly do we steal with planes?
I've sent a v2 that works nicely with "[IGT PATCH] tests/kms_setmode: Add tests when not stealing encoders on same crtc."
For all other calls disabling connectors to steal its encoder is rejected, but the behavior is preserved for set_config only.

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