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

[Kandpal, Suraj]

> -----Original Message-----
> From: Manna, Animesh <animesh.manna at intel.com>
> Sent: Wednesday, December 18, 2024 4:04 PM
> To: Kandpal, Suraj <suraj.kandpal 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: 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.

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 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.
> 

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