[PATCH 2/3] drm/i915/watermark: Modify latency programmed into PKG_C_LATENCY

[Kandpal, Suraj]

> -----Original Message-----
> From: Golani, Mitulkumar Ajitkumar
> <mitulkumar.ajitkumar.golani at intel.com>
> Sent: Monday, November 11, 2024 6:56 PM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>; intel-xe at lists.freedesktop.org;
> intel-gfx at lists.freedesktop.org
> Cc: Govindapillai, Vinod <vinod.govindapillai at intel.com>; Kandpal, Suraj
> <suraj.kandpal at intel.com>
> Subject: RE: [PATCH 2/3] drm/i915/watermark: Modify latency programmed
> into PKG_C_LATENCY
> 
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of
> > Suraj Kandpal
> > Sent: 11 November 2024 18:03
> > To: intel-xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> > Cc: Govindapillai, Vinod <vinod.govindapillai at intel.com>; Kandpal,
> > Suraj <suraj.kandpal at intel.com>
> > Subject: [PATCH 2/3] drm/i915/watermark: Modify latency programmed
> > into PKG_C_LATENCY
> >
> > Increase the latency programmed into PKG_C_LATENCY latency to be a
> > multiple of line time which is written into WM_LINETIME.
> >
> > --v2
> > -Fix commit subject line [Sai Teja]
> > -Use individual DISPLAY_VER checks instead of range [Sai Teja]
> > -Initialize max_linetime [Sai Teja]
> >
> > --v3
> > -take into account the scenario when adjusted_latency is 0 [Vinod]
> >
> > WA: 22020299601
> > Signed-off-by: Suraj Kandpal <suraj.kandpal at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/skl_watermark.c | 26
> > ++++++++++++++------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index a97e90ac643f..e061015a89b0 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -2848,9 +2848,11 @@ static int skl_wm_add_affected_planes(struct
> > intel_atomic_state *state,
> >   * Program PKG_C_LATENCY Added Wake Time = 0
> >   */
> >  static void
> > -skl_program_dpkgc_latency(struct drm_i915_private *i915, bool
> > enable_dpkgc)
> > +skl_program_dpkgc_latency(struct drm_i915_private *i915,
> > +			  bool enable_dpkgc,
> > +			  u32 max_linetime)
> >  {
> > -	u32 max_latency = LNL_PKG_C_LATENCY_MASK;
> > +	u32 adjusted_latency = LNL_PKG_C_LATENCY_MASK;
> >  	u32 clear = 0, val = 0;
> >  	u32 added_wake_time = 0;
> >
> > @@ -2858,15 +2860,22 @@ skl_program_dpkgc_latency(struct
> > drm_i915_private *i915, bool enable_dpkgc)
> >  		return;
> >
> >  	if (enable_dpkgc) {
> > -		max_latency = skl_watermark_max_latency(i915, 1);
> > -		if (max_latency == 0)
> > -			max_latency = LNL_PKG_C_LATENCY_MASK;
> > +		adjusted_latency = skl_watermark_max_latency(i915, 1);
> > +
> > +		/* Wa_22020299601 */
> > +		if ((DISPLAY_VER(i915) == 20 || DISPLAY_VER(i915) == 30) &&
> > +		    adjusted_latency != 0)
> > +			adjusted_latency = max_linetime *
> > +				DIV_ROUND_UP(adjusted_latency,
> > max_linetime);
> > +		else
> > +			adjusted_latency = LNL_PKG_C_LATENCY_MASK;
> 
> You are already initialized it on the first instance, and wrote a patch #1 to get
> rid of duplicate of initialization. But why again ?

The reason i added the first patch to remove the else block for enable dpkgc variable which tells us if it's
fixed refresh rate or not.
In the second patch we add that else block to make sure that the wa is not applied in cases adjusted latency is 0.
Also adjusted_latency = skl_watermark_max_latency(i915, 1); 
Makes it so we will reassign adjusted latency which may become 0 too due to which the above else block was added.

> Also any reason to move away from 'max_latency' to 'adjusted_latency' ?
> all I can read through your commit message is, you are making this latency as
> multiple of linetime, any adjustment we are making ?

Yes original value would have been the simple max latency now the new value is linetime * ceil(max latency/linetime)
And if we are not taking the wa into picture we still program the max latency. Also the idea of keeping it adjusted latency
is that we write to the pkgc_latency register's Latency bits.
But we can rename it to latency if it seems confusing.

> 
> > +
> >  		added_wake_time = DSB_EXE_TIME +
> >  			i915->display.sagv.block_time_us;
> >  	}
> >
> >  	clear |= LNL_ADDED_WAKE_TIME_MASK |
> > LNL_PKG_C_LATENCY_MASK;
> > -	val |= REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, max_latency);
> > +	val |= REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK,
> > adjusted_latency);
> 
> Also you can think of this combine with below line for simplification ?

Sure ill send this refactor as a separate patch.

Regards,
Suraj Kandpal
> 
> >  	val |= REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK,
> > added_wake_time);
> >
> >  	intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val);
> @@
> > -2879,6 +2888,7 @@ skl_compute_wm(struct intel_atomic_state *state)
> >  	struct intel_crtc_state __maybe_unused *new_crtc_state;
> >  	int ret, i;
> >  	bool enable_dpkgc = false;
> > +	u32 max_linetime = 0;
> >
> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> >  		ret = skl_build_pipe_wm(state, crtc); @@ -2908,9 +2918,11
> @@
> > skl_compute_wm(struct intel_atomic_state *state)
> >  		     new_crtc_state->vrr.vmin == new_crtc_state->vrr.flipline)
> > ||
> >  		    !new_crtc_state->vrr.enable)
> >  			enable_dpkgc = true;
> > +
> > +		max_linetime = max(new_crtc_state->linetime,
> > max_linetime);
> >  	}
> >
> > -	skl_program_dpkgc_latency(to_i915(state->base.dev),
> > enable_dpkgc);
> > +	skl_program_dpkgc_latency(to_i915(state->base.dev), enable_dpkgc,
> > +max_linetime);
> >
> >  	skl_print_wm_changes(state);
> >
> > --
> > 2.34.1