Re: [RFC] tentative fix for drm/i915/gt regression on preempt-rt

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2023-07-03 16:30:01 [+0100], Tvrtko Ursulin wrote:
> Hi,
Hi,

> 
> Atomic requirement from that commit text is likely referring to removing the
> old big sleeping mutex we had in the reset path. So it looks plausible that
> preempt_disable() section is not strictly needed and perhaps motivation
> simply was, given those 20-50us polls on hw registers involved, to make them
> happen as fast as possible and so minimize visual glitching during resets.
> 
> Although that reasoning would only apply on some hw generations, where the
> irqsave spinlock is not held across the whole sequence anyway.
> 
> And I suspect those same platforms would be the annoying ones, if one simply
> wanted to try without the preempt_disable section, given our wait_for_atomic
> macro will complain loudly if not used from an atomic context.

It does not complain on RT, I did patch it out.
   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/tree/patches/0005-drm-i915-Don-t-check-for-atomic-context-on-PREEMPT_R.patch?h=linux-6.4.y-rt-patches

Interesting, the link there refers to an older posting but this patch is
not included. For the other I didn't receive feedback at which point I
stopped pushing and put it on the list for later…

> But I think we do have a macro for short register waits which works with
> preempting enabled. I will try and cook up a patch and submit to our CI
> during the week, then see what happens.
>
> Or even moving the preempt_disable down so it just encompasses the register
> write + wait. That would then be under the spinlock which is presumable okay
> on RT? (Yes I know it wouldn't' solve one half of your "complaint" but lets
> just entertain the idea for now.)

You can't
	preempt_disable();
	spin_lock();

You could
	spin_lock();
	preempt_disable();

But if there is no need then there is no need ;)
What I worry a bit the udelays…

Thanks!

> Regards,
> 
> Tvrtko

Sebastian