On Mon, 28 Apr 2025, Animesh Manna <animesh.ma...@intel.com> wrote:
> Simplify alpm check in enable/disable with has_alpm.
> Add a check for alpm during lobf disable which can be enabled
> with panel replay/psr2.
>
> Suggested-by: Jouni Högander <jouni.hogan...@intel.com>
> Signed-off-by: Animesh Manna <animesh.ma...@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_alpm.c | 23 +++++++++++++------
> .../drm/i915/display/intel_display_types.h | 3 +++
> drivers/gpu/drm/i915/display/intel_psr.c | 2 ++
> 3 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 1bf08b80c23f..aa3f442bf8bd 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -322,6 +322,8 @@ void intel_alpm_lobf_compute_config(struct intel_dp
> *intel_dp,
>
> crtc_state->has_lobf = (context_latency + guardband) >
> (first_sdp_position + waketime_in_lines);
> +
> + crtc_state->has_alpm |= crtc_state->has_lobf;
I'm averse to using bitwise operators on logical booleans.
> }
>
> static void lnl_alpm_configure(struct intel_dp *intel_dp,
> @@ -332,8 +334,7 @@ static void lnl_alpm_configure(struct intel_dp *intel_dp,
> enum port port = dp_to_dig_port(intel_dp)->base.port;
> u32 alpm_ctl;
>
> - if (DISPLAY_VER(display) < 20 || (!intel_psr_needs_alpm(intel_dp,
> crtc_state) &&
> - !crtc_state->has_lobf))
> + if (DISPLAY_VER(display) < 20 || !crtc_state->has_alpm)
> return;
>
> mutex_lock(&intel_dp->alpm_parameters.lock);
> @@ -417,12 +418,20 @@ void intel_alpm_pre_plane_update(struct
> intel_atomic_state *state,
> if (!intel_dp_is_edp(intel_dp))
> continue;
>
> - if (old_crtc_state->has_lobf) {
> - mutex_lock(&intel_dp->alpm_parameters.lock);
> + mutex_lock(&intel_dp->alpm_parameters.lock);
> + if (crtc_state->has_alpm) {
> + u32 alpm_ctl = intel_de_read(display, ALPM_CTL(display,
> cpu_transcoder));
> + if (alpm_ctl & ALPM_CTL_LOBF_ENABLE) {
> + alpm_ctl &= ~ALPM_CTL_LOBF_ENABLE;
> + intel_de_write(display, ALPM_CTL(display,
> cpu_transcoder), alpm_ctl);
> + drm_dbg_kms(display->drm, "Link off between
> frames (LOBF) disabled\n");
> + }
> + } else {
> intel_de_write(display, ALPM_CTL(display,
> cpu_transcoder), 0);
> - drm_dbg_kms(display->drm, "Link off between frames
> (LOBF) disabled\n");
> - mutex_unlock(&intel_dp->alpm_parameters.lock);
> + drm_dbg_kms(display->drm,
> + "Link off between frames (LOBF) with ALPM
> disabled\n");
> }
> + mutex_unlock(&intel_dp->alpm_parameters.lock);
> }
> }
>
> @@ -431,7 +440,7 @@ static void intel_alpm_enable_sink(struct intel_dp
> *intel_dp,
> {
> u8 val;
>
> - if (!intel_psr_needs_alpm(intel_dp, crtc_state) &&
> !crtc_state->has_lobf)
> + if (!crtc_state->has_alpm)
> return;
>
> val = DP_ALPM_ENABLE | DP_ALPM_LOCK_ERROR_IRQ_HPD_ENABLE;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 7415564d058a..6edcfa5d9c41 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1328,6 +1328,9 @@ struct intel_crtc_state {
>
> /* LOBF flag */
> bool has_lobf;
> +
> + /* ALPM flag */
What benefit does this or the above "LOBF flag" comment give?
If you don't know what "has_lobf" or "has_alpm" mean, how on earth would
adding the word "flag" help here?
> + bool has_alpm;
> };
>
> enum intel_pipe_crc_source {
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index ccd66bbc72f7..e643f36057f8 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1707,6 +1707,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>
> crtc_state->has_sel_update = intel_sel_update_config_valid(intel_dp,
> crtc_state);
>
> + crtc_state->has_alpm = intel_psr_needs_alpm(intel_dp, crtc_state);
Looks like the below thing can disable PSR usage, but you'll still leave
has_sel_update and (with this patch) has_alpm enabled. Are you taking
all those combos into account? Including in readout?
BR,
Jani.
> +
> /* Wa_18037818876 */
> if (intel_psr_needs_wa_18037818876(intel_dp, crtc_state)) {
> crtc_state->has_psr = false;
--
Jani Nikula, Intel
