Re: [PATCH 4/4] drm/i915/dmc_wl: Enable the debugfs only with enable_dmc_wl_debugfs=1

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Jani Nikula (2025-01-27 10:35:57-03:00)
>On Mon, 27 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> Quoting Jani Nikula (2025-01-27 09:01:39-03:00)
>>>On Fri, 17 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> 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?

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@intel.com>
>>>> ---
>>>>  .../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