[PATCH 2/2] drm/i915: Use vblank works for post vblank updates
Ville Syrjala
ville.syrjala at linux.intel.com
Wed Feb 20 20:27:27 UTC 2019
From: Ville Syrjälä <ville.syrjala at 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 at linux.intel.com>
---
drivers/gpu/drm/i915/intel_atomic.c | 9 ++
drivers/gpu/drm/i915/intel_display.c | 159 ++++++++++++++++++---------
drivers/gpu/drm/i915/intel_display.h | 8 ++
drivers/gpu/drm/i915/intel_drv.h | 8 ++
drivers/gpu/drm/i915/intel_sprite.c | 3 +
5 files changed, 137 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index da419e157332..ab1c95e24334 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -157,6 +157,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
@@ -188,6 +191,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 415d8968f2c5..2264b863b9b9 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;
@@ -13026,6 +13027,27 @@ 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_crtc_schedule_vblank_work(struct intel_atomic_state *state,
+ struct intel_crtc_state *new_crtc_state)
+{
+ atomic_inc(&state->cleanup_count);
+
+ /*
+ * drm_atomic_helper_swap_state() has already made
+ * new_crtc_state->base.state NULL hence we need our own pointer.
+ */
+ new_crtc_state->state = state;
+
+ if (!new_crtc_state->base.active ||
+ WARN_ON(drm_vblank_work_schedule(&new_crtc_state->vblank_work,
+ new_crtc_state->vbl_count,
+ true) != 0))
+ intel_crtc_vblank_work(&new_crtc_state->vblank_work, 0);
+}
+
static void intel_update_crtc(struct drm_crtc *crtc,
struct drm_atomic_state *state,
struct drm_crtc_state *old_crtc_state,
@@ -13067,6 +13089,10 @@ 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);
+
+ /* FIXME should probably schedule from within the critical section */
+ intel_crtc_schedule_vblank_work(to_intel_atomic_state(state),
+ pipe_config);
}
static void intel_update_crtcs(struct drm_atomic_state *state)
@@ -13212,6 +13238,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);
@@ -13219,6 +13251,52 @@ 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_flush_vblank_works(struct intel_atomic_state *state)
+{
+ struct intel_crtc_state *old_crtc_state;
+ struct intel_crtc *crtc;
+ int i;
+
+ for_each_old_intel_crtc_in_state(state, crtc, old_crtc_state, i)
+ drm_vblank_work_flush(&old_crtc_state->vblank_work);
+}
+
static void intel_atomic_commit_tail(struct drm_atomic_state *state)
{
struct drm_device *dev = state->dev;
@@ -13228,10 +13306,11 @@ 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;
+ intel_atomic_flush_vblank_works(intel_state);
+
intel_atomic_commit_fence_wait(intel_state);
drm_atomic_helper_wait_for_dependencies(state);
@@ -13247,7 +13326,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,6 +13385,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
intel_modeset_verify_disabled(dev, state);
}
+ INIT_WORK(&state->commit_work, intel_atomic_cleanup_work);
+ atomic_set(&intel_state->cleanup_count, 1);
+
/* Complete the events for pipes that have now been disabled */
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
bool modeset = needs_modeset(new_crtc_state);
@@ -13318,63 +13400,33 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
new_crtc_state->event = NULL;
}
+
+ /* FIXME should we do this before or after update_crtcs()? */
+ if (!new_crtc_state->active)
+ intel_crtc_schedule_vblank_work(intel_state,
+ to_intel_crtc_state(new_crtc_state));
}
/* 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
@@ -13395,8 +13447,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)
@@ -13540,6 +13592,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;
@@ -14432,6 +14485,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;
@@ -15909,9 +15965,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_display.h b/drivers/gpu/drm/i915/intel_display.h
index 2220588e86ac..2e034bbbbd01 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -401,6 +401,14 @@ struct intel_link_m_n {
(__i)++) \
for_each_if(plane)
+#define for_each_old_intel_crtc_in_state(__state, crtc, old_crtc_state, __i) \
+ for ((__i) = 0; \
+ (__i) < (__state)->base.dev->mode_config.num_crtc && \
+ ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
+ (old_crtc_state) = to_intel_crtc_state((__state)->base.crtcs[__i].old_state), 1); \
+ (__i)++) \
+ for_each_if(crtc)
+
#define for_each_new_intel_crtc_in_state(__state, crtc, new_crtc_state, __i) \
for ((__i) = 0; \
(__i) < (__state)->base.dev->mode_config.num_crtc && \
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7a883c3e41d8..dac1822b805d 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))
--
2.19.2
More information about the Intel-gfx-trybot
mailing list