On Fri, 22 Apr 2016, Matt Roper <matthew.d.ro...@intel.com> 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). > > 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=89055
From the bug, Tested-by: Cezar Burlacu <cezar.burl...@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92181 > Signed-off-by: Matt Roper <matthew.d.ro...@intel.com> > Reviewed-by: Maarten Lankhorst <maarten.lankho...@linux.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 | 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 62609ab..fe5adcd 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3221,23 +3221,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, > @@ -3716,66 +3699,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) > { > @@ -3783,6 +3709,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; > > @@ -3799,8 +3726,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; > @@ -3809,7 +3738,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; > } > @@ -3822,8 +3751,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 > @@ -3838,10 +3770,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; > } > > @@ -3853,26 +3820,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, -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
