[PATCH 2/6] drm/i915/display: use wakelock in the remaining read operations

[Gustavo Sousa]

Quoting Luca Coelho (2024-02-07 07:30:03-03:00)
>In the patch where the framework is implemented, we only add the
>wakelock usage in the read-modify-write operations.  This patch brings
>in the wakelock implementation to the remaining read operations.  The
>idea is to keep these in separate blocks to ease bisections in case
>there are any issues.

Hm, while I understand the motivation, I can't help but think that we
are making the first patch somewhat incomplete. For example, if we were
to pick up the first patch and then have a new one on top enabling the
DMC Wakelock, we would have code paths in the display code that would
not grab the wakelock.

Furthermore, not sure if we would have bisection really working here
(without further instrumentation), because the wakelock feature is only
enabled later on the series.

>
>Signed-off-by: Luca Coelho <luciano.coelho at intel.com>
>---
> drivers/gpu/drm/i915/display/intel_de.h | 38 +++++++++++++++++++++++--
> 1 file changed, 35 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
>index ef24748d522d..0bf73b1e1cfe 100644
>--- a/drivers/gpu/drm/i915/display/intel_de.h
>+++ b/drivers/gpu/drm/i915/display/intel_de.h
>@@ -13,26 +13,58 @@
> static inline u32
> intel_de_read(struct drm_i915_private *i915, i915_reg_t reg)
> {
>-        return intel_uncore_read(&i915->uncore, reg);
>+        u32 val;
>+
>+        intel_dmc_wl_get(i915, reg);
>+
>+        val = intel_uncore_read(&i915->uncore, reg);
>+
>+        intel_dmc_wl_put(i915, reg);
>+
>+        return val;
> }
> 
> static inline u8
> intel_de_read8(struct drm_i915_private *i915, i915_reg_t reg)
> {
>-        return intel_uncore_read8(&i915->uncore, reg);
>+        u8 val;
>+
>+        intel_dmc_wl_get(i915, reg);
>+
>+        val = intel_uncore_read8(&i915->uncore, reg);
>+
>+        intel_dmc_wl_put(i915, reg);
>+
>+        return val;
> }
> 
> static inline u64
> intel_de_read64_2x32(struct drm_i915_private *i915,
>                      i915_reg_t lower_reg, i915_reg_t upper_reg)
> {
>-        return intel_uncore_read64_2x32(&i915->uncore, lower_reg, upper_reg);
>+        u64 val;
>+
>+        /* Both lower and upper registers will almost certainly be in
>+         * the same range (and very likely one immediately after the
>+         * other), so we can check only the lower one.
>+         */

The words "almost certainly" and "very likely" concern me :-)

I think it doesn't really hurt to also intel_dmc_wl_get(i915, upper_reg)
here, since it would actually just bump the refcount, no?

--
Gustavo Sousa

>+        intel_dmc_wl_get(i915, lower_reg);
>+
>+        val = intel_uncore_read64_2x32(&i915->uncore, lower_reg, upper_reg);
>+
>+        intel_dmc_wl_put(i915, lower_reg);
>+
>+        return val;
> }
> 
> static inline void
> intel_de_posting_read(struct drm_i915_private *i915, i915_reg_t reg)
> {
>+        intel_dmc_wl_get(i915, reg);
>+
>         intel_uncore_posting_read(&i915->uncore, reg);
>+
>+        intel_dmc_wl_put(i915, reg);
> }
> 
> static inline void
>-- 
>2.39.2
>