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

[Tvrtko Ursulin]

On 13/06/2024 11:20, Sebastian Andrzej Siewior wrote:
> The !in_atomic() check in _wait_for_atomic() triggers on PREEMPT_RT
> because the uncore::lock is a spinlock_t and does not disable
> preemption or interrupts.
> 
> Changing the uncore:lock to a raw_spinlock_t doubles the worst case
> latency on an otherwise idle testbox during testing. Therefore I'm
> currently unsure about changing this.
> 
> Link: https://lore.kernel.org/all/20211006164628.s2mtsdd2jdbfyf7g@linutronix.de/
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy at linutronix.de>
> ---
>   drivers/gpu/drm/i915/i915_utils.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index 06ec6ceb61d57..2ca54bc235925 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -274,7 +274,7 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
>   #define wait_for(COND, MS)		_wait_for((COND), (MS) * 1000, 10, 1000)
>   
>   /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
> -#if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
> +#if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT) && !defined(CONFIG_PREEMPT_RT)
>   # define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
>   #else
>   # define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)

I think this could be okay-ish in principle, but the commit text is not 
entirely accurate because there is no direct coupling between the wait 
helpers and the uncore lock. They can be used from any atomic context.

Okay-ish in principle because there is sufficient testing in Intel's CI 
on non-PREEMPT_RT kernels to catch any conceptual misuses.

But see also the caller in skl_pcode_request. It is a bit harder to hit 
since it is the fallback path. Or gen5_rps_enable which nests under a 
different lock.

Hmm would there be a different helper, or combination of helpers, which 
could replace in_atomic() which would do the right thing on both 
kernels? Something to tell us we are neither under a spin_lock, nor 
preempt_disable, nor interrupts disabled, nor bottom-half. On either 
stock or PREEMPT_RT.

WARN_ON_ONCE((ATOMIC) && !(!preemptible() || in_hardirq() || 
in_serving_softirq())

Would this work?

Regards,

Tvrtko