drm/i915: Refactor work that can sleep out of commit (v7)

Once we integrate our work into the atomic pipeline, plane commit
operations will need to happen with interrupts disabled, due to vblank
evasion.  Our commit functions today include sleepable work, so those
operations need to be split out and run either before or after the
atomic register programming.

The solution here calculates which of those operations will need to be
performed during the 'check' phase and sets flags in an intel_crtc
sub-struct.  New intel_begin_crtc_commit() and
intel_finish_crtc_commit() functions are added before and after the
actual register programming; these will eventually be called from the
atomic plane helper's .atomic_begin() and .atomic_end() entrypoints.

v2: Fix broken sprite code split

v3: Make the pre/post commit work crtc-based to match how we eventually
    want this to be called from the atomic plane helpers.

v4: Some platforms that haven't had their watermark code reworked were
    waiting for vblank, then calling update_sprite_watermarks in their
    platform-specific disable code.  These also need to be flagged out
    of the critical section.

v5: Sprite plane test for primary show/hide should just set the flag to
    wait for pending flips, not actually perform the wait.  (Ander)

v6:
 - Rebase onto latest di-nightly; picks up an important runtime PM fix.
 - Handle 'wait_for_flips' flag in intel_begin_crtc_commit(). (Ander)
 - Use wait_for_flips flag for primary plane update rather than
   performing the wait in the check routine.
 - Added kerneldoc to pre_disable/post_enable functions that are no
   longer static.  (Ander)
 - Replace assert_pipe_enabled() in intel_disable_primary_hw_plane()
   with an intel_crtc->active test; it turns out assert_pipe_enabled()
   grabs some mutexes and can sleep, which we can't do with interrupts
   disabled.

v7:
 - Check for fb != NULL when deciding whether the sprite plane hides the
   primary plane during a sprite update.  (PRTS)
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

drivers/gpu/drm/i915/intel_display.c

@@ -2165,7 +2165,8 @@ static void intel_disable_primary_hw_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
+	if (WARN_ON(!intel_crtc->active))
+		return;
 
 	if (!intel_crtc->primary_enabled)
 		return;
@@ -11737,7 +11738,11 @@ static int
 intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = state->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
@@ -11752,10 +11757,40 @@ intel_check_primary_plane(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
-	intel_crtc_wait_for_pending_flips(crtc);
-	if (intel_crtc_has_pending_flip(crtc)) {
-		DRM_ERROR("pipe is still busy with an old pageflip\n");
-		return -EBUSY;
+	if (intel_crtc->active) {
+		intel_crtc->atomic.wait_for_flips = true;
+
+		/*
+		 * FBC does not work on some platforms for rotated
+		 * planes, so disable it when rotation is not 0 and
+		 * update it when rotation is set back to 0.
+		 *
+		 * FIXME: This is redundant with the fbc update done in
+		 * the primary plane enable function except that that
+		 * one is done too late. We eventually need to unify
+		 * this.
+		 */
+		if (intel_crtc->primary_enabled &&
+		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
+		    dev_priv->fbc.plane == intel_crtc->plane &&
+		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
+			intel_crtc->atomic.disable_fbc = true;
+		}
+
+		if (state->visible) {
+			/*
+			 * BDW signals flip done immediately if the plane
+			 * is disabled, even if the plane enable is already
+			 * armed to occur at the next vblank :(
+			 */
+			if (IS_BROADWELL(dev) && !intel_crtc->primary_enabled)
+				intel_crtc->atomic.wait_vblank = true;
+		}
+
+		intel_crtc->atomic.fb_bits |=
+			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
+
+		intel_crtc->atomic.update_fbc = true;
 	}
 
 	return 0;
@@ -11773,18 +11808,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_rect *src = &state->src;
-	enum pipe pipe = intel_plane->pipe;
-
-	if (!fb) {
-		/*
-		 * 'prepare' is never called when plane is being disabled, so
-		 * we need to handle frontbuffer tracking here
-		 */
-		mutex_lock(&dev->struct_mutex);
-		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
-				  INTEL_FRONTBUFFER_PRIMARY(pipe));
-		mutex_unlock(&dev->struct_mutex);
-	}
 
 	plane->fb = fb;
 	crtc->x = src->x1 >> 16;
@@ -11801,26 +11824,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
-		/*
-		 * FBC does not work on some platforms for rotated
-		 * planes, so disable it when rotation is not 0 and
-		 * update it when rotation is set back to 0.
-		 *
-		 * FIXME: This is redundant with the fbc update done in
-		 * the primary plane enable function except that that
-		 * one is done too late. We eventually need to unify
-		 * this.
-		 */
-		if (intel_crtc->primary_enabled &&
-		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-		    dev_priv->fbc.plane == intel_crtc->plane &&
-		    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
-			intel_fbc_disable(dev);
-		}
-
 		if (state->visible) {
-			bool was_enabled = intel_crtc->primary_enabled;
-
 			/* FIXME: kill this fastboot hack */
 			intel_update_pipe_size(intel_crtc);
 
@@ -11828,14 +11832,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 
 			dev_priv->display.update_primary_plane(crtc, plane->fb,
 					crtc->x, crtc->y);
-
-			/*
-			 * BDW signals flip done immediately if the plane
-			 * is disabled, even if the plane enable is already
-			 * armed to occur at the next vblank :(
-			 */
-			if (IS_BROADWELL(dev) && !was_enabled)
-				intel_wait_for_vblank(dev, intel_crtc->pipe);
 		} else {
 			/*
 			 * If clipping results in a non-visible primary plane,
@@ -11846,13 +11842,59 @@ intel_commit_primary_plane(struct drm_plane *plane,
 			 */
 			intel_disable_primary_hw_plane(plane, crtc);
 		}
+	}
+}
+
+static void intel_begin_crtc_commit(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+	if (intel_crtc->atomic.wait_for_flips)
+		intel_crtc_wait_for_pending_flips(crtc);
 
+	if (intel_crtc->atomic.disable_fbc)
+		intel_fbc_disable(dev);
+
+	if (intel_crtc->atomic.pre_disable_primary)
+		intel_pre_disable_primary(crtc);
+
+	if (intel_crtc->atomic.update_wm)
+		intel_update_watermarks(crtc);
+
+	intel_runtime_pm_get(dev_priv);
+}
+
+static void intel_finish_crtc_commit(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_plane *p;
+
+	intel_runtime_pm_put(dev_priv);
+
+	if (intel_crtc->atomic.wait_vblank)
+		intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
+
+	if (intel_crtc->atomic.update_fbc) {
 		mutex_lock(&dev->struct_mutex);
 		intel_fbc_update(dev);
 		mutex_unlock(&dev->struct_mutex);
 	}
+
+	if (intel_crtc->atomic.post_enable_primary)
+		intel_post_enable_primary(crtc);
+
+	drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
+		if (intel_crtc->atomic.update_sprite_watermarks & drm_plane_index(p))
+			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
+						       false, false);
+
+	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
 }
 
 int
@@ -11863,9 +11905,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   uint32_t src_w, uint32_t src_h)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_framebuffer *old_fb = plane->fb;
-	struct intel_plane_state state;
+	struct intel_plane_state state = {{ 0 }};
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret;
@@ -11903,9 +11944,33 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 			return ret;
 	}
 
-	intel_runtime_pm_get(dev_priv);
+	if (!state.base.fb) {
+		unsigned fb_bits = 0;
+
+		switch (plane->type) {
+		case DRM_PLANE_TYPE_PRIMARY:
+			fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
+			break;
+		case DRM_PLANE_TYPE_CURSOR:
+			fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
+			break;
+		case DRM_PLANE_TYPE_OVERLAY:
+			fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
+			break;
+		}
+
+		/*
+		 * 'prepare' is never called when plane is being disabled, so
+		 * we need to handle frontbuffer tracking here
+		 */
+		mutex_lock(&dev->struct_mutex);
+		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, fb_bits);
+		mutex_unlock(&dev->struct_mutex);
+	}
+
+	intel_begin_crtc_commit(crtc);
 	intel_plane->commit_plane(plane, &state);
-	intel_runtime_pm_put(dev_priv);
+	intel_finish_crtc_commit(crtc);
 
 	if (fb != old_fb && old_fb) {
 		if (intel_crtc->active)
@@ -12012,6 +12077,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int crtc_w, crtc_h;
 	unsigned stride;
 	int ret;
@@ -12027,7 +12093,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 
 	/* if we want to turn off the cursor ignore width and height */
 	if (!obj)
-		return 0;
+		goto finish;
 
 	/* Check for which cursor types we support */
 	crtc_w = drm_rect_width(&state->orig_dst);
@@ -12054,6 +12120,16 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	}
 	mutex_unlock(&dev->struct_mutex);
 
+finish:
+	if (intel_crtc->active) {
+		if (intel_crtc->cursor_width !=
+		    drm_rect_width(&state->orig_dst))
+			intel_crtc->atomic.update_wm = true;
+
+		intel_crtc->atomic.fb_bits |=
+			INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
+	}
+
 	return ret;
 }
 
@@ -12066,9 +12142,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
-	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width;
 	uint32_t addr;
 
 	plane->fb = state->base.fb;
@@ -12088,17 +12161,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	if (intel_crtc->cursor_bo == obj)
 		goto update;
 
-	/*
-	 * 'prepare' is only called when fb != NULL; we still need to update
-	 * frontbuffer tracking for the 'disable' case here.
-	 */
-	if (!obj) {
-		mutex_lock(&dev->struct_mutex);
-		i915_gem_track_fb(old_obj, NULL,
-				  INTEL_FRONTBUFFER_CURSOR(pipe));
-		mutex_unlock(&dev->struct_mutex);
-	}
-
 	if (!obj)
 		addr = 0;
 	else if (!INTEL_INFO(dev)->cursor_needs_physical)
@@ -12109,18 +12171,11 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	intel_crtc->cursor_addr = addr;
 	intel_crtc->cursor_bo = obj;
 update:
-	old_width = intel_crtc->cursor_width;
-
 	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
 	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
 
-	if (intel_crtc->active) {
-		if (old_width != intel_crtc->cursor_width)
-			intel_update_watermarks(crtc);
+	if (intel_crtc->active)
 		intel_crtc_update_cursor(crtc, state->visible);
-
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
-	}
 }
 
 static const struct drm_plane_funcs intel_cursor_plane_funcs = {

drivers/gpu/drm/i915/intel_drv.h

@@ -251,6 +251,12 @@ struct intel_plane_state {
 	struct drm_rect orig_src;
 	struct drm_rect orig_dst;
 	bool visible;
+
+	/*
+	 * used only for sprite planes to determine when to implicitly
+	 * enable/disable the primary plane
+	 */
+	bool hides_primary;
 };
 
 struct intel_plane_config {
@@ -415,6 +421,27 @@ struct skl_pipe_wm {
 	uint32_t linetime;
 };
 
+/*
+ * Tracking of operations that need to be performed at the beginning/end of an
+ * atomic commit, outside the atomic section where interrupts are disabled.
+ * These are generally operations that grab mutexes or might otherwise sleep
+ * and thus can't be run with interrupts disabled.
+ */
+struct intel_crtc_atomic_commit {
+	/* Sleepable operations to perform before commit */
+	bool wait_for_flips;
+	bool disable_fbc;
+	bool pre_disable_primary;
+	bool update_wm;
+
+	/* Sleepable operations to perform after commit */
+	unsigned fb_bits;
+	bool wait_vblank;
+	bool update_fbc;
+	bool post_enable_primary;
+	unsigned update_sprite_watermarks;
+};
+
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
@@ -468,6 +495,8 @@ struct intel_crtc {
 
 	int scanline_offset;
 	struct intel_mmio_flip mmio_flip;
+
+	struct intel_crtc_atomic_commit atomic;
 };
 
 struct intel_plane_wm_parameters {
@@ -1215,6 +1244,8 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
 bool intel_pipe_update_start(struct intel_crtc *crtc,
 			     uint32_t *start_vbl_count);
 void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
+void intel_post_enable_primary(struct drm_crtc *crtc);
+void intel_pre_disable_primary(struct drm_crtc *crtc);
 
 /* intel_tv.c */
 void intel_tv_init(struct drm_device *dev);

drivers/gpu/drm/i915/intel_sprite.c

@@ -771,9 +771,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
 	 */
-	intel_wait_for_vblank(dev, pipe);
-
-	intel_update_sprite_watermarks(plane, crtc, 0, 0, 0, false, false);
+	intel_crtc->atomic.wait_vblank = true;
+	intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
 static int
@@ -976,12 +975,21 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
 	 */
-	intel_wait_for_vblank(dev, pipe);
-
-	intel_update_sprite_watermarks(plane, crtc, 0, 0, 0, false, false);
+	intel_crtc->atomic.wait_vblank = true;
+	intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
-static void
+/**
+ * intel_post_enable_primary - Perform operations after enabling primary plane
+ * @crtc: the CRTC whose primary plane was just enabled
+ *
+ * Performs potentially sleeping operations that must be done after the primary
+ * plane is enabled, such as updating FBC and IPS.  Note that this may be
+ * called due to an explicit primary plane update, or due to an implicit
+ * re-enable that is caused when a sprite plane is updated to no longer
+ * completely hide the primary plane.
+ */
+void
 intel_post_enable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -1008,7 +1016,17 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	mutex_unlock(&dev->struct_mutex);
 }
 
-static void
+/**
+ * intel_pre_disable_primary - Perform operations before disabling primary plane
+ * @crtc: the CRTC whose primary plane is to be disabled
+ *
+ * Performs potentially sleeping operations that must be done before the
+ * primary plane is enabled, such as updating FBC and IPS.  Note that this may
+ * be called due to an explicit primary plane update, or due to an implicit
+ * disable that is caused when a sprite plane completely hides the primary
+ * plane.
+ */
+void
 intel_pre_disable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -1113,7 +1131,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 
 	if (!fb) {
 		state->visible = false;
-		return 0;
+		goto finish;
 	}
 
 	/* Don't modify another pipe's plane */
@@ -1260,6 +1278,29 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	dst->y1 = crtc_y;
 	dst->y2 = crtc_y + crtc_h;
 
+finish:
+	/*
+	 * If the sprite is completely covering the primary plane,
+	 * we can disable the primary and save power.
+	 */
+	state->hides_primary = fb != NULL && drm_rect_equals(dst, clip) &&
+		!colorkey_enabled(intel_plane);
+	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
+
+	if (intel_crtc->active) {
+		if (intel_crtc->primary_enabled == state->hides_primary)
+			intel_crtc->atomic.wait_for_flips = true;
+
+		if (intel_crtc->primary_enabled && state->hides_primary)
+			intel_crtc->atomic.pre_disable_primary = true;
+
+		intel_crtc->atomic.fb_bits |=
+			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
+
+		if (!intel_crtc->primary_enabled && !state->hides_primary)
+			intel_crtc->atomic.post_enable_primary = true;
+	}
+
 	return 0;
 }
 
@@ -1267,37 +1308,14 @@ static void
 intel_commit_sprite_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
-	struct drm_device *dev = plane->dev;
 	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
-	struct drm_rect *dst = &state->dst;
-	const struct drm_rect *clip = &state->clip;
-	bool primary_enabled;
-
-	/*
-	 * 'prepare' is never called when plane is being disabled, so we need
-	 * to handle frontbuffer tracking here
-	 */
-	if (!fb) {
-		mutex_lock(&dev->struct_mutex);
-		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
-				  INTEL_FRONTBUFFER_SPRITE(pipe));
-		mutex_unlock(&dev->struct_mutex);
-	}
-
-	/*
-	 * If the sprite is completely covering the primary plane,
-	 * we can disable the primary and save power.
-	 */
-	primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
-	WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
 
 	intel_plane->crtc_x = state->orig_dst.x1;
 	intel_plane->crtc_y = state->orig_dst.y1;
@@ -1310,15 +1328,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
-		bool primary_was_enabled = intel_crtc->primary_enabled;
-
-		intel_crtc->primary_enabled = primary_enabled;
-
-		if (primary_was_enabled != primary_enabled)
-			intel_crtc_wait_for_pending_flips(crtc);
-
-		if (primary_was_enabled && !primary_enabled)
-			intel_pre_disable_primary(crtc);
+		intel_crtc->primary_enabled = !state->hides_primary;
 
 		if (state->visible) {
 			crtc_x = state->dst.x1;
@@ -1335,12 +1345,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 		} else {
 			intel_plane->disable_plane(plane, crtc);
 		}
-
-
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_SPRITE(pipe));
-
-		if (!primary_was_enabled && primary_enabled)
-			intel_post_enable_primary(crtc);
 	}
 }