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

[Matt Roper <matthew.d.roper@intel.com>]

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

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx