Re: [PATCH 1/6] drm/atomic: add connectors_changed to separate it from mode_changed, v2

Ander Conselvan De Oliveira
2015-07-23T12:27:42Z

On Tue, 2015-07-21 at 13:28 +0200, Maarten Lankhorst wrote:
> This can be a separate case from mode_changed, when connectors stay
> the
> same but only the mode is different. Drivers may choose to implement
> specific
> optimizations to prevent a full modeset for this case.
>
> Changes since v1:
> - Update kerneldocs slightly.
>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I have a couple of nits below, but anyway,

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> ---
> drivers/gpu/drm/drm_atomic_helper.c | 39
> +++++++++++++++++++++++++++---------
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> include/drm/drm_atomic.h | 3 ++-
> include/drm/drm_crtc.h | 8 +++++---
> 4 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 0ea8c5d476ef..ac6601071414 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -124,7 +124,7 @@ steal_encoder(struct drm_atomic_state *state,
> if (IS_ERR(crtc_state))
> return PTR_ERR(crtc_state);
>
> - crtc_state->mode_changed = true;
> + crtc_state->connectors_changed = true;
>
> list_for_each_entry(connector, &config->connector_list,
> head) {
> if (connector->state->best_encoder != encoder)
> @@ -174,14 +174,14 @@ update_connector_routing(struct
> drm_atomic_state *state, int conn_idx)
> idx = drm_crtc_index(connector->state
> ->crtc);
>
> crtc_state = state->crtc_states[idx];
> - crtc_state->mode_changed = true;
> + crtc_state->connectors_changed = true;
> }
>
> if (connector_state->crtc) {
> idx = drm_crtc_index(connector_state->crtc);
>
> crtc_state = state->crtc_states[idx];
> - crtc_state->mode_changed = true;
> + crtc_state->connectors_changed = true;
> }
> }
>
> @@ -233,7 +233,7 @@ update_connector_routing(struct drm_atomic_state
> *state, int conn_idx)
> idx = drm_crtc_index(connector_state->crtc);
>
> crtc_state = state->crtc_states[idx];
> - crtc_state->mode_changed = true;
> + crtc_state->connectors_changed = true;
>

There's a comment above the call to update_connector_routing() that
mentions setting crtc_state->mode_changed. That should be updated to
reflect the changes above.

> DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on
> [CRTC:%d]\n",
> connector->base.id,
>

[...]

> @@ -338,9 +340,14 @@ mode_fixup(struct drm_atomic_state *state)
> *
> * Check the state object to see if the requested state is physically possible.
> * This does all the crtc and connector related computations for an atomic
> - * update. It computes and updates crtc_state->mode_changed, adds any additional
> - * connectors needed for full modesets and calls down into ->mode_fixup
> - * functions of the driver backend.
> + * update and adds any additional connectors needed for full modesets and calls

The first 'and' should be replaced with a comma.

[...]

Ander
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org