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@xxxxxxxxx> > > > > --- > > > > 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.
