On Wed, Jul 09, 2025 at 06:11:17PM +0000, Hogander, Jouni wrote:
> On Wed, 2025-07-09 at 20:03 +0300, Ville Syrjälä wrote:
> > On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander wrote:
> > > Currently disabling PSR2 via enable_psr module parameter causes
> > > Panel
> > > Replay being disabled as well. This patch changes this by still
> > > allowing
> > > Panel Replay even if PSR2 is disabled.
> > >
> > > After this patch enable_psr module parameter values are:
> > >
> > > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> > > 0 = PSR1 : no, PSR2 = no, Panel Replay : no
> > > 1 = PSR1 : yes, PSR2 = no, Panel Replay : yes
> > > 2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> > > 3 = PSR1 : yes, PSR2 = no, Panel Replay : no
> > >
> > > I.e. values different than -1 and 0 are handled as bitmasks where
> > > BIT0
> > > disables PSR2 and BIT1 disables Panel Replay.
> > >
> > > v2:
> > > - make it more clear that enable_psr is bitmask for disabling
> > > different
> > > PSR modes
> > >
> > > Signed-off-by: Jouni Högander <jouni.hogan...@intel.com>
> > > ---
> > > .../drm/i915/display/intel_display_params.c | 6 ++---
> > > drivers/gpu/drm/i915/display/intel_psr.c | 22 ++++++++++++++-
> > > ----
> > > 2 files changed, 19 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > index 75316247ee8a..195af19ece5f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > @@ -116,9 +116,9 @@ intel_display_param_named_unsafe(enable_fbc,
> > > int, 0400,
> > > "(default: -1 (use per-chip default))");
> > >
> > > intel_display_param_named_unsafe(enable_psr, int, 0400,
> > > - "Enable PSR "
> > > - "(0=disabled, 1=enable up to PSR1, 2=enable up to PSR2) "
> > > - "Default: -1 (use per-chip default)");
> > > + "Enable PSR (0=disabled, 1=disable PSR2 (BIT0), 2=disable
> > > Panel Replay (BIT1))."
> > > + "Values different from 0 and -1 are handled as bitmask to
> > > disable different PSR modes."
> > > + "E.g. value 3 disables both PSR2 and Panel Replay.
> > > Default: -1 (use per-chip default)");
> >
> > This thing is very unintuitive. Why don't we just get replace it
> > with a new disable_psr modparam that is clearly just a bitmask of
> > what to disable?
>
> I was thinkinig we should keep it backward compatible. I know this
> parameter is in use.
I agree on keeping this backward compatible.
Also our experience with disable_power_well shows that negative
name in the parameter can be much more unintuitive and confusing.
>
> BR,
>
> Jouni Högander
>
> > >
> > > intel_display_param_named(psr_safest_params, bool, 0400,
> > > "Replace PSR VBT parameters by the safest and not optimal
> > > ones. This "
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index a2b5688f0c82..959b868672d0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -254,13 +254,16 @@ static bool psr2_global_enabled(struct
> > > intel_dp *intel_dp)
> > > {
> > > struct intel_display *display =
> > > to_intel_display(intel_dp);
> > >
> > > + return display->params.enable_psr == -1 ||
> > > + !(display->params.enable_psr & 0x1);
> > > +}
> > > +
> > > +static bool sel_update_global_enabled(struct intel_dp *intel_dp)
> > > +{
> > > switch (intel_dp->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
> > > - case I915_PSR_DEBUG_DISABLE:
> > > case I915_PSR_DEBUG_FORCE_PSR1:
> > > return false;
> > > default:
> > > - if (display->params.enable_psr == 1)
> > > - return false;
> > > return true;
> > > }
> > > }
> > > @@ -269,7 +272,8 @@ static bool panel_replay_global_enabled(struct
> > > intel_dp *intel_dp)
> > > {
> > > struct intel_display *display =
> > > to_intel_display(intel_dp);
> > >
> > > - if ((display->params.enable_psr != -1) ||
> > > + if ((display->params.enable_psr != -1 &&
> > > + display->params.enable_psr & 0x2) ||
> > > (intel_dp->psr.debug &
> > > I915_PSR_DEBUG_PANEL_REPLAY_DISABLE))
> > > return false;
> > > return true;
> > > @@ -1415,6 +1419,12 @@ static bool intel_psr2_config_valid(struct
> > > intel_dp *intel_dp,
> > > if (!intel_dp->psr.sink_psr2_support)
> > > return false;
> > >
> > > + if (!psr2_global_enabled(intel_dp)) {
> > > + drm_dbg_kms(display->drm,
> > > + "PSR2 disabled by flag\n");
> > > + return false;
> > > + }
> > > +
> > > /* JSL and EHL only supports eDP 1.3 */
> > > if (display->platform.jasperlake || display-
> > > >platform.elkhartlake) {
> > > drm_dbg_kms(display->drm, "PSR2 not supported by
> > > phy\n");
> > > @@ -1517,7 +1527,7 @@ static bool
> > > intel_sel_update_config_valid(struct intel_dp *intel_dp,
> > > goto unsupported;
> > > }
> > >
> > > - if (!psr2_global_enabled(intel_dp)) {
> > > + if (!sel_update_global_enabled(intel_dp)) {
> > > drm_dbg_kms(display->drm,
> > > "Selective update disabled by
> > > flag\n");
> > > goto unsupported;
> > > @@ -1664,7 +1674,7 @@ void intel_psr_compute_config(struct intel_dp
> > > *intel_dp,
> > > u8 active_pipes = 0;
> > >
> > > if (!psr_global_enabled(intel_dp)) {
> > > - drm_dbg_kms(display->drm, "PSR disabled by
> > > flag\n");
> > > + drm_dbg_kms(display->drm, "PSR/Panel Replay
> > > disabled by flag\n");
> > > return;
> > > }
> > >
> > > --
> > > 2.43.0
> >
>
