On Thu, 2025-01-23 at 13:10 -0300, Gustavo Sousa wrote:
> 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.
Yeah, fair enough. It's a matter of preference.
--
Cheers,
Luca.
