Re: [PATCH 13/13] drm/i915/xe3lpd: Use DMC wakelock by default
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 > > > > --- > > > > 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.
Re: [PATCH 13/13] drm/i915/xe3lpd: Use DMC wakelock by default
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 >>> --- >>> 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
Re: [PATCH 13/13] drm/i915/xe3lpd: Use DMC wakelock by default
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 >> --- >> 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
Re: [PATCH 13/13] drm/i915/xe3lpd: Use DMC wakelock by default
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 > --- > 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.
