[PATCH 13/13] drm/i915/xe3lpd: Use DMC wakelock by default

[Luca Coelho]

On Tue, 2024-11-05 at 18:12 -0300, Gustavo Sousa wrote:
> Quoting Gustavo Sousa (2024-11-05 10:46:52-03:00)
> > Quoting Luca Coelho (2024-11-01 11:27:10-03:00)
> > > On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> > > > Although Bspec doesn't explicitly mentions that, as of Xe3_LPD, using
> > > > DMC wakelock is the officially recommended way of accessing registers
> > > > that would be off during DC5/DC6 and the legacy method (where the DMC
> > > > intercepts MMIO to wake up the hardware) is to be avoided.
> > > > 
> > > > As such, update the driver to use the DMC wakelock by default starting
> > > > with Xe3_LPD. Since the feature is somewhat new to the driver, also
> > > > allow disabling it via a module parameter for debugging purposes.
> > > > 
> > > > For that, make the existing parameter allow values -1 (per-chip
> > > > default), 0 (disabled) and 1 (enabled), similarly to what is done for
> > > > other parameters.
> > > > 
> > > > Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display_params.c | 4 ++--
> > > >  drivers/gpu/drm/i915/display/intel_display_params.h | 2 +-
> > > >  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 6 +++++-
> > > >  3 files changed, 8 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > index 024de8abcb1a..bf00e5f1f145 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > @@ -123,10 +123,10 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
> > > >          "(0=disabled, 1=enabled) "
> > > >          "Default: 1");
> > > >  
> > > > -intel_display_param_named_unsafe(enable_dmc_wl, bool, 0400,
> > > > +intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
> > > >          "Enable DMC wakelock "
> > > >          "(0=disabled, 1=enabled) "
> > > > -        "Default: 0");
> > > > +        "Default: -1 (use per-chip default)");
> > > 
> > > We're already explaining the possible values in the previous
> > > parentheses, so maybe the -1 should also be explained there?
> > 
> > Yep that makes sense. I was following the trend of what was done for
> > enable_fbc and enable_psr, but I guess following other examples in this
> > same file where we tag the default one with "[default]" is better.
> 
> Ended up simply doing this:
> 
>     diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
>     index bf00e5f1f145..dc666aefa362 100644
>     --- a/drivers/gpu/drm/i915/display/intel_display_params.c
>     +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
>     @@ -125,8 +125,8 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
>      
>      intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
>      	"Enable DMC wakelock "
>     -	"(0=disabled, 1=enabled) "
>     -	"Default: -1 (use per-chip default)");
>     +	"(-1=use per-chip default, 0=disabled, 1=enabled) "
>     +	"Default: -1");
>      
>      __maybe_unused
>      static void _param_print_bool(struct drm_printer *p, const char *driver_name,
> 
> , because repeating the word "default" in "(-1=use per-chip default
> [default], 0=disabled, 1=enabled)" looked weird.

Yep, this looks good!

--
Cheers,
Luca.