| Message ID | 20250314065015.879143-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/psr: Add PSR pause/resume reference count | expand |
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Jouni > Högander > Sent: Friday, March 14, 2025 12:20 PM > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > Cc: Hogander, Jouni <jouni.hogander@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.hogander@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. 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
On Fri, 2025-03-21 at 09:44 +0000, Manna, Animesh wrote: > > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf > > Of Jouni > > Högander > > Sent: Friday, March 14, 2025 12:20 PM > > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > > Cc: Hogander, Jouni <jouni.hogander@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.hogander@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? 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 >
> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@intel.com> > Sent: Friday, March 21, 2025 7:15 PM > To: intel-xe@lists.freedesktop.org; Manna, Animesh > <animesh.manna@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-bounces@lists.freedesktop.org> On Behalf > > > Of Jouni Högander > > > Sent: Friday, March 14, 2025 12:20 PM > > > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > > > Cc: Hogander, Jouni <jouni.hogander@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.hogander@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. 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 > >
On Thu, 2025-03-27 at 05:55 +0000, Manna, Animesh wrote: > > > > -----Original Message----- > > From: Hogander, Jouni <jouni.hogander@intel.com> > > Sent: Friday, March 21, 2025 7:15 PM > > To: intel-xe@lists.freedesktop.org; Manna, Animesh > > <animesh.manna@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-bounces@lists.freedesktop.org> On > > > > Behalf > > > > Of Jouni Högander > > > > Sent: Friday, March 14, 2025 12:20 PM > > > > To: intel-gfx@lists.freedesktop.org; > > > > intel-xe@lists.freedesktop.org > > > > Cc: Hogander, Jouni <jouni.hogander@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.hogander@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? 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 > > > >
> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@intel.com> > Sent: Thursday, March 27, 2025 3:33 PM > To: intel-xe@lists.freedesktop.org; Manna, Animesh > <animesh.manna@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.hogander@intel.com> > > > Sent: Friday, March 21, 2025 7:15 PM > > > To: intel-xe@lists.freedesktop.org; Manna, Animesh > > > <animesh.manna@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-bounces@lists.freedesktop.org> On > > > > > Behalf Of Jouni Högander > > > > > Sent: Friday, March 14, 2025 12:20 PM > > > > > To: intel-gfx@lists.freedesktop.org; > > > > > intel-xe@lists.freedesktop.org > > > > > Cc: Hogander, Jouni <jouni.hogander@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.hogander@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 > > > > > >
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) { + 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 ||
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.hogander@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(-)