> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>
> Sent: Monday, December 9, 2024 7:19 PM
> To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH v2] drm/i915/display: Adjust Added Wake Time with
> PKG_C_LATENCY
>
>
>
> > -----Original Message-----
> > From: Manna, Animesh <animesh.manna@xxxxxxxxx>
> > Sent: Monday, December 9, 2024 1:17 PM
> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Manna, Animesh <animesh.manna@xxxxxxxxx>; Kandpal, Suraj
> > <suraj.kandpal@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> > ---
> > 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
