[Intel-xe] [PATCH] drm/i915: handle uncore spinlock when not available

[Luca Coelho]

On Thu, 2023-10-19 at 10:28 -0400, Rodrigo Vivi wrote:
> On Wed, Oct 18, 2023 at 03:45:00PM +0300, 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 at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.h | 20 ++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_vblank.c  | 18 +++++++++++-------
> >  2 files changed, 31 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..07acf5d414f6 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 uncore_spin_lock(spinlock_t *lock)
> > +{
> > +#ifdef I915
> > +	spin_lock(lock);
> > +#endif
> 
> okay, I understand that your goal here is not to fix xe in case the
> flow is totally broken there because of the lack of the locks to
> serialize the display here. Nor also remove this serialization in
> i915, if not needed. And that's the safest option I agree.
> 
> I just wonder if we could do in some more generic way like avoiding
> the term 'uncore' and using 'dev_priv' as argument instead of the
> lock directly. But not hard feelings...

Makes sense.  I'll change it.


> > +}
> > +
> > +static inline void uncore_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..0e8b71063353 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);
> > +	uncore_spin_lock(&dev_priv->uncore.lock);
> > +	local_irq_save(irqflags);
> 
> shouldn't we save before lock or restore before unlock?
> or to be on the safe side also create other helpers for these?

Oh, nice catch.  I did go into spin_lock_irqsave() to check the order,
but for some reason didn't do it right here.  This is the correct
order:

irq_save->lock->unlock->irq_restore

I'll fix it.

Thanks for the review! I accidentally sent this to intel-xe again,
though my intention was to send it to intel-gfx.  I'll send v2 to both.


--
Cheers,
Luca.