| Message ID | 1476707838-25253-6-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 17, 2016 at 02:37:04PM +0200, Maarten Lankhorst wrote: > This kills another dereference of connector->state. connector_mask > holds all unchanged connectors at least and any changed connectors > are already in state anyway. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 120775fcfb70..1915ec44f7eb 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1230,8 +1230,13 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, > struct drm_mode_config *config = &state->dev->mode_config; > struct drm_connector *connector; > struct drm_connector_state *conn_state; > + struct drm_crtc_state *crtc_state; > int ret; > > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > + if (IS_ERR(crtc_state)) > + return PTR_ERR(crtc_state); > + > ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx); > if (ret) > return ret; > @@ -1240,11 +1245,11 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, > crtc->base.id, crtc->name, state); > > /* > - * Changed connectors are already in @state, so only need to look at the > - * current configuration. > + * Changed connectors are already in @state, so only need to look > + * at the connector_mask in crtc_state. > */ > drm_for_each_connector(connector, state->dev) { > - if (connector->state->crtc != crtc) > + if (!(crtc_state->connector_mask & (1 << drm_connector_index(connector)))) > continue; So this being the new crtc state, connector_mask will include all newly enabled connectors (these will have been already added to the top level state), and also any connector that was already enabled and isn't getting disabled (these are the ones we need to explicitly add to the top level state here). /me wishes the top level state would be renamed to drm_atomic_transaction or something... Any connector that is getting disabled will already have been added to the top level state as well, so them not being included in the new crtc state's connectors_mask is fine. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > conn_state = drm_atomic_get_connector_state(state, connector); > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Nov 03, 2016 at 05:32:42PM +0200, Ville Syrjälä wrote: > On Mon, Oct 17, 2016 at 02:37:04PM +0200, Maarten Lankhorst wrote: > > This kills another dereference of connector->state. connector_mask > > holds all unchanged connectors at least and any changed connectors > > are already in state anyway. > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 120775fcfb70..1915ec44f7eb 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -1230,8 +1230,13 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, > > struct drm_mode_config *config = &state->dev->mode_config; > > struct drm_connector *connector; > > struct drm_connector_state *conn_state; > > + struct drm_crtc_state *crtc_state; > > int ret; > > > > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > > + if (IS_ERR(crtc_state)) > > + return PTR_ERR(crtc_state); > > + > > ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx); > > if (ret) > > return ret; > > @@ -1240,11 +1245,11 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, > > crtc->base.id, crtc->name, state); > > > > /* > > - * Changed connectors are already in @state, so only need to look at the > > - * current configuration. > > + * Changed connectors are already in @state, so only need to look > > + * at the connector_mask in crtc_state. > > */ > > drm_for_each_connector(connector, state->dev) { > > - if (connector->state->crtc != crtc) > > + if (!(crtc_state->connector_mask & (1 << drm_connector_index(connector)))) > > continue; > > So this being the new crtc state, connector_mask will include all newly enabled > connectors (these will have been already added to the top level state), and > also any connector that was already enabled and isn't getting disabled (these > are the ones we need to explicitly add to the top level state here). > > /me wishes the top level state would be renamed to drm_atomic_transaction or something... +1 on drm_atomic_transaction, that's an awesome name. Not sure how bad it would be to roll this out though ... -Daniel > > Any connector that is getting disabled will already have been added to > the top level state as well, so them not being included in the new crtc > state's connectors_mask is fine. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > conn_state = drm_atomic_get_connector_state(state, connector); > > -- > > 2.7.4 > > > > _______________________________________________ > > 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
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 120775fcfb70..1915ec44f7eb 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1230,8 +1230,13 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, struct drm_mode_config *config = &state->dev->mode_config; struct drm_connector *connector; struct drm_connector_state *conn_state; + struct drm_crtc_state *crtc_state; int ret; + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx); if (ret) return ret; @@ -1240,11 +1245,11 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, crtc->base.id, crtc->name, state); /* - * Changed connectors are already in @state, so only need to look at the - * current configuration. + * Changed connectors are already in @state, so only need to look + * at the connector_mask in crtc_state. */ drm_for_each_connector(connector, state->dev) { - if (connector->state->crtc != crtc) + if (!(crtc_state->connector_mask & (1 << drm_connector_index(connector)))) continue; conn_state = drm_atomic_get_connector_state(state, connector);
This kills another dereference of connector->state. connector_mask holds all unchanged connectors at least and any changed connectors are already in state anyway. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_atomic.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)