| Message ID | 20231006114210.535229-1-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/lnl: Remove watchdog timers for PSR | expand |
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@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. > > - 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; BR, Jouni Högander > + > > /* > * No separate pipe reg write mask on hsw/bdw, so have to > unmask all
On Fri, Oct 06, 2023 at 02:42:10PM +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. I can't figure out what you're saying here. What bspec seems to be saying is that the max_sleep thing got nuked from the hw so we no longer need to mask it. > > BSpec: 69895 > > Signed-off-by: Mika Kahola <mika.kahola@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; > > - 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; That would mean you never enable PSR1 on lnl+ > + 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; The hpd mask bit also seems gone now, though there is no explanation why it disappeared. > + > > /* > * No separate pipe reg write mask on hsw/bdw, so have to unmask all > -- > 2.34.1
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Friday, October 6, 2023 3:29 PM > To: Kahola, Mika <mika.kahola@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/lnl: Remove watchdog timers for PSR > > On Fri, Oct 06, 2023 at 02:42:10PM +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. > > I can't figure out what you're saying here. What bspec seems to be saying is that the max_sleep thing got nuked from the hw so we > no longer need to mask it. Well, my understanding was that we would need to remove these from the driver as these are irrelevant. > > > > > BSpec: 69895 > > > > Signed-off-by: Mika Kahola <mika.kahola@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; > > > > - 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; > > That would mean you never enable PSR1 on lnl+ > > > + 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; > > The hpd mask bit also seems gone now, though there is no explanation why it disappeared. > > > + > > > > /* > > * No separate pipe reg write mask on hsw/bdw, so have to unmask all > > -- > > 2.34.1 > > -- > Ville Syrjälä > Intel
> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@intel.com> > Sent: Friday, October 6, 2023 3:12 PM > To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@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@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
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; - 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; + /* * No separate pipe reg write mask on hsw/bdw, so have to unmask all
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@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)