[4/4] drm/atomic-helpers: Make encoder picking more robust
diff mbox

Message ID 1438615451-856-4-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Aug. 3, 2015, 3:24 p.m. UTC
We've had a few issues with atomic where subtle bugs in the encoder
picking logic lead to accidental self-stealing of the encoder,
resulting in a NULL connector_state->crtc in update_connector_routing
and subsequent.

Linus applied some duct-tape for an mst regression in

commit 27667f4744fc5a0f3e50910e78740bac5670d18b
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed Jul 29 22:18:16 2015 -0700

    i915: temporary fix for DP MST docking station NULL pointer dereference

But that was incomplete (the code will still oops when debuggin is
enabled) and mangled the state even further. So instead WARN and bail
out as the more future-proof option.

Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Ander Conselvan de Oliveira Aug. 4, 2015, 8:56 a.m. UTC | #1
For the series:

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>


On Mon, 2015-08-03 at 17:24 +0200, Daniel Vetter wrote:
> We've had a few issues with atomic where subtle bugs in the encoder
> picking logic lead to accidental self-stealing of the encoder,
> resulting in a NULL connector_state->crtc in update_connector_routing
> and subsequent.
> 
> Linus applied some duct-tape for an mst regression in
> 
> commit 27667f4744fc5a0f3e50910e78740bac5670d18b
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Wed Jul 29 22:18:16 2015 -0700
> 
>     i915: temporary fix for DP MST docking station NULL pointer dereference
> 
> But that was incomplete (the code will still oops when debuggin is
> enabled) and mangled the state even further. So instead WARN and bail
> out as the more future-proof option.
> 
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8694ca9beee3..9dcc7280e572 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -234,13 +234,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
>  		}
>  	}
>  
> +	if (WARN_ON(!connector_state->crtc))
> +		return -EINVAL;
> +
>  	connector_state->best_encoder = new_encoder;
> -	if (connector_state->crtc) {
> -		idx = drm_crtc_index(connector_state->crtc);
> +	idx = drm_crtc_index(connector_state->crtc);
>  
> -		crtc_state = state->crtc_states[idx];
> -		crtc_state->mode_changed = true;
> -	}
> +	crtc_state = state->crtc_states[idx];
> +	crtc_state->mode_changed = true;
>  
>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
>  			 connector->base.id,
Thierry Reding Aug. 4, 2015, 8:56 a.m. UTC | #2
On Mon, Aug 03, 2015 at 05:24:11PM +0200, Daniel Vetter wrote:
> We've had a few issues with atomic where subtle bugs in the encoder
> picking logic lead to accidental self-stealing of the encoder,
> resulting in a NULL connector_state->crtc in update_connector_routing
> and subsequent.
> 
> Linus applied some duct-tape for an mst regression in
> 
> commit 27667f4744fc5a0f3e50910e78740bac5670d18b
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Wed Jul 29 22:18:16 2015 -0700
> 
>     i915: temporary fix for DP MST docking station NULL pointer dereference
> 
> But that was incomplete (the code will still oops when debuggin is
> enabled) and mangled the state even further. So instead WARN and bail
> out as the more future-proof option.
> 
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8694ca9beee3..9dcc7280e572 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -234,13 +234,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
>  		}
>  	}
>  
> +	if (WARN_ON(!connector_state->crtc))
> +		return -EINVAL;
> +
>  	connector_state->best_encoder = new_encoder;
> -	if (connector_state->crtc) {
> -		idx = drm_crtc_index(connector_state->crtc);
> +	idx = drm_crtc_index(connector_state->crtc);
>  
> -		crtc_state = state->crtc_states[idx];
> -		crtc_state->mode_changed = true;
> -	}
> +	crtc_state = state->crtc_states[idx];
> +	crtc_state->mode_changed = true;
>  
>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
>  			 connector->base.id,

Reviewed-by: Thierry Reding <treding@nvidia.com>
Daniel Vetter Aug. 4, 2015, 9:11 a.m. UTC | #3
On Tue, Aug 04, 2015 at 11:56:08AM +0300, Ander Conselvan De Oliveira wrote:
> For the series:
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

Thanks for the review.
-Daniel
> 
> 
> On Mon, 2015-08-03 at 17:24 +0200, Daniel Vetter wrote:
> > We've had a few issues with atomic where subtle bugs in the encoder
> > picking logic lead to accidental self-stealing of the encoder,
> > resulting in a NULL connector_state->crtc in update_connector_routing
> > and subsequent.
> > 
> > Linus applied some duct-tape for an mst regression in
> > 
> > commit 27667f4744fc5a0f3e50910e78740bac5670d18b
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date:   Wed Jul 29 22:18:16 2015 -0700
> > 
> >     i915: temporary fix for DP MST docking station NULL pointer dereference
> > 
> > But that was incomplete (the code will still oops when debuggin is
> > enabled) and mangled the state even further. So instead WARN and bail
> > out as the more future-proof option.
> > 
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 8694ca9beee3..9dcc7280e572 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -234,13 +234,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
> >  		}
> >  	}
> >  
> > +	if (WARN_ON(!connector_state->crtc))
> > +		return -EINVAL;
> > +
> >  	connector_state->best_encoder = new_encoder;
> > -	if (connector_state->crtc) {
> > -		idx = drm_crtc_index(connector_state->crtc);
> > +	idx = drm_crtc_index(connector_state->crtc);
> >  
> > -		crtc_state = state->crtc_states[idx];
> > -		crtc_state->mode_changed = true;
> > -	}
> > +	crtc_state = state->crtc_states[idx];
> > +	crtc_state->mode_changed = true;
> >  
> >  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
> >  			 connector->base.id,

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8694ca9beee3..9dcc7280e572 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -234,13 +234,14 @@  update_connector_routing(struct drm_atomic_state *state, int conn_idx)
 		}
 	}
 
+	if (WARN_ON(!connector_state->crtc))
+		return -EINVAL;
+
 	connector_state->best_encoder = new_encoder;
-	if (connector_state->crtc) {
-		idx = drm_crtc_index(connector_state->crtc);
+	idx = drm_crtc_index(connector_state->crtc);
 
-		crtc_state = state->crtc_states[idx];
-		crtc_state->mode_changed = true;
-	}
+	crtc_state = state->crtc_states[idx];
+	crtc_state->mode_changed = true;
 
 	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
 			 connector->base.id,