Re: [PATCH v2 01/20] drm/atomic: add connectors_changed to separate it from mode_changed

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 07, 2015 at 09:08:12AM +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.
> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c  | 25 +++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  include/drm/drm_atomic.h             |  3 ++-
>  include/drm/drm_crtc.h               |  8 +++++---
>  4 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 66e76f4f43be..b818e3111380 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;
>  
>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
>  			 connector->base.id,
> @@ -256,7 +256,8 @@ mode_fixup(struct drm_atomic_state *state)
>  	bool ret;
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (!crtc_state->mode_changed)
> +		if (!crtc_state->mode_changed &&
> +		    !crtc_state->connectors_changed)
>  			continue;
>  
>  		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
> @@ -312,7 +313,8 @@ mode_fixup(struct drm_atomic_state *state)
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		const struct drm_crtc_helper_funcs *funcs;
>  
> -		if (!crtc_state->mode_changed)
> +		if (!crtc_state->mode_changed &&
> +		    !crtc_state->connectors_changed)
>  			continue;
>  
>  		funcs = crtc->helper_private;
> @@ -373,7 +375,17 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  		if (crtc->state->enable != crtc_state->enable) {
>  			DRM_DEBUG_ATOMIC("[CRTC:%d] enable changed\n",
>  					 crtc->base.id);
> +
> +			/*
> +			 * For clarity this assignment is done here, but
> +			 * enable == 0 is only true when there are no
> +			 * connectors and a NULL mode.
> +			 *
> +			 * The other way around is true as well. enable != 0
> +			 * iff connectors are attached and a mode is set.
> +			 */
>  			crtc_state->mode_changed = true;

I'd drop this one so that connectors_changed and mode_changed are truly
orthogonal. Also ->enable implies connectors changed since we do check
that there's only connected connectors if the crtc is on. Needs kerneldoc
update too ofc.

> +			crtc_state->connectors_changed = true;
>  		}
>  	}
>  
> @@ -2072,6 +2084,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>  	state->mode_changed = false;
>  	state->active_changed = false;
>  	state->planes_changed = false;
> +	state->connectors_changed = false;
>  	state->event = NULL;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e91613b1c871..6ddb462b4124 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -421,7 +421,7 @@ static const intel_limit_t intel_limits_bxt = {
>  static bool
>  needs_modeset(struct drm_crtc_state *state)
>  {
> -	return state->mode_changed || state->active_changed;
> +	return drm_atomic_crtc_needs_modeset(state);
>  }
>  
>  /**
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 8a3a913320eb..e67aeac2aee0 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -166,7 +166,8 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
>  static inline bool
>  drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
>  {
> -	return state->mode_changed || state->active_changed;
> +	return state->mode_changed || state->active_changed ||
> +	       state->connectors_changed;
>  }
>  
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 57ca8cc383a6..e39d6f1de5f7 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -255,12 +255,13 @@ struct drm_atomic_state;
>   * @crtc: backpointer to the CRTC
>   * @enable: whether the CRTC should be enabled, gates all other state
>   * @active: whether the CRTC is actively displaying (used for DPMS)
> - * @mode_changed: for use by helpers and drivers when computing state updates
> - * @active_changed: for use by helpers and drivers when computing state updates
> + * @planes_changed: planes on this crtc are updated
> + * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
> + * @active_changed: crtc_state->active

missing a "has changed".

> + * @connectors_changed: connectors to this crtc have been updated
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @last_vblank_count: for helpers and drivers to capture the vblank of the
>   * 	update to ensure framebuffer cleanup isn't done too early
> - * @planes_changed: for use by helpers and drivers when computing state updates
>   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
>   * @mode: current mode timings
>   * @event: optional pointer to a DRM event to signal upon completion of the
> @@ -283,6 +284,7 @@ struct drm_crtc_state {
>  	bool planes_changed : 1;
>  	bool mode_changed : 1;
>  	bool active_changed : 1;
> +	bool connectors_changed : 1;
>  
>  	/* attached planes bitmask:
>  	 * WARNING: transitional helpers do not maintain plane_mask so
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel