Re: [PATCH 8/9] drm/atomic: Remove drm_atomic_connectors_for_crtc.

Thierry Reding
2015-12-07T10:34:44Z

[-- Attachment #1.1: Type: text/plain, Size: 2005 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]