> -----Original Message-----
> From: Hogander, Jouni <jouni.hogan...@intel.com>
> Sent: Thursday, March 27, 2025 3:33 PM
> To: intel...@lists.freedesktop.org; Manna, Animesh
> <animesh.ma...@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/psr: Add PSR pause/resume reference count
>
> On Thu, 2025-03-27 at 05:55 +0000, Manna, Animesh wrote:
> >
> >
> > > -----Original Message-----
> > > From: Hogander, Jouni <jouni.hogan...@intel.com>
> > > Sent: Friday, March 21, 2025 7:15 PM
> > > To: intel...@lists.freedesktop.org; Manna, Animesh
> > > <animesh.ma...@intel.com>; intel-gfx@lists.freedesktop.org
> > > Subject: Re: [PATCH] drm/i915/psr: Add PSR pause/resume reference
> > > count
> > >
> > > On Fri, 2025-03-21 at 09:44 +0000, Manna, Animesh wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Intel-gfx <intel-gfx-boun...@lists.freedesktop.org> On
> > > > > Behalf Of Jouni Högander
> > > > > Sent: Friday, March 14, 2025 12:20 PM
> > > > > To: intel-gfx@lists.freedesktop.org;
> > > > > intel...@lists.freedesktop.org
> > > > > Cc: Hogander, Jouni <jouni.hogan...@intel.com>
> > > > > Subject: [PATCH] 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.
> > > > >
> > > > > Signed-off-by: Jouni Högander <jouni.hogan...@intel.com>
> > > > > ---
> > > > > .../drm/i915/display/intel_display_types.h | 2 +-
> > > > > drivers/gpu/drm/i915/display/intel_psr.c | 26 +++++++++--
> > > > > ----
> > > > > ----
> > > > > 2 files changed, 13 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 99a6fd2900b9c..65c808bba1c6c 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 4e938bad808cc..4d4ecf7555b66 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -2024,7 +2024,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 @@ -2210,7 +2210,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)) @@
> > > > > -
> > > > > 2223,12 +2222,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);
> > > > >
> > > > > @@ -2251,13 +2248,14 @@ void intel_psr_resume(struct intel_dp
> > > > > *intel_dp)
> > > > >
> > > > > mutex_lock(&psr->lock);
> > > > >
> > > > > - if (!psr->paused)
> > > > > - goto unlock;
> > > > > + if (!psr->enabled) {
> > > >
> > > > Any reason not to check intel_dp->psr.pause_counter here, maybe we
> > > > can check for !psr->enabled && psr->pause_counter > 0.
> > > > Other changes are LGTM.
> > >
> > > Where you would decrease pause_counter? Are you concerned on
> > > unbalanced pause/resume calls?
> >
> > Yes without intel_psr_pause() getting called if resume function is
> > called while psr is enabled here pause_counter will be decremented
> > which might result unbalanced situation.
> > We may not hit the above scenario but good to add a check if
> > pause_counter > 0 then only later decrement it in the same place
> > currently added.
>
> That would cause problems and break the whole idea reference count. It
> would also hide possible issue.
>
> I can add drm_WARN_ON(!psr->pause_counter) into intel_psr_pause? That
> would reveal possible unbalance problem. If pause_counter is not properly
> decreased that would be visible in IGT testcases checking PSR entry. What do
> you think?
Sure, got it.. looks ok to me.
Regards,
Animesh
>
> BR,
>
> Jouni Högander
> >
> > Regards,
> > Animesh
> >
> > >
> > > BR,
> > >
> > > Jouni Högander
> > >
> > > >
> > > > Regards,
> > > > Animesh
> > > >
> > > > > + mutex_unlock(&psr->lock);
> > > > > + return;
> > > > > + }
> > > > >
> > > > > - psr->paused = false;
> > > > > - intel_psr_activate(intel_dp);
> > > > > + if (--intel_dp->psr.pause_counter == 0)
> > > > > + intel_psr_activate(intel_dp);
> > > > >
> > > > > -unlock:
> > > > > mutex_unlock(&psr->lock);
> > > > > }
> > > > >
> > > > > @@ -3322,7 +3320,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
> > > >
> >
