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@xxxxxxxxx>
>> ---
>
>Looks good. With a small optional nitpick below.
>
>Reviewed-by: Luca Coelho <luciano.coelho@xxxxxxxxx>
>
>[...]
>> 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.
