| Message ID | 1441850275-20353-5-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 10-09-15 om 03:57 schreef Matt Roper: > As suggested by Ville, the general flow should now roughly follow: > > // whatever the user wanted > compute_final_atomic_state() > > // include all crtcs in the intermediate state which are > // getting disabled (even temporarily to perform a modeset) > compute_intermediate_atomic_state() > > ret = check_state_change(old, intermediate) > ret = check_state_change(intermediate, new) > > // commit all planes in one go to make them pop out as > // atomically as possible > for_each_crtc_in(intermediate) { > commit_planes() > } > > for_each_crtc_in(intermediate) { > disable_crtc() > } > > for_each_crtc_in(new) { > if (!currently_active) > crtc_enable() > } > > // commit all planes in one go to make them pop in as atomically > // as possible > for_each_crtc_in(new) { > commit_planes() > } > > v2: Because we're potentially performing two state swaps here, the > actual set of FB's that need to be cleaned up at the end of the > process may need to be fetched from the intermediate state rather > than the final state, so use our own intel_cleanup_planes() rather > than the helper version. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > I know Maarten had some reservations about this approach, so hopefully > providing an implementation will allow us to continue the discussion and come > to an agreement on whether or not intermediate states are the way to go. I still don't like it. Intermediate wm's should be calculated in the check function, if it can potentially fail. The final state should be swapped in right away, not any intermediate state or async modesets will never work. And nothing should depend on the current state in the crtc_disable callbacks, if it does it's a bug or it needs to get passed the old crtc_state so it knows what to disable. This is probably why crtc->config is not dead yet. Not committing DPLL state right after swap_state is a special case right now, but that's easily fixed by changing pll->active from a refcount to a crtc mask. ~Maarten
On Thu, Sep 10, 2015 at 07:40:25AM +0200, Maarten Lankhorst wrote: > Op 10-09-15 om 03:57 schreef Matt Roper: > > As suggested by Ville, the general flow should now roughly follow: > > > > // whatever the user wanted > > compute_final_atomic_state() > > > > // include all crtcs in the intermediate state which are > > // getting disabled (even temporarily to perform a modeset) > > compute_intermediate_atomic_state() > > > > ret = check_state_change(old, intermediate) > > ret = check_state_change(intermediate, new) > > > > // commit all planes in one go to make them pop out as > > // atomically as possible > > for_each_crtc_in(intermediate) { > > commit_planes() > > } > > > > for_each_crtc_in(intermediate) { > > disable_crtc() > > } > > > > for_each_crtc_in(new) { > > if (!currently_active) > > crtc_enable() > > } > > > > // commit all planes in one go to make them pop in as atomically > > // as possible > > for_each_crtc_in(new) { > > commit_planes() > > } > > > > v2: Because we're potentially performing two state swaps here, the > > actual set of FB's that need to be cleaned up at the end of the > > process may need to be fetched from the intermediate state rather > > than the final state, so use our own intel_cleanup_planes() rather > > than the helper version. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > I know Maarten had some reservations about this approach, so hopefully > > providing an implementation will allow us to continue the discussion and come > > to an agreement on whether or not intermediate states are the way to go. > I still don't like it. Intermediate wm's should be calculated in the check function, if it > can potentially fail. You're mixing intermediate wms and intermediate atomic state. What the latter buys us is: - easy to reason about things: pipe/planes/etc. can only transition on<->off, no need for those silly needs_modeset checks all over the place. - watermark _programming_ happens naturally from plane commit, no need to sprinkle wm updates all over the place. there would an intermediate and final wm values for both atomic states. The only special case would probably be a single wm update call after the pipe has been disabled, and that's only because we probably won't get a vblank irq there anymore. - cxsr programming also happens naturally from the wm updates. With the current way of doing things, the only reasonable way of making this work is probably to force disable cxsr around the entire modeset. > The final state should be swapped in right away, not any intermediate state or async > modesets will never work. I think that's just a big failing of the current atomic design. If we actually had proper separation of the user state and internal state we wouldn't have any problems. > And nothing should depend on the current state in the crtc_disable callbacks, if it > does it's a bug or it needs to get passed the old crtc_state so it knows what to disable. > This is probably why crtc->config is not dead yet. > > Not committing DPLL state right after swap_state is a special case right now, but > that's easily fixed by changing pll->active from a refcount to a crtc mask. > > ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5081a9e..d63ad1d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13027,6 +13027,36 @@ static int intel_atomic_check(struct drm_device *dev, return intel_atomic_check_planes(state->dev, state); } +/* + * An Intel-specific version of drm_atomic_helper_cleanup_planes(). + * Since we've potentially swapped to an intermediate state before + * swapping again to the final state, the actual framebuffers that + * need to be cleaned up will have been swapped into the intermediate + * state object for any CRTC's that are undergoing a modeset, so we + * need to grab them from there instead of the old_state object. + */ +static void +intel_cleanup_planes(struct drm_device *dev, + struct drm_atomic_state *old_state) +{ + struct drm_atomic_state *inter = + to_intel_atomic_state(old_state)->intermediate; + struct drm_plane *plane; + struct drm_plane_state *plane_state; + int i; + + for_each_plane_in_state(old_state, plane, plane_state, i) { + const struct drm_plane_helper_funcs *funcs; + + funcs = plane->helper_private; + + plane_state = inter->plane_states[i] ?: plane_state; + + if (funcs->cleanup_fb) + funcs->cleanup_fb(plane, plane_state); + } +} + /** * intel_atomic_commit - commit validated state object * @dev: DRM device @@ -13048,11 +13078,12 @@ static int intel_atomic_commit(struct drm_device *dev, bool async) { struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_atomic_state *istate = to_intel_atomic_state(state); + struct drm_atomic_state *inter = istate->intermediate; struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *old_crtc_state; int ret = 0; int i; - bool any_ms = false; if (async) { DRM_DEBUG_KMS("i915 does not yet support async commit\n"); @@ -13063,59 +13094,84 @@ static int intel_atomic_commit(struct drm_device *dev, if (ret) return ret; - drm_atomic_helper_swap_state(dev, state); + if (!istate->any_ms) { + /* + * If there's no modeset involved, just move to the final state + * and jump directly to the final plane updates. + */ + drm_atomic_helper_swap_state(dev, state); + intel_modeset_update_crtc_state(state); + goto nomodeset; + } - for_each_crtc_in_state(state, crtc, crtc_state, i) { - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + /* Swap in intermediate state */ + drm_atomic_helper_swap_state(dev, inter); - if (!needs_modeset(crtc->state)) - continue; + /* + * Commit all planes in one go to make them pop out as atomically + * as possible. + */ + for_each_crtc_in_state(inter, crtc, old_crtc_state, i) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - any_ms = true; intel_pre_plane_update(intel_crtc); + drm_atomic_helper_commit_planes_on_crtc(old_crtc_state); + intel_post_plane_update(intel_crtc); + } + + for_each_crtc_in_state(inter, crtc, old_crtc_state, i) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - if (crtc_state->active) { - intel_crtc_disable_planes(crtc, crtc_state->plane_mask); + if (old_crtc_state->active) { + intel_crtc_disable_planes(crtc, + old_crtc_state->plane_mask); dev_priv->display.crtc_disable(crtc); intel_crtc->active = false; intel_disable_shared_dpll(intel_crtc); } } + /* Commit final state to the underlying crtc/plane objects */ + drm_atomic_helper_swap_state(dev, state); + /* Only after disabling all output pipelines that will be changed can we * update the the output configuration. */ intel_modeset_update_crtc_state(state); + intel_shared_dpll_commit(state); - if (any_ms) { - intel_shared_dpll_commit(state); - - drm_atomic_helper_update_legacy_modeset_state(state->dev, state); - modeset_update_crtc_power_domains(state); - } + drm_atomic_helper_update_legacy_modeset_state(state->dev, state); + modeset_update_crtc_power_domains(state); - /* Now enable the clocks, plane, pipe, and connectors that we set up. */ - for_each_crtc_in_state(state, crtc, crtc_state, i) { - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + /* Now enable the clocks, pipe, and connectors that we set up. */ + for_each_crtc_in_state(state, crtc, old_crtc_state, i) { bool modeset = needs_modeset(crtc->state); if (modeset && crtc->state->active) { update_scanline_offset(to_intel_crtc(crtc)); dev_priv->display.crtc_enable(crtc); } + } - if (!modeset) - intel_pre_plane_update(intel_crtc); +nomodeset: - drm_atomic_helper_commit_planes_on_crtc(crtc_state); + /* + * Commit all planes in one go to make them pop in as atomically as + * possible. + */ + for_each_crtc_in_state(state, crtc, old_crtc_state, i) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + + intel_pre_plane_update(intel_crtc); + drm_atomic_helper_commit_planes_on_crtc(old_crtc_state); intel_post_plane_update(intel_crtc); } /* FIXME: add subpixel order */ drm_atomic_helper_wait_for_vblanks(dev, state); - drm_atomic_helper_cleanup_planes(dev, state); + intel_cleanup_planes(dev, state); - if (any_ms) + if (istate->any_ms) intel_modeset_check_state(dev, state); drm_atomic_state_free(state);
As suggested by Ville, the general flow should now roughly follow: // whatever the user wanted compute_final_atomic_state() // include all crtcs in the intermediate state which are // getting disabled (even temporarily to perform a modeset) compute_intermediate_atomic_state() ret = check_state_change(old, intermediate) ret = check_state_change(intermediate, new) // commit all planes in one go to make them pop out as // atomically as possible for_each_crtc_in(intermediate) { commit_planes() } for_each_crtc_in(intermediate) { disable_crtc() } for_each_crtc_in(new) { if (!currently_active) crtc_enable() } // commit all planes in one go to make them pop in as atomically // as possible for_each_crtc_in(new) { commit_planes() } v2: Because we're potentially performing two state swaps here, the actual set of FB's that need to be cleaned up at the end of the process may need to be fetched from the intermediate state rather than the final state, so use our own intel_cleanup_planes() rather than the helper version. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- I know Maarten had some reservations about this approach, so hopefully providing an implementation will allow us to continue the discussion and come to an agreement on whether or not intermediate states are the way to go. drivers/gpu/drm/i915/intel_display.c | 104 +++++++++++++++++++++++++++-------- 1 file changed, 80 insertions(+), 24 deletions(-)