| Message ID | 20240909063218.447934-1-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/psr: Implment WA to help reach PC10 | expand |
> -----Original Message----- > From: Kandpal, Suraj <suraj.kandpal@intel.com> > Sent: Monday, September 9, 2024 12:02 PM > To: intel-gfx@lists.freedesktop.org > Cc: Shankar, Uma <uma.shankar@intel.com>; Hogander, Jouni > <jouni.hogander@intel.com>; Deak, Imre <imre.deak@intel.com>; Kandpal, Suraj > <suraj.kandpal@intel.com> > Subject: [PATCH] drm/i915/psr: Implment WA to help reach PC10 Not: Typo in implement > To reach PC10 when PKG_C_LATENCY is configure we must do the following > things > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be entered > 2) Allow PSR2 deep sleep when DC5 can be entered > 3) DC5 can be entered when all transocoder have either PSR1, PSR2 or eDP 1.5 PR > ALPM enabled and VBI is disabled and flips and pushes are not happening. > > --v2 > -Switch condition and do an early return [Jani] -Do some checks in > compute_config [Jani] -Do not use register reads as a method of checking states > for DPKGC or delayed vblank [Jani] -Use another way to see is vblank interrupts > are disabled or not [Jani] > > --v3 > -Use has_psr to check if psr can be enabled or not for dc5_entry cond [Uma] - > Move the dc5 entry computation to psr_compute_config [Jouni] -No need to > change sequence of enabled and activate, so dont make hsw_psr1_activate > return anything [Jouni] -Use has_psr to stop psr1 activation [Jouni] -Use lineage > no. in WA -Add the display ver restrictions for WA > > --v4 > -use more appropriate name for check_vblank_limit() [Jouni] -Cover the case for > idle frames when dpkgc is not configured [Jouni] -Check psr only for edp [Jouni] > > --v5 > -move psr1 handling to plane update [Jouni] -add todo for cases when vblank is > enabled when psr enabled [Jouni] -use intel_display instead of drm_i915_private > > --v6 > -check target_dc_state [Jouni] > -fix condition in pre/post plane update [Jouni] > > WA: 22019444797 > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > .../drm/i915/display/intel_display_types.h | 3 + > drivers/gpu/drm/i915/display/intel_psr.c | 112 +++++++++++++++++- > 2 files changed, 114 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > b/drivers/gpu/drm/i915/display/intel_display_types.h > index 733de5edcfdb..59c81f0a3abd 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -1577,6 +1577,9 @@ struct intel_psr { > #define I915_PSR_DEBUG_PANEL_REPLAY_DISABLE 0x40 > > u32 debug; > + bool is_dpkgc_configured; > + bool is_dc5_entry_possible; > + bool is_wa_delayed_vblank_limit; > bool sink_support; > bool source_support; > bool enabled; > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > b/drivers/gpu/drm/i915/display/intel_psr.c > index b30fa067ce6e..e50b476494a0 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -26,6 +26,7 @@ > #include <drm/drm_atomic_helper.h> > #include <drm/drm_damage_helper.h> > #include <drm/drm_debugfs.h> > +#include <drm/drm_vblank.h> > > #include "i915_drv.h" > #include "i915_reg.h" > @@ -874,6 +875,78 @@ static u8 psr_compute_idle_frames(struct intel_dp > *intel_dp) > return idle_frames; > } > > +static bool > +intel_psr_check_wa_delayed_vblank(const struct drm_display_mode > +*adjusted_mode) { > + return (adjusted_mode->crtc_vblank_start - > +adjusted_mode->crtc_vdisplay) >= 6; } > + > +/* > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and > + * VRR is not enabled > + */ > +static bool intel_psr_is_dpkgc_configured(struct intel_display *display, > + struct intel_atomic_state *state) { > + struct intel_crtc *intel_crtc; > + struct intel_crtc_state *crtc_state; > + int i; > + > + if (DISPLAY_VER(display) < 20) > + return false; > + > + for_each_new_intel_crtc_in_state(state, intel_crtc, crtc_state, i) { > + if (!intel_crtc->active) > + continue; > + > + if (crtc_state->vrr.enable) > + return false; > + } > + > + return true; > +} > + > +/* > + * DC5 entry is only possible if vblank interrupt is disabled > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all > + * enabled encoders. > + */ > +static bool > +intel_psr_is_dc5_entry_possible(struct intel_display *display, > + struct intel_atomic_state *state) > +{ > + struct intel_crtc *intel_crtc; > + struct intel_crtc_state *crtc_state; > + int i; > + > + if ((display->power.domains.target_dc_state & > + DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0) > + return false; > + > + for_each_new_intel_crtc_in_state(state, intel_crtc, crtc_state, i) { > + struct drm_crtc *crtc = &intel_crtc->base; > + struct drm_vblank_crtc *vblank; > + struct intel_encoder *encoder; > + > + if (!intel_crtc->active) > + continue; > + > + vblank = drm_crtc_vblank_crtc(crtc); > + > + if (vblank->enabled) > + return false; > + > + if (crtc_state->has_psr) > + return false; It should be !has_psr > + > + for_each_encoder_on_crtc(display->drm, crtc, encoder) > + if (encoder->type != INTEL_OUTPUT_EDP) > + return false; > + } > + > + return true; > +} > + > static void hsw_activate_psr1(struct intel_dp *intel_dp) { > struct intel_display *display = to_intel_display(intel_dp); @@ -986,7 > +1059,15 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp) > u32 val = EDP_PSR2_ENABLE; > u32 psr_val = 0; > > - val |= EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > + /* > + * Wa_22019444797 > + * TODO: Disable idle frames when vblank gets enabled while > + * PSR2 is enabled > + */ > + if (DISPLAY_VER(dev_priv) != 20 || > + !intel_dp->psr.is_dpkgc_configured || Why ! for dpkgc, Here this can be enabled if dpkgc_configured right ? > + intel_dp->psr.is_dc5_entry_possible) > + val |= > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > > if (DISPLAY_VER(display) < 14 && !IS_ALDERLAKE_P(dev_priv)) > val |= EDP_SU_TRACK_ENABLE; > @@ -2667,10 +2748,20 @@ void intel_psr_pre_plane_update(struct > intel_atomic_state *state, > const struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > struct intel_encoder *encoder; > + bool dpkgc_configured = false, dc5_entry_possible = false; > + bool wa_delayed_vblank_limit = false; > > if (!HAS_PSR(display)) > return; > > + if (DISPLAY_VER(display) == 20) { > + dpkgc_configured = intel_psr_is_dpkgc_configured(display, > state); > + dc5_entry_possible = > + intel_psr_is_dc5_entry_possible(display, state); > + wa_delayed_vblank_limit = > + intel_psr_check_wa_delayed_vblank(&new_crtc_state- > >hw.adjusted_mode); > + } > + > for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, > old_crtc_state->uapi.encoder_mask) > { > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); @@ - > 2679,6 +2770,12 @@ void intel_psr_pre_plane_update(struct intel_atomic_state > *state, > > mutex_lock(&psr->lock); > > + if (DISPLAY_VER(i915) == 20) { > + psr->is_dpkgc_configured = dpkgc_configured; > + psr->is_dc5_entry_possible = dc5_entry_possible; > + psr->is_wa_delayed_vblank_limit = > wa_delayed_vblank_limit; We can drop the variables and directly assign this to psr->... and use it subsequently. Also it would be good to have this done in compute and than just use it here. > + } > + > /* > * Reasons to disable: > * - PSR disabled in new state > @@ -2686,6 +2783,7 @@ void intel_psr_pre_plane_update(struct > intel_atomic_state *state, > * - Changing between PSR versions > * - Region Early Transport changing > * - Display WA #1136: skl, bxt > + * - Display WA_22019444797 > */ > needs_to_disable |= > intel_crtc_needs_modeset(new_crtc_state); > needs_to_disable |= !new_crtc_state->has_psr; @@ -2695,6 > +2793,10 @@ void intel_psr_pre_plane_update(struct intel_atomic_state *state, > psr->su_region_et_enabled; > needs_to_disable |= DISPLAY_VER(i915) < 11 && > new_crtc_state->wm_level_disabled; > + /* TODO: Disable PSR1 when vblank gets enabled while PSR1 is > enabled */ > + needs_to_disable |= DISPLAY_VER(display) == 20 && > dpkgc_configured && > + (wa_delayed_vblank_limit || dc5_entry_possible) && > + !new_crtc_state->has_sel_update && > +!new_crtc_state->has_panel_replay; Good to move this to a small helper function which can be extended if required as well. Also seems used in post_plane as well. @Hogander, Jouni Can you also ack if this handling for PSR is ok. > if (psr->enabled && needs_to_disable) > intel_psr_disable_locked(intel_dp); > @@ -2735,6 +2837,14 @@ void intel_psr_post_plane_update(struct > intel_atomic_state *state, > keep_disabled |= DISPLAY_VER(display) < 11 && > crtc_state->wm_level_disabled; > > + /* > + * Wa_22019444797 > + * TODO: Disable PSR1 when vblank gets enabled while PSR1 is > enabled > + */ > + keep_disabled |= DISPLAY_VER(display) == 20 && psr- > >is_dpkgc_configured && > + (psr->is_wa_delayed_vblank_limit || psr- > >is_dc5_entry_possible) && > + !crtc_state->has_sel_update && !crtc_state- > >has_panel_replay; > + > if (!psr->enabled && !keep_disabled) > intel_psr_enable_locked(intel_dp, crtc_state); > else if (psr->enabled && !crtc_state->wm_level_disabled) > -- > 2.43.2
> -----Original Message----- > From: Shankar, Uma <uma.shankar@intel.com> > Sent: Thursday, September 19, 2024 5:44 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel- > gfx@lists.freedesktop.org; Hogander, Jouni <jouni.hogander@intel.com> > Cc: Deak, Imre <imre.deak@intel.com> > Subject: RE: [PATCH] drm/i915/psr: Implment WA to help reach PC10 > > > > > -----Original Message----- > > From: Kandpal, Suraj <suraj.kandpal@intel.com> > > Sent: Monday, September 9, 2024 12:02 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Shankar, Uma <uma.shankar@intel.com>; Hogander, Jouni > > <jouni.hogander@intel.com>; Deak, Imre <imre.deak@intel.com>; > Kandpal, > > Suraj <suraj.kandpal@intel.com> > > Subject: [PATCH] drm/i915/psr: Implment WA to help reach PC10 > > Not: Typo in implement Will fix in next revision > > > To reach PC10 when PKG_C_LATENCY is configure we must do the > following > > things > > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be > > entered > > 2) Allow PSR2 deep sleep when DC5 can be entered > > 3) DC5 can be entered when all transocoder have either PSR1, PSR2 or > > eDP 1.5 PR ALPM enabled and VBI is disabled and flips and pushes are not > happening. > > > > --v2 > > -Switch condition and do an early return [Jani] -Do some checks in > > compute_config [Jani] -Do not use register reads as a method of > > checking states for DPKGC or delayed vblank [Jani] -Use another way to > > see is vblank interrupts are disabled or not [Jani] > > > > --v3 > > -Use has_psr to check if psr can be enabled or not for dc5_entry cond > > [Uma] - Move the dc5 entry computation to psr_compute_config [Jouni] > > -No need to change sequence of enabled and activate, so dont make > > hsw_psr1_activate return anything [Jouni] -Use has_psr to stop psr1 > > activation [Jouni] -Use lineage no. in WA -Add the display ver > > restrictions for WA > > > > --v4 > > -use more appropriate name for check_vblank_limit() [Jouni] -Cover the > > case for idle frames when dpkgc is not configured [Jouni] -Check psr > > only for edp [Jouni] > > > > --v5 > > -move psr1 handling to plane update [Jouni] -add todo for cases when > > vblank is enabled when psr enabled [Jouni] -use intel_display instead > > of drm_i915_private > > > > --v6 > > -check target_dc_state [Jouni] > > -fix condition in pre/post plane update [Jouni] > > > > WA: 22019444797 > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > .../drm/i915/display/intel_display_types.h | 3 + > > drivers/gpu/drm/i915/display/intel_psr.c | 112 +++++++++++++++++- > > 2 files changed, 114 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index 733de5edcfdb..59c81f0a3abd 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1577,6 +1577,9 @@ struct intel_psr { > > #define I915_PSR_DEBUG_PANEL_REPLAY_DISABLE 0x40 > > > > u32 debug; > > + bool is_dpkgc_configured; > > + bool is_dc5_entry_possible; > > + bool is_wa_delayed_vblank_limit; > > bool sink_support; > > bool source_support; > > bool enabled; > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index b30fa067ce6e..e50b476494a0 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -26,6 +26,7 @@ > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_damage_helper.h> > > #include <drm/drm_debugfs.h> > > +#include <drm/drm_vblank.h> > > > > #include "i915_drv.h" > > #include "i915_reg.h" > > @@ -874,6 +875,78 @@ static u8 psr_compute_idle_frames(struct > intel_dp > > *intel_dp) > > return idle_frames; > > } > > > > +static bool > > +intel_psr_check_wa_delayed_vblank(const struct drm_display_mode > > +*adjusted_mode) { > > + return (adjusted_mode->crtc_vblank_start - > > +adjusted_mode->crtc_vdisplay) >= 6; } > > + > > +/* > > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and > > + * VRR is not enabled > > + */ > > +static bool intel_psr_is_dpkgc_configured(struct intel_display *display, > > + struct intel_atomic_state *state) { > > + struct intel_crtc *intel_crtc; > > + struct intel_crtc_state *crtc_state; > > + int i; > > + > > + if (DISPLAY_VER(display) < 20) > > + return false; > > + > > + for_each_new_intel_crtc_in_state(state, intel_crtc, crtc_state, i) { > > + if (!intel_crtc->active) > > + continue; > > + > > + if (crtc_state->vrr.enable) > > + return false; > > + } > > + > > + return true; > > +} > > + > > +/* > > + * DC5 entry is only possible if vblank interrupt is disabled > > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all > > + * enabled encoders. > > + */ > > +static bool > > +intel_psr_is_dc5_entry_possible(struct intel_display *display, > > + struct intel_atomic_state *state) { > > + struct intel_crtc *intel_crtc; > > + struct intel_crtc_state *crtc_state; > > + int i; > > + > > + if ((display->power.domains.target_dc_state & > > + DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0) > > + return false; > > + > > + for_each_new_intel_crtc_in_state(state, intel_crtc, crtc_state, i) { > > + struct drm_crtc *crtc = &intel_crtc->base; > > + struct drm_vblank_crtc *vblank; > > + struct intel_encoder *encoder; > > + > > + if (!intel_crtc->active) > > + continue; > > + > > + vblank = drm_crtc_vblank_crtc(crtc); > > + > > + if (vblank->enabled) > > + return false; > > + > > + if (crtc_state->has_psr) > > + return false; > > It should be !has_psr > > > + > > + for_each_encoder_on_crtc(display->drm, crtc, encoder) > > + if (encoder->type != INTEL_OUTPUT_EDP) > > + return false; > > + } > > + > > + return true; > > +} > > + > > static void hsw_activate_psr1(struct intel_dp *intel_dp) { > > struct intel_display *display = to_intel_display(intel_dp); @@ > > -986,7 > > +1059,15 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp) > > u32 val = EDP_PSR2_ENABLE; > > u32 psr_val = 0; > > > > - val |= EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > > + /* > > + * Wa_22019444797 > > + * TODO: Disable idle frames when vblank gets enabled while > > + * PSR2 is enabled > > + */ > > + if (DISPLAY_VER(dev_priv) != 20 || > > + !intel_dp->psr.is_dpkgc_configured || > > Why ! for dpkgc, Here this can be enabled if dpkgc_configured right ? > So we want idle frames to be written in these scenarios display ver !=20 if it is equal to 20 Then we write idle frames if dpkgc is not configured and if dpkgc is confirgured then we write Idle frames if dc5 entry is possible > > + intel_dp->psr.is_dc5_entry_possible) > > + val |= > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > > > > if (DISPLAY_VER(display) < 14 && !IS_ALDERLAKE_P(dev_priv)) > > val |= EDP_SU_TRACK_ENABLE; > > @@ -2667,10 +2748,20 @@ void intel_psr_pre_plane_update(struct > > intel_atomic_state *state, > > const struct intel_crtc_state *new_crtc_state = > > intel_atomic_get_new_crtc_state(state, crtc); > > struct intel_encoder *encoder; > > + bool dpkgc_configured = false, dc5_entry_possible = false; > > + bool wa_delayed_vblank_limit = false; > > > > if (!HAS_PSR(display)) > > return; > > > > + if (DISPLAY_VER(display) == 20) { > > + dpkgc_configured = intel_psr_is_dpkgc_configured(display, > > state); > > + dc5_entry_possible = > > + intel_psr_is_dc5_entry_possible(display, state); > > + wa_delayed_vblank_limit = > > + > intel_psr_check_wa_delayed_vblank(&new_crtc_state- > > >hw.adjusted_mode); > > + } > > + > > for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, > > old_crtc_state- > >uapi.encoder_mask) > > { > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); @@ - > > 2679,6 +2770,12 @@ void intel_psr_pre_plane_update(struct > > intel_atomic_state *state, > > > > mutex_lock(&psr->lock); > > > > + if (DISPLAY_VER(i915) == 20) { > > + psr->is_dpkgc_configured = dpkgc_configured; > > + psr->is_dc5_entry_possible = dc5_entry_possible; > > + psr->is_wa_delayed_vblank_limit = > > wa_delayed_vblank_limit; > > We can drop the variables and directly assign this to psr->... and use it > subsequently. So the reason I had assigned this to variables is I did not want to call the 3 functions Above inside the for_each_encoder loop since the functions themselves have loops inside them Which would just increase the cost hence the variables. > Also it would be good to have this done in compute and than just use it > here. It was done in compute a few revisions ago @Hogander, Jouni suggested to move it to pre_plane_update. > > > + } > > + > > /* > > * Reasons to disable: > > * - PSR disabled in new state > > @@ -2686,6 +2783,7 @@ void intel_psr_pre_plane_update(struct > > intel_atomic_state *state, > > * - Changing between PSR versions > > * - Region Early Transport changing > > * - Display WA #1136: skl, bxt > > + * - Display WA_22019444797 > > */ > > needs_to_disable |= > > intel_crtc_needs_modeset(new_crtc_state); > > needs_to_disable |= !new_crtc_state->has_psr; @@ -2695,6 > > +2793,10 @@ void intel_psr_pre_plane_update(struct intel_atomic_state > > +*state, > > psr->su_region_et_enabled; > > needs_to_disable |= DISPLAY_VER(i915) < 11 && > > new_crtc_state->wm_level_disabled; > > + /* TODO: Disable PSR1 when vblank gets enabled while PSR1 > is > > enabled */ > > + needs_to_disable |= DISPLAY_VER(display) == 20 && > > dpkgc_configured && > > + (wa_delayed_vblank_limit || dc5_entry_possible) > && > > + !new_crtc_state->has_sel_update && > > +!new_crtc_state->has_panel_replay; > > Good to move this to a small helper function which can be extended if > required as well. > Also seems used in post_plane as well. Sure will move it into its own helper function Regards, Suraj Kandpal > > @Hogander, Jouni Can you also ack if this handling for PSR is ok. > > > if (psr->enabled && needs_to_disable) > > intel_psr_disable_locked(intel_dp); > > @@ -2735,6 +2837,14 @@ void intel_psr_post_plane_update(struct > > intel_atomic_state *state, > > keep_disabled |= DISPLAY_VER(display) < 11 && > > crtc_state->wm_level_disabled; > > > > + /* > > + * Wa_22019444797 > > + * TODO: Disable PSR1 when vblank gets enabled while PSR1 > is > > enabled > > + */ > > + keep_disabled |= DISPLAY_VER(display) == 20 && psr- > > >is_dpkgc_configured && > > + (psr->is_wa_delayed_vblank_limit || psr- > > >is_dc5_entry_possible) && > > + !crtc_state->has_sel_update && !crtc_state- > > >has_panel_replay; > > + > > if (!psr->enabled && !keep_disabled) > > intel_psr_enable_locked(intel_dp, crtc_state); > > else if (psr->enabled && !crtc_state->wm_level_disabled) > > -- > > 2.43.2
On Thu, 2024-09-19 at 12:44 +0000, Kandpal, Suraj wrote: > > > > -----Original Message----- > > From: Shankar, Uma <uma.shankar@intel.com> > > Sent: Thursday, September 19, 2024 5:44 PM > > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel- > > gfx@lists.freedesktop.org; Hogander, Jouni > > <jouni.hogander@intel.com> > > Cc: Deak, Imre <imre.deak@intel.com> > > Subject: RE: [PATCH] drm/i915/psr: Implment WA to help reach PC10 > > > > > > > > > -----Original Message----- > > > From: Kandpal, Suraj <suraj.kandpal@intel.com> > > > Sent: Monday, September 9, 2024 12:02 PM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Shankar, Uma <uma.shankar@intel.com>; Hogander, Jouni > > > <jouni.hogander@intel.com>; Deak, Imre <imre.deak@intel.com>; > > Kandpal, > > > Suraj <suraj.kandpal@intel.com> > > > Subject: [PATCH] drm/i915/psr: Implment WA to help reach PC10 > > > > Not: Typo in implement > > Will fix in next revision > > > > > > To reach PC10 when PKG_C_LATENCY is configure we must do the > > following > > > things > > > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be > > > entered > > > 2) Allow PSR2 deep sleep when DC5 can be entered > > > 3) DC5 can be entered when all transocoder have either PSR1, PSR2 > > > or > > > eDP 1.5 PR ALPM enabled and VBI is disabled and flips and pushes > > > are not > > happening. > > > > > > --v2 > > > -Switch condition and do an early return [Jani] -Do some checks > > > in > > > compute_config [Jani] -Do not use register reads as a method of > > > checking states for DPKGC or delayed vblank [Jani] -Use another > > > way to > > > see is vblank interrupts are disabled or not [Jani] > > > > > > --v3 > > > -Use has_psr to check if psr can be enabled or not for dc5_entry > > > cond > > > [Uma] - Move the dc5 entry computation to psr_compute_config > > > [Jouni] > > > -No need to change sequence of enabled and activate, so dont make > > > hsw_psr1_activate return anything [Jouni] -Use has_psr to stop > > > psr1 > > > activation [Jouni] -Use lineage no. in WA -Add the display ver > > > restrictions for WA > > > > > > --v4 > > > -use more appropriate name for check_vblank_limit() [Jouni] - > > > Cover the > > > case for idle frames when dpkgc is not configured [Jouni] -Check > > > psr > > > only for edp [Jouni] > > > > > > --v5 > > > -move psr1 handling to plane update [Jouni] -add todo for cases > > > when > > > vblank is enabled when psr enabled [Jouni] -use intel_display > > > instead > > > of drm_i915_private > > > > > > --v6 > > > -check target_dc_state [Jouni] > > > -fix condition in pre/post plane update [Jouni] > > > > > > WA: 22019444797 > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > > --- > > > .../drm/i915/display/intel_display_types.h | 3 + > > > drivers/gpu/drm/i915/display/intel_psr.c | 112 > > > +++++++++++++++++- > > > 2 files changed, 114 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index 733de5edcfdb..59c81f0a3abd 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -1577,6 +1577,9 @@ struct intel_psr { > > > #define I915_PSR_DEBUG_PANEL_REPLAY_DISABLE 0x40 > > > > > > u32 debug; > > > + bool is_dpkgc_configured; > > > + bool is_dc5_entry_possible; > > > + bool is_wa_delayed_vblank_limit; > > > bool sink_support; > > > bool source_support; > > > bool enabled; > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index b30fa067ce6e..e50b476494a0 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -26,6 +26,7 @@ > > > #include <drm/drm_atomic_helper.h> > > > #include <drm/drm_damage_helper.h> > > > #include <drm/drm_debugfs.h> > > > +#include <drm/drm_vblank.h> > > > > > > #include "i915_drv.h" > > > #include "i915_reg.h" > > > @@ -874,6 +875,78 @@ static u8 psr_compute_idle_frames(struct > > intel_dp > > > *intel_dp) > > > return idle_frames; > > > } > > > > > > +static bool > > > +intel_psr_check_wa_delayed_vblank(const struct drm_display_mode > > > +*adjusted_mode) { > > > + return (adjusted_mode->crtc_vblank_start - > > > +adjusted_mode->crtc_vdisplay) >= 6; } > > > + > > > +/* > > > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and > > > + * VRR is not enabled > > > + */ > > > +static bool intel_psr_is_dpkgc_configured(struct intel_display > > > *display, > > > + struct > > > intel_atomic_state *state) { > > > + struct intel_crtc *intel_crtc; > > > + struct intel_crtc_state *crtc_state; > > > + int i; > > > + > > > + if (DISPLAY_VER(display) < 20) > > > + return false; > > > + > > > + for_each_new_intel_crtc_in_state(state, intel_crtc, > > > crtc_state, i) { > > > + if (!intel_crtc->active) > > > + continue; > > > + > > > + if (crtc_state->vrr.enable) > > > + return false; > > > + } > > > + > > > + return true; > > > +} > > > + > > > +/* > > > + * DC5 entry is only possible if vblank interrupt is disabled > > > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all > > > + * enabled encoders. > > > + */ > > > +static bool > > > +intel_psr_is_dc5_entry_possible(struct intel_display *display, > > > + struct intel_atomic_state *state) > > > { > > > + struct intel_crtc *intel_crtc; > > > + struct intel_crtc_state *crtc_state; > > > + int i; > > > + > > > + if ((display->power.domains.target_dc_state & > > > + DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0) > > > + return false; > > > + > > > + for_each_new_intel_crtc_in_state(state, intel_crtc, > > > crtc_state, i) { > > > + struct drm_crtc *crtc = &intel_crtc->base; > > > + struct drm_vblank_crtc *vblank; > > > + struct intel_encoder *encoder; > > > + > > > + if (!intel_crtc->active) > > > + continue; > > > + > > > + vblank = drm_crtc_vblank_crtc(crtc); > > > + > > > + if (vblank->enabled) > > > + return false; > > > + > > > + if (crtc_state->has_psr) > > > + return false; > > > > It should be !has_psr > > > > > + > > > + for_each_encoder_on_crtc(display->drm, crtc, > > > encoder) > > > + if (encoder->type != INTEL_OUTPUT_EDP) > > > + return false; > > > + } > > > + > > > + return true; > > > +} > > > + > > > static void hsw_activate_psr1(struct intel_dp *intel_dp) { > > > struct intel_display *display = > > > to_intel_display(intel_dp); @@ > > > -986,7 > > > +1059,15 @@ static void hsw_activate_psr2(struct intel_dp > > > *intel_dp) > > > u32 val = EDP_PSR2_ENABLE; > > > u32 psr_val = 0; > > > > > > - val |= > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > > > + /* > > > + * Wa_22019444797 > > > + * TODO: Disable idle frames when vblank gets enabled > > > while > > > + * PSR2 is enabled > > > + */ > > > + if (DISPLAY_VER(dev_priv) != 20 || > > > + !intel_dp->psr.is_dpkgc_configured || > > > > Why ! for dpkgc, Here this can be enabled if dpkgc_configured right > > ? > > > > So we want idle frames to be written in these scenarios display ver > !=20 if it is equal to 20 > Then we write idle frames if dpkgc is not configured and if dpkgc is > confirgured then we write > Idle frames if dc5 entry is possible > > > > + intel_dp->psr.is_dc5_entry_possible) > > > + val |= > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > > > > > > if (DISPLAY_VER(display) < 14 && > > > !IS_ALDERLAKE_P(dev_priv)) > > > val |= EDP_SU_TRACK_ENABLE; > > > @@ -2667,10 +2748,20 @@ void intel_psr_pre_plane_update(struct > > > intel_atomic_state *state, > > > const struct intel_crtc_state *new_crtc_state = > > > intel_atomic_get_new_crtc_state(state, crtc); > > > struct intel_encoder *encoder; > > > + bool dpkgc_configured = false, dc5_entry_possible = > > > false; > > > + bool wa_delayed_vblank_limit = false; > > > > > > if (!HAS_PSR(display)) > > > return; > > > > > > + if (DISPLAY_VER(display) == 20) { > > > + dpkgc_configured = > > > intel_psr_is_dpkgc_configured(display, > > > state); > > > + dc5_entry_possible = > > > + intel_psr_is_dc5_entry_possible(display, > > > state); > > > + wa_delayed_vblank_limit = > > > + > > intel_psr_check_wa_delayed_vblank(&new_crtc_state- > > > > hw.adjusted_mode); > > > + } > > > + > > > for_each_intel_encoder_mask_with_psr(state->base.dev, > > > encoder, > > > old_crtc_state- > > > uapi.encoder_mask) > > > { > > > struct intel_dp *intel_dp = > > > enc_to_intel_dp(encoder); @@ - > > > 2679,6 +2770,12 @@ void intel_psr_pre_plane_update(struct > > > intel_atomic_state *state, > > > > > > mutex_lock(&psr->lock); > > > > > > + if (DISPLAY_VER(i915) == 20) { > > > + psr->is_dpkgc_configured = > > > dpkgc_configured; > > > + psr->is_dc5_entry_possible = > > > dc5_entry_possible; > > > + psr->is_wa_delayed_vblank_limit = > > > wa_delayed_vblank_limit; > > > > We can drop the variables and directly assign this to psr->... and > > use it > > subsequently. > > So the reason I had assigned this to variables is I did not want to > call the 3 functions > Above inside the for_each_encoder loop since the functions themselves > have loops inside them > Which would just increase the cost hence the variables. > > > > Also it would be good to have this done in compute and than just > > use it > > here. > > It was done in compute a few revisions ago @Hogander, Jouni suggested > to move it to > pre_plane_update. It needs to be done in pre/post_planee update because only there you know what will be the state of all crtcs. I.e. compute_config is done for all crtcs and all encoders. BR, Jouni Högander > > > > > > + } > > > + > > > /* > > > * Reasons to disable: > > > * - PSR disabled in new state > > > @@ -2686,6 +2783,7 @@ void intel_psr_pre_plane_update(struct > > > intel_atomic_state *state, > > > * - Changing between PSR versions > > > * - Region Early Transport changing > > > * - Display WA #1136: skl, bxt > > > + * - Display WA_22019444797 > > > */ > > > needs_to_disable |= > > > intel_crtc_needs_modeset(new_crtc_state); > > > needs_to_disable |= !new_crtc_state->has_psr; @@ > > > -2695,6 > > > +2793,10 @@ void intel_psr_pre_plane_update(struct > > > intel_atomic_state > > > +*state, > > > psr->su_region_et_enabled; > > > needs_to_disable |= DISPLAY_VER(i915) < 11 && > > > new_crtc_state->wm_level_disabled; > > > + /* TODO: Disable PSR1 when vblank gets enabled > > > while PSR1 > > is > > > enabled */ > > > + needs_to_disable |= DISPLAY_VER(display) == 20 && > > > dpkgc_configured && > > > + (wa_delayed_vblank_limit || > > > dc5_entry_possible) > > && > > > + !new_crtc_state->has_sel_update && > > > +!new_crtc_state->has_panel_replay; > > > > Good to move this to a small helper function which can be extended > > if > > required as well. > > Also seems used in post_plane as well. > > Sure will move it into its own helper function > > Regards, > Suraj Kandpal > > > > > @Hogander, Jouni Can you also ack if this handling for PSR is ok. > > > > > if (psr->enabled && needs_to_disable) > > > intel_psr_disable_locked(intel_dp); > > > @@ -2735,6 +2837,14 @@ void intel_psr_post_plane_update(struct > > > intel_atomic_state *state, > > > keep_disabled |= DISPLAY_VER(display) < 11 && > > > crtc_state->wm_level_disabled; > > > > > > + /* > > > + * Wa_22019444797 > > > + * TODO: Disable PSR1 when vblank gets enabled > > > while PSR1 > > is > > > enabled > > > + */ > > > + keep_disabled |= DISPLAY_VER(display) == 20 && > > > psr- > > > > is_dpkgc_configured && > > > + (psr->is_wa_delayed_vblank_limit || psr- > > > > is_dc5_entry_possible) && > > > + !crtc_state->has_sel_update && > > > !crtc_state- > > > > has_panel_replay; > > > + > > > if (!psr->enabled && !keep_disabled) > > > intel_psr_enable_locked(intel_dp, > > > crtc_state); > > > else if (psr->enabled && !crtc_state- > > > >wm_level_disabled) > > > -- > > > 2.43.2 >
On Thu, 2024-09-19 at 12:14 +0000, Shankar, Uma wrote: > > > > -----Original Message----- > > From: Kandpal, Suraj <suraj.kandpal@intel.com> > > Sent: Monday, September 9, 2024 12:02 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Shankar, Uma <uma.shankar@intel.com>; Hogander, Jouni > > <jouni.hogander@intel.com>; Deak, Imre <imre.deak@intel.com>; > > Kandpal, Suraj > > <suraj.kandpal@intel.com> > > Subject: [PATCH] drm/i915/psr: Implment WA to help reach PC10 > > Not: Typo in implement > > > To reach PC10 when PKG_C_LATENCY is configure we must do the > > following > > things > > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be > > entered > > 2) Allow PSR2 deep sleep when DC5 can be entered > > 3) DC5 can be entered when all transocoder have either PSR1, PSR2 > > or eDP 1.5 PR > > ALPM enabled and VBI is disabled and flips and pushes are not > > happening. > > > > --v2 > > -Switch condition and do an early return [Jani] -Do some checks in > > compute_config [Jani] -Do not use register reads as a method of > > checking states > > for DPKGC or delayed vblank [Jani] -Use another way to see is > > vblank interrupts > > are disabled or not [Jani] > > > > --v3 > > -Use has_psr to check if psr can be enabled or not for dc5_entry > > cond [Uma] - > > Move the dc5 entry computation to psr_compute_config [Jouni] -No > > need to > > change sequence of enabled and activate, so dont make > > hsw_psr1_activate > > return anything [Jouni] -Use has_psr to stop psr1 activation > > [Jouni] -Use lineage > > no. in WA -Add the display ver restrictions for WA > > > > --v4 > > -use more appropriate name for check_vblank_limit() [Jouni] -Cover > > the case for > > idle frames when dpkgc is not configured [Jouni] -Check psr only > > for edp [Jouni] > > > > --v5 > > -move psr1 handling to plane update [Jouni] -add todo for cases > > when vblank is > > enabled when psr enabled [Jouni] -use intel_display instead of > > drm_i915_private > > > > --v6 > > -check target_dc_state [Jouni] > > -fix condition in pre/post plane update [Jouni] > > > > WA: 22019444797 > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > .../drm/i915/display/intel_display_types.h | 3 + > > drivers/gpu/drm/i915/display/intel_psr.c | 112 > > +++++++++++++++++- > > 2 files changed, 114 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index 733de5edcfdb..59c81f0a3abd 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1577,6 +1577,9 @@ struct intel_psr { > > #define I915_PSR_DEBUG_PANEL_REPLAY_DISABLE 0x40 > > > > u32 debug; > > + bool is_dpkgc_configured; > > + bool is_dc5_entry_possible; > > + bool is_wa_delayed_vblank_limit; > > bool sink_support; > > bool source_support; > > bool enabled; > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index b30fa067ce6e..e50b476494a0 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -26,6 +26,7 @@ > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_damage_helper.h> > > #include <drm/drm_debugfs.h> > > +#include <drm/drm_vblank.h> > > > > #include "i915_drv.h" > > #include "i915_reg.h" > > @@ -874,6 +875,78 @@ static u8 psr_compute_idle_frames(struct > > intel_dp > > *intel_dp) > > return idle_frames; > > } > > > > +static bool > > +intel_psr_check_wa_delayed_vblank(const struct drm_display_mode > > +*adjusted_mode) { > > + return (adjusted_mode->crtc_vblank_start - > > +adjusted_mode->crtc_vdisplay) >= 6; } > > + > > +/* > > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and > > + * VRR is not enabled > > + */ > > +static bool intel_psr_is_dpkgc_configured(struct intel_display > > *display, > > + struct intel_atomic_state > > *state) { > > + struct intel_crtc *intel_crtc; > > + struct intel_crtc_state *crtc_state; > > + int i; > > + > > + if (DISPLAY_VER(display) < 20) > > + return false; > > + > > + for_each_new_intel_crtc_in_state(state, intel_crtc, > > crtc_state, i) { > > + if (!intel_crtc->active) > > + continue; > > + > > + if (crtc_state->vrr.enable) > > + return false; > > + } > > + > > + return true; > > +} > > + > > +/* > > + * DC5 entry is only possible if vblank interrupt is disabled > > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all > > + * enabled encoders. > > + */ > > +static bool > > +intel_psr_is_dc5_entry_possible(struct intel_display *display, > > + struct intel_atomic_state *state) > > +{ > > + struct intel_crtc *intel_crtc; > > + struct intel_crtc_state *crtc_state; > > + int i; > > + > > + if ((display->power.domains.target_dc_state & > > + DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0) > > + return false; > > + > > + for_each_new_intel_crtc_in_state(state, intel_crtc, > > crtc_state, i) { > > + struct drm_crtc *crtc = &intel_crtc->base; > > + struct drm_vblank_crtc *vblank; > > + struct intel_encoder *encoder; > > + > > + if (!intel_crtc->active) > > + continue; > > + > > + vblank = drm_crtc_vblank_crtc(crtc); > > + > > + if (vblank->enabled) > > + return false; > > + > > + if (crtc_state->has_psr) > > + return false; > > It should be !has_psr > > + > > + for_each_encoder_on_crtc(display->drm, crtc, > > encoder) > > + if (encoder->type != INTEL_OUTPUT_EDP) > > + return false; I'm not sure if we need to care about dual eDP case. One PSR and another non-PSR. That will return true from this function even thought it's not possible. That can be solved by checking CAN_PSR(intel_dp) here. > > + } > > + > > + return true; > > +} > > + > > static void hsw_activate_psr1(struct intel_dp *intel_dp) { > > struct intel_display *display = to_intel_display(intel_dp); > > @@ -986,7 > > +1059,15 @@ static void hsw_activate_psr2(struct intel_dp > > *intel_dp) > > u32 val = EDP_PSR2_ENABLE; > > u32 psr_val = 0; > > > > - val |= > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > > + /* > > + * Wa_22019444797 > > + * TODO: Disable idle frames when vblank gets enabled while > > + * PSR2 is enabled > > + */ > > + if (DISPLAY_VER(dev_priv) != 20 || > > + !intel_dp->psr.is_dpkgc_configured || > > Why ! for dpkgc, Here this can be enabled if dpkgc_configured right ? > > > + intel_dp->psr.is_dc5_entry_possible) > > + val |= > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > > > > if (DISPLAY_VER(display) < 14 && !IS_ALDERLAKE_P(dev_priv)) > > val |= EDP_SU_TRACK_ENABLE; > > @@ -2667,10 +2748,20 @@ void intel_psr_pre_plane_update(struct > > intel_atomic_state *state, > > const struct intel_crtc_state *new_crtc_state = > > intel_atomic_get_new_crtc_state(state, crtc); > > struct intel_encoder *encoder; > > + bool dpkgc_configured = false, dc5_entry_possible = false; > > + bool wa_delayed_vblank_limit = false; > > > > if (!HAS_PSR(display)) > > return; > > > > + if (DISPLAY_VER(display) == 20) { > > + dpkgc_configured = > > intel_psr_is_dpkgc_configured(display, > > state); > > + dc5_entry_possible = > > + intel_psr_is_dc5_entry_possible(display, > > state); > > + wa_delayed_vblank_limit = > > + intel_psr_check_wa_delayed_vblank(&new_crtc > > _state- > > > hw.adjusted_mode); > > + } > > + > > for_each_intel_encoder_mask_with_psr(state->base.dev, > > encoder, > > old_crtc_state- > > >uapi.encoder_mask) > > { > > struct intel_dp *intel_dp = > > enc_to_intel_dp(encoder); @@ - > > 2679,6 +2770,12 @@ void intel_psr_pre_plane_update(struct > > intel_atomic_state > > *state, > > > > mutex_lock(&psr->lock); > > > > + if (DISPLAY_VER(i915) == 20) { > > + psr->is_dpkgc_configured = > > dpkgc_configured; > > + psr->is_dc5_entry_possible = > > dc5_entry_possible; > > + psr->is_wa_delayed_vblank_limit = > > wa_delayed_vblank_limit; > > We can drop the variables and directly assign this to psr->... and > use it subsequently. > Also it would be good to have this done in compute and than just use > it here. > > > + } > > + > > /* > > * Reasons to disable: > > * - PSR disabled in new state > > @@ -2686,6 +2783,7 @@ void intel_psr_pre_plane_update(struct > > intel_atomic_state *state, > > * - Changing between PSR versions > > * - Region Early Transport changing > > * - Display WA #1136: skl, bxt > > + * - Display WA_22019444797 > > */ > > needs_to_disable |= > > intel_crtc_needs_modeset(new_crtc_state); > > needs_to_disable |= !new_crtc_state->has_psr; @@ - > > 2695,6 > > +2793,10 @@ void intel_psr_pre_plane_update(struct > > intel_atomic_state *state, > > psr->su_region_et_enabled; > > needs_to_disable |= DISPLAY_VER(i915) < 11 && > > new_crtc_state->wm_level_disabled; > > + /* TODO: Disable PSR1 when vblank gets enabled > > while PSR1 is > > enabled */ > > + needs_to_disable |= DISPLAY_VER(display) == 20 && > > dpkgc_configured && > > + (wa_delayed_vblank_limit || > > dc5_entry_possible) && > > + !new_crtc_state->has_sel_update && > > +!new_crtc_state->has_panel_replay; > > Good to move this to a small helper function which can be extended if > required as well. > Also seems used in post_plane as well. > > @Hogander, Jouni Can you also ack if this handling for PSR is ok. This need_to_disable/keep_disabled is ok. I think there is a bug in check itself: dc5_entry_possible should be !dc5_entry_possible What do you think? BR, Jouni Högander > > > if (psr->enabled && needs_to_disable) > > intel_psr_disable_locked(intel_dp); > > @@ -2735,6 +2837,14 @@ void intel_psr_post_plane_update(struct > > intel_atomic_state *state, > > keep_disabled |= DISPLAY_VER(display) < 11 && > > crtc_state->wm_level_disabled; > > > > + /* > > + * Wa_22019444797 > > + * TODO: Disable PSR1 when vblank gets enabled > > while PSR1 is > > enabled > > + */ > > + keep_disabled |= DISPLAY_VER(display) == 20 && psr- > > > is_dpkgc_configured && > > + (psr->is_wa_delayed_vblank_limit || psr- > > > is_dc5_entry_possible) && > > + !crtc_state->has_sel_update && !crtc_state- > > > has_panel_replay; > > + > > if (!psr->enabled && !keep_disabled) > > intel_psr_enable_locked(intel_dp, > > crtc_state); > > else if (psr->enabled && !crtc_state- > > >wm_level_disabled) > > -- > > 2.43.2 >
> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@intel.com> > Sent: Friday, September 20, 2024 11:06 AM > To: Shankar, Uma <uma.shankar@intel.com>; Kandpal, Suraj > <suraj.kandpal@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Deak, Imre <imre.deak@intel.com> > Subject: Re: [PATCH] drm/i915/psr: Implment WA to help reach PC10 > > On Thu, 2024-09-19 at 12:14 +0000, Shankar, Uma wrote: > > > > > > > -----Original Message----- > > > From: Kandpal, Suraj <suraj.kandpal@intel.com> > > > Sent: Monday, September 9, 2024 12:02 PM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Shankar, Uma <uma.shankar@intel.com>; Hogander, Jouni > > > <jouni.hogander@intel.com>; Deak, Imre <imre.deak@intel.com>; > > > Kandpal, Suraj <suraj.kandpal@intel.com> > > > Subject: [PATCH] drm/i915/psr: Implment WA to help reach PC10 > > > > Not: Typo in implement > > > > > To reach PC10 when PKG_C_LATENCY is configure we must do the > > > following things > > > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be > > > entered > > > 2) Allow PSR2 deep sleep when DC5 can be entered > > > 3) DC5 can be entered when all transocoder have either PSR1, PSR2 or > > > eDP 1.5 PR ALPM enabled and VBI is disabled and flips and pushes are > > > not happening. > > > > > > --v2 > > > -Switch condition and do an early return [Jani] -Do some checks in > > > compute_config [Jani] -Do not use register reads as a method of > > > checking states for DPKGC or delayed vblank [Jani] -Use another way > > > to see is vblank interrupts are disabled or not [Jani] > > > > > > --v3 > > > -Use has_psr to check if psr can be enabled or not for dc5_entry > > > cond [Uma] - Move the dc5 entry computation to psr_compute_config > > > [Jouni] -No need to change sequence of enabled and activate, so dont > > > make hsw_psr1_activate return anything [Jouni] -Use has_psr to stop > > > psr1 activation [Jouni] -Use lineage no. in WA -Add the display ver > > > restrictions for WA > > > > > > --v4 > > > -use more appropriate name for check_vblank_limit() [Jouni] -Cover > > > the case for idle frames when dpkgc is not configured [Jouni] -Check > > > psr only for edp [Jouni] > > > > > > --v5 > > > -move psr1 handling to plane update [Jouni] -add todo for cases when > > > vblank is enabled when psr enabled [Jouni] -use intel_display > > > instead of drm_i915_private > > > > > > --v6 > > > -check target_dc_state [Jouni] > > > -fix condition in pre/post plane update [Jouni] > > > > > > WA: 22019444797 > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > > --- > > > .../drm/i915/display/intel_display_types.h | 3 + > > > drivers/gpu/drm/i915/display/intel_psr.c | 112 > > > +++++++++++++++++- > > > 2 files changed, 114 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index 733de5edcfdb..59c81f0a3abd 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -1577,6 +1577,9 @@ struct intel_psr { > > > #define I915_PSR_DEBUG_PANEL_REPLAY_DISABLE 0x40 > > > > > > u32 debug; > > > + bool is_dpkgc_configured; > > > + bool is_dc5_entry_possible; > > > + bool is_wa_delayed_vblank_limit; > > > bool sink_support; > > > bool source_support; > > > bool enabled; > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index b30fa067ce6e..e50b476494a0 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -26,6 +26,7 @@ > > > #include <drm/drm_atomic_helper.h> > > > #include <drm/drm_damage_helper.h> > > > #include <drm/drm_debugfs.h> > > > +#include <drm/drm_vblank.h> > > > > > > #include "i915_drv.h" > > > #include "i915_reg.h" > > > @@ -874,6 +875,78 @@ static u8 psr_compute_idle_frames(struct > > > intel_dp > > > *intel_dp) > > > return idle_frames; > > > } > > > > > > +static bool > > > +intel_psr_check_wa_delayed_vblank(const struct drm_display_mode > > > +*adjusted_mode) { > > > + return (adjusted_mode->crtc_vblank_start - > > > +adjusted_mode->crtc_vdisplay) >= 6; } > > > + > > > +/* > > > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and > > > + * VRR is not enabled > > > + */ > > > +static bool intel_psr_is_dpkgc_configured(struct intel_display > > > *display, > > > + struct intel_atomic_state > > > *state) { > > > + struct intel_crtc *intel_crtc; > > > + struct intel_crtc_state *crtc_state; > > > + int i; > > > + > > > + if (DISPLAY_VER(display) < 20) > > > + return false; > > > + > > > + for_each_new_intel_crtc_in_state(state, intel_crtc, > > > crtc_state, i) { > > > + if (!intel_crtc->active) > > > + continue; > > > + > > > + if (crtc_state->vrr.enable) > > > + return false; > > > + } > > > + > > > + return true; > > > +} > > > + > > > +/* > > > + * DC5 entry is only possible if vblank interrupt is disabled > > > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all > > > + * enabled encoders. > > > + */ > > > +static bool > > > +intel_psr_is_dc5_entry_possible(struct intel_display *display, > > > + struct intel_atomic_state *state) { > > > + struct intel_crtc *intel_crtc; > > > + struct intel_crtc_state *crtc_state; > > > + int i; > > > + > > > + if ((display->power.domains.target_dc_state & > > > + DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0) > > > + return false; > > > + > > > + for_each_new_intel_crtc_in_state(state, intel_crtc, > > > crtc_state, i) { > > > + struct drm_crtc *crtc = &intel_crtc->base; > > > + struct drm_vblank_crtc *vblank; > > > + struct intel_encoder *encoder; > > > + > > > + if (!intel_crtc->active) > > > + continue; > > > + > > > + vblank = drm_crtc_vblank_crtc(crtc); > > > + > > > + if (vblank->enabled) > > > + return false; > > > + > > > + if (crtc_state->has_psr) > > > + return false; > > > > It should be !has_psr > > > + > > > + for_each_encoder_on_crtc(display->drm, crtc, > > > encoder) > > > + if (encoder->type != INTEL_OUTPUT_EDP) > > > + return false; > > I'm not sure if we need to care about dual eDP case. One PSR and another non- > PSR. That will return true from this function even thought it's not possible. That > can be solved by checking CAN_PSR(intel_dp) here. So , if (encoder->type != INTEL_OUTPUT_EDP && CAN_PSR(intel_dp)) return false; > > > > + } > > > + > > > + return true; > > > +} > > > + > > > static void hsw_activate_psr1(struct intel_dp *intel_dp) { > > > struct intel_display *display = to_intel_display(intel_dp); > > > @@ -986,7 > > > +1059,15 @@ static void hsw_activate_psr2(struct intel_dp > > > *intel_dp) > > > u32 val = EDP_PSR2_ENABLE; > > > u32 psr_val = 0; > > > > > > - val |= > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > > > + /* > > > + * Wa_22019444797 > > > + * TODO: Disable idle frames when vblank gets enabled while > > > + * PSR2 is enabled > > > + */ > > > + if (DISPLAY_VER(dev_priv) != 20 || > > > + !intel_dp->psr.is_dpkgc_configured || > > > > Why ! for dpkgc, Here this can be enabled if dpkgc_configured right ? > > > > > + intel_dp->psr.is_dc5_entry_possible) > > > + val |= > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > > > > > > if (DISPLAY_VER(display) < 14 && !IS_ALDERLAKE_P(dev_priv)) > > > val |= EDP_SU_TRACK_ENABLE; @@ -2667,10 +2748,20 @@ > > > void intel_psr_pre_plane_update(struct intel_atomic_state *state, > > > const struct intel_crtc_state *new_crtc_state = > > > intel_atomic_get_new_crtc_state(state, crtc); > > > struct intel_encoder *encoder; > > > + bool dpkgc_configured = false, dc5_entry_possible = false; > > > + bool wa_delayed_vblank_limit = false; > > > > > > if (!HAS_PSR(display)) > > > return; > > > > > > + if (DISPLAY_VER(display) == 20) { > > > + dpkgc_configured = > > > intel_psr_is_dpkgc_configured(display, > > > state); > > > + dc5_entry_possible = > > > + intel_psr_is_dc5_entry_possible(display, > > > state); > > > + wa_delayed_vblank_limit = > > > + intel_psr_check_wa_delayed_vblank(&new_crtc > > > _state- > > > > hw.adjusted_mode); > > > + } > > > + > > > for_each_intel_encoder_mask_with_psr(state->base.dev, > > > encoder, > > > old_crtc_state- > > > >uapi.encoder_mask) > > > { > > > struct intel_dp *intel_dp = > > > enc_to_intel_dp(encoder); @@ - > > > 2679,6 +2770,12 @@ void intel_psr_pre_plane_update(struct > > > intel_atomic_state *state, > > > > > > mutex_lock(&psr->lock); > > > > > > + if (DISPLAY_VER(i915) == 20) { > > > + psr->is_dpkgc_configured = > > > dpkgc_configured; > > > + psr->is_dc5_entry_possible = > > > dc5_entry_possible; > > > + psr->is_wa_delayed_vblank_limit = > > > wa_delayed_vblank_limit; > > > > We can drop the variables and directly assign this to psr->... and use > > it subsequently. > > Also it would be good to have this done in compute and than just use > > it here. > > > > > + } > > > + > > > /* > > > * Reasons to disable: > > > * - PSR disabled in new state @@ -2686,6 +2783,7 @@ > > > void intel_psr_pre_plane_update(struct intel_atomic_state *state, > > > * - Changing between PSR versions > > > * - Region Early Transport changing > > > * - Display WA #1136: skl, bxt > > > + * - Display WA_22019444797 > > > */ > > > needs_to_disable |= > > > intel_crtc_needs_modeset(new_crtc_state); > > > needs_to_disable |= !new_crtc_state->has_psr; @@ - > > > 2695,6 > > > +2793,10 @@ void intel_psr_pre_plane_update(struct > > > intel_atomic_state *state, > > > psr->su_region_et_enabled; > > > needs_to_disable |= DISPLAY_VER(i915) < 11 && > > > new_crtc_state->wm_level_disabled; > > > + /* TODO: Disable PSR1 when vblank gets enabled > > > while PSR1 is > > > enabled */ > > > + needs_to_disable |= DISPLAY_VER(display) == 20 && > > > dpkgc_configured && > > > + (wa_delayed_vblank_limit || > > > dc5_entry_possible) && > > > + !new_crtc_state->has_sel_update && > > > +!new_crtc_state->has_panel_replay; > > > > Good to move this to a small helper function which can be extended if > > required as well. > > Also seems used in post_plane as well. > > > > @Hogander, Jouni Can you also ack if this handling for PSR is ok. > > This need_to_disable/keep_disabled is ok. I think there is a bug in check itself: > > dc5_entry_possible should be !dc5_entry_possible > > What do you think? > Dc5_entry_possible returns true when we can enter dc5. And the condition to disable ps1 is to have a delayed_vblank > 6 or When dc5 can be entered so that check would be correct Regards, Suraj Kandpal > BR, > > Jouni Högander > > > > > > if (psr->enabled && needs_to_disable) > > > intel_psr_disable_locked(intel_dp); > > > @@ -2735,6 +2837,14 @@ void intel_psr_post_plane_update(struct > > > intel_atomic_state *state, > > > keep_disabled |= DISPLAY_VER(display) < 11 && > > > crtc_state->wm_level_disabled; > > > > > > + /* > > > + * Wa_22019444797 > > > + * TODO: Disable PSR1 when vblank gets enabled > > > while PSR1 is > > > enabled > > > + */ > > > + keep_disabled |= DISPLAY_VER(display) == 20 && psr- > > > > is_dpkgc_configured && > > > + (psr->is_wa_delayed_vblank_limit || psr- > > > > is_dc5_entry_possible) && > > > + !crtc_state->has_sel_update && !crtc_state- > > > > has_panel_replay; > > > + > > > if (!psr->enabled && !keep_disabled) > > > intel_psr_enable_locked(intel_dp, > > > crtc_state); > > > else if (psr->enabled && !crtc_state- > > > >wm_level_disabled) > > > -- > > > 2.43.2 > >
On Fri, 2024-09-20 at 06:29 +0000, Kandpal, Suraj wrote: > > > > -----Original Message----- > > From: Hogander, Jouni <jouni.hogander@intel.com> > > Sent: Friday, September 20, 2024 11:06 AM > > To: Shankar, Uma <uma.shankar@intel.com>; Kandpal, Suraj > > <suraj.kandpal@intel.com>; intel-gfx@lists.freedesktop.org > > Cc: Deak, Imre <imre.deak@intel.com> > > Subject: Re: [PATCH] drm/i915/psr: Implment WA to help reach PC10 > > > > On Thu, 2024-09-19 at 12:14 +0000, Shankar, Uma wrote: > > > > > > > > > > -----Original Message----- > > > > From: Kandpal, Suraj <suraj.kandpal@intel.com> > > > > Sent: Monday, September 9, 2024 12:02 PM > > > > To: intel-gfx@lists.freedesktop.org > > > > Cc: Shankar, Uma <uma.shankar@intel.com>; Hogander, Jouni > > > > <jouni.hogander@intel.com>; Deak, Imre <imre.deak@intel.com>; > > > > Kandpal, Suraj <suraj.kandpal@intel.com> > > > > Subject: [PATCH] drm/i915/psr: Implment WA to help reach PC10 > > > > > > Not: Typo in implement > > > > > > > To reach PC10 when PKG_C_LATENCY is configure we must do the > > > > following things > > > > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be > > > > entered > > > > 2) Allow PSR2 deep sleep when DC5 can be entered > > > > 3) DC5 can be entered when all transocoder have either PSR1, > > > > PSR2 or > > > > eDP 1.5 PR ALPM enabled and VBI is disabled and flips and > > > > pushes are > > > > not happening. > > > > > > > > --v2 > > > > -Switch condition and do an early return [Jani] -Do some checks > > > > in > > > > compute_config [Jani] -Do not use register reads as a method of > > > > checking states for DPKGC or delayed vblank [Jani] -Use another > > > > way > > > > to see is vblank interrupts are disabled or not [Jani] > > > > > > > > --v3 > > > > -Use has_psr to check if psr can be enabled or not for > > > > dc5_entry > > > > cond [Uma] - Move the dc5 entry computation to > > > > psr_compute_config > > > > [Jouni] -No need to change sequence of enabled and activate, so > > > > dont > > > > make hsw_psr1_activate return anything [Jouni] -Use has_psr to > > > > stop > > > > psr1 activation [Jouni] -Use lineage no. in WA -Add the display > > > > ver > > > > restrictions for WA > > > > > > > > --v4 > > > > -use more appropriate name for check_vblank_limit() [Jouni] - > > > > Cover > > > > the case for idle frames when dpkgc is not configured [Jouni] - > > > > Check > > > > psr only for edp [Jouni] > > > > > > > > --v5 > > > > -move psr1 handling to plane update [Jouni] -add todo for cases > > > > when > > > > vblank is enabled when psr enabled [Jouni] -use intel_display > > > > instead of drm_i915_private > > > > > > > > --v6 > > > > -check target_dc_state [Jouni] > > > > -fix condition in pre/post plane update [Jouni] > > > > > > > > WA: 22019444797 > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > > > --- > > > > .../drm/i915/display/intel_display_types.h | 3 + > > > > drivers/gpu/drm/i915/display/intel_psr.c | 112 > > > > +++++++++++++++++- > > > > 2 files changed, 114 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > index 733de5edcfdb..59c81f0a3abd 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > @@ -1577,6 +1577,9 @@ struct intel_psr { > > > > #define I915_PSR_DEBUG_PANEL_REPLAY_DISABLE 0x40 > > > > > > > > u32 debug; > > > > + bool is_dpkgc_configured; > > > > + bool is_dc5_entry_possible; > > > > + bool is_wa_delayed_vblank_limit; > > > > bool sink_support; > > > > bool source_support; > > > > bool enabled; > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > > index b30fa067ce6e..e50b476494a0 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > @@ -26,6 +26,7 @@ > > > > #include <drm/drm_atomic_helper.h> > > > > #include <drm/drm_damage_helper.h> > > > > #include <drm/drm_debugfs.h> > > > > +#include <drm/drm_vblank.h> > > > > > > > > #include "i915_drv.h" > > > > #include "i915_reg.h" > > > > @@ -874,6 +875,78 @@ static u8 psr_compute_idle_frames(struct > > > > intel_dp > > > > *intel_dp) > > > > return idle_frames; > > > > } > > > > > > > > +static bool > > > > +intel_psr_check_wa_delayed_vblank(const struct > > > > drm_display_mode > > > > +*adjusted_mode) { > > > > + return (adjusted_mode->crtc_vblank_start - > > > > +adjusted_mode->crtc_vdisplay) >= 6; } > > > > + > > > > +/* > > > > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and > > > > + * VRR is not enabled > > > > + */ > > > > +static bool intel_psr_is_dpkgc_configured(struct intel_display > > > > *display, > > > > + struct > > > > intel_atomic_state > > > > *state) { > > > > + struct intel_crtc *intel_crtc; > > > > + struct intel_crtc_state *crtc_state; > > > > + int i; > > > > + > > > > + if (DISPLAY_VER(display) < 20) > > > > + return false; > > > > + > > > > + for_each_new_intel_crtc_in_state(state, intel_crtc, > > > > crtc_state, i) { > > > > + if (!intel_crtc->active) > > > > + continue; > > > > + > > > > + if (crtc_state->vrr.enable) > > > > + return false; > > > > + } > > > > + > > > > + return true; > > > > +} > > > > + > > > > +/* > > > > + * DC5 entry is only possible if vblank interrupt is disabled > > > > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all > > > > + * enabled encoders. > > > > + */ > > > > +static bool > > > > +intel_psr_is_dc5_entry_possible(struct intel_display *display, > > > > + struct intel_atomic_state > > > > *state) { > > > > + struct intel_crtc *intel_crtc; > > > > + struct intel_crtc_state *crtc_state; > > > > + int i; > > > > + > > > > + if ((display->power.domains.target_dc_state & > > > > + DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0) > > > > + return false; > > > > + > > > > + for_each_new_intel_crtc_in_state(state, intel_crtc, > > > > crtc_state, i) { > > > > + struct drm_crtc *crtc = &intel_crtc->base; > > > > + struct drm_vblank_crtc *vblank; > > > > + struct intel_encoder *encoder; > > > > + > > > > + if (!intel_crtc->active) > > > > + continue; > > > > + > > > > + vblank = drm_crtc_vblank_crtc(crtc); > > > > + > > > > + if (vblank->enabled) > > > > + return false; > > > > + > > > > + if (crtc_state->has_psr) > > > > + return false; > > > > > > It should be !has_psr > > > > + > > > > + for_each_encoder_on_crtc(display->drm, crtc, > > > > encoder) > > > > + if (encoder->type != INTEL_OUTPUT_EDP) > > > > + return false; > > > > I'm not sure if we need to care about dual eDP case. One PSR and > > another non- > > PSR. That will return true from this function even thought it's not > > possible. That > > can be solved by checking CAN_PSR(intel_dp) here. > > So , > if (encoder->type != INTEL_OUTPUT_EDP && > CAN_PSR(intel_dp)) > return false; > > > > > > + } > > > > + > > > > + return true; > > > > +} > > > > + > > > > static void hsw_activate_psr1(struct intel_dp *intel_dp) { > > > > struct intel_display *display = > > > > to_intel_display(intel_dp); > > > > @@ -986,7 > > > > +1059,15 @@ static void hsw_activate_psr2(struct intel_dp > > > > *intel_dp) > > > > u32 val = EDP_PSR2_ENABLE; > > > > u32 psr_val = 0; > > > > > > > > - val |= > > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > > > > + /* > > > > + * Wa_22019444797 > > > > + * TODO: Disable idle frames when vblank gets enabled > > > > while > > > > + * PSR2 is enabled > > > > + */ > > > > + if (DISPLAY_VER(dev_priv) != 20 || > > > > + !intel_dp->psr.is_dpkgc_configured || > > > > > > Why ! for dpkgc, Here this can be enabled if dpkgc_configured > > > right ? > > > > > > > + intel_dp->psr.is_dc5_entry_possible) > > > > + val |= > > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); > > > > > > > > if (DISPLAY_VER(display) < 14 && > > > > !IS_ALDERLAKE_P(dev_priv)) > > > > val |= EDP_SU_TRACK_ENABLE; @@ -2667,10 > > > > +2748,20 @@ > > > > void intel_psr_pre_plane_update(struct intel_atomic_state > > > > *state, > > > > const struct intel_crtc_state *new_crtc_state = > > > > intel_atomic_get_new_crtc_state(state, crtc); > > > > struct intel_encoder *encoder; > > > > + bool dpkgc_configured = false, dc5_entry_possible = > > > > false; > > > > + bool wa_delayed_vblank_limit = false; > > > > > > > > if (!HAS_PSR(display)) > > > > return; > > > > > > > > + if (DISPLAY_VER(display) == 20) { > > > > + dpkgc_configured = > > > > intel_psr_is_dpkgc_configured(display, > > > > state); > > > > + dc5_entry_possible = > > > > + intel_psr_is_dc5_entry_possible(display > > > > , > > > > state); > > > > + wa_delayed_vblank_limit = > > > > + intel_psr_check_wa_delayed_vblank(&new_ > > > > crtc > > > > _state- > > > > > hw.adjusted_mode); > > > > + } > > > > + > > > > for_each_intel_encoder_mask_with_psr(state->base.dev, > > > > encoder, > > > > old_crtc_state- > > > > > uapi.encoder_mask) > > > > { > > > > struct intel_dp *intel_dp = > > > > enc_to_intel_dp(encoder); @@ - > > > > 2679,6 +2770,12 @@ void intel_psr_pre_plane_update(struct > > > > intel_atomic_state *state, > > > > > > > > mutex_lock(&psr->lock); > > > > > > > > + if (DISPLAY_VER(i915) == 20) { > > > > + psr->is_dpkgc_configured = > > > > dpkgc_configured; > > > > + psr->is_dc5_entry_possible = > > > > dc5_entry_possible; > > > > + psr->is_wa_delayed_vblank_limit = > > > > wa_delayed_vblank_limit; > > > > > > We can drop the variables and directly assign this to psr->... > > > and use > > > it subsequently. > > > Also it would be good to have this done in compute and than just > > > use > > > it here. > > > > > > > + } > > > > + > > > > /* > > > > * Reasons to disable: > > > > * - PSR disabled in new state @@ -2686,6 > > > > +2783,7 @@ > > > > void intel_psr_pre_plane_update(struct intel_atomic_state > > > > *state, > > > > * - Changing between PSR versions > > > > * - Region Early Transport changing > > > > * - Display WA #1136: skl, bxt > > > > + * - Display WA_22019444797 > > > > */ > > > > needs_to_disable |= > > > > intel_crtc_needs_modeset(new_crtc_state); > > > > needs_to_disable |= !new_crtc_state->has_psr; > > > > @@ - > > > > 2695,6 > > > > +2793,10 @@ void intel_psr_pre_plane_update(struct > > > > intel_atomic_state *state, > > > > psr->su_region_et_enabled; > > > > needs_to_disable |= DISPLAY_VER(i915) < 11 && > > > > new_crtc_state->wm_level_disabled; > > > > + /* TODO: Disable PSR1 when vblank gets enabled > > > > while PSR1 is > > > > enabled */ > > > > + needs_to_disable |= DISPLAY_VER(display) == 20 > > > > && > > > > dpkgc_configured && > > > > + (wa_delayed_vblank_limit || > > > > dc5_entry_possible) && > > > > + !new_crtc_state->has_sel_update && > > > > +!new_crtc_state->has_panel_replay; > > > > > > Good to move this to a small helper function which can be > > > extended if > > > required as well. > > > Also seems used in post_plane as well. > > > > > > @Hogander, Jouni Can you also ack if this handling for PSR is ok. > > > > This need_to_disable/keep_disabled is ok. I think there is a bug in > > check itself: > > > > dc5_entry_possible should be !dc5_entry_possible > > > > What do you think? > > > > Dc5_entry_possible returns true when we can enter dc5. > And the condition to disable ps1 is to have a delayed_vblank > 6 or > When dc5 can be entered so that check would be correct "When PKG_C_LATENCY is configured (not all 1s), enable PSR1(SRD_CTL SRD Enable == 1) only when the transcoder has Vblank delayed less than 6 lines OR DC5 can be entered. " I think this emphasizes suggestion from Uma, move this as a helper. Also add explanation there. BR, Jouni Högander > > Regards, > Suraj Kandpal > > > BR, > > > > Jouni Högander > > > > > > > > > if (psr->enabled && needs_to_disable) > > > > intel_psr_disable_locked(intel_dp); > > > > @@ -2735,6 +2837,14 @@ void intel_psr_post_plane_update(struct > > > > intel_atomic_state *state, > > > > keep_disabled |= DISPLAY_VER(display) < 11 && > > > > crtc_state->wm_level_disabled; > > > > > > > > + /* > > > > + * Wa_22019444797 > > > > + * TODO: Disable PSR1 when vblank gets enabled > > > > while PSR1 is > > > > enabled > > > > + */ > > > > + keep_disabled |= DISPLAY_VER(display) == 20 && > > > > psr- > > > > > is_dpkgc_configured && > > > > + (psr->is_wa_delayed_vblank_limit || > > > > psr- > > > > > is_dc5_entry_possible) && > > > > + !crtc_state->has_sel_update && > > > > !crtc_state- > > > > > has_panel_replay; > > > > + > > > > if (!psr->enabled && !keep_disabled) > > > > intel_psr_enable_locked(intel_dp, > > > > crtc_state); > > > > else if (psr->enabled && !crtc_state- > > > > > wm_level_disabled) > > > > -- > > > > 2.43.2 > > > >
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 733de5edcfdb..59c81f0a3abd 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1577,6 +1577,9 @@ struct intel_psr { #define I915_PSR_DEBUG_PANEL_REPLAY_DISABLE 0x40 u32 debug; + bool is_dpkgc_configured; + bool is_dc5_entry_possible; + bool is_wa_delayed_vblank_limit; bool sink_support; bool source_support; bool enabled; diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index b30fa067ce6e..e50b476494a0 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -26,6 +26,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_damage_helper.h> #include <drm/drm_debugfs.h> +#include <drm/drm_vblank.h> #include "i915_drv.h" #include "i915_reg.h" @@ -874,6 +875,78 @@ static u8 psr_compute_idle_frames(struct intel_dp *intel_dp) return idle_frames; } +static bool +intel_psr_check_wa_delayed_vblank(const struct drm_display_mode *adjusted_mode) +{ + return (adjusted_mode->crtc_vblank_start - adjusted_mode->crtc_vdisplay) >= 6; +} + +/* + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and + * VRR is not enabled + */ +static bool intel_psr_is_dpkgc_configured(struct intel_display *display, + struct intel_atomic_state *state) +{ + struct intel_crtc *intel_crtc; + struct intel_crtc_state *crtc_state; + int i; + + if (DISPLAY_VER(display) < 20) + return false; + + for_each_new_intel_crtc_in_state(state, intel_crtc, crtc_state, i) { + if (!intel_crtc->active) + continue; + + if (crtc_state->vrr.enable) + return false; + } + + return true; +} + +/* + * DC5 entry is only possible if vblank interrupt is disabled + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all + * enabled encoders. + */ +static bool +intel_psr_is_dc5_entry_possible(struct intel_display *display, + struct intel_atomic_state *state) +{ + struct intel_crtc *intel_crtc; + struct intel_crtc_state *crtc_state; + int i; + + if ((display->power.domains.target_dc_state & + DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0) + return false; + + for_each_new_intel_crtc_in_state(state, intel_crtc, crtc_state, i) { + struct drm_crtc *crtc = &intel_crtc->base; + struct drm_vblank_crtc *vblank; + struct intel_encoder *encoder; + + if (!intel_crtc->active) + continue; + + vblank = drm_crtc_vblank_crtc(crtc); + + if (vblank->enabled) + return false; + + if (crtc_state->has_psr) + return false; + + for_each_encoder_on_crtc(display->drm, crtc, encoder) + if (encoder->type != INTEL_OUTPUT_EDP) + return false; + } + + return true; +} + static void hsw_activate_psr1(struct intel_dp *intel_dp) { struct intel_display *display = to_intel_display(intel_dp); @@ -986,7 +1059,15 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp) u32 val = EDP_PSR2_ENABLE; u32 psr_val = 0; - val |= EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); + /* + * Wa_22019444797 + * TODO: Disable idle frames when vblank gets enabled while + * PSR2 is enabled + */ + if (DISPLAY_VER(dev_priv) != 20 || + !intel_dp->psr.is_dpkgc_configured || + intel_dp->psr.is_dc5_entry_possible) + val |= EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp)); if (DISPLAY_VER(display) < 14 && !IS_ALDERLAKE_P(dev_priv)) val |= EDP_SU_TRACK_ENABLE; @@ -2667,10 +2748,20 @@ void intel_psr_pre_plane_update(struct intel_atomic_state *state, const struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); struct intel_encoder *encoder; + bool dpkgc_configured = false, dc5_entry_possible = false; + bool wa_delayed_vblank_limit = false; if (!HAS_PSR(display)) return; + if (DISPLAY_VER(display) == 20) { + dpkgc_configured = intel_psr_is_dpkgc_configured(display, state); + dc5_entry_possible = + intel_psr_is_dc5_entry_possible(display, state); + wa_delayed_vblank_limit = + intel_psr_check_wa_delayed_vblank(&new_crtc_state->hw.adjusted_mode); + } + for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, old_crtc_state->uapi.encoder_mask) { struct intel_dp *intel_dp = enc_to_intel_dp(encoder); @@ -2679,6 +2770,12 @@ void intel_psr_pre_plane_update(struct intel_atomic_state *state, mutex_lock(&psr->lock); + if (DISPLAY_VER(i915) == 20) { + psr->is_dpkgc_configured = dpkgc_configured; + psr->is_dc5_entry_possible = dc5_entry_possible; + psr->is_wa_delayed_vblank_limit = wa_delayed_vblank_limit; + } + /* * Reasons to disable: * - PSR disabled in new state @@ -2686,6 +2783,7 @@ void intel_psr_pre_plane_update(struct intel_atomic_state *state, * - Changing between PSR versions * - Region Early Transport changing * - Display WA #1136: skl, bxt + * - Display WA_22019444797 */ needs_to_disable |= intel_crtc_needs_modeset(new_crtc_state); needs_to_disable |= !new_crtc_state->has_psr; @@ -2695,6 +2793,10 @@ void intel_psr_pre_plane_update(struct intel_atomic_state *state, psr->su_region_et_enabled; needs_to_disable |= DISPLAY_VER(i915) < 11 && new_crtc_state->wm_level_disabled; + /* TODO: Disable PSR1 when vblank gets enabled while PSR1 is enabled */ + needs_to_disable |= DISPLAY_VER(display) == 20 && dpkgc_configured && + (wa_delayed_vblank_limit || dc5_entry_possible) && + !new_crtc_state->has_sel_update && !new_crtc_state->has_panel_replay; if (psr->enabled && needs_to_disable) intel_psr_disable_locked(intel_dp); @@ -2735,6 +2837,14 @@ void intel_psr_post_plane_update(struct intel_atomic_state *state, keep_disabled |= DISPLAY_VER(display) < 11 && crtc_state->wm_level_disabled; + /* + * Wa_22019444797 + * TODO: Disable PSR1 when vblank gets enabled while PSR1 is enabled + */ + keep_disabled |= DISPLAY_VER(display) == 20 && psr->is_dpkgc_configured && + (psr->is_wa_delayed_vblank_limit || psr->is_dc5_entry_possible) && + !crtc_state->has_sel_update && !crtc_state->has_panel_replay; + if (!psr->enabled && !keep_disabled) intel_psr_enable_locked(intel_dp, crtc_state); else if (psr->enabled && !crtc_state->wm_level_disabled)
To reach PC10 when PKG_C_LATENCY is configure we must do the following things 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be entered 2) Allow PSR2 deep sleep when DC5 can be entered 3) DC5 can be entered when all transocoder have either PSR1, PSR2 or eDP 1.5 PR ALPM enabled and VBI is disabled and flips and pushes are not happening. --v2 -Switch condition and do an early return [Jani] -Do some checks in compute_config [Jani] -Do not use register reads as a method of checking states for DPKGC or delayed vblank [Jani] -Use another way to see is vblank interrupts are disabled or not [Jani] --v3 -Use has_psr to check if psr can be enabled or not for dc5_entry cond [Uma] -Move the dc5 entry computation to psr_compute_config [Jouni] -No need to change sequence of enabled and activate, so dont make hsw_psr1_activate return anything [Jouni] -Use has_psr to stop psr1 activation [Jouni] -Use lineage no. in WA -Add the display ver restrictions for WA --v4 -use more appropriate name for check_vblank_limit() [Jouni] -Cover the case for idle frames when dpkgc is not configured [Jouni] -Check psr only for edp [Jouni] --v5 -move psr1 handling to plane update [Jouni] -add todo for cases when vblank is enabled when psr enabled [Jouni] -use intel_display instead of drm_i915_private --v6 -check target_dc_state [Jouni] -fix condition in pre/post plane update [Jouni] WA: 22019444797 Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- .../drm/i915/display/intel_display_types.h | 3 + drivers/gpu/drm/i915/display/intel_psr.c | 112 +++++++++++++++++- 2 files changed, 114 insertions(+), 1 deletion(-)