| Message ID | 20230314090142.947764-2-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | High refresh rate PSR fixes | expand |
On Tue, Mar 14, 2023 at 11:01:41AM +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. > > Bspec: 71580 > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > drivers/gpu/drm/i915/display/intel_psr.c | 12 +++++------- > drivers/gpu/drm/i915/display/skl_watermark.c | 7 +++++-- > 3 files changed, 11 insertions(+), 9 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 44610b20cd29..a6edd65b8edb 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -1177,13 +1177,11 @@ static void intel_psr_enable_source(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 (crtc_state->hw.adjusted_mode.crtc_vblank_start != > - crtc_state->hw.adjusted_mode.crtc_vdisplay) That looks like something we want to keep. The delayed vblank w/a is still supposedly necessary. > - intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, 0, > - wa_16013835468_bit_get(intel_dp)); > - } > + if ((IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) || > + IS_DISPLAY_VER(dev_priv, 12, 13)) && I think we need this for all KBL+. > + crtc_state->wm_level_disabled) > + 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) > 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? > */ And that is more or less corresponding w/a for SKL/BXT that did not yet have these chicken bits. > -- > 2.34.1
On Thu, 2023-03-16 at 20:21 +0200, Ville Syrjälä wrote: > On Tue, Mar 14, 2023 at 11:01:41AM +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. > > > > Bspec: 71580 > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > > drivers/gpu/drm/i915/display/intel_psr.c | 12 +++++----- > > -- > > drivers/gpu/drm/i915/display/skl_watermark.c | 7 +++++-- > > 3 files changed, 11 insertions(+), 9 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 44610b20cd29..a6edd65b8edb 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -1177,13 +1177,11 @@ static void intel_psr_enable_source(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 (crtc_state->hw.adjusted_mode.crtc_vblank_start > > != > > - crtc_state->hw.adjusted_mode.crtc_vdisplay) > > That looks like something we want to keep. The delayed vblank w/a is > still supposedly necessary. 16013835468 and 14015648006 are specifically mentioning "low power watermark (level 1 and up) is disabled" I haven't seen or couldn't find any older Wa speaking generally about delayed vblank. > > > - intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, > > 0, > > - > > wa_16013835468_bit_get(intel_dp)); > > - } > > + if ((IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) || > > + IS_DISPLAY_VER(dev_priv, 12, 13)) && > > I think we need this for all KBL+. Do you have Wa lineage for a reference? > > > + crtc_state->wm_level_disabled) > > + 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) > > 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? > > */ > > And that is more or less corresponding w/a for SKL/BXT that did not > yet have > these chicken bits. Ok, I will check this as well. > > > -- > > 2.34.1 >
On Fri, Mar 17, 2023 at 06:54:02AM +0000, Hogander, Jouni wrote: > On Thu, 2023-03-16 at 20:21 +0200, Ville Syrjälä wrote: > > On Tue, Mar 14, 2023 at 11:01:41AM +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. > > > > > > Bspec: 71580 > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > > > drivers/gpu/drm/i915/display/intel_psr.c | 12 +++++----- > > > -- > > > drivers/gpu/drm/i915/display/skl_watermark.c | 7 +++++-- > > > 3 files changed, 11 insertions(+), 9 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 44610b20cd29..a6edd65b8edb 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -1177,13 +1177,11 @@ static void intel_psr_enable_source(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 (crtc_state->hw.adjusted_mode.crtc_vblank_start > > > != > > > - crtc_state->hw.adjusted_mode.crtc_vdisplay) > > > > That looks like something we want to keep. The delayed vblank w/a is > > still supposedly necessary. > > 16013835468 and 14015648006 are specifically mentioning "low power > watermark (level 1 and up) is disabled" I haven't seen or couldn't find > any older Wa speaking generally about delayed vblank. 14015648006: "High refresh rate panels with small vblank size (either because of the panel vblank size or the internal delayed vblank) must have some watermark levels disabled..." -> that's the w/a you're now implementing, so the comment is lying to us by claiming it was already implemented 16013835468: "Display underrun when using delayed vblank with PSR2..." -> that's the one we actually already had implemented > > > > > > - intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, > > > 0, > > > - > > > wa_16013835468_bit_get(intel_dp)); > > > - } > > > + if ((IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) || > > > + IS_DISPLAY_VER(dev_priv, 12, 13)) && > > > > I think we need this for all KBL+. > > Do you have Wa lineage for a reference? I think it was part of the w/a 1136. You see it only lists skl/bxt/kbl a-b to need the full psr disable, leaving kbl c+ to do something different. And the latency reporting chicken bits were added exactly for kbl c+. But yeah, the way this is now documented in bpsec is very poor. And sadly the older hsds have now disappread into bit heaven so we can't double check the full details there. But I'm 99% sure I read through those hsds in the past and came to this same conclusion back then. > > > > > > + crtc_state->wm_level_disabled) > > > + 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) > > > 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? > > > */ > > > > And that is more or less corresponding w/a for SKL/BXT that did not > > yet have > > these chicken bits. > > Ok, I will check this as well. > > > > > > -- > > > 2.34.1 > > >
On Fri, 2023-03-17 at 14:33 +0200, Ville Syrjälä wrote: > On Fri, Mar 17, 2023 at 06:54:02AM +0000, Hogander, Jouni wrote: > > On Thu, 2023-03-16 at 20:21 +0200, Ville Syrjälä wrote: > > > On Tue, Mar 14, 2023 at 11:01:41AM +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. > > > > > > > > Bspec: 71580 > > > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > > > > drivers/gpu/drm/i915/display/intel_psr.c | 12 +++++- > > > > ---- > > > > -- > > > > drivers/gpu/drm/i915/display/skl_watermark.c | 7 +++++- > > > > - > > > > 3 files changed, 11 insertions(+), 9 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 44610b20cd29..a6edd65b8edb 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > @@ -1177,13 +1177,11 @@ static void > > > > intel_psr_enable_source(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 (crtc_state- > > > > >hw.adjusted_mode.crtc_vblank_start > > > > != > > > > - crtc_state->hw.adjusted_mode.crtc_vdisplay) > > > > > > That looks like something we want to keep. The delayed vblank w/a > > > is > > > still supposedly necessary. > > > > 16013835468 and 14015648006 are specifically mentioning "low power > > watermark (level 1 and up) is disabled" I haven't seen or couldn't > > find > > any older Wa speaking generally about delayed vblank. > > 14015648006: > "High refresh rate panels with small vblank size (either because of > the > panel vblank size or the internal delayed vblank) must have some > watermark levels disabled..." > -> that's the w/a you're now implementing, so the comment is > lying to us by claiming it was already implemented > > 16013835468: > "Display underrun when using delayed vblank with PSR2..." > -> that's the one we actually already had implemented > > > > > > > > > > - intel_de_rmw(dev_priv, > > > > GEN8_CHICKEN_DCPR_1, > > > > 0, > > > > - > > > > wa_16013835468_bit_get(intel_dp)); > > > > - } > > > > + if ((IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) || > > > > + IS_DISPLAY_VER(dev_priv, 12, 13)) && > > > > > > I think we need this for all KBL+. > > > > Do you have Wa lineage for a reference? > > I think it was part of the w/a 1136. You see it only lists > skl/bxt/kbl a-b to need the full psr disable, leaving kbl c+ > to do something different. And the latency reporting chicken > bits were added exactly for kbl c+. > > But yeah, the way this is now documented in bpsec is very poor. > And sadly the older hsds have now disappread into bit heaven > so we can't double check the full details there. But I'm 99% > sure I read through those hsds in the past and came to this > same conclusion back then. I just sent new version of the set. I.e. disabling PSR for < 12 and using chicken bit for >= 12 if any wm level is disabled. That is at least safe bet... Can you please check the set if that is ok to you? : https://patchwork.freedesktop.org/series/115109/ > > > > > > > > > > + crtc_state->wm_level_disabled) > > > > + 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) > > > > 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? > > > > */ > > > > > > And that is more or less corresponding w/a for SKL/BXT that did > > > not > > > yet have > > > these chicken bits. > > > > Ok, I will check this as well. > > > > > > > > > -- > > > > 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 44610b20cd29..a6edd65b8edb 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -1177,13 +1177,11 @@ static void intel_psr_enable_source(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 (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 ((IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) || + IS_DISPLAY_VER(dev_priv, 12, 13)) && + crtc_state->wm_level_disabled) + 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) 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. Bspec: 71580 Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- drivers/gpu/drm/i915/display/intel_display_types.h | 1 + drivers/gpu/drm/i915/display/intel_psr.c | 12 +++++------- drivers/gpu/drm/i915/display/skl_watermark.c | 7 +++++-- 3 files changed, 11 insertions(+), 9 deletions(-)