| Message ID | 1448357676-27837-9-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 24, 2015 at 10:34:35AM +0100, Maarten Lankhorst wrote: [...] > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index cee31d43cd1c..fb79c54b2aed 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -420,8 +420,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > * crtc only changed its mode but has the same set of connectors. > */ > for_each_crtc_in_state(state, crtc, crtc_state, i) { > - int num_connectors; > - > /* > * We must set ->active_changed after walking connectors for > * otherwise an update that only changes active would result in > @@ -449,10 +447,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > if (ret != 0) > return ret; > > - num_connectors = drm_atomic_connectors_for_crtc(state, > - crtc); > - > - if (crtc_state->enable != !!num_connectors) { > + if (crtc_state->enable != !!crtc_state->connector_mask) { I have difficulty to doubly negate in my head, so something like this would be a lot clearer in my opinion: bool enable = crtc_state->connector_mask != 0; ... if (crtc_state->enable != enable) ... Or perhaps even: bool disable = crtc_state->connector_mask == 0; ... if (crtc_state->enable == disable) ... > DRM_DEBUG_ATOMIC("[CRTC:%d] enabled/connectors mismatch\n", > crtc->base.id); > > @@ -1668,7 +1663,7 @@ static int update_output_state(struct drm_atomic_state *state, > if (crtc == set->crtc) > continue; > > - if (!drm_atomic_connectors_for_crtc(state, crtc)) { > + if (!crtc_state->connector_mask) { Similarly I think it would be more natural to say: if (crtc->state->connector_mask == 0) here. Anyway, this is mostly about personal taste, and the change looks correct to me (after checking twice that I got the double negation right), so: Reviewed-by: Thierry Reding <treding@nvidia.com>
Op 07-12-15 om 11:34 schreef Thierry Reding: > On Tue, Nov 24, 2015 at 10:34:35AM +0100, Maarten Lankhorst wrote: > [...] >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index cee31d43cd1c..fb79c54b2aed 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -420,8 +420,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, >> * crtc only changed its mode but has the same set of connectors. >> */ >> for_each_crtc_in_state(state, crtc, crtc_state, i) { >> - int num_connectors; >> - >> /* >> * We must set ->active_changed after walking connectors for >> * otherwise an update that only changes active would result in >> @@ -449,10 +447,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, >> if (ret != 0) >> return ret; >> >> - num_connectors = drm_atomic_connectors_for_crtc(state, >> - crtc); >> - >> - if (crtc_state->enable != !!num_connectors) { >> + if (crtc_state->enable != !!crtc_state->connector_mask) { > I have difficulty to doubly negate in my head, so something like this > would be a lot clearer in my opinion: Maybe if enable == !connector_mask? > bool enable = crtc_state->connector_mask != 0; > > ... > > if (crtc_state->enable != enable) > ... > > Or perhaps even: > > bool disable = crtc_state->connector_mask == 0; > > ... > > if (crtc_state->enable == disable) > ... > >> DRM_DEBUG_ATOMIC("[CRTC:%d] enabled/connectors mismatch\n", >> crtc->base.id); >> >> @@ -1668,7 +1663,7 @@ static int update_output_state(struct drm_atomic_state *state, >> if (crtc == set->crtc) >> continue; >> >> - if (!drm_atomic_connectors_for_crtc(state, crtc)) { >> + if (!crtc_state->connector_mask) { > Similarly I think it would be more natural to say: > > if (crtc->state->connector_mask == 0) > > here. > > Anyway, this is mostly about personal taste, and the change looks > correct to me (after checking twice that I got the double negation > right), so: > > Reviewed-by: Thierry Reding <treding@nvidia.com>
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 5789649a4099..9db2941f636e 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1162,35 +1162,6 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state, EXPORT_SYMBOL(drm_atomic_add_affected_planes); /** - * drm_atomic_connectors_for_crtc - count number of connected outputs - * @state: atomic state - * @crtc: DRM crtc - * - * This function counts all connectors which will be connected to @crtc - * according to @state. Useful to recompute the enable state for @crtc. - */ -int -drm_atomic_connectors_for_crtc(struct drm_atomic_state *state, - struct drm_crtc *crtc) -{ - struct drm_connector *connector; - struct drm_connector_state *conn_state; - - int i, num_connected_connectors = 0; - - for_each_connector_in_state(state, connector, conn_state, i) { - if (conn_state->crtc == crtc) - num_connected_connectors++; - } - - DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d]\n", - state, num_connected_connectors, crtc->base.id); - - return num_connected_connectors; -} -EXPORT_SYMBOL(drm_atomic_connectors_for_crtc); - -/** * drm_atomic_legacy_backoff - locking backoff for legacy ioctls * @state: atomic state * diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index cee31d43cd1c..fb79c54b2aed 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -420,8 +420,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, * crtc only changed its mode but has the same set of connectors. */ for_each_crtc_in_state(state, crtc, crtc_state, i) { - int num_connectors; - /* * We must set ->active_changed after walking connectors for * otherwise an update that only changes active would result in @@ -449,10 +447,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (ret != 0) return ret; - num_connectors = drm_atomic_connectors_for_crtc(state, - crtc); - - if (crtc_state->enable != !!num_connectors) { + if (crtc_state->enable != !!crtc_state->connector_mask) { DRM_DEBUG_ATOMIC("[CRTC:%d] enabled/connectors mismatch\n", crtc->base.id); @@ -1668,7 +1663,7 @@ static int update_output_state(struct drm_atomic_state *state, if (crtc == set->crtc) continue; - if (!drm_atomic_connectors_for_crtc(state, crtc)) { + if (!crtc_state->connector_mask) { ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL); if (ret < 0) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 7a9f4768591e..d62a541110a3 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -327,7 +327,7 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, /* The pixelvalve can only feed one encoder (and encoders are * 1:1 with connectors.) */ - if (drm_atomic_connectors_for_crtc(state->state, crtc) > 1) + if (hweight32(state->connector_mask) > 1) return -EINVAL; drm_atomic_crtc_state_for_each_plane(plane, state) { diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 4b74c97d297a..cdc97589b5ce 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -130,10 +130,6 @@ int __must_check drm_atomic_add_affected_planes(struct drm_atomic_state *state, struct drm_crtc *crtc); -int -drm_atomic_connectors_for_crtc(struct drm_atomic_state *state, - struct drm_crtc *crtc); - void drm_atomic_legacy_backoff(struct drm_atomic_state *state); void
Now that connector_mask is reliable there's no need for this function any more. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_atomic.c | 29 ----------------------------- drivers/gpu/drm/drm_atomic_helper.c | 9 ++------- drivers/gpu/drm/vc4/vc4_crtc.c | 2 +- include/drm/drm_atomic.h | 4 ---- 4 files changed, 3 insertions(+), 41 deletions(-)