RE: [PATCH v2 04/10] drm/i915/alpm: Refactor Auxless wake time calculation

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 07 Nov 2025, "Manna, Animesh" <animesh.manna@intel.com> wrote:
>> -----Original Message-----
>> From: Kandpal, Suraj <suraj.kandpal@intel.com>
>> Sent: Friday, November 7, 2025 9:31 AM
>> To: Manna, Animesh <animesh.manna@intel.com>; intel-
>> gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org; dri-
>> devel@lists.freedesktop.org
>> Cc: Nikula, Jani <jani.nikula@intel.com>; Manna, Animesh
>> <animesh.manna@intel.com>; Hogander, Jouni
>> <jouni.hogander@intel.com>
>> 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 <jouni.hogander@intel.com>
>> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> > ---
>> >  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