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

["Vivekanandan, Balasubramani" <balasubramani.vivekanandan@intel.com>]

On 17.01.2025 19:06, Gustavo Sousa 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.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>


Looks good to me.

Reviewed-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>

Regards,
Bala

> ---
>  .../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)
> -- 
> 2.48.0
>