[Intel-gfx] [PATCH v2] drm/i915/psr: Set "SF Partial Frame Enable" also on full update

[Hogander, Jouni]

On Fri, 2022-02-25 at 16:41 -0500, Lyude Paul wrote:
> JFYI I've been running the patch on my laptop for about a day now,
> flickering
> is totally gone and also I'm a bit astonished at the power savings!

Good to hear you had now this good user experience with i915 + latest
PSR2 fixes.

> Tested-by: Lyude Paul <lyude at redhat.com>
> 

Thank you a lot for testing support you provided here.

> On Thu, 2022-02-24 at 15:02 -0500, Lyude Paul wrote:
> > Also - I realized this is missing an appropriate Fixes: tag for the
> > commit
> > that enabled PSR2 selective fetch on tigerlake in the first place
> > 
> > On Wed, 2022-02-23 at 17:32 +0000, Souza, Jose wrote:
> > > On Wed, 2022-02-23 at 14:48 +0200, Jouni Högander wrote:
> > > > Currently we are observing occasional screen flickering when
> > > > PSR2 selective fetch is enabled. More specifically glitch seems
> > > > to happen on full frame update when cursor moves to coords
> > > > x = -1 or y = -1.
> > > > 
> > > > According to Bspec SF Single full frame should not be set if
> > > > SF Partial Frame Enable is not set. This happened to be true
> > > > for
> > > > ADLP as PSR2_MAN_TRK_CTL_ENABLE is always set and for ADLP it's
> > > > actually "SF Partial Frame Enable" (Bit 31).
> > > > 
> > > > Setting "SF Partial Frame Enable" bit also on full update seems
> > > > to
> > > > fix screen flickering.
> > > > 
> > > > Also make code more clear by setting PSR2_MAN_TRK_CTL_ENABLE
> > > > only if not on ADLP as this bit doesn't exist in ADLP.
> > > 
> > > Bit exist but has another name.
> > > 
> > > > Bspec: 49274
> > > > 
> > > > v2: Fix Mihai Harpau email address
> > > > 
> > > > Reported-by: Lyude Paul <lyude at redhat.com>
> > > > Cc: Mihai Harpau <mharpau at gmail.com>
> > > > Cc: José Roberto de Souza <jose.souza at intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Bugzilla: 
> > > > https://gitlab.freedesktop.org/drm/intel/-/issues/5077
> > > > Signed-off-by: Jouni Högander <jouni.hogander at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_psr.c | 20
> > > > ++++++++++++++++++--
> > > >  drivers/gpu/drm/i915/i915_reg.h          |  1 +
> > > >  2 files changed, 19 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 2e0b092f4b6b..90aca75e05e0 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -1439,6 +1439,13 @@ static inline u32
> > > > man_trk_ctl_single_full_frame_bit_get(struct drm_i915_private
> > > >                PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME;
> > > >  }
> > > >  
> > > > +static inline u32 man_trk_ctl_partial_frame_bit_get(struct
> > > > drm_i915_private *dev_priv)
> > > > +{
> > > > +       return IS_ALDERLAKE_P(dev_priv) ?
> > > > +              ADLP_PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE :
> > > > +              PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> > > > +}
> > > > +
> > > >  static void psr_force_hw_tracking_exit(struct intel_dp
> > > > *intel_dp)
> > > >  {
> > > >         struct drm_i915_private *dev_priv =
> > > > dp_to_i915(intel_dp);
> > > > @@ -1543,7 +1550,17 @@ static void psr2_man_trk_ctl_calc(struct
> > > > intel_crtc_state *crtc_state,
> > > >  {
> > > >         struct intel_crtc *crtc = to_intel_crtc(crtc_state-
> > > > >uapi.crtc);
> > > >         struct drm_i915_private *dev_priv = to_i915(crtc-
> > > > >base.dev);
> > > > -       u32 val = PSR2_MAN_TRK_CTL_ENABLE;
> > > > +       u32 val = 0;
> > > > +
> > > > +       /*
> > > > +        * ADL_P doesn't have HW tracking nor manual tracking
> > > > enable
> > > > +        * bit
> > > > +        */
> > > 
> > > Nit: Unnecessary comment.
> > > 
> > > Reviewed-by: José Roberto de Souza <jose.souza at intel.com>
> > > 
> > > > +       if (!IS_ALDERLAKE_P(dev_priv))
> > > > +               val = PSR2_MAN_TRK_CTL_ENABLE;
> > > > +
> > > > +       /* SF partial frame enable has to be set even on full
> > > > update */
> > > > +       val |= man_trk_ctl_partial_frame_bit_get(dev_priv);
> > > >  
> > > >         if (full_update) {
> > > >                 /*
> > > > @@ -1563,7 +1580,6 @@ static void psr2_man_trk_ctl_calc(struct
> > > > intel_crtc_state *crtc_state,
> > > >         } else {
> > > >                 drm_WARN_ON(crtc_state->uapi.crtc->dev, clip-
> > > > >y1 % 4 ||
> > > > clip->y2 % 4);
> > > >  
> > > > -               val |=
> > > > PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> > > >                 val |=
> > > > PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 /
> > > > 4
> > > > + 1);
> > > >                 val |=
> > > > PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4
> > > > +
> > > > 1);
> > > >         }
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 2b8a3086ed35..89bbb64e520d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -2316,6 +2316,7 @@
> > > >  #define 
> > > > ADLP_PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(val)       REG_FIELD
> > > > _PREP(ADL
> > > > P_
> > > > PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR_MASK, val)
> > > >  #define 
> > > > ADLP_PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR_MASK         REG_GENMA
> > > > SK(12, 0)
> > > >  #define 
> > > > ADLP_PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(val)         REG_FIELD
> > > > _PREP(ADL
> > > > P_
> > > > PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR_MASK, val)
> > > > +#define 
> > > > ADLP_PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE         REG_BIT(3
> > > > 1)
> > > >  #define 
> > > > ADLP_PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME            REG_BIT(1
> > > > 4)
> > > >  #define 
> > > > ADLP_PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME         REG_BIT(1
> > > > 3)
> > > >  

BR,

Jouni Högander