[4/4] drm/i915: Update modeset programming to use intermediate state (v2)
diff mbox

Message ID 1441850275-20353-5-git-send-email-matthew.d.roper@intel.com
State New
Headers show

Commit Message

Matt Roper Sept. 10, 2015, 1:57 a.m. UTC
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(-)

Comments

Maarten Lankhorst Sept. 10, 2015, 5:40 a.m. UTC | #1
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
Ville Syrjälä Sept. 22, 2015, 7:55 p.m. UTC | #2
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

Patch
diff mbox

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);