On Mon, 27 Jan 2025, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> On Fri, 17 Jan 2025, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote:
>> We use a spinlock to protect DMC wakelock debugfs data, since it is also
>> accessed by the core DMC wakelock logic. Taking the spinlock when the
>> debugfs is not in use introduces a small but unnecessary penalty.
>>
>> Since the debugfs functionality is only expected to be used for, uh,
>> debugging sessions, let's protect it behind a module parameter
>> enable_dmc_wl_debugfs. That way, we only take the lock if the feature
>> was enabled in the first place.
>
> If the debug struct were an opaque pointer, you could check for that
> being != NULL. Register the debugfs always, and have that initialize
> everything as needed?
...the underlying point being that I'm generally against adding any new
module parameters.
BR,
Jani.
>
> BR,
> Jani.
>
>>
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
>> ---
>> .../gpu/drm/i915/display/intel_display_params.c | 5 +++++
>> .../gpu/drm/i915/display/intel_display_params.h | 1 +
>> .../gpu/drm/i915/display/intel_dmc_wl_debugfs.c | 16 +++++++++++++++-
>> 3 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
>> index c4f1ab43fc0c..bc36d1b0ef87 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_params.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
>> @@ -133,6 +133,11 @@ intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
>> "(-1=use per-chip default, 0=disabled, 1=enabled, 2=match any register, 3=always locked) "
>> "Default: -1");
>>
>> +intel_display_param_named_unsafe(enable_dmc_wl_debugfs, bool, 0400,
>> + "Enable DMC wakelock debugfs"
>> + "(0=disabled, 1=enabled) "
>> + "Default: 0");
>> +
>> __maybe_unused
>> static void _param_print_bool(struct drm_printer *p, const char *driver_name,
>> const char *name, bool val)
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h
>> index 5317138e6044..cb7dc1bc6846 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_params.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_params.h
>> @@ -48,6 +48,7 @@ struct drm_printer;
>> param(bool, psr_safest_params, false, 0400) \
>> param(bool, enable_psr2_sel_fetch, true, 0400) \
>> param(int, enable_dmc_wl, -1, 0400) \
>> + param(bool, enable_dmc_wl_debugfs, false, 0400) \
>>
>> #define MEMBER(T, member, ...) T member;
>> struct intel_display_params {
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c b/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c
>> index 1493d296ac98..f4e4c7a5a730 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl_debugfs.c
>> @@ -37,6 +37,9 @@
>> * which exports a buffer of untracked register offsets and also allows extra
>> * register offsets to be tracked by the driver.
>> *
>> + * The debugfs directory is only exported if the module parameter
>> + * enable_dmc_wl_debugfs=1 is passed.
>> + *
>> * Untracked offsets
>> * -----------------
>> *
>> @@ -411,6 +414,9 @@ void intel_dmc_wl_debugfs_register(struct intel_display *display)
>> {
>> struct dentry *dir;
>>
>> + if (!display->params.enable_dmc_wl_debugfs)
>> + return;
>> +
>> if (!HAS_DMC_WAKELOCK(display))
>> return;
>>
>> @@ -453,6 +459,9 @@ void intel_dmc_wl_debugfs_log_untracked(struct intel_display *display, u32 offse
>> struct intel_dmc_wl_dbg *dbg = &display->wl.dbg;
>> unsigned long flags;
>>
>> + if (!display->params.enable_dmc_wl_debugfs)
>> + return;
>> +
>> spin_lock_irqsave(&dbg->lock, flags);
>>
>> if (!dbg->untracked.size)
>> @@ -479,9 +488,14 @@ void intel_dmc_wl_debugfs_log_untracked(struct intel_display *display, u32 offse
>> bool intel_dmc_wl_debugfs_offset_in_extra_ranges(struct intel_display *display, u32 offset)
>> {
>> struct intel_dmc_wl_dbg *dbg = &display->wl.dbg;
>> - bool ret = false;
>> + bool ret;
>> unsigned long flags;
>>
>> + if (!display->params.enable_dmc_wl_debugfs)
>> + return false;
>> +
>> + ret = false;
>> +
>> spin_lock_irqsave(&dbg->lock, flags);
>>
>> if (!dbg->extra_ranges)
--
Jani Nikula, Intel
