Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jul 11, 2025 at 10:33:58AM +0000, Hogander, Jouni wrote:
> On Fri, 2025-07-11 at 02:11 +0300, Ville Syrjälä wrote:
> > On Thu, Jul 10, 2025 at 05:27:13PM -0400, Rodrigo Vivi wrote:
> > > On Thu, Jul 10, 2025 at 11:09:42PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Jul 10, 2025 at 11:42:52AM -0400, Rodrigo Vivi wrote:
> > > > > 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.hogander@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.
> > > > 
> > > > IMO it's an unusable mess so I wouldn't bother trying to preserve
> > > > it.
> > > > The only value that seems to make any sense currently is =0. 
> > > 
> > > fair enough. what about simply removing all the options entirely?
> > > enable_psr=0 keeps disabling it, otherwise enabled it. And we
> > > reduce
> > > all the knobs option. Afterall, this should be our end goal anyway.
> > > 
> > > > If I
> > > > need to use any other value I always give up immediately and just
> > > > hack the code instead.
> > > 
> > > Well, the param actually exists for us to request reporters to try
> > > different config. The devs can always modify the code.
> > > 
> > > Question now is, are all these variants useful for collecting debug
> > > information of some sort?
> > > 
> > > If so, as long as it is documented and we can ask different values,
> > > we should be good.
> > > 
> > > > 
> > > > If we keep calling it 'enable_psr' then it should clearly be a
> > > > bitmask of things to *enable*, not things to *disable*.
> > > > 
> > > > > 
> > > > > Also our experience with disable_power_well shows that negative
> > > > > name in the parameter can be much more unintuitive and
> > > > > confusing.
> > > > 
> > > > That one is rather different because it doesn't "disable power
> > > > wells"
> > > > but rather it "disables power well disabling". But yes, it is a
> > > > very
> > > > poor name as well.
> > > > 
> > > > Calling it "enable_power_wells" wouldn't really help though.
> > > > It should perhaps be something more like
> > > > 'dont_disable_power_wells'
> > > > or 'keep_power_wells_on'.
> > > 
> > > okay, fair enough, disable power well is another level of
> > > complication.
> > > 
> > > back to disable_psr idea:
> > > 
> > > disable_psr=0 == enable PSR? to me this is already uninituitive
> > > anyway.
> > > disable_psr=1 == disable PSR1?
> > > disable_psr=2 == disable PSR2? and keep only PSR=1?
> > > 
> > > I still don't see a clean obvious intuitive way of handling it.
> > > Perhaps what I had suggested another day:
> > > 
> > > PSR1 = BIT0
> > > PSR2 = BIT1 (PSR2 infers PSR1 enabled)
> > > PANEL_REPLAY = BIT2 (also infers PSR1(and 2?) enabled)
> > 
> > With a bitmask I don't think inferring anything is helpful.
> > If the corresponding bit isn't set then don't use that
> > mode, period.
> > 
> > Another option would to have a separate named parameter
> > for each mode. Would be easier to understand but dunno
> > if we really want to add that many modparams just for this.
> 
> I'm now thinking adding enable_panel_replay would make most sense:
> 
> -1 : Enable chip default (Default)
>  0 : Disable
>  1 : Enable PR full frame update
> 
> Keep enable_psr as it is and remove all bindings to Panel Replay from
> there. What do you think?

yeap, perhaps it is the easiest and more intuintive way...

> 
> BR,
> 
> Jouni Högander
> 
> > 
> > > (Peraps even bit3 for early transport?)
> > > 
> > > This is backwards compatible because
> > > 
> > > 0 = disabled,
> > > 1 = up to psr1,
> > > 2 = up to psr2, (no panel replay)
> > > 3 = up to psr2, (same as 2)
> > > 4 = panel replay on
> > > ...
> > > 
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > 
>