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.ro...@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.srip...@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.ro...@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
