[Intel-gfx] [PATCH 4/4] drm/i915: Update modeset programming to use intermediate state (v2)

[Ville Syrjälä]

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 at linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper at 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

-- 
Ville Syrjälä
Intel OTC