[Intel-gfx] [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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch