On Wed, 2023-10-25 at 11:25 +0100, Tvrtko Ursulin wrote: > On 25/10/2023 11:18, Tvrtko Ursulin wrote: > > > > On 23/10/2023 11:33, Luca Coelho wrote: > > > The uncore code may not always be available (e.g. when we build the > > > display code with Xe), so we can't always rely on having the uncore's > > > spinlock. > > > > > > To handle this, split the spin_lock/unlock_irqsave/restore() into > > > spin_lock/unlock() followed by a call to local_irq_save/restore() and > > > create wrapper functions for locking and unlocking the uncore's > > > spinlock. In these functions, we have a condition check and only > > > actually try to lock/unlock the spinlock when I915 is defined, and > > > thus uncore is available. > > > > > > This keeps the ifdefs contained in these new functions and all such > > > logic inside the display code. > > > > > > Signed-off-by: Luca Coelho <luciano.coelho@xxxxxxxxx> > > > --- > > > > > > In v2: > > > > > > * Renamed uncore_spin_*() to intel_spin_*() > > > * Corrected the order: save, lock, unlock, restore > > > > > > In v3: > > > > > > * Undid the change to pass drm_i915_private instead of the lock > > > itself, since we would have to include i915_drv.h and that pulls > > > in a truckload of other includes. > > > > > > drivers/gpu/drm/i915/display/intel_display.h | 20 ++++++++++++++++++++ > > > drivers/gpu/drm/i915/display/intel_vblank.c | 19 ++++++++++++------- > > > 2 files changed, 32 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h > > > b/drivers/gpu/drm/i915/display/intel_display.h > > > index 0e5dffe8f018..2a33fcc8ce68 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display.h > > > @@ -559,4 +559,24 @@ bool assert_port_valid(struct drm_i915_private > > > *i915, enum port port); > > > bool intel_scanout_needs_vtd_wa(struct drm_i915_private *i915); > > > +/* > > > + * The uncore version of the spin lock functions is used to decide > > > + * whether we need to lock the uncore lock or not. This is only > > > + * needed in i915, not in Xe. Keep the decision-making centralized > > > + * here. > > > + */ > > > +static inline void intel_spin_lock(spinlock_t *lock) > > > +{ > > > +#ifdef I915 > > > + spin_lock(lock); > > > +#endif > > > +} > > > + > > > +static inline void intel_spin_unlock(spinlock_t *lock) > > > +{ > > > +#ifdef I915 > > > + spin_unlock(lock); > > > +#endif > > > +} > > > + > > > #endif > > > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c > > > b/drivers/gpu/drm/i915/display/intel_vblank.c > > > index 2cec2abf9746..9b482d648762 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_vblank.c > > > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c > > > @@ -306,7 +306,8 @@ static bool i915_get_crtc_scanoutpos(struct > > > drm_crtc *_crtc, > > > * register reads, potentially with preemption disabled, so the > > > * following code must not block on uncore.lock. > > > */ > > > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > + local_irq_save(irqflags); > > > > Does Xe needs interrupts off? I'm actually not sure, but this is how it was in the Xe driver code, so I kept it. > > > + intel_spin_lock(&dev_priv->uncore.lock); > > > > My 2p/c is that intel_spin_lock as a name does not work when it is > > specifically about the single and specific (uncore) lock. One cannot > > call intel_spin_lock(some->other->lock) etc. Right, this was changed when I was passing only dev_priv, but I couldn't do that wihtout adding i915_drv.h, which was not good either... But yeah, this is too generic, while the actual case is not. > > Perhaps call it i915_uncore_lock_irqsave(i915, flags) so it is clear it > > is only for i915. I wanted to avoid "i915", since we also call it when the display is used with xe... > Or, if the implementation will later gain the #else block for Xe, > perhaps intel_uncore_lock_...? But still, uncore doesn't exist in Xe, so this is still not good... Any other suggestions? -- Cheers, Luca.