[PATCH 5/6] drm/i915/display: add module parameter to enable DMC wakelock

[Luca Coelho]

On Tue, 2024-02-20 at 13:46 -0300, Gustavo Sousa wrote:
> Quoting Luca Coelho (2024-02-07 07:30:06-03:00)
> > This feature should be disabled by default until properly tested and
> > mature.  Add a module parameter to enable the feature for testing,
> > while keeping it disabled by default for now.
> > 
> > Additionally, only allow enabling via module parameter if the display
> > version is greater than 20.
> 
> I would have two separate patches here, one for the display version
> check and another for the module parameter, since we are dealing with
> two different concepts.

Done.


> > Signed-off-by: Luca Coelho <luciano.coelho at intel.com>
> > ---
> > .../gpu/drm/i915/display/intel_display_params.c  |  5 +++++
> > .../gpu/drm/i915/display/intel_display_params.h  |  1 +
> > drivers/gpu/drm/i915/display/intel_dmc_wl.c      | 16 ++++++++++++++--
> > 3 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
> > index 11e03cfb774d..f40b223cc8a1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > @@ -116,6 +116,11 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
> >         "(0=disabled, 1=enabled) "
> >         "Default: 1");
> > 
> > +intel_display_param_named_unsafe(enable_dmc_wl, bool, 0400,
> > +        "Enable DMC wakelock "
> > +        "(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 6206cc51df04..bf8dbbdb20a1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_params.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_params.h
> > @@ -46,6 +46,7 @@ struct drm_i915_private;
> >         param(int, enable_psr, -1, 0600) \
> >         param(bool, psr_safest_params, false, 0400) \
> >         param(bool, enable_psr2_sel_fetch, true, 0400) \
> > +        param(bool, enable_dmc_wl, false, 0400) \
> > 
> > #define MEMBER(T, member, ...) T member;
> > struct intel_display_params {
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > index abe1ea92cf65..21ab67805ff1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > @@ -79,6 +79,10 @@ void intel_dmc_wl_enable(struct drm_i915_private *i915)
> >         struct intel_dmc_wl *wl = &i915->display.wl;
> >         unsigned long flags;
> > 
> > +        if (!i915->display.params.enable_dmc_wl ||
> > +            DISPLAY_VER(i915) <= 20)
> 
> s/<= 20/< 20/ ?
> 
> > +                return;
> > +
> >         spin_lock_irqsave(&wl->lock, flags);
> > 
> >         if (wl->enabled)
> > @@ -105,6 +109,10 @@ void intel_dmc_wl_disable(struct drm_i915_private *i915)
> >         struct intel_dmc_wl *wl = &i915->display.wl;
> >         unsigned long flags;
> > 
> > +        if (!i915->display.params.enable_dmc_wl ||
> > +            DISPLAY_VER(i915) <= 20)
> 
> s/<= 20/< 20/ ?
> 
> > +                return;
> > +
> >         spin_lock_irqsave(&wl->lock, flags);
> > 
> >         /* FIXME: should we cancel the work? */
> > @@ -135,7 +143,9 @@ void intel_dmc_wl_get(struct drm_i915_private *i915, i915_reg_t reg)
> >         unsigned long flags;
> >         u32 ctl_value;
> > 
> > -        if (!intel_dmc_wl_check_range(reg.reg))
> > +        if (!i915->display.params.enable_dmc_wl ||
> > +            DISPLAY_VER(i915) <= 20 ||
> 
> s/<= 20/< 20/ ?

Right, I fixed all these.


> > +            !intel_dmc_wl_check_range(reg.reg))
> 
> I would keep this condition line into its own "if" statement, just
> because the first two conditions are like "is this thing enabled?" and
> this last one is like "do we need the lock for this reg?".
> 
> >                 return;
> > 
> >         spin_lock_irqsave(&wl->lock, flags);
> > @@ -166,7 +176,9 @@ void intel_dmc_wl_put(struct drm_i915_private *i915, i915_reg_t reg)
> >         struct intel_dmc_wl *wl = &i915->display.wl;
> >         unsigned long flags;
> > 
> > -        if (!intel_dmc_wl_check_range(reg.reg))
> > +        if (!i915->display.params.enable_dmc_wl ||
> > +            DISPLAY_VER(i915) <= 20 ||
> > +            !intel_dmc_wl_check_range(reg.reg))
> 
> Ditto.

Done in both places.

--
Cheers,
Luca.