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@xxxxxxxxxxxxxxxxxxxxx
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
