[PATCH v2] drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

[Manna, Animesh]

> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal at intel.com>
> Sent: Monday, December 9, 2024 7:19 PM
> To: Manna, Animesh <animesh.manna at intel.com>; intel-
> gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org
> Subject: RE: [PATCH v2] drm/i915/display: Adjust Added Wake Time with
> PKG_C_LATENCY
> 
> 
> 
> > -----Original Message-----
> > From: Manna, Animesh <animesh.manna at intel.com>
> > Sent: Monday, December 9, 2024 1:17 PM
> > To: intel-gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org
> > Cc: Manna, Animesh <animesh.manna at intel.com>; Kandpal, Suraj
> > <suraj.kandpal at intel.com>
> > Subject: [PATCH v2] drm/i915/display: Adjust Added Wake Time with
> > PKG_C_LATENCY
> >
> > The PKG_C_LATENCY Added Wake Time field is not working.
> > When added wake time is needed, such as for flip queue DSB execution,
> > increase the PKG_C_LATENCY Pkg C Latency field by the added wake time.
> 
> No need to mention the issue when It comes to WA only what the patch is
> doing the Rest of the info is present in the WA

Thanks for review.
Is it generalized rule? Not sure if want to add the background/workaround details what is the issue.

> 
> >
> > WA: 22020432604
> 
> This needs to come just above the CC with no new line in between CC and
> WA no.

Is it generalized rule? Good to know if captured somewhere, I see from git log many are not following the above.
 
> 
> >
> > v1: Initial version.
> > v2: Rebase and cosmetic changes.
> >
> > Cc: Suraj Kandpal <suraj.kandpal at intel.com>
> > Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/skl_watermark.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index d93f6786db0e..f6f7205e06eb 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -2894,6 +2894,12 @@ intel_program_dpkgc_latency(struct
> > intel_atomic_state *state)
> >  			display->sagv.block_time_us;
> >  	}
> >
> > +	/* Wa_22020432604 */
> > +	if (DISPLAY_VER(i915) == 30) {
> > +		latency += added_wake_time;
> 
> This wouldn't be the correct place to place it in since this would change the
> value in case the latency fetched is 0 From skl_watermark_max_latency and
> we actually want to write all 1's and want to disable the deep pkgc The best
> place would be right after fetching  max_latency  so it plays nice with the
> other WA and makes sure that pkgc latency Is a multiple of max line time
> when latency is not 0 So something like
> 
> If (display_ver && !latency)
> 	latency += added_wake_time;

Got your point, Will take care in next version.

> 
> this may also require you to move around where added_wake_time is
> assigned so that's a different patch
> 
> 
> > +		added_wake_time = 0;
> 
> Also lets not re assign 0 to added wake time variable let it just be written its
> not going to be used anyways and wil Not have any extra writes from our
> side

If added_wake_time is adjusted to pkgc latency then writing the same in register is not logically correct atleast from code readability POV, so better to reset to zero.

Regards,
Animesh

> 
> Regards,
> Suraj Kandpal
> 
> > +	}
> 
> 
> > +
> >  	clear = LNL_ADDED_WAKE_TIME_MASK |
> > LNL_PKG_C_LATENCY_MASK;
> >  	val = REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, latency) |
> >  		REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK,
> > added_wake_time);
> > --
> > 2.29.0