[Intel-gfx] [PATCH] drm/i915/lnl: Remove watchdog timers for PSR

[Kahola, Mika]

> -----Original Message-----
> From: Hogander, Jouni <jouni.hogander at intel.com>
> Sent: Friday, October 6, 2023 3:12 PM
> To: Kahola, Mika <mika.kahola at intel.com>; intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/lnl: Remove watchdog timers for PSR
> 
> On Fri, 2023-10-06 at 14:42 +0300, Mika Kahola wrote:
> > Currently we are not using watchdog timers for PSR/PSR2.
> > The patch disables these timers so they are not in use.
> >
> > BSpec: 69895
> >
> > Signed-off-by: Mika Kahola <mika.kahola at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 24 +++++++++++++++++-----
> > --
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 850b11f20285..13b58dceb2bf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -672,11 +672,15 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >         struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >         enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> >         u32 max_sleep_time = 0x1f;
> > -       u32 val = EDP_PSR_ENABLE;
> > +       u32 val = 0;
> 
> This is not related to the commit message. I.e. PSR1 is left disabled completely for DISPLAY_VER >= 20.
 Ah, ok. Just spotted these from the spec that these are not in use for DISPLAY_VER >= 20. But since PSR1
is disabled completely, I will drop these.

> >
> > -       val |=
> > EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> >
> > -       val |= EDP_PSR_MAX_SLEEP_TIME(max_sleep_time);
> > +       if (DISPLAY_VER(dev_priv) < 20) {
> > +               val =  EDP_PSR_ENABLE;
> > +               val |= EDP_PSR_MAX_SLEEP_TIME(max_sleep_time);
> > +       }
> > +
> > +       val |=
> > EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> >         if (IS_HASWELL(dev_priv))
> >                 val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
> >
> > @@ -1398,10 +1402,16 @@ static void intel_psr_enable_source(struct
> > intel_dp *intel_dp,
> >          * runtime_pm besides preventing  other hw tracking issues now
> > we
> >          * can rely on frontbuffer tracking.
> >          */
> > -       mask = EDP_PSR_DEBUG_MASK_MEMUP |
> > -              EDP_PSR_DEBUG_MASK_HPD |
> > -              EDP_PSR_DEBUG_MASK_LPSP |
> > -              EDP_PSR_DEBUG_MASK_MAX_SLEEP;
> > +       if (DISPLAY_VER(dev_priv) >= 20)
> > +               mask = EDP_PSR_DEBUG_MASK_MEMUP |
> > +                      EDP_PSR_DEBUG_MASK_HPD |
> > +                      EDP_PSR_DEBUG_MASK_LPSP;
> > +       else
> > +               mask = EDP_PSR_DEBUG_MASK_MEMUP |
> > +                      EDP_PSR_DEBUG_MASK_HPD |
> > +                      EDP_PSR_DEBUG_MASK_LPSP |
> > +                      EDP_PSR_DEBUG_MASK_MAX_SLEEP;
> 
> I would choose this:
> 
>        mask = EDP_PSR_DEBUG_MASK_MEMUP |
>               EDP_PSR_DEBUG_MASK_HPD |
>               EDP_PSR_DEBUG_MASK_LPSP;
> 
>        if (DISPLAY_VER(dev_priv) < 20)
>                mask |= EDP_PSR_DEBUG_MASK_MAX_SLEEP;
Right. This looks a bit cleaner solution. I will update

Thanks for the comments and review!

-Mika-

> 
> BR,
> 
> Jouni Högander
> 
> > +
> >
> >         /*
> >          * No separate pipe reg write mask on hsw/bdw, so have to
> > unmask all