[2/2] drm/i915: Use vblank works for post vblank updates

Submitted by Ville Syrjala on Feb. 19, 2019, 7:50 p.m.

Details

Message ID 20190219195059.31916-2-ville.syrjala@linux.intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX - Try Bot

Not browsing as part of any series.

Commit Message

Ville Syrjala Feb. 19, 2019, 7:50 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Utilize drm_vblank_work to perform the post-vblank updates
(eg. single buffered gamma LUT and watermark programming).

I had to disable the state checker because it clobbers
the old_crtc_state. old_crtc_state->commit in particular
needs to be preserved for some of the cleanup stuff. I'm
not quite sure how this is working at all currently, nor
do I really understand why we need to move the commit
from the new crtc state to the old crtc state.

I'm also scheduling the vblank work a bit too late. We should
do it from within intel_finish_crtc_commit() but I was too
lazy for that right now. Currently we could potentially
miss the target vblank, at which point the current code would
schedule the update for the next vblank. So no tear but not
correct either.

At least with this I can get what appears to be tear free
gamma updates with two pipes being updated from a single
atomic commit. Or at least I can't see any obvious tearing
anymore. Previosuly one of them would tear for sure. I'd
need to pimp my tests to use CRCs to actually confirm
whether there is any tearing.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |   9 ++
 drivers/gpu/drm/i915/intel_display.c | 137 +++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |   8 ++
 drivers/gpu/drm/i915/intel_sprite.c  |   3 +
 4 files changed, 107 insertions(+), 50 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 7cf9290ea34a..4cb22864d0f0 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -156,6 +156,9 @@  intel_digital_connector_duplicate_state(struct drm_connector *connector)
 	return &state->base;
 }
 
+ void intel_crtc_vblank_work(struct drm_vblank_work *work,
+			     u64 count);
+
 /**
  * intel_crtc_duplicate_state - duplicate crtc state
  * @crtc: drm crtc
@@ -187,6 +190,12 @@  intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->fb_bits = 0;
 	crtc_state->update_planes = 0;
 
+	crtc_state->vbl_count = 0;
+	crtc_state->put_domains = 0;
+	crtc_state->state = NULL;
+	drm_vblank_work_init(&crtc_state->vblank_work,
+			     crtc, intel_crtc_vblank_work);
+
 	return &crtc_state->base;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b4a6eeb4573..60f9567988ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11659,6 +11659,7 @@  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
 	saved_state->pch_pfit.force_thru = crtc_state->pch_pfit.force_thru;
 	saved_state->ips_force_disable = crtc_state->ips_force_disable;
+	saved_state->vblank_work = crtc_state->vblank_work;
 	if (IS_G4X(dev_priv) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		saved_state->wm = crtc_state->wm;
@@ -13012,6 +13013,9 @@  u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
 	return dev->driver->get_vblank_counter(dev, crtc->pipe);
 }
 
+void intel_crtc_vblank_work(struct drm_vblank_work *work,
+			    u64 count);
+
 static void intel_update_crtc(struct drm_crtc *crtc,
 			      struct drm_atomic_state *state,
 			      struct drm_crtc_state *old_crtc_state,
@@ -13053,6 +13057,20 @@  static void intel_update_crtc(struct drm_crtc *crtc,
 		i9xx_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
 
 	intel_finish_crtc_commit(crtc, old_crtc_state);
+
+	atomic_inc(&to_intel_atomic_state(state)->cleanup_count);
+	/*
+	 * drm_atomic_helper_swap_state() has already made
+	 * new_crtc_state->base.state NULL hence we need our own pointer.
+	 */
+	pipe_config->state = to_intel_atomic_state(state);
+
+	/* FIXME should probably schedule from within the critical section */
+	if (!pipe_config->base.active ||
+	    WARN_ON(drm_vblank_work_schedule(&pipe_config->vblank_work,
+					     pipe_config->vbl_count,
+					     true) != 0))
+		intel_crtc_vblank_work(&pipe_config->vblank_work, 0);
 }
 
 static void intel_update_crtcs(struct drm_atomic_state *state)
@@ -13198,6 +13216,12 @@  static void intel_atomic_cleanup_work(struct work_struct *work)
 		container_of(work, struct drm_atomic_state, commit_work);
 	struct drm_i915_private *i915 = to_i915(state->dev);
 
+	/* should be nop */
+	drm_atomic_helper_wait_for_flip_done(&i915->drm, state);
+
+	if (intel_can_enable_sagv(state))
+		intel_enable_sagv(i915);
+
 	drm_atomic_helper_cleanup_planes(&i915->drm, state);
 	drm_atomic_helper_commit_cleanup_done(state);
 	drm_atomic_state_put(state);
@@ -13205,6 +13229,42 @@  static void intel_atomic_cleanup_work(struct work_struct *work)
 	intel_atomic_helper_free_state(i915);
 }
 
+void intel_crtc_vblank_work(struct drm_vblank_work *work,
+			    u64 count)
+{
+	struct intel_crtc_state *new_crtc_state =
+		container_of(work, struct intel_crtc_state, vblank_work);
+	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	struct intel_atomic_state *state = new_crtc_state->state;
+	struct intel_crtc_state *old_crtc_state =
+		intel_atomic_get_old_crtc_state(state, crtc);
+
+	if (new_crtc_state->base.active &&
+	    !needs_modeset(&new_crtc_state->base) &&
+	    (new_crtc_state->base.color_mgmt_changed ||
+	     new_crtc_state->update_pipe))
+		intel_color_load_luts(new_crtc_state);
+
+	/*
+	 * Now that the vblank has passed, we can go ahead and program the
+	 * optimal watermarks on platforms that need two-step watermark
+	 * programming.
+	 */
+	if (dev_priv->display.optimize_watermarks)
+		dev_priv->display.optimize_watermarks(state, new_crtc_state);
+
+	intel_post_plane_update(old_crtc_state);
+
+	if (new_crtc_state->put_domains)
+		modeset_put_power_domains(dev_priv, new_crtc_state->put_domains);
+
+	new_crtc_state->state = NULL;
+
+	if (atomic_dec_and_test(&state->cleanup_count))
+		queue_work(system_highpri_wq, &state->base.commit_work);
+}
+
 static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -13214,10 +13274,12 @@  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
 	struct drm_crtc *crtc;
 	struct intel_crtc *intel_crtc;
-	u64 put_domains[I915_MAX_PIPES] = {};
 	intel_wakeref_t wakeref = 0;
 	int i;
 
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i)
+		drm_vblank_work_flush(&to_intel_crtc_state(old_crtc_state)->vblank_work);
+
 	intel_atomic_commit_fence_wait(intel_state);
 
 	drm_atomic_helper_wait_for_dependencies(state);
@@ -13233,7 +13295,7 @@  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		if (needs_modeset(new_crtc_state) ||
 		    to_intel_crtc_state(new_crtc_state)->update_pipe) {
 
-			put_domains[intel_crtc->pipe] =
+			new_intel_crtc_state->put_domains =
 				modeset_get_crtc_power_domains(crtc,
 					new_intel_crtc_state);
 		}
@@ -13306,61 +13368,29 @@  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		}
 	}
 
+	INIT_WORK(&state->commit_work, intel_atomic_cleanup_work);
+	atomic_set(&intel_state->cleanup_count, 1);
+
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	dev_priv->display.update_crtcs(state);
 
-	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
-	 * already, but still need the state for the delayed optimization. To
-	 * fix this:
-	 * - wrap the optimization/post_plane_update stuff into a per-crtc work.
-	 * - schedule that vblank worker _before_ calling hw_done
-	 * - at the start of commit_tail, cancel it _synchrously
-	 * - switch over to the vblank wait helper in the core after that since
-	 *   we don't need out special handling any more.
-	 */
-	drm_atomic_helper_wait_for_flip_done(dev, state);
-
-	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
-		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
-
-		if (new_crtc_state->active &&
-		    !needs_modeset(new_crtc_state) &&
-		    (new_intel_crtc_state->base.color_mgmt_changed ||
-		     new_intel_crtc_state->update_pipe))
-			intel_color_load_luts(new_intel_crtc_state);
-	}
+	drm_atomic_helper_commit_hw_done(state);
 
 	/*
-	 * Now that the vblank has passed, we can go ahead and program the
-	 * optimal watermarks on platforms that need two-step watermark
-	 * programming.
+	 * FIXME intel_modeset_verify_crtc() clobbers
+	 * old_crtc_state->commit which causes explosions
+	 * int the parallel works.
 	 *
-	 * TODO: Move this (and other cleanup) to an async worker eventually.
+	 * FIXME how does this even work in the
+	 * pre-vblank work era?
 	 */
-	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
-		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
-
-		if (dev_priv->display.optimize_watermarks)
-			dev_priv->display.optimize_watermarks(intel_state,
-							      new_intel_crtc_state);
-	}
-
-	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
-		intel_post_plane_update(to_intel_crtc_state(old_crtc_state));
-
-		if (put_domains[i])
-			modeset_put_power_domains(dev_priv, put_domains[i]);
-
-		intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state);
-	}
+	if (0 && intel_state->modeset) {
+		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+			intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state);
+		}
 
-	if (intel_state->modeset)
 		intel_verify_planes(intel_state);
-
-	if (intel_state->modeset && intel_can_enable_sagv(state))
-		intel_enable_sagv(dev_priv);
-
-	drm_atomic_helper_commit_hw_done(state);
+	}
 
 	if (intel_state->modeset) {
 		/* As one of the primary mmio accessors, KMS has a high
@@ -13381,8 +13411,8 @@  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	 * schedule point (cond_resched()) here anyway to keep latencies
 	 * down.
 	 */
-	INIT_WORK(&state->commit_work, intel_atomic_cleanup_work);
-	queue_work(system_highpri_wq, &state->commit_work);
+	if (atomic_dec_and_test(&intel_state->cleanup_count))
+		queue_work(system_highpri_wq, &state->commit_work);
 }
 
 static void intel_atomic_commit_work(struct work_struct *work)
@@ -13526,6 +13556,7 @@  static int intel_atomic_commit(struct drm_device *dev,
 		if (intel_state->modeset)
 			flush_workqueue(dev_priv->modeset_wq);
 		intel_atomic_commit_tail(state);
+		drm_atomic_helper_wait_for_flip_done(dev, state);
 	}
 
 	return 0;
@@ -14418,6 +14449,9 @@  static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 
 	intel_color_init(intel_crtc);
 
+	drm_vblank_work_init(&crtc_state->vblank_work,
+			     &intel_crtc->base, intel_crtc_vblank_work);
+
 	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
 
 	return 0;
@@ -15895,9 +15929,12 @@  static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	for_each_intel_crtc(dev, crtc) {
 		struct intel_crtc_state *crtc_state =
 			to_intel_crtc_state(crtc->base.state);
+		struct drm_vblank_work save_vblank_work =
+			crtc_state->vblank_work;
 
 		__drm_atomic_helper_crtc_destroy_state(&crtc_state->base);
 		memset(crtc_state, 0, sizeof(*crtc_state));
+		crtc_state->vblank_work = save_vblank_work;
 		crtc_state->base.crtc = &crtc->base;
 
 		crtc_state->base.active = crtc_state->base.enable =
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index eec4ed93c335..9b365966da5f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -514,6 +514,8 @@  struct intel_atomic_state {
 	struct i915_sw_fence commit_ready;
 
 	struct llist_node freed;
+
+	atomic_t cleanup_count;
 };
 
 struct intel_plane_state {
@@ -982,6 +984,12 @@  struct intel_crtc_state {
 
 	/* Forward Error correction State */
 	bool fec_enable;
+
+	struct drm_vblank_work vblank_work;
+	struct intel_atomic_state *state;
+	u64 vbl_count;
+
+	u64 put_domains;
 };
 
 struct intel_crtc {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 610398607e8e..f1f1f669ebd9 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -206,6 +206,9 @@  void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
 		new_crtc_state->base.event = NULL;
 	}
 
+	new_crtc_state->vbl_count =
+		drm_crtc_accurate_vblank_count(&crtc->base) + 1;
+
 	local_irq_enable();
 
 	if (intel_vgpu_active(dev_priv))