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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 23 Oct 2023, Luca Coelho <luciano.coelho@intel.com> 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>
> ---
>
> 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