Re: [PATCH 14/16] drm/i915/gen9: Calculate watermarks during atomic 'check'

From: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
To: Matt Roper <matthew.d.roper@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 14/16] drm/i915/gen9: Calculate watermarks during atomic 'check'
Date: Tue, 5 Apr 2016 15:01:23 +0200	[thread overview]
Message-ID: <20160405130123.GA18027@patrik-desktop.isw.intel.com> (raw)
In-Reply-To: <1459475198-30094-15-git-send-email-matthew.d.roper@intel.com>

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@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@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 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx