diff mbox series

[v5,2/6] drm/i915/psr: Modify/fix Wa_16013835468 and prepare for Wa_14015648006

Message ID 20230322103412.123943-3-jouni.hogander@intel.com (mailing list archive)
State New, archived
Headers show
Series High refresh rate PSR fixes | expand

Commit Message

Hogander, Jouni March 22, 2023, 10:34 a.m. UTC 
Wa_16013835468 is a separate from Wa_14015648006 and needs to be
applied for TGL onwards. Fix this by removing all the references to
Wa_14015648006 and apply Wa_16013835468 according to Bspec.

Also move workaround into separate function as a preparation for
Wa_14015648006 implementation. Apply this workaround in post plane
hook.

Bspec: 55378

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 45 ++++++++++++++++--------
 1 file changed, 30 insertions(+), 15 deletions(-)

Comments

Ville Syrjälä March 28, 2023, 11:17 a.m. UTC | #1 
On Wed, Mar 22, 2023 at 12:34:08PM +0200, Jouni Högander wrote:
> Wa_16013835468 is a separate from Wa_14015648006 and needs to be
> applied for TGL onwards. Fix this by removing all the references to
> Wa_14015648006 and apply Wa_16013835468 according to Bspec.
> 
> Also move workaround into separate function as a preparation for
> Wa_14015648006 implementation. Apply this workaround in post plane
> hook.
> 
> Bspec: 55378
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 45 ++++++++++++++++--------
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 8dbf452d63c2..e66677e0554b 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1173,18 +1173,6 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
>  			     intel_dp->psr.psr2_sel_fetch_enabled ?
>  			     IGNORE_PSR2_HW_TRACKING : 0);
>  
> -	/*
> -	 * Wa_16013835468
> -	 * Wa_14015648006
> -	 */
> -	if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) ||
> -	    IS_DISPLAY_VER(dev_priv, 12, 13)) {
> -		if (crtc_state->hw.adjusted_mode.crtc_vblank_start !=
> -		    crtc_state->hw.adjusted_mode.crtc_vdisplay)
> -			intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, 0,
> -				     wa_16013835468_bit_get(intel_dp));
> -	}
> -
>  	if (intel_dp->psr.psr2_enabled) {
>  		if (DISPLAY_VER(dev_priv) == 9)
>  			intel_de_rmw(dev_priv, CHICKEN_TRANS(cpu_transcoder), 0,
> @@ -1359,10 +1347,8 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp)
>  
>  	/*
>  	 * Wa_16013835468
> -	 * Wa_14015648006
>  	 */
> -	if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) ||
> -	    IS_DISPLAY_VER(dev_priv, 12, 13))
> +	if (DISPLAY_VER(dev_priv) >= 12)
>  		intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
>  			     wa_16013835468_bit_get(intel_dp), 0);
>  
> @@ -1941,6 +1927,30 @@ void intel_psr_pre_plane_update(struct intel_atomic_state *state,
>  	}
>  }
>  
> +/*
> + * Wa_16013835468
> + */
> +static void wm_optimization_wa(struct intel_dp *intel_dp,
> +			   const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	bool set_wa_bit = false;
> +
> +	/* Wa_16013835468 */
> +	if (DISPLAY_VER(dev_priv) >= 12)
> +		set_wa_bit |= crtc_state->hw.adjusted_mode.crtc_vblank_start !=
> +			crtc_state->hw.adjusted_mode.crtc_vdisplay;
> +
> +	set_wa_bit &= intel_dp->psr.enabled;
> +
> +	if (set_wa_bit)
> +		intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, 0,
> +			     wa_16013835468_bit_get(intel_dp));
> +	else
> +		intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
> +			     wa_16013835468_bit_get(intel_dp), 0);
> +}
> +
>  static void _intel_psr_post_plane_update(const struct intel_atomic_state *state,
>  					 const struct intel_crtc_state *crtc_state)
>  {
> @@ -1966,6 +1976,11 @@ static void _intel_psr_post_plane_update(const struct intel_atomic_state *state,
>  		if (!psr->enabled && !keep_disabled)
>  			intel_psr_enable_locked(intel_dp, crtc_state);
>  
> +		/*
> +		 * Wa_16013835468
> +		 */
> +		wm_optimization_wa(intel_dp, crtc_state);

Hmm. I think the correct thing would probably be to set the
bit in pre_plane_update() and clear it in post_plane_update().
Otherwise we risk running with the bit in the wrong position
for a while.

> +
>  		/* Force a PSR exit when enabling CRC to avoid CRC timeouts */
>  		if (crtc_state->crc_enabled && psr->enabled)
>  			psr_force_hw_tracking_exit(intel_dp);
> -- 
> 2.34.1
Hogander, Jouni March 28, 2023, 11:31 a.m. UTC | #2 
On Tue, 2023-03-28 at 14:17 +0300, Ville Syrjälä wrote:
> On Wed, Mar 22, 2023 at 12:34:08PM +0200, Jouni Högander wrote:
> > Wa_16013835468 is a separate from Wa_14015648006 and needs to be
> > applied for TGL onwards. Fix this by removing all the references to
> > Wa_14015648006 and apply Wa_16013835468 according to Bspec.
> > 
> > Also move workaround into separate function as a preparation for
> > Wa_14015648006 implementation. Apply this workaround in post plane
> > hook.
> > 
> > Bspec: 55378
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 45 ++++++++++++++++----
> > ----
> >  1 file changed, 30 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 8dbf452d63c2..e66677e0554b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1173,18 +1173,6 @@ static void intel_psr_enable_source(struct
> > intel_dp *intel_dp,
> >                              intel_dp->psr.psr2_sel_fetch_enabled ?
> >                              IGNORE_PSR2_HW_TRACKING : 0);
> >  
> > -       /*
> > -        * Wa_16013835468
> > -        * Wa_14015648006
> > -        */
> > -       if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) ||
> > -           IS_DISPLAY_VER(dev_priv, 12, 13)) {
> > -               if (crtc_state->hw.adjusted_mode.crtc_vblank_start
> > !=
> > -                   crtc_state->hw.adjusted_mode.crtc_vdisplay)
> > -                       intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
> > 0,
> > -                                   
> > wa_16013835468_bit_get(intel_dp));
> > -       }
> > -
> >         if (intel_dp->psr.psr2_enabled) {
> >                 if (DISPLAY_VER(dev_priv) == 9)
> >                         intel_de_rmw(dev_priv,
> > CHICKEN_TRANS(cpu_transcoder), 0,
> > @@ -1359,10 +1347,8 @@ static void intel_psr_disable_locked(struct
> > intel_dp *intel_dp)
> >  
> >         /*
> >          * Wa_16013835468
> > -        * Wa_14015648006
> >          */
> > -       if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) ||
> > -           IS_DISPLAY_VER(dev_priv, 12, 13))
> > +       if (DISPLAY_VER(dev_priv) >= 12)
> >                 intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
> >                              wa_16013835468_bit_get(intel_dp), 0);
> >  
> > @@ -1941,6 +1927,30 @@ void intel_psr_pre_plane_update(struct
> > intel_atomic_state *state,
> >         }
> >  }
> >  
> > +/*
> > + * Wa_16013835468
> > + */
> > +static void wm_optimization_wa(struct intel_dp *intel_dp,
> > +                          const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +       struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +       bool set_wa_bit = false;
> > +
> > +       /* Wa_16013835468 */
> > +       if (DISPLAY_VER(dev_priv) >= 12)
> > +               set_wa_bit |= crtc_state-
> > >hw.adjusted_mode.crtc_vblank_start !=
> > +                       crtc_state->hw.adjusted_mode.crtc_vdisplay;
> > +
> > +       set_wa_bit &= intel_dp->psr.enabled;
> > +
> > +       if (set_wa_bit)
> > +               intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, 0,
> > +                            wa_16013835468_bit_get(intel_dp));
> > +       else
> > +               intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
> > +                            wa_16013835468_bit_get(intel_dp), 0);
> > +}
> > +
> >  static void _intel_psr_post_plane_update(const struct
> > intel_atomic_state *state,
> >                                          const struct
> > intel_crtc_state *crtc_state)
> >  {
> > @@ -1966,6 +1976,11 @@ static void
> > _intel_psr_post_plane_update(const struct intel_atomic_state
> > *state,
> >                 if (!psr->enabled && !keep_disabled)
> >                         intel_psr_enable_locked(intel_dp,
> > crtc_state);
> >  
> > +               /*
> > +                * Wa_16013835468
> > +                */
> > +               wm_optimization_wa(intel_dp, crtc_state);
> 
> Hmm. I think the correct thing would probably be to set the
> bit in pre_plane_update() and clear it in post_plane_update().
> Otherwise we risk running with the bit in the wrong position
> for a while.

I see intel_update_watermarks being called in intel_display.c:
intel_pre_plane_update. Is it ok to configure the bit before that is
complete? This was the main reason to place it in
pst_post_plane_update.

> 
> > +
> >                 /* Force a PSR exit when enabling CRC to avoid CRC
> > timeouts */
> >                 if (crtc_state->crc_enabled && psr->enabled)
> >                         psr_force_hw_tracking_exit(intel_dp);
> > -- 
> > 2.34.1
>
Ville Syrjälä March 28, 2023, 11:40 a.m. UTC | #3 
On Tue, Mar 28, 2023 at 11:31:56AM +0000, Hogander, Jouni wrote:
> On Tue, 2023-03-28 at 14:17 +0300, Ville Syrjälä wrote:
> > On Wed, Mar 22, 2023 at 12:34:08PM +0200, Jouni Högander wrote:
> > > Wa_16013835468 is a separate from Wa_14015648006 and needs to be
> > > applied for TGL onwards. Fix this by removing all the references to
> > > Wa_14015648006 and apply Wa_16013835468 according to Bspec.
> > > 
> > > Also move workaround into separate function as a preparation for
> > > Wa_14015648006 implementation. Apply this workaround in post plane
> > > hook.
> > > 
> > > Bspec: 55378
> > > 
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_psr.c | 45 ++++++++++++++++----
> > > ----
> > >  1 file changed, 30 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 8dbf452d63c2..e66677e0554b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1173,18 +1173,6 @@ static void intel_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > >                              intel_dp->psr.psr2_sel_fetch_enabled ?
> > >                              IGNORE_PSR2_HW_TRACKING : 0);
> > >  
> > > -       /*
> > > -        * Wa_16013835468
> > > -        * Wa_14015648006
> > > -        */
> > > -       if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) ||
> > > -           IS_DISPLAY_VER(dev_priv, 12, 13)) {
> > > -               if (crtc_state->hw.adjusted_mode.crtc_vblank_start
> > > !=
> > > -                   crtc_state->hw.adjusted_mode.crtc_vdisplay)
> > > -                       intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
> > > 0,
> > > -                                   
> > > wa_16013835468_bit_get(intel_dp));
> > > -       }
> > > -
> > >         if (intel_dp->psr.psr2_enabled) {
> > >                 if (DISPLAY_VER(dev_priv) == 9)
> > >                         intel_de_rmw(dev_priv,
> > > CHICKEN_TRANS(cpu_transcoder), 0,
> > > @@ -1359,10 +1347,8 @@ static void intel_psr_disable_locked(struct
> > > intel_dp *intel_dp)
> > >  
> > >         /*
> > >          * Wa_16013835468
> > > -        * Wa_14015648006
> > >          */
> > > -       if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) ||
> > > -           IS_DISPLAY_VER(dev_priv, 12, 13))
> > > +       if (DISPLAY_VER(dev_priv) >= 12)
> > >                 intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
> > >                              wa_16013835468_bit_get(intel_dp), 0);
> > >  
> > > @@ -1941,6 +1927,30 @@ void intel_psr_pre_plane_update(struct
> > > intel_atomic_state *state,
> > >         }
> > >  }
> > >  
> > > +/*
> > > + * Wa_16013835468
> > > + */
> > > +static void wm_optimization_wa(struct intel_dp *intel_dp,
> > > +                          const struct intel_crtc_state
> > > *crtc_state)
> > > +{
> > > +       struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > +       bool set_wa_bit = false;
> > > +
> > > +       /* Wa_16013835468 */
> > > +       if (DISPLAY_VER(dev_priv) >= 12)
> > > +               set_wa_bit |= crtc_state-
> > > >hw.adjusted_mode.crtc_vblank_start !=
> > > +                       crtc_state->hw.adjusted_mode.crtc_vdisplay;
> > > +
> > > +       set_wa_bit &= intel_dp->psr.enabled;
> > > +
> > > +       if (set_wa_bit)
> > > +               intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, 0,
> > > +                            wa_16013835468_bit_get(intel_dp));
> > > +       else
> > > +               intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
> > > +                            wa_16013835468_bit_get(intel_dp), 0);
> > > +}
> > > +
> > >  static void _intel_psr_post_plane_update(const struct
> > > intel_atomic_state *state,
> > >                                          const struct
> > > intel_crtc_state *crtc_state)
> > >  {
> > > @@ -1966,6 +1976,11 @@ static void
> > > _intel_psr_post_plane_update(const struct intel_atomic_state
> > > *state,
> > >                 if (!psr->enabled && !keep_disabled)
> > >                         intel_psr_enable_locked(intel_dp,
> > > crtc_state);
> > >  
> > > +               /*
> > > +                * Wa_16013835468
> > > +                */
> > > +               wm_optimization_wa(intel_dp, crtc_state);
> > 
> > Hmm. I think the correct thing would probably be to set the
> > bit in pre_plane_update() and clear it in post_plane_update().
> > Otherwise we risk running with the bit in the wrong position
> > for a while.
> 
> I see intel_update_watermarks being called in intel_display.c:
> intel_pre_plane_update. Is it ok to configure the bit before that is
> complete? This was the main reason to place it in
> pst_post_plane_update.

Yes. Setting the bit (to disable the magic optimization) is
fine any time. Clearing must only be done once we know it's
safe to do so.

Hmm. That actually makes me wonder about full modesets...
Either we need to always set the bit during full modeset
and then let the post_plane_update() clear it, or we need
to make sure we configure the bit correctly in
post_plane_update() regardless and do it before actually
enabling PSR.

Oh, and intel_update_watermarks()/etc. doesn't actually do
anything on skl+. On these platforms the watermarks are part
of the plane registers, and they are double buffered so no
magic pre/post sequence is needed to update them. Instead
they just get programmed alongside all the other double
buffered plane registers.

> 
> > 
> > > +
> > >                 /* Force a PSR exit when enabling CRC to avoid CRC
> > > timeouts */
> > >                 if (crtc_state->crc_enabled && psr->enabled)
> > >                         psr_force_hw_tracking_exit(intel_dp);
> > > -- 
> > > 2.34.1
> > 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 8dbf452d63c2..e66677e0554b 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1173,18 +1173,6 @@  static void intel_psr_enable_source(struct intel_dp *intel_dp,
 			     intel_dp->psr.psr2_sel_fetch_enabled ?
 			     IGNORE_PSR2_HW_TRACKING : 0);
 
-	/*
-	 * Wa_16013835468
-	 * Wa_14015648006
-	 */
-	if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) ||
-	    IS_DISPLAY_VER(dev_priv, 12, 13)) {
-		if (crtc_state->hw.adjusted_mode.crtc_vblank_start !=
-		    crtc_state->hw.adjusted_mode.crtc_vdisplay)
-			intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, 0,
-				     wa_16013835468_bit_get(intel_dp));
-	}
-
 	if (intel_dp->psr.psr2_enabled) {
 		if (DISPLAY_VER(dev_priv) == 9)
 			intel_de_rmw(dev_priv, CHICKEN_TRANS(cpu_transcoder), 0,
@@ -1359,10 +1347,8 @@  static void intel_psr_disable_locked(struct intel_dp *intel_dp)
 
 	/*
 	 * Wa_16013835468
-	 * Wa_14015648006
 	 */
-	if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) ||
-	    IS_DISPLAY_VER(dev_priv, 12, 13))
+	if (DISPLAY_VER(dev_priv) >= 12)
 		intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
 			     wa_16013835468_bit_get(intel_dp), 0);
 
@@ -1941,6 +1927,30 @@  void intel_psr_pre_plane_update(struct intel_atomic_state *state,
 	}
 }
 
+/*
+ * Wa_16013835468
+ */
+static void wm_optimization_wa(struct intel_dp *intel_dp,
+			   const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	bool set_wa_bit = false;
+
+	/* Wa_16013835468 */
+	if (DISPLAY_VER(dev_priv) >= 12)
+		set_wa_bit |= crtc_state->hw.adjusted_mode.crtc_vblank_start !=
+			crtc_state->hw.adjusted_mode.crtc_vdisplay;
+
+	set_wa_bit &= intel_dp->psr.enabled;
+
+	if (set_wa_bit)
+		intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, 0,
+			     wa_16013835468_bit_get(intel_dp));
+	else
+		intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
+			     wa_16013835468_bit_get(intel_dp), 0);
+}
+
 static void _intel_psr_post_plane_update(const struct intel_atomic_state *state,
 					 const struct intel_crtc_state *crtc_state)
 {
@@ -1966,6 +1976,11 @@  static void _intel_psr_post_plane_update(const struct intel_atomic_state *state,
 		if (!psr->enabled && !keep_disabled)
 			intel_psr_enable_locked(intel_dp, crtc_state);
 
+		/*
+		 * Wa_16013835468
+		 */
+		wm_optimization_wa(intel_dp, crtc_state);
+
 		/* Force a PSR exit when enabling CRC to avoid CRC timeouts */
 		if (crtc_state->crc_enabled && psr->enabled)
 			psr_force_hw_tracking_exit(intel_dp);