[Intel-gfx] [PATCH v2 13/21] drm/i915: Extract wm latency adjustment to its own function

[Matt Roper]

On Thu, Aug 18, 2022 at 04:41:54PM -0700, Radhakrishna Sripada wrote:
> Watermark latency is adjusted in cases when latency is 0us for level
> greater than 1, the subsequent levels are disabled. Extract this logic
> into its own function.
> 
> Suggested-by: Matt Roper <matthew.d.roper at intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 88 ++++++++++++++++++---------------
>  1 file changed, 48 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ef7553b494ea..898e56d2eaf7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2861,15 +2861,59 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
>  	result->enable = true;
>  }
>  
> +static void
> +adjust_wm_latency(u16 wm[], int max_level, int read_latency,
> +		  bool wm_lv_0_adjust_needed)
> +{
> +	int i, level;
> +
> +	/*
> +	 * If a level n (n > 1) has a 0us latency, all levels m (m >= n)
> +	 * need to be disabled. We make sure to sanitize the values out
> +	 * of the punit to satisfy this requirement.
> +	 */
> +	for (level = 1; level <= max_level; level++) {
> +		if (wm[level] == 0) {
> +			for (i = level + 1; i <= max_level; i++)
> +				wm[i] = 0;
> +
> +			max_level = level - 1;
> +			break;
> +		}
> +	}
> +
> +	/*
> +	 * WaWmMemoryReadLatency
> +	 *
> +	 * punit doesn't take into account the read latency so we need
> +	 * to add proper adjustement to each valid level we retrieve
> +	 * from the punit when level 0 response data is 0us.
> +	 */
> +	if (wm[0] == 0) {
> +		for (level = 0; level <= max_level; level++)
> +			wm[level] += read_latency;
> +	}
> +
> +	/*
> +	 * WA Level-0 adjustment for 16GB DIMMs: SKL+
> +	 * If we could not get dimm info enable this WA to prevent from
> +	 * any underrun. If not able to get Dimm info assume 16GB dimm
> +	 * to avoid any underrun.
> +	 */
> +	if (wm_lv_0_adjust_needed)
> +		wm[0] += 1;
> +}
> +
>  static void intel_read_wm_latency(struct drm_i915_private *dev_priv,
>  				  u16 wm[])
>  {
>  	struct intel_uncore *uncore = &dev_priv->uncore;
>  
>  	if (DISPLAY_VER(dev_priv) >= 9) {
> +		int read_latency = DISPLAY_VER(dev_priv) >= 12 ? 3 : 2;
>  		u32 val;
> -		int ret, i;
> -		int level, max_level = ilk_wm_max_level(dev_priv);
> +		int ret;
> +		int max_level = ilk_wm_max_level(dev_priv);
>  		int mult = IS_DG2(dev_priv) ? 2 : 1;
>  
>  		/* read the first set of memory latencies[0:3] */
> @@ -2909,44 +2953,8 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv,
>  		wm[7] = ((val >> GEN9_MEM_LATENCY_LEVEL_3_7_SHIFT) &
>  				GEN9_MEM_LATENCY_LEVEL_MASK) * mult;
>  
> -		/*
> -		 * If a level n (n > 1) has a 0us latency, all levels m (m >= n)
> -		 * need to be disabled. We make sure to sanitize the values out
> -		 * of the punit to satisfy this requirement.
> -		 */
> -		for (level = 1; level <= max_level; level++) {
> -			if (wm[level] == 0) {
> -				for (i = level + 1; i <= max_level; i++)
> -					wm[i] = 0;
> -
> -				max_level = level - 1;
> -
> -				break;
> -			}
> -		}
> -
> -		/*
> -		 * WaWmMemoryReadLatency
> -		 *
> -		 * punit doesn't take into account the read latency so we need
> -		 * to add proper adjustement to each valid level we retrieve
> -		 * from the punit when level 0 response data is 0us.
> -		 */
> -		if (wm[0] == 0) {
> -			u8 adjust = DISPLAY_VER(dev_priv) >= 12 ? 3 : 2;
> -
> -			for (level = 0; level <= max_level; level++)
> -				wm[level] += adjust;
> -		}
> -
> -		/*
> -		 * WA Level-0 adjustment for 16GB DIMMs: SKL+
> -		 * If we could not get dimm info enable this WA to prevent from
> -		 * any underrun. If not able to get Dimm info assume 16GB dimm
> -		 * to avoid any underrun.
> -		 */
> -		if (dev_priv->dram_info.wm_lv_0_adjust_needed)
> -			wm[0] += 1;
> +		adjust_wm_latency(wm, max_level, read_latency,
> +				  dev_priv->dram_info.wm_lv_0_adjust_needed);

Would it be cleaner to just pass the dev_priv as a parameter to the
function and have it extract the structure field?  As far as I can tell,
we're never going to pass anything except exactly this field to this
function.

Aside from that,

Reviewed-by: Matt Roper <matthew.d.roper at intel.com>

>  	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		u64 sskpd = intel_uncore_read64(uncore, MCH_SSKPD);
>  
> -- 
> 2.25.1
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation