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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 07-07-15 om 10:59 schreef Daniel Vetter:
> 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.

They are orthogonal, think of this case:

1. crtc previously enabled, connector removed, mode stays same ->
	connector_changed = true, mode_changed = false

2. crtc previously enabled, connectors stay the same, different mode -> 
	connectors_changed = false, mode_changed = true

The following is enforced by the checks:
crtc disabled, implies 0 connectors, no mode.
crtc enabled implies > 0 connectors, and a mode.

Hence the connectors_changed and mode_changed here are for documentation purposes only. :)

So if someone wonders what happens when enable is changed they don't have to dig through
the entire drm_atomic_helper_check_modeset function and still not be sure if it's coincidence
or not. 

You're right about the kerneldoc, I'll fix it.

>> +			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".
Oops, indeed!

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel