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 at nvidia.com>