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

From: Jani Nikula <jani.nikula@linux.intel.com>
To: "Kandpal, Suraj" <suraj.kandpal@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: Thu, 08 Feb 2024 10:59:01 +0200	[thread overview]
Message-ID: <874jejcox6.fsf@intel.com> (raw)
In-Reply-To: <SN7PR11MB675090C2560FBDD0C8D50714E3472@SN7PR11MB6750.namprd11.prod.outlook.com>

On Mon, 05 Feb 2024, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>> On Mon, 05 Feb 2024, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
>> > +		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

How is ~0 & LNL_PKG_C_LATENCY_MASK different from
LNL_PKG_C_LATENCY_MASK?

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

val |= REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, max_value);

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

Is that a good reason to change a function that only reads hardware to
something writes the hardware?

BR,
Jani.

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

-- 
Jani Nikula, Intel