Re: [Intel-gfx] [PATCH v3] drm/i915: handle uncore spinlock when not available

["Coelho, Luciano" <luciano.coelho@intel.com>]

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@intel.com>
> > > ---
> > > 
> > > 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.