| Message ID | 20241021222744.294371-14-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD | expand |
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@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? -- Cheers, Luca.
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@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. Thanks! I'll update this on the next version. -- Gustavo Sousa
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@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. -- Gustavo Sousa > >Thanks! I'll update this on the next version. > >-- >Gustavo Sousa
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@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.
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)"); __maybe_unused static void _param_print_bool(struct drm_printer *p, const char *driver_name, diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h index dcb6face936a..5317138e6044 100644 --- a/drivers/gpu/drm/i915/display/intel_display_params.h +++ b/drivers/gpu/drm/i915/display/intel_display_params.h @@ -47,7 +47,7 @@ struct drm_printer; param(int, enable_psr, -1, 0600) \ param(bool, psr_safest_params, false, 0400) \ param(bool, enable_psr2_sel_fetch, true, 0400) \ - param(bool, enable_dmc_wl, false, 0400) \ + param(int, enable_dmc_wl, -1, 0400) \ #define MEMBER(T, member, ...) T member; struct intel_display_params { diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c index 55f07f3c9863..f58031811e79 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c @@ -258,7 +258,11 @@ static bool __intel_dmc_wl_supported(struct intel_display *display) static void intel_dmc_wl_sanitize_param(struct intel_display *display) { if (!HAS_DMC_WAKELOCK(display)) - display->params.enable_dmc_wl = false; + display->params.enable_dmc_wl = 0; + else if (display->params.enable_dmc_wl >= 0) + display->params.enable_dmc_wl = !!display->params.enable_dmc_wl; + else + display->params.enable_dmc_wl = DISPLAY_VER(display) >= 30; drm_dbg_kms(display->drm, "Sanitized enable_dmc_wl value: %d\n", display->params.enable_dmc_wl);
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@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(-)