[PATCH] drm/i915/psr: Fix drm_WARN_ON in intel_psr_disable

[Kandpal, Suraj]

> -----Original Message-----
> From: Hogander, Jouni <jouni.hogander at intel.com>
> Sent: Friday, February 14, 2025 1:01 PM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>; intel-xe at lists.freedesktop.org;
> intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/psr: Fix drm_WARN_ON in intel_psr_disable
>
> On Fri, 2025-02-14 at 04:27 +0000, Kandpal, Suraj wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of
> > > Jouni Högander
> > > Sent: Thursday, February 13, 2025 4:46 PM
> > > To: intel-gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org
> > > Cc: Hogander, Jouni <jouni.hogander at intel.com>
> > > Subject: [PATCH] drm/i915/psr: Fix drm_WARN_ON in intel_psr_disable
> > >
> > > Currently intel_psr_disable is dumping out warning if PSR is not
> > > supported. On monitor supporting only Panel Replay we are seeing
> > > this warning.
> > > Fix this by
> > > checking Panel Replay support as well.
> >
> > LGTM,
> > Reviewed-by: Suraj Kandpal <suraj.kandpal at intel.com>
>
> Thank you very much Suraj.
>
> >
> > Note: Should we be looking into separating the code for panel replay
> > and psr with functions being Shared between the two files ? Will make
> > going through the code much easier. Just wondering if that Makes sense
> > in the future >
>
> I'm not yet buying this idea as they share so much. I have been thinking adding
> helpers for the purpose related to problem fixed in this patch:
>
> has_psr() // psr1, psr2, panel replay
> has_psr1()
> has_psr2()
> has_panel_replay()
>
> that would ease readability. What do you think?

That sounds better specially keeping in mind that psr checks happen all over the code and this
Should reduce some confusion when it comes to checking psr versions without having to really have a
Deeper look as to how those variables work

Regards,
Suraj Kandpal

>
> BR,
>
> Jouni Högander
>
> >
> > >
> > > Signed-off-by: Jouni Högander <jouni.hogander at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_psr.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 861e50ceef85..c77eb1ba3db3 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -2186,7 +2186,8 @@ void intel_psr_disable(struct intel_dp
> > > *intel_dp,
> > >   if (!old_crtc_state->has_psr)
> > >           return;
> > >
> > > - if (drm_WARN_ON(display->drm, !CAN_PSR(intel_dp)))
> > > + if (drm_WARN_ON(display->drm, !CAN_PSR(intel_dp) &&
> > > +                 !CAN_PANEL_REPLAY(intel_dp))
> > )
> > >           return;
> > >
> > >   mutex_lock(&intel_dp->psr.lock);
> > > --
> > > 2.43.0
> >

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20250214/9d365274/attachment.htm>