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

[Luca Coelho]

On Tue, 2024-02-20 at 13:45 -0300, Gustavo Sousa wrote:
> 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.

Yeah, you're totally right.  It seemed to make more sense when I
started the implementation, before moving a lot of stuff around, but
now it should just be squashed.


> > 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 :-)

Okay, my choice of words was too soft, apparently.  In more direct
words, I don't see any reason for two 32-bit reads to be combined into
one 64-bit read, unless  the two 32-bit reads are contiguous.  And this
is the case in the two places where this function is currently used,
namely in i915_get_vblank_counter() and in intel_vrr_get_config().

I also can't see why these two "half-registers" would be in separate
power wells.  But then again, I don't see anything explicitly
forbidding this to happen in the code, so...


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

...I'll check the upper reg as you suggest as well. :)

--
Cheers,
Luca.