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@xxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: Ville Syrj?l? <ville.syrjala@xxxxxxxxxxxxxxx>
> 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.
>
> 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
>
> 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.
> + */
> +static inline void intel_vblank_section_enter(struct drm_i915_private *i915)
let's avoid inline here.
> +{
> +#ifdef I915
> + spin_lock(&i915->uncore.lock);
> +#endif
> +}
> +
> +static inline void intel_vblank_section_exit(struct drm_i915_private *i915)
and here
With these changes:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> +{
> +#ifdef I915
> + spin_unlock(&i915->uncore.lock);
> +#endif
> +}
> +
> static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
> bool in_vblank_irq,
> int *vpos, int *hpos,
> @@ -302,11 +322,12 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
> }
>
> /*
> - * Lock uncore.lock, as we will do multiple timing critical raw
> - * register reads, potentially with preemption disabled, so the
> - * following code must not block on uncore.lock.
> + * Enter vblank critical section, as we will do multiple
> + * timing critical raw register reads, potentially with
> + * preemption disabled, so the following code must not block.
> */
> - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> + local_irq_save(irqflags);
> + intel_vblank_section_enter(dev_priv);
>
> /* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
>
> @@ -374,7 +395,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_vblank_section_exit(dev_priv);
> + local_irq_restore(irqflags);
>
> /*
> * While in vblank, position will be negative
> @@ -412,9 +434,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_vblank_section_enter(dev_priv);
> +
> position = __intel_get_crtc_scanline(crtc);
> - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +
> + intel_vblank_section_exit(dev_priv);
> + local_irq_restore(irqflags);
>
> return position;
> }
> @@ -537,7 +563,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_vblank_section_enter(i915);
>
> drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
>
> @@ -546,7 +572,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_vblank_section_exit(i915);
> spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags);
> }
> --
> 2.39.2
>
