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

From: Ander Conselvan De Oliveira <conselvan2@gmail.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/6] drm/atomic: add connectors_changed to separate it from mode_changed, v2
Date: Thu, 23 Jul 2015 15:27:42 +0300	[thread overview]
Message-ID: <1437654462.3604.10.camel@gmail.com> (raw)
In-Reply-To: <1437478142-416-2-git-send-email-maarten.lankhorst@linux.intel.com>

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
http://lists.freedesktop.org/mailman/listinfo/dri-devel