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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removal of atomic context protection (interrupt and preemption disabling) during time-critical vblank register reads for the Xe driver on non-RT kernels.
--

commit 4b863f1335422d07b094a5072752ea5c148f7cbc
Author: Mike Galbraith <umgwanakikbuti@gmail.com>

drm/i915: Use preempt_disable/enable_rt() where recommended

This commit replaces unconditional local_irq_save() calls with a new
intel_vblank_section_enter_irqf() helper and adds conditional
preempt_disable() for PREEMPT_RT during vblank timestamping.

> 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
[ ... ]
> @@ -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();

[Severity: High]
Does this leave the timing-critical vblank register reads completely
unprotected on non-RT kernels when the #else path is taken?

Previously, local_irq_save() was unconditionally called to protect the
timestamping delta. With this change, the #else implementation of
intel_vblank_section_enter_irqf() is a no-op. Because preempt_disable()
is now strictly guarded by CONFIG_PREEMPT_RT, a standard non-RT kernel
taking this path executes with both interrupts and preemption enabled.

Could this allow random latency spikes during context switches or
interrupts, artificially inflating the delta between stime and etime
and resulting in timestamping failures?

>  
>  	/* Get optional system timestamp before query. */
>  	if (stime)
[ ... ]
> @@ -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);

[Severity: High]
Similarly, does this leave __intel_get_crtc_scanline() entirely unprotected
from interrupts and preemption when the #else path is used on non-RT kernels?

>  
>  	return position;
>  }

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