On Mon, 23 Oct 2023, Luca Coelho <luciano.coelho@xxxxxxxxx> 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> > --- > > Note: this patch was accidentally sent only to intel-xe[1], but should > have been sent to intel-gfx. Thus, this is v2. > > In v2: > > * Renamed uncore_spin_*() to intel_spin_*() > * Corrected the order: save, lock, unlock, restore > > [1] https://patchwork.freedesktop.org/patch/563288/ > > > drivers/gpu/drm/i915/display/intel_display.h | 22 +++++++++++++++++++- > drivers/gpu/drm/i915/display/intel_vblank.c | 19 ++++++++++------- > 2 files changed, 33 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h > index 0e5dffe8f018..099476906f4c 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" In general, please avoid including headers from headers. In particular, please don't include i915_drv.h from headers. The header interdependencies are pretty bad already, and we need to clean it up. BR, Jani. > > enum drm_scaling_filter; > struct dpll; > @@ -41,7 +42,6 @@ struct drm_file; > struct drm_format_info; > struct drm_framebuffer; > struct drm_i915_gem_object; > -struct drm_i915_private; > struct drm_mode_fb_cmd2; > struct drm_modeset_acquire_ctx; > struct drm_plane; > @@ -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(struct drm_i915_private *i915) > +{ > +#ifdef I915 > + spin_lock(&i915->uncore.lock); > +#endif > +} > + > +static inline void intel_spin_unlock(struct drm_i915_private *i915) > +{ > +#ifdef I915 > + spin_unlock(&i915->uncore.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..7c624ea7e902 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); > + intel_spin_lock(dev_priv); > > /* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */ > > @@ -374,7 +375,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc, > > /* preempt_enable_rt() should go right here in PREEMPT_RT patchset. */ > > - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > + intel_spin_unlock(dev_priv); > + local_irq_restore(irqflags); > > /* > * While in vblank, position will be negative > @@ -412,9 +414,13 @@ int intel_get_crtc_scanline(struct intel_crtc *crtc) > unsigned long irqflags; > int position; > > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > + local_irq_save(irqflags); > + intel_spin_lock(dev_priv); > + > position = __intel_get_crtc_scanline(crtc); > - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > + > + intel_spin_unlock(dev_priv); > + local_irq_restore(irqflags); > > return position; > } > @@ -537,7 +543,7 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state, > * Need to audit everything to make sure it's safe. > */ > spin_lock_irqsave(&i915->drm.vblank_time_lock, irqflags); > - spin_lock(&i915->uncore.lock); > + intel_spin_lock(i915); > > drm_calc_timestamping_constants(&crtc->base, &adjusted_mode); > > @@ -546,7 +552,6 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state, > crtc->mode_flags = mode_flags; > > crtc->scanline_offset = intel_crtc_scanline_offset(crtc_state); > - > - spin_unlock(&i915->uncore.lock); > + intel_spin_unlock(i915); > spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags); > } -- Jani Nikula, Intel
