[Intel-gfx] [PATCH 14/16] drm/i915/gen9: Calculate watermarks during atomic 'check'
Patrik Jakobsson
patrik.jakobsson at linux.intel.com
Tue Apr 5 13:01:23 UTC 2016
On Thu, Mar 31, 2016 at 06:46:36PM -0700, Matt Roper wrote:
> 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).
This one also fixes: https://bugs.freedesktop.org/show_bug.cgi?id=92181
>
> 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.
>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 2 +-
> drivers/gpu/drm/i915/intel_pm.c | 141 +++++++++++++----------------------
> 3 files changed, 55 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c514549..f1bea9f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13523,7 +13523,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 6471f69..3abd394 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -303,7 +303,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 1bef89a..e4de5aa 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3187,23 +3187,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,
> @@ -3654,72 +3637,16 @@ 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)
> {
> struct drm_device *dev = state->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 = intel_state->active_crtcs;
> int ret;
>
> @@ -3736,8 +3663,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_crtcs != to_i915(dev)->active_crtcs)
> + if (intel_state->active_crtcs != to_i915(dev)->active_crtcs) {
> 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;
> @@ -3746,7 +3675,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;
> }
> @@ -3757,12 +3686,53 @@ skl_compute_ddb(struct drm_atomic_state *state)
> static int
> skl_compute_wm(struct drm_atomic_state *state)
> {
> - int ret;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *cstate;
> + 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;
> + int ret, i;
> +
> + /* 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;
> }
>
> @@ -3774,26 +3744,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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
---
Intel Sweden AB Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027
More information about the Intel-gfx
mailing list