[PATCH v3 0/8] drm/i915: PREEMPT_RT related fixups.

[PATCH v3 0/8] drm/i915: PREEMPT_RT related fixups.

[Sebastian Andrzej Siewior]

Hi,

The following patches are from the PREEMPT_RT queue.  It is mostly about
disabling interrupts/preemption which leads to problems. Unfortunately
DRM_I915_LOW_LEVEL_TRACEPOINTS had to be disabled because it acquires locks
from within trace points. Making the lock a raw_spinlock_t led to higher
latencies during video playback
  https://lore.kernel.org/all/20211006164628.s2mtsdd2jdbfyf7g@linutronix.de/

and I'm not sure if I hit the worse case here.
I tested it on a SandyBridge with built-in i915 by using X, OpenGL and
playing videos without noticing any warnings. However, some code paths
were not entered.
I carry them for some time now and most issues were reported by other
people and they reported that things work for them since.

v2…v3 https://lore.kernel.org/all/20240613102818.4056866-1-bigeasy@linutronix.de/
  - Collected tags.
  - Added comment to 3/8 explaining why RT is excluded from the test.
  
v1…v2:
  - The tracing disable bits (4/8) have been reworked after Steven pointed out
    that something isn't right.
  - The irq_work() bits have been dropped because they are no longer
    needed.


Sebastian

[PATCH v3 1/8] drm/i915: Use preempt_disable/enable_rt() where recommended

[Sebastian Andrzej Siewior]

From: Mike Galbraith <umgwanakikbuti@gmail.com>

Mario Kleiner suggest in commit
  ad3543ede630f ("drm/intel: Push get_scanout_position() timestamping into kms driver.")

a spots where preemption should be disabled on PREEMPT_RT. The
difference is that on PREEMPT_RT the intel_uncore::lock disables neither
preemption nor interrupts and so region remains preemptible.

The area covers only register reads and writes. The part that worries me
is:
- __intel_get_crtc_scanline() the worst case is 100us if no match is
  found.

- intel_crtc_scanlines_since_frame_timestamp() not sure how long this
  may take in the worst case.

It was in the RT queue for a while and nobody complained.
Disable preemption on PREEPMPT_RT during timestamping.

[bigeasy: patch description.]

Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
Signed-off-by: Mike Galbraith <umgwanakikbuti@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpu/drm/i915/display/intel_vblank.c | 38 +++++++++++++++------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index baf7354cb6e2c..71d6071121460 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -276,6 +276,26 @@ int intel_crtc_scanline_to_hw(struct intel_crtc *crtc, int scanline)
  * all register accesses to the same cacheline to be serialized,
  * otherwise they may hang.
  */
+static void intel_vblank_section_enter_irqsave(struct drm_i915_private *i915, unsigned long *flags)
+	__acquires(i915->uncore.lock)
+{
+#ifdef I915
+	spin_lock_irqsave(&i915->uncore.lock, *flags);
+#else
+	*flags = 0;
+#endif
+}
+
+static void intel_vblank_section_exit_irqrestore(struct drm_i915_private *i915, unsigned long flags)
+	__releases(i915->uncore.lock)
+{
+#ifdef I915
+	spin_unlock_irqrestore(&i915->uncore.lock, flags);
+#else
+	if (flags)
+		return;
+#endif
+}
 static void intel_vblank_section_enter(struct drm_i915_private *i915)
 	__acquires(i915->uncore.lock)
 {
@@ -333,10 +353,10 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
 	 * timing critical raw register reads, potentially with
 	 * preemption disabled, so the following code must not block.
 	 */
-	local_irq_save(irqflags);
-	intel_vblank_section_enter(dev_priv);
+	intel_vblank_section_enter_irqsave(dev_priv, &irqflags);
 
-	/* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
+		preempt_disable();
 
 	/* Get optional system timestamp before query. */
 	if (stime)
@@ -400,10 +420,10 @@ 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. */
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
+		preempt_enable();
 
-	intel_vblank_section_exit(dev_priv);
-	local_irq_restore(irqflags);
+	intel_vblank_section_exit_irqrestore(dev_priv, irqflags);
 
 	/*
 	 * While in vblank, position will be negative
@@ -441,13 +461,11 @@ int intel_get_crtc_scanline(struct intel_crtc *crtc)
 	unsigned long irqflags;
 	int position;
 
-	local_irq_save(irqflags);
-	intel_vblank_section_enter(dev_priv);
+	intel_vblank_section_enter_irqsave(dev_priv, &irqflags);
 
 	position = __intel_get_crtc_scanline(crtc);
 
-	intel_vblank_section_exit(dev_priv);
-	local_irq_restore(irqflags);
+	intel_vblank_section_exit_irqrestore(dev_priv, irqflags);
 
 	return position;
 }
-- 
2.45.2

[PATCH v3 2/8] drm/i915: Don't disable interrupts on PREEMPT_RT during atomic updates

[Sebastian Andrzej Siewior]

From: Mike Galbraith <umgwanakikbuti@gmail.com>

Commit
   8d7849db3eab7 ("drm/i915: Make sprite updates atomic")

started disabling interrupts across atomic updates. This breaks on PREEMPT_RT
because within this section the code attempt to acquire spinlock_t locks which
are sleeping locks on PREEMPT_RT.

According to the comment the interrupts are disabled to avoid random delays and
not required for protection or synchronisation.
If this needs to happen with disabled interrupts on PREEMPT_RT, and the
whole section is restricted to register access then all sleeping locks
need to be acquired before interrupts are disabled and some function
maybe moved after enabling interrupts again.
This includes:
- prepare_to_wait() + finish_wait() due its wake queue.
- drm_crtc_vblank_put() -> vblank_disable_fn() drm_device::vbl_lock.
- skl_pfit_enable(), intel_update_plane(), vlv_atomic_update_fifo() and
  maybe others due to intel_uncore::lock
- drm_crtc_arm_vblank_event() due to drm_device::event_lock and
  drm_device::vblank_time_lock.

Don't disable interrupts on PREEMPT_RT during atomic updates.

[bigeasy: drop local locks, commit message]

Signed-off-by: Mike Galbraith <umgwanakikbuti@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpu/drm/i915/display/intel_crtc.c   | 9 ++++++---
 drivers/gpu/drm/i915/display/intel_vblank.c | 6 ++++--
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index 25593f6aae7de..22b80004574fa 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -512,7 +512,8 @@ void intel_pipe_update_start(struct intel_atomic_state *state,
 	 */
 	intel_psr_wait_for_idle_locked(new_crtc_state);
 
-	local_irq_disable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_irq_disable();
 
 	crtc->debug.min_vbl = evade.min;
 	crtc->debug.max_vbl = evade.max;
@@ -530,7 +531,8 @@ void intel_pipe_update_start(struct intel_atomic_state *state,
 	return;
 
 irq_disable:
-	local_irq_disable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_irq_disable();
 }
 
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_VBLANK_EVADE)
@@ -632,7 +634,8 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
 	 */
 	intel_vrr_send_push(new_crtc_state);
 
-	local_irq_enable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_irq_enable();
 
 	if (intel_vgpu_active(dev_priv))
 		goto out;
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 71d6071121460..d639b51a49195 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -700,11 +700,13 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
 			break;
 		}
 
-		local_irq_enable();
+		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+			local_irq_enable();
 
 		timeout = schedule_timeout(timeout);
 
-		local_irq_disable();
+		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+			local_irq_disable();
 	}
 
 	finish_wait(wq, &wait);
-- 
2.45.2

[PATCH v3 3/8] drm/i915: Don't check for atomic context on PREEMPT_RT

[Sebastian Andrzej Siewior]

The !in_atomic() check in _wait_for_atomic() triggers on PREEMPT_RT
because the uncore::lock is a spinlock_t and does not disable
preemption or interrupts.

Changing the uncore:lock to a raw_spinlock_t doubles the worst case
latency on an otherwise idle testbox during testing.

Ignore _WAIT_FOR_ATOMIC_CHECK() on PREEMPT_RT.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Link: https://lore.kernel.org/all/20211006164628.s2mtsdd2jdbfyf7g@linutronix.de/
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpu/drm/i915/i915_utils.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 06ec6ceb61d57..12cbf04990182 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -273,8 +273,13 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 						   (Wmax))
 #define wait_for(COND, MS)		_wait_for((COND), (MS) * 1000, 10, 1000)
 
-/* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
-#if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
+/*
+ * If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false.
+ * On PREEMPT_RT the context isn't becoming atomic because it is used in an
+ * interrupt handler or because a spinlock_t is acquired. This leads to
+ * warnings which don't occur otherwise and therefore the check is disabled.
+ */
+#if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT) && !defined(CONFIG_PREEMPT_RT)
 # define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
 #else
 # define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
-- 
2.45.2

[PATCH v3 4/8] drm/i915: Disable tracing points on PREEMPT_RT

[Sebastian Andrzej Siewior]

Luca Abeni reported this:
| BUG: scheduling while atomic: kworker/u8:2/15203/0x00000003
| CPU: 1 PID: 15203 Comm: kworker/u8:2 Not tainted 4.19.1-rt3 #10
| Call Trace:
|  rt_spin_lock+0x3f/0x50
|  gen6_read32+0x45/0x1d0 [i915]
|  g4x_get_vblank_counter+0x36/0x40 [i915]
|  trace_event_raw_event_i915_pipe_update_start+0x7d/0xf0 [i915]

The tracing events use trace_intel_pipe_update_start() among other events
use functions acquire spinlock_t locks which are transformed into
sleeping locks on PREEMPT_RT. A few trace points use
intel_get_crtc_scanline(), others use ->get_vblank_counter() wich also
might acquire a sleeping locks on PREEMPT_RT.
At the time the arguments are evaluated within trace point, preemption
is disabled and so the locks must not be acquired on PREEMPT_RT.

Based on this I don't see any other way than disable trace points on
PREMPT_RT.

Acked-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reported-by: Luca Abeni <lucabe72@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpu/drm/i915/display/intel_display_trace.h | 4 ++++
 drivers/gpu/drm/i915/i915_trace.h                  | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display_trace.h b/drivers/gpu/drm/i915/display/intel_display_trace.h
index 49a5e6d9dc0d7..b15c999d91e68 100644
--- a/drivers/gpu/drm/i915/display/intel_display_trace.h
+++ b/drivers/gpu/drm/i915/display/intel_display_trace.h
@@ -9,6 +9,10 @@
 #if !defined(__INTEL_DISPLAY_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ)
 #define __INTEL_DISPLAY_TRACE_H__
 
+#if defined(CONFIG_PREEMPT_RT) && !defined(NOTRACE)
+#define NOTRACE
+#endif
+
 #include <linux/string_helpers.h>
 #include <linux/types.h>
 #include <linux/tracepoint.h>
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index ce1cbee1b39dd..247e7d9448d70 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -6,6 +6,10 @@
 #if !defined(_I915_TRACE_H_) || defined(TRACE_HEADER_MULTI_READ)
 #define _I915_TRACE_H_
 
+#if defined(CONFIG_PREEMPT_RT) && !defined(NOTRACE)
+#define NOTRACE
+#endif
+
 #include <linux/stringify.h>
 #include <linux/types.h>
 #include <linux/tracepoint.h>
-- 
2.45.2

[PATCH v3 5/8] drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock()

[Sebastian Andrzej Siewior]

execlists_dequeue() is invoked from a function which uses
local_irq_disable() to disable interrupts so the spin_lock() behaves
like spin_lock_irq().
This breaks PREEMPT_RT because local_irq_disable() + spin_lock() is not
the same as spin_lock_irq().

execlists_dequeue_irq() and execlists_dequeue() has each one caller
only. If intel_engine_cs::active::lock is acquired and released with the
_irq suffix then it behaves almost as if execlists_dequeue() would be
invoked with disabled interrupts. The difference is the last part of the
function which is then invoked with enabled interrupts.
I can't tell if this makes a difference. From looking at it, it might
work to move the last unlock at the end of the function as I didn't find
anything that would acquire the lock again.

Reported-by: Clark Williams <williams@redhat.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 .../drm/i915/gt/intel_execlists_submission.c    | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 21829439e6867..ed29a2dd6ea0a 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -1303,7 +1303,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 	 * and context switches) submission.
 	 */
 
-	spin_lock(&sched_engine->lock);
+	spin_lock_irq(&sched_engine->lock);
 
 	/*
 	 * If the queue is higher priority than the last
@@ -1403,7 +1403,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 				 * Even if ELSP[1] is occupied and not worthy
 				 * of timeslices, our queue might be.
 				 */
-				spin_unlock(&sched_engine->lock);
+				spin_unlock_irq(&sched_engine->lock);
 				return;
 			}
 		}
@@ -1429,7 +1429,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 
 		if (last && !can_merge_rq(last, rq)) {
 			spin_unlock(&ve->base.sched_engine->lock);
-			spin_unlock(&engine->sched_engine->lock);
+			spin_unlock_irq(&engine->sched_engine->lock);
 			return; /* leave this for another sibling */
 		}
 
@@ -1591,7 +1591,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 	 */
 	sched_engine->queue_priority_hint = queue_prio(sched_engine);
 	i915_sched_engine_reset_on_empty(sched_engine);
-	spin_unlock(&sched_engine->lock);
+	spin_unlock_irq(&sched_engine->lock);
 
 	/*
 	 * We can skip poking the HW if we ended up with exactly the same set
@@ -1617,13 +1617,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 	}
 }
 
-static void execlists_dequeue_irq(struct intel_engine_cs *engine)
-{
-	local_irq_disable(); /* Suspend interrupts across request submission */
-	execlists_dequeue(engine);
-	local_irq_enable(); /* flush irq_work (e.g. breadcrumb enabling) */
-}
-
 static void clear_ports(struct i915_request **ports, int count)
 {
 	memset_p((void **)ports, NULL, count);
@@ -2478,7 +2471,7 @@ static void execlists_submission_tasklet(struct tasklet_struct *t)
 	}
 
 	if (!engine->execlists.pending[0]) {
-		execlists_dequeue_irq(engine);
+		execlists_dequeue(engine);
 		start_timeslice(engine);
 	}
 
-- 
2.45.2

[PATCH v3 6/8] drm/i915: Drop the irqs_disabled() check

[Sebastian Andrzej Siewior]

The !irqs_disabled() check triggers on PREEMPT_RT even with
i915_sched_engine::lock acquired. The reason is the lock is transformed
into a sleeping lock on PREEMPT_RT and does not disable interrupts.

There is no need to check for disabled interrupts. The lockdep
annotation below already check if the lock has been acquired by the
caller and will yell if the interrupts are not disabled.

Remove the !irqs_disabled() check.

Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Acked-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpu/drm/i915/i915_request.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 519e096c607cd..466b5ee8ed6d2 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -608,7 +608,6 @@ bool __i915_request_submit(struct i915_request *request)
 
 	RQ_TRACE(request, "\n");
 
-	GEM_BUG_ON(!irqs_disabled());
 	lockdep_assert_held(&engine->sched_engine->lock);
 
 	/*
@@ -717,7 +716,6 @@ void __i915_request_unsubmit(struct i915_request *request)
 	 */
 	RQ_TRACE(request, "\n");
 
-	GEM_BUG_ON(!irqs_disabled());
 	lockdep_assert_held(&engine->sched_engine->lock);
 
 	/*
-- 
2.45.2

[PATCH v3 7/8] drm/i915/guc: Consider also RCU depth in busy loop.

[Sebastian Andrzej Siewior]

intel_guc_send_busy_loop() looks at in_atomic() and irqs_disabled() to
decide if it should busy-spin while waiting or if it may sleep.
Both checks will report false on PREEMPT_RT if sleeping spinlocks are
acquired leading to RCU splats while the function sleeps.

Check also if RCU has been disabled.

Reported-by: "John B. Wyatt IV" <jwyatt@redhat.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 57b9031327767..ff213b79ba83d 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -362,7 +362,7 @@ static inline int intel_guc_send_busy_loop(struct intel_guc *guc,
 {
 	int err;
 	unsigned int sleep_period_ms = 1;
-	bool not_atomic = !in_atomic() && !irqs_disabled();
+	bool not_atomic = !in_atomic() && !irqs_disabled() && !rcu_preempt_depth();
 
 	/*
 	 * FIXME: Have caller pass in if we are in an atomic context to avoid
-- 
2.45.2

[PATCH v3 8/8] Revert "drm/i915: Depend on !PREEMPT_RT."

[Sebastian Andrzej Siewior]

Once the known issues are addressed, it should be safe to enable the
driver.

Acked-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpu/drm/i915/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 5932024f8f954..a02162d6b710e 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -3,7 +3,6 @@ config DRM_I915
 	tristate "Intel 8xx/9xx/G3x/G4x/HD Graphics"
 	depends on DRM
 	depends on X86 && PCI
-	depends on !PREEMPT_RT
 	select INTEL_GTT if X86
 	select INTERVAL_TREE
 	# we need shmfs for the swappable backing store, and in particular
-- 
2.45.2

✗ Fi.CI.CHECKPATCH: warning for drm/i915: PREEMPT_RT related fixups. (rev12)

[Patchwork]

== Series Details ==

Series: drm/i915: PREEMPT_RT related fixups. (rev12)
URL   : https://patchwork.freedesktop.org/series/95463/
State : warning

== Summary ==

Error: dim checkpatch failed
4262ab871c62 drm/i915: Use preempt_disable/enable_rt() where recommended
-:7: WARNING:COMMIT_LOG_LONG_LINE: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#7: 
  ad3543ede630f ("drm/intel: Push get_scanout_position() timestamping into kms driver.")

total: 0 errors, 1 warnings, 0 checks, 67 lines checked
c7ade8db8951 drm/i915: Don't disable interrupts on PREEMPT_RT during atomic updates
-:10: WARNING:COMMIT_LOG_LONG_LINE: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#10: 
started disabling interrupts across atomic updates. This breaks on PREEMPT_RT

total: 0 errors, 1 warnings, 0 checks, 42 lines checked
eca7042a432a drm/i915: Don't check for atomic context on PREEMPT_RT
89e439da009d drm/i915: Disable tracing points on PREEMPT_RT
-:27: WARNING:BAD_REPORTED_BY_LINK: Reported-by: should be immediately followed by Closes: with a URL to the report
#27: 
Reported-by: Luca Abeni <lucabe72@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>

total: 0 errors, 1 warnings, 0 checks, 20 lines checked
4d47a00f3c89 drm/i915/gt: Use spin_lock_irq() instead of local_irq_disable() + spin_lock()
-:22: WARNING:BAD_REPORTED_BY_LINK: Reported-by: should be immediately followed by Closes: with a URL to the report
#22: 
Reported-by: Clark Williams <williams@redhat.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

total: 0 errors, 1 warnings, 0 checks, 53 lines checked
5a7b403521d4 drm/i915: Drop the irqs_disabled() check
-:16: WARNING:BAD_REPORTED_BY_LINK: Reported-by: should be immediately followed by Closes: with a URL to the report
#16: 
Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Acked-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

total: 0 errors, 1 warnings, 0 checks, 14 lines checked
7eb47c2ced67 drm/i915/guc: Consider also RCU depth in busy loop.
-:13: WARNING:BAD_REPORTED_BY_LINK: Reported-by: should be immediately followed by Closes: with a URL to the report
#13: 
Reported-by: "John B. Wyatt IV" <jwyatt@redhat.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

-:26: ERROR:IN_ATOMIC: do not use in_atomic in drivers
#26: FILE: drivers/gpu/drm/i915/gt/uc/intel_guc.h:365:
+	bool not_atomic = !in_atomic() && !irqs_disabled() && !rcu_preempt_depth();

total: 1 errors, 1 warnings, 0 checks, 8 lines checked
7a32e2fb06b0 Revert "drm/i915: Depend on !PREEMPT_RT."

✗ Fi.CI.SPARSE: warning for drm/i915: PREEMPT_RT related fixups. (rev12)

[Patchwork]

== Series Details ==

Series: drm/i915: PREEMPT_RT related fixups. (rev12)
URL   : https://patchwork.freedesktop.org/series/95463/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

✗ Fi.CI.BAT: failure for drm/i915: PREEMPT_RT related fixups. (rev12)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4434 bytes --]

== Series Details ==

Series: drm/i915: PREEMPT_RT related fixups. (rev12)
URL   : https://patchwork.freedesktop.org/series/95463/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_15013 -> Patchwork_95463v12
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_95463v12 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_95463v12, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_95463v12/index.html

Participating hosts (42 -> 33)
------------------------------

  Missing    (9): bat-adlp-9 bat-adlp-6 fi-snb-2520m bat-adln-1 fi-elk-e7500 bat-dg2-14 bat-dg2-13 bat-dg2-11 bat-arlh-2 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_95463v12:

### IGT changes ###

#### Possible regressions ####

  * igt@debugfs_test@read_all_entries:
    - fi-kbl-7567u:       [PASS][1] -> [DMESG-WARN][2] +5 other tests dmesg-warn
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15013/fi-kbl-7567u/igt@debugfs_test@read_all_entries.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_95463v12/fi-kbl-7567u/igt@debugfs_test@read_all_entries.html

  
Known issues
------------

  Here are the changes found in Patchwork_95463v12 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@module-reload:
    - fi-kbl-7567u:       [PASS][3] -> [DMESG-WARN][4] ([i915#10062] / [i915#180] / [i915#1982] / [i915#9925])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15013/fi-kbl-7567u/igt@i915_pm_rpm@module-reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_95463v12/fi-kbl-7567u/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@sanitycheck:
    - fi-kbl-7567u:       [PASS][5] -> [DMESG-WARN][6] ([i915#11328]) +40 other tests dmesg-warn
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15013/fi-kbl-7567u/igt@i915_selftest@live@sanitycheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_95463v12/fi-kbl-7567u/igt@i915_selftest@live@sanitycheck.html

  * igt@kms_busy@basic@flip:
    - fi-kbl-7567u:       [PASS][7] -> [DMESG-WARN][8] ([i915#180])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15013/fi-kbl-7567u/igt@kms_busy@basic@flip.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_95463v12/fi-kbl-7567u/igt@kms_busy@basic@flip.html

  * igt@kms_frontbuffer_tracking@basic:
    - bat-arls-2:         [PASS][9] -> [DMESG-WARN][10] ([i915#7507])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15013/bat-arls-2/igt@kms_frontbuffer_tracking@basic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_95463v12/bat-arls-2/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_pm_rpm@basic-pci-d3-state:
    - fi-kbl-7567u:       [PASS][11] -> [DMESG-WARN][12] ([i915#10062] / [i915#180] / [i915#9925]) +39 other tests dmesg-warn
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15013/fi-kbl-7567u/igt@kms_pm_rpm@basic-pci-d3-state.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_95463v12/fi-kbl-7567u/igt@kms_pm_rpm@basic-pci-d3-state.html

  
  [i915#10062]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10062
  [i915#11328]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11328
  [i915#180]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/180
  [i915#1982]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1982
  [i915#7507]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/7507
  [i915#9925]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9925


Build changes
-------------

  * Linux: CI_DRM_15013 -> Patchwork_95463v12

  CI-20190529: 20190529
  CI_DRM_15013: 0318a12ff6fb8c321458aa2b373e9322896ee951 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7906: ae91ba26f657bf11264f64bd2dc21f471a5d18f5 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_95463v12: 0318a12ff6fb8c321458aa2b373e9322896ee951 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_95463v12/index.html

[-- Attachment #2: Type: text/html, Size: 5466 bytes --]

RE: ✗ Fi.CI.BAT: failure for drm/i915: PREEMPT_RT related fixups. (rev12)

[Saarinen, Jani]

Hi, 
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Patchwork
> Sent: Friday, 28 June 2024 16.43
> To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: ✗ Fi.CI.BAT: failure for drm/i915: PREEMPT_RT related fixups. (rev12)
> 
> Patch Details
> Series:	drm/i915: PREEMPT_RT related fixups. (rev12)
> URL:	https://patchwork.freedesktop.org/series/95463/
> State:	failure
> Details:	https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_95463v12/index.html
> 
> CI Bug Log - changes from CI_DRM_15013 -> Patchwork_95463v12
> 
> Summary
> 
> 
> FAILURE
> 
> Serious unknown changes coming with Patchwork_95463v12 absolutely need
> to be verified manually.
> 
> If you think the reported changes have nothing to do with the changes
> introduced in Patchwork_95463v12, please notify your bug team (I915-ci-
> infra@lists.freedesktop.org) to allow them to document this new failure
> mode, which will reduce false positives in CI.
> 
> External URL: https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_95463v12/index.html
> 
> 
> Participating hosts (42 -> 33)
> 
> 
> Missing (9): bat-adlp-9 bat-adlp-6 fi-snb-2520m bat-adln-1 fi-elk-e7500 bat-
> dg2-14 bat-dg2-13 bat-dg2-11 bat-arlh-2
^^^this would be good to understand why this many systems down or was this CI system issues? 

> 
> 
> Possible new issues
> 
> 
> Here are the unknown changes that may have been introduced in
> Patchwork_95463v12:
> 
> 
> IGT changes
> 
> 
> Possible regressions
> 
> 
> *	igt@debugfs_test@read_all_entries:
> 
> 	*	fi-kbl-7567u: PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15013/fi-kbl-7567u/igt@debugfs_test@read_all_entries.html>
> -> DMESG-WARN <https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_95463v12/fi-kbl-
> 7567u/igt@debugfs_test@read_all_entries.html>  +5 other tests dmesg-warn
This is known issue https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11328 

> 
> 
> Known issues
> 
> 
> Here are the changes found in Patchwork_95463v12 that come from known
> issues:
> 
> 
> IGT changes
> 
> 
> Issues hit
> 
> 
> *	igt@i915_pm_rpm@module-reload:
> 
> 	*	fi-kbl-7567u: PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15013/fi-kbl-7567u/igt@i915_pm_rpm@module-reload.html>  -
> > DMESG-WARN <https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_95463v12/fi-kbl-7567u/igt@i915_pm_rpm@module-
> reload.html>  (i915#10062 <https://gitlab.freedesktop.org/drm/i915/kernel/-
> /issues/10062>  / i915#180 <https://gitlab.freedesktop.org/drm/i915/kernel/-
> /issues/180>  / i915#1982 <https://gitlab.freedesktop.org/drm/i915/kernel/-
> /issues/1982>  / i915#9925 <https://gitlab.freedesktop.org/drm/i915/kernel/-
> /issues/9925> )
> 
> *	igt@i915_selftest@live@sanitycheck:
> 
> 	*	fi-kbl-7567u: PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15013/fi-kbl-7567u/igt@i915_selftest@live@sanitycheck.html>
> -> DMESG-WARN <https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_95463v12/fi-kbl-
> 7567u/igt@i915_selftest@live@sanitycheck.html>  (i915#11328
> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11328> ) +40 other
> tests dmesg-warn
> 
> *	igt@kms_busy@basic@flip:
> 
> 	*	fi-kbl-7567u: PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15013/fi-kbl-7567u/igt@kms_busy@basic@flip.html>  ->
> DMESG-WARN <https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_95463v12/fi-kbl-7567u/igt@kms_busy@basic@flip.html>
> (i915#180 <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/180> )
> 
> *	igt@kms_frontbuffer_tracking@basic:
> 
> 	*	bat-arls-2: PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15013/bat-arls-2/igt@kms_frontbuffer_tracking@basic.html>  -
> > DMESG-WARN <https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_95463v12/bat-arls-
> 2/igt@kms_frontbuffer_tracking@basic.html>  (i915#7507
> <https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/7507> )
> 
> *	igt@kms_pm_rpm@basic-pci-d3-state:
> 
> 	*	fi-kbl-7567u: PASS <https://intel-gfx-ci.01.org/tree/drm-
> tip/CI_DRM_15013/fi-kbl-7567u/igt@kms_pm_rpm@basic-pci-d3-
> state.html>  -> DMESG-WARN <https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_95463v12/fi-kbl-7567u/igt@kms_pm_rpm@basic-pci-d3-
> state.html>  (i915#10062 <https://gitlab.freedesktop.org/drm/i915/kernel/-
> /issues/10062>  / i915#180 <https://gitlab.freedesktop.org/drm/i915/kernel/-
> /issues/180>  / i915#9925 <https://gitlab.freedesktop.org/drm/i915/kernel/-
> /issues/9925> ) +39 other tests dmesg-warn
> 
> 
> Build changes
> 
> 
> *	Linux: CI_DRM_15013 -> Patchwork_95463v12
> 
> CI-20190529: 20190529
> CI_DRM_15013: 0318a12ff6fb8c321458aa2b373e9322896ee951 @
> git://anongit.freedesktop.org/gfx-ci/linux
> IGT_7906: ae91ba26f657bf11264f64bd2dc21f471a5d18f5 @
> https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
> Patchwork_95463v12: 0318a12ff6fb8c321458aa2b373e9322896ee951 @
> git://anongit.freedesktop.org/gfx-ci/linux

Re: [PATCH v3 0/8] drm/i915: PREEMPT_RT related fixups.

[Sebastian Andrzej Siewior]

On 2024-06-28 14:57:59 [+0200], To intel-gfx@lists.freedesktop.org wrote:
Hi,

> The following patches are from the PREEMPT_RT queue.  It is mostly about
> disabling interrupts/preemption which leads to problems. Unfortunately
> DRM_I915_LOW_LEVEL_TRACEPOINTS had to be disabled because it acquires locks
> from within trace points. Making the lock a raw_spinlock_t led to higher
> latencies during video playback
>   https://lore.kernel.org/all/20211006164628.s2mtsdd2jdbfyf7g@linutronix.de/
> 
> and I'm not sure if I hit the worse case here.
> I tested it on a SandyBridge with built-in i915 by using X, OpenGL and
> playing videos without noticing any warnings. However, some code paths
> were not entered.
> I carry them for some time now and most issues were reported by other
> people and they reported that things work for them since.

These patches were not picked. Did I forget something or was this just
overseen?

Sebastian

Re: [PATCH v3 0/8] drm/i915: PREEMPT_RT related fixups.

[Ville Syrjälä]

On Wed, Oct 02, 2024 at 06:25:43PM +0200, Sebastian Andrzej Siewior wrote:
> On 2024-06-28 14:57:59 [+0200], To intel-gfx@lists.freedesktop.org wrote:
> Hi,
> 
> > The following patches are from the PREEMPT_RT queue.  It is mostly about
> > disabling interrupts/preemption which leads to problems. Unfortunately
> > DRM_I915_LOW_LEVEL_TRACEPOINTS had to be disabled because it acquires locks
> > from within trace points. Making the lock a raw_spinlock_t led to higher
> > latencies during video playback
> >   https://lore.kernel.org/all/20211006164628.s2mtsdd2jdbfyf7g@linutronix.de/
> > 
> > and I'm not sure if I hit the worse case here.
> > I tested it on a SandyBridge with built-in i915 by using X, OpenGL and
> > playing videos without noticing any warnings. However, some code paths
> > were not entered.
> > I carry them for some time now and most issues were reported by other
> > people and they reported that things work for them since.
> 
> These patches were not picked. Did I forget something or was this just
> overseen?

This looks quite poorly justified. Eg. you seem to be now
leaving interrupts enabled (and even preemption enabled I
guess) when we're racing against the raster beam. On first
blush that seems like a recipe for failure.

First step would be to set CONFIG_DRM_I915_DEBUG_VBLANK_EVADE=y,
run a bunch of tests that stress the display stuff (eg.
kms_atomic_transitions and other stuff from igt, and also
some real workloads) and probably throw in a bunch of
other load/perturbance at the system to make life hard.
After the system has been sufficiently hammered one can
compare sys/kernel/debug/dri/0/crtc-*/i915_update_info
against a baseline. Bonus points for doing it on a potato.

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 0/8] drm/i915: PREEMPT_RT related fixups.

[Sebastian Andrzej Siewior]

On 2024-10-02 19:58:08 [+0300], Ville Syrjälä wrote:
> > These patches were not picked. Did I forget something or was this just
> > overseen?
> 
> This looks quite poorly justified. Eg. you seem to be now
> leaving interrupts enabled (and even preemption enabled I
> guess) when we're racing against the raster beam. On first
> blush that seems like a recipe for failure.

I can't really parse this. I may leave interrupts enabled in vblanc code
(the first two patches).

> First step would be to set CONFIG_DRM_I915_DEBUG_VBLANK_EVADE=y,
> run a bunch of tests that stress the display stuff (eg.
> kms_atomic_transitions and other stuff from igt, and also
> some real workloads) and probably throw in a bunch of
> other load/perturbance at the system to make life hard.
> After the system has been sufficiently hammered one can
> compare sys/kernel/debug/dri/0/crtc-*/i915_update_info
> against a baseline. Bonus points for doing it on a potato.

So you have a specific test for me to run?
 
Sebastian

Re: [PATCH v3 0/8] drm/i915: PREEMPT_RT related fixups.

[Ville Syrjälä]

On Fri, Oct 04, 2024 at 08:49:51AM +0200, Sebastian Andrzej Siewior wrote:
> On 2024-10-02 19:58:08 [+0300], Ville Syrjälä wrote:
> > > These patches were not picked. Did I forget something or was this just
> > > overseen?
> > 
> > This looks quite poorly justified. Eg. you seem to be now
> > leaving interrupts enabled (and even preemption enabled I
> > guess) when we're racing against the raster beam. On first
> > blush that seems like a recipe for failure.
> 
> I can't really parse this. I may leave interrupts enabled in vblanc code
> (the first two patches).

vblank evasion 101:
We need to write a boatload of registers atomically within one 
frame. We try to guarantee that by checking if we're too close 
the point at which the hardware latches said registers, and if 
so we defer the update into the next frame.

raster scan/time ->
.....vertical active.........><...vertical blank...><...vertical active...><...vertical blank
     <VBLANK_EVASION_TIME_US> ^^                                          ^
            ^                 ||                                          |
            |   registers are /|                            registers are /
            |   latched here   |                             latched here
            |                  |
    If we're somewhere         |
    in here we delay           |
    writing the registers...   ...until we are about here and then we have most
                               of the frame to write all the registers. Though
                               since we use an interrupt to wait for this point
                               interrupt latency does eat into the budget a bit

.....vertical active.........><..vertical blank...><...
     <VBLANK_EVASION_TIME_US> ^
  <-.                         |
    |           registers are /
    |           latched here
    |
    If we're somewhere to the left of this
    point we proceed to write the registers
    immediately and now in the worst case we
    have exactly VBLANK_EVASION_TIME_US to
    write all the registers

So once vblank evasion has declared things to be safe we might have
as short a time as VBLANK_EVASION_TIME_US to write all the registers.
If the CPU gets stolen from us at that point we can no longer guarantee
anything. The magic value has been tuned empirically over the years,
until we've found something that seems to work well enough, without
being too long to negatively affect performance.

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 0/8] drm/i915: PREEMPT_RT related fixups.

[Sebastian Andrzej Siewior]

On 2024-10-04 11:31:22 [+0300], Ville Syrjälä wrote:
> 
> So once vblank evasion has declared things to be safe we might have
> as short a time as VBLANK_EVASION_TIME_US to write all the registers.
> If the CPU gets stolen from us at that point we can no longer guarantee
> anything. The magic value has been tuned empirically over the years,
> until we've found something that seems to work well enough, without
> being too long to negatively affect performance.

what happens if this gets delayed? Just flicker or worse?

Is this something that affects all i915 based HW or only old ones? As
far as I remember, there is a register lock which is only required on
older HW.

Sebastian

Re: [PATCH v3 0/8] drm/i915: PREEMPT_RT related fixups.

[Ville Syrjälä]

On Fri, Oct 04, 2024 at 10:45:25AM +0200, Sebastian Andrzej Siewior wrote:
> On 2024-10-04 11:31:22 [+0300], Ville Syrjälä wrote:
> > 
> > So once vblank evasion has declared things to be safe we might have
> > as short a time as VBLANK_EVASION_TIME_US to write all the registers.
> > If the CPU gets stolen from us at that point we can no longer guarantee
> > anything. The magic value has been tuned empirically over the years,
> > until we've found something that seems to work well enough, without
> > being too long to negatively affect performance.
> 
> what happens if this gets delayed? Just flicker or worse?

In the best best case it just gets you a corrupted frame
of some sort, in the worst case the hardware falls over.
Depends on what kind of update is happening, and what
platform we're dealing with.

We've tried to mitigate some of the worst issues by
trying to order the register writes more carefully,
but some of the ordering constraints (eg. scalers vs.
DDB) are more or less in conflict with each other
so making it 100% safe seems impossible.

> 
> Is this something that affects all i915 based HW or only old ones? As
> far as I remember, there is a register lock which is only required on
> older HW.

Currently it affects everything. There is a new double buffer
latching inhibit bit on some of the very latest platforms that
we could probably use to make things more safe if vblank evasion
fails, but we've not hooked that up. But vblank evasion would still
be necessary at least for cursor updates since those are
done as mailbox style updates (ie. multiple updates per frame)
and there is no way to guarantee forward progress without vblank
evasion.

Register access locks aren't relevant here, and most
register accesses in the vblank evade critical section
are lockless anyway. The locks were too expensive and we
determined that we an safely use lockless accesses here.

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 0/8] drm/i915: PREEMPT_RT related fixups.

[Sebastian Andrzej Siewior]

On 2024-10-04 12:07:24 [+0300], Ville Syrjälä wrote:
> > what happens if this gets delayed? Just flicker or worse?
> 
> In the best best case it just gets you a corrupted frame
> of some sort, in the worst case the hardware falls over.
> Depends on what kind of update is happening, and what
> platform we're dealing with.
> 
> We've tried to mitigate some of the worst issues by
> trying to order the register writes more carefully,
> but some of the ordering constraints (eg. scalers vs.
> DDB) are more or less in conflict with each other
> so making it 100% safe seems impossible.

So based on what you are saying, this _has_ to happen with disabled
interrupts.

> > 
> > Is this something that affects all i915 based HW or only old ones? As
> > far as I remember, there is a register lock which is only required on
> > older HW.
> 
> Currently it affects everything. There is a new double buffer
> latching inhibit bit on some of the very latest platforms that
> we could probably use to make things more safe if vblank evasion
> fails, but we've not hooked that up. But vblank evasion would still
> be necessary at least for cursor updates since those are
> done as mailbox style updates (ie. multiple updates per frame)
> and there is no way to guarantee forward progress without vblank
> evasion.

This sounds sad. Especially since the delay is at 100us.

> Register access locks aren't relevant here, and most
> register accesses in the vblank evade critical section
> are lockless anyway. The locks were too expensive and we
> determined that we an safely use lockless accesses here.

The register lock question was only an example of something that is not
required on newish hardware. Also disabling interrupts within in patch
1, 2 would require to make uncore:lock a raw_spinlock_t since it is
acquire there.

What do suggest as in how do we move forward? In terms of testing, I
have here an i7 sandy bridge with embedded GPU.

Sebastian