> -----Original Message----- > From: Hogander, Jouni <[email protected]> > Sent: Tuesday, September 23, 2025 4:03 PM > To: [email protected]; Manna, Animesh > <[email protected]>; [email protected] > 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 <[email protected]> On Behalf Of > > > Jouni Högander > > > Sent: Friday, September 5, 2025 12:31 PM > > > To: [email protected]; [email protected] > > > Cc: Hogander, Jouni <[email protected]> > > > 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? > > > > > > 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 <[email protected]> > > > --- > > > 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? 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 > >
