[PATCH v2 0/7] drm/i915/display: Handle vblank evasion with CONFIG_PREEMPT_RT

[PATCH v2 0/7] drm/i915/display: Handle vblank evasion with CONFIG_PREEMPT_RT

[Maarten Lankhorst]

There is a critical section between intel_pipe_update_start() and
intel_pipe_update_end() where we only program hardware registers,
should not take any lock and complete as fast as possible.

The previous approach used to remove the local_irq_enable/disable()
in this critical, but that increases the probability that the time
sensitive section does not complete in 100 µs, potentially causing
the hardware to hang.

I went through all the lockdep splats that occurred in CI, and fixed
them 1 by 1 until there were none left. This additionally improves
latency by not removing any locks in the fastpath.

In intel_de.h the implicit DMC wakelock was added, ideally we can get rid
of it, but for now we can simply use the same _fw variants as are
needed on i915.

I believe this series is enough to make xe and perhaps good enough to make
i915's display RT safe.

Changes since v1:
- Fix compilation on i915.
- Submit to intel-gfx ml too.s

Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Mike Galbraith <umgwanakikbuti@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Clark Williams <clrkwllms@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>

Maarten Lankhorst (6):
  drm/i915/display: Make get_vblank_counter use intel_de_read_fw()
  drm/i915/display: Use intel_de_write_fw in intel_pipe_fastset
  drm/i915/display: Move vblank put until after critical section
  drm/i915/display: Remove locking from intel_vblank_evade critical
    section
  drm/i915/display: Make icl_dsi_frame_update use _fw too
  drm/i915/display: Enable interrupts earlier on PREEMPT_RT

Mike Galbraith (1):
  drm/i915: Use preempt_disable/enable_rt() where recommended

 drivers/gpu/drm/i915/display/icl_dsi.c        |  4 +-
 drivers/gpu/drm/i915/display/intel_crtc.c     | 10 +++
 drivers/gpu/drm/i915/display/intel_cursor.c   |  8 +-
 drivers/gpu/drm/i915/display/intel_de.h       |  6 ++
 drivers/gpu/drm/i915/display/intel_display.c  | 36 ++++-----
 drivers/gpu/drm/i915/display/intel_vblank.c   | 80 ++++++++++++-------
 drivers/gpu/drm/i915/display/intel_vrr.c      | 16 ++--
 .../drm/xe/compat-i915-headers/intel_uncore.h |  2 +
 8 files changed, 103 insertions(+), 59 deletions(-)

-- 
2.51.0

[PATCH v2 1/7] drm/i915/display: Make get_vblank_counter use intel_de_read_fw()

[Maarten Lankhorst]

Fixes the following lockdep splat on PREEMPT_RT:
<3> BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
<3> in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1373, name: xe_module_load
<3> preempt_count: 1, expected: 0
<3> RCU nest depth: 0, expected: 0
<4> 11 locks held by xe_module_load/1373:
<4>  #0: ffff888107b691a0 (&dev->mutex){....}-{3:3}, at: __driver_attach+0x104/0x220
<4>  #1: ffff88813cd30280 (&dev->clientlist_mutex){+.+.}-{3:3}, at: drm_client_register+0x32/0xe0
<4>  #2: ffffffff837f88f8 (registration_lock){+.+.}-{3:3}, at: register_framebuffer+0x1b/0x50
<4>  #3: ffffffff835985e0 (console_lock){+.+.}-{0:0}, at: fbcon_fb_registered+0x6f/0x90
<4>  #4: ffff88812589e6a0 (&helper->lock){+.+.}-{3:3}, at: __drm_fb_helper_restore_fbdev_mode_unlocked+0x7b/0x110
<4>  #5: ffff88813cd30158 (&dev->master_mutex){+.+.}-{3:3}, at: drm_master_internal_acquire+0x20/0x50
<4>  #6: ffff88812589e488 (&client->modeset_mutex){+.+.}-{3:3}, at: drm_client_modeset_commit_locked+0x2a/0x1b0
<4>  #7: ffffc9000031eef0 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_client_modeset_commit_atomic+0x4c/0x2b0
<4>  #8: ffffc9000031ef18 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_client_modeset_commit_atomic+0x4c/0x2b0
<4>  #9: ffff888114f7b8b8 (&intel_dp->psr.lock){+.+.}-{3:3}, at: intel_psr_lock+0xc5/0xf0 [xe]
<4>  #10: ffff88812a0cbbc0 (&wl->lock){+.+.}-{2:2}, at: intel_dmc_wl_get+0x3c/0x140 [xe]

This splat will happen otherwise on all tracepoints too, for similar reasons.

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/display/intel_vblank.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 671f357c65638..2b106ffa3f5f5 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -133,7 +133,7 @@ u32 g4x_get_vblank_counter(struct drm_crtc *crtc)
 	if (!vblank->max_vblank_count)
 		return 0;
 
-	return intel_de_read(display, PIPE_FRMCOUNT_G4X(display, pipe));
+	return intel_de_read_fw(display, PIPE_FRMCOUNT_G4X(display, pipe));
 }
 
 static u32 intel_crtc_scanlines_since_frame_timestamp(struct intel_crtc *crtc)
-- 
2.51.0

[PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in intel_pipe_fastset

[Maarten Lankhorst]

intel_set_pipe_src_size(), hsw_set_linetime_wm(),
intel_cpu_transcoder_set_m1_n1() and intel_set_transcoder_timings_lrr()
are called from an atomic context on PREEMPT_RT, and should be using the
_fw functions.

This likely prevents a deadlock on i915.

Again noticed when trying to disable preemption in vblank evasion:
<3> BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
<3> in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1505, name: kms_cursor_lega
<3> preempt_count: 1, expected: 0
<3> RCU nest depth: 0, expected: 0
<4> 4 locks held by kms_cursor_lega/1505:
<4>  #0: ffffc90003c6f988 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_mode_atomic_ioctl+0x13b/0xe90
<4>  #1: ffffc90003c6f9b0 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_mode_atomic_ioctl+0x13b/0xe90
<4>  #2: ffff888135b838b8 (&intel_dp->psr.lock){+.+.}-{3:3}, at: intel_psr_lock+0xc5/0xf0 [xe]
<4>  #3: ffff88812607bbc0 (&wl->lock){+.+.}-{2:2}, at: intel_dmc_wl_get+0x3c/0x140 [xe]
<4> CPU: 6 UID: 0 PID: 1505 Comm: kms_cursor_lega Tainted: G     U              6.18.0-rc3-lgci-xe-xe-pw-156729v1+ #1 PREEMPT_{RT,(lazy)}
<4> Tainted: [U]=USER
<4> Hardware name: Intel Corporation Panther Lake Client Platform/PTL-UH LP5 T3 RVP1, BIOS PTLPFWI1.R00.3383.D02.2509240621 09/24/2025
<4> Call Trace:
<4>  <TASK>
<4>  dump_stack_lvl+0xc1/0xf0
<4>  dump_stack+0x10/0x20
<4>  __might_resched+0x174/0x260
<4>  rt_spin_lock+0x63/0x200
<4>  ? intel_dmc_wl_get+0x3c/0x140 [xe]
<4>  intel_dmc_wl_get+0x3c/0x140 [xe]
<4>  intel_set_pipe_src_size+0x89/0xe0 [xe]
<4>  intel_update_crtc+0x3c1/0x950 [xe]
<4>  ? intel_pre_update_crtc+0x258/0x400 [xe]
<4>  skl_commit_modeset_enables+0x217/0x720 [xe]
<4>  intel_atomic_commit_tail+0xd4e/0x1af0 [xe]
<4>  ? lock_release+0xce/0x2a0
<4>  intel_atomic_commit+0x2e5/0x330 [xe]
<4>  ? intel_atomic_commit+0x2e5/0x330 [xe]
<4>  drm_atomic_commit+0xaf/0xf0
<4>  ? __pfx___drm_printfn_info+0x10/0x10
<4>  drm_mode_atomic_ioctl+0xbd5/0xe90
<4>  ? lock_acquire+0xc4/0x2e0
<4>  ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
<4>  drm_ioctl_kernel+0xb6/0x120
<4>  drm_ioctl+0x2d7/0x5a0
<4>  ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
<4>  ? rt_spin_unlock+0xa0/0x140
<4>  ? __pm_runtime_resume+0x53/0x90
<4>  xe_drm_ioctl+0x56/0x90 [xe]
<4>  __x64_sys_ioctl+0xa8/0x110
<4>  ? lock_acquire+0xc4/0x2e0
<4>  x64_sys_call+0x1144/0x26a0
<4>  do_syscall_64+0x93/0xae0
<4>  ? lock_release+0xce/0x2a0
<4>  ? __task_pid_nr_ns+0xd9/0x270
<4>  ? do_syscall_64+0x1b7/0xae0
<4>  ? find_held_lock+0x31/0x90
<4>  ? __task_pid_nr_ns+0xcf/0x270
<4>  ? __lock_acquire+0x43e/0x2860
<4>  ? __task_pid_nr_ns+0xd9/0x270
<4>  ? lock_acquire+0xc4/0x2e0
<4>  ? find_held_lock+0x31/0x90
<4>  ? __task_pid_nr_ns+0xcf/0x270
<4>  ? lock_release+0xce/0x2a0
<4>  ? __task_pid_nr_ns+0xd9/0x270
<4>  ? do_syscall_64+0x1b7/0xae0
<4>  ? do_syscall_64+0x1b7/0xae0
<4>  entry_SYSCALL_64_after_hwframe+0x76/0x7e

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++++++----------
 drivers/gpu/drm/i915/display/intel_vrr.c     | 16 ++++-----
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 42ec787986666..1bff1148fe9d7 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1573,9 +1573,9 @@ static void hsw_set_linetime_wm(const struct intel_crtc_state *crtc_state)
 	struct intel_display *display = to_intel_display(crtc_state);
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 
-	intel_de_write(display, WM_LINETIME(crtc->pipe),
-		       HSW_LINETIME(crtc_state->linetime) |
-		       HSW_IPS_LINETIME(crtc_state->ips_linetime));
+	intel_de_write_fw(display, WM_LINETIME(crtc->pipe),
+			  HSW_LINETIME(crtc_state->linetime) |
+			  HSW_IPS_LINETIME(crtc_state->ips_linetime));
 }
 
 static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
@@ -2543,14 +2543,14 @@ void intel_set_m_n(struct intel_display *display,
 		   i915_reg_t data_m_reg, i915_reg_t data_n_reg,
 		   i915_reg_t link_m_reg, i915_reg_t link_n_reg)
 {
-	intel_de_write(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m);
-	intel_de_write(display, data_n_reg, m_n->data_n);
-	intel_de_write(display, link_m_reg, m_n->link_m);
+	intel_de_write_fw(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m);
+	intel_de_write_fw(display, data_n_reg, m_n->data_n);
+	intel_de_write_fw(display, link_m_reg, m_n->link_m);
 	/*
 	 * On BDW+ writing LINK_N arms the double buffered update
 	 * of all the M/N registers, so it must be written last.
 	 */
-	intel_de_write(display, link_n_reg, m_n->link_n);
+	intel_de_write_fw(display, link_n_reg, m_n->link_n);
 }
 
 bool intel_cpu_transcoder_has_m2_n2(struct intel_display *display,
@@ -2737,9 +2737,9 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
 	}
 
 	if (DISPLAY_VER(display) >= 13) {
-		intel_de_write(display,
-			       TRANS_SET_CONTEXT_LATENCY(display, cpu_transcoder),
-			       crtc_state->set_context_latency);
+		intel_de_write_fw(display,
+				  TRANS_SET_CONTEXT_LATENCY(display, cpu_transcoder),
+				  crtc_state->set_context_latency);
 
 		/*
 		 * VBLANK_START not used by hw, just clear it
@@ -2755,9 +2755,9 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
 	 * The hardware actually ignores TRANS_VBLANK.VBLANK_END in DP mode.
 	 * But let's write it anyway to keep the state checker happy.
 	 */
-	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
-		       VBLANK_START(crtc_vblank_start - 1) |
-		       VBLANK_END(crtc_vblank_end - 1));
+	intel_de_write_fw(display, TRANS_VBLANK(display, cpu_transcoder),
+			  VBLANK_START(crtc_vblank_start - 1) |
+			  VBLANK_END(crtc_vblank_end - 1));
 	/*
 	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
 	 * bits are not required. Since the support for these bits is going to
@@ -2771,9 +2771,9 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
 	 * The double buffer latch point for TRANS_VTOTAL
 	 * is the transcoder's undelayed vblank.
 	 */
-	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
-		       VACTIVE(crtc_vdisplay - 1) |
-		       VTOTAL(crtc_vtotal - 1));
+	intel_de_write_fw(display, TRANS_VTOTAL(display, cpu_transcoder),
+			  VACTIVE(crtc_vdisplay - 1) |
+			  VTOTAL(crtc_vtotal - 1));
 
 	intel_vrr_set_fixed_rr_timings(crtc_state);
 	intel_vrr_transcoder_enable(crtc_state);
@@ -2790,8 +2790,8 @@ static void intel_set_pipe_src_size(const struct intel_crtc_state *crtc_state)
 	/* pipesrc controls the size that is scaled from, which should
 	 * always be the user's requested size.
 	 */
-	intel_de_write(display, PIPESRC(display, pipe),
-		       PIPESRC_WIDTH(width - 1) | PIPESRC_HEIGHT(height - 1));
+	intel_de_write_fw(display, PIPESRC(display, pipe),
+			  PIPESRC_WIDTH(width - 1) | PIPESRC_HEIGHT(height - 1));
 }
 
 static bool intel_pipe_is_interlaced(const struct intel_crtc_state *crtc_state)
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index 00cbc126fb366..2e19673697fa4 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -300,12 +300,12 @@ void intel_vrr_set_fixed_rr_timings(const struct intel_crtc_state *crtc_state)
 	if (!intel_vrr_possible(crtc_state))
 		return;
 
-	intel_de_write(display, TRANS_VRR_VMIN(display, cpu_transcoder),
-		       intel_vrr_fixed_rr_hw_vmin(crtc_state) - 1);
-	intel_de_write(display, TRANS_VRR_VMAX(display, cpu_transcoder),
-		       intel_vrr_fixed_rr_hw_vmax(crtc_state) - 1);
-	intel_de_write(display, TRANS_VRR_FLIPLINE(display, cpu_transcoder),
-		       intel_vrr_fixed_rr_hw_flipline(crtc_state) - 1);
+	intel_de_write_fw(display, TRANS_VRR_VMIN(display, cpu_transcoder),
+			  intel_vrr_fixed_rr_hw_vmin(crtc_state) - 1);
+	intel_de_write_fw(display, TRANS_VRR_VMAX(display, cpu_transcoder),
+			  intel_vrr_fixed_rr_hw_vmax(crtc_state) - 1);
+	intel_de_write_fw(display, TRANS_VRR_FLIPLINE(display, cpu_transcoder),
+			  intel_vrr_fixed_rr_hw_flipline(crtc_state) - 1);
 }
 
 static
@@ -693,7 +693,7 @@ static void intel_vrr_tg_enable(const struct intel_crtc_state *crtc_state,
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
 	u32 vrr_ctl;
 
-	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), TRANS_PUSH_EN);
+	intel_de_write_fw(display, TRANS_PUSH(display, cpu_transcoder), TRANS_PUSH_EN);
 
 	vrr_ctl = VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state);
 
@@ -705,7 +705,7 @@ static void intel_vrr_tg_enable(const struct intel_crtc_state *crtc_state,
 	if (cmrr_enable)
 		vrr_ctl |= VRR_CTL_CMRR_ENABLE;
 
-	intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), vrr_ctl);
+	intel_de_write_fw(display, TRANS_VRR_CTL(display, cpu_transcoder), vrr_ctl);
 }
 
 static void intel_vrr_tg_disable(const struct intel_crtc_state *old_crtc_state)
-- 
2.51.0

[PATCH v2 3/7] drm/i915/display: Move vblank put until after critical section

[Maarten Lankhorst]

drm_crtc_vblank_put may take some locks, this should probably
not be the first thing we do after entering the time sensitive
part.

A better place is after programming is completed. Add a flag
to put the vblank after completion.

In the case of drm_vblank_work_schedule, we may not even need
to disable the vblank interrupt any more if it takes its own
reference.

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/display/intel_cursor.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
index 7aa14348aa6d4..6b3bc8d94e51a 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -816,6 +816,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
 		to_intel_crtc_state(crtc->base.state);
 	struct intel_crtc_state *new_crtc_state;
 	struct intel_vblank_evade_ctx evade;
+	bool has_vblank = false;
 	int ret;
 
 	/*
@@ -913,6 +914,8 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
 	intel_psr_lock(crtc_state);
 
 	if (!drm_WARN_ON(display->drm, drm_crtc_vblank_get(&crtc->base))) {
+		has_vblank = true;
+
 		/*
 		 * TODO: maybe check if we're still in PSR
 		 * and skip the vblank evasion entirely?
@@ -922,8 +925,6 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
 		local_irq_disable();
 
 		intel_vblank_evade(&evade);
-
-		drm_crtc_vblank_put(&crtc->base);
 	} else {
 		local_irq_disable();
 	}
@@ -939,6 +940,9 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
 
 	intel_psr_unlock(crtc_state);
 
+	if (has_vblank)
+		drm_crtc_vblank_put(&crtc->base);
+
 	if (old_plane_state->ggtt_vma != new_plane_state->ggtt_vma) {
 		drm_vblank_work_init(&old_plane_state->unpin_work, &crtc->base,
 				     intel_cursor_unpin_work);
-- 
2.51.0

[PATCH v2 4/7] drm/i915/display: Remove locking from intel_vblank_evade critical section

[Maarten Lankhorst]

finish_wait() may take a lock, which means that it can take any amount
of time. On PREEMPT-RT we should not be taking any lock after disabling
preemption, so ensure that the completion is done before disabling
interrupts.

This also has the benefit of making vblank evasion more deterministic,
by performing the final vblank check after all locking is done.

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/display/intel_vblank.c | 35 ++++++++++-----------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 2b106ffa3f5f5..3628d2a1b8f38 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -708,6 +708,13 @@ void intel_vblank_evade_init(const struct intel_crtc_state *old_crtc_state,
 		evade->min -= vblank_delay;
 }
 
+static inline int vblank_evadable(struct intel_vblank_evade_ctx *evade, int *scanline)
+{
+	*scanline = intel_get_crtc_scanline(evade->crtc);
+
+	return *scanline < evade->min || *scanline > evade->max;
+}
+
 /* must be called with vblank interrupt already enabled! */
 int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
 {
@@ -715,23 +722,22 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
 	struct intel_display *display = to_intel_display(crtc);
 	long timeout = msecs_to_jiffies_timeout(1);
 	wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
-	DEFINE_WAIT(wait);
 	int scanline;
 
 	if (evade->min <= 0 || evade->max <= 0)
 		return 0;
 
-	for (;;) {
-		/*
-		 * prepare_to_wait() has a memory barrier, which guarantees
-		 * other CPUs can see the task state update by the time we
-		 * read the scanline.
-		 */
-		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
+	while (!vblank_evadable(evade, &scanline)) {
+		local_irq_enable();
 
-		scanline = intel_get_crtc_scanline(crtc);
-		if (scanline < evade->min || scanline > evade->max)
-			break;
+		DEFINE_WAIT(wait);
+		while (!vblank_evadable(evade, &scanline) && timeout > 0) {
+			prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
+			timeout = schedule_timeout(timeout);
+		}
+		finish_wait(wq, &wait);
+
+		local_irq_disable();
 
 		if (!timeout) {
 			drm_dbg_kms(display->drm,
@@ -740,15 +746,8 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
 			break;
 		}
 
-		local_irq_enable();
-
-		timeout = schedule_timeout(timeout);
-
-		local_irq_disable();
 	}
 
-	finish_wait(wq, &wait);
-
 	/*
 	 * On VLV/CHV DSI the scanline counter would appear to
 	 * increment approx. 1/3 of a scanline before start of vblank.
-- 
2.51.0

[PATCH v2 5/7] drm/i915/display: Make icl_dsi_frame_update use _fw too

[Maarten Lankhorst]

Don't use the dmc lock inside the vblank critical section,
not even as last call.

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/display/icl_dsi.c                | 4 ++--
 drivers/gpu/drm/i915/display/intel_de.h               | 6 ++++++
 drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
index 70d4c1bc70fc3..e52b434ac8f11 100644
--- a/drivers/gpu/drm/i915/display/icl_dsi.c
+++ b/drivers/gpu/drm/i915/display/icl_dsi.c
@@ -243,8 +243,8 @@ void icl_dsi_frame_update(struct intel_crtc_state *crtc_state)
 	else
 		return;
 
-	intel_de_rmw(display, DSI_CMD_FRMCTL(port), 0,
-		     DSI_FRAME_UPDATE_REQUEST);
+	intel_de_rmw_fw(display, DSI_CMD_FRMCTL(port), 0,
+			DSI_FRAME_UPDATE_REQUEST);
 }
 
 static void dsi_program_swing_and_deemphasis(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
index 9ecdcf6b73e4d..15f606a4a2e9d 100644
--- a/drivers/gpu/drm/i915/display/intel_de.h
+++ b/drivers/gpu/drm/i915/display/intel_de.h
@@ -214,6 +214,12 @@ intel_de_write_fw(struct intel_display *display, i915_reg_t reg, u32 val)
 	intel_uncore_write_fw(__to_uncore(display), reg, val);
 }
 
+static inline void
+intel_de_rmw_fw(struct intel_display *display, i915_reg_t reg, u32 clear, u32 set)
+{
+	intel_uncore_rmw_fw(__to_uncore(display), reg, clear, set);
+}
+
 static inline u32
 intel_de_read_notrace(struct intel_display *display, i915_reg_t reg)
 {
diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
index d012f02bc84f7..57d5ffabf2d52 100644
--- a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
+++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
@@ -91,6 +91,8 @@ static inline u32 intel_uncore_rmw(struct intel_uncore *uncore,
 	return xe_mmio_rmw32(__compat_uncore_to_mmio(uncore), reg, clear, set);
 }
 
+#define intel_uncore_rmw_fw intel_uncore_rmw
+
 static inline int intel_wait_for_register(struct intel_uncore *uncore,
 					  i915_reg_t i915_reg, u32 mask,
 					  u32 value, unsigned int timeout)
-- 
2.51.0

[PATCH v2 6/7] drm/i915/display: Enable interrupts earlier on PREEMPT_RT

[Maarten Lankhorst]

The last part of the vblank evasion is about updating bookkeeping,
not programming hardware registers.

The interrupts cannot stay disabled here on PREEMPT_RT since the
spinlocks get converted to mutexes.

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/display/intel_crtc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index 9d2a23c96c61b..b87f6b4a4f3d7 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -688,6 +688,14 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
 	    intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI))
 		icl_dsi_frame_update(new_crtc_state);
 
+#if IS_ENABLED(CONFIG_PREEMPT_RT)
+	/*
+	 * Timing sensitive register writing completed, non-deterministic
+	 * locking from here on out.
+	 */
+	local_irq_enable();
+#endif
+
 	/* We're still in the vblank-evade critical section, this can't race.
 	 * Would be slightly nice to just grab the vblank count and arm the
 	 * event outside of the critical section - the spinlock might spin for a
@@ -735,7 +743,9 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
 	if (!state->base.legacy_cursor_update)
 		intel_vrr_send_push(NULL, new_crtc_state);
 
+#if !IS_ENABLED(CONFIG_PREEMPT_RT)
 	local_irq_enable();
+#endif
 
 	if (intel_vgpu_active(dev_priv))
 		goto out;
-- 
2.51.0

[PATCH v2 7/7] drm/i915: Use preempt_disable/enable_rt() where recommended

[Maarten Lankhorst]

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>
Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/i915/display/intel_vblank.c | 43 ++++++++++++++++-----
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 3628d2a1b8f38..dd95336e6d792 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -317,6 +317,20 @@ static void intel_vblank_section_exit(struct intel_display *display)
 	struct drm_i915_private *i915 = to_i915(display->drm);
 	spin_unlock(&i915->uncore.lock);
 }
+
+static void intel_vblank_section_enter_irqf(struct intel_display *display, unsigned long *flags)
+	__acquires(i915->uncore.lock)
+{
+	struct drm_i915_private *i915 = to_i915(display->drm);
+	spin_lock_irqsave(&i915->uncore.lock, *flags);
+}
+
+static void intel_vblank_section_exit_irqf(struct intel_display *display, unsigned long flags)
+	__releases(i915->uncore.lock)
+{
+	struct drm_i915_private *i915 = to_i915(display->drm);
+	spin_unlock_irqrestore(&i915->uncore.lock, flags);
+}
 #else
 static void intel_vblank_section_enter(struct intel_display *display)
 {
@@ -325,6 +339,17 @@ static void intel_vblank_section_enter(struct intel_display *display)
 static void intel_vblank_section_exit(struct intel_display *display)
 {
 }
+
+static void intel_vblank_section_enter_irqf(struct intel_display *display, unsigned long *flags)
+{
+	*flags = 0;
+}
+
+static void intel_vblank_section_exit_irqf(struct intel_display *display, unsigned long flags)
+{
+	if (flags)
+		return;
+}
 #endif
 
 static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
@@ -361,10 +386,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(display);
+	intel_vblank_section_enter_irqf(display, &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)
@@ -428,10 +453,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(display);
-	local_irq_restore(irqflags);
+	intel_vblank_section_exit_irqf(display, irqflags);
 
 	/*
 	 * While in vblank, position will be negative
@@ -469,13 +494,11 @@ int intel_get_crtc_scanline(struct intel_crtc *crtc)
 	unsigned long irqflags;
 	int position;
 
-	local_irq_save(irqflags);
-	intel_vblank_section_enter(display);
+	intel_vblank_section_enter_irqf(display, &irqflags);
 
 	position = __intel_get_crtc_scanline(crtc);
 
-	intel_vblank_section_exit(display);
-	local_irq_restore(irqflags);
+	intel_vblank_section_exit_irqf(display, irqflags);
 
 	return position;
 }
-- 
2.51.0

Re: [PATCH v2 0/7] drm/i915/display: Handle vblank evasion with CONFIG_PREEMPT_RT

[Sebastian Andrzej Siewior]

Thank you for looking into this. I applied this on top of
drm-intel-next-2025-11-04 for testing.

On 2025-11-04 09:36:24 [+0100], Maarten Lankhorst wrote:
> There is a critical section between intel_pipe_update_start() and
> intel_pipe_update_end() where we only program hardware registers,
> should not take any lock and complete as fast as possible.
> 
> The previous approach used to remove the local_irq_enable/disable()
> in this critical, but that increases the probability that the time
> sensitive section does not complete in 100 µs, potentially causing
> the hardware to hang.
> 
> I went through all the lockdep splats that occurred in CI, and fixed
> them 1 by 1 until there were none left. This additionally improves
> latency by not removing any locks in the fastpath.
> 
> In intel_de.h the implicit DMC wakelock was added, ideally we can get rid
> of it, but for now we can simply use the same _fw variants as are
> needed on i915.
> 
> I believe this series is enough to make xe and perhaps good enough to make
> i915's display RT safe.

I've been playing with it:
- DRM_XE_DEBUG_GUC ended in a segfault:
| xe 0000:00:02.0: enabling device (0006 -> 0007)
| Console: switching to colour dummy device 80x25
| xe 0000:00:02.0: vgaarb: deactivate vga console
| xe 0000:00:02.0: [drm] Running in SR-IOV PF mode
| xe 0000:00:02.0: [drm] Found tigerlake/uy (device ID 9a49) integrated display version 12.00 stepping C0
| xe 0000:00:02.0: vgaarb: VGA decodes changed: olddecodes=io+mem,decodes=none:owns=mem
| xe 0000:00:02.0: [drm] Finished loading DMC firmware i915/tgl_dmc_ver2_12.bin (v2.12)
| xe 0000:00:02.0: [drm] Tile0: GT0: Using GuC firmware from i915/tgl_guc_70.bin version 70.49.4
| xe 0000:00:02.0: [drm] Tile0: GT0: Using HuC firmware from i915/tgl_huc.bin version 7.9.3
| xe 0000:00:02.0: [drm] Tile0: GT0: vcs1 fused off
| xe 0000:00:02.0: [drm] Tile0: GT0: vcs3 fused off
| xe 0000:00:02.0: [drm] Tile0: GT0: vcs4 fused off
| xe 0000:00:02.0: [drm] Tile0: GT0: vcs5 fused off
| xe 0000:00:02.0: [drm] Tile0: GT0: vcs6 fused off
| xe 0000:00:02.0: [drm] Tile0: GT0: vcs7 fused off
| xe 0000:00:02.0: [drm] Tile0: GT0: vecs1 fused off
| xe 0000:00:02.0: [drm] Tile0: GT0: vecs2 fused off
| xe 0000:00:02.0: [drm] Tile0: GT0: vecs3 fused off
| BUG: kernel NULL pointer dereference, address: 0000000000000000
| #PF: supervisor read access in kernel mode
| #PF: error_code(0x0000) - not-present page
| PGD 0 P4D 0
| Oops: Oops: 0000 [#1] SMP NOPTI
| CPU: 1 UID: 0 PID: 631 Comm: kworker/u32:9 Tainted: G     U      E       6.18.0-rc1+ #31 PREEMPT_{RT,(lazy)}
| Tainted: [U]=USER, [E]=UNSIGNED_MODULE
| Hardware name: LENOVO 20TD00GLGE/20TD00GLGE, BIOS R1EET64W(1.64 ) 03/18/2025
| Workqueue:  drm_sched_run_job_work [gpu_sched]
| RIP: 0010:stack_depot_save_flags+0x168/0xb00
| Code: c2 44 31 d0 41 c1 ca 08 44 29 d0 41 89 c0 45 89 c2 44 23 15 82 68 a1 03 49 c1 e2 04 4c 03 15 7f 68 a1 03 65 ff 05 f8 b9 f6 01 <4d> 8b 0a 4d 39 ca 75 1c e9 ae  00 00 00 66 66 2e 0f 1f 84 00 00 00
| RSP: 0018:ffffaa58c1a9f6a8 EFLAGS: 00010282
| RAX: 0000000006425a49 RBX: 0000000000000001 RCX: 000000000000000e
| RDX: 000000000000000e RSI: 00000000be7f6f1a RDI: 0000000000000001
| RBP: ffffaa58c1a9f700 R08: 0000000006425a49 R09: 000000000739c857
| R10: 0000000000000000 R11: 00000000000025b3 R12: 0000000000004502
| R13: ffff9c3a8f2b18b8 R14: 0000000000002800 R15: 000000000000000d
| FS:  0000000000000000(0000) GS:ffff9c3e55884000(0000) knlGS:0000000000000000
| CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
| CR2: 0000000000000000 CR3: 000000003e854003 CR4: 0000000000f72ef0
| PKRU: 55555554
| Call Trace:
| Call Trace:
|  <TASK>
|  fast_req_track+0x58/0xb0 [xe]
|  h2g_write+0x39f/0x720 [xe]
|  __guc_ct_send_locked+0x1e4/0x10f0 [xe]
|  guc_ct_send_locked+0xa4/0x690 [xe]
|  guc_ct_send+0x74/0x250 [xe]
|  xe_guc_ct_send+0x19/0x50 [xe]
|  __register_exec_queue.isra.0+0x7e/0xa0 [xe]
|  register_exec_queue+0x2f4/0x750 [xe]
|  guc_exec_queue_run_job+0x4f6/0x8f0 [xe]
|  drm_sched_run_job_work+0x1ef/0x450 [gpu_sched]

This happens also without the series and without PREEMPT_RT enabled. I
just to a while to figure this one out on the hardware in question since
it all just froze…

Other than that, XE seems fine.

i915.
- drm/i915/display: Move vblank put until after critical section
 - intel_vblank_evade() is invoked with irq-off
   - within its callchain intel_vblank_section_enter() does spin_lock()
     in I915. XE does nothing so it is fine.
   - intel_crtc_scanlines_since_frame_timestamp() does a while loop
     What is the expected/ possible worst case here and when does it happen?
   While at it, I noticed:
   local_irq_disable();
   if (new_plane_state->uapi.visible) {
           intel_plane_update_noarm(NULL, plane, crtc_state, new_plane_state);
           if (plane->fbc)
                  intel_fbc_dirty_rect_update_noarm(dsb, plane);
                     if (!HAS_FBC_DIRTY_RECT(display))
                         return;
                     mutex_lock(&fbc->lock); <----

   Haven't checked the callbacks but it feels like a lot of code with
   disabled interrupts.

- The GEM_BUG_ON(!irqs_disabled) in __i915_request_submit()/
  __i915_request_unsubmit() case bugs/ warnings:
| WARNING: CPU: 3 PID: 2115 at drivers/gpu/drm/i915/i915_request.c:611 __i915_request_submit+0x1db/0x1f0 [i915]
| CPU: 3 UID: 0 PID: 2115 Comm: modprobe Not tainted 6.18.0-rc1+ #17 PREEMPT_{RT,(lazy)}
| Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.30 06/29/2012
| RIP: 0010:__i915_request_submit+0x1db/0x1f0 [i915]
| Call Trace:
|  <TASK>
|  i915_request_submit+0x29/0x40 [i915]
|  i9xx_submit_request+0xe/0x70 [i915]
|  submit_notify+0xc1/0x230 [i915]
|  __i915_sw_fence_complete+0x88/0x290 [i915]
|  __engine_park+0x2d2/0x410 [i915]
|  ____intel_wakeref_put_last+0x25/0x90 [i915]
|  intel_gt_resume.part.0+0x2ec/0x380 [i915]
|  intel_gt_init+0x14d/0x3d0 [i915]
|  i915_gem_init+0x14b/0x290 [i915]
|  i915_driver_probe+0x74a/0xc10 [i915]
|  i915_pci_probe+0xd7/0x190 [i915]
|  local_pci_probe+0x41/0x80

- The change of irq-enable is not enough:
| BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
| in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 2115, name: modprobe
| preempt_count: 0, expected: 0
| RCU nest depth: 0, expected: 0
| 4 locks held by modprobe/2115:
|  #0: ffff99b9425161a0 (&dev->mutex){....}-{4:4}, at: __driver_attach+0xaf/0x1c0
|  #1: ffffaa224810f6c0 (crtc_ww_class_acquire){+.+.}-{0:0}, at: intel_initial_commit+0x4c/0x200 [i915]
|  #2: ffffaa224810f6e8 (crtc_ww_class_mutex){+.+.}-{4:4}, at: intel_initial_commit+0x4c/0x200 [i915]
|  #3: ffff99b94a6c9030 (&uncore->lock){+.+.}-{3:3}, at: gen6_write32+0x50/0x290 [i915]
| irq event stamp: 513344
| hardirqs last  enabled at (513343): [<ffffffff8ba8d84c>] _raw_spin_unlock_irqrestore+0x4c/0x60
| hardirqs last disabled at (513344): [<ffffffffc1543646>] intel_pipe_update_start+0x216/0x2c0 [i915]
| softirqs last  enabled at (512766): [<ffffffff8af045cf>] __local_bh_enable_ip+0x10f/0x170
| softirqs last disabled at (512712): [<ffffffffc14dfb6a>] __i915_request_queue+0x3a/0x70 [i915]
| CPU: 3 UID: 0 PID: 2115 Comm: modprobe Tainted: G        W           6.18.0-rc1+ #17 PREEMPT_{RT,(lazy)}
| Tainted: [W]=WARN
| Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.30 06/29/2012
| Call Trace:
|  <TASK>
|  dump_stack_lvl+0x68/0x90
|  __might_resched.cold+0xf0/0x12b
|  rt_spin_lock+0x5f/0x200
|  gen6_write32+0x50/0x290 [i915]
|  ilk_set_pipeconf+0x12d/0x230 [i915]
|  ilk_color_commit_arm+0x2d/0x70 [i915]
|  intel_update_crtc+0x15b/0x690 [i915]
|  intel_commit_modeset_enables+0xa6/0xd0 [i915]
|  intel_atomic_commit_tail+0xd55/0x19a0 [i915]
|  intel_atomic_commit+0x25d/0x2a0 [i915]
|  drm_atomic_commit+0xad/0xe0 [drm]
|  intel_initial_commit+0x16c/0x200 [i915]
|  intel_display_driver_probe+0x2e/0x80 [i915]
|  i915_driver_probe+0x791/0xc10 [i915]
|  i915_pci_probe+0xd7/0x190 [i915]

This is the intel_pipe_update_start()/ intel_pipe_update_end() part in intel_update_crtc().

Sebastian

Re: [PATCH v2 0/7] drm/i915/display: Handle vblank evasion with CONFIG_PREEMPT_RT

[Maarten Lankhorst]

Hey,

Den 2025-11-05 kl. 14:47, skrev Sebastian Andrzej Siewior:
> Thank you for looking into this. I applied this on top of
> drm-intel-next-2025-11-04 for testing.
> 
> On 2025-11-04 09:36:24 [+0100], Maarten Lankhorst wrote:
>> There is a critical section between intel_pipe_update_start() and
>> intel_pipe_update_end() where we only program hardware registers,
>> should not take any lock and complete as fast as possible.
>>
>> The previous approach used to remove the local_irq_enable/disable()
>> in this critical, but that increases the probability that the time
>> sensitive section does not complete in 100 µs, potentially causing
>> the hardware to hang.
>>
>> I went through all the lockdep splats that occurred in CI, and fixed
>> them 1 by 1 until there were none left. This additionally improves
>> latency by not removing any locks in the fastpath.
>>
>> In intel_de.h the implicit DMC wakelock was added, ideally we can get rid
>> of it, but for now we can simply use the same _fw variants as are
>> needed on i915.
>>
>> I believe this series is enough to make xe and perhaps good enough to make
>> i915's display RT safe.
> 
> I've been playing with it:
> - DRM_XE_DEBUG_GUC ended in a segfault:
> | xe 0000:00:02.0: enabling device (0006 -> 0007)
> | Console: switching to colour dummy device 80x25
> | xe 0000:00:02.0: vgaarb: deactivate vga console
> | xe 0000:00:02.0: [drm] Running in SR-IOV PF mode
> | xe 0000:00:02.0: [drm] Found tigerlake/uy (device ID 9a49) integrated display version 12.00 stepping C0
> | xe 0000:00:02.0: vgaarb: VGA decodes changed: olddecodes=io+mem,decodes=none:owns=mem
> | xe 0000:00:02.0: [drm] Finished loading DMC firmware i915/tgl_dmc_ver2_12.bin (v2.12)
> | xe 0000:00:02.0: [drm] Tile0: GT0: Using GuC firmware from i915/tgl_guc_70.bin version 70.49.4
> | xe 0000:00:02.0: [drm] Tile0: GT0: Using HuC firmware from i915/tgl_huc.bin version 7.9.3
> | xe 0000:00:02.0: [drm] Tile0: GT0: vcs1 fused off
> | xe 0000:00:02.0: [drm] Tile0: GT0: vcs3 fused off
> | xe 0000:00:02.0: [drm] Tile0: GT0: vcs4 fused off
> | xe 0000:00:02.0: [drm] Tile0: GT0: vcs5 fused off
> | xe 0000:00:02.0: [drm] Tile0: GT0: vcs6 fused off
> | xe 0000:00:02.0: [drm] Tile0: GT0: vcs7 fused off
> | xe 0000:00:02.0: [drm] Tile0: GT0: vecs1 fused off
> | xe 0000:00:02.0: [drm] Tile0: GT0: vecs2 fused off
> | xe 0000:00:02.0: [drm] Tile0: GT0: vecs3 fused off
> | BUG: kernel NULL pointer dereference, address: 0000000000000000
> | #PF: supervisor read access in kernel mode
> | #PF: error_code(0x0000) - not-present page
> | PGD 0 P4D 0
> | Oops: Oops: 0000 [#1] SMP NOPTI
> | CPU: 1 UID: 0 PID: 631 Comm: kworker/u32:9 Tainted: G     U      E       6.18.0-rc1+ #31 PREEMPT_{RT,(lazy)}
> | Tainted: [U]=USER, [E]=UNSIGNED_MODULE
> | Hardware name: LENOVO 20TD00GLGE/20TD00GLGE, BIOS R1EET64W(1.64 ) 03/18/2025
> | Workqueue:  drm_sched_run_job_work [gpu_sched]
> | RIP: 0010:stack_depot_save_flags+0x168/0xb00
> | Code: c2 44 31 d0 41 c1 ca 08 44 29 d0 41 89 c0 45 89 c2 44 23 15 82 68 a1 03 49 c1 e2 04 4c 03 15 7f 68 a1 03 65 ff 05 f8 b9 f6 01 <4d> 8b 0a 4d 39 ca 75 1c e9 ae  00 00 00 66 66 2e 0f 1f 84 00 00 00
> | RSP: 0018:ffffaa58c1a9f6a8 EFLAGS: 00010282
> | RAX: 0000000006425a49 RBX: 0000000000000001 RCX: 000000000000000e
> | RDX: 000000000000000e RSI: 00000000be7f6f1a RDI: 0000000000000001
> | RBP: ffffaa58c1a9f700 R08: 0000000006425a49 R09: 000000000739c857
> | R10: 0000000000000000 R11: 00000000000025b3 R12: 0000000000004502
> | R13: ffff9c3a8f2b18b8 R14: 0000000000002800 R15: 000000000000000d
> | FS:  0000000000000000(0000) GS:ffff9c3e55884000(0000) knlGS:0000000000000000
> | CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> | CR2: 0000000000000000 CR3: 000000003e854003 CR4: 0000000000f72ef0
> | PKRU: 55555554
> | Call Trace:
> | Call Trace:
> |  <TASK>
> |  fast_req_track+0x58/0xb0 [xe]
> |  h2g_write+0x39f/0x720 [xe]
> |  __guc_ct_send_locked+0x1e4/0x10f0 [xe]
> |  guc_ct_send_locked+0xa4/0x690 [xe]
> |  guc_ct_send+0x74/0x250 [xe]
> |  xe_guc_ct_send+0x19/0x50 [xe]
> |  __register_exec_queue.isra.0+0x7e/0xa0 [xe]
> |  register_exec_queue+0x2f4/0x750 [xe]
> |  guc_exec_queue_run_job+0x4f6/0x8f0 [xe]
> |  drm_sched_run_job_work+0x1ef/0x450 [gpu_sched]
> 
> This happens also without the series and without PREEMPT_RT enabled. I
> just to a while to figure this one out on the hardware in question since
> it all just froze…
> 
> Other than that, XE seems fine.
> 
> i915.
> - drm/i915/display: Move vblank put until after critical section
>  - intel_vblank_evade() is invoked with irq-off
>    - within its callchain intel_vblank_section_enter() does spin_lock()
>      in I915. XE does nothing so it is fine.
>    - intel_crtc_scanlines_since_frame_timestamp() does a while loop
>      What is the expected/ possible worst case here and when does it happen?
The typical case is no vblank happened between those reads, and returns without looping.
The worst case is a vblank happening in the hardware during that loop. If that happens, it's retried once.
PIPE_FRMTMSTMP triggers after every completed frame update. 
You have to complete the loop exactly once every 16.7 ms at 60 fps to loop forever at exactly the wrong time and then keep hitting it.
It looks like only a specific output on a few specific platforms are using it.

>    While at it, I noticed:
>    local_irq_disable();
>    if (new_plane_state->uapi.visible) {
>            intel_plane_update_noarm(NULL, plane, crtc_state, new_plane_state);
>            if (plane->fbc)
>                   intel_fbc_dirty_rect_update_noarm(dsb, plane);
>                      if (!HAS_FBC_DIRTY_RECT(display))
>                          return;
>                      mutex_lock(&fbc->lock); <----
> 
Eek, how did that slip through CI? There should be no lock whatsoever in there since that mutex would blow up on !RT too...

>    Haven't checked the callbacks but it feels like a lot of code with
>    disabled interrupts.
> 
> - The GEM_BUG_ON(!irqs_disabled) in __i915_request_submit()/
>   __i915_request_unsubmit() case bugs/ warnings:
> | WARNING: CPU: 3 PID: 2115 at drivers/gpu/drm/i915/i915_request.c:611 __i915_request_submit+0x1db/0x1f0 [i915]
> | CPU: 3 UID: 0 PID: 2115 Comm: modprobe Not tainted 6.18.0-rc1+ #17 PREEMPT_{RT,(lazy)}
> | Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.30 06/29/2012
> | RIP: 0010:__i915_request_submit+0x1db/0x1f0 [i915]
> | Call Trace:
> |  <TASK>
> |  i915_request_submit+0x29/0x40 [i915]
> |  i9xx_submit_request+0xe/0x70 [i915]
> |  submit_notify+0xc1/0x230 [i915]
> |  __i915_sw_fence_complete+0x88/0x290 [i915]
> |  __engine_park+0x2d2/0x410 [i915]
> |  ____intel_wakeref_put_last+0x25/0x90 [i915]
> |  intel_gt_resume.part.0+0x2ec/0x380 [i915]
> |  intel_gt_init+0x14d/0x3d0 [i915]
> |  i915_gem_init+0x14b/0x290 [i915]
> |  i915_driver_probe+0x74a/0xc10 [i915]
> |  i915_pci_probe+0xd7/0x190 [i915]
> |  local_pci_probe+0x41/0x80
I didn't apply the i915 specific patches that I saw in the -rt patchset, I only tested the xe display.
For i915, I did -DNOTRACE to the entire subdirectory to disable tracing on PREEMPT_RT,
but the other non-display related patches are still needed, did you apply those?

- 0003-drm-i915-Don-t-check-for-atomic-context-on-PREEMPT_R.patch
(0004 can be droped since upstream commit ed1fbee3debb ("drm/i915: Disable tracepoints for PREEMPT_RT"))
- 0005-drm-i915-gt-Use-spin_lock_irq-instead-of-local_irq_d.patch
- 0006-drm-i915-Drop-the-irqs_disabled-check.patch
- 0007-drm-i915-guc-Consider-also-RCU-depth-in-busy-loop.patch

> - The change of irq-enable is not enough:
> | BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
> | in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 2115, name: modprobe
> | preempt_count: 0, expected: 0
> | RCU nest depth: 0, expected: 0
> | 4 locks held by modprobe/2115:
> |  #0: ffff99b9425161a0 (&dev->mutex){....}-{4:4}, at: __driver_attach+0xaf/0x1c0
> |  #1: ffffaa224810f6c0 (crtc_ww_class_acquire){+.+.}-{0:0}, at: intel_initial_commit+0x4c/0x200 [i915]
> |  #2: ffffaa224810f6e8 (crtc_ww_class_mutex){+.+.}-{4:4}, at: intel_initial_commit+0x4c/0x200 [i915]
> |  #3: ffff99b94a6c9030 (&uncore->lock){+.+.}-{3:3}, at: gen6_write32+0x50/0x290 [i915]
> | irq event stamp: 513344
> | hardirqs last  enabled at (513343): [<ffffffff8ba8d84c>] _raw_spin_unlock_irqrestore+0x4c/0x60
> | hardirqs last disabled at (513344): [<ffffffffc1543646>] intel_pipe_update_start+0x216/0x2c0 [i915]
> | softirqs last  enabled at (512766): [<ffffffff8af045cf>] __local_bh_enable_ip+0x10f/0x170
> | softirqs last disabled at (512712): [<ffffffffc14dfb6a>] __i915_request_queue+0x3a/0x70 [i915]
> | CPU: 3 UID: 0 PID: 2115 Comm: modprobe Tainted: G        W           6.18.0-rc1+ #17 PREEMPT_{RT,(lazy)}
> | Tainted: [W]=WARN
> | Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.30 06/29/2012
> | Call Trace:
> |  <TASK>
> |  dump_stack_lvl+0x68/0x90
> |  __might_resched.cold+0xf0/0x12b
> |  rt_spin_lock+0x5f/0x200
> |  gen6_write32+0x50/0x290 [i915]
> |  ilk_set_pipeconf+0x12d/0x230 [i915]
> |  ilk_color_commit_arm+0x2d/0x70 [i915]
> |  intel_update_crtc+0x15b/0x690 [i915]
> |  intel_commit_modeset_enables+0xa6/0xd0 [i915]
> |  intel_atomic_commit_tail+0xd55/0x19a0 [i915]
> |  intel_atomic_commit+0x25d/0x2a0 [i915]
> |  drm_atomic_commit+0xad/0xe0 [drm]
> |  intel_initial_commit+0x16c/0x200 [i915]
> |  intel_display_driver_probe+0x2e/0x80 [i915]
> |  i915_driver_probe+0x791/0xc10 [i915]
> |  i915_pci_probe+0xd7/0x190 [i915]
> 
> This is the intel_pipe_update_start()/ intel_pipe_update_end() part in intel_update_crtc().

From the log it seems ilk_set_pipeconf() needs an update too.
Can you check if the warnings there go away when you replace intel_de_write() with intel_de_write_fw() and the intel_de_posting_read() with intel_de_read_fw() in ilk_set_pipeconf() and the listed i915 patches from PREEMPT_RT series applied?

Kind regards,
~Maarten Lankhorst

Re: [PATCH v2 0/7] drm/i915/display: Handle vblank evasion with CONFIG_PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2025-11-05 21:42:53 [+0100], Maarten Lankhorst wrote:
> Hey,
Hi,

sorry for the long break. I somehow marked this as read by accident.
Today I just checked if there was some progress and noticed that there
was indeed a reply…

> Den 2025-11-05 kl. 14:47, skrev Sebastian Andrzej Siewior:
> > i915.
> > - drm/i915/display: Move vblank put until after critical section
> >  - intel_vblank_evade() is invoked with irq-off
> >    - within its callchain intel_vblank_section_enter() does spin_lock()
> >      in I915. XE does nothing so it is fine.
> >    - intel_crtc_scanlines_since_frame_timestamp() does a while loop
> >      What is the expected/ possible worst case here and when does it happen?
> The typical case is no vblank happened between those reads, and returns without looping.
> The worst case is a vblank happening in the hardware during that loop. If that happens, it's retried once.
> PIPE_FRMTMSTMP triggers after every completed frame update. 
> You have to complete the loop exactly once every 16.7 ms at 60 fps to loop forever at exactly the wrong time and then keep hitting it.
> It looks like only a specific output on a few specific platforms are using it.

I see. I am just worried that this loops sometimes too often or at the
wrong time. But you are saying that this does not happen?

> >    While at it, I noticed:
> >    local_irq_disable();
> >    if (new_plane_state->uapi.visible) {
> >            intel_plane_update_noarm(NULL, plane, crtc_state, new_plane_state);
> >            if (plane->fbc)
> >                   intel_fbc_dirty_rect_update_noarm(dsb, plane);
> >                      if (!HAS_FBC_DIRTY_RECT(display))
> >                          return;
> >                      mutex_lock(&fbc->lock); <----
> > 
> Eek, how did that slip through CI? There should be no lock whatsoever
> in there since that mutex would blow up on !RT too...

yeah ;) Well I can't tell if this is magically somehow excluded but
based on review in looks possible.

> >    Haven't checked the callbacks but it feels like a lot of code with
> >    disabled interrupts.
> > 
> > - The GEM_BUG_ON(!irqs_disabled) in __i915_request_submit()/
> >   __i915_request_unsubmit() case bugs/ warnings:
> > | WARNING: CPU: 3 PID: 2115 at drivers/gpu/drm/i915/i915_request.c:611 __i915_request_submit+0x1db/0x1f0 [i915]
> > | CPU: 3 UID: 0 PID: 2115 Comm: modprobe Not tainted 6.18.0-rc1+ #17 PREEMPT_{RT,(lazy)}
> > | Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.30 06/29/2012
> > | RIP: 0010:__i915_request_submit+0x1db/0x1f0 [i915]
> > | Call Trace:
> > |  <TASK>
> > |  i915_request_submit+0x29/0x40 [i915]
> > |  i9xx_submit_request+0xe/0x70 [i915]
> > |  submit_notify+0xc1/0x230 [i915]
> > |  __i915_sw_fence_complete+0x88/0x290 [i915]
> > |  __engine_park+0x2d2/0x410 [i915]
> > |  ____intel_wakeref_put_last+0x25/0x90 [i915]
> > |  intel_gt_resume.part.0+0x2ec/0x380 [i915]
> > |  intel_gt_init+0x14d/0x3d0 [i915]
> > |  i915_gem_init+0x14b/0x290 [i915]
> > |  i915_driver_probe+0x74a/0xc10 [i915]
> > |  i915_pci_probe+0xd7/0x190 [i915]
> > |  local_pci_probe+0x41/0x80
> I didn't apply the i915 specific patches that I saw in the -rt patchset, I only tested the xe display.
> For i915, I did -DNOTRACE to the entire subdirectory to disable tracing on PREEMPT_RT,
> but the other non-display related patches are still needed, did you apply those?

Nope, I did just test what you provided. Your cover letter suggested
that it might be enough for i915 or I misunderstood.

I don't remember seeing any events for i915. So this -DNOTRACE must be
part of the existing patches somewhere…

> - 0003-drm-i915-Don-t-check-for-atomic-context-on-PREEMPT_R.patch
> (0004 can be droped since upstream commit ed1fbee3debb ("drm/i915: Disable tracepoints for PREEMPT_RT"))
> - 0005-drm-i915-gt-Use-spin_lock_irq-instead-of-local_irq_d.patch
> - 0006-drm-i915-Drop-the-irqs_disabled-check.patch
> - 0007-drm-i915-guc-Consider-also-RCU-depth-in-busy-loop.patch
> 
> > - The change of irq-enable is not enough:
> > | BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
> > | in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 2115, name: modprobe
> > | preempt_count: 0, expected: 0
> > | RCU nest depth: 0, expected: 0
> > | 4 locks held by modprobe/2115:
> > |  #0: ffff99b9425161a0 (&dev->mutex){....}-{4:4}, at: __driver_attach+0xaf/0x1c0
> > |  #1: ffffaa224810f6c0 (crtc_ww_class_acquire){+.+.}-{0:0}, at: intel_initial_commit+0x4c/0x200 [i915]
> > |  #2: ffffaa224810f6e8 (crtc_ww_class_mutex){+.+.}-{4:4}, at: intel_initial_commit+0x4c/0x200 [i915]
> > |  #3: ffff99b94a6c9030 (&uncore->lock){+.+.}-{3:3}, at: gen6_write32+0x50/0x290 [i915]
> > | irq event stamp: 513344
> > | hardirqs last  enabled at (513343): [<ffffffff8ba8d84c>] _raw_spin_unlock_irqrestore+0x4c/0x60
> > | hardirqs last disabled at (513344): [<ffffffffc1543646>] intel_pipe_update_start+0x216/0x2c0 [i915]
> > | softirqs last  enabled at (512766): [<ffffffff8af045cf>] __local_bh_enable_ip+0x10f/0x170
> > | softirqs last disabled at (512712): [<ffffffffc14dfb6a>] __i915_request_queue+0x3a/0x70 [i915]
> > | CPU: 3 UID: 0 PID: 2115 Comm: modprobe Tainted: G        W           6.18.0-rc1+ #17 PREEMPT_{RT,(lazy)}
> > | Tainted: [W]=WARN
> > | Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.30 06/29/2012
> > | Call Trace:
> > |  <TASK>
> > |  dump_stack_lvl+0x68/0x90
> > |  __might_resched.cold+0xf0/0x12b
> > |  rt_spin_lock+0x5f/0x200
> > |  gen6_write32+0x50/0x290 [i915]
> > |  ilk_set_pipeconf+0x12d/0x230 [i915]
> > |  ilk_color_commit_arm+0x2d/0x70 [i915]
> > |  intel_update_crtc+0x15b/0x690 [i915]
> > |  intel_commit_modeset_enables+0xa6/0xd0 [i915]
> > |  intel_atomic_commit_tail+0xd55/0x19a0 [i915]
> > |  intel_atomic_commit+0x25d/0x2a0 [i915]
> > |  drm_atomic_commit+0xad/0xe0 [drm]
> > |  intel_initial_commit+0x16c/0x200 [i915]
> > |  intel_display_driver_probe+0x2e/0x80 [i915]
> > |  i915_driver_probe+0x791/0xc10 [i915]
> > |  i915_pci_probe+0xd7/0x190 [i915]
> > 
> > This is the intel_pipe_update_start()/ intel_pipe_update_end() part in intel_update_crtc().
> 
> From the log it seems ilk_set_pipeconf() needs an update too.
> Can you check if the warnings there go away when you replace
> intel_de_write() with intel_de_write_fw() and the
> intel_de_posting_read() with intel_de_read_fw() in ilk_set_pipeconf()
> and the listed i915 patches from PREEMPT_RT series applied?

I added the following patches
- 0005-drm-i915-gt-Use-spin_lock_irq-instead-of-local_irq_d.patch
- 0006-drm-i915-Drop-the-irqs_disabled-check.patch
- 0007-drm-i915-guc-Consider-also-RCU-depth-in-busy-loop.patch
- drm-i915-Consider-RCU-read-section-as-atomic.patch

from https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/tree/patches?h=v6.18-rc4-rt3-patches

plus the diff below (which is mostly
0002-drm-i915-Don-t-disable-interrupts-on-PREEMPT_RT-duri.patch that
clashes now with one of the earlier patches). This looks without any
errors/ warnings.

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index ac433023636c3..3d4537c7ce672 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -3169,8 +3169,8 @@ void ilk_set_pipeconf(const struct intel_crtc_state *crtc_state)
 	val |= TRANSCONF_FRAME_START_DELAY(crtc_state->framestart_delay - 1);
 	val |= TRANSCONF_MSA_TIMING_DELAY(crtc_state->msa_timing_delay);
 
-	intel_de_write(display, TRANSCONF(display, cpu_transcoder), val);
-	intel_de_posting_read(display, TRANSCONF(display, cpu_transcoder));
+	intel_de_write_fw(display, TRANSCONF(display, cpu_transcoder), val);
+	intel_de_read_fw(display, TRANSCONF(display, cpu_transcoder));
 }
 
 static void hsw_set_transconf(const struct intel_crtc_state *crtc_state)
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 5e939004b6463..40a9234e6e5dc 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
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index ef429b81a5228..d69b3763380c3 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -570,7 +570,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;
@@ -588,7 +589,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)
diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
index 6b3bc8d94e51a..d9f1e7e72f500 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -922,11 +922,13 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
 		 */
 		intel_psr_wait_for_idle_locked(crtc_state);
 
-		local_irq_disable();
+		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+			local_irq_disable();
 
 		intel_vblank_evade(&evade);
 	} else {
-		local_irq_disable();
+		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+			local_irq_disable();
 	}
 
 	if (new_plane_state->uapi.visible) {
@@ -936,7 +938,8 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
 		intel_plane_disable_arm(NULL, plane, crtc_state);
 	}
 
-	local_irq_enable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_irq_enable();
 
 	intel_psr_unlock(crtc_state);
 
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index a7f9195215f30..ad62e32da06a3 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -750,7 +750,8 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
 		return 0;
 
 	while (!vblank_evadable(evade, &scanline)) {
-		local_irq_enable();
+		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+			local_irq_enable();
 
 		DEFINE_WAIT(wait);
 		while (!vblank_evadable(evade, &scanline) && timeout > 0) {
@@ -759,7 +760,8 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
 		}
 		finish_wait(wq, &wait);
 
-		local_irq_disable();
+		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+			local_irq_disable();
 
 		if (!timeout) {
 			drm_dbg_kms(display->drm,

> Kind regards,
> ~Maarten Lankhorst

Sebastian

Re: [PATCH v2 0/7] drm/i915/display: Handle vblank evasion with CONFIG_PREEMPT_RT

[Maarten Lankhorst]

Hey,

Den 2025-11-10 kl. 17:09, skrev Sebastian Andrzej Siewior:
> On 2025-11-05 21:42:53 [+0100], Maarten Lankhorst wrote:
>> Hey,
> Hi,
> 
> sorry for the long break. I somehow marked this as read by accident.
> Today I just checked if there was some progress and noticed that there
> was indeed a reply…
> 
>> Den 2025-11-05 kl. 14:47, skrev Sebastian Andrzej Siewior:
>>> i915.
>>> - drm/i915/display: Move vblank put until after critical section
>>>  - intel_vblank_evade() is invoked with irq-off
>>>    - within its callchain intel_vblank_section_enter() does spin_lock()
>>>      in I915. XE does nothing so it is fine.
>>>    - intel_crtc_scanlines_since_frame_timestamp() does a while loop
>>>      What is the expected/ possible worst case here and when does it happen?
>> The typical case is no vblank happened between those reads, and returns without looping.
>> The worst case is a vblank happening in the hardware during that loop. If that happens, it's retried once.
>> PIPE_FRMTMSTMP triggers after every completed frame update. 
>> You have to complete the loop exactly once every 16.7 ms at 60 fps to loop forever at exactly the wrong time and then keep hitting it.
>> It looks like only a specific output on a few specific platforms are using it.
> 
> I see. I am just worried that this loops sometimes too often or at the
> wrong time. But you are saying that this does not happen?
> 
>>>    While at it, I noticed:
>>>    local_irq_disable();
>>>    if (new_plane_state->uapi.visible) {
>>>            intel_plane_update_noarm(NULL, plane, crtc_state, new_plane_state);
>>>            if (plane->fbc)
>>>                   intel_fbc_dirty_rect_update_noarm(dsb, plane);
>>>                      if (!HAS_FBC_DIRTY_RECT(display))
>>>                          return;
>>>                      mutex_lock(&fbc->lock); <----
>>>
>> Eek, how did that slip through CI? There should be no lock whatsoever
>> in there since that mutex would blow up on !RT too...
> 
> yeah ;) Well I can't tell if this is magically somehow excluded but
> based on review in looks possible.
> 
>>>    Haven't checked the callbacks but it feels like a lot of code with
>>>    disabled interrupts.
>>>
>>> - The GEM_BUG_ON(!irqs_disabled) in __i915_request_submit()/
>>>   __i915_request_unsubmit() case bugs/ warnings:
>>> | WARNING: CPU: 3 PID: 2115 at drivers/gpu/drm/i915/i915_request.c:611 __i915_request_submit+0x1db/0x1f0 [i915]
>>> | CPU: 3 UID: 0 PID: 2115 Comm: modprobe Not tainted 6.18.0-rc1+ #17 PREEMPT_{RT,(lazy)}
>>> | Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.30 06/29/2012
>>> | RIP: 0010:__i915_request_submit+0x1db/0x1f0 [i915]
>>> | Call Trace:
>>> |  <TASK>
>>> |  i915_request_submit+0x29/0x40 [i915]
>>> |  i9xx_submit_request+0xe/0x70 [i915]
>>> |  submit_notify+0xc1/0x230 [i915]
>>> |  __i915_sw_fence_complete+0x88/0x290 [i915]
>>> |  __engine_park+0x2d2/0x410 [i915]
>>> |  ____intel_wakeref_put_last+0x25/0x90 [i915]
>>> |  intel_gt_resume.part.0+0x2ec/0x380 [i915]
>>> |  intel_gt_init+0x14d/0x3d0 [i915]
>>> |  i915_gem_init+0x14b/0x290 [i915]
>>> |  i915_driver_probe+0x74a/0xc10 [i915]
>>> |  i915_pci_probe+0xd7/0x190 [i915]
>>> |  local_pci_probe+0x41/0x80
>> I didn't apply the i915 specific patches that I saw in the -rt patchset, I only tested the xe display.
>> For i915, I did -DNOTRACE to the entire subdirectory to disable tracing on PREEMPT_RT,
>> but the other non-display related patches are still needed, did you apply those?
> 
> Nope, I did just test what you provided. Your cover letter suggested
> that it might be enough for i915 or I misunderstood.
> 
> I don't remember seeing any events for i915. So this -DNOTRACE must be
> part of the existing patches somewhere…
> 
>> - 0003-drm-i915-Don-t-check-for-atomic-context-on-PREEMPT_R.patch
>> (0004 can be droped since upstream commit ed1fbee3debb ("drm/i915: Disable tracepoints for PREEMPT_RT"))
>> - 0005-drm-i915-gt-Use-spin_lock_irq-instead-of-local_irq_d.patch
>> - 0006-drm-i915-Drop-the-irqs_disabled-check.patch
>> - 0007-drm-i915-guc-Consider-also-RCU-depth-in-busy-loop.patch
>>
>>> - The change of irq-enable is not enough:
>>> | BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
>>> | in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 2115, name: modprobe
>>> | preempt_count: 0, expected: 0
>>> | RCU nest depth: 0, expected: 0
>>> | 4 locks held by modprobe/2115:
>>> |  #0: ffff99b9425161a0 (&dev->mutex){....}-{4:4}, at: __driver_attach+0xaf/0x1c0
>>> |  #1: ffffaa224810f6c0 (crtc_ww_class_acquire){+.+.}-{0:0}, at: intel_initial_commit+0x4c/0x200 [i915]
>>> |  #2: ffffaa224810f6e8 (crtc_ww_class_mutex){+.+.}-{4:4}, at: intel_initial_commit+0x4c/0x200 [i915]
>>> |  #3: ffff99b94a6c9030 (&uncore->lock){+.+.}-{3:3}, at: gen6_write32+0x50/0x290 [i915]
>>> | irq event stamp: 513344
>>> | hardirqs last  enabled at (513343): [<ffffffff8ba8d84c>] _raw_spin_unlock_irqrestore+0x4c/0x60
>>> | hardirqs last disabled at (513344): [<ffffffffc1543646>] intel_pipe_update_start+0x216/0x2c0 [i915]
>>> | softirqs last  enabled at (512766): [<ffffffff8af045cf>] __local_bh_enable_ip+0x10f/0x170
>>> | softirqs last disabled at (512712): [<ffffffffc14dfb6a>] __i915_request_queue+0x3a/0x70 [i915]
>>> | CPU: 3 UID: 0 PID: 2115 Comm: modprobe Tainted: G        W           6.18.0-rc1+ #17 PREEMPT_{RT,(lazy)}
>>> | Tainted: [W]=WARN
>>> | Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.30 06/29/2012
>>> | Call Trace:
>>> |  <TASK>
>>> |  dump_stack_lvl+0x68/0x90
>>> |  __might_resched.cold+0xf0/0x12b
>>> |  rt_spin_lock+0x5f/0x200
>>> |  gen6_write32+0x50/0x290 [i915]
>>> |  ilk_set_pipeconf+0x12d/0x230 [i915]
>>> |  ilk_color_commit_arm+0x2d/0x70 [i915]
>>> |  intel_update_crtc+0x15b/0x690 [i915]
>>> |  intel_commit_modeset_enables+0xa6/0xd0 [i915]
>>> |  intel_atomic_commit_tail+0xd55/0x19a0 [i915]
>>> |  intel_atomic_commit+0x25d/0x2a0 [i915]
>>> |  drm_atomic_commit+0xad/0xe0 [drm]
>>> |  intel_initial_commit+0x16c/0x200 [i915]
>>> |  intel_display_driver_probe+0x2e/0x80 [i915]
>>> |  i915_driver_probe+0x791/0xc10 [i915]
>>> |  i915_pci_probe+0xd7/0x190 [i915]
>>>
>>> This is the intel_pipe_update_start()/ intel_pipe_update_end() part in intel_update_crtc().
>>
>> From the log it seems ilk_set_pipeconf() needs an update too.
>> Can you check if the warnings there go away when you replace
>> intel_de_write() with intel_de_write_fw() and the
>> intel_de_posting_read() with intel_de_read_fw() in ilk_set_pipeconf()
>> and the listed i915 patches from PREEMPT_RT series applied?
> 
> I added the following patches
> - 0005-drm-i915-gt-Use-spin_lock_irq-instead-of-local_irq_d.patch
> - 0006-drm-i915-Drop-the-irqs_disabled-check.patch
> - 0007-drm-i915-guc-Consider-also-RCU-depth-in-busy-loop.patch
> - drm-i915-Consider-RCU-read-section-as-atomic.patch
> 
> from https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/tree/patches?h=v6.18-rc4-rt3-patches
> 
> plus the diff below (which is mostly
> 0002-drm-i915-Don-t-disable-interrupts-on-PREEMPT_RT-duri.patch that
> clashes now with one of the earlier patches). This looks without any
> errors/ warnings.
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index ac433023636c3..3d4537c7ce672 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -3169,8 +3169,8 @@ void ilk_set_pipeconf(const struct intel_crtc_state *crtc_state)
>  	val |= TRANSCONF_FRAME_START_DELAY(crtc_state->framestart_delay - 1);
>  	val |= TRANSCONF_MSA_TIMING_DELAY(crtc_state->msa_timing_delay);
>  
> -	intel_de_write(display, TRANSCONF(display, cpu_transcoder), val);
> -	intel_de_posting_read(display, TRANSCONF(display, cpu_transcoder));
> +	intel_de_write_fw(display, TRANSCONF(display, cpu_transcoder), val);
> +	intel_de_read_fw(display, TRANSCONF(display, cpu_transcoder));
>  }
>  
>  static void hsw_set_transconf(const struct intel_crtc_state *crtc_state)
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 5e939004b6463..40a9234e6e5dc 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

I would like to recommend dropping the patch below. The hardware doesn't like being
programmed during vblank time, and may lock up or show glitches on the screen,
especially at older machines.

That's why the whole complicated preparations exist, to be able to complete
programming the hardware before the vblank.

I created my series to be able to run that the timing sensitive parts safely without
any jitter from locking in between.

I tried running the following series through CI in response:
https://patchwork.freedesktop.org/series/157258/ 

After looking at the results and investigating more closely, I think the FBC warning
you mentioned is a false positive.

The code either runs in intel_pre_plane_update() when programming the hardware directly,
or it gets added to the list of mmio's programmed by the hardware, without the
vblank evasion mode active.

Still I would like to clean it up, but it's not as urgent as it was.

> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index ef429b81a5228..d69b3763380c3 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -570,7 +570,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;
> @@ -588,7 +589,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)
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 6b3bc8d94e51a..d9f1e7e72f500 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -922,11 +922,13 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>  		 */
>  		intel_psr_wait_for_idle_locked(crtc_state);
>  
> -		local_irq_disable();
> +		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +			local_irq_disable();
>  
>  		intel_vblank_evade(&evade);
>  	} else {
> -		local_irq_disable();
> +		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +			local_irq_disable();
>  	}
>  
>  	if (new_plane_state->uapi.visible) {
> @@ -936,7 +938,8 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>  		intel_plane_disable_arm(NULL, plane, crtc_state);
>  	}
>  
> -	local_irq_enable();
> +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +		local_irq_enable();
>  
>  	intel_psr_unlock(crtc_state);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index a7f9195215f30..ad62e32da06a3 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -750,7 +750,8 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>  		return 0;
>  
>  	while (!vblank_evadable(evade, &scanline)) {
> -		local_irq_enable();
> +		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +			local_irq_enable();
>  
>  		DEFINE_WAIT(wait);
>  		while (!vblank_evadable(evade, &scanline) && timeout > 0) {
> @@ -759,7 +760,8 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>  		}
>  		finish_wait(wq, &wait);
>  
> -		local_irq_disable();
> +		if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +			local_irq_disable();
>  
>  		if (!timeout) {
>  			drm_dbg_kms(display->drm,
> 
> 
> Sebastian

Kind regards,
~Maarten Lankhorst

Re: [PATCH v2 0/7] drm/i915/display: Handle vblank evasion with CONFIG_PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2025-11-10 19:17:46 [+0100], Maarten Lankhorst wrote:
> Hey,
Hi Maarten,

> I would like to recommend dropping the patch below. The hardware doesn't like being
> programmed during vblank time, and may lock up or show glitches on the screen,
> especially at older machines.

Okay. I did include them for testing as requested. I would keep them in
the series until we settle on something upstream.

> That's why the whole complicated preparations exist, to be able to complete
> programming the hardware before the vblank.
> 
> I created my series to be able to run that the timing sensitive parts safely without
> any jitter from locking in between.
> 
> I tried running the following series through CI in response:
> https://patchwork.freedesktop.org/series/157258/ 
> 
> After looking at the results and investigating more closely, I think the FBC warning
> you mentioned is a false positive.
> 
> The code either runs in intel_pre_plane_update() when programming the hardware directly,
> or it gets added to the list of mmio's programmed by the hardware, without the
> vblank evasion mode active.
> 
> Still I would like to clean it up, but it's not as urgent as it was.

Okay. Good.
If there is anything you want me to test or look at, just le me know.

> Kind regards,
> ~Maarten Lankhorst

Sebastian

RE: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in intel_pipe_fastset

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Maarten
> Lankhorst
> Sent: Tuesday, November 4, 2025 2:06 PM
> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst <dev@lankhorst.se>; Mario
> Kleiner <mario.kleiner.de@gmail.com>; Mike Galbraith
> <umgwanakikbuti@gmail.com>; Thomas Gleixner <tglx@linutronix.de>; Sebastian
> Andrzej Siewior <bigeasy@linutronix.de>; Clark Williams
> <clrkwllms@kernel.org>; Steven Rostedt <rostedt@goodmis.org>
> Subject: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in
> intel_pipe_fastset
> 
> intel_set_pipe_src_size(), hsw_set_linetime_wm(),
> intel_cpu_transcoder_set_m1_n1() and intel_set_transcoder_timings_lrr()
> are called from an atomic context on PREEMPT_RT, and should be using the _fw
> functions.

This could be ok but we need to be sure that all are called with power domains up.
I think would be safe to keep this under RT check so that we don't end up breaking any
generic non RT usecase.

@Ville Syrjälä Any thoughts on this ?

Regards,
Uma Shankar

> This likely prevents a deadlock on i915.
> 
> Again noticed when trying to disable preemption in vblank evasion:
> <3> BUG: sleeping function called from invalid context at
> kernel/locking/spinlock_rt.c:48 <3> in_atomic(): 1, irqs_disabled(): 0, non_block:
> 0, pid: 1505, name: kms_cursor_lega <3> preempt_count: 1, expected: 0 <3>
> RCU nest depth: 0, expected: 0 <4> 4 locks held by kms_cursor_lega/1505:
> <4>  #0: ffffc90003c6f988 (crtc_ww_class_acquire){+.+.}-{0:0}, at:
> drm_mode_atomic_ioctl+0x13b/0xe90 <4>  #1: ffffc90003c6f9b0
> (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_mode_atomic_ioctl+0x13b/0xe90 <4>
> #2: ffff888135b838b8 (&intel_dp->psr.lock){+.+.}-{3:3}, at:
> intel_psr_lock+0xc5/0xf0 [xe] <4>  #3: ffff88812607bbc0 (&wl->lock){+.+.}-{2:2},
> at: intel_dmc_wl_get+0x3c/0x140 [xe]
> <4> CPU: 6 UID: 0 PID: 1505 Comm: kms_cursor_lega Tainted: G     U
> 6.18.0-rc3-lgci-xe-xe-pw-156729v1+ #1 PREEMPT_{RT,(lazy)}
> <4> Tainted: [U]=USER
> <4> Hardware name: Intel Corporation Panther Lake Client Platform/PTL-UH LP5
> T3 RVP1, BIOS PTLPFWI1.R00.3383.D02.2509240621 09/24/2025 <4> Call Trace:
> <4>  <TASK>
> <4>  dump_stack_lvl+0xc1/0xf0
> <4>  dump_stack+0x10/0x20
> <4>  __might_resched+0x174/0x260
> <4>  rt_spin_lock+0x63/0x200
> <4>  ? intel_dmc_wl_get+0x3c/0x140 [xe]
> <4>  intel_dmc_wl_get+0x3c/0x140 [xe]
> <4>  intel_set_pipe_src_size+0x89/0xe0 [xe] <4>  intel_update_crtc+0x3c1/0x950
> [xe] <4>  ? intel_pre_update_crtc+0x258/0x400 [xe] <4>
> skl_commit_modeset_enables+0x217/0x720 [xe] <4>
> intel_atomic_commit_tail+0xd4e/0x1af0 [xe] <4>  ? lock_release+0xce/0x2a0 <4>
> intel_atomic_commit+0x2e5/0x330 [xe] <4>  ? intel_atomic_commit+0x2e5/0x330
> [xe] <4>  drm_atomic_commit+0xaf/0xf0 <4>  ?
> __pfx___drm_printfn_info+0x10/0x10
> <4>  drm_mode_atomic_ioctl+0xbd5/0xe90
> <4>  ? lock_acquire+0xc4/0x2e0
> <4>  ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
> <4>  drm_ioctl_kernel+0xb6/0x120
> <4>  drm_ioctl+0x2d7/0x5a0
> <4>  ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
> <4>  ? rt_spin_unlock+0xa0/0x140
> <4>  ? __pm_runtime_resume+0x53/0x90
> <4>  xe_drm_ioctl+0x56/0x90 [xe]
> <4>  __x64_sys_ioctl+0xa8/0x110
> <4>  ? lock_acquire+0xc4/0x2e0
> <4>  x64_sys_call+0x1144/0x26a0
> <4>  do_syscall_64+0x93/0xae0
> <4>  ? lock_release+0xce/0x2a0
> <4>  ? __task_pid_nr_ns+0xd9/0x270
> <4>  ? do_syscall_64+0x1b7/0xae0
> <4>  ? find_held_lock+0x31/0x90
> <4>  ? __task_pid_nr_ns+0xcf/0x270
> <4>  ? __lock_acquire+0x43e/0x2860
> <4>  ? __task_pid_nr_ns+0xd9/0x270
> <4>  ? lock_acquire+0xc4/0x2e0
> <4>  ? find_held_lock+0x31/0x90
> <4>  ? __task_pid_nr_ns+0xcf/0x270
> <4>  ? lock_release+0xce/0x2a0
> <4>  ? __task_pid_nr_ns+0xd9/0x270
> <4>  ? do_syscall_64+0x1b7/0xae0
> <4>  ? do_syscall_64+0x1b7/0xae0
> <4>  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++++++----------
>  drivers/gpu/drm/i915/display/intel_vrr.c     | 16 ++++-----
>  2 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 42ec787986666..1bff1148fe9d7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1573,9 +1573,9 @@ static void hsw_set_linetime_wm(const struct
> intel_crtc_state *crtc_state)
>  	struct intel_display *display = to_intel_display(crtc_state);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> 
> -	intel_de_write(display, WM_LINETIME(crtc->pipe),
> -		       HSW_LINETIME(crtc_state->linetime) |
> -		       HSW_IPS_LINETIME(crtc_state->ips_linetime));
> +	intel_de_write_fw(display, WM_LINETIME(crtc->pipe),
> +			  HSW_LINETIME(crtc_state->linetime) |
> +			  HSW_IPS_LINETIME(crtc_state->ips_linetime));
>  }
> 
>  static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> @@ -2543,14 +2543,14 @@ void intel_set_m_n(struct intel_display *display,
>  		   i915_reg_t data_m_reg, i915_reg_t data_n_reg,
>  		   i915_reg_t link_m_reg, i915_reg_t link_n_reg)  {
> -	intel_de_write(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m);
> -	intel_de_write(display, data_n_reg, m_n->data_n);
> -	intel_de_write(display, link_m_reg, m_n->link_m);
> +	intel_de_write_fw(display, data_m_reg, TU_SIZE(m_n->tu) | m_n-
> >data_m);
> +	intel_de_write_fw(display, data_n_reg, m_n->data_n);
> +	intel_de_write_fw(display, link_m_reg, m_n->link_m);
>  	/*
>  	 * On BDW+ writing LINK_N arms the double buffered update
>  	 * of all the M/N registers, so it must be written last.
>  	 */
> -	intel_de_write(display, link_n_reg, m_n->link_n);
> +	intel_de_write_fw(display, link_n_reg, m_n->link_n);
>  }
> 
>  bool intel_cpu_transcoder_has_m2_n2(struct intel_display *display, @@ -2737,9
> +2737,9 @@ static void intel_set_transcoder_timings_lrr(const struct
> intel_crtc_state *crtc
>  	}
> 
>  	if (DISPLAY_VER(display) >= 13) {
> -		intel_de_write(display,
> -			       TRANS_SET_CONTEXT_LATENCY(display,
> cpu_transcoder),
> -			       crtc_state->set_context_latency);
> +		intel_de_write_fw(display,
> +				  TRANS_SET_CONTEXT_LATENCY(display,
> cpu_transcoder),
> +				  crtc_state->set_context_latency);
> 
>  		/*
>  		 * VBLANK_START not used by hw, just clear it @@ -2755,9
> +2755,9 @@ static void intel_set_transcoder_timings_lrr(const struct
> intel_crtc_state *crtc
>  	 * The hardware actually ignores TRANS_VBLANK.VBLANK_END in DP
> mode.
>  	 * But let's write it anyway to keep the state checker happy.
>  	 */
> -	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
> -		       VBLANK_START(crtc_vblank_start - 1) |
> -		       VBLANK_END(crtc_vblank_end - 1));
> +	intel_de_write_fw(display, TRANS_VBLANK(display, cpu_transcoder),
> +			  VBLANK_START(crtc_vblank_start - 1) |
> +			  VBLANK_END(crtc_vblank_end - 1));
>  	/*
>  	 * For platforms that always use VRR Timing Generator, the
> VTOTAL.Vtotal
>  	 * bits are not required. Since the support for these bits is going to @@ -
> 2771,9 +2771,9 @@ static void intel_set_transcoder_timings_lrr(const struct
> intel_crtc_state *crtc
>  	 * The double buffer latch point for TRANS_VTOTAL
>  	 * is the transcoder's undelayed vblank.
>  	 */
> -	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
> -		       VACTIVE(crtc_vdisplay - 1) |
> -		       VTOTAL(crtc_vtotal - 1));
> +	intel_de_write_fw(display, TRANS_VTOTAL(display, cpu_transcoder),
> +			  VACTIVE(crtc_vdisplay - 1) |
> +			  VTOTAL(crtc_vtotal - 1));
> 
>  	intel_vrr_set_fixed_rr_timings(crtc_state);
>  	intel_vrr_transcoder_enable(crtc_state);
> @@ -2790,8 +2790,8 @@ static void intel_set_pipe_src_size(const struct
> intel_crtc_state *crtc_state)
>  	/* pipesrc controls the size that is scaled from, which should
>  	 * always be the user's requested size.
>  	 */
> -	intel_de_write(display, PIPESRC(display, pipe),
> -		       PIPESRC_WIDTH(width - 1) | PIPESRC_HEIGHT(height -
> 1));
> +	intel_de_write_fw(display, PIPESRC(display, pipe),
> +			  PIPESRC_WIDTH(width - 1) | PIPESRC_HEIGHT(height
> - 1));
>  }
> 
>  static bool intel_pipe_is_interlaced(const struct intel_crtc_state *crtc_state) diff --
> git a/drivers/gpu/drm/i915/display/intel_vrr.c
> b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 00cbc126fb366..2e19673697fa4 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -300,12 +300,12 @@ void intel_vrr_set_fixed_rr_timings(const struct
> intel_crtc_state *crtc_state)
>  	if (!intel_vrr_possible(crtc_state))
>  		return;
> 
> -	intel_de_write(display, TRANS_VRR_VMIN(display, cpu_transcoder),
> -		       intel_vrr_fixed_rr_hw_vmin(crtc_state) - 1);
> -	intel_de_write(display, TRANS_VRR_VMAX(display, cpu_transcoder),
> -		       intel_vrr_fixed_rr_hw_vmax(crtc_state) - 1);
> -	intel_de_write(display, TRANS_VRR_FLIPLINE(display, cpu_transcoder),
> -		       intel_vrr_fixed_rr_hw_flipline(crtc_state) - 1);
> +	intel_de_write_fw(display, TRANS_VRR_VMIN(display, cpu_transcoder),
> +			  intel_vrr_fixed_rr_hw_vmin(crtc_state) - 1);
> +	intel_de_write_fw(display, TRANS_VRR_VMAX(display, cpu_transcoder),
> +			  intel_vrr_fixed_rr_hw_vmax(crtc_state) - 1);
> +	intel_de_write_fw(display, TRANS_VRR_FLIPLINE(display,
> cpu_transcoder),
> +			  intel_vrr_fixed_rr_hw_flipline(crtc_state) - 1);
>  }
> 
>  static
> @@ -693,7 +693,7 @@ static void intel_vrr_tg_enable(const struct
> intel_crtc_state *crtc_state,
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  	u32 vrr_ctl;
> 
> -	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
> TRANS_PUSH_EN);
> +	intel_de_write_fw(display, TRANS_PUSH(display, cpu_transcoder),
> +TRANS_PUSH_EN);
> 
>  	vrr_ctl = VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state);
> 
> @@ -705,7 +705,7 @@ static void intel_vrr_tg_enable(const struct
> intel_crtc_state *crtc_state,
>  	if (cmrr_enable)
>  		vrr_ctl |= VRR_CTL_CMRR_ENABLE;
> 
> -	intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> vrr_ctl);
> +	intel_de_write_fw(display, TRANS_VRR_CTL(display, cpu_transcoder),
> +vrr_ctl);
>  }
> 
>  static void intel_vrr_tg_disable(const struct intel_crtc_state *old_crtc_state)
> --
> 2.51.0

RE: [PATCH v2 3/7] drm/i915/display: Move vblank put until after critical section

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Maarten
> Lankhorst
> Sent: Tuesday, November 4, 2025 2:06 PM
> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst <dev@lankhorst.se>; Mario
> Kleiner <mario.kleiner.de@gmail.com>; Mike Galbraith
> <umgwanakikbuti@gmail.com>; Thomas Gleixner <tglx@linutronix.de>; Sebastian
> Andrzej Siewior <bigeasy@linutronix.de>; Clark Williams
> <clrkwllms@kernel.org>; Steven Rostedt <rostedt@goodmis.org>
> Subject: [PATCH v2 3/7] drm/i915/display: Move vblank put until after critical
> section
> 
> drm_crtc_vblank_put may take some locks, this should probably not be the first
> thing we do after entering the time sensitive part.
> 
> A better place is after programming is completed. Add a flag to put the vblank after
> completion.
> 
> In the case of drm_vblank_work_schedule, we may not even need to disable the
> vblank interrupt any more if it takes its own reference.

This looks good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/i915/display/intel_cursor.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 7aa14348aa6d4..6b3bc8d94e51a 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -816,6 +816,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>  		to_intel_crtc_state(crtc->base.state);
>  	struct intel_crtc_state *new_crtc_state;
>  	struct intel_vblank_evade_ctx evade;
> +	bool has_vblank = false;
>  	int ret;
> 
>  	/*
> @@ -913,6 +914,8 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>  	intel_psr_lock(crtc_state);
> 
>  	if (!drm_WARN_ON(display->drm, drm_crtc_vblank_get(&crtc->base))) {
> +		has_vblank = true;
> +
>  		/*
>  		 * TODO: maybe check if we're still in PSR
>  		 * and skip the vblank evasion entirely?
> @@ -922,8 +925,6 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>  		local_irq_disable();
> 
>  		intel_vblank_evade(&evade);
> -
> -		drm_crtc_vblank_put(&crtc->base);
>  	} else {
>  		local_irq_disable();
>  	}
> @@ -939,6 +940,9 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
> 
>  	intel_psr_unlock(crtc_state);
> 
> +	if (has_vblank)
> +		drm_crtc_vblank_put(&crtc->base);
> +
>  	if (old_plane_state->ggtt_vma != new_plane_state->ggtt_vma) {
>  		drm_vblank_work_init(&old_plane_state->unpin_work, &crtc-
> >base,
>  				     intel_cursor_unpin_work);
> --
> 2.51.0

Re: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in intel_pipe_fastset

[Ville Syrjälä]

On Wed, Nov 26, 2025 at 07:19:47PM +0000, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Maarten
> > Lankhorst
> > Sent: Tuesday, November 4, 2025 2:06 PM
> > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst <dev@lankhorst.se>; Mario
> > Kleiner <mario.kleiner.de@gmail.com>; Mike Galbraith
> > <umgwanakikbuti@gmail.com>; Thomas Gleixner <tglx@linutronix.de>; Sebastian
> > Andrzej Siewior <bigeasy@linutronix.de>; Clark Williams
> > <clrkwllms@kernel.org>; Steven Rostedt <rostedt@goodmis.org>
> > Subject: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in
> > intel_pipe_fastset
> > 
> > intel_set_pipe_src_size(), hsw_set_linetime_wm(),
> > intel_cpu_transcoder_set_m1_n1() and intel_set_transcoder_timings_lrr()
> > are called from an atomic context on PREEMPT_RT, and should be using the _fw
> > functions.
> 
> This could be ok but we need to be sure that all are called with power domains up.
> I think would be safe to keep this under RT check so that we don't end up breaking any
> generic non RT usecase.

When removing the locks from register accesses one needs to consider
what platforms the code runs on, what other register are on the same
cacheline, and whether they can be accessed in parallel. If there is
something there then we may not be able to remove the locks.

That's assuming the "system hangs when same cacheline is accessed from
multiple cpus" issue is real for display registers, and I'm actually
not 100% it is. But we'd need to run some tests on the affected systems
(~ivb/hsw) to get any kind of confidence here. IIRC some old
intel_gpu_top thhat directly poked the registers was very good at
hitting it on hsw at least, so that would be a decent starting point.

Anyways, I'm going to be replacing the uncore lock with a display
specific lock soonish, and I suppose I can just make that a raw
spinlock to appease RT.

> 
> @Ville Syrjälä Any thoughts on this ?
> 
> Regards,
> Uma Shankar
> 
> > This likely prevents a deadlock on i915.
> > 
> > Again noticed when trying to disable preemption in vblank evasion:
> > <3> BUG: sleeping function called from invalid context at
> > kernel/locking/spinlock_rt.c:48 <3> in_atomic(): 1, irqs_disabled(): 0, non_block:
> > 0, pid: 1505, name: kms_cursor_lega <3> preempt_count: 1, expected: 0 <3>
> > RCU nest depth: 0, expected: 0 <4> 4 locks held by kms_cursor_lega/1505:
> > <4>  #0: ffffc90003c6f988 (crtc_ww_class_acquire){+.+.}-{0:0}, at:
> > drm_mode_atomic_ioctl+0x13b/0xe90 <4>  #1: ffffc90003c6f9b0
> > (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_mode_atomic_ioctl+0x13b/0xe90 <4>
> > #2: ffff888135b838b8 (&intel_dp->psr.lock){+.+.}-{3:3}, at:
> > intel_psr_lock+0xc5/0xf0 [xe] <4>  #3: ffff88812607bbc0 (&wl->lock){+.+.}-{2:2},
> > at: intel_dmc_wl_get+0x3c/0x140 [xe]
> > <4> CPU: 6 UID: 0 PID: 1505 Comm: kms_cursor_lega Tainted: G     U
> > 6.18.0-rc3-lgci-xe-xe-pw-156729v1+ #1 PREEMPT_{RT,(lazy)}
> > <4> Tainted: [U]=USER
> > <4> Hardware name: Intel Corporation Panther Lake Client Platform/PTL-UH LP5
> > T3 RVP1, BIOS PTLPFWI1.R00.3383.D02.2509240621 09/24/2025 <4> Call Trace:
> > <4>  <TASK>
> > <4>  dump_stack_lvl+0xc1/0xf0
> > <4>  dump_stack+0x10/0x20
> > <4>  __might_resched+0x174/0x260
> > <4>  rt_spin_lock+0x63/0x200
> > <4>  ? intel_dmc_wl_get+0x3c/0x140 [xe]
> > <4>  intel_dmc_wl_get+0x3c/0x140 [xe]
> > <4>  intel_set_pipe_src_size+0x89/0xe0 [xe] <4>  intel_update_crtc+0x3c1/0x950
> > [xe] <4>  ? intel_pre_update_crtc+0x258/0x400 [xe] <4>
> > skl_commit_modeset_enables+0x217/0x720 [xe] <4>
> > intel_atomic_commit_tail+0xd4e/0x1af0 [xe] <4>  ? lock_release+0xce/0x2a0 <4>
> > intel_atomic_commit+0x2e5/0x330 [xe] <4>  ? intel_atomic_commit+0x2e5/0x330
> > [xe] <4>  drm_atomic_commit+0xaf/0xf0 <4>  ?
> > __pfx___drm_printfn_info+0x10/0x10
> > <4>  drm_mode_atomic_ioctl+0xbd5/0xe90
> > <4>  ? lock_acquire+0xc4/0x2e0
> > <4>  ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
> > <4>  drm_ioctl_kernel+0xb6/0x120
> > <4>  drm_ioctl+0x2d7/0x5a0
> > <4>  ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
> > <4>  ? rt_spin_unlock+0xa0/0x140
> > <4>  ? __pm_runtime_resume+0x53/0x90
> > <4>  xe_drm_ioctl+0x56/0x90 [xe]
> > <4>  __x64_sys_ioctl+0xa8/0x110
> > <4>  ? lock_acquire+0xc4/0x2e0
> > <4>  x64_sys_call+0x1144/0x26a0
> > <4>  do_syscall_64+0x93/0xae0
> > <4>  ? lock_release+0xce/0x2a0
> > <4>  ? __task_pid_nr_ns+0xd9/0x270
> > <4>  ? do_syscall_64+0x1b7/0xae0
> > <4>  ? find_held_lock+0x31/0x90
> > <4>  ? __task_pid_nr_ns+0xcf/0x270
> > <4>  ? __lock_acquire+0x43e/0x2860
> > <4>  ? __task_pid_nr_ns+0xd9/0x270
> > <4>  ? lock_acquire+0xc4/0x2e0
> > <4>  ? find_held_lock+0x31/0x90
> > <4>  ? __task_pid_nr_ns+0xcf/0x270
> > <4>  ? lock_release+0xce/0x2a0
> > <4>  ? __task_pid_nr_ns+0xd9/0x270
> > <4>  ? do_syscall_64+0x1b7/0xae0
> > <4>  ? do_syscall_64+0x1b7/0xae0
> > <4>  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > 
> > Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++++++----------
> >  drivers/gpu/drm/i915/display/intel_vrr.c     | 16 ++++-----
> >  2 files changed, 26 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 42ec787986666..1bff1148fe9d7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1573,9 +1573,9 @@ static void hsw_set_linetime_wm(const struct
> > intel_crtc_state *crtc_state)
> >  	struct intel_display *display = to_intel_display(crtc_state);
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > 
> > -	intel_de_write(display, WM_LINETIME(crtc->pipe),
> > -		       HSW_LINETIME(crtc_state->linetime) |
> > -		       HSW_IPS_LINETIME(crtc_state->ips_linetime));
> > +	intel_de_write_fw(display, WM_LINETIME(crtc->pipe),
> > +			  HSW_LINETIME(crtc_state->linetime) |
> > +			  HSW_IPS_LINETIME(crtc_state->ips_linetime));
> >  }
> > 
> >  static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> > @@ -2543,14 +2543,14 @@ void intel_set_m_n(struct intel_display *display,
> >  		   i915_reg_t data_m_reg, i915_reg_t data_n_reg,
> >  		   i915_reg_t link_m_reg, i915_reg_t link_n_reg)  {
> > -	intel_de_write(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m);
> > -	intel_de_write(display, data_n_reg, m_n->data_n);
> > -	intel_de_write(display, link_m_reg, m_n->link_m);
> > +	intel_de_write_fw(display, data_m_reg, TU_SIZE(m_n->tu) | m_n-
> > >data_m);
> > +	intel_de_write_fw(display, data_n_reg, m_n->data_n);
> > +	intel_de_write_fw(display, link_m_reg, m_n->link_m);
> >  	/*
> >  	 * On BDW+ writing LINK_N arms the double buffered update
> >  	 * of all the M/N registers, so it must be written last.
> >  	 */
> > -	intel_de_write(display, link_n_reg, m_n->link_n);
> > +	intel_de_write_fw(display, link_n_reg, m_n->link_n);
> >  }
> > 
> >  bool intel_cpu_transcoder_has_m2_n2(struct intel_display *display, @@ -2737,9
> > +2737,9 @@ static void intel_set_transcoder_timings_lrr(const struct
> > intel_crtc_state *crtc
> >  	}
> > 
> >  	if (DISPLAY_VER(display) >= 13) {
> > -		intel_de_write(display,
> > -			       TRANS_SET_CONTEXT_LATENCY(display,
> > cpu_transcoder),
> > -			       crtc_state->set_context_latency);
> > +		intel_de_write_fw(display,
> > +				  TRANS_SET_CONTEXT_LATENCY(display,
> > cpu_transcoder),
> > +				  crtc_state->set_context_latency);
> > 
> >  		/*
> >  		 * VBLANK_START not used by hw, just clear it @@ -2755,9
> > +2755,9 @@ static void intel_set_transcoder_timings_lrr(const struct
> > intel_crtc_state *crtc
> >  	 * The hardware actually ignores TRANS_VBLANK.VBLANK_END in DP
> > mode.
> >  	 * But let's write it anyway to keep the state checker happy.
> >  	 */
> > -	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
> > -		       VBLANK_START(crtc_vblank_start - 1) |
> > -		       VBLANK_END(crtc_vblank_end - 1));
> > +	intel_de_write_fw(display, TRANS_VBLANK(display, cpu_transcoder),
> > +			  VBLANK_START(crtc_vblank_start - 1) |
> > +			  VBLANK_END(crtc_vblank_end - 1));
> >  	/*
> >  	 * For platforms that always use VRR Timing Generator, the
> > VTOTAL.Vtotal
> >  	 * bits are not required. Since the support for these bits is going to @@ -
> > 2771,9 +2771,9 @@ static void intel_set_transcoder_timings_lrr(const struct
> > intel_crtc_state *crtc
> >  	 * The double buffer latch point for TRANS_VTOTAL
> >  	 * is the transcoder's undelayed vblank.
> >  	 */
> > -	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
> > -		       VACTIVE(crtc_vdisplay - 1) |
> > -		       VTOTAL(crtc_vtotal - 1));
> > +	intel_de_write_fw(display, TRANS_VTOTAL(display, cpu_transcoder),
> > +			  VACTIVE(crtc_vdisplay - 1) |
> > +			  VTOTAL(crtc_vtotal - 1));
> > 
> >  	intel_vrr_set_fixed_rr_timings(crtc_state);
> >  	intel_vrr_transcoder_enable(crtc_state);
> > @@ -2790,8 +2790,8 @@ static void intel_set_pipe_src_size(const struct
> > intel_crtc_state *crtc_state)
> >  	/* pipesrc controls the size that is scaled from, which should
> >  	 * always be the user's requested size.
> >  	 */
> > -	intel_de_write(display, PIPESRC(display, pipe),
> > -		       PIPESRC_WIDTH(width - 1) | PIPESRC_HEIGHT(height -
> > 1));
> > +	intel_de_write_fw(display, PIPESRC(display, pipe),
> > +			  PIPESRC_WIDTH(width - 1) | PIPESRC_HEIGHT(height
> > - 1));
> >  }
> > 
> >  static bool intel_pipe_is_interlaced(const struct intel_crtc_state *crtc_state) diff --
> > git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index 00cbc126fb366..2e19673697fa4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -300,12 +300,12 @@ void intel_vrr_set_fixed_rr_timings(const struct
> > intel_crtc_state *crtc_state)
> >  	if (!intel_vrr_possible(crtc_state))
> >  		return;
> > 
> > -	intel_de_write(display, TRANS_VRR_VMIN(display, cpu_transcoder),
> > -		       intel_vrr_fixed_rr_hw_vmin(crtc_state) - 1);
> > -	intel_de_write(display, TRANS_VRR_VMAX(display, cpu_transcoder),
> > -		       intel_vrr_fixed_rr_hw_vmax(crtc_state) - 1);
> > -	intel_de_write(display, TRANS_VRR_FLIPLINE(display, cpu_transcoder),
> > -		       intel_vrr_fixed_rr_hw_flipline(crtc_state) - 1);
> > +	intel_de_write_fw(display, TRANS_VRR_VMIN(display, cpu_transcoder),
> > +			  intel_vrr_fixed_rr_hw_vmin(crtc_state) - 1);
> > +	intel_de_write_fw(display, TRANS_VRR_VMAX(display, cpu_transcoder),
> > +			  intel_vrr_fixed_rr_hw_vmax(crtc_state) - 1);
> > +	intel_de_write_fw(display, TRANS_VRR_FLIPLINE(display,
> > cpu_transcoder),
> > +			  intel_vrr_fixed_rr_hw_flipline(crtc_state) - 1);
> >  }
> > 
> >  static
> > @@ -693,7 +693,7 @@ static void intel_vrr_tg_enable(const struct
> > intel_crtc_state *crtc_state,
> >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >  	u32 vrr_ctl;
> > 
> > -	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
> > TRANS_PUSH_EN);
> > +	intel_de_write_fw(display, TRANS_PUSH(display, cpu_transcoder),
> > +TRANS_PUSH_EN);
> > 
> >  	vrr_ctl = VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state);
> > 
> > @@ -705,7 +705,7 @@ static void intel_vrr_tg_enable(const struct
> > intel_crtc_state *crtc_state,
> >  	if (cmrr_enable)
> >  		vrr_ctl |= VRR_CTL_CMRR_ENABLE;
> > 
> > -	intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> > vrr_ctl);
> > +	intel_de_write_fw(display, TRANS_VRR_CTL(display, cpu_transcoder),
> > +vrr_ctl);
> >  }
> > 
> >  static void intel_vrr_tg_disable(const struct intel_crtc_state *old_crtc_state)
> > --
> > 2.51.0

-- 
Ville Syrjälä
Intel

RE: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in intel_pipe_fastset

[Shankar, Uma]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Thursday, November 27, 2025 1:13 AM
> To: Shankar, Uma <uma.shankar@intel.com>
> Cc: Maarten Lankhorst <dev@lankhorst.se>; intel-gfx@lists.freedesktop.org;
> intel-xe@lists.freedesktop.org; linux-rt-devel@lists.linux.dev; Mario Kleiner
> <mario.kleiner.de@gmail.com>; Mike Galbraith <umgwanakikbuti@gmail.com>;
> Thomas Gleixner <tglx@linutronix.de>; Sebastian Andrzej Siewior
> <bigeasy@linutronix.de>; Clark Williams <clrkwllms@kernel.org>; Steven
> Rostedt <rostedt@goodmis.org>
> Subject: Re: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in
> intel_pipe_fastset
> 
> On Wed, Nov 26, 2025 at 07:19:47PM +0000, Shankar, Uma wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf
> > > Of Maarten Lankhorst
> > > Sent: Tuesday, November 4, 2025 2:06 PM
> > > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > > Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst
> > > <dev@lankhorst.se>; Mario Kleiner <mario.kleiner.de@gmail.com>; Mike
> > > Galbraith <umgwanakikbuti@gmail.com>; Thomas Gleixner
> > > <tglx@linutronix.de>; Sebastian Andrzej Siewior
> > > <bigeasy@linutronix.de>; Clark Williams <clrkwllms@kernel.org>;
> > > Steven Rostedt <rostedt@goodmis.org>
> > > Subject: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in
> > > intel_pipe_fastset
> > >
> > > intel_set_pipe_src_size(), hsw_set_linetime_wm(),
> > > intel_cpu_transcoder_set_m1_n1() and
> > > intel_set_transcoder_timings_lrr()
> > > are called from an atomic context on PREEMPT_RT, and should be using
> > > the _fw functions.
> >
> > This could be ok but we need to be sure that all are called with power domains
> up.
> > I think would be safe to keep this under RT check so that we don't end
> > up breaking any generic non RT usecase.
> 
> When removing the locks from register accesses one needs to consider what
> platforms the code runs on, what other register are on the same cacheline, and
> whether they can be accessed in parallel. If there is something there then we may
> not be able to remove the locks.
> 
> That's assuming the "system hangs when same cacheline is accessed from
> multiple cpus" issue is real for display registers, and I'm actually not 100% it is.
> But we'd need to run some tests on the affected systems
> (~ivb/hsw) to get any kind of confidence here. IIRC some old intel_gpu_top thhat
> directly poked the registers was very good at hitting it on hsw at least, so that
> would be a decent starting point.
> 
> Anyways, I'm going to be replacing the uncore lock with a display specific lock
> soonish, and I suppose I can just make that a raw spinlock to appease RT.

Thanks Ville, yeah I am also not confident to switch to the fw version. Even if we have
to try this should be made limited to RT cases, where we can contain and stabilize as
we test and find out any issues.

> >
> > @Ville Syrjälä Any thoughts on this ?
> >
> > Regards,
> > Uma Shankar
> >
> > > This likely prevents a deadlock on i915.
> > >
> > > Again noticed when trying to disable preemption in vblank evasion:
> > > <3> BUG: sleeping function called from invalid context at
> > > kernel/locking/spinlock_rt.c:48 <3> in_atomic(): 1, irqs_disabled(): 0,
> non_block:
> > > 0, pid: 1505, name: kms_cursor_lega <3> preempt_count: 1, expected:
> > > 0 <3> RCU nest depth: 0, expected: 0 <4> 4 locks held by
> kms_cursor_lega/1505:
> > > <4>  #0: ffffc90003c6f988 (crtc_ww_class_acquire){+.+.}-{0:0}, at:
> > > drm_mode_atomic_ioctl+0x13b/0xe90 <4>  #1: ffffc90003c6f9b0
> > > (crtc_ww_class_mutex){+.+.}-{3:3}, at:
> > > drm_mode_atomic_ioctl+0x13b/0xe90 <4>
> > > #2: ffff888135b838b8 (&intel_dp->psr.lock){+.+.}-{3:3}, at:
> > > intel_psr_lock+0xc5/0xf0 [xe] <4>  #3: ffff88812607bbc0
> > > (&wl->lock){+.+.}-{2:2},
> > > at: intel_dmc_wl_get+0x3c/0x140 [xe]
> > > <4> CPU: 6 UID: 0 PID: 1505 Comm: kms_cursor_lega Tainted: G     U
> > > 6.18.0-rc3-lgci-xe-xe-pw-156729v1+ #1 PREEMPT_{RT,(lazy)} <4>
> > > Tainted: [U]=USER <4> Hardware name: Intel Corporation Panther Lake
> > > Client Platform/PTL-UH LP5
> > > T3 RVP1, BIOS PTLPFWI1.R00.3383.D02.2509240621 09/24/2025 <4> Call
> Trace:
> > > <4>  <TASK>
> > > <4>  dump_stack_lvl+0xc1/0xf0
> > > <4>  dump_stack+0x10/0x20
> > > <4>  __might_resched+0x174/0x260
> > > <4>  rt_spin_lock+0x63/0x200
> > > <4>  ? intel_dmc_wl_get+0x3c/0x140 [xe] <4>
> > > intel_dmc_wl_get+0x3c/0x140 [xe] <4>
> > > intel_set_pipe_src_size+0x89/0xe0 [xe] <4>
> > > intel_update_crtc+0x3c1/0x950 [xe] <4>  ?
> > > intel_pre_update_crtc+0x258/0x400 [xe] <4>
> > > skl_commit_modeset_enables+0x217/0x720 [xe] <4>
> > > intel_atomic_commit_tail+0xd4e/0x1af0 [xe] <4>  ?
> > > lock_release+0xce/0x2a0 <4>
> > > intel_atomic_commit+0x2e5/0x330 [xe] <4>  ?
> > > intel_atomic_commit+0x2e5/0x330 [xe] <4>  drm_atomic_commit+0xaf/0xf0
> <4>  ?
> > > __pfx___drm_printfn_info+0x10/0x10
> > > <4>  drm_mode_atomic_ioctl+0xbd5/0xe90 <4>  ?
> > > lock_acquire+0xc4/0x2e0 <4>  ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
> > > <4>  drm_ioctl_kernel+0xb6/0x120
> > > <4>  drm_ioctl+0x2d7/0x5a0
> > > <4>  ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
> > > <4>  ? rt_spin_unlock+0xa0/0x140
> > > <4>  ? __pm_runtime_resume+0x53/0x90 <4>  xe_drm_ioctl+0x56/0x90
> > > [xe] <4>  __x64_sys_ioctl+0xa8/0x110 <4>  ? lock_acquire+0xc4/0x2e0
> > > <4>  x64_sys_call+0x1144/0x26a0 <4>  do_syscall_64+0x93/0xae0 <4>  ?
> > > lock_release+0xce/0x2a0 <4>  ? __task_pid_nr_ns+0xd9/0x270 <4>  ?
> > > do_syscall_64+0x1b7/0xae0 <4>  ? find_held_lock+0x31/0x90 <4>  ?
> > > __task_pid_nr_ns+0xcf/0x270 <4>  ? __lock_acquire+0x43e/0x2860 <4>
> > > ? __task_pid_nr_ns+0xd9/0x270 <4>  ? lock_acquire+0xc4/0x2e0 <4>  ?
> > > find_held_lock+0x31/0x90 <4>  ? __task_pid_nr_ns+0xcf/0x270 <4>  ?
> > > lock_release+0xce/0x2a0 <4>  ? __task_pid_nr_ns+0xd9/0x270 <4>  ?
> > > do_syscall_64+0x1b7/0xae0 <4>  ? do_syscall_64+0x1b7/0xae0 <4>
> > > entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > >
> > > Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++++++----------
> > >  drivers/gpu/drm/i915/display/intel_vrr.c     | 16 ++++-----
> > >  2 files changed, 26 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 42ec787986666..1bff1148fe9d7 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -1573,9 +1573,9 @@ static void hsw_set_linetime_wm(const struct
> > > intel_crtc_state *crtc_state)
> > >  	struct intel_display *display = to_intel_display(crtc_state);
> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >
> > > -	intel_de_write(display, WM_LINETIME(crtc->pipe),
> > > -		       HSW_LINETIME(crtc_state->linetime) |
> > > -		       HSW_IPS_LINETIME(crtc_state->ips_linetime));
> > > +	intel_de_write_fw(display, WM_LINETIME(crtc->pipe),
> > > +			  HSW_LINETIME(crtc_state->linetime) |
> > > +			  HSW_IPS_LINETIME(crtc_state->ips_linetime));
> > >  }
> > >
> > >  static void hsw_set_frame_start_delay(const struct intel_crtc_state
> > > *crtc_state) @@ -2543,14 +2543,14 @@ void intel_set_m_n(struct
> intel_display *display,
> > >  		   i915_reg_t data_m_reg, i915_reg_t data_n_reg,
> > >  		   i915_reg_t link_m_reg, i915_reg_t link_n_reg)  {
> > > -	intel_de_write(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m);
> > > -	intel_de_write(display, data_n_reg, m_n->data_n);
> > > -	intel_de_write(display, link_m_reg, m_n->link_m);
> > > +	intel_de_write_fw(display, data_m_reg, TU_SIZE(m_n->tu) | m_n-
> > > >data_m);
> > > +	intel_de_write_fw(display, data_n_reg, m_n->data_n);
> > > +	intel_de_write_fw(display, link_m_reg, m_n->link_m);
> > >  	/*
> > >  	 * On BDW+ writing LINK_N arms the double buffered update
> > >  	 * of all the M/N registers, so it must be written last.
> > >  	 */
> > > -	intel_de_write(display, link_n_reg, m_n->link_n);
> > > +	intel_de_write_fw(display, link_n_reg, m_n->link_n);
> > >  }
> > >
> > >  bool intel_cpu_transcoder_has_m2_n2(struct intel_display *display,
> > > @@ -2737,9
> > > +2737,9 @@ static void intel_set_transcoder_timings_lrr(const struct
> > > intel_crtc_state *crtc
> > >  	}
> > >
> > >  	if (DISPLAY_VER(display) >= 13) {
> > > -		intel_de_write(display,
> > > -			       TRANS_SET_CONTEXT_LATENCY(display,
> > > cpu_transcoder),
> > > -			       crtc_state->set_context_latency);
> > > +		intel_de_write_fw(display,
> > > +				  TRANS_SET_CONTEXT_LATENCY(display,
> > > cpu_transcoder),
> > > +				  crtc_state->set_context_latency);
> > >
> > >  		/*
> > >  		 * VBLANK_START not used by hw, just clear it @@ -2755,9
> > > +2755,9 @@ static void intel_set_transcoder_timings_lrr(const struct
> > > intel_crtc_state *crtc
> > >  	 * The hardware actually ignores TRANS_VBLANK.VBLANK_END in DP
> > > mode.
> > >  	 * But let's write it anyway to keep the state checker happy.
> > >  	 */
> > > -	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
> > > -		       VBLANK_START(crtc_vblank_start - 1) |
> > > -		       VBLANK_END(crtc_vblank_end - 1));
> > > +	intel_de_write_fw(display, TRANS_VBLANK(display, cpu_transcoder),
> > > +			  VBLANK_START(crtc_vblank_start - 1) |
> > > +			  VBLANK_END(crtc_vblank_end - 1));
> > >  	/*
> > >  	 * For platforms that always use VRR Timing Generator, the
> > > VTOTAL.Vtotal
> > >  	 * bits are not required. Since the support for these bits is
> > > going to @@ -
> > > 2771,9 +2771,9 @@ static void intel_set_transcoder_timings_lrr(const
> > > struct intel_crtc_state *crtc
> > >  	 * The double buffer latch point for TRANS_VTOTAL
> > >  	 * is the transcoder's undelayed vblank.
> > >  	 */
> > > -	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
> > > -		       VACTIVE(crtc_vdisplay - 1) |
> > > -		       VTOTAL(crtc_vtotal - 1));
> > > +	intel_de_write_fw(display, TRANS_VTOTAL(display, cpu_transcoder),
> > > +			  VACTIVE(crtc_vdisplay - 1) |
> > > +			  VTOTAL(crtc_vtotal - 1));
> > >
> > >  	intel_vrr_set_fixed_rr_timings(crtc_state);
> > >  	intel_vrr_transcoder_enable(crtc_state);
> > > @@ -2790,8 +2790,8 @@ static void intel_set_pipe_src_size(const
> > > struct intel_crtc_state *crtc_state)
> > >  	/* pipesrc controls the size that is scaled from, which should
> > >  	 * always be the user's requested size.
> > >  	 */
> > > -	intel_de_write(display, PIPESRC(display, pipe),
> > > -		       PIPESRC_WIDTH(width - 1) | PIPESRC_HEIGHT(height -
> > > 1));
> > > +	intel_de_write_fw(display, PIPESRC(display, pipe),
> > > +			  PIPESRC_WIDTH(width - 1) | PIPESRC_HEIGHT(height
> > > - 1));
> > >  }
> > >
> > >  static bool intel_pipe_is_interlaced(const struct intel_crtc_state
> > > *crtc_state) diff -- git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > index 00cbc126fb366..2e19673697fa4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > @@ -300,12 +300,12 @@ void intel_vrr_set_fixed_rr_timings(const
> > > struct intel_crtc_state *crtc_state)
> > >  	if (!intel_vrr_possible(crtc_state))
> > >  		return;
> > >
> > > -	intel_de_write(display, TRANS_VRR_VMIN(display, cpu_transcoder),
> > > -		       intel_vrr_fixed_rr_hw_vmin(crtc_state) - 1);
> > > -	intel_de_write(display, TRANS_VRR_VMAX(display, cpu_transcoder),
> > > -		       intel_vrr_fixed_rr_hw_vmax(crtc_state) - 1);
> > > -	intel_de_write(display, TRANS_VRR_FLIPLINE(display, cpu_transcoder),
> > > -		       intel_vrr_fixed_rr_hw_flipline(crtc_state) - 1);
> > > +	intel_de_write_fw(display, TRANS_VRR_VMIN(display, cpu_transcoder),
> > > +			  intel_vrr_fixed_rr_hw_vmin(crtc_state) - 1);
> > > +	intel_de_write_fw(display, TRANS_VRR_VMAX(display, cpu_transcoder),
> > > +			  intel_vrr_fixed_rr_hw_vmax(crtc_state) - 1);
> > > +	intel_de_write_fw(display, TRANS_VRR_FLIPLINE(display,
> > > cpu_transcoder),
> > > +			  intel_vrr_fixed_rr_hw_flipline(crtc_state) - 1);
> > >  }
> > >
> > >  static
> > > @@ -693,7 +693,7 @@ static void intel_vrr_tg_enable(const struct
> > > intel_crtc_state *crtc_state,
> > >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > >  	u32 vrr_ctl;
> > >
> > > -	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
> > > TRANS_PUSH_EN);
> > > +	intel_de_write_fw(display, TRANS_PUSH(display, cpu_transcoder),
> > > +TRANS_PUSH_EN);
> > >
> > >  	vrr_ctl = VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state);
> > >
> > > @@ -705,7 +705,7 @@ static void intel_vrr_tg_enable(const struct
> > > intel_crtc_state *crtc_state,
> > >  	if (cmrr_enable)
> > >  		vrr_ctl |= VRR_CTL_CMRR_ENABLE;
> > >
> > > -	intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> > > vrr_ctl);
> > > +	intel_de_write_fw(display, TRANS_VRR_CTL(display, cpu_transcoder),
> > > +vrr_ctl);
> > >  }
> > >
> > >  static void intel_vrr_tg_disable(const struct intel_crtc_state
> > > *old_crtc_state)
> > > --
> > > 2.51.0
> 
> --
> Ville Syrjälä
> Intel

RE: [PATCH v2 4/7] drm/i915/display: Remove locking from intel_vblank_evade critical section

[Shankar, Uma]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Maarten
> Lankhorst
> Sent: Tuesday, November 4, 2025 2:06 PM
> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst <dev@lankhorst.se>; Mario
> Kleiner <mario.kleiner.de@gmail.com>; Mike Galbraith
> <umgwanakikbuti@gmail.com>; Thomas Gleixner <tglx@linutronix.de>; Sebastian
> Andrzej Siewior <bigeasy@linutronix.de>; Clark Williams
> <clrkwllms@kernel.org>; Steven Rostedt <rostedt@goodmis.org>
> Subject: [PATCH v2 4/7] drm/i915/display: Remove locking from
> intel_vblank_evade critical section
> 
> finish_wait() may take a lock, which means that it can take any amount of time.
> On PREEMPT-RT we should not be taking any lock after disabling preemption, so
> ensure that the completion is done before disabling interrupts.
> 
> This also has the benefit of making vblank evasion more deterministic, by
> performing the final vblank check after all locking is done.
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/i915/display/intel_vblank.c | 35 ++++++++++-----------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c
> b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 2b106ffa3f5f5..3628d2a1b8f38 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -708,6 +708,13 @@ void intel_vblank_evade_init(const struct
> intel_crtc_state *old_crtc_state,
>  		evade->min -= vblank_delay;
>  }
> 
> +static inline int vblank_evadable(struct intel_vblank_evade_ctx *evade,
> +int *scanline) {
> +	*scanline = intel_get_crtc_scanline(evade->crtc);
> +
> +	return *scanline < evade->min || *scanline > evade->max; }
> +
>  /* must be called with vblank interrupt already enabled! */  int
> intel_vblank_evade(struct intel_vblank_evade_ctx *evade)  { @@ -715,23 +722,22
> @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>  	struct intel_display *display = to_intel_display(crtc);
>  	long timeout = msecs_to_jiffies_timeout(1);
>  	wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
> -	DEFINE_WAIT(wait);
>  	int scanline;
> 
>  	if (evade->min <= 0 || evade->max <= 0)
>  		return 0;
> 
> -	for (;;) {
> -		/*
> -		 * prepare_to_wait() has a memory barrier, which guarantees
> -		 * other CPUs can see the task state update by the time we
> -		 * read the scanline.
> -		 */
> -		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> +	while (!vblank_evadable(evade, &scanline)) {
> +		local_irq_enable();
> 
> -		scanline = intel_get_crtc_scanline(crtc);
> -		if (scanline < evade->min || scanline > evade->max)
> -			break;
> +		DEFINE_WAIT(wait);
> +		while (!vblank_evadable(evade, &scanline) && timeout > 0) {

Not sure if doing the scanline check with interrupts on is ok. The scanlines can move
if we get interrupted or what happens if we get a vblank interrupt. Looks vulnerable to race.

I will try to check this further and get back.

Regards,
Uma Shankar

> +			prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> +			timeout = schedule_timeout(timeout);
> +		}
> +		finish_wait(wq, &wait);
> +
> +		local_irq_disable();
> 
>  		if (!timeout) {
>  			drm_dbg_kms(display->drm,
> @@ -740,15 +746,8 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx
> *evade)
>  			break;
>  		}
> 
> -		local_irq_enable();
> -
> -		timeout = schedule_timeout(timeout);
> -
> -		local_irq_disable();
>  	}
> 
> -	finish_wait(wq, &wait);
> -
>  	/*
>  	 * On VLV/CHV DSI the scanline counter would appear to
>  	 * increment approx. 1/3 of a scanline before start of vblank.
> --
> 2.51.0

RE: [PATCH v2 6/7] drm/i915/display: Enable interrupts earlier on PREEMPT_RT

[Shankar, Uma]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Maarten
> Lankhorst
> Sent: Tuesday, November 4, 2025 2:07 PM
> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst <dev@lankhorst.se>; Mario
> Kleiner <mario.kleiner.de@gmail.com>; Mike Galbraith
> <umgwanakikbuti@gmail.com>; Thomas Gleixner <tglx@linutronix.de>; Sebastian
> Andrzej Siewior <bigeasy@linutronix.de>; Clark Williams
> <clrkwllms@kernel.org>; Steven Rostedt <rostedt@goodmis.org>
> Subject: [PATCH v2 6/7] drm/i915/display: Enable interrupts earlier on
> PREEMPT_RT
> 
> The last part of the vblank evasion is about updating bookkeeping, not
> programming hardware registers.
> 
> The interrupts cannot stay disabled here on PREEMPT_RT since the spinlocks get
> converted to mutexes.
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/i915/display/intel_crtc.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 9d2a23c96c61b..b87f6b4a4f3d7 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -688,6 +688,14 @@ void intel_pipe_update_end(struct intel_atomic_state
> *state,
>  	    intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI))
>  		icl_dsi_frame_update(new_crtc_state);
> 
> +#if IS_ENABLED(CONFIG_PREEMPT_RT)
> +	/*
> +	 * Timing sensitive register writing completed, non-deterministic
> +	 * locking from here on out.
> +	 */
> +	local_irq_enable();
> +#endif

I think we do have VRR send push etc handled here, also arming registers are being updated.
Not sure we can allow interrupts here. Please check once


> +
>  	/* We're still in the vblank-evade critical section, this can't race.
>  	 * Would be slightly nice to just grab the vblank count and arm the
>  	 * event outside of the critical section - the spinlock might spin for a @@ -
> 735,7 +743,9 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
>  	if (!state->base.legacy_cursor_update)
>  		intel_vrr_send_push(NULL, new_crtc_state);
> 
> +#if !IS_ENABLED(CONFIG_PREEMPT_RT)
>  	local_irq_enable();
> +#endif
> 
>  	if (intel_vgpu_active(dev_priv))
>  		goto out;
> --
> 2.51.0

Re: [PATCH v2 4/7] drm/i915/display: Remove locking from intel_vblank_evade critical section

[Ville Syrjälä]

On Tue, Nov 04, 2025 at 09:36:28AM +0100, Maarten Lankhorst wrote:
> finish_wait() may take a lock, which means that it can take any amount
> of time. On PREEMPT-RT we should not be taking any lock after disabling
> preemption, so ensure that the completion is done before disabling
> interrupts.
> 
> This also has the benefit of making vblank evasion more deterministic,
> by performing the final vblank check after all locking is done.
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/i915/display/intel_vblank.c | 35 ++++++++++-----------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 2b106ffa3f5f5..3628d2a1b8f38 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -708,6 +708,13 @@ void intel_vblank_evade_init(const struct intel_crtc_state *old_crtc_state,
>  		evade->min -= vblank_delay;
>  }
>  
> +static inline int vblank_evadable(struct intel_vblank_evade_ctx *evade, int *scanline)

The name is confusing. But having a function
would be nice since we need two checks I guess.
scanline_is_safe() or something?

Also type should be bool, and inline looks pointless.

> +{
> +	*scanline = intel_get_crtc_scanline(evade->crtc);
> +
> +	return *scanline < evade->min || *scanline > evade->max;
> +}
> +
>  /* must be called with vblank interrupt already enabled! */
>  int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>  {
> @@ -715,23 +722,22 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>  	struct intel_display *display = to_intel_display(crtc);
>  	long timeout = msecs_to_jiffies_timeout(1);
>  	wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
> -	DEFINE_WAIT(wait);
>  	int scanline;
>  
>  	if (evade->min <= 0 || evade->max <= 0)
>  		return 0;
>  
> -	for (;;) {
> -		/*
> -		 * prepare_to_wait() has a memory barrier, which guarantees
> -		 * other CPUs can see the task state update by the time we
> -		 * read the scanline.
> -		 */
> -		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> +	while (!vblank_evadable(evade, &scanline)) {
> +		local_irq_enable();
>  
> -		scanline = intel_get_crtc_scanline(crtc);
> -		if (scanline < evade->min || scanline > evade->max)
> -			break;
> +		DEFINE_WAIT(wait);
> +		while (!vblank_evadable(evade, &scanline) && timeout > 0) {
> +			prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> +			timeout = schedule_timeout(timeout);
> +		}
> +		finish_wait(wq, &wait);
> +
> +		local_irq_disable();
>  
>  		if (!timeout) {

This looks to introduce the classic "didn't check the
condition after timeout" race.

I guess what you're going for is something like this (done
through a somewhat less intrusive reordering of the current
code):

for (;;) {
	if (scanline_is_safe(evade, &scanline))
		break;

	if (!timeout) {
		drm_dbg_kms(display->drm,
			    "Potential atomic update failure on pipe %c\n",
			    pipe_name(crtc->pipe));
		break;
	}

	local_irq_enable();

	prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);

	if (!scanline_is_safe(evade, &scanline))
		timeout = schedule_timeout(timeout);

	finish_wait(wq, &wait);

	local_irq_disable();
}

And then maybe the whole prepare+wait+finish thing there
could be a simple wait_event_timeout(). That would make
that inner thing a loop though. We might not want that
just because jiffies is so coarse and we don't really
want to wait multiple times there.

>  			drm_dbg_kms(display->drm,
> @@ -740,15 +746,8 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>  			break;
>  		}
>  
> -		local_irq_enable();
> -
> -		timeout = schedule_timeout(timeout);
> -
> -		local_irq_disable();
>  	}
>  
> -	finish_wait(wq, &wait);
> -
>  	/*
>  	 * On VLV/CHV DSI the scanline counter would appear to
>  	 * increment approx. 1/3 of a scanline before start of vblank.
> -- 
> 2.51.0

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 6/7] drm/i915/display: Enable interrupts earlier on PREEMPT_RT

[Ville Syrjälä]

On Wed, Nov 26, 2025 at 08:45:31PM +0000, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Maarten
> > Lankhorst
> > Sent: Tuesday, November 4, 2025 2:07 PM
> > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst <dev@lankhorst.se>; Mario
> > Kleiner <mario.kleiner.de@gmail.com>; Mike Galbraith
> > <umgwanakikbuti@gmail.com>; Thomas Gleixner <tglx@linutronix.de>; Sebastian
> > Andrzej Siewior <bigeasy@linutronix.de>; Clark Williams
> > <clrkwllms@kernel.org>; Steven Rostedt <rostedt@goodmis.org>
> > Subject: [PATCH v2 6/7] drm/i915/display: Enable interrupts earlier on
> > PREEMPT_RT
> > 
> > The last part of the vblank evasion is about updating bookkeeping, not
> > programming hardware registers.
> > 
> > The interrupts cannot stay disabled here on PREEMPT_RT since the spinlocks get
> > converted to mutexes.
> > 
> > Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> > ---
> >  drivers/gpu/drm/i915/display/intel_crtc.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > index 9d2a23c96c61b..b87f6b4a4f3d7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > @@ -688,6 +688,14 @@ void intel_pipe_update_end(struct intel_atomic_state
> > *state,
> >  	    intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI))
> >  		icl_dsi_frame_update(new_crtc_state);
> > 
> > +#if IS_ENABLED(CONFIG_PREEMPT_RT)
> > +	/*
> > +	 * Timing sensitive register writing completed, non-deterministic
> > +	 * locking from here on out.
> > +	 */
> > +	local_irq_enable();
> > +#endif
> 
> I think we do have VRR send push etc handled here, also arming registers are being updated.
> Not sure we can allow interrupts here. Please check once

Yeah, this doesn't seem exactly great.

Even without VRR we want the register writes and vblank event arming
to happen in the same frame. Though without VRR I suppose the worst
that could happen is that we complete the commit one frame too late.

With VRR however we need the vblank event arming and push to happen
in the same frame. Otherwise we'll complete the flip early and leave
push send assrted, which causes the next frame to terminate at vmin.
Basically that makes the next frame a mailbox flip as far as push
send is concerned.

The race is already there, but allowing the CPU to get scheduled away
will widen it. We do try to handle it in the vblank evasion, but I
think we're handling it way too early (in intel_vblank_evade_init())
so that part itself is racy. I suppose we should rather do the vmin
vs. vmax evasion decision after we've actually read out the current
scanline. That should at least make it a bit more robust.

One other thing we could maybe think about is arming the vblank
event after the push is sent (with seq = current+1), and then
immediately check if the push bit already cleared, and if so
cancel the arming and send the event directly (with seq = current).
But that's just a quick idead that popped to my head, didn't really
think it through in detail.

I'm tempted to say we should just make the vblank locks raw spinlocks
as well. But I've not looked at what other drivers do in the vblank
hooks so dunno how feasible that is.

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 4/7] drm/i915/display: Remove locking from intel_vblank_evade critical section

[Maarten Lankhorst]

Hey Uma,

Den 2025-11-26 kl. 21:03, skrev Shankar, Uma:
> 
> 
>> -----Original Message-----
>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Maarten
>> Lankhorst
>> Sent: Tuesday, November 4, 2025 2:06 PM
>> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>> Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst <dev@lankhorst.se>; Mario
>> Kleiner <mario.kleiner.de@gmail.com>; Mike Galbraith
>> <umgwanakikbuti@gmail.com>; Thomas Gleixner <tglx@linutronix.de>; Sebastian
>> Andrzej Siewior <bigeasy@linutronix.de>; Clark Williams
>> <clrkwllms@kernel.org>; Steven Rostedt <rostedt@goodmis.org>
>> Subject: [PATCH v2 4/7] drm/i915/display: Remove locking from
>> intel_vblank_evade critical section
>>
>> finish_wait() may take a lock, which means that it can take any amount of time.
>> On PREEMPT-RT we should not be taking any lock after disabling preemption, so
>> ensure that the completion is done before disabling interrupts.
>>
>> This also has the benefit of making vblank evasion more deterministic, by
>> performing the final vblank check after all locking is done.
>>
>> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
>> ---
>>  drivers/gpu/drm/i915/display/intel_vblank.c | 35 ++++++++++-----------
>>  1 file changed, 17 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c
>> b/drivers/gpu/drm/i915/display/intel_vblank.c
>> index 2b106ffa3f5f5..3628d2a1b8f38 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
>> @@ -708,6 +708,13 @@ void intel_vblank_evade_init(const struct
>> intel_crtc_state *old_crtc_state,
>>  		evade->min -= vblank_delay;
>>  }
>>
>> +static inline int vblank_evadable(struct intel_vblank_evade_ctx *evade,
>> +int *scanline) {
>> +	*scanline = intel_get_crtc_scanline(evade->crtc);
>> +
>> +	return *scanline < evade->min || *scanline > evade->max; }
>> +
>>  /* must be called with vblank interrupt already enabled! */  int
>> intel_vblank_evade(struct intel_vblank_evade_ctx *evade)  { @@ -715,23 +722,22
>> @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>>  	struct intel_display *display = to_intel_display(crtc);
>>  	long timeout = msecs_to_jiffies_timeout(1);
>>  	wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
>> -	DEFINE_WAIT(wait);
>>  	int scanline;
>>
>>  	if (evade->min <= 0 || evade->max <= 0)
>>  		return 0;
>>
>> -	for (;;) {
>> -		/*
>> -		 * prepare_to_wait() has a memory barrier, which guarantees
>> -		 * other CPUs can see the task state update by the time we
>> -		 * read the scanline.
>> -		 */
>> -		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>> +	while (!vblank_evadable(evade, &scanline)) {
>> +		local_irq_enable();
>>
>> -		scanline = intel_get_crtc_scanline(crtc);
>> -		if (scanline < evade->min || scanline > evade->max)
>> -			break;
>> +		DEFINE_WAIT(wait);
>> +		while (!vblank_evadable(evade, &scanline) && timeout > 0) {
> 
> Not sure if doing the scanline check with interrupts on is ok. The scanlines can move
> if we get interrupted or what happens if we get a vblank interrupt. Looks vulnerable to race.
> 
> I will try to check this further and get back.

There is a double check here to make it safe:

while (!vblank_evade()) {
	drop locks();

	while (!vblank_evadable())
		wait();

	re-acquire locks();
}

Even if the middle vblank evadable is unsafe, and it has to be to be able to wait, we re-check after acquiring locks.
We must do so there anyway since the locking acquire can add any amount of latency as well.

Kind regards,
Maarten Lankhorst

Re: [PATCH v2 6/7] drm/i915/display: Enable interrupts earlier on PREEMPT_RT

[Maarten Lankhorst]

Hey,

Den 2025-11-26 kl. 23:57, skrev Ville Syrjälä:
> On Wed, Nov 26, 2025 at 08:45:31PM +0000, Shankar, Uma wrote:
>>
>>
>>> -----Original Message-----
>>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Maarten
>>> Lankhorst
>>> Sent: Tuesday, November 4, 2025 2:07 PM
>>> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>>> Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst <dev@lankhorst.se>; Mario
>>> Kleiner <mario.kleiner.de@gmail.com>; Mike Galbraith
>>> <umgwanakikbuti@gmail.com>; Thomas Gleixner <tglx@linutronix.de>; Sebastian
>>> Andrzej Siewior <bigeasy@linutronix.de>; Clark Williams
>>> <clrkwllms@kernel.org>; Steven Rostedt <rostedt@goodmis.org>
>>> Subject: [PATCH v2 6/7] drm/i915/display: Enable interrupts earlier on
>>> PREEMPT_RT
>>>
>>> The last part of the vblank evasion is about updating bookkeeping, not
>>> programming hardware registers.
>>>
>>> The interrupts cannot stay disabled here on PREEMPT_RT since the spinlocks get
>>> converted to mutexes.
>>>
>>> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_crtc.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
>>> b/drivers/gpu/drm/i915/display/intel_crtc.c
>>> index 9d2a23c96c61b..b87f6b4a4f3d7 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
>>> @@ -688,6 +688,14 @@ void intel_pipe_update_end(struct intel_atomic_state
>>> *state,
>>>  	    intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI))
>>>  		icl_dsi_frame_update(new_crtc_state);
>>>
>>> +#if IS_ENABLED(CONFIG_PREEMPT_RT)
>>> +	/*
>>> +	 * Timing sensitive register writing completed, non-deterministic
>>> +	 * locking from here on out.
>>> +	 */
>>> +	local_irq_enable();
>>> +#endif
>>
>> I think we do have VRR send push etc handled here, also arming registers are being updated.
>> Not sure we can allow interrupts here. Please check once
> 
> Yeah, this doesn't seem exactly great.
> 
> Even without VRR we want the register writes and vblank event arming
> to happen in the same frame. Though without VRR I suppose the worst
> that could happen is that we complete the commit one frame too late.
> 
> With VRR however we need the vblank event arming and push to happen
> in the same frame. Otherwise we'll comple		te the flip early and leave
> push send assrted, which causes the next frame to terminate at vmin.
> Basically that makes the next frame a mailbox flip as far as push
> send is concerned.
> 
> The race is already there, but allowing the CPU to get scheduled away
> will widen it. We do try to handle it in the vblank evasion, but I
> think we're handling it way too early (in intel_vblank_evade_init())
> so that part itself is racy. I suppose we should rather do the vmin
> vs. vmax evasion decision after we've actually read out the current
> scanline. That should at least make it a bit more robust.
> 
> One other thing we could maybe think about is arming the vblank
> event after the push is sent (with seq = current+1), and then
> immediately check if the push bit already cleared, and if so
> cancel the arming and send the event directly (with seq = current).
> But that's just a quick idead that popped to my head, didn't really
> think it through in detail.
> 
> I'm tempted to say we should just make the vblank locks raw spinlocks
> as well. But I've not looked at what other drivers do in the vblank
> hooks so dunno how feasible that is.
> 
Ideally we make the time critical code even faster, and fastest is
pre-obtaining the vblank locks so no race is even possible in the timing sensitive part.

If we acquire a drm_vblank_get() and dev->event_lock before we begin
vblank evasion, and release them afterwards then no conversion of the
lock to a raw spinlock is required.

This eliminates even more jitter, and turns the support of PREEMPT_RT
in something positive. :-)

Kind regards,
~Maarten Lankhorst

Re: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in intel_pipe_fastset

[Maarten Lankhorst]

Hey,

Den 2025-11-26 kl. 20:56, skrev Shankar, Uma:
> 
> 
>> -----Original Message-----
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Sent: Thursday, November 27, 2025 1:13 AM
>> To: Shankar, Uma <uma.shankar@intel.com>
>> Cc: Maarten Lankhorst <dev@lankhorst.se>; intel-gfx@lists.freedesktop.org;
>> intel-xe@lists.freedesktop.org; linux-rt-devel@lists.linux.dev; Mario Kleiner
>> <mario.kleiner.de@gmail.com>; Mike Galbraith <umgwanakikbuti@gmail.com>;
>> Thomas Gleixner <tglx@linutronix.de>; Sebastian Andrzej Siewior
>> <bigeasy@linutronix.de>; Clark Williams <clrkwllms@kernel.org>; Steven
>> Rostedt <rostedt@goodmis.org>
>> Subject: Re: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in
>> intel_pipe_fastset
>>
>> On Wed, Nov 26, 2025 at 07:19:47PM +0000, Shankar, Uma wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf
>>>> Of Maarten Lankhorst
>>>> Sent: Tuesday, November 4, 2025 2:06 PM
>>>> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>>>> Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst
>>>> <dev@lankhorst.se>; Mario Kleiner <mario.kleiner.de@gmail.com>; Mike
>>>> Galbraith <umgwanakikbuti@gmail.com>; Thomas Gleixner
>>>> <tglx@linutronix.de>; Sebastian Andrzej Siewior
>>>> <bigeasy@linutronix.de>; Clark Williams <clrkwllms@kernel.org>;
>>>> Steven Rostedt <rostedt@goodmis.org>
>>>> Subject: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in
>>>> intel_pipe_fastset
>>>>
>>>> intel_set_pipe_src_size(), hsw_set_linetime_wm(),
>>>> intel_cpu_transcoder_set_m1_n1() and
>>>> intel_set_transcoder_timings_lrr()
>>>> are called from an atomic context on PREEMPT_RT, and should be using
>>>> the _fw functions.
>>>
>>> This could be ok but we need to be sure that all are called with power domains
>> up.
>>> I think would be safe to keep this under RT check so that we don't end
>>> up breaking any generic non RT usecase.
>>
>> When removing the locks from register accesses one needs to consider what
>> platforms the code runs on, what other register are on the same cacheline, and
>> whether they can be accessed in parallel. If there is something there then we may
>> not be able to remove the locks.
>>
>> That's assuming the "system hangs when same cacheline is accessed from
>> multiple cpus" issue is real for display registers, and I'm actually not 100% it is.
>> But we'd need to run some tests on the affected systems
>> (~ivb/hsw) to get any kind of confidence here. IIRC some old intel_gpu_top thhat
>> directly poked the registers was very good at hitting it on hsw at least, so that
>> would be a decent starting point.
>>
>> Anyways, I'm going to be replacing the uncore lock with a display specific lock
>> soonish, and I suppose I can just make that a raw spinlock to appease RT.
> 
> Thanks Ville, yeah I am also not confident to switch to the fw version. Even if we have
> to try this should be made limited to RT cases, where we can contain and stabilize as
> we test and find out any issues.
> Direct poking of registers requires root privileges, so that should not be something we have to worry about. It's not something required by any driver.

The specific calls are called during modeset, fastset and pageflip, and by design there is no chance of those racing with each other. They're serialized between each other.

Kind regards,
~Maarten Lankhorst

Re: [PATCH v2 4/7] drm/i915/display: Remove locking from intel_vblank_evade critical section

[Maarten Lankhorst]

Den 2025-11-26 kl. 22:17, skrev Ville Syrjälä:
> On Tue, Nov 04, 2025 at 09:36:28AM +0100, Maarten Lankhorst wrote:
>> finish_wait() may take a lock, which means that it can take any amount
>> of time. On PREEMPT-RT we should not be taking any lock after disabling
>> preemption, so ensure that the completion is done before disabling
>> interrupts.
>>
>> This also has the benefit of making vblank evasion more deterministic,
>> by performing the final vblank check after all locking is done.
>>
>> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
>> ---
>>  drivers/gpu/drm/i915/display/intel_vblank.c | 35 ++++++++++-----------
>>  1 file changed, 17 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
>> index 2b106ffa3f5f5..3628d2a1b8f38 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
>> @@ -708,6 +708,13 @@ void intel_vblank_evade_init(const struct intel_crtc_state *old_crtc_state,
>>  		evade->min -= vblank_delay;
>>  }
>>  
>> +static inline int vblank_evadable(struct intel_vblank_evade_ctx *evade, int *scanline)
> 
> The name is confusing. But having a function
> would be nice since we need two checks I guess.
> scanline_is_safe() or something?
> 
> Also type should be bool, and inline looks pointless.
> 
>> +{
>> +	*scanline = intel_get_crtc_scanline(evade->crtc);
>> +
>> +	return *scanline < evade->min || *scanline > evade->max;
>> +}
>> +
>>  /* must be called with vblank interrupt already enabled! */
>>  int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>>  {
>> @@ -715,23 +722,22 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>>  	struct intel_display *display = to_intel_display(crtc);
>>  	long timeout = msecs_to_jiffies_timeout(1);
>>  	wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
>> -	DEFINE_WAIT(wait);
>>  	int scanline;
>>  
>>  	if (evade->min <= 0 || evade->max <= 0)
>>  		return 0;
>>  
>> -	for (;;) {
>> -		/*
>> -		 * prepare_to_wait() has a memory barrier, which guarantees
>> -		 * other CPUs can see the task state update by the time we
>> -		 * read the scanline.
>> -		 */
>> -		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>> +	while (!vblank_evadable(evade, &scanline)) {
>> +		local_irq_enable();
>>  
>> -		scanline = intel_get_crtc_scanline(crtc);
>> -		if (scanline < evade->min || scanline > evade->max)
>> -			break;
>> +		DEFINE_WAIT(wait);
>> +		while (!vblank_evadable(evade, &scanline) && timeout > 0) {
>> +			prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>> +			timeout = schedule_timeout(timeout);
>> +		}
>> +		finish_wait(wq, &wait);
>> +
>> +		local_irq_disable();
>>  
>>  		if (!timeout) {
> 
> This looks to introduce the classic "didn't check the
> condition after timeout" race.
> 
> I guess what you're going for is something like this (done
> through a somewhat less intrusive reordering of the current
> code):
> 
> for (;;) {
> 	if (scanline_is_safe(evade, &scanline))
> 		break;
> 
> 	if (!timeout) {
> 		drm_dbg_kms(display->drm,
> 			    "Potential atomic update failure on pipe %c\n",
> 			    pipe_name(crtc->pipe));
> 		break;
> 	}
> 
> 	local_irq_enable();
> 
> 	prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> 
> 	if (!scanline_is_safe(evade, &scanline))
> 		timeout = schedule_timeout(timeout);
> 
> 	finish_wait(wq, &wait);
> 
> 	local_irq_disable();
> }
> 
> And then maybe the whole prepare+wait+finish thing there
> could be a simple wait_event_timeout(). That would make
> that inner thing a loop though. We might not want that
> just because jiffies is so coarse and we don't really
> want to wait multiple times there.
This could be a wait_event_timeout(), except it takes the struct waitqueue, and we
only have a pointer to the queue.

Wasn't sure if it wait_event_timeout(*queue, ...) was allowed, but it seems at least
the msm driver and even the atomic helper does so, so should be ok to use.
> 
>>  			drm_dbg_kms(display->drm,
>> @@ -740,15 +746,8 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>>  			break;
>>  		}
>>  
>> -		local_irq_enable();
>> -
>> -		timeout = schedule_timeout(timeout);
>> -
>> -		local_irq_disable();
>>  	}
>>  
>> -	finish_wait(wq, &wait);
>> -
>>  	/*
>>  	 * On VLV/CHV DSI the scanline counter would appear to
>>  	 * increment approx. 1/3 of a scanline before start of vblank.
>> -- 
>> 2.51.0
> 

Re: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in intel_pipe_fastset

[Maarten Lankhorst]

Hey,

Den 2025-11-26 kl. 20:42, skrev Ville Syrjälä:
> On Wed, Nov 26, 2025 at 07:19:47PM +0000, Shankar, Uma wrote:
>>
>>
>>> -----Original Message-----
>>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Maarten
>>> Lankhorst
>>> Sent: Tuesday, November 4, 2025 2:06 PM
>>> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>>> Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst <dev@lankhorst.se>; Mario
>>> Kleiner <mario.kleiner.de@gmail.com>; Mike Galbraith
>>> <umgwanakikbuti@gmail.com>; Thomas Gleixner <tglx@linutronix.de>; Sebastian
>>> Andrzej Siewior <bigeasy@linutronix.de>; Clark Williams
>>> <clrkwllms@kernel.org>; Steven Rostedt <rostedt@goodmis.org>
>>> Subject: [PATCH v2 2/7] drm/i915/display: Use intel_de_write_fw in
>>> intel_pipe_fastset
>>>
>>> intel_set_pipe_src_size(), hsw_set_linetime_wm(),
>>> intel_cpu_transcoder_set_m1_n1() and intel_set_transcoder_timings_lrr()
>>> are called from an atomic context on PREEMPT_RT, and should be using the _fw
>>> functions.
>>
>> This could be ok but we need to be sure that all are called with power domains up.
>> I think would be safe to keep this under RT check so that we don't end up breaking any
>> generic non RT usecase.
> 
> When removing the locks from register accesses one needs to consider
> what platforms the code runs on, what other register are on the same
> cacheline, and whether they can be accessed in parallel. If there is
> something there then we may not be able to remove the locks.
> 
> That's assuming the "system hangs when same cacheline is accessed from
> multiple cpus" issue is real for display registers, and I'm actually
> not 100% it is. But we'd need to run some tests on the affected systems
> (~ivb/hsw) to get any kind of confidence here. IIRC some old
> intel_gpu_top thhat directly poked the registers was very good at
> hitting it on hsw at least, so that would be a decent starting point.
> 
> Anyways, I'm going to be replacing the uncore lock with a display
> specific lock soonish, and I suppose I can just make that a raw
> spinlock to appease RT.


You're correct about the cacheline problem!

We shouldn't be having a race in those specific registers though. On the current
kernel they would likely hang the system if the _fw variants are not taken, as it
means the uncore lock would be taken twice.

This means the fastset path is already broken on systems requiring uncore lock, and
this simply fixes it instead.

Kind regards,
~Maarten Lankhorst