| Message ID | 20230320165945.3564891-3-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | High refresh rate PSR fixes | expand |
On Mon, Mar 20, 2023 at 06:59:43PM +0200, Jouni Högander wrote: > PSR WM optimization should be disabled based on any wm level being > disabled. Currently it's disabled always when using delayed vblank. > Also same WA should be applied for ICL as well > > Bspec: 71580 > > v2: > - set/clear chicken bit in post_plane_update > - apply for ICL as well > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > .../drm/i915/display/intel_display_types.h | 1 + > drivers/gpu/drm/i915/display/intel_psr.c | 67 ++++++++++++------- > drivers/gpu/drm/i915/display/skl_watermark.c | 7 +- > 3 files changed, 50 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index c32bfba06ca1..60504c390408 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -1152,6 +1152,7 @@ struct intel_crtc_state { > bool has_psr2; > bool enable_psr2_sel_fetch; > bool req_psr2_sdp_prior_scanline; > + bool wm_level_disabled; > u32 dc3co_exitline; > u16 su_y_granularity; > struct drm_dp_vsc_sdp psr_vsc; > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index 8dbf452d63c2..1050d777a108 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, > @@ -1362,7 +1350,7 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp) > * Wa_14015648006 > */ > if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) || > - IS_DISPLAY_VER(dev_priv, 12, 13)) > + IS_DISPLAY_VER(dev_priv, 11, 13)) > intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, > wa_16013835468_bit_get(intel_dp), 0); > > @@ -1941,33 +1929,64 @@ void intel_psr_pre_plane_update(struct intel_atomic_state *state, > } > } > > +/* > + * Wa_16013835468 > + * Wa_14015648006 > + */ > +static void wa_16013835468(struct intel_dp *intel_dp, bool wm_level_disabled) > +{ > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + > + if (!IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) && > + !IS_DISPLAY_VER(dev_priv, 11, 13)) > + return; This is still confusing the two different workarounds, and actually losing the one about the delayed vblank. > + > + if (intel_dp->psr.enabled && wm_level_disabled) > + 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) > + const struct intel_crtc_state *old_crtc_state, > + const struct intel_crtc_state *new_crtc_state) The rename reduces the signal-to-noise ratio near zero. > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > struct intel_encoder *encoder; > > - if (!crtc_state->has_psr) > + if (!new_crtc_state->has_psr) > return; > > for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, > - crtc_state->uapi.encoder_mask) { > + new_crtc_state->uapi.encoder_mask) { > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > struct intel_psr *psr = &intel_dp->psr; > bool keep_disabled = false; > > mutex_lock(&psr->lock); > > - drm_WARN_ON(&dev_priv->drm, psr->enabled && !crtc_state->active_planes); > + drm_WARN_ON(&dev_priv->drm, psr->enabled && > + !new_crtc_state->active_planes); > > keep_disabled |= psr->sink_not_reliable; > - keep_disabled |= !crtc_state->active_planes; > + keep_disabled |= !new_crtc_state->active_planes; > > if (!psr->enabled && !keep_disabled) > - intel_psr_enable_locked(intel_dp, crtc_state); > + intel_psr_enable_locked(intel_dp, new_crtc_state); > + > + /* > + * Wa_16013835468 > + * Wa_14015648006 > + */ > + if (old_crtc_state->wm_level_disabled != > + new_crtc_state->wm_level_disabled) > + wa_16013835468(intel_dp, > + new_crtc_state->wm_level_disabled); > > /* Force a PSR exit when enabling CRC to avoid CRC timeouts */ > - if (crtc_state->crc_enabled && psr->enabled) > + if (new_crtc_state->crc_enabled && psr->enabled) > psr_force_hw_tracking_exit(intel_dp); > > mutex_unlock(&psr->lock); > @@ -1977,15 +1996,17 @@ static void _intel_psr_post_plane_update(const struct intel_atomic_state *state, > void intel_psr_post_plane_update(const struct intel_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > - struct intel_crtc_state *crtc_state; > + struct intel_crtc_state *new_crtc_state, *old_crtc_state; > struct intel_crtc *crtc; > int i; > > if (!HAS_PSR(dev_priv)) > return; > > - for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) > - _intel_psr_post_plane_update(state, crtc_state); > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > + new_crtc_state, i) > + _intel_psr_post_plane_update(state, old_crtc_state, > + new_crtc_state); > } > > static int _psr2_ready_for_pipe_update_locked(struct intel_dp *intel_dp) > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c > index 50a9a6adbe32..afb751c024ba 100644 > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > @@ -2273,9 +2273,12 @@ static int skl_wm_check_vblank(struct intel_crtc_state *crtc_state) > return level; > > /* > - * FIXME PSR needs to toggle LATENCY_REPORTING_REMOVED_PIPE_* > + * PSR needs to toggle LATENCY_REPORTING_REMOVED_PIPE_* > * based on whether we're limited by the vblank duration. > - * > + */ > + crtc_state->wm_level_disabled = level < i915->display.wm.num_levels - 1; > + > + /* > * FIXME also related to skl+ w/a 1136 (also unimplemented as of > * now) perhaps? > */ > -- > 2.34.1
Thank you Ville for checking my patch. Please see my responses below. On Tue, 2023-03-21 at 17:34 +0200, Ville Syrjälä wrote: > On Mon, Mar 20, 2023 at 06:59:43PM +0200, Jouni Högander wrote: > > PSR WM optimization should be disabled based on any wm level being > > disabled. Currently it's disabled always when using delayed vblank. > > Also same WA should be applied for ICL as well > > > > Bspec: 71580 > > > > v2: > > - set/clear chicken bit in post_plane_update > > - apply for ICL as well > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > --- > > .../drm/i915/display/intel_display_types.h | 1 + > > drivers/gpu/drm/i915/display/intel_psr.c | 67 ++++++++++++--- > > ---- > > drivers/gpu/drm/i915/display/skl_watermark.c | 7 +- > > 3 files changed, 50 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index c32bfba06ca1..60504c390408 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1152,6 +1152,7 @@ struct intel_crtc_state { > > bool has_psr2; > > bool enable_psr2_sel_fetch; > > bool req_psr2_sdp_prior_scanline; > > + bool wm_level_disabled; > > u32 dc3co_exitline; > > u16 su_y_granularity; > > struct drm_dp_vsc_sdp psr_vsc; > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index 8dbf452d63c2..1050d777a108 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, > > @@ -1362,7 +1350,7 @@ static void intel_psr_disable_locked(struct > > intel_dp *intel_dp) > > * Wa_14015648006 > > */ > > if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) || > > - IS_DISPLAY_VER(dev_priv, 12, 13)) > > + IS_DISPLAY_VER(dev_priv, 11, 13)) > > intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, > > wa_16013835468_bit_get(intel_dp), 0); > > > > @@ -1941,33 +1929,64 @@ void intel_psr_pre_plane_update(struct > > intel_atomic_state *state, > > } > > } > > > > +/* > > + * Wa_16013835468 > > + * Wa_14015648006 > > + */ > > +static void wa_16013835468(struct intel_dp *intel_dp, bool > > wm_level_disabled) > > +{ > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + > > + if (!IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) && > > + !IS_DISPLAY_VER(dev_priv, 11, 13)) > > + return; > > This is still confusing the two different workarounds, > and actually losing the one about the delayed vblank. This is only one Wa. There are two lineage numbers describing same workaround. Can you please point me out the delayed vblank wa description? I can't find it from Bspec? Original implementation is referring lineages 1601383546 and 14015648006. Neither of these are about delayed vblank specifically. > > > + > > + if (intel_dp->psr.enabled && wm_level_disabled) > > + 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) > > + const struct > > intel_crtc_state *old_crtc_state, > > + const struct > > intel_crtc_state *new_crtc_state) > > The rename reduces the signal-to-noise ratio near zero. Can you please elaborate this a bit more? What would you suggest? I need to use both, old and new state to detect difference in wm_level_disabled. I want to set/clear the chicken bit only when necessary. > > > { > > struct drm_i915_private *dev_priv = to_i915(state- > > >base.dev); > > struct intel_encoder *encoder; > > > > - if (!crtc_state->has_psr) > > + if (!new_crtc_state->has_psr) > > return; > > > > for_each_intel_encoder_mask_with_psr(state->base.dev, > > encoder, > > - crtc_state- > > >uapi.encoder_mask) { > > + new_crtc_state- > > >uapi.encoder_mask) { > > struct intel_dp *intel_dp = > > enc_to_intel_dp(encoder); > > struct intel_psr *psr = &intel_dp->psr; > > bool keep_disabled = false; > > > > mutex_lock(&psr->lock); > > > > - drm_WARN_ON(&dev_priv->drm, psr->enabled && > > !crtc_state->active_planes); > > + drm_WARN_ON(&dev_priv->drm, psr->enabled && > > + !new_crtc_state->active_planes); > > > > keep_disabled |= psr->sink_not_reliable; > > - keep_disabled |= !crtc_state->active_planes; > > + keep_disabled |= !new_crtc_state->active_planes; > > > > if (!psr->enabled && !keep_disabled) > > - intel_psr_enable_locked(intel_dp, > > crtc_state); > > + intel_psr_enable_locked(intel_dp, > > new_crtc_state); > > + > > + /* > > + * Wa_16013835468 > > + * Wa_14015648006 > > + */ > > + if (old_crtc_state->wm_level_disabled != > > + new_crtc_state->wm_level_disabled) > > + wa_16013835468(intel_dp, > > + new_crtc_state- > > >wm_level_disabled); > > > > /* Force a PSR exit when enabling CRC to avoid CRC > > timeouts */ > > - if (crtc_state->crc_enabled && psr->enabled) > > + if (new_crtc_state->crc_enabled && psr->enabled) > > psr_force_hw_tracking_exit(intel_dp); > > > > mutex_unlock(&psr->lock); > > @@ -1977,15 +1996,17 @@ static void > > _intel_psr_post_plane_update(const struct intel_atomic_state > > *state, > > void intel_psr_post_plane_update(const struct intel_atomic_state > > *state) > > { > > struct drm_i915_private *dev_priv = to_i915(state- > > >base.dev); > > - struct intel_crtc_state *crtc_state; > > + struct intel_crtc_state *new_crtc_state, *old_crtc_state; > > struct intel_crtc *crtc; > > int i; > > > > if (!HAS_PSR(dev_priv)) > > return; > > > > - for_each_new_intel_crtc_in_state(state, crtc, crtc_state, > > i) > > - _intel_psr_post_plane_update(state, crtc_state); > > + for_each_oldnew_intel_crtc_in_state(state, crtc, > > old_crtc_state, > > + new_crtc_state, i) > > + _intel_psr_post_plane_update(state, old_crtc_state, > > + new_crtc_state); > > } > > > > static int _psr2_ready_for_pipe_update_locked(struct intel_dp > > *intel_dp) > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c > > b/drivers/gpu/drm/i915/display/skl_watermark.c > > index 50a9a6adbe32..afb751c024ba 100644 > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > > @@ -2273,9 +2273,12 @@ static int skl_wm_check_vblank(struct > > intel_crtc_state *crtc_state) > > return level; > > > > /* > > - * FIXME PSR needs to toggle > > LATENCY_REPORTING_REMOVED_PIPE_* > > + * PSR needs to toggle LATENCY_REPORTING_REMOVED_PIPE_* > > * based on whether we're limited by the vblank duration. > > - * > > + */ > > + crtc_state->wm_level_disabled = level < i915- > > >display.wm.num_levels - 1; > > + > > + /* > > * FIXME also related to skl+ w/a 1136 (also unimplemented > > as of > > * now) perhaps? > > */ > > -- > > 2.34.1 >
On Tue, Mar 21, 2023 at 03:57:11PM +0000, Hogander, Jouni wrote: > Thank you Ville for checking my patch. Please see my responses below. > > On Tue, 2023-03-21 at 17:34 +0200, Ville Syrjälä wrote: > > On Mon, Mar 20, 2023 at 06:59:43PM +0200, Jouni Högander wrote: > > > PSR WM optimization should be disabled based on any wm level being > > > disabled. Currently it's disabled always when using delayed vblank. > > > Also same WA should be applied for ICL as well > > > > > > Bspec: 71580 > > > > > > v2: > > > - set/clear chicken bit in post_plane_update > > > - apply for ICL as well > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > --- > > > .../drm/i915/display/intel_display_types.h | 1 + > > > drivers/gpu/drm/i915/display/intel_psr.c | 67 ++++++++++++--- > > > ---- > > > drivers/gpu/drm/i915/display/skl_watermark.c | 7 +- > > > 3 files changed, 50 insertions(+), 25 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index c32bfba06ca1..60504c390408 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -1152,6 +1152,7 @@ struct intel_crtc_state { > > > bool has_psr2; > > > bool enable_psr2_sel_fetch; > > > bool req_psr2_sdp_prior_scanline; > > > + bool wm_level_disabled; > > > u32 dc3co_exitline; > > > u16 su_y_granularity; > > > struct drm_dp_vsc_sdp psr_vsc; > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 8dbf452d63c2..1050d777a108 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, > > > @@ -1362,7 +1350,7 @@ static void intel_psr_disable_locked(struct > > > intel_dp *intel_dp) > > > * Wa_14015648006 > > > */ > > > if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) || > > > - IS_DISPLAY_VER(dev_priv, 12, 13)) > > > + IS_DISPLAY_VER(dev_priv, 11, 13)) > > > intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, > > > wa_16013835468_bit_get(intel_dp), 0); > > > > > > @@ -1941,33 +1929,64 @@ void intel_psr_pre_plane_update(struct > > > intel_atomic_state *state, > > > } > > > } > > > > > > +/* > > > + * Wa_16013835468 > > > + * Wa_14015648006 > > > + */ > > > +static void wa_16013835468(struct intel_dp *intel_dp, bool > > > wm_level_disabled) > > > +{ > > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > + > > > + if (!IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) && > > > + !IS_DISPLAY_VER(dev_priv, 11, 13)) > > > + return; > > > > This is still confusing the two different workarounds, > > and actually losing the one about the delayed vblank. > > This is only one Wa. There are two lineage numbers describing same > workaround. > > Can you please point me out the delayed vblank wa description? I can't > find it from Bspec? Original implementation is referring lineages > 1601383546 and 14015648006. Neither of these are about delayed vblank > specifically. I thought I already copy pasted the quote from the spec in an earlier reply? 1601383546: "Display underrun when using delayed vblank with PSR2. Workaround: If using PSR2 on transcoder A with delayed vblank, set bit 0x46430[23]=0x1. If using PSR2 on transcoder B with delayed vblank, set bit 0x46430[24]=0x1." > > > > > > + > > > + if (intel_dp->psr.enabled && wm_level_disabled) > > > + 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) > > > + const struct > > > intel_crtc_state *old_crtc_state, > > > + const struct > > > intel_crtc_state *new_crtc_state) > > > > The rename reduces the signal-to-noise ratio near zero. > > Can you please elaborate this a bit more? What would you suggest? I > need to use both, old and new state to detect difference in > wm_level_disabled. I want to set/clear the chicken bit only when > necessary. I would either just always set/clear the chicken bit (dunno if there's any real point in optimizing it out), or split the rename to a separate patch. > > > > > > { > > > struct drm_i915_private *dev_priv = to_i915(state- > > > >base.dev); > > > struct intel_encoder *encoder; > > > > > > - if (!crtc_state->has_psr) > > > + if (!new_crtc_state->has_psr) > > > return; > > > > > > for_each_intel_encoder_mask_with_psr(state->base.dev, > > > encoder, > > > - crtc_state- > > > >uapi.encoder_mask) { > > > + new_crtc_state- > > > >uapi.encoder_mask) { > > > struct intel_dp *intel_dp = > > > enc_to_intel_dp(encoder); > > > struct intel_psr *psr = &intel_dp->psr; > > > bool keep_disabled = false; > > > > > > mutex_lock(&psr->lock); > > > > > > - drm_WARN_ON(&dev_priv->drm, psr->enabled && > > > !crtc_state->active_planes); > > > + drm_WARN_ON(&dev_priv->drm, psr->enabled && > > > + !new_crtc_state->active_planes); > > > > > > keep_disabled |= psr->sink_not_reliable; > > > - keep_disabled |= !crtc_state->active_planes; > > > + keep_disabled |= !new_crtc_state->active_planes; > > > > > > if (!psr->enabled && !keep_disabled) > > > - intel_psr_enable_locked(intel_dp, > > > crtc_state); > > > + intel_psr_enable_locked(intel_dp, > > > new_crtc_state); > > > + > > > + /* > > > + * Wa_16013835468 > > > + * Wa_14015648006 > > > + */ > > > + if (old_crtc_state->wm_level_disabled != > > > + new_crtc_state->wm_level_disabled) > > > + wa_16013835468(intel_dp, > > > + new_crtc_state- > > > >wm_level_disabled); > > > > > > /* Force a PSR exit when enabling CRC to avoid CRC > > > timeouts */ > > > - if (crtc_state->crc_enabled && psr->enabled) > > > + if (new_crtc_state->crc_enabled && psr->enabled) > > > psr_force_hw_tracking_exit(intel_dp); > > > > > > mutex_unlock(&psr->lock); > > > @@ -1977,15 +1996,17 @@ static void > > > _intel_psr_post_plane_update(const struct intel_atomic_state > > > *state, > > > void intel_psr_post_plane_update(const struct intel_atomic_state > > > *state) > > > { > > > struct drm_i915_private *dev_priv = to_i915(state- > > > >base.dev); > > > - struct intel_crtc_state *crtc_state; > > > + struct intel_crtc_state *new_crtc_state, *old_crtc_state; > > > struct intel_crtc *crtc; > > > int i; > > > > > > if (!HAS_PSR(dev_priv)) > > > return; > > > > > > - for_each_new_intel_crtc_in_state(state, crtc, crtc_state, > > > i) > > > - _intel_psr_post_plane_update(state, crtc_state); > > > + for_each_oldnew_intel_crtc_in_state(state, crtc, > > > old_crtc_state, > > > + new_crtc_state, i) > > > + _intel_psr_post_plane_update(state, old_crtc_state, > > > + new_crtc_state); > > > } > > > > > > static int _psr2_ready_for_pipe_update_locked(struct intel_dp > > > *intel_dp) > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c > > > b/drivers/gpu/drm/i915/display/skl_watermark.c > > > index 50a9a6adbe32..afb751c024ba 100644 > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > > > @@ -2273,9 +2273,12 @@ static int skl_wm_check_vblank(struct > > > intel_crtc_state *crtc_state) > > > return level; > > > > > > /* > > > - * FIXME PSR needs to toggle > > > LATENCY_REPORTING_REMOVED_PIPE_* > > > + * PSR needs to toggle LATENCY_REPORTING_REMOVED_PIPE_* > > > * based on whether we're limited by the vblank duration. > > > - * > > > + */ > > > + crtc_state->wm_level_disabled = level < i915- > > > >display.wm.num_levels - 1; > > > + > > > + /* > > > * FIXME also related to skl+ w/a 1136 (also unimplemented > > > as of > > > * now) perhaps? > > > */ > > > -- > > > 2.34.1 > > >
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index c32bfba06ca1..60504c390408 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1152,6 +1152,7 @@ struct intel_crtc_state { bool has_psr2; bool enable_psr2_sel_fetch; bool req_psr2_sdp_prior_scanline; + bool wm_level_disabled; u32 dc3co_exitline; u16 su_y_granularity; struct drm_dp_vsc_sdp psr_vsc; diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index 8dbf452d63c2..1050d777a108 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, @@ -1362,7 +1350,7 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp) * Wa_14015648006 */ if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) || - IS_DISPLAY_VER(dev_priv, 12, 13)) + IS_DISPLAY_VER(dev_priv, 11, 13)) intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, wa_16013835468_bit_get(intel_dp), 0); @@ -1941,33 +1929,64 @@ void intel_psr_pre_plane_update(struct intel_atomic_state *state, } } +/* + * Wa_16013835468 + * Wa_14015648006 + */ +static void wa_16013835468(struct intel_dp *intel_dp, bool wm_level_disabled) +{ + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); + + if (!IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) && + !IS_DISPLAY_VER(dev_priv, 11, 13)) + return; + + if (intel_dp->psr.enabled && wm_level_disabled) + 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) + const struct intel_crtc_state *old_crtc_state, + const struct intel_crtc_state *new_crtc_state) { struct drm_i915_private *dev_priv = to_i915(state->base.dev); struct intel_encoder *encoder; - if (!crtc_state->has_psr) + if (!new_crtc_state->has_psr) return; for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, - crtc_state->uapi.encoder_mask) { + new_crtc_state->uapi.encoder_mask) { struct intel_dp *intel_dp = enc_to_intel_dp(encoder); struct intel_psr *psr = &intel_dp->psr; bool keep_disabled = false; mutex_lock(&psr->lock); - drm_WARN_ON(&dev_priv->drm, psr->enabled && !crtc_state->active_planes); + drm_WARN_ON(&dev_priv->drm, psr->enabled && + !new_crtc_state->active_planes); keep_disabled |= psr->sink_not_reliable; - keep_disabled |= !crtc_state->active_planes; + keep_disabled |= !new_crtc_state->active_planes; if (!psr->enabled && !keep_disabled) - intel_psr_enable_locked(intel_dp, crtc_state); + intel_psr_enable_locked(intel_dp, new_crtc_state); + + /* + * Wa_16013835468 + * Wa_14015648006 + */ + if (old_crtc_state->wm_level_disabled != + new_crtc_state->wm_level_disabled) + wa_16013835468(intel_dp, + new_crtc_state->wm_level_disabled); /* Force a PSR exit when enabling CRC to avoid CRC timeouts */ - if (crtc_state->crc_enabled && psr->enabled) + if (new_crtc_state->crc_enabled && psr->enabled) psr_force_hw_tracking_exit(intel_dp); mutex_unlock(&psr->lock); @@ -1977,15 +1996,17 @@ static void _intel_psr_post_plane_update(const struct intel_atomic_state *state, void intel_psr_post_plane_update(const struct intel_atomic_state *state) { struct drm_i915_private *dev_priv = to_i915(state->base.dev); - struct intel_crtc_state *crtc_state; + struct intel_crtc_state *new_crtc_state, *old_crtc_state; struct intel_crtc *crtc; int i; if (!HAS_PSR(dev_priv)) return; - for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) - _intel_psr_post_plane_update(state, crtc_state); + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, + new_crtc_state, i) + _intel_psr_post_plane_update(state, old_crtc_state, + new_crtc_state); } static int _psr2_ready_for_pipe_update_locked(struct intel_dp *intel_dp) diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c index 50a9a6adbe32..afb751c024ba 100644 --- a/drivers/gpu/drm/i915/display/skl_watermark.c +++ b/drivers/gpu/drm/i915/display/skl_watermark.c @@ -2273,9 +2273,12 @@ static int skl_wm_check_vblank(struct intel_crtc_state *crtc_state) return level; /* - * FIXME PSR needs to toggle LATENCY_REPORTING_REMOVED_PIPE_* + * PSR needs to toggle LATENCY_REPORTING_REMOVED_PIPE_* * based on whether we're limited by the vblank duration. - * + */ + crtc_state->wm_level_disabled = level < i915->display.wm.num_levels - 1; + + /* * FIXME also related to skl+ w/a 1136 (also unimplemented as of * now) perhaps? */
PSR WM optimization should be disabled based on any wm level being disabled. Currently it's disabled always when using delayed vblank. Also same WA should be applied for ICL as well Bspec: 71580 v2: - set/clear chicken bit in post_plane_update - apply for ICL as well Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- .../drm/i915/display/intel_display_types.h | 1 + drivers/gpu/drm/i915/display/intel_psr.c | 67 ++++++++++++------- drivers/gpu/drm/i915/display/skl_watermark.c | 7 +- 3 files changed, 50 insertions(+), 25 deletions(-)