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 <[email protected]> > > > > > > > > --- > > > > > > > > .../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 > > >
