Moving watermark calculation into the check phase will allow us to to reject display configurations for which there are no valid watermark values before we start trying to program the hardware (although those tests will come in a subsequent patch). Another advantage of moving this calculation to the check phase is that we can calculate the watermarks in a single shot as part of the atomic transaction. The watermark interfaces we inherited from our legacy modesetting days are a bit broken in the atomic design because they use per-crtc entry points but actually re-calculate and re-program something that is really more of a global state. That worked okay in the legacy modesetting world because operations only ever updated a single CRTC at a time. However in the atomic world, a transaction can involve multiple CRTC's, which means we wind up computing and programming the watermarks NxN times (where N is the number of CRTC's involved). With this patch we eliminate the redundant re-calculation of watermark data for atomic states (which was the cause of the WARN_ON(!wm_changed) problems that have plagued us for a while). We still need to work on the 'commit' side of watermark handling so that we aren't doing redundant NxN programming of watermarks, but that's content for future patches. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92181 Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> --- drivers/gpu/drm/i915/intel_display.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_pm.c | 138 +++++++++++++---------------------- 3 files changed, 52 insertions(+), 90 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7e8fa88..8bedf12 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13599,7 +13599,7 @@ static int intel_atomic_commit(struct drm_device *dev, drm_atomic_helper_swap_state(dev, state); dev_priv->wm.config = intel_state->wm_config; - dev_priv->wm.skl_results.ddb = intel_state->ddb; + dev_priv->wm.skl_results = intel_state->wm_results; intel_shared_dpll_commit(state); if (intel_state->modeset) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 2282494..6316215 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -313,7 +313,7 @@ struct intel_atomic_state { bool skip_intermediate_wm; /* Gen9+ only */ - struct skl_ddb_allocation ddb; + struct skl_wm_values wm_results; }; struct intel_plane_state { diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index f07054a..59d4574 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3224,23 +3224,6 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal, return ret; } -static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb, - const struct intel_crtc *intel_crtc) -{ - struct drm_device *dev = intel_crtc->base.dev; - struct drm_i915_private *dev_priv = dev->dev_private; - const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb; - - /* - * If ddb allocation of pipes changed, it may require recalculation of - * watermarks - */ - if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe))) - return true; - - return false; -} - static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, struct intel_crtc_state *cstate, struct intel_plane_state *intel_pstate, @@ -3711,66 +3694,9 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate, else *changed = true; - intel_crtc->wm.active.skl = *pipe_wm; - return 0; } -static void skl_update_other_pipe_wm(struct drm_device *dev, - struct drm_crtc *crtc, - struct skl_wm_values *r) -{ - struct intel_crtc *intel_crtc; - struct intel_crtc *this_crtc = to_intel_crtc(crtc); - - /* - * If the WM update hasn't changed the allocation for this_crtc (the - * crtc we are currently computing the new WM values for), other - * enabled crtcs will keep the same allocation and we don't need to - * recompute anything for them. - */ - if (!skl_ddb_allocation_changed(&r->ddb, this_crtc)) - return; - - /* - * Otherwise, because of this_crtc being freshly enabled/disabled, the - * other active pipes need new DDB allocation and WM values. - */ - for_each_intel_crtc(dev, intel_crtc) { - struct skl_pipe_wm pipe_wm = {}; - bool wm_changed; - - if (this_crtc->pipe == intel_crtc->pipe) - continue; - - if (!intel_crtc->active) - continue; - - skl_update_pipe_wm(intel_crtc->base.state, - &r->ddb, &pipe_wm, &wm_changed); - - /* - * If we end up re-computing the other pipe WM values, it's - * because it was really needed, so we expect the WM values to - * be different. - */ - WARN_ON(!wm_changed); - - skl_compute_wm_results(dev, &pipe_wm, r, intel_crtc); - r->dirty_pipes |= drm_crtc_mask(&intel_crtc->base); - } -} - -static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe) -{ - watermarks->wm_linetime[pipe] = 0; - memset(watermarks->plane[pipe], 0, - sizeof(uint32_t) * 8 * I915_MAX_PLANES); - memset(watermarks->plane_trans[pipe], - 0, sizeof(uint32_t) * I915_MAX_PLANES); - watermarks->plane_trans[pipe][PLANE_CURSOR] = 0; -} - static int skl_compute_ddb(struct drm_atomic_state *state) { @@ -3778,6 +3704,7 @@ skl_compute_ddb(struct drm_atomic_state *state) struct drm_i915_private *dev_priv = to_i915(dev); struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct intel_crtc *intel_crtc; + struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb; unsigned realloc_pipes = dev_priv->active_crtcs; int ret; @@ -3794,8 +3721,10 @@ skl_compute_ddb(struct drm_atomic_state *state) * any other display updates race with this transaction, so we need * to grab the lock on *all* CRTC's. */ - if (intel_state->active_pipe_changes) + if (intel_state->active_pipe_changes) { realloc_pipes = ~0; + intel_state->wm_results.dirty_pipes = ~0; + } for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) { struct intel_crtc_state *cstate; @@ -3804,7 +3733,7 @@ skl_compute_ddb(struct drm_atomic_state *state) if (IS_ERR(cstate)) return PTR_ERR(cstate); - ret = skl_allocate_pipe_ddb(cstate, &intel_state->ddb); + ret = skl_allocate_pipe_ddb(cstate, ddb); if (ret) return ret; } @@ -3817,8 +3746,11 @@ skl_compute_wm(struct drm_atomic_state *state) { struct drm_crtc *crtc; struct drm_crtc_state *cstate; - int ret, i; + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); + struct skl_wm_values *results = &intel_state->wm_results; + struct skl_pipe_wm *pipe_wm; bool changed = false; + int ret, i; /* * If this transaction isn't actually touching any CRTC's, don't @@ -3833,10 +3765,45 @@ skl_compute_wm(struct drm_atomic_state *state) if (!changed) return 0; + /* Clear all dirty flags */ + results->dirty_pipes = 0; + ret = skl_compute_ddb(state); if (ret) return ret; + /* + * Calculate WM's for all pipes that are part of this transaction. + * Note that the DDB allocation above may have added more CRTC's that + * weren't otherwise being modified (and set bits in dirty_pipes) if + * pipe allocations had to change. + * + * FIXME: Now that we're doing this in the atomic check phase, we + * should allow skl_update_pipe_wm() to return failure in cases where + * no suitable watermark values can be found. + */ + for_each_crtc_in_state(state, crtc, cstate, i) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_crtc_state *intel_cstate = + to_intel_crtc_state(cstate); + + pipe_wm = &intel_cstate->wm.skl.optimal; + ret = skl_update_pipe_wm(cstate, &results->ddb, pipe_wm, + &changed); + if (ret) + return ret; + + if (changed) + results->dirty_pipes |= drm_crtc_mask(crtc); + + if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) + /* This pipe's WM's did not change */ + continue; + + intel_cstate->update_wm_pre = true; + skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc); + } + return 0; } @@ -3848,26 +3815,21 @@ static void skl_update_wm(struct drm_crtc *crtc) struct skl_wm_values *results = &dev_priv->wm.skl_results; struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state); struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal; - bool wm_changed; - /* Clear all dirty flags */ - results->dirty_pipes = 0; - - skl_clear_wm(results, intel_crtc->pipe); - - skl_update_pipe_wm(crtc->state, &results->ddb, pipe_wm, &wm_changed); - if (!wm_changed) + if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) return; - skl_compute_wm_results(dev, pipe_wm, results, intel_crtc); - results->dirty_pipes |= drm_crtc_mask(&intel_crtc->base); + intel_crtc->wm.active.skl = *pipe_wm; + + mutex_lock(&dev_priv->wm.wm_mutex); - skl_update_other_pipe_wm(dev, crtc, results); skl_write_wm_values(dev_priv, results); skl_flush_wm_values(dev_priv, results); /* store the new configuration */ dev_priv->wm.skl_hw = *results; + + mutex_unlock(&dev_priv->wm.wm_mutex); } static void ilk_compute_wm_config(struct drm_device *dev, -- 2.1.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
