On 17/06/2024 11:07, Sebastian Andrzej Siewior wrote:
On 2024-06-14 13:19:25 [+0100], Tvrtko Ursulin wrote:
So the question is why do you need to know if the context is atomic?
The only impact is avoiding disabling preemption. Is it that important
to avoid it?
If so would cant_migrate() work? It requires CONFIG_DEBUG_ATOMIC_SLEEP=y
to do the trick.
... catching misuse of atomic wait helpers step 2 - are you calling it from
a non-atomic context without the real need. So should use the non-atomic
helper instead.
When i915 development was very active and with a lot of contributors it was
beneficial to catch these things which code review would easily miss.
Now that the pace is much, much slower, it is probably not very important.
So this patch is acceptable for what I am concerned and:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
Actually please also add the PREEMPT_RT angle to the comment above
_WAIT_FOR_ATOMIC_CHECK. Sometimes lines change and git blame makes it hard
to find the commit text.
Do you want me the repost the series? Are the bots happy enough?
I did a re-test but am not 100% certain yet. CI looks frustratingly
noisy at the moment.
igt@debugfs_test@read_all_entries appears to be a fluke which is not new.
But igt@gem_exec_parallel@engines@basic from the latest run seem new.
So I queued another re-test.
I have the following as far this patch:
------->8--------------
The !in_atomic() check in _wait_for_atomic() triggers on PREEMPT_RT
because the uncore::lock is a spinlock_t and does not disable
preemption or interrupts.
Changing the uncore:lock to a raw_spinlock_t doubles the worst case
latency on an otherwise idle testbox during testing.
Ignore _WAIT_FOR_ATOMIC_CHECK() on PREEMPT_RT.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
Link: https://lore.kernel.org/all/20211006164628.s2mtsdd2jdbfyf7g@xxxxxxxxxxxxx/
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_utils.h | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 06ec6ceb61d57..f0d3c5cdc1b1b 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -273,8 +273,13 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
(Wmax))
#define wait_for(COND, MS) _wait_for((COND), (MS) * 1000, 10, 1000)
-/* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
-#if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
+/*
+ * If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false.
+ * On PREEMPT_RT the context isn't becoming atomic because it is used in an
+ * interrupt handler or because a spinlock_t is acquired. This leads warnings
+ * which don't occur otherwise and is therefore disabled.
Ack, thanks!
Regards,
Tvrtko
+ */
+#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)
Regards,
Tvrtko
Sebastian
