On Mon, 27 Jan 2025, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote:
> Quoting Jani Nikula (2025-01-27 10:35:57-03:00)
>>On Mon, 27 Jan 2025, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote:
>>> Quoting Jani Nikula (2025-01-27 09:01:39-03:00)
>>>>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?
>>>
>>> Hm... I'm failing to see how this would keep us from having to take the
>>> spinlock once we have the pointer being non-NULL.
>>>
>>> The idea of the parameter is to protect us from taking the spinlock when
>>> we are not debugging DMC wakelock offsets.
>>
>>If you only allocate and assign the pointer when you enable the feature
>>via debugfs, wouldn't that achieve the goal?
>
> But then how are we going to protect ourselves from races when checking
> the pointer for NULL-ness?
If you don't allow dynamically disabling and freeing the stuff, except
at takedown, where's the race?
BR,
Jani.
>
> Maybe I'm missing some technical background here...
>
> Is there a way to atomically do that without a lock?
>
> Could RCU (which I still need to learn) help somehow here?
>
> --
> Gustavo Sousa
>
>>
>>BR,
>>Jani.
>>
>>>
>>> --
>>> Gustavo Sousa
>>>
>>>>
>>>>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
>>
>>--
>>Jani Nikula, Intel
--
Jani Nikula, Intel
