> -----Original Message-----
> From: Manna, Animesh <animesh.manna@xxxxxxxxx>
> Sent: Wednesday, December 18, 2024 4:04 PM
> To: Kandpal, Suraj <suraj.kandpal@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: 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.
I think this was a generalized rule as Matt Roper had pointed out we really don't want to tell other what faults ended up causing us to have this WA
>
> >
> > >
> > > 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.
>
Sure I have no problem with that
Regards,
Suraj Kandpal
> 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
