| Message ID | 1436252911-5703-2-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
On Tue, Jul 07, 2015 at 12:05:40PM +0200, Maarten Lankhorst wrote: > Op 07-07-15 om 10:59 schreef Daniel Vetter: > > On Tue, Jul 07, 2015 at 09:08:12AM +0200, Maarten Lankhorst wrote: > >> @@ -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. Right, I guess I should have read your comment properly and disregarded the kerneldoc ;-) -Daniel
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; + 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 + * @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
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(-)