Re: [PATCH v4 2/2] drm/i915/alpm: Stop writing ALPM registers when PSR is enabled

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

On Tue, 2025-05-13 at 06:01 +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogander@intel.com>
> > Sent: Tuesday, May 13, 2025 11:18 AM
> > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > Cc: Manna, Animesh <animesh.manna@intel.com>; Kandpal, Suraj
> > <suraj.kandpal@intel.com>; Hogander, Jouni
> > <jouni.hogander@intel.com>
> > Subject: [PATCH v4 2/2] drm/i915/alpm: Stop writing ALPM registers
> > when PSR
> > is enabled
> > 
> > Currently we are seeing these on PTL:
> > 
> > xe 0000:00:02.0: [drm] *ERROR* Timeout waiting for DDI BUF A to get
> > active
> > 
> > These seem to be caused by writing ALPM registers while Panel
> > Replay is
> > enabled.
> > 
> > Fix this by writing ALPM registers only when Panel Replay is about
> > to be
> > enabled.
> > 
> > v4: improve comment on intel_psr_panel_replay_enable_sink call
> > v3: enable/disable ALPM from PSR code
> > 
> > Fixes: 172757acd6f6 ("drm/i915/lobf: Add lobf enablement in post
> > plane
> > update")
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> 
> LGTM,
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>

Thank you Suraj for checking my patches. These are not pushed to drm-
intel-next.

BR,

Jouni Högander

> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_alpm.c |  4 ++--
> > drivers/gpu/drm/i915/display/intel_psr.c  | 13 ++++++++++---
> >  2 files changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> > b/drivers/gpu/drm/i915/display/intel_alpm.c
> > index da70a98a1292..c176bdbc19a3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> > @@ -453,8 +453,8 @@ void intel_alpm_post_plane_update(struct
> > intel_atomic_state *state,
> >  		intel_atomic_get_old_crtc_state(state, crtc);
> >  	struct intel_encoder *encoder;
> > 
> > -	if ((!crtc_state->has_lobf ||
> > -	     crtc_state->has_lobf == old_crtc_state->has_lobf) &&
> > !crtc_state-
> > > has_psr)
> > +	if (crtc_state->has_psr || !crtc_state->has_lobf ||
> > +	    crtc_state->has_lobf == old_crtc_state->has_lobf)
> >  		return;
> > 
> >  	for_each_intel_encoder_mask(display->drm, encoder, diff --
> > git
> > a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index ccd66bbc72f7..430ad4ef7146 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -800,6 +800,8 @@ static void _psr_enable_sink(struct intel_dp
> > *intel_dp,
> > static void intel_psr_enable_sink(struct intel_dp *intel_dp,
> >  				  const struct intel_crtc_state
> > *crtc_state)  {
> > +	intel_alpm_enable_sink(intel_dp, crtc_state);
> > +
> >  	crtc_state->has_panel_replay ?
> >  		_panel_replay_enable_sink(intel_dp, crtc_state) :
> >  		_psr_enable_sink(intel_dp, crtc_state); @@ -1962,6
> > +1964,8
> > @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
> >  	     IS_DISPLAY_VERx100_STEP(display, 3000, STEP_A0,
> > STEP_B0)) &&
> >  	    !intel_dp->psr.panel_replay_enabled)
> >  		intel_dmc_block_pkgc(display, intel_dp->psr.pipe,
> > true);
> > +
> > +	intel_alpm_configure(intel_dp, crtc_state);
> >  }
> > 
> >  static bool psr_interrupt_error_check(struct intel_dp *intel_dp)
> > @@ -2029,8
> > +2033,9 @@ static void intel_psr_enable_locked(struct intel_dp
> > *intel_dp,
> >  			    intel_dp->psr.sel_update_enabled ? "2"
> > : "1");
> > 
> >  	/*
> > -	 * Enabling here only for PSR. Panel Replay enable bit is
> > already
> > -	 * written at this point. See
> > +	 * Enabling sink PSR/Panel Replay here only for PSR. Panel
> > Replay
> > enable
> > +	 * bit is already written at this point. Sink ALPM is
> > enabled here for
> > +	 * PSR and Panel Replay. See
> >  	 * intel_psr_panel_replay_enable_sink. Modifiers/options:
> >  	 *  - Selective Update
> >  	 *  - Region Early Transport
> > @@ -2172,6 +2177,9 @@ static void intel_psr_disable_locked(struct
> > intel_dp
> > *intel_dp)
> >  	if (intel_dp_is_edp(intel_dp))
> > 
> > 	intel_snps_phy_update_psr_power_state(&dp_to_dig_port(inte
> > l_dp)-
> > > base, false);
> > 
> > +	if (intel_dp->psr.panel_replay_enabled &&
> > intel_dp_is_edp(intel_dp))
> > +		intel_alpm_disable(intel_dp);
> > +
> >  	/* Disable PSR on Sink */
> >  	if (!intel_dp->psr.panel_replay_enabled) {
> >  		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> > 0);
> > @@ -3498,7 +3506,6 @@ static void psr_alpm_check(struct intel_dp
> > *intel_dp)
> >  	if (intel_alpm_get_error(intel_dp)) {
> >  		intel_psr_disable_locked(intel_dp);
> >  		psr->sink_not_reliable = true;
> > -		intel_alpm_disable(intel_dp);
> >  	}
> >  }
> > 
> > --
> > 2.43.0
>