Re: [PATCH v6 1/2] drm/i915: Refactor intel_can_enable_sagv

[James Ausmus <james.ausmus@intel.com>]

On Wed, Oct 23, 2019 at 12:08:03PM +0300, Stanislav Lisovskiy wrote:
> Currently intel_can_enable_sagv function contains
> a mix of workarounds for different platforms
> some of them are not valid for gens >= 11 already,
> so lets split it into separate functions.
> 
> v2:
>     - Rework watermark calculation algorithm to
>       attempt to calculate Level 0 watermark
>       with added sagv block time latency and
>       check if it fits in DBuf in order to
>       determine if SAGV can be enabled already
>       at this stage, just as BSpec 49325 states.
>       if that fails rollback to usual Level 0
>       latency and disable SAGV.
>     - Remove unneeded tabs(James Ausmus)
> 
> v3: Rebased the patch
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@intel.com>
> Cc: James Ausmus <james.ausmus@intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |   8 +
>  drivers/gpu/drm/i915/intel_pm.c               | 228 +++++++++++++++++-
>  2 files changed, 228 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 8358152e403e..f09c80c96470 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -490,6 +490,13 @@ struct intel_atomic_state {
>  	 */
>  	u8 active_pipe_changes;
>  
> +	/*
> +	 * For Gen12 only after calculating watermarks with
> +	 * additional latency, we can determine if SAGV can be enabled
> +	 * or not for that particular configuration.
> +	 */
> +	bool gen12_can_sagv;
> +
>  	u8 active_pipes;
>  	/* minimum acceptable cdclk for each pipe */
>  	int min_cdclk[I915_MAX_PIPES];
> @@ -642,6 +649,7 @@ struct skl_plane_wm {
>  	struct skl_wm_level wm[8];
>  	struct skl_wm_level uv_wm[8];
>  	struct skl_wm_level trans_wm;
> +	struct skl_wm_level sagv_wm_l0;
>  	bool is_planar;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 362234449087..c0419e4d83de 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3751,7 +3751,7 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>  
> -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> +bool skl_can_enable_sagv(struct intel_atomic_state *state)
>  {
>  	struct drm_device *dev = state->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -3817,6 +3817,75 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state)
>  	return true;
>  }
>  
> +bool icl_can_enable_sagv(struct intel_atomic_state *state)
> +{
> +	struct drm_device *dev = state->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_crtc *crtc;
> +	struct intel_crtc_state *new_crtc_state;
> +	int level, latency;
> +	int i;
> +	int plane_id;
> +
> +	if (!intel_has_sagv(dev_priv))
> +		return false;
> +
> +	/*
> +	 * If there are no active CRTCs, no additional checks need be performed
> +	 */
> +	if (hweight8(state->active_pipes) == 0)
> +		return true;
> +
> +	for_each_new_intel_crtc_in_state(state, crtc,
> +					     new_crtc_state, i) {
> +
> +		if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> +			return false;
> +
> +		if (!new_crtc_state->base.enable)
> +			continue;
> +
> +		for_each_plane_id_on_crtc(crtc, plane_id) {
> +			struct skl_plane_wm *wm =
> +				&new_crtc_state->wm.skl.optimal.planes[plane_id];
> +
> +			/* Skip this plane if it's not enabled */
> +			if (!wm->wm[0].plane_en)
> +				continue;
> +
> +			/* Find the highest enabled wm level for this plane */
> +			for (level = ilk_wm_max_level(dev_priv);
> +			     !wm->wm[level].plane_en; --level)
> +			     { }
> +
> +			latency = dev_priv->wm.skl_latency[level];
> +
> +			/*
> +			 * If any of the planes on this pipe don't enable wm levels that
> +			 * incur memory latencies higher than sagv_block_time_us we
> +			 * can't enable SAGV.
> +			 */
> +			if (latency < dev_priv->sagv_block_time_us)
> +				return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +bool intel_can_enable_sagv(struct intel_atomic_state *state)
> +{
> +	struct drm_device *dev = state->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	if (INTEL_GEN(dev_priv) >= 12)
> +		return state->gen12_can_sagv;

This loses the interlaced mode check that the skl and icl functions
have, which is still needed for TGL


> +	else if (INTEL_GEN(dev_priv) == 11)
> +		return icl_can_enable_sagv(state);
> +
> +	return skl_can_enable_sagv(state);
> +}
> +
>  static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
>  			      const struct intel_crtc_state *crtc_state,
>  			      const u64 total_data_rate,
> @@ -3936,6 +4005,7 @@ static int skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
>  				 int color_plane);
>  static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
>  				 int level,
> +				 u32 latency,
>  				 const struct skl_wm_params *wp,
>  				 const struct skl_wm_level *result_prev,
>  				 struct skl_wm_level *result /* out */);
> @@ -3958,7 +4028,8 @@ skl_cursor_allocation(const struct intel_crtc_state *crtc_state,
>  	WARN_ON(ret);
>  
>  	for (level = 0; level <= max_level; level++) {
> -		skl_compute_plane_wm(crtc_state, level, &wp, &wm, &wm);
> +		u32 latency = dev_priv->wm.skl_latency[level];
> +		skl_compute_plane_wm(crtc_state, level, latency, &wp, &wm, &wm);
>  		if (wm.min_ddb_alloc == U16_MAX)
>  			break;
>  
> @@ -4310,6 +4381,73 @@ icl_get_total_relative_data_rate(struct intel_crtc_state *crtc_state,
>  	return total_data_rate;
>  }
>  
> +static int
> +tgl_check_pipe_fits_sagv_wm(struct intel_crtc_state *crtc_state,
> +		      struct skl_ddb_allocation *ddb /* out */)
> +{
> +	struct drm_crtc *crtc = crtc_state->base.crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct skl_ddb_entry *alloc = &crtc_state->wm.skl.ddb;
> +	u16 alloc_size;
> +	u16 total[I915_MAX_PLANES] = {};
> +	u64 total_data_rate;
> +	enum plane_id plane_id;
> +	int num_active;
> +	u64 plane_data_rate[I915_MAX_PLANES] = {};
> +	u64 uv_plane_data_rate[I915_MAX_PLANES] = {};
> +	u32 blocks;
> +
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		total_data_rate =
> +			icl_get_total_relative_data_rate(crtc_state,
> +							 plane_data_rate);

This function is already only called on gen12+ - could drop the if
check, and the entire else block.


> +	else
> +		total_data_rate =
> +			skl_get_total_relative_data_rate(crtc_state,
> +							 plane_data_rate,
> +							 uv_plane_data_rate);
> +
> +
> +	skl_ddb_get_pipe_allocation_limits(dev_priv, crtc_state, total_data_rate,
> +					   ddb, alloc, &num_active);
> +	alloc_size = skl_ddb_entry_size(alloc);
> +	if (alloc_size == 0)
> +		return -ENOSPC;
> +
> +	/* Allocate fixed number of blocks for cursor. */
> +	total[PLANE_CURSOR] = skl_cursor_allocation(crtc_state, num_active);
> +	alloc_size -= total[PLANE_CURSOR];
> +	crtc_state->wm.skl.plane_ddb_y[PLANE_CURSOR].start =
> +		alloc->end - total[PLANE_CURSOR];
> +	crtc_state->wm.skl.plane_ddb_y[PLANE_CURSOR].end = alloc->end;
> +
> +	/*
> +	 * Do check if we can fit L0 + sagv_block_time and
> +	 * disable SAGV if we can't.
> +	 */
> +	blocks = 0;
> +	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> +		const struct skl_plane_wm *wm =
> +			&crtc_state->wm.skl.optimal.planes[plane_id];
> +
> +		if (plane_id == PLANE_CURSOR) {
> +			if (WARN_ON(wm->sagv_wm_l0.min_ddb_alloc >
> +				    total[PLANE_CURSOR])) {
> +				blocks = U32_MAX;
> +				break;
> +			}
> +			continue;
> +		}
> +
> +		blocks += wm->sagv_wm_l0.min_ddb_alloc;
> +		if (blocks > alloc_size) {
> +			return -ENOSPC;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int
>  skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state,
>  		      struct skl_ddb_allocation *ddb /* out */)
> @@ -4739,12 +4877,12 @@ static bool skl_wm_has_lines(struct drm_i915_private *dev_priv, int level)
>  
>  static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
>  				 int level,
> +				 u32 latency,
>  				 const struct skl_wm_params *wp,
>  				 const struct skl_wm_level *result_prev,
>  				 struct skl_wm_level *result /* out */)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> -	u32 latency = dev_priv->wm.skl_latency[level];
>  	uint_fixed_16_16_t method1, method2;
>  	uint_fixed_16_16_t selected_result;
>  	u32 res_blocks, res_lines, min_ddb_alloc = 0;
> @@ -4865,19 +5003,45 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
>  static void
>  skl_compute_wm_levels(const struct intel_crtc_state *crtc_state,
>  		      const struct skl_wm_params *wm_params,
> -		      struct skl_wm_level *levels)
> +		      struct skl_plane_wm *plane_wm,
> +		      bool yuv)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>  	int level, max_level = ilk_wm_max_level(dev_priv);
> +	/*
> +	 * Check which kind of plane is it and based on that calculate
> +	 * correspondent WM levels.
> +	 */
> +	struct skl_wm_level *levels = yuv ? plane_wm->uv_wm : plane_wm->wm;
>  	struct skl_wm_level *result_prev = &levels[0];
>  
>  	for (level = 0; level <= max_level; level++) {
>  		struct skl_wm_level *result = &levels[level];
> +		u32 latency = dev_priv->wm.skl_latency[level];
>  
> -		skl_compute_plane_wm(crtc_state, level, wm_params,
> -				     result_prev, result);
> +		skl_compute_plane_wm(crtc_state, level, latency,
> +				     wm_params, result_prev, result);
>  
>  		result_prev = result;
> +		if (level == 0) {
> +			/*
> +			 * For Gen12 if it is an L0 we need to also
> +			 * consider sagv_block_time when calculating
> +			 * L0 watermark - we will need that when making
> +			 * a decision whether enable SAGV or not.
> +			 * For older gens we agreed to copy L0 value for
> +			 * compatibility.
> +			 */
> +			if ((INTEL_GEN(dev_priv) >= 12)) {
> +				latency += dev_priv->sagv_block_time_us;
> +				skl_compute_plane_wm(crtc_state, level, latency,
> +				     wm_params, result_prev,
> +				    &plane_wm->sagv_wm_l0);
> +			}
> +			else 
> +				memcpy(&plane_wm->sagv_wm_l0, &levels[0],
> +					sizeof(struct skl_wm_level));
> +		}
>  	}
>  }
>  
> @@ -4971,7 +5135,7 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
>  	if (ret)
>  		return ret;
>  
> -	skl_compute_wm_levels(crtc_state, &wm_params, wm->wm);
> +	skl_compute_wm_levels(crtc_state, &wm_params, wm, false);
>  	skl_compute_transition_wm(crtc_state, &wm_params, wm);
>  
>  	return 0;
> @@ -4993,7 +5157,7 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
>  	if (ret)
>  		return ret;
>  
> -	skl_compute_wm_levels(crtc_state, &wm_params, wm->uv_wm);
> +	skl_compute_wm_levels(crtc_state, &wm_params, wm, true);
>  
>  	return 0;
>  }
> @@ -5544,10 +5708,13 @@ static int skl_wm_add_affected_planes(struct intel_atomic_state *state,
>  static int
>  skl_compute_wm(struct intel_atomic_state *state)
>  {
> +	struct drm_device *dev = state->base.dev;
> +	const struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *crtc;
>  	struct intel_crtc_state *new_crtc_state;
>  	struct intel_crtc_state *old_crtc_state;
>  	struct skl_ddb_values *results = &state->wm_results;
> +	struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
>  	int ret, i;
>  
>  	/* Clear all dirty flags */
> @@ -5557,6 +5724,8 @@ skl_compute_wm(struct intel_atomic_state *state)
>  	if (ret)
>  		return ret;
>  
> +	state->gen12_can_sagv = false;
> +
>  	/*
>  	 * Calculate WM's for all pipes that are part of this transaction.
>  	 * Note that skl_ddb_add_affected_pipes may have added more CRTC's that
> @@ -5579,6 +5748,49 @@ skl_compute_wm(struct intel_atomic_state *state)
>  			results->dirty_pipes |= BIT(crtc->pipe);
>  	}
>  
> +	if (INTEL_GEN(dev_priv) < 12)
> +		goto compute_ddb;

I understand why you are goto'ing to avoid the extra indent below - can
the block between here and compute_ddb just be extracted to a separate
function instead?

-James

> +
> +	/*
> +	 * Lets assume we can tolerate SAGV for now,
> +	 * until watermark calculations prove the opposite
> +	 * if any of the pipe planes in the state will
> +	 * fail the requirements it will be assigned to false
> +	 * in skl_compute_ddb.
> +	 */
> +	state->gen12_can_sagv = true;
> +
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		ret = tgl_check_pipe_fits_sagv_wm(new_crtc_state, ddb);
> +		if (ret) {
> +			state->gen12_can_sagv = false;
> +			break;
> +		}
> +	}
> +
> +	if (state->gen12_can_sagv) {
> +		/*
> +		 * If we determined that we can actually enable SAGV, then
> +		 * actually use those levels tgl_check_pipe_fits_sagv_wm
> +		 * has already taken care of checking if L0 + sagv block time
> +		 * fits into ddb.
> +		 */
> +		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +			struct intel_plane *plane;
> +			for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> +				enum plane_id plane_id = plane->id;
> +				struct skl_plane_wm *plane_wm = \
> +				    &new_crtc_state->wm.skl.optimal.planes[plane_id];
> +				struct skl_wm_level *sagv_wm0 = &plane_wm->sagv_wm_l0;
> +				struct skl_wm_level *l0_wm0 = &plane_wm->wm[0];
> +				memcpy(l0_wm0, sagv_wm0, sizeof(struct skl_wm_level));
> +			}
> +		}
> +	}
> +
> +compute_ddb:
>  	ret = skl_compute_ddb(state);
>  	if (ret)
>  		return ret;
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx