On Thu, Nov 17, 2011 at 2:54 PM, Mario Kleiner mario.kleiner@tuebingen.mpg.de wrote:
Jesse, cc'ing you in case you have thoughts about this for the intel side of things?
On Nov 17, 2011, at 4:39 PM, Andrew Lutomirski wrote:
2011/11/17 Michel Dänzer michel@daenzer.net:
[ Dropping intel-gfx list from CC, as it automatically rejects posts from non-subscribers ]
On Mit, 2011-11-16 at 21:39 -0800, Andy Lutomirski wrote:
There are two possible races when disabling vblanks. If the IRQ fired but the hardware didn't update its counter yet, then we store too low a hardware counter. (Sensible hardware never does this. Apparently not all hardware is sensible.)
The thing is, 'the' IRQ and 'the' hardware counter aren't necessarily about the same thing. We want an IRQ which triggers at the beginning of vertical blank, but the Radeon hardware counters increment at the beginning of scanout, i.e. at the end of vertical blank. Does that make the hardware 'broken' or 'not sensible'?
Perhaps not. I think that using that counter as the return value of get_vblank_counter is a bit unfortunate in that case, though. Anyway, I'm not particularly attached to the comment as written.
Am I correct in guessing that Radeon fires its vblank irq at the beginning of vblank instead of at the end And doesn't this mean that Radeon has a separate issue when vblank_get happens during the vblank interval? A trick similar to drm_calc_vbltimestamp_from_scanoutpos ought to be able to fix that.
The Radeon's i tested (R500, R600) fire the vblank irq even a few scanlines before the start of vblank. I think most gpu's fire very close to the beginning of vblank, because one wants to use the vblank irq to trigger some work that needs to be done within the vblank interval. Some Radeon's sometimes fire a spurious vblank irq at the moment vblank irq's get enabled, althought that could also be some tiny bug in the kms driver, maybe missing some acknowledge of a pending irq somewhere in the vblank off or on function. That's the reason for some of the fun inside drm_calc_vbltimestamp_from_scanoutpos() and drm_handle_vblank(), to associate a vblank irq and the vblank timestamps with the correct vblank interval, even if the irq handler executes a bit outside the vblank interval.
If, on the other hand, the counter updated but the IRQ didn't fire yet, we store too high a counter.
We handled the former case with a heuristic based on timestamps and we did not handle the latter case. By saving a little more state, we can handle both cases exactly: all we need to do is watch for changes in the difference between the hardware and software vblank counts.
I'm afraid that can't work:
Some (AFAIR also Intel) hardware resets the counter to 0 when the CRTC is disabled / enabled (e.g. for DPMS, or a modeset). That's why we ended up only counting interrupts while the IRQ is enabled, and only using the hardware counter to fill in while the IRQ is disabled. The hardware counter cannot be assumed to have any defined behaviour between enabling and disabling the IRQ.
To compensate for this, the drivers call drm_vblank_pre_modeset (which enables the IRQ, which also updates the virtual counter from the hardware counter) before disabling the CRTC and drm_vblank_post_modeset (which disables the IRQ, which also records the hardware counter) after enabling the CRTC.
Shouldn't this be fixable, though, by adding appropriate logic in drm_vblank_pre_modeset and drm_vblank_post_modeset?
Would this be a lot simpler if drm had a function (or call to the driver) to compute both the vblank count and the last vblank timestamp? That way the irq handler, drm_update_vblank_count, vblank_disable_and_save, and pre/post modeset could keep everything exactly in sync. IIRC i915 has this capability in hardware, and if Radeon indeed updates the frame count exactly at the end of the vblank interval, it could compute this exactly as well.
I think Andrews idea is very close to what i proposed in Matthews RFC e-mail thread, if we use the scanout position as a "clock", except we could get rid of the calibration loop, the part i like least about my proposal, if we know for sure when each gpu increments its hardware counter.
At least intel, radeon and nouveau (hopefully soonish - patches almost ready for submission) expose the dev->driver->get_scanout_position() hook, so we know the scanline at time of vblank counter query. If we knew at which scanline each gpu increments its counter we could simply compare against scanline at counter query time and decide if we need to add 1 to the value or not. We could make the hardware counter value look like as if it is from a hardware counter that always increments at leading edge of vblank, which i think would be consistent with what the irq driven vblank increment does.
Pseudocode (with bonus goto included!):
// To be called from vblank irq on/off routines instead of driver->get_vblank_count(crtc): uint32 get_effective_hw_vblank_count(crtc) {
retry:
uint32 scanline_pre = driver->get_scanout_position(crtc); uint32 hw_count = driver->get_vblank_count(crtc); uint32 scanline_post = driver->get_scanout_position(crtc);
if ((scanline_pre outside vblank) && (scanline_post outside vblank) { // Inside active scanout -> All counters stable -> no problem: return hw_count; }
// Inside vblank -> careful! if (scanline_where_gpu_increments_its_counter intersects interval [scanline_pre ; scanline_post]) { // Exact time of get_vblank_count() query "uncertain" wrt. hw counter increment. cpu_relax(); // or something similar... goto retry; }
// Inside vblank. Query happened before or after hw increment? if (scanline_post < scanline_where_gpu_increments_its_counter) { // Query happened before hw counter increment for sure. Fake // the increment has already happened: hw_count++; }
return hw_count; }
Something like this? The worst case busy waiting time would be maybe 2 scanline durations ( < 50 microseconds) in the very rare case where we hit exactly the scanline where hw counter increments. That amount of busy waiting sounds acceptable to me, given that we'd still keep a vblank off delay of a few milliseconds to avoid many redundant on/off transitions during a typical OpenGL bufferswap and that this never gets directly called from interrupt context.
We could put this into the drm, then the drivers would need to tell the drm the scanline where increment happens, or we could replace the driver->get_vblank_count() hook in each driver with some variant of this? Drivers with such a thing could lower the vblank off delay to a few msecs, other drivers would leave it at a high value.
I have some partly-written code to query the vblank count and scanout position together (it'll be more efficient that way, I think, as well as simpler). I'll see how it works tonight on i915.
For added fun, can we get notified when the system is about to go idle? A decent heuristic for when to turn off vblanks might be if the system goes idle with refcount == 0 or if a vblank happens that no one cares about.
--Andy