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
