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