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 Luca Coelho (2025-01-22 07:24:43-03:00)
>On Fri, 2025-01-17 at 19:06 -0300, 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.  With a small optional nitpick below.
>
>Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
>
>[...]
>> 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
>> @@ -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;
>
>Why not keep this as it was...

Yeah, I suppose that's fine... I think the compiler is going to optimize
it. I can send a v2 with this change.

>
>>          unsigned long flags;
>>  
>> +        if (!display->params.enable_dmc_wl_debugfs)
>> +                return false;
>> +
>> +        ret = false;
>> +
>
>...then you don't need to set it here, and can return ret in the if
>above for consistency.

In the if above, I guess I prefer the "return false" because it is
explicit.

--
Gustavo Sousa

>
>--
>Cheers,
>Luca.