<div dir="ltr"><div dir="ltr">On Tue, Dec 14, 2021 at 3:03 PM Sebastian Andrzej Siewior <<a href="mailto:bigeasy@linutronix.de">bigeasy@linutronix.de</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Mike Galbraith <<a href="mailto:umgwanakikbuti@gmail.com" target="_blank">umgwanakikbuti@gmail.com</a>><br>
<br>
Mario Kleiner suggest in commit<br>
ad3543ede630f ("drm/intel: Push get_scanout_position() timestamping into kms driver.")<br>
<br>
a spots where preemption should be disabled on PREEMPT_RT. The<br>
difference is that on PREEMPT_RT the intel_uncore::lock disables neither<br>
preemption nor interrupts and so region remains preemptible.<br>
<br></blockquote><div> </div><div>Hi, first thank you for implementing these preempt disables according to the markers i left long ago. And sorry for the rather late reply.<br></div><div><br></div><div>I had a look at the code, as of Linux 5.16, and did also a little test run (of a standard kernel, not with PREEMPT_RT, only CONFIG_PREEMPT_VOLUNTARY=y) on my Intel Kabylake GT2, so some thoughts:<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
The area covers only register reads and writes. The part that worries me<br>
is:<br>
- __intel_get_crtc_scanline() the worst case is 100us if no match is<br>
found.<br></blockquote><div><br></div><div>This one can be a problem indeed on (maybe all?) modern Intel gpu's since Haswell, ie. the last ~10 years. I was able to reproduce it on my Kabylake Intel gpu.</div><div><br></div><div>Most of the time that for-loop with up to 100 repetitions (~ 100 udelay(1) + one mmio register read) (cfe. <a href="https://elixir.bootlin.com/linux/v5.17-rc1/source/drivers/gpu/drm/i915/i915_irq.c#L856">https://elixir.bootlin.com/linux/v5.17-rc1/source/drivers/gpu/drm/i915/i915_irq.c#L856</a>) will not execute, because most of the time that function gets called from the vblank irq handler and then that trigger condition (if (HAS_DDI(dev_priv) && !position)) is not true. However, it also gets called as part of power-saving on behalf of userspace context, whenever the desktop graphics goes idle for two video refresh cycles. If the desktop shows graphics activity again, and vblank interrupts need to get reenabled, the probability of hitting that case is then ~1-4% depending on video mode. How many loops it runs also varies.<br></div><div><br></div><div>On my little Intel(R) Core(TM) i5-8250U CPU machine with a mostly idle desktop, I observed about one hit every couple of seconds of regular use, and each hit took between 125 usecs and almost 250 usecs. I guess udelay(1) can take a bit longer than 1 usec?</div><div><br></div><div>So that's too much for preempt-rt. What one could do is the following:</div><div><br></div><div>1. In the for-loop in __intel_get_crtc_scanline(), add a preempt_enable() before the udelay(1); and a preempt_disable() again after it. Or potentially around the whole for-loop if the overhead of preempt_en/disable() is significant?</div><div><br></div><div>2. In intel_get_crtc_scanline() also wrap the call to __intel_get_crtc_scanline() into a preempt_disable() and preempt_enable(), so we can be sure that __intel_get_crtc_scanline() always gets called with preemption disabled.</div><div><br></div><div>Why should this work ok'ish? The point of the original preempt disable inside <span class="gmail-nf"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/i915_get_crtc_scanoutpos">i915_get_crtc_scanoutpos</a></span> is that those two *stime = ktime_get() and *etime = ktime_get() clock queries happen as close to the scanout position query as possible to get a small confidence interval for when exactly the scanoutpos was read/determined from the display hardware. error = (etime - stime) is the error margin. If that margin becomes greater than 20 usecs, then the higher-level code will consider the measurement invalid and repeat the whole procedure up to 3 times before giving up.</div><div><br></div><div>Normally, in my experience with different graphics chips, one would observe error < 3 usecs, so the measurement almost always succeeds at first try, only very rarely takes two attempts. The preempt disable is meant to make sure that this stays the case on a PREEMPT_RT kernel.<br></div><div><br></div><div>The problem here are the relatively rare cases where we hit that up to 100 iterations for-loop. Here even on a regular kernel, due to hardware quirks, we already exceed the 20 usecs tolerance by a huge amount of more than 100 usecs, leading to a retry of the measurement. And my tests showed that often the two succeeding retries also fail, because of hardware quirks can apparently create a blackout situation approaching 1 msec, so we lose anyway, regardless if we get preempted on a RT kernel or not. That's why enabling preemption on RT again during that for-loop should not make the situation worse and at least keep RT as real-time as intended.</div><div><br></div><div>In practice I would also expect that this failure case is the one least likely to impair userspace applications greatly in practice. The cases that mostly matter are the ones executed during vblank hardware irq, where the for-loop never executes and error margin and preempt off time is only about 1 usec. My own software which depends on very precise timestamps from the mechanism never reported >> 20 usecs errors during startup tests or runtime tests.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- intel_crtc_scanlines_since_frame_timestamp() not sure how long this<br>
may take in the worst case.<br>
<br></blockquote><div> </div><div>intel_crtc_scanlines_since_frame_timestamp() should be harmless. That do-while loop just tries to make sure that two register reads that should happen within the same video refresh cycle are happening in the same refresh cycle. As such, the while loop will almost always just execute only once, and at most two times, so that's at most 6 mmio register reads for two loop iterations.</div><div><br></div><div>In the long run one could try to test if <span class="gmail-n"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/__intel_get_crtc_scanline_from_timestamp">__intel_get_crtc_scanline_from_timestamp</a>() wouldn't be the better choice for affected hardware always. Code and register descriptions suggest the feature is supported by all potentially affected hardware, so if it would turn out that that method works as accurate and reliable as the old one, we could save the overhead and time delays for that 100 for-loop iterations and make the whole timestamping more reliable on modern hw.<br></span></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
It was in the RT queue for a while and nobody complained.<br>
Disable preemption on PREEPMPT_RT during timestamping.<br>
<br></blockquote><div><br></div><div>Do other patches exist to implement the preempt_dis/enable() also for AMD and NVidia / nouveau / vc4? I left corresponding markers for radeon/amdgpu-kms and RaspberryPi's vc4 driver. Ideally all kms drivers which use scanout position queries should have such code. Always a preempt_disable() before the "if (stime) *stime = ktime_get();" statement, and a preempt_enable() after the "if (etime) *etime = ktime_get();" statement.</div><div><br></div><div>Checking Linux 5.16 code, this should be safe - short preempt off interval with only a few mmio register reads - for all kms drivers that support it atm. I found the following functions to modify:</div><div><br></div><div>amdgpu: amdgpu_display_get_crtc_scanoutpos()<br>radeon: radeon_get_crtc_scanoutpos()<br>msm: mdp5_crtc_get_scanout_position() and dpu_crtc_get_scanout_position()<br>ltdc: ltdc_crtc_get_scanout_position()<br>vc4: vc4_crtc_get_scanout_position()<br></div><div><br></div><div>nouveau: In nvkm_head_mthd_scanoutpos(), one needs to put the preempt_disable() right before</div><div><pre><span class="gmail-n"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/args">args</a></span><span class="gmail-o">-></span><span class="gmail-n"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/v0">v0</a></span><span class="gmail-p">.</span><span class="gmail-n"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/time">time</a></span><span class="gmail-p">[</span><span class="gmail-mi">0</span><span class="gmail-p">]</span> <span class="gmail-o">=</span> <span class="gmail-n"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/ktime_to_ns">ktime_to_ns</a></span><span class="gmail-p">(</span><span class="gmail-n"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/ktime_get">ktime_get</a></span><span class="gmail-p">());<br></span></pre><pre><span class="gmail-p">and the preempt_enable() right after<br><span class="gmail-n"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/args">args</a></span><span class="gmail-o">-></span><span class="gmail-n"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/v0">v0</a></span><span class="gmail-p">.</span><span class="gmail-n"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/time">time</a></span><span class="gmail-p">[</span><span class="gmail-mi">1</span><span class="gmail-p">]</span> <span class="gmail-o">=</span> <span class="gmail-n"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/ktime_to_ns">ktime_to_ns</a></span><span class="gmail-p">(</span><span class="gmail-n"><a href="https://elixir.bootlin.com/linux/v5.17-rc1/C/ident/ktime_get">ktime_get</a></span><span class="gmail-p">());<br></span></span></pre></div><div><br></div><div>Is the plan to integrate these patches into the mainline kernel soon, as part of ongoing preempt-rt upstreaming?<br></div><div><br></div><div>thanks,</div><div>-mario<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[bigeasy: patch description.]<br>
<br>
Cc: Mario Kleiner <<a href="mailto:mario.kleiner.de@gmail.com" target="_blank">mario.kleiner.de@gmail.com</a>><br>
Signed-off-by: Mike Galbraith <<a href="mailto:umgwanakikbuti@gmail.com" target="_blank">umgwanakikbuti@gmail.com</a>><br>
Signed-off-by: Thomas Gleixner <<a href="mailto:tglx@linutronix.de" target="_blank">tglx@linutronix.de</a>><br>
Signed-off-by: Sebastian Andrzej Siewior <<a href="mailto:bigeasy@linutronix.de" target="_blank">bigeasy@linutronix.de</a>><br>
---<br>
drivers/gpu/drm/i915/i915_irq.c | 6 ++++--<br>
1 file changed, 4 insertions(+), 2 deletions(-)<br>
<br>
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c<br>
index 038a9ec563c10..8e9ff0bcbc7e4 100644<br>
--- a/drivers/gpu/drm/i915/i915_irq.c<br>
+++ b/drivers/gpu/drm/i915/i915_irq.c<br>
@@ -916,7 +916,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,<br>
*/<br>
spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);<br>
<br>
- /* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */<br>
+ if (IS_ENABLED(CONFIG_PREEMPT_RT))<br>
+ preempt_disable();<br>
<br>
/* Get optional system timestamp before query. */<br>
if (stime)<br>
@@ -980,7 +981,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,<br>
if (etime)<br>
*etime = ktime_get();<br>
<br>
- /* preempt_enable_rt() should go right here in PREEMPT_RT patchset. */<br>
+ if (IS_ENABLED(CONFIG_PREEMPT_RT))<br>
+ preempt_enable();<br>
<br>
spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);<br>
<br>
-- <br>
2.34.1<br>
<br>
</blockquote></div></div>