Re: [PATCH v2 3/4] drm/i915/psr: New interface adding PSR idle poll into dsb commit

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Aug 06, 2025 at 08:22:12AM +0300, Jouni Högander wrote:
> We are currently observing crc failures after we started using dsb for PSR
> updates as well. This seems to happen because PSR HW is still sending
> couple of updates using old framebuffers on wake-up.
> 
> This patch is preparing to fix that by adding interface which can be used
> to add poll ensuring PSR HW is idle into dsb commit.
> 
> v2: add pass crtc_state->dsb_commit as parameter
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 40 +++++++++++++++++++++---
>  drivers/gpu/drm/i915/display/intel_psr.h |  1 +
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 172bc0c39968..2254dd5a3ac4 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -42,6 +42,7 @@
>  #include "intel_dmc.h"
>  #include "intel_dp.h"
>  #include "intel_dp_aux.h"
> +#include "intel_dsb.h"
>  #include "intel_frontbuffer.h"
>  #include "intel_hdmi.h"
>  #include "intel_psr.h"
> @@ -2991,7 +2992,8 @@ void intel_psr_post_plane_update(struct intel_atomic_state *state,
>  #define PSR_IDLE_TIMEOUT_MS 50
>  
>  static int
> -_psr2_ready_for_pipe_update_locked(const struct intel_crtc_state *new_crtc_state)
> +_psr2_ready_for_pipe_update_locked(const struct intel_crtc_state *new_crtc_state,
> +				   struct intel_dsb *dsb)
>  {
>  	struct intel_display *display = to_intel_display(new_crtc_state);
>  	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> @@ -3001,6 +3003,13 @@ _psr2_ready_for_pipe_update_locked(const struct intel_crtc_state *new_crtc_state
>  	 * As all higher states has bit 4 of PSR2 state set we can just wait for
>  	 * EDP_PSR2_STATUS_STATE_DEEP_SLEEP to be cleared.
>  	 */
> +	if (dsb) {
> +		intel_dsb_poll(dsb, EDP_PSR2_STATUS(display, cpu_transcoder),
> +			       EDP_PSR2_STATUS_STATE_DEEP_SLEEP, 0, 200,
> +			       PSR_IDLE_TIMEOUT_MS * 1000 / 200);

The paramters look like they'll fit in the register. We should probably 
add some warns to intel_dsb_poll() to validate that though...

> +		return true;
> +	}
> +
>  	return intel_de_wait_for_clear(display,
>  				       EDP_PSR2_STATUS(display, cpu_transcoder),
>  				       EDP_PSR2_STATUS_STATE_DEEP_SLEEP,
> @@ -3008,11 +3017,19 @@ _psr2_ready_for_pipe_update_locked(const struct intel_crtc_state *new_crtc_state
>  }
>  
>  static int
> -_psr1_ready_for_pipe_update_locked(const struct intel_crtc_state *new_crtc_state)
> +_psr1_ready_for_pipe_update_locked(const struct intel_crtc_state *new_crtc_state,
> +				   struct intel_dsb *dsb)
>  {
>  	struct intel_display *display = to_intel_display(new_crtc_state);
>  	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
>  
> +	if (dsb) {
> +		intel_dsb_poll(dsb, psr_status_reg(display, cpu_transcoder),
> +			       EDP_PSR_STATUS_STATE_MASK, 0, 200,
> +			       PSR_IDLE_TIMEOUT_MS * 1000 / 200);
> +		return true;
> +	}
> +
>  	return intel_de_wait_for_clear(display,
>  				       psr_status_reg(display, cpu_transcoder),
>  				       EDP_PSR_STATUS_STATE_MASK,
> @@ -3045,9 +3062,11 @@ void intel_psr_wait_for_idle_locked(const struct intel_crtc_state *new_crtc_stat
>  			continue;
>  
>  		if (intel_dp->psr.sel_update_enabled)
> -			ret = _psr2_ready_for_pipe_update_locked(new_crtc_state);
> +			ret = _psr2_ready_for_pipe_update_locked(new_crtc_state,
> +								 NULL);
>  		else
> -			ret = _psr1_ready_for_pipe_update_locked(new_crtc_state);
> +			ret = _psr1_ready_for_pipe_update_locked(new_crtc_state,
> +								 NULL);
>  
>  		if (ret)
>  			drm_err(display->drm,
> @@ -3055,6 +3074,19 @@ void intel_psr_wait_for_idle_locked(const struct intel_crtc_state *new_crtc_stat
>  	}
>  }
>  
> +void intel_psr_wait_for_idle_dsb(const struct intel_crtc_state *new_crtc_state)
> +{
> +	if (!new_crtc_state->has_psr || new_crtc_state->has_panel_replay)
> +		return;
> +
> +	if (new_crtc_state->has_sel_update)
> +		_psr2_ready_for_pipe_update_locked(new_crtc_state,
> +						   new_crtc_state->dsb_commit);

Please pass the dsb all the way from the top so that it's easier to
change the DSB usage model if needed.

Otherwise lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +	else
> +		_psr1_ready_for_pipe_update_locked(new_crtc_state,
> +						   new_crtc_state->dsb_commit);
> +}
> +
>  static bool __psr_wait_for_idle_locked(struct intel_dp *intel_dp)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
> index 9b061a22361f..0cd0542b2450 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> @@ -52,6 +52,7 @@ void intel_psr_get_config(struct intel_encoder *encoder,
>  void intel_psr_irq_handler(struct intel_dp *intel_dp, u32 psr_iir);
>  void intel_psr_short_pulse(struct intel_dp *intel_dp);
>  void intel_psr_wait_for_idle_locked(const struct intel_crtc_state *new_crtc_state);
> +void intel_psr_wait_for_idle_dsb(const struct intel_crtc_state *new_crtc_state);
>  bool intel_psr_enabled(struct intel_dp *intel_dp);
>  int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
>  				struct intel_crtc *crtc);
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel