RE: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register

From: "Kandpal, Suraj" <suraj.kandpal@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Cc: "Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>
Subject: RE: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
Date: Mon, 5 Feb 2024 17:22:30 +0000	[thread overview]
Message-ID: <SN7PR11MB675090C2560FBDD0C8D50714E3472@SN7PR11MB6750.namprd11.prod.outlook.com> (raw)
In-Reply-To: <87le7zdxid.fsf@intel.com>

> Subject: Re: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
> 
> On Mon, 05 Feb 2024, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> > Program the PKGC_LATENCY register with the highest latency from level
> > 1 and above LP registers else program with all 1's.
> > This is used to improve package C residency by sending the highest
> > latency tolerance requirement (LTR) when the planes are done with the
> > frame until the next frame programming window (set context latency,
> > window 2) starts.
> > Bspec: 68986
> >
> > --v2
> > -Fix indentation [Chaitanya]
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/skl_watermark.c | 31
> > ++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index 051a02ac01a4..1ce4b33a407a 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -3394,6 +3394,34 @@ static void skl_read_wm_latency(struct
> drm_i915_private *i915, u16 wm[])
> >  	adjust_wm_latency(i915, wm, num_levels, read_latency);  }
> >
> > +/*
> > + * Program PKG_C_LATENCY Pkg C with highest valid latency from
> > + * watermark level1 and up and above. If watermark level 1 is
> > + * invalid program it with all 1's.
> > + * Program PKG_C_LATENCY Added Wake Time = 0.
> > + */
> > +static void intel_program_pkgc_latency(struct drm_i915_private *i915,
> > +				       u16 wm_latency[])
> > +{
> > +	u16 max_value = 0;
> > +	u32 clear = 0, val = 0;
> > +	int max_level = i915->display.wm.num_levels, i;
> 
> max_level seems useless, only used once.

Sure will fix this.

> 
> > +
> > +	for (i = 1; i <= max_level; i++) {
> 
> Array access goes out of bounds. Boom.
> 

Will fix this

> > +		if (wm_latency[i] == 0)
> > +			break;
> > +		else if (wm_latency[i] > max_value)
> > +			max_value = wm_latency[i];
> > +	}
> > +
> > +	if (max_value == 0)
> > +		max_value = ~0 & LNL_PKG_C_LATENCY_MASK;
> 
> What does "~0 &" gain you here?
> 

So max value is 0 for all bits except 0-12 as we need to set them as all 1's to disable deep pkgc State

> > +
> > +	clear |= LNL_ADDED_WAKE_TIME_MASK |
> LNL_PKG_C_LATENCY_MASK;
> > +	val |= max_value;
> 
> If you have fields defined for the register, why not use it for setting max value
> too?

Sorry I didn't get you here .

> 
> > +	intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val); }
> > +
> >  static void skl_setup_wm_latency(struct drm_i915_private *i915)  {
> >  	if (HAS_HW_SAGV_WM(i915))
> > @@ -3407,6 +3435,9 @@ static void skl_setup_wm_latency(struct
> drm_i915_private *i915)
> >  		skl_read_wm_latency(i915, i915->display.wm.skl_latency);
> >
> >  	intel_print_wm_latency(i915, "Gen9 Plane",
> > i915->display.wm.skl_latency);
> > +
> > +	if (DISPLAY_VER(i915) >= 20)
> > +		intel_program_pkgc_latency(i915, i915-
> >display.wm.skl_latency);
> 
> Before this, nothing in the skl_wm_init() path actually writes any registers, it's
> all readout. Is this the right place to be doing this?
> 

Yes since all latency values are all ready and available for use which we can program in the deep pkgc register.

Regards,
Suraj Kandpal
> >  }
> >
> >  static const struct intel_wm_funcs skl_wm_funcs = {
> 
> --
> Jani Nikula, Intel