| Message ID | 1455785683-5477-3-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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);
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(-)