Re: [PATCH] drm/i915/alpm: Compute ALPM parameters into crtc_state->alpm_parameters

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Tue, 2025-09-23 at 14:17 +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogander@intel.com>
> > Sent: Tuesday, September 23, 2025 4:03 PM
> > To: intel-xe@lists.freedesktop.org; Manna, Animesh
> > <animesh.manna@intel.com>; intel-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH] drm/i915/alpm: Compute ALPM parameters into
> > crtc_state->alpm_parameters
> > 
> > On Tue, 2025-09-23 at 09:26 +0000, Manna, Animesh wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On
> > > > Behalf Of
> > > > Jouni Högander
> > > > Sent: Friday, September 5, 2025 12:31 PM
> > > > To: intel-gfx@lists.freedesktop.org;
> > > > intel-xe@lists.freedesktop.org
> > > > Cc: Hogander, Jouni <jouni.hogander@intel.com>
> > > > Subject: [PATCH] drm/i915/alpm: Compute ALPM parameters into
> > > > crtc_state-
> > > > > alpm_parameters
> > > > 
> > > > Currently ALPM parameters are computed directly into
> > > > intel_dp->alpm_parameters. This is a problem when compute
> > > > config
> > > > ends up to not using the computed state.
> 
> There are 6 alpm params added part of crtc_state, 2 are independent
> of mode and 4 are dependent of mode.
> If ALPM enablement conditions are not met that time intel_dp will be
> holding these values but not used as ALPM will not be enabled.
> Is it the only problem or something bigger?

Difference between new crtc state and old crtc state might not be ALPM
being enabled and disabled but parameter are changing. Then we would
end up writing parameters calculated for that new crtc state which was
not committed into the HW.

> 
> > > > 
> > > > Fix this by adding ALPM parameters into intel_crtc_state and
> > > > compute
> > > > into there. Copy needed parts of crtc_state->alpm_parameters
> > > > into
> > > > intel_dp->alpm.alpm_parameters (io_wake_lines and
> > > > fast_wake_lines)
> > > > when they are configured into HW.
> 
> Good to mentioned why copy is needed for 2 params and not done for
> rest 4 params.
> 
> > > > 
> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_alpm.c     | 38 ++++++++++-
> > > > ----
> > > > ----
> > > >  drivers/gpu/drm/i915/display/intel_alpm.h     |  2 +-
> > > >  .../drm/i915/display/intel_display_types.h    | 16 +++++---
> > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 20 +++++-----
> > > >  4 files changed, 43 insertions(+), 33 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > > b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > > index ed7a7ed486b5..bbc4fa2e084e 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > > @@ -133,7 +133,7 @@ static int
> > > > _lnl_compute_aux_less_wake_time(const
> > > > struct intel_crtc_state *crtc_s
> > > > 
> > > >  static int
> > > >  _lnl_compute_aux_less_alpm_params(struct intel_dp *intel_dp,
> > > > -				  const struct
> > > > intel_crtc_state
> > > > *crtc_state)
> > > > +				  struct intel_crtc_state
> > > > *crtc_state)
> > > >  {
> > > >  	struct intel_display *display =
> > > > to_intel_display(intel_dp);
> > > >  	int aux_less_wake_time, aux_less_wake_lines,
> > > > silence_period, @@
> > > > -157,15 +157,15 @@ _lnl_compute_aux_less_alpm_params(struct
> > > > intel_dp *intel_dp,
> > > >  	if (display->params.psr_safest_params)
> > > >  		aux_less_wake_lines =
> > > > ALPM_CTL_AUX_LESS_WAKE_TIME_MASK;
> > > > 
> > > > -	intel_dp->alpm_parameters.aux_less_wake_lines =
> > > > aux_less_wake_lines;
> > > > -	intel_dp->alpm_parameters.silence_period_sym_clocks =
> > > > silence_period;
> > > > -	intel_dp->alpm_parameters.lfps_half_cycle_num_of_syms
> > > > =
> > > > lfps_half_cycle;
> > > > +	crtc_state->alpm_parameters.aux_less_wake_lines =
> > > > aux_less_wake_lines;
> > > > +	crtc_state->alpm_parameters.silence_period_sym_clocks
> > > > =
> > > > silence_period;
> > > > +	crtc_state-
> > > > >alpm_parameters.lfps_half_cycle_num_of_syms =
> > > > lfps_half_cycle;
> > > > 
> > > >  	return true;
> > > >  }
> > > > 
> > > >  static bool _lnl_compute_alpm_params(struct intel_dp
> > > > *intel_dp,
> > > > -				     const struct
> > > > intel_crtc_state
> > > > *crtc_state)
> > > > +				     struct intel_crtc_state
> > > > *crtc_state)
> > > >  {
> > > >  	struct intel_display *display =
> > > > to_intel_display(intel_dp);
> > > >  	int check_entry_lines;
> > > > @@ -186,7 +186,7 @@ static bool _lnl_compute_alpm_params(struct
> > > > intel_dp *intel_dp,
> > > >  	if (display->params.psr_safest_params)
> > > >  		check_entry_lines = 15;
> > > > 
> > > > -	intel_dp->alpm_parameters.check_entry_lines =
> > > > check_entry_lines;
> > > > +	crtc_state->alpm_parameters.check_entry_lines =
> > > > check_entry_lines;
> > > > 
> > > >  	return true;
> > > >  }
> > > > @@ -217,7 +217,7 @@ static int io_buffer_wake_time(const struct
> > > > intel_crtc_state *crtc_state)
> > > >  }
> > > > 
> > > >  bool intel_alpm_compute_params(struct intel_dp *intel_dp,
> > > > -			       const struct intel_crtc_state
> > > > *crtc_state)
> > > > +			       struct intel_crtc_state
> > > > *crtc_state)
> > > >  {
> > > >  	struct intel_display *display =
> > > > to_intel_display(intel_dp);
> > > >  	int io_wake_lines, io_wake_time, fast_wake_lines,
> > > > fast_wake_time;
> > > > @@ -255,8 +255,8 @@ bool intel_alpm_compute_params(struct
> > > > intel_dp
> > > > *intel_dp,
> > > >  		io_wake_lines = fast_wake_lines =
> > > > max_wake_lines;
> > > > 
> > > >  	/* According to Bspec lower limit should be set as 7
> > > > lines. */
> > > > -	intel_dp->alpm_parameters.io_wake_lines =
> > > > max(io_wake_lines, 7);
> > > > -	intel_dp->alpm_parameters.fast_wake_lines =
> > > > max(fast_wake_lines,
> > > > 7);
> > > > +	crtc_state->alpm_parameters.io_wake_lines =
> > > > max(io_wake_lines,
> > > > 7);
> > > > +	crtc_state->alpm_parameters.fast_wake_lines =
> > > > max(fast_wake_lines, 7);
> > > > 
> > > >  	return true;
> > > >  }
> > > > @@ -306,9 +306,9 @@ void intel_alpm_lobf_compute_config(struct
> > > > intel_dp
> > > > *intel_dp,
> > > >  		    adjusted_mode->crtc_vdisplay -
> > > > context_latency;
> > > >  	first_sdp_position = adjusted_mode->crtc_vtotal -
> > > > adjusted_mode-
> > > > > crtc_vsync_start;
> > > >  	if (intel_alpm_aux_less_wake_supported(intel_dp))
> > > > -		waketime_in_lines = intel_dp-
> > > > > alpm_parameters.io_wake_lines;
> > > > +		waketime_in_lines = crtc_state-
> > > > > alpm_parameters.io_wake_lines;
> > > >  	else
> > > > -		waketime_in_lines = intel_dp-
> > > > > alpm_parameters.aux_less_wake_lines;
> > > > +		waketime_in_lines = crtc_state-
> > > > > alpm_parameters.aux_less_wake_lines;
> > > > 
> > > >  	crtc_state->has_lobf = (context_latency + guardband) >
> > > >  		(first_sdp_position + waketime_in_lines); @@ -
> > > > 334,7 +334,7
> > @@
> > > > static void lnl_alpm_configure(struct intel_dp *intel_dp,
> > > >  		alpm_ctl = ALPM_CTL_ALPM_ENABLE |
> > > >  			ALPM_CTL_ALPM_AUX_LESS_ENABLE |
> > > > 
> > > > 	ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_50_SYMBOLS |
> > > > -			ALPM_CTL_AUX_LESS_WAKE_TIME(intel_dp-
> > > > > alpm_parameters.aux_less_wake_lines);
> > > > +			ALPM_CTL_AUX_LESS_WAKE_TIME(crtc_state
> > > > -
> > > > > alpm_parameters.aux_less_wake_lines);
> > > > 
> > > >  		if (intel_dp->as_sdp_supported) {
> > > >  			u32 pr_alpm_ctl =
> > > > PR_ALPM_CTL_ADAPTIVE_SYNC_SDP_POSITION_T1;
> > > > @@ -352,7 +352,7 @@ static void lnl_alpm_configure(struct
> > > > intel_dp
> > > > *intel_dp,
> > > > 
> > > >  	} else {
> > > >  		alpm_ctl = ALPM_CTL_EXTENDED_FAST_WAKE_ENABLE
> > > > |
> > > > -
> > > > 			ALPM_CTL_EXTENDED_FAST_WAKE_TIME(intel_dp-
> > > > > alpm_parameters.fast_wake_lines);
> > > > +			ALPM_CTL_EXTENDED_FAST_WAKE_TIME(crtc_
> > > > stat
> > > > e-
> > > > > alpm_parameters.fast_wake_lines);
> > > >  	}
> > > > 
> > > >  	if (crtc_state->has_lobf) {
> > > > @@ -360,7 +360,7 @@ static void lnl_alpm_configure(struct
> > > > intel_dp
> > > > *intel_dp,
> > > >  		drm_dbg_kms(display->drm, "Link off between
> > > > frames
> > > > (LOBF) enabled\n");
> > > >  	}
> > > > 
> > > > -	alpm_ctl |= ALPM_CTL_ALPM_ENTRY_CHECK(intel_dp-
> > > > > alpm_parameters.check_entry_lines);
> > > > +	alpm_ctl |= ALPM_CTL_ALPM_ENTRY_CHECK(crtc_state-
> > > > > alpm_parameters.check_entry_lines);
> > > > 
> > > >  	intel_de_write(display, ALPM_CTL(display,
> > > > cpu_transcoder),
> > > > alpm_ctl);
> > > >  	mutex_unlock(&intel_dp->alpm_parameters.lock);
> > > > @@ -371,6 +371,8 @@ void intel_alpm_configure(struct intel_dp
> > > > *intel_dp,
> > > >  {
> > > >  	lnl_alpm_configure(intel_dp, crtc_state);
> > > >  	intel_dp->alpm_parameters.transcoder = crtc_state-
> > > > > cpu_transcoder;
> > > > +	intel_dp->alpm_parameters.io_wake_lines = crtc_state-
> > > > > alpm_parameters.io_wake_lines;
> > > > +	intel_dp->alpm_parameters.fast_wake_lines =
> > > > crtc_state-
> > > > > alpm_parameters.fast_wake_lines;
> > > 
> > > As I understood io_wake_lines and fast_wake_lines will be needed
> > > in
> > > psr_work() and getting crtc_state will not be possible.
> > > Is it needed to store to crtc_state? maybe for the above reason
> > > we can
> > > keep in intel_dp only.
> > 
> > Whole point of the patch is to have computed parameters that are
> > not yet
> > applied in crtc_state. Why this patch set is doing that: please
> > check commit
> > message and ask for clarification/improvements if needed.
> 
> Added my doubts above.
>   
> > 
> > > Even if we don’t write into h/w still these values are saved in
> > > intel_dp during early return from lnl_alpm_configure().
> > 
> > How about if we do it only when intel_psr_needs_alpm() == true?
> 
> Yes it will work.
> 
> Later if alpm got disabled then intel_dp-alpm params should be
> cleared... rt?

I think this is currently not done. It's not matter of this patch and I
don't think this is needed. It would probably not cause any harm
either.

BR,

Jouni Högander

> 
> Regards,
> Animesh 
> > 
> > > 
> > > >  }
> > > > 
> > > >  void intel_alpm_port_configure(struct intel_dp *intel_dp, @@
> > > > -388,14 +390,14 @@ void intel_alpm_port_configure(struct
> > > > intel_dp
> > > > *intel_dp,
> > > >  			PORT_ALPM_CTL_MAX_PHY_SWING_SETUP(15)
> > > > |
> > > >  			PORT_ALPM_CTL_MAX_PHY_SWING_HOLD(0) |
> > > >  			PORT_ALPM_CTL_SILENCE_PERIOD(
> > > > -				intel_dp-
> > > > > alpm_parameters.silence_period_sym_clocks);
> > > > +				crtc_state-
> > > > > alpm_parameters.silence_period_sym_clocks);
> > > >  		lfps_ctl_val =
> > > > PORT_ALPM_LFPS_CTL_LFPS_CYCLE_COUNT(LFPS_CYCLE_COUNT) |
> > > > 
> > > > 	PORT_ALPM_LFPS_CTL_LFPS_HALF_CYCLE_DURATION(
> > > > -				intel_dp-
> > > > > alpm_parameters.lfps_half_cycle_num_of_syms) |
> > > > +				crtc_state-
> > > > > alpm_parameters.lfps_half_cycle_num_of_syms) |
> > > > 
> > > > 	PORT_ALPM_LFPS_CTL_FIRST_LFPS_HALF_CYCLE_DURATION(
> > > > -				intel_dp-
> > > > > alpm_parameters.lfps_half_cycle_num_of_syms) |
> > > > +				crtc_state-
> > > > > alpm_parameters.lfps_half_cycle_num_of_syms) |
> > > > 
> > > > 	PORT_ALPM_LFPS_CTL_LAST_LFPS_HALF_CYCLE_DURATION(
> > > > -				intel_dp-
> > > > > alpm_parameters.lfps_half_cycle_num_of_syms);
> > > > +				crtc_state-
> > > > > alpm_parameters.lfps_half_cycle_num_of_syms);
> > > >  	}
> > > > 
> > > >  	intel_de_write(display, PORT_ALPM_CTL(port),
> > > > alpm_ctl_val); diff
> > > > --git a/drivers/gpu/drm/i915/display/intel_alpm.h
> > > > b/drivers/gpu/drm/i915/display/intel_alpm.h
> > > > index a861c20b5d79..53599b464dea 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> > > > @@ -17,7 +17,7 @@ struct intel_crtc;
> > > > 
> > > >  void intel_alpm_init(struct intel_dp *intel_dp);
> > > >  bool intel_alpm_compute_params(struct intel_dp *intel_dp,
> > > > -			       const struct intel_crtc_state
> > > > *crtc_state);
> > > > +			       struct intel_crtc_state
> > > > *crtc_state);
> > > >  void intel_alpm_lobf_compute_config(struct intel_dp *intel_dp,
> > > >  				    struct intel_crtc_state
> > > > *crtc_state,
> > > >  				    struct drm_connector_state
> > > > *conn_state);
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index fd9d2527889b..1fc778067397 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -1339,6 +1339,17 @@ struct intel_crtc_state {
> > > > 
> > > >  	/* LOBF flag */
> > > >  	bool has_lobf;
> > > > +
> > > > +	struct {
> > > > +		u8 io_wake_lines;
> > > > +		u8 fast_wake_lines;
> > > > +
> > > > +		/* LNL and beyond */
> > > > +		u8 check_entry_lines;
> > > > +		u8 aux_less_wake_lines;
> > > > +		u8 silence_period_sym_clocks;
> > > > +		u8 lfps_half_cycle_num_of_syms;
> > > > +	} alpm_parameters;
> > > 
> > > The naming can be alpm_state here instead of alpm_parameters
> > > which is
> > > present in intel_dp.
> > 
> > Ok, I will change this.
> > 
> > BR,
> > 
> > Jouni Högander
> > 
> > > 
> > > Regards,
> > > Animesh
> > > 
> > > >  };
> > > > 
> > > >  enum intel_pipe_crc_source {
> > > > @@ -1847,11 +1858,6 @@ struct intel_dp {
> > > >  		enum transcoder transcoder;
> > > >  		struct mutex lock;
> > > > 
> > > > -		/* LNL and beyond */
> > > > -		u8 check_entry_lines;
> > > > -		u8 aux_less_wake_lines;
> > > > -		u8 silence_period_sym_clocks;
> > > > -		u8 lfps_half_cycle_num_of_syms;
> > > >  		bool lobf_disable_debug;
> > > >  		bool sink_alpm_error;
> > > >  	} alpm_parameters;
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 22433fe2ee14..dd6df3154508 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -953,15 +953,16 @@ static u32 intel_psr2_get_tp_time(struct
> > > > intel_dp
> > > > *intel_dp)
> > > >  	return val;
> > > >  }
> > > > 
> > > > -static int psr2_block_count_lines(struct intel_dp *intel_dp)
> > > > +static int
> > > > +psr2_block_count_lines(u8 io_wake_lines, u8 fast_wake_lines)
> > > >  {
> > > > -	return intel_dp->alpm_parameters.io_wake_lines < 9 &&
> > > > -		intel_dp->alpm_parameters.fast_wake_lines < 9
> > > > ? 8
> > > > : 12;
> > > > +	return io_wake_lines < 9 && fast_wake_lines < 9 ? 8 :
> > > > 12;
> > > >  }
> > > > 
> > > >  static int psr2_block_count(struct intel_dp *intel_dp)
> > > >  {
> > > > -	return psr2_block_count_lines(intel_dp) / 4;
> > > > +	return psr2_block_count_lines(intel_dp-
> > > > > alpm_parameters.io_wake_lines,
> > > > +				      intel_dp-
> > > > > alpm_parameters.fast_wake_lines) / 4;
> > > >  }
> > > > 
> > > >  static u8 frames_before_su_entry(struct intel_dp *intel_dp) @@
> > > > -1367,11 +1368,12 @@ static bool
> > > > wake_lines_fit_into_vblank(struct
> > > > intel_dp *intel_dp,
> > > >  	int wake_lines;
> > > > 
> > > >  	if (aux_less)
> > > > -		wake_lines = intel_dp-
> > > > > alpm_parameters.aux_less_wake_lines;
> > > > +		wake_lines = crtc_state-
> > > > > alpm_parameters.aux_less_wake_lines;
> > > >  	else
> > > >  		wake_lines = DISPLAY_VER(display) < 20 ?
> > > > -			psr2_block_count_lines(intel_dp) :
> > > > -			intel_dp-
> > > > >alpm_parameters.io_wake_lines;
> > > > +			psr2_block_count_lines(crtc_state-
> > > > > alpm_parameters.io_wake_lines,
> > > > +					       crtc_state-
> > > > > alpm_parameters.fast_wake_lines) :
> > > > +			crtc_state-
> > > > >alpm_parameters.io_wake_lines;
> > > > 
> > > >  	if (crtc_state->req_psr2_sdp_prior_scanline)
> > > >  		vblank -= 1;
> > > > @@ -1384,7 +1386,7 @@ static bool
> > > > wake_lines_fit_into_vblank(struct
> > > > intel_dp *intel_dp,
> > > >  }
> > > > 
> > > >  static bool alpm_config_valid(struct intel_dp *intel_dp,
> > > > -			      const struct intel_crtc_state
> > > > *crtc_state,
> > > > +			      struct intel_crtc_state
> > > > *crtc_state,
> > > >  			      bool aux_less)
> > > >  {
> > > >  	struct intel_display *display =
> > > > to_intel_display(intel_dp);
> > > > @@ -1589,7 +1591,7 @@ static bool _psr_compute_config(struct
> > > > intel_dp *intel_dp,
> > > > 
> > > >  static bool
> > > >  _panel_replay_compute_config(struct intel_dp *intel_dp,
> > > > -			     const struct intel_crtc_state
> > > > *crtc_state,
> > > > +			     struct intel_crtc_state
> > > > *crtc_state,
> > > >  			     const struct drm_connector_state
> > > > *conn_state)
> > > >  {
> > > >  	struct intel_display *display =
> > > > to_intel_display(intel_dp);
> > > > --
> > > > 2.43.0
> > > 
>