[PATCH 00/10] drm/intel/display: Changes required to make vblank evasion PREEMPT_RT safe.

[PATCH 00/10] drm/intel/display: Changes required to make vblank evasion PREEMPT_RT safe.

[Maarten Lankhorst]

CONFIG_PREEMPT_RT requires code to be as deterministic as possible,
and codes with interrupts disabled cannot use spinlocks.

In order to handle this correctly, we ensure any locking requirements
are handled before disabling interrupts, and only start locking again
after interrupts are re-enabled and timing sensitive path completed.

This also has the benefit of making the vblank evasion code more
deterministic; without locking there's much less jitter.

Previously, we scheduled vblank work on completion.
This is now handled by scheduling the vblank in advance, only arming
it upon completion without locks.

Maarten Lankhorst (9):
  drm/vblank_work: Add methods to schedule vblank_work in 2 stages
  drm/vblank: Add a 2-stage version of drm_crtc_arm_vblank_event
  drm/intel/display: Make intel_crtc_arm_vblank_event static
  drm/intel/display: Convert vblank event handling to 2-stage arming
  drm/i915/display: Move vblank put until after critical section
  drm/i915/display: Remove locking from intel_vblank_evade critical
    section
  drm/i915/display: Handle vlv dsi workaround in scanline_in_safe_range
    too
  drm/i915/display: Make get_vblank_counter use intel_de_read_fw()
  drm/i915/display: Do not take uncore lock in i915_get_vblank_counter

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

 drivers/gpu/drm/drm_vblank.c                  |  64 +++++++++-
 drivers/gpu/drm/drm_vblank_work.c             | 110 +++++++++++++----
 drivers/gpu/drm/i915/display/intel_crtc.c     |  86 ++++++-------
 drivers/gpu/drm/i915/display/intel_crtc.h     |   1 -
 drivers/gpu/drm/i915/display/intel_cursor.c   |   8 +-
 drivers/gpu/drm/i915/display/intel_de.h       |   8 ++
 drivers/gpu/drm/i915/display/intel_vblank.c   | 115 ++++++++++--------
 drivers/gpu/drm/i915/display/intel_vblank.h   |   1 +
 drivers/gpu/drm/i915/intel_uncore.h           |  26 ++--
 .../drm/xe/compat-i915-headers/intel_uncore.h |   7 ++
 include/drm/drm_vblank.h                      |  14 ++-
 include/drm/drm_vblank_work.h                 |  12 ++
 12 files changed, 324 insertions(+), 128 deletions(-)

-- 
2.53.0

[PATCH 01/10] drm/vblank_work: Add methods to schedule vblank_work in 2 stages

[Maarten Lankhorst]

In case of vblank evasion in intel/display, it's necessary to
perform some work in advance, so the critical section will always run in
constant time on PREEMPT_RT.

By preparing all the work in advance, the part that needs to finish in
constant time only has to write a single variable instead. This allows
PREEMPT_RT to keep the interrupts disabled at the most critical part,
without completely reworking all locks to be raw spinlocks.

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/drm_vblank_work.c | 110 +++++++++++++++++++++++-------
 include/drm/drm_vblank_work.h     |  12 ++++
 2 files changed, 96 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank_work.c b/drivers/gpu/drm/drm_vblank_work.c
index 70f0199251ea0..ed62c189fe041 100644
--- a/drivers/gpu/drm/drm_vblank_work.c
+++ b/drivers/gpu/drm/drm_vblank_work.c
@@ -54,7 +54,8 @@ void drm_handle_vblank_works(struct drm_vblank_crtc *vblank)
 	assert_spin_locked(&vblank->dev->event_lock);
 
 	list_for_each_entry_safe(work, next, &vblank->pending_work, node) {
-		if (!drm_vblank_passed(count, work->count))
+		/* READ_ONCE pairs with WRITE_ONCE in drm_vblank_work_enable() */
+		if (!READ_ONCE(work->armed) || !drm_vblank_passed(count, work->count))
 			continue;
 
 		list_del_init(&work->node);
@@ -86,30 +87,8 @@ void drm_vblank_cancel_pending_works(struct drm_vblank_crtc *vblank)
 	wake_up_all(&vblank->work_wait_queue);
 }
 
-/**
- * drm_vblank_work_schedule - schedule a vblank work
- * @work: vblank work to schedule
- * @count: target vblank count
- * @nextonmiss: defer until the next vblank if target vblank was missed
- *
- * Schedule @work for execution once the crtc vblank count reaches @count.
- *
- * If the crtc vblank count has already reached @count and @nextonmiss is
- * %false the work starts to execute immediately.
- *
- * If the crtc vblank count has already reached @count and @nextonmiss is
- * %true the work is deferred until the next vblank (as if @count has been
- * specified as crtc vblank count + 1).
- *
- * If @work is already scheduled, this function will reschedule said work
- * using the new @count. This can be used for self-rearming work items.
- *
- * Returns:
- * %1 if @work was successfully (re)scheduled, %0 if it was either already
- * scheduled or cancelled, or a negative error code on failure.
- */
-int drm_vblank_work_schedule(struct drm_vblank_work *work,
-			     u64 count, bool nextonmiss)
+static int __drm_vblank_work_schedule(struct drm_vblank_work *work,
+				      u64 count, bool nextonmiss, bool armed)
 {
 	struct drm_vblank_crtc *vblank = work->vblank;
 	struct drm_device *dev = vblank->dev;
@@ -139,6 +118,7 @@ int drm_vblank_work_schedule(struct drm_vblank_work *work,
 		rescheduling = true;
 	}
 
+	work->armed = armed;
 	work->count = count;
 	cur_vbl = drm_vblank_count(dev, vblank->pipe);
 	passed = drm_vblank_passed(cur_vbl, count);
@@ -147,7 +127,7 @@ int drm_vblank_work_schedule(struct drm_vblank_work *work,
 			     "crtc %d vblank %llu already passed (current %llu)\n",
 			     vblank->pipe, count, cur_vbl);
 
-	if (!nextonmiss && passed) {
+	if (!nextonmiss && passed && armed) {
 		drm_vblank_put(dev, vblank->pipe);
 		ret = kthread_queue_work(vblank->worker, &work->base);
 
@@ -167,8 +147,86 @@ int drm_vblank_work_schedule(struct drm_vblank_work *work,
 		wake_up_all(&vblank->work_wait_queue);
 	return ret;
 }
+
+/**
+ * drm_vblank_work_schedule - schedule a vblank work
+ * @work: vblank work to schedule
+ * @count: target vblank count
+ * @nextonmiss: defer until the next vblank if target vblank was missed
+ *
+ * Schedule @work for execution once the crtc vblank count reaches @count.
+ *
+ * If the crtc vblank count has already reached @count and @nextonmiss is
+ * %false the work starts to execute immediately.
+ *
+ * If the crtc vblank count has already reached @count and @nextonmiss is
+ * %true the work is deferred until the next vblank (as if @count has been
+ * specified as crtc vblank count + 1).
+ *
+ * If @work is already scheduled, this function will reschedule said work
+ * using the new @count. This can be used for self-rearming work items.
+ *
+ * Returns:
+ * %1 if @work was successfully (re)scheduled, %0 if it was either already
+ * scheduled or cancelled, or a negative error code on failure.
+ */
+int drm_vblank_work_schedule(struct drm_vblank_work *work,
+			     u64 count, bool nextonmiss)
+{
+	return __drm_vblank_work_schedule(work, count, nextonmiss, true);
+}
 EXPORT_SYMBOL(drm_vblank_work_schedule);
 
+
+/**
+ * drm_vblank_work_schedule_disabled - schedule a vblank work, withoug enabling
+ * @work: vblank work to schedule
+ * @count: target vblank count
+ *
+ * Schedule @work for execution once the crtc vblank count reaches @count.
+ *
+ * The vblank work will not be scheduled until drm_vblank_work_enable() is called.
+ * If the crtc vblank count has already reached @count, the work will still
+ * not be scheduled until the first following vblank.
+ *
+ * If @work is already scheduled, this function will reschedule said work
+ * using the new @count. This can be used for self-rearming work items.
+ *
+ * Returns:
+ * %1 if @work was successfully (re)scheduled, %0 if it was either already
+ * scheduled or cancelled, or a negative error code on failure.
+ */
+int drm_vblank_work_schedule_disabled(struct drm_vblank_work *work, u64 count)
+{
+	return __drm_vblank_work_schedule(work, count, true, false);
+}
+EXPORT_SYMBOL(drm_vblank_work_schedule_disabled);
+
+/**
+ * drm_vblank_work_enable - enable vblank work
+ * @work: vblank work to enable
+ *
+ * This function is specifically only for when drm_vblank_work_schedule_disabled() is
+ * called. It allows for the work to be armed in any context, without any locks.
+ *
+ * The work will be signalled earliest at the @count argument, if it has been passed,
+ * it will signalled at the next vblank.
+ *
+ * This is particularly useful for PREEMPT_RT, where the spin_lock is converted
+ * into a sleeping rtmutex, and vblank evasion requires some work to be
+ * scheduled on completion with interrupts disabled.
+ */
+void drm_vblank_work_enable(struct drm_vblank_work *work)
+{
+	WARN_ON(work->armed);
+
+	/* Ensure previous writes are visible */
+	smp_wmb();
+
+	WRITE_ONCE(work->armed, true);
+}
+EXPORT_SYMBOL(drm_vblank_work_enable);
+
 /**
  * drm_vblank_work_cancel_sync - cancel a vblank work and wait for it to
  * finish executing
diff --git a/include/drm/drm_vblank_work.h b/include/drm/drm_vblank_work.h
index e04d436b72973..e19351200da24 100644
--- a/include/drm/drm_vblank_work.h
+++ b/include/drm/drm_vblank_work.h
@@ -47,6 +47,14 @@ struct drm_vblank_work {
 	 */
 	int cancelling;
 
+	/**
+	 * @armed: If false, the work item has been added to the
+	 * drm_vblank_crtc.pending_work list, but will not yet be signalled.
+	 *
+	 * Call drm_vblank_work_enable() to fire on next vblank.
+	 */
+	bool armed;
+
 	/**
 	 * @node: The position of this work item in
 	 * &drm_vblank_crtc.pending_work.
@@ -64,6 +72,10 @@ struct drm_vblank_work {
 
 int drm_vblank_work_schedule(struct drm_vblank_work *work,
 			     u64 count, bool nextonmiss);
+
+int drm_vblank_work_schedule_disabled(struct drm_vblank_work *work, u64 count);
+void drm_vblank_work_enable(struct drm_vblank_work *work);
+
 void drm_vblank_work_init(struct drm_vblank_work *work, struct drm_crtc *crtc,
 			  void (*func)(struct kthread_work *work));
 bool drm_vblank_work_cancel_sync(struct drm_vblank_work *work);
-- 
2.53.0

Re: [PATCH 01/10] drm/vblank_work: Add methods to schedule vblank_work in 2 stages

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing read memory barrier (`smp_rmb` or `smp_load_acquire`) when reading `work->armed` in `drm_handle_vblank_works` breaks transitive ordering.
- [Medium] `drm_vblank_work_schedule` early return logic fails to arm a disabled work if the target vblank count is identical.

Pre-existing issues:
- [High] Use-after-free risk due to calling `list_del_init(&work->node)` after `kthread_queue_work()`.
--

commit 2d70bdfd10b5fd90f1bf2ecac514ae3f86ae65ba
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/vblank_work: Add methods to schedule vblank_work in 2 stages

In case of vblank evasion in intel/display, it's necessary to
perform some work in advance, so the critical section will always run in
constant time on PREEMPT_RT.

> diff --git a/drivers/gpu/drm/drm_vblank_work.c b/drivers/gpu/drm/drm_vblank_work.c
> index 70f0199251ea0..ed62c189fe041 100644
> --- a/drivers/gpu/drm/drm_vblank_work.c
> +++ b/drivers/gpu/drm/drm_vblank_work.c
> @@ -54,7 +54,8 @@ void drm_handle_vblank_works(struct drm_vblank_crtc *vblank)
>  	assert_spin_locked(&vblank->dev->event_lock);
>  
>  	list_for_each_entry_safe(work, next, &vblank->pending_work, node) {
> -		if (!drm_vblank_passed(count, work->count))
> +		/* READ_ONCE pairs with WRITE_ONCE in drm_vblank_work_enable() */
> +		if (!READ_ONCE(work->armed) || !drm_vblank_passed(count, work->count))
>  			continue;

[Severity: High]
Does this READ_ONCE() need a corresponding read memory barrier like
smp_rmb() or smp_load_acquire() to establish transitive ordering?

Since drm_vblank_work_enable() uses an smp_wmb() before writing to
work->armed, if the reader just uses READ_ONCE() here without an acquire
barrier before transferring the work via kthread_queue_work(), the memory
model doesn't guarantee the writer's data updates will be transitively
visible to the worker thread.

Could the kthread end up reading stale driver state as a result?

[ ... ]

> @@ -139,6 +118,7 @@ int drm_vblank_work_schedule(struct drm_vblank_work *work,
>  		rescheduling = true;
>  	}
>  
> +	work->armed = armed;
>  	work->count = count;

[Severity: Medium]
Does the early return logic in __drm_vblank_work_schedule() break if a work
item is first scheduled disabled, and then re-scheduled to be armed with the
exact same target count?

Earlier in __drm_vblank_work_schedule(), if work->count == count, the function
takes an early return:

	} else if (work->count == count) {
		/* Already scheduled w/ same vbl count */
		goto out;
	} else {

If a driver uses drm_vblank_work_schedule_disabled() to schedule the work,
and later calls drm_vblank_work_schedule() with the exact same count to arm it,
doesn't the early return bypass this new work->armed = armed assignment?

Could this leave the work permanently disabled and never executed?

[ ... ]

> @@ -147,7 +127,7 @@ int drm_vblank_work_schedule(struct drm_vblank_work *work,
>  			     "crtc %d vblank %llu already passed (current %llu)\n",
>  			     vblank->pipe, count, cur_vbl);
>  
> -	if (!nextonmiss && passed) {
> +	if (!nextonmiss && passed && armed) {
>  		drm_vblank_put(dev, vblank->pipe);
>  		ret = kthread_queue_work(vblank->worker, &work->base);
>  

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk here when
rescheduling a work item that has already passed?

If rescheduling is true, the code right below this in
__drm_vblank_work_schedule() does:

		ret = kthread_queue_work(vblank->worker, &work->base);

		if (rescheduling) {
			list_del_init(&work->node);
			wake = true;
		}

Since kthread_queue_work() immediately places the work on the worker's queue,
could the worker thread run concurrently and execute the work before
list_del_init() is called on the current CPU?

If the work is freed by the executing kthread, the list_del_init() call
would then access freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702072154.171324-1-dev@lankhorst.se?part=1

[PATCH 02/10] drm/vblank: Add a 2-stage version of drm_crtc_arm_vblank_event

[Maarten Lankhorst]

When trying to fix the hardware programming in intel/display, I had
to take all the vblank locks with local_irqs_disabled(). This
required converting the entire vblank code to raw spinlocks.

In the alternative approach, do all preparations in advance, and only
enable the vblank_event with interrupts disabled, this requires only
a simple write and prevents a complete re-architecture of the code.

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
---
 drivers/gpu/drm/drm_vblank.c | 64 +++++++++++++++++++++++++++++++++++-
 include/drm/drm_vblank.h     | 14 +++++++-
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f90fb2d13e423..b52e0215312f8 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1127,12 +1127,71 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
 
 	assert_spin_locked(&dev->event_lock);
 
+	WARN_ON(e->postponed);
 	e->pipe = pipe;
 	e->sequence = drm_crtc_accurate_vblank_count(crtc) + 1;
 	list_add_tail(&e->base.link, &dev->vblank_event_list);
 }
 EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
 
+/**
+ * drm_crtc_prepare_arm_vblank_event - arm vblank event *before* pageflip.
+ * @crtc: the source CRTC of the vblank event
+ * @e: the event to send
+ *
+ * See drm_crtc_arm_vblank_event(). This function is a 2-stage version of
+ * that call. This function is called *BEFORE* programming the hardware.
+ *
+ * After programming, call drm_crtc_arm_prepared_vblank_event() and the
+ * event will be scheduled on the next vblank.
+ *
+ * This is mainly useful for code that has to run on PREEMPT_RT kernels,
+ * with interrupts disabled, since all vblank spinlocks are converted to
+ * rtmutexes, and code running with irqs disabled cannot take any vblank lock.
+ *
+ * It also increases determinism for any hardware
+ * programming, since no vblank related locks are taking when arming.
+ */
+void drm_crtc_prepare_arm_vblank_event(struct drm_crtc *crtc,
+				       struct drm_pending_vblank_event *e)
+{
+	drm_crtc_arm_vblank_event(crtc, e);
+
+	/* Set the flag, so that the event is not fired yet */
+	e->postponed = true;
+}
+EXPORT_SYMBOL(drm_crtc_prepare_arm_vblank_event);
+
+/**
+ * drm_crtc_arm_prepared_vblank_event - arm prepared vblank event *after* pageflip.
+ * @crtc: the source CRTC of the vblank event
+ * @e: the event to send
+ *
+ * See drm_crtc_prepare_arm_vblank_event(). This function is a 2-stage version of
+ * that call. This function is called directly *AFTER* programming the hardware.
+ *
+ * Before this function is called, drm_crtc_prepare_arm_vblank_event() should be
+ * called instead.
+ *
+ * This is mainly useful for code that has to run on PREEMPT_RT kernels,
+ * with interrupts disabled, since all vblank spinlocks are converted to
+ * rtmutexes, and code running with irqs disabled cannot take any vblank lock.
+ *
+ * It also increases determinism for any hardware
+ * programming, since no vblank related locks are taking when arming.
+ */
+void drm_crtc_arm_prepared_vblank_event(struct drm_pending_vblank_event *e)
+{
+	WARN_ON(!e->postponed);
+
+	/* Ensure previous writes are visible */
+	smp_wmb();
+
+	/* remove the flag to be processed as a normal event */
+	WRITE_ONCE(e->postponed, false);
+}
+EXPORT_SYMBOL(drm_crtc_arm_prepared_vblank_event);
+
 /**
  * drm_crtc_send_vblank_event - helper to send vblank event after pageflip
  * @crtc: the source CRTC of the vblank event
@@ -1390,6 +1449,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
 	list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
 		if (e->pipe != pipe)
 			continue;
+
+		WARN_ON(e->postponed);
 		drm_dbg_core(dev, "Sending premature vblank event on disable: "
 			     "wanted %llu, current %llu\n",
 			     e->sequence, seq);
@@ -1892,7 +1953,8 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
 	seq = drm_vblank_count_and_time(dev, pipe, &now);
 
 	list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
-		if (e->pipe != pipe)
+		/* Matches WRITE_ONCE in drm_crtc_arm_prepared_vblank_event() */
+		if (e->pipe != pipe || READ_ONCE(e->postponed))
 			continue;
 		if (!drm_vblank_passed(seq, e->sequence))
 			continue;
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 2fcef9c0f5b1b..956d5621eb7f9 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -53,6 +53,13 @@ struct drm_pending_vblank_event {
 	 * @sequence: frame event should be triggered at
 	 */
 	u64 sequence;
+
+	/**
+	 * @postponed: whether drm_crtc_prepare_arm_vblank_event() is called,
+	 * and drm_crtc_arm_prepared_vblank_event has yet to be called to arm.
+	 */
+	bool postponed;
+
 	/**
 	 * @event: Actual event which will be sent to userspace.
 	 */
@@ -294,7 +301,12 @@ int drm_crtc_next_vblank_start(struct drm_crtc *crtc, ktime_t *vblanktime);
 void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 			       struct drm_pending_vblank_event *e);
 void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
-			      struct drm_pending_vblank_event *e);
+			       struct drm_pending_vblank_event *e);
+
+void drm_crtc_prepare_arm_vblank_event(struct drm_crtc *crtc,
+				       struct drm_pending_vblank_event *e);
+void drm_crtc_arm_prepared_vblank_event(struct drm_pending_vblank_event *e);
+
 void drm_vblank_set_event(struct drm_pending_vblank_event *e,
 			  u64 *seq,
 			  ktime_t *now);
-- 
2.53.0

Re: [PATCH 02/10] drm/vblank: Add a 2-stage version of drm_crtc_arm_vblank_event

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Insufficient memory barrier for ordering MMIO hardware programming against vblank event arming
--

commit e7abec27222d12389651c409a1f108b4e73b0d3e
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/vblank: Add a 2-stage version of drm_crtc_arm_vblank_event

This patch introduces drm_crtc_prepare_arm_vblank_event() and
drm_crtc_arm_prepared_vblank_event() to allow arming vblank events in two
stages. This avoids taking vblank-related spinlocks with interrupts disabled
during hardware programming, which is useful for PREEMPT_RT kernels.

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f90fb2d13e423..b52e0215312f8 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
[ ... ]
> +void drm_crtc_arm_prepared_vblank_event(struct drm_pending_vblank_event *e)
> +{
> +	WARN_ON(!e->postponed);
> +
> +	/* Ensure previous writes are visible */
> +	smp_wmb();
> +
> +	/* remove the flag to be processed as a normal event */
> +	WRITE_ONCE(e->postponed, false);
> +}

[Severity: Medium]
Is smp_wmb() sufficient to order prior MMIO hardware programming against this
memory write to e->postponed?

If the intent is to order the prior hardware programming (MMIO writes) against
setting e->postponed to false, smp_wmb() is strictly a CPU-to-CPU barrier for
normal memory. On weakly-ordered architectures (like ARM64), the CPU can
reorder the normal memory write (postponed = false) before the MMIO writes are
issued to the bus, potentially causing the vblank interrupt handler to process
the event before the hardware programming is complete.

Does this need a full wmb() to correctly order I/O against memory?

Conversely, if it were intended for CPU memory ordering, the reader in
drm_handle_vblank_events() lacks a matching smp_rmb(). Since all other CPU
memory fields are protected by event_lock, there shouldn't be lockless CPU
memory writes to order here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702072154.171324-1-dev@lankhorst.se?part=2

[PATCH 03/10] drm/intel/display: Make intel_crtc_arm_vblank_event static

[Maarten Lankhorst]

Only used inside intel_crtc.c now, so no need to export it any more.

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

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index 10ed9bdfee763..805645318747f 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -684,7 +684,7 @@ static void dbg_vblank_evade(struct intel_crtc *crtc, ktime_t end)
 static void dbg_vblank_evade(struct intel_crtc *crtc, ktime_t end) {}
 #endif
 
-void intel_crtc_arm_vblank_event(struct intel_crtc_state *crtc_state)
+static void intel_crtc_arm_vblank_event(struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	unsigned long irqflags;
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h b/drivers/gpu/drm/i915/display/intel_crtc.h
index 12507b51ee77e..f65cbafe2b42a 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.h
+++ b/drivers/gpu/drm/i915/display/intel_crtc.h
@@ -33,7 +33,6 @@ int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
 			     int usecs);
 int intel_scanlines_to_usecs(const struct drm_display_mode *adjusted_mode,
 			     int scanlines);
-void intel_crtc_arm_vblank_event(struct intel_crtc_state *crtc_state);
 void intel_crtc_prepare_vblank_event(struct intel_crtc_state *crtc_state,
 				     struct drm_pending_vblank_event **event);
 u32 intel_crtc_max_vblank_count(const struct intel_crtc_state *crtc_state);
-- 
2.53.0

Re: [PATCH 03/10] drm/intel/display: Make intel_crtc_arm_vblank_event static

[Ville Syrjälä]

On Thu, Jul 02, 2026 at 09:21:46AM +0200, Maarten Lankhorst wrote:
> Only used inside intel_crtc.c now, so no need to export it any more.
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_crtc.c | 2 +-
>  drivers/gpu/drm/i915/display/intel_crtc.h | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 10ed9bdfee763..805645318747f 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -684,7 +684,7 @@ static void dbg_vblank_evade(struct intel_crtc *crtc, ktime_t end)
>  static void dbg_vblank_evade(struct intel_crtc *crtc, ktime_t end) {}
>  #endif
>  
> -void intel_crtc_arm_vblank_event(struct intel_crtc_state *crtc_state)
> +static void intel_crtc_arm_vblank_event(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	unsigned long irqflags;
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h b/drivers/gpu/drm/i915/display/intel_crtc.h
> index 12507b51ee77e..f65cbafe2b42a 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.h
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.h
> @@ -33,7 +33,6 @@ int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
>  			     int usecs);
>  int intel_scanlines_to_usecs(const struct drm_display_mode *adjusted_mode,
>  			     int scanlines);
> -void intel_crtc_arm_vblank_event(struct intel_crtc_state *crtc_state);
>  void intel_crtc_prepare_vblank_event(struct intel_crtc_state *crtc_state,
>  				     struct drm_pending_vblank_event **event);
>  u32 intel_crtc_max_vblank_count(const struct intel_crtc_state *crtc_state);
> -- 
> 2.53.0

-- 
Ville Syrjälä
Intel

[PATCH 04/10] drm/intel/display: Convert vblank event handling to 2-stage arming

[Maarten Lankhorst]

This is converts the vblank functions to be called with interrupts
disabled, even on PREEMPT_RT kernels.

Because the PREEMPT_RT kernel converts all spinlocks to rt-mutexes,
the normal vblank functions cannot be used inside the critical section.

Instead, prepare the vblank at the start, and then enable the vblank
work after the hardware programming is completed.

This allows us to keep programming the hardware with interrupts
disabled, and still schedule completion on PREEMPT_RT on next vblank.

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

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index 805645318747f..34a159f7c9a43 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -527,6 +527,10 @@ static void intel_crtc_vblank_work_init(struct intel_crtc_state *crtc_state)
 
 	drm_vblank_work_init(&crtc_state->vblank_work, &crtc->base,
 			     intel_crtc_vblank_work);
+
+	drm_vblank_work_schedule_disabled(&crtc_state->vblank_work,
+					  drm_crtc_accurate_vblank_count(&crtc->base) + 1);
+
 	/*
 	 * Interrupt latency is critical for getting the vblank
 	 * work executed as early as possible during the vblank.
@@ -571,6 +575,21 @@ int intel_scanlines_to_usecs(const struct drm_display_mode *adjusted_mode,
 				adjusted_mode->crtc_clock);
 }
 
+static void intel_crtc_arm_vblank_event(struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	unsigned long irqflags;
+
+	if (!crtc_state->uapi.event)
+		return;
+
+	drm_WARN_ON(crtc->base.dev, drm_crtc_vblank_get(&crtc->base) != 0);
+
+	spin_lock_irqsave(&crtc->base.dev->event_lock, irqflags);
+	drm_crtc_prepare_arm_vblank_event(&crtc->base, crtc_state->uapi.event);
+	spin_unlock_irqrestore(&crtc->base.dev->event_lock, irqflags);
+}
+
 /**
  * intel_pipe_update_start() - start update of a set of display registers
  * @state: the atomic state
@@ -607,6 +626,8 @@ void intel_pipe_update_start(struct intel_atomic_state *state,
 
 	if (intel_crtc_needs_vblank_work(new_crtc_state))
 		intel_crtc_vblank_work_init(new_crtc_state);
+	else
+		intel_crtc_arm_vblank_event(new_crtc_state);
 
 	if (state->base.legacy_cursor_update) {
 		struct intel_plane *plane;
@@ -684,23 +705,6 @@ static void dbg_vblank_evade(struct intel_crtc *crtc, ktime_t end)
 static void dbg_vblank_evade(struct intel_crtc *crtc, ktime_t end) {}
 #endif
 
-static void intel_crtc_arm_vblank_event(struct intel_crtc_state *crtc_state)
-{
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
-	unsigned long irqflags;
-
-	if (!crtc_state->uapi.event)
-		return;
-
-	drm_WARN_ON(crtc->base.dev, drm_crtc_vblank_get(&crtc->base) != 0);
-
-	spin_lock_irqsave(&crtc->base.dev->event_lock, irqflags);
-	drm_crtc_arm_vblank_event(&crtc->base, crtc_state->uapi.event);
-	spin_unlock_irqrestore(&crtc->base.dev->event_lock, irqflags);
-
-	crtc_state->uapi.event = NULL;
-}
-
 void intel_crtc_prepare_vblank_event(struct intel_crtc_state *crtc_state,
 				     struct drm_pending_vblank_event **event)
 {
@@ -754,29 +758,10 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
 	 * event outside of the critical section - the spinlock might spin for a
 	 * while ... */
 	if (intel_crtc_needs_vblank_work(new_crtc_state)) {
-		drm_vblank_work_schedule(&new_crtc_state->vblank_work,
-					 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
-					 false);
-	} else {
-		intel_crtc_arm_vblank_event(new_crtc_state);
-	}
-
-	if (state->base.legacy_cursor_update) {
-		struct intel_plane *plane;
-		struct intel_plane_state *old_plane_state;
-		int i;
-
-		for_each_old_intel_plane_in_state(state, plane, old_plane_state, i) {
-			if (old_plane_state->hw.crtc == &crtc->base &&
-			    old_plane_state->unpin_work.vblank) {
-				drm_vblank_work_schedule(&old_plane_state->unpin_work,
-							 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
-							 false);
-
-				/* Remove plane from atomic state, cleanup/free is done from vblank worker. */
-				memset(&state->base.planes[i], 0, sizeof(state->base.planes[i]));
-			}
-		}
+		drm_vblank_work_enable(&new_crtc_state->vblank_work);
+	} else if (new_crtc_state->uapi.event) {
+		drm_crtc_arm_prepared_vblank_event(new_crtc_state->uapi.event);
+		new_crtc_state->uapi.event = NULL;
 	}
 
 	/*
@@ -800,6 +785,25 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
 
 	local_irq_enable();
 
+	/* Run after local_irq_enable(), not timing sensitive */
+	if (state->base.legacy_cursor_update) {
+		struct intel_plane *plane;
+		struct intel_plane_state *old_plane_state;
+		int i;
+
+		for_each_old_intel_plane_in_state(state, plane, old_plane_state, i) {
+			if (old_plane_state->hw.crtc == &crtc->base &&
+			    old_plane_state->unpin_work.vblank) {
+				drm_vblank_work_schedule(&old_plane_state->unpin_work,
+							 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
+							 false);
+
+				/* Remove plane from atomic state, cleanup/free is done from vblank worker. */
+				memset(&state->base.planes[i], 0, sizeof(state->base.planes[i]));
+			}
+		}
+	}
+
 	if (intel_parent_vgpu_active(display))
 		goto out;
 
-- 
2.53.0

Re: [PATCH 04/10] drm/intel/display: Convert vblank event handling to 2-stage arming

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Moving vblank event preparation and counter sampling before vblank evasion causes events and workers to fire one frame early if an evasion wait occurs.

Pre-existing issues:
- [High] drm_crtc_vblank_put() is called within a local_irq_disable() critical section, which can acquire a sleepable rt_mutex on PREEMPT_RT kernels, causing a lockdep splat and potential system crash.
--

commit 90a4948ef379d3e7a2ebc312abe7069b65f38a68
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/intel/display: Convert vblank event handling to 2-stage arming

This converts the vblank functions to be called with interrupts
disabled, even on PREEMPT_RT kernels. Instead of using the normal
vblank functions inside the critical section, it prepares the vblank
at the start and enables the work after hardware programming completes.

> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 805645318747f..34a159f7c9a43 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
[ ... ]
> @@ -607,6 +626,8 @@ void intel_pipe_update_start(struct intel_atomic_state *state,
>  
>  	if (intel_crtc_needs_vblank_work(new_crtc_state))
>  		intel_crtc_vblank_work_init(new_crtc_state);
> +	else
> +		intel_crtc_arm_vblank_event(new_crtc_state);

[Severity: High]
Does moving the vblank event preparation and counter sampling before the
evasion wait cause the event to fire a frame early if the wait actually
spins?

If intel_pipe_update_start() locks the target vblank sequence to the
current vblank count + 1, and the CPU is delayed so that the evasion
wait spins:

intel_pipe_update_start() {
    ...
    scanline = intel_vblank_evade(&evade);
    ...
}

The hardware latching point will have passed, meaning the registers will
apply at N+2. However, because the event sequence is already locked to N+1
prior to the evasion, the event will fire a frame early in
intel_pipe_update_end() when un-postponed:

intel_pipe_update_end() {
    ...
    drm_crtc_arm_prepared_vblank_event(new_crtc_state->uapi.event);
    ...
}

Could this lead to screen tearing or premature compositor synchronization?

[Severity: High]
This is a pre-existing issue, but does calling drm_crtc_vblank_put() inside
the local_irq_disable() block later in this function cause issues on
PREEMPT_RT kernels?

intel_pipe_update_start() {
    ...
    local_irq_disable();
    ...
    scanline = intel_vblank_evade(&evade);
    drm_crtc_vblank_put(&crtc->base);
    ...
}

If this call drops the last vblank reference and immediate vblank disable is
configured, vblank_disable_fn() executes synchronously and acquires
dev->vbl_lock.

Since this spinlock is converted to a sleepable rt_mutex on PREEMPT_RT
kernels, will this cause a lockdep splat or panic by sleeping in atomic
context?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702072154.171324-1-dev@lankhorst.se?part=4

[PATCH 05/10] 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>
Reviewed-by: Uma Shankar <uma.shankar@intel.com>
---
 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 88384dea868b4..68b44a00567e4 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.53.0

Re: [PATCH 05/10] drm/i915/display: Move vblank put until after critical section

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The fix for moving `drm_crtc_vblank_put()` out of the time-sensitive critical section was only applied to the legacy cursor update, leaving the identical anti-pattern in the main atomic update path.
--

commit 43066708128be1b281ea9a6a390e979ec4569021
Author: Maarten Lankhorst <dev@lankhorst.se>

    drm/i915/display: Move vblank put until after critical section

This commit moves drm_crtc_vblank_put() after the time-sensitive critical
section in intel_legacy_cursor_update() to avoid acquiring locks with
interrupts disabled. It also introduces a flag to defer putting the vblank
reference until programming is completed.

> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 88384dea868b4..68b44a00567e4 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c

[ ... ]

> @@ -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);
> +

[Severity: High]
This isn't a bug introduced by this patch, but should the same fix be applied
to the main atomic update path in intel_pipe_update_start()?

If the kernel parameter drm.vblankoffdelay < 0 is set, dropping the vblank
reference synchronously takes dev->vbl_lock inside vblank_disable_fn().
On PREEMPT_RT kernels, acquiring this spinlock (rt_mutex) with hardware
interrupts disabled causes a "BUG: sleeping function called from invalid
context" panic. Even on non-RT kernels, taking locks here causes jitter
that can disrupt hardware programming.

drivers/gpu/drm/i915/display/intel_crtc.c:intel_pipe_update_start() {
    ...
	local_irq_disable();

	crtc->debug.min_vbl = evade.min;
	crtc->debug.max_vbl = evade.max;
	trace_intel_pipe_update_start(crtc);

	scanline = intel_vblank_evade(&evade);

	drm_crtc_vblank_put(&crtc->base);
    ...
}

Could drm_crtc_vblank_put() also be moved out of the IRQ-disabled section
here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702072154.171324-1-dev@lankhorst.se?part=5

[PATCH 06/10] 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_crtc.c   |  2 +-
 drivers/gpu/drm/i915/display/intel_vblank.c | 30 +++++++++------------
 drivers/gpu/drm/i915/display/intel_vblank.h |  1 +
 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index 34a159f7c9a43..8218938985b41 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -734,7 +734,7 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
 	struct intel_crtc_state *new_crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
 	enum pipe pipe = crtc->pipe;
-	int scanline_end = intel_get_crtc_scanline(crtc);
+	int scanline_end = __intel_get_crtc_scanline(crtc);
 	u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
 	ktime_t end_vbl_time = ktime_get();
 
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 28d81199792ef..ca08059e088ea 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -241,7 +241,7 @@ int intel_crtc_scanline_offset(const struct intel_crtc_state *crtc_state)
  * intel_de_read_fw(), only for fast reads of display block, no need for
  * forcewake etc.
  */
-static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
+int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 {
 	struct intel_display *display = to_intel_display(crtc);
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(&crtc->base);
@@ -732,6 +732,16 @@ void intel_vblank_evade_init(const struct intel_crtc_state *old_crtc_state,
 		evade->min -= vblank_delay;
 }
 
+static bool scanline_in_safe_range(struct intel_vblank_evade_ctx *evade, int *scanline, bool unlocked)
+{
+	if (unlocked)
+		*scanline = intel_get_crtc_scanline(evade->crtc);
+	else
+		*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)
 {
@@ -739,24 +749,12 @@ 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);
-
-		scanline = intel_get_crtc_scanline(crtc);
-		if (scanline < evade->min || scanline > evade->max)
-			break;
-
+	while (!scanline_in_safe_range(evade, &scanline, false)) {
 		if (!timeout) {
 			drm_dbg_kms(display->drm,
 				    "Potential atomic update failure on pipe %c\n",
@@ -766,13 +764,11 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
 
 		local_irq_enable();
 
-		timeout = schedule_timeout(timeout);
+		timeout = wait_event_timeout(*wq, scanline_in_safe_range(evade, &scanline, true), 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.
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.h b/drivers/gpu/drm/i915/display/intel_vblank.h
index 98d04cacd65f8..aa1974400e9fc 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.h
+++ b/drivers/gpu/drm/i915/display/intel_vblank.h
@@ -38,6 +38,7 @@ u32 g4x_get_vblank_counter(struct drm_crtc *crtc);
 bool intel_crtc_get_vblank_timestamp(struct drm_crtc *crtc, int *max_error,
 				     ktime_t *vblank_time, bool in_vblank_irq);
 int intel_get_crtc_scanline(struct intel_crtc *crtc);
+int __intel_get_crtc_scanline(struct intel_crtc *crtc);
 void intel_wait_for_pipe_scanline_stopped(struct intel_crtc *crtc);
 void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc);
 void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state,
-- 
2.53.0

Re: [PATCH 06/10] drm/i915/display: Remove locking from intel_vblank_evade critical section

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Bypassing `uncore->lock` for scanline reads breaks the hardware serialization workaround for older platforms like Ivy Bridge and Haswell, potentially causing hardware hangs.

Pre-existing issues:
- [High] drm_crtc_vblank_put() is called with local interrupts disabled in intel_pipe_update_start().
--

commit a415cb59c253b3a3fa754a68afc62b743249b0f2
Author: Maarten Lankhorst <dev@lankhorst.se>

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.

> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 34a159f7c9a43..8218938985b41 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -734,7 +734,7 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
>  	struct intel_crtc_state *new_crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
>  	enum pipe pipe = crtc->pipe;
> -	int scanline_end = intel_get_crtc_scanline(crtc);
> +	int scanline_end = __intel_get_crtc_scanline(crtc);

[Severity: High]
Does bypassing uncore->lock here break the hardware serialization workaround
for older platforms?

Calling __intel_get_crtc_scanline() without the lock in intel_pipe_update_end()
risks concurrent mmio access with other threads, such as when reading the
PIPEDSL register, which could cause hardware hangs on platforms like Ivy Bridge
and Haswell.

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 28d81199792ef..ca08059e088ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -732,6 +732,16 @@ void intel_vblank_evade_init(const struct intel_crtc_state *old_crtc_state,
>  		evade->min -= vblank_delay;
>  }
>  
> +static bool scanline_in_safe_range(struct intel_vblank_evade_ctx *evade, int *scanline, bool unlocked)
> +{
> +	if (unlocked)
> +		*scanline = intel_get_crtc_scanline(evade->crtc);
> +	else
> +		*scanline = __intel_get_crtc_scanline(evade->crtc);

[Severity: High]
Could this lockless scanline read cause a hardware hang during evasion?

Similar to the above, calling __intel_get_crtc_scanline() here during
the preempt-disabled evasion loop in scanline_in_safe_range() bypasses the
required hardware serialization lock. Older platforms might experience hangs
due to unsynchronized mmio cacheline access.

> +
> +	return *scanline < evade->min || *scanline > evade->max;
> +}

[ ... ]

[Severity: High]
This is a pre-existing issue, but intel_pipe_update_start() calls
drm_crtc_vblank_put() with local interrupts disabled:

drivers/gpu/drm/i915/display/intel_crtc.c:intel_pipe_update_start() {
    ...
    local_irq_disable();

    crtc->debug.min_vbl = evade.min;
    crtc->debug.max_vbl = evade.max;
    trace_intel_pipe_update_start(crtc);

    scanline = intel_vblank_evade(&evade);

    drm_crtc_vblank_put(&crtc->base);
    ...
}

Could this lead to scheduling while atomic on preempt-rt kernels?

When the reference count drops to zero, drm_crtc_vblank_put() might acquire
dev->vbl_lock, which is a spinlock_t that sleeps on preempt-rt.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702072154.171324-1-dev@lankhorst.se?part=6

[PATCH 07/10] drm/i915/display: Handle vlv dsi workaround in scanline_in_safe_range too

[Maarten Lankhorst]

Now that we have a macro, might as well handle the VLV dsi workaround
too.

This makes the vblank evasion code slightly more deterministic, by not
looping with interrupts disabled.

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
Reviewed-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/display/intel_vblank.c | 36 ++++++++++-----------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index ca08059e088ea..6f1fa952b5391 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -739,6 +739,24 @@ static bool scanline_in_safe_range(struct intel_vblank_evade_ctx *evade, int *sc
 	else
 		*scanline = __intel_get_crtc_scanline(evade->crtc);
 
+	/*
+	 * On VLV/CHV DSI the scanline counter would appear to
+	 * increment approx. 1/3 of a scanline before start of vblank.
+	 * The registers still get latched at start of vblank however.
+	 * This means we must not write any registers on the first
+	 * line of vblank (since not the whole line is actually in
+	 * vblank). And unfortunately we can't use the interrupt to
+	 * wait here since it will fire too soon. We could use the
+	 * frame start interrupt instead since it will fire after the
+	 * critical scanline, but that would require more changes
+	 * in the interrupt code. So for now we'll just do the nasty
+	 * thing and poll for the bad scanline to pass us by.
+	 *
+	 * FIXME figure out if BXT+ DSI suffers from this as well
+	 */
+	if (evade->need_vlv_dsi_wa && *scanline == evade->vblank_start)
+		return false;
+
 	return *scanline < evade->min || *scanline > evade->max;
 }
 
@@ -769,24 +787,6 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
 		local_irq_disable();
 	}
 
-	/*
-	 * On VLV/CHV DSI the scanline counter would appear to
-	 * increment approx. 1/3 of a scanline before start of vblank.
-	 * The registers still get latched at start of vblank however.
-	 * This means we must not write any registers on the first
-	 * line of vblank (since not the whole line is actually in
-	 * vblank). And unfortunately we can't use the interrupt to
-	 * wait here since it will fire too soon. We could use the
-	 * frame start interrupt instead since it will fire after the
-	 * critical scanline, but that would require more changes
-	 * in the interrupt code. So for now we'll just do the nasty
-	 * thing and poll for the bad scanline to pass us by.
-	 *
-	 * FIXME figure out if BXT+ DSI suffers from this as well
-	 */
-	while (evade->need_vlv_dsi_wa && scanline == evade->vblank_start)
-		scanline = intel_get_crtc_scanline(crtc);
-
 	return scanline;
 }
 
-- 
2.53.0

Re: [PATCH 07/10] drm/i915/display: Handle vlv dsi workaround in scanline_in_safe_range too

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving the VLV/CHV DSI hardware workaround into `scanline_in_safe_range` replaces a microsecond-level busy-wait with an unconditional waitqueue sleep, causing a massive latency spike and breaking vblank evasion timing.
--

commit a51b3c62707f214ae277254b37dc4aad780644a2
Author: Maarten Lankhorst <dev@lankhorst.se>

    drm/i915/display: Handle vlv dsi workaround in scanline_in_safe_range too

This commit moves the VLV/CHV DSI hardware workaround into
scanline_in_safe_range(). This is intended to make the vblank evasion
code slightly more deterministic by not looping with interrupts disabled.

> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index ca08059e088ea..6f1fa952b5391 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -739,6 +739,24 @@ static bool scanline_in_safe_range(struct intel_vblank_evade_ctx *evade, int *sc
>  	else
>  		*scanline = __intel_get_crtc_scanline(evade->crtc);
>  
> +	/*
> +	 * On VLV/CHV DSI the scanline counter would appear to
> +	 * increment approx. 1/3 of a scanline before start of vblank.
[ ... ]
> +	 *
> +	 * FIXME figure out if BXT+ DSI suffers from this as well
> +	 */
> +	if (evade->need_vlv_dsi_wa && *scanline == evade->vblank_start)
> +		return false;

[Severity: High]
If this returns false because we are in the bad scanline window, will the
wait_event_timeout() in intel_vblank_evade() put the thread to sleep for the
timeout duration?

Since the vblank interrupt fires early on these platforms and there are no
subsequent interrupts for the frame, it seems this wait queue would sleep
until the 1 jiffy timeout expires, rather than busy-waiting the ~10
microseconds for the scanline to pass. Could this cause the atomic commit
thread to completely miss the safe window to update display registers?

> +
>  	return *scanline < evade->min || *scanline > evade->max;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702072154.171324-1-dev@lankhorst.se?part=7

[PATCH 08/10] 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>
Reviewed-by: Uma Shankar <uma.shankar@intel.com>
---
 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 6f1fa952b5391..07dca05332e5b 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -316,6 +316,20 @@ static void intel_vblank_section_exit(struct intel_display *display)
 	struct intel_uncore *uncore = to_intel_uncore(display->drm);
 	spin_unlock(&uncore->lock);
 }
+
+static void intel_vblank_section_enter_irqf(struct intel_display *display, unsigned long *flags)
+	__acquires(i915->uncore.lock)
+{
+	struct intel_uncore *uncore = to_intel_uncore(display->drm);
+	spin_lock_irqsave(&uncore->lock, *flags);
+}
+
+static void intel_vblank_section_exit_irqf(struct intel_display *display, unsigned long flags)
+	__releases(i915->uncore.lock)
+{
+	struct intel_uncore *uncore = to_intel_uncore(display->drm);
+	spin_unlock_irqrestore(&uncore->lock, flags);
+}
 #else
 static void intel_vblank_section_enter(struct intel_display *display)
 {
@@ -324,6 +338,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,
@@ -360,10 +385,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)
@@ -427,10 +452,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
@@ -468,13 +493,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.53.0

Re: [PATCH 08/10] drm/i915: Use preempt_disable/enable_rt() where recommended

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The patch inadvertently removes `local_irq_save()` protection from the Xe driver's vblank timestamping critical section, allowing hard IRQs to interrupt timing-critical register reads.
- [Low] The sparse context annotations `__acquires(i915->uncore.lock)` and `__releases(i915->uncore.lock)` reference an undeclared identifier `i915`.
--

(Note: The format of this report is altered to provide a free-form summary
without directly quoting the patch code, due to automated recitation
restrictions.)

commit 8fb6242d95640a7bbab92fc65f4627e01aae6b0b
Author: Mike Galbraith <umgwanakikbuti@gmail.com>
drm/i915: Use preempt_disable/enable_rt() where recommended

This patch changes preemption disabling in the intel vblank timestamping
critical section, moving spin_lock_irqsave() into helper functions and
explicitly disabling preemption for preempt-rt configurations.

[Severity: Low]
Will this cause a build failure when running the sparse static analyzer?

The sparse annotations __acquires(i915->uncore.lock) and
__releases(i915->uncore.lock) reference the i915 identifier in
intel_vblank_section_enter_irqf() and intel_vblank_section_exit_irqf().
However, the function parameter is named display, and i915 is not locally
declared. Should this reference the uncore lock through the display parameter
instead?

[Severity: Medium]
Could this inadvertently remove IRQ protection for the Xe driver during timing
critical register reads?

Previously, i915_get_crtc_scanoutpos() explicitly called local_irq_save()
before calling the intel_vblank_section_enter() stub. With this patch, the
IRQ save is moved inside intel_vblank_section_enter_irqf().

However, the #else block implementation of intel_vblank_section_enter_irqf()
merely sets *flags = 0. This means builds where that #else block is active
(like the Xe driver) will no longer disable interrupts during this critical
section.

Might this allow hard IRQs to interrupt the sequence, creating an
artificially large time delta between stime and etime, and causing the DRM
core's max_error check to spuriously fail?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702072154.171324-1-dev@lankhorst.se?part=8

[PATCH 09/10] 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 07dca05332e5b..c0cc0a4c25dbe 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -132,7 +132,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.53.0

Re: [PATCH 09/10] drm/i915/display: Make get_vblank_counter use intel_de_read_fw()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Bypassing the DMC wakelock using `intel_de_read_fw()` in `g4x_get_vblank_counter()` causes unprotected reads of powered-down registers on modern platforms (Xe2+).

Pre-existing issues:
- [High] Tracepoints calling `intel_get_crtc_scanline()` acquire the sleepable `uncore->lock` in atomic context, leading to a PREEMPT_RT lockdep splat.
- [High] Acquiring `wl->lock` in `icl_dsi_frame_update()` while hardware interrupts are disabled causes a PREEMPT_RT lockdep splat.
--

commit a4aff240080d4b3a8f37dee585514c6912bb489d
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/i915/display: Make get_vblank_counter use intel_de_read_fw()

This commit modifies g4x_get_vblank_counter() to use intel_de_read_fw()
instead of intel_de_read() to avoid taking a sleeping lock in atomic
context, fixing a lockdep splat on PREEMPT_RT.

>     Fixes the following lockdep splat on PREEMPT_RT:
>     <3> BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
[ ... ]
>     <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.

[Severity: High]
This is a pre-existing issue, but do tracepoints calling
intel_get_crtc_scanline() still acquire the sleepable uncore->lock in
atomic context, leading to a PREEMPT_RT lockdep splat?

For example, trace_intel_pipe_update_start executes with preemption disabled:

intel_display_trace.h:TRACE_EVENT() {
    ...
    __entry->scanline = intel_get_crtc_scanline(crtc);
    ...
}

Which calls the locking function:

intel_vblank.c:intel_get_crtc_scanline() {
    ...
    intel_vblank_section_enter_irqf(display, &irqflags);
    ...
}

And acquires the lock:

intel_vblank.c:intel_vblank_section_enter_irqf() {
    ...
    spin_lock_irqsave(&uncore->lock, *flags);
    ...
}

On PREEMPT_RT, uncore->lock becomes an rt_spinlock which can sleep. Acquiring
a sleeping lock with preemption disabled causes a BUG splat and system
instability.

While i915_get_vblank_counter() was fixed in a subsequent patch in this series
(commit drm/i915/display: Do not take uncore lock in i915_get_vblank_counter),
intel_get_crtc_scanline() remains unfixed at the end of the series.

[Severity: High]
This is a pre-existing issue, but does acquiring wl->lock in
icl_dsi_frame_update() while hardware interrupts are disabled cause a
PREEMPT_RT lockdep splat?

The hardware update sequence between intel_pipe_update_start() and
intel_pipe_update_end() executes with hardware interrupts disabled via
local_irq_disable().

Within this section:

intel_crtc.c:intel_pipe_update_end() {
    ...
    icl_dsi_frame_update(new_crtc_state);
    ...
}

Which calls:

icl_dsi.c:icl_dsi_frame_update() {
    ...
    intel_de_rmw(display, DSI_CMD_FRMCTL(port), 0,
                 DSI_FRAME_UPDATE_REQUEST);
    ...
}

This attempts to acquire the DMC wakelock (wl->lock), which is a sleepable
rt_spinlock on PREEMPT_RT. Taking it while IRQs are disabled triggers a
sleeping function called from invalid context bug.

> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 07dca05332e5b..c0cc0a4c25dbe 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -132,7 +132,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));

[Severity: High]
Does bypassing the DMC wakelock here cause unprotected reads of powered-down
registers on modern platforms?

PIPE_FRMCOUNT_G4X (0x70040) is within the powered_off_ranges
(0x60000 - 0x7FFFF) defined in intel_dmc_wl.c. On modern platforms like Xe2+,
MMIO reads do not automatically wake the hardware from DC states, and the
DMC wakelock must be explicitly held. Could using intel_de_read_fw() lead
to reading garbage data (0xFFFFFFFF) or cause MMIO timeouts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702072154.171324-1-dev@lankhorst.se?part=9

Re: [PATCH 09/10] drm/i915/display: Make get_vblank_counter use intel_de_read_fw()

[Ville Syrjälä]

On Thu, Jul 02, 2026 at 09:21:52AM +0200, Maarten Lankhorst wrote:
> 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 07dca05332e5b..c0cc0a4c25dbe 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -132,7 +132,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));

Sashiko didn't complain about potential ivb/hsw same-cacheline issues
here, so I'll point it out. There isn't too much on that cacheline
though so perhaps not a huge issue (assuming the issue is real in the
first place, which I'm not 100% convinced about). But I'm still thinking
that I'll just try to convert the lock into a raw spinlock while moving
all the mmio stuff directly into intel_de.[ch]. That would avoid part
of the problem at least.

The dmc wakelock stuff I'm not sure we should even worry about here.
If the pipe isn't active we shouldn't really end up calling this anyway,
except maybe if we race with the DMC. Although we don't have register
accessors that skip the DMC stuff while still taking the uncore lock.
I'm not super keen on adding yet another register accessor variant
either, so not sure how to deal with this sort of stuff in the end...

>  }
>  
>  static u32 intel_crtc_scanlines_since_frame_timestamp(struct intel_crtc *crtc)
> -- 
> 2.53.0

-- 
Ville Syrjälä
Intel

[PATCH 10/10] drm/i915/display: Do not take uncore lock in i915_get_vblank_counter

[Maarten Lankhorst]

This fixes a lockdep splat that occurs in the code that should be run
with interrupts disabled. The uncore and DMC locks should not be taken
and released repeatedly in a timing sensitive path.

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

diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
index 1029790194296..497a308322847 100644
--- a/drivers/gpu/drm/i915/display/intel_de.h
+++ b/drivers/gpu/drm/i915/display/intel_de.h
@@ -65,6 +65,14 @@ intel_de_read64_2x32(struct intel_display *display, intel_reg_t reg)
 	return (u64)upper << 32 | lower;
 }
 
+static inline u64
+intel_de_read64_2x32_fw(struct intel_display *display,
+			i915_reg_t lower_reg, i915_reg_t upper_reg)
+{
+	return intel_uncore_read64_2x32_fw(__to_uncore(display),
+					   lower_reg, upper_reg);
+}
+
 static inline void
 intel_de_posting_read(struct intel_display *display, intel_reg_t reg)
 {
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index c0cc0a4c25dbe..5ca22899055d7 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -109,8 +109,8 @@ u32 i915_get_vblank_counter(struct drm_crtc *crtc)
 	 * we get a low value that's stable across two reads of the high
 	 * register.
 	 */
-	frame = intel_de_read64_2x32_volatile(display, PIPEFRAMEPIXEL(display, pipe),
-					      PIPEFRAME(display, pipe));
+	frame = intel_de_read64_2x32_fw(display, PIPEFRAMEPIXEL(display, pipe),
+				        PIPEFRAME(display, pipe));
 
 	pixel = frame & PIPE_PIXEL_MASK;
 	frame = (frame >> PIPE_FRAME_LOW_SHIFT) & 0xffffff;
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index fafc2ca9a2376..507398a562649 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -449,13 +449,28 @@ static inline void intel_uncore_rmw_fw(struct intel_uncore *uncore,
 		intel_uncore_write_fw(uncore, reg, val);
 }
 
+static inline u64
+intel_uncore_read64_2x32_fw(struct intel_uncore *uncore,
+			    i915_reg_t lower_reg, i915_reg_t upper_reg)
+{
+	u32 upper, lower, old_upper, loop = 0;
+	upper = intel_uncore_read_fw(uncore, upper_reg);
+	do {
+		old_upper = upper;
+		lower = intel_uncore_read_fw(uncore, lower_reg);
+		upper = intel_uncore_read_fw(uncore, upper_reg);
+	} while (upper != old_upper && loop++ < 2);
+
+	return (u64)upper << 32 | lower;
+}
+
 static inline u64
 intel_uncore_read64_2x32(struct intel_uncore *uncore,
 			 i915_reg_t lower_reg, i915_reg_t upper_reg)
 {
-	u32 upper, lower, old_upper, loop = 0;
 	enum forcewake_domains fw_domains;
 	unsigned long flags;
+	u64 ret;
 
 	fw_domains = intel_uncore_forcewake_for_reg(uncore, lower_reg,
 						    FW_REG_READ);
@@ -466,17 +481,12 @@ intel_uncore_read64_2x32(struct intel_uncore *uncore,
 	spin_lock_irqsave(&uncore->lock, flags);
 	intel_uncore_forcewake_get__locked(uncore, fw_domains);
 
-	upper = intel_uncore_read_fw(uncore, upper_reg);
-	do {
-		old_upper = upper;
-		lower = intel_uncore_read_fw(uncore, lower_reg);
-		upper = intel_uncore_read_fw(uncore, upper_reg);
-	} while (upper != old_upper && loop++ < 2);
+	ret = intel_uncore_read64_2x32_fw(uncore, lower_reg, upper_reg);
 
 	intel_uncore_forcewake_put__locked(uncore, fw_domains);
 	spin_unlock_irqrestore(&uncore->lock, flags);
 
-	return (u64)upper << 32 | lower;
+	return ret;
 }
 
 static inline int intel_uncore_write_and_verify(struct intel_uncore *uncore,
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 08d7ab9336725..764bc94044537 100644
--- a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
+++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
@@ -74,6 +74,13 @@ intel_uncore_read64_2x32(struct intel_uncore *uncore,
 	return (u64)upper << 32 | lower;
 }
 
+static inline u64
+intel_uncore_read64_2x32_fw(struct intel_uncore *uncore,
+			    i915_reg_t i915_lower_reg, i915_reg_t i915_upper_reg)
+{
+	return intel_uncore_read64_2x32(uncore, i915_lower_reg, i915_upper_reg);
+}
+
 static inline void intel_uncore_posting_read(struct intel_uncore *uncore,
 					     i915_reg_t i915_reg)
 {
-- 
2.53.0

Re: [PATCH 10/10] drm/i915/display: Do not take uncore lock in i915_get_vblank_counter

[Ville Syrjälä]

On Thu, Jul 02, 2026 at 09:21:53AM +0200, Maarten Lankhorst wrote:
> This fixes a lockdep splat that occurs in the code that should be run
> with interrupts disabled. The uncore and DMC locks should not be taken
> and released repeatedly in a timing sensitive path.
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/i915/display/intel_de.h       |  8 ++++++
>  drivers/gpu/drm/i915/display/intel_vblank.c   |  4 +--
>  drivers/gpu/drm/i915/intel_uncore.h           | 26 +++++++++++++------
>  .../drm/xe/compat-i915-headers/intel_uncore.h |  7 +++++
>  4 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
> index 1029790194296..497a308322847 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.h
> +++ b/drivers/gpu/drm/i915/display/intel_de.h
> @@ -65,6 +65,14 @@ intel_de_read64_2x32(struct intel_display *display, intel_reg_t reg)
>  	return (u64)upper << 32 | lower;
>  }
>  
> +static inline u64
> +intel_de_read64_2x32_fw(struct intel_display *display,
> +			i915_reg_t lower_reg, i915_reg_t upper_reg)
> +{
> +	return intel_uncore_read64_2x32_fw(__to_uncore(display),
> +					   lower_reg, upper_reg);
> +}
> +
>  static inline void
>  intel_de_posting_read(struct intel_display *display, intel_reg_t reg)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index c0cc0a4c25dbe..5ca22899055d7 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -109,8 +109,8 @@ u32 i915_get_vblank_counter(struct drm_crtc *crtc)
>  	 * we get a low value that's stable across two reads of the high
>  	 * register.
>  	 */
> -	frame = intel_de_read64_2x32_volatile(display, PIPEFRAMEPIXEL(display, pipe),
> -					      PIPEFRAME(display, pipe));
> +	frame = intel_de_read64_2x32_fw(display, PIPEFRAMEPIXEL(display, pipe),
> +				        PIPEFRAME(display, pipe));

This is the only user of intel_de_read64_2x32_volatile() and it
doesn't need any locking/wakelocks/etc so we should just convert
intel_de_read64_2x32_volatile() into intel_de_read64_2x32_volatile_fw().
And we should just skip the uncore dependency (so that we'll have one 
less thing to worry about later) so I'd just implement it directly in
intel_de.[ch] in terms of intel_de_read_fw().

>  
>  	pixel = frame & PIPE_PIXEL_MASK;
>  	frame = (frame >> PIPE_FRAME_LOW_SHIFT) & 0xffffff;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index fafc2ca9a2376..507398a562649 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -449,13 +449,28 @@ static inline void intel_uncore_rmw_fw(struct intel_uncore *uncore,
>  		intel_uncore_write_fw(uncore, reg, val);
>  }
>  
> +static inline u64
> +intel_uncore_read64_2x32_fw(struct intel_uncore *uncore,
> +			    i915_reg_t lower_reg, i915_reg_t upper_reg)
> +{
> +	u32 upper, lower, old_upper, loop = 0;
> +	upper = intel_uncore_read_fw(uncore, upper_reg);
> +	do {
> +		old_upper = upper;
> +		lower = intel_uncore_read_fw(uncore, lower_reg);
> +		upper = intel_uncore_read_fw(uncore, upper_reg);
> +	} while (upper != old_upper && loop++ < 2);
> +
> +	return (u64)upper << 32 | lower;
> +}
> +
>  static inline u64
>  intel_uncore_read64_2x32(struct intel_uncore *uncore,
>  			 i915_reg_t lower_reg, i915_reg_t upper_reg)
>  {
> -	u32 upper, lower, old_upper, loop = 0;
>  	enum forcewake_domains fw_domains;
>  	unsigned long flags;
> +	u64 ret;
>  
>  	fw_domains = intel_uncore_forcewake_for_reg(uncore, lower_reg,
>  						    FW_REG_READ);
> @@ -466,17 +481,12 @@ intel_uncore_read64_2x32(struct intel_uncore *uncore,
>  	spin_lock_irqsave(&uncore->lock, flags);
>  	intel_uncore_forcewake_get__locked(uncore, fw_domains);
>  
> -	upper = intel_uncore_read_fw(uncore, upper_reg);
> -	do {
> -		old_upper = upper;
> -		lower = intel_uncore_read_fw(uncore, lower_reg);
> -		upper = intel_uncore_read_fw(uncore, upper_reg);
> -	} while (upper != old_upper && loop++ < 2);
> +	ret = intel_uncore_read64_2x32_fw(uncore, lower_reg, upper_reg);
>  
>  	intel_uncore_forcewake_put__locked(uncore, fw_domains);
>  	spin_unlock_irqrestore(&uncore->lock, flags);
>  
> -	return (u64)upper << 32 | lower;
> +	return ret;
>  }
>  
>  static inline int intel_uncore_write_and_verify(struct intel_uncore *uncore,
> 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 08d7ab9336725..764bc94044537 100644
> --- a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
> +++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
> @@ -74,6 +74,13 @@ intel_uncore_read64_2x32(struct intel_uncore *uncore,
>  	return (u64)upper << 32 | lower;
>  }
>  
> +static inline u64
> +intel_uncore_read64_2x32_fw(struct intel_uncore *uncore,
> +			    i915_reg_t i915_lower_reg, i915_reg_t i915_upper_reg)
> +{
> +	return intel_uncore_read64_2x32(uncore, i915_lower_reg, i915_upper_reg);
> +}
> +
>  static inline void intel_uncore_posting_read(struct intel_uncore *uncore,
>  					     i915_reg_t i915_reg)
>  {
> -- 
> 2.53.0

-- 
Ville Syrjälä
Intel

Re: [PATCH 00/10] drm/intel/display: Changes required to make vblank evasion PREEMPT_RT safe.

[Ville Syrjälä]

On Thu, Jul 02, 2026 at 09:21:43AM +0200, Maarten Lankhorst wrote:
> CONFIG_PREEMPT_RT requires code to be as deterministic as possible,
> and codes with interrupts disabled cannot use spinlocks.
> 
> In order to handle this correctly, we ensure any locking requirements
> are handled before disabling interrupts, and only start locking again
> after interrupts are re-enabled and timing sensitive path completed.
> 
> This also has the benefit of making the vblank evasion code more
> deterministic; without locking there's much less jitter.
> 
> Previously, we scheduled vblank work on completion.
> This is now handled by scheduling the vblank in advance, only arming
> it upon completion without locks.

Sashiko found a bunch of stuff that I was also thinking would be
a problem. Don't think I'll need to repeat it here.

I'm still thinking the easy way out would be to just convert
everything to raw spinlocks. The uncore lock I think should become
easier to deal with once I manage to move all of it into the
display code. I admit that the vblank locking is kinda annoying
and potentially inefficient. I've been pondering about making the
vblank locks to per-crtc to at least avoid some unnecessary contention,
but the fact that a lot of the drivers poke at the vblank internals
directly makes it a lot more work than it should be :/

> 
> Maarten Lankhorst (9):
>   drm/vblank_work: Add methods to schedule vblank_work in 2 stages
>   drm/vblank: Add a 2-stage version of drm_crtc_arm_vblank_event
>   drm/intel/display: Make intel_crtc_arm_vblank_event static
>   drm/intel/display: Convert vblank event handling to 2-stage arming
>   drm/i915/display: Move vblank put until after critical section
>   drm/i915/display: Remove locking from intel_vblank_evade critical
>     section
>   drm/i915/display: Handle vlv dsi workaround in scanline_in_safe_range
>     too
>   drm/i915/display: Make get_vblank_counter use intel_de_read_fw()
>   drm/i915/display: Do not take uncore lock in i915_get_vblank_counter
> 
> Mike Galbraith (1):
>   drm/i915: Use preempt_disable/enable_rt() where recommended
> 
>  drivers/gpu/drm/drm_vblank.c                  |  64 +++++++++-
>  drivers/gpu/drm/drm_vblank_work.c             | 110 +++++++++++++----
>  drivers/gpu/drm/i915/display/intel_crtc.c     |  86 ++++++-------
>  drivers/gpu/drm/i915/display/intel_crtc.h     |   1 -
>  drivers/gpu/drm/i915/display/intel_cursor.c   |   8 +-
>  drivers/gpu/drm/i915/display/intel_de.h       |   8 ++
>  drivers/gpu/drm/i915/display/intel_vblank.c   | 115 ++++++++++--------
>  drivers/gpu/drm/i915/display/intel_vblank.h   |   1 +
>  drivers/gpu/drm/i915/intel_uncore.h           |  26 ++--
>  .../drm/xe/compat-i915-headers/intel_uncore.h |   7 ++
>  include/drm/drm_vblank.h                      |  14 ++-
>  include/drm/drm_vblank_work.h                 |  12 ++
>  12 files changed, 324 insertions(+), 128 deletions(-)
> 
> -- 
> 2.53.0

-- 
Ville Syrjälä
Intel