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

[Coelho, Luciano]

On Wed, 2023-11-29 at 13:01 -0500, Rodrigo Vivi wrote:
> On Wed, Nov 29, 2023 at 11:17:28AM +0200, 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.
> > 
> > Cc: Tvrtko Ursulin <tvrto.ursulin at intel.com>
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Ville Syrj?l? <ville.syrjala at linux.intel.com>
> > Signed-off-by: Luca Coelho <luciano.coelho at 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.
> > 
> > In v4:
> > 
> >    * After a brief attempt to replace this with a different patch,
> >      we're back to this one;
> >    * Pass drm_i195_private again, and move the functions to
> >      intel_vblank.c, so we don't need to include i915_drv.h in a
> >      header file and it's already included in intel_vblank.c;
> > 
> >  drivers/gpu/drm/i915/display/intel_display.h |  1 +
> >  drivers/gpu/drm/i915/display/intel_vblank.c  | 45 +++++++++++++++-----
> >  2 files changed, 36 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> > index 8548f49e3972..5ff299bc4b87 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -29,6 +29,7 @@
> >  
> >  #include "i915_reg_defs.h"
> >  #include "intel_display_limits.h"
> > +#include "i915_drv.h"
> 
> please move this include to intel_vblank.c

Oops, this is a leftover of some tests I was making to see just how
much worse things would get by adding this here.

Actually, why don't we move the drm_i915_private structure (and maybe
others?) to a lighter header file than i915_drv.h? IMHO it's really
annoying to have the forward declarations for it in many places just
because we don't want to include the actual header.  When I want to
find its global definition, cscope always returns tens of results
because of the forward declarations... This is obviously orthogonal to
the current patch.


> >  enum drm_scaling_filter;
> >  struct dpll;
> > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> > index 2cec2abf9746..d9625db82681 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> > @@ -265,6 +265,26 @@ int intel_crtc_scanline_to_hw(struct intel_crtc *crtc, int scanline)
> >  	return (scanline + vtotal - crtc->scanline_offset) % vtotal;
> >  }
> >  
> > +/*
> > + * 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.
> 
> maybe we could add brief mention that it is only needed because old hardware
> that is not supported by Xe.

Good idea, I'll add it.

> 
> > + */
> > +static inline void intel_vblank_section_enter(struct drm_i915_private *i915)
> 
> let's avoid inline here.

Okay, I'll remove it.


> > +{
> > +#ifdef I915
> > +	spin_lock(&i915->uncore.lock);
> > +#endif
> > +}
> > +
> > +static inline void intel_vblank_section_exit(struct drm_i915_private *i915)
> 
> and here

Okay.


> With these changes:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

Thanks for the review, Rodrigo!

--
Cheers,
Luca.