On Fri, 07 Nov 2025, "Manna, Animesh" <[email protected]> wrote:
>> -----Original Message-----
>> From: Kandpal, Suraj <[email protected]>
>> Sent: Friday, November 7, 2025 9:31 AM
>> To: Manna, Animesh <[email protected]>; intel-
>> [email protected]; [email protected]; dri-
>> [email protected]
>> Cc: Nikula, Jani <[email protected]>; Manna, Animesh
>> <[email protected]>; Hogander, Jouni
>> <[email protected]>
>> Subject: RE: [PATCH v2 04/10] drm/i915/alpm: Refactor Auxless wake time
>> calculation
>>
>> > Subject: [PATCH v2 04/10] drm/i915/alpm: Refactor Auxless wake time
>> > calculation
>> >
>> > Divide the auxless wake time calculation in parts which will help
>> > later to add Xe3p related modification.
>> >
>> > v1: Initial version.
>>
>> No need for this you can start off with v2 directly Same for all patches
>> where
>> this has occured
>>
>> > v2: Refactor first existing calculation. [Jani]
>> >
>>
>> Add Bspec link
>>
>> > Cc: Jouni Högander <[email protected]>
>> > Signed-off-by: Animesh Manna <[email protected]>
>> > ---
>> > drivers/gpu/drm/i915/display/intel_alpm.c | 37
>> > ++++++++++++++++-------
>> > 1 file changed, 26 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
>> > b/drivers/gpu/drm/i915/display/intel_alpm.c
>> > index 779718d0c8dd..8d07455a62c2 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_alpm.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
>> > @@ -85,6 +85,26 @@ static int get_lfps_half_cycle_clocks(const struct
>> > intel_crtc_state *crtc_state)
>> > 1000 / (2 * LFPS_CYCLE_COUNT);
>> > }
>> >
>> > +static int get_tphy2_p2_to_p0(struct intel_dp *intel_dp) {
>> > + return 12 * 1000;
>> > +}
>> > +
>> > +static int get_establishment_period(struct intel_dp *intel_dp,
>> > + const struct intel_crtc_state *crtc_state) {
>> > + int port_clock = crtc_state->port_clock;
>> > + int t1 = 50 * 1000;
>> > + int tps4 = (252 * 10);
>>
>> Where did this * 10 come from?
>
> The Link Rate Divider (Rate Div) is 10 for 8b/10b and 32
> 128b/132b. Also please check the next patch for more clarity.
Please separate pure code movement and changes like this.
git show --color-moved on the commit should give you a nice idea what
code movement looks like. It's a breeze to review when very few things
change during code movement.
BR,
Jani.
>
>>
>> > + long tml_phy_lock = 1000 * 1000 * tps4 / port_clock / 10;
>>
>> Why the extra /10 required here also if you had not multiplied tps4 with 10
>> then this wouldn't be required You also removed the comment telling us
>> portclock need to be in 10Kb/s
>
> Divide by 10 always needed due to unit size of 10kb/sec, earlier both negated
> but for 128b/132b mode rate_div is 32. It will be more clear with next patch
> where added the changes for 128b/132b.
>
>>
>> > + int tcds, establishment_period;
>> > +
>> > + tcds = (7 + DIV_ROUND_UP(6500, tml_phy_lock) + 1) * tml_phy_lock;
>> > + establishment_period = (SILENCE_PERIOD_TIME + t1 + tcds);
>> > +
>> > + return establishment_period;
>> > +}
>> > +
>> > /*
>> > * AUX-Less Wake Time = CEILING( ((PHY P2 to P0) + tLFPS_Period, Max+
>> > * tSilence, Max+ tPHY Establishment + tCDS) / tline) @@ -104,19
>> > +124,14 @@ static int get_lfps_half_cycle_clocks(const struct
>> > intel_crtc_state
>> > *crtc_state)
>> > * tML_PHY_LOCK = TPS4 Length * ( 10 / (Link Rate in MHz) )
>> > * TPS4 Length = 252 Symbols
>> > */
>> > -static int _lnl_compute_aux_less_wake_time(const struct
>> > intel_crtc_state
>> > *crtc_state)
>> > +static int _lnl_compute_aux_less_wake_time(struct intel_dp *intel_dp,
>> > + const struct intel_crtc_state
>>
>> I don’t see any justified reason to send intel_dp here
>
> Its needed in next patch, fix the function prototype here.
>
> Regards,
> Animesh
>
>>
>> Regards,
>> Suraj Kandpal
>>
>> > *crtc_state)
>> > {
>> > - int tphy2_p2_to_p0 = 12 * 1000;
>> > - int t1 = 50 * 1000;
>> > - int tps4 = 252;
>> > - /* port_clock is link rate in 10kbit/s units */
>> > - int tml_phy_lock = 1000 * 1000 * tps4 / crtc_state->port_clock;
>> > - int num_ml_phy_lock = 7 + DIV_ROUND_UP(6500, tml_phy_lock) + 1;
>> > - int t2 = num_ml_phy_lock * tml_phy_lock;
>> > - int tcds = 1 * t2;
>> > + int tphy2_p2_to_p0 = get_tphy2_p2_to_p0(intel_dp);
>> > + int establishment_period = get_establishment_period(intel_dp,
>> > +crtc_state);
>> >
>> > return DIV_ROUND_UP(tphy2_p2_to_p0 +
>> > get_lfps_cycle_time(crtc_state) +
>> > - SILENCE_PERIOD_TIME + t1 + tcds, 1000);
>> > + establishment_period, 1000);
>> > }
>> >
>> > static int
>> > @@ -128,7 +143,7 @@ _lnl_compute_aux_less_alpm_params(struct
>> intel_dp
>> > *intel_dp,
>> > lfps_half_cycle;
>> >
>> > aux_less_wake_time =
>> > - _lnl_compute_aux_less_wake_time(crtc_state);
>> > + _lnl_compute_aux_less_wake_time(intel_dp, crtc_state);
>> > aux_less_wake_lines = intel_usecs_to_scanlines(&crtc_state-
>> > >hw.adjusted_mode,
>> > aux_less_wake_time);
>> > silence_period = get_silence_period_symbols(crtc_state);
>> > --
>> > 2.29.0
>
--
Jani Nikula, Intel
