Re: [PATCH v3] drm/i915/psr: Add PSR pause/resume reference count

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

On Fri, 2025-03-28 at 16:31 +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogander@intel.com>
> > Sent: Friday, March 28, 2025 1:36 PM
> > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > Cc: Manna, Animesh <animesh.manna@intel.com>; Hogander, Jouni
> > <jouni.hogander@intel.com>
> > Subject: [PATCH v3] drm/i915/psr: Add PSR pause/resume reference
> > count
> > 
> > We have now seen this:
> > 
> > <4> [2120.434153] i915 0000:00:02.0: [drm] drm_WARN_ON(psr->paused)
> > <4> [2120.434196] WARNING: CPU: 3 PID: 4430 at
> > drivers/gpu/drm/i915/display/intel_psr.c:2227
> > intel_psr_pause+0x16e/0x180 [i915]
> > 
> > Comment for drm_WARN_ON(display->drm, psr->paused) in
> > intel_psr_pause says:
> > 
> > "If we ever hit this, we will need to add refcount to pause/resume"
> > 
> > This patch is implementing PSR pause/resume refcount.
> > 
> > v3: Incorporate changes missing from v2
> > v2: Add drm_warn for detecting possible unbalanced pause/resume
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> 
> LGTM.
> Reviewed-by: Animesh Manna <animesh.manna@intel.com>

Thank you Animesh for checking my patch. This is now pushed to drm-
intel-next.

BR,

Jouni Högander
> 
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  2 +-
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 31 ++++++++++-----
> > ----
> >  2 files changed, 18 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 7d9cc430a6b8b..367b53a9eae2a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1620,7 +1620,7 @@ struct intel_psr {
> >  	bool sink_support;
> >  	bool source_support;
> >  	bool enabled;
> > -	bool paused;
> > +	int pause_counter;
> >  	enum pipe pipe;
> >  	enum transcoder transcoder;
> >  	bool active;
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 708fbcfa9089a..0c57965f8f2f2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -2025,7 +2025,7 @@ static void intel_psr_enable_locked(struct
> > intel_dp
> > *intel_dp,
> > 
> >  	intel_psr_enable_source(intel_dp, crtc_state);
> >  	intel_dp->psr.enabled = true;
> > -	intel_dp->psr.paused = false;
> > +	intel_dp->psr.pause_counter = 0;
> > 
> >  	/*
> >  	 * Link_ok is sticky and set here on PSR enable. We can
> > assume link
> > @@ -2211,7 +2211,6 @@ void intel_psr_disable(struct intel_dp
> > *intel_dp,
> >   */
> >  void intel_psr_pause(struct intel_dp *intel_dp)  {
> > -	struct intel_display *display =
> > to_intel_display(intel_dp);
> >  	struct intel_psr *psr = &intel_dp->psr;
> > 
> >  	if (!CAN_PSR(intel_dp) && !CAN_PANEL_REPLAY(intel_dp)) @@
> > -
> > 2224,12 +2223,10 @@ void intel_psr_pause(struct intel_dp *intel_dp)
> >  		return;
> >  	}
> > 
> > -	/* If we ever hit this, we will need to add refcount to
> > pause/resume
> > */
> > -	drm_WARN_ON(display->drm, psr->paused);
> > -
> > -	intel_psr_exit(intel_dp);
> > -	intel_psr_wait_exit_locked(intel_dp);
> > -	psr->paused = true;
> > +	if (intel_dp->psr.pause_counter++ == 0) {
> > +		intel_psr_exit(intel_dp);
> > +		intel_psr_wait_exit_locked(intel_dp);
> > +	}
> > 
> >  	mutex_unlock(&psr->lock);
> > 
> > @@ -2245,6 +2242,7 @@ void intel_psr_pause(struct intel_dp
> > *intel_dp)
> >   */
> >  void intel_psr_resume(struct intel_dp *intel_dp)  {
> > +	struct intel_display *display =
> > to_intel_display(intel_dp);
> >  	struct intel_psr *psr = &intel_dp->psr;
> > 
> >  	if (!CAN_PSR(intel_dp) && !CAN_PANEL_REPLAY(intel_dp)) @@
> > -
> > 2252,13 +2250,18 @@ void intel_psr_resume(struct intel_dp
> > *intel_dp)
> > 
> >  	mutex_lock(&psr->lock);
> > 
> > -	if (!psr->paused)
> > -		goto unlock;
> > +	if (!psr->enabled)
> > +		goto out;
> > 
> > -	psr->paused = false;
> > -	intel_psr_activate(intel_dp);
> > +	if (!psr->pause_counter) {
> > +		drm_warn(display->drm, "Unbalanced PSR
> > pause/resume!\n");
> > +		goto out;
> > +	}
> > 
> > -unlock:
> > +	if (--intel_dp->psr.pause_counter == 0)
> > +		intel_psr_activate(intel_dp);
> > +
> > +out:
> >  	mutex_unlock(&psr->lock);
> >  }
> > 
> > @@ -3323,7 +3326,7 @@ void intel_psr_flush(struct intel_display
> > *display,
> >  		 * we have to ensure that the PSR is not activated
> > until
> >  		 * intel_psr_resume() is called.
> >  		 */
> > -		if (intel_dp->psr.paused)
> > +		if (intel_dp->psr.pause_counter)
> >  			goto unlock;
> > 
> >  		if (origin == ORIGIN_FLIP ||
> > --
> > 2.43.0
>