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

[Gustavo Sousa]

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.

>
>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/ ?

>+            !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.

--
Gustavo Sousa

>                 return;
> 
>         spin_lock_irqsave(&wl->lock, flags);
>-- 
>2.39.2
>