[Intel-gfx] [PATCH] drm/i915: don't use uncore spinlock to protect critical section in vblank

* [Intel-gfx] [PATCH] drm/i915: don't use uncore spinlock to protect critical section in vblank
@ 2023-11-16 11:27 Luca Coelho
  2023-11-16 16:03 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
  2023-11-17  7:19 ` [Intel-gfx] [Intel-xe] [PATCH] " Ville Syrjälä
  0 siblings, 2 replies; 13+ messages in thread
From: Luca Coelho @ 2023-11-16 11:27 UTC (permalink / raw)
  To: intel-gfx; +Cc: intel-xe, rodrigo.vivi

Since we're abstracting the display code from the underlying driver
(i.e. i915 vs xe), we can't use the uncore's spinlock to protect
critical sections of our code.

After further inspection, it seems that the spinlock is not needed at
all and this can be handled by disabling preemption and interrupts
instead.

Change the vblank code so that we don't use uncore's spinlock anymore
and update the comments accordingly.  While at it, also remove
comments pointing out where to insert RT-specific calls, since we're
now explicitly calling preempt_disable/enable() anywyay.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---

Note: this replaces my previous patch discussed here:
https://patchwork.freedesktop.org/patch/563857/


 drivers/gpu/drm/i915/display/intel_vblank.c | 32 ++++++++++-----------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 2cec2abf9746..28e38960806e 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -302,13 +302,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.
+	 * We will do multiple timing critical raw register reads, so
+	 * disable interrupts and preemption to make sure this code
+	 * doesn't get blocked.
 	 */
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-
-	/* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
+	local_irq_save(irqflags);
+	preempt_disable();
 
 	/* Get optional system timestamp before query. */
 	if (stime)
@@ -372,9 +371,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
 	if (etime)
 		*etime = ktime_get();
 
-	/* preempt_enable_rt() should go right here in PREEMPT_RT patchset. */
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+	preempt_enable();
+	local_irq_restore(irqflags);
 
 	/*
 	 * While in vblank, position will be negative
@@ -408,13 +406,14 @@ bool intel_crtc_get_vblank_timestamp(struct drm_crtc *crtc, int *max_error,
 
 int intel_get_crtc_scanline(struct intel_crtc *crtc)
 {
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	unsigned long irqflags;
 	int position;
 
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+	local_irq_save(irqflags);
+	preempt_disable();
 	position = __intel_get_crtc_scanline(crtc);
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+	preempt_enable();
+	local_irq_restore(irqflags);
 
 	return position;
 }
@@ -528,16 +527,16 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state,
 	 * Belts and suspenders locking to guarantee everyone sees 100%
 	 * consistent state during fastset seamless refresh rate changes.
 	 *
-	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
-	 * uncore.lock takes care of __intel_get_crtc_scanline() which
-	 * may get called elsewhere as well.
+	 * vblank_time_lock takes care of all drm_vblank.c stuff.  For
+	 * __intel_get_crtc_scanline() we don't need locking or
+	 * disabling preemption and irqs, since this is already done
+	 * by the vblank_time_lock spinlock calls.
 	 *
 	 * TODO maybe just protect everything (including
 	 * __intel_get_crtc_scanline()) with vblank_time_lock?
 	 * Need to audit everything to make sure it's safe.
 	 */
 	spin_lock_irqsave(&i915->drm.vblank_time_lock, irqflags);
-	spin_lock(&i915->uncore.lock);
 
 	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
 
@@ -547,6 +546,5 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state,
 
 	crtc->scanline_offset = intel_crtc_scanline_offset(crtc_state);
 
-	spin_unlock(&i915->uncore.lock);
 	spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags);
 }
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread