| Message ID | 506B2834.7060601@tuebingen.mpg.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 2, 2012 at 7:45 PM, Mario Kleiner <mario.kleiner@tuebingen.mpg.de> wrote: > > I'm fine with removing the hack and fixing this properly, especially if you > say that it didn't work realiably in some cases. But i hope this means that > timestamping sanity tests via flip_test are a part of your regular QA > testing before you release a new kms driver, so bugs get caught pre-release? Running i-g-t tests is part of our nightly testrun. If you add in the machines Chris&me have we pretty much cover every platform ever shipped by Intel ;-) > Otherwise many scientists would get pretty nervous when using Intel gpus in > the future. ... so I hope your scientists won't get nervous ;-) I still have a few things I need to add to the flip test though: - interactions with wait_vblank (both the blocking version and the event version) - checking whether the timestamps are accurately space (since we can at least read out the line counter, we should be far accurater that 1% there). - if you have any ideas for further test, their' highly welcome. One thing we unfortunately can't really test is whether the timestamp is really when the hw starts scanning out the first line of the new frame. But extremely low jitter and whether we pick the right frame (or accidentally overcorrect by an entire frame like this code sometimes managed to do) should able be covered. Eventually I want to also get rid of the delayed vblank disabling and make the frame counter readout race-free (we probably need to move a bit of drm core logic into the driver for that). Again, I'll only merge patches once I'm happy with the evilness of the tests ... > Btw. if you remove the hack in the followup patch, you can also remove the > do_gettimeofday(&tnow). Indeed, thanks for spotting this. I'll resend v2 of this patch. Yours, Daniel
On 02.10.12 20:08, Daniel Vetter wrote: > On Tue, Oct 2, 2012 at 7:45 PM, Mario Kleiner > <mario.kleiner@tuebingen.mpg.de> wrote: >> >> I'm fine with removing the hack and fixing this properly, especially if you >> say that it didn't work realiably in some cases. But i hope this means that >> timestamping sanity tests via flip_test are a part of your regular QA >> testing before you release a new kms driver, so bugs get caught pre-release? > > Running i-g-t tests is part of our nightly testrun. If you add in the > machines Chris&me have we pretty much cover every platform ever > shipped by Intel ;-) > Cool, that makes me happy :) >> Otherwise many scientists would get pretty nervous when using Intel gpus in >> the future. > > ... so I hope your scientists won't get nervous ;-) I still have a few > things I need to add to the flip test though: I'll have a closer look at flip_test in the near future and see if i have some ideas. Psychtoolbox has many built-in tests for timestamp consistency. I'll have a look how it compares to flip_test and if some of the simple tests could be transplanted. Many of the more complex offline tests are unfortunately in Octave scripting language and require lots of human intervention, interpretation and special test equipment, so not really suitable for quick QA testing. > - interactions with wait_vblank (both the blocking version and the > event version) > - checking whether the timestamps are accurately space (since we can > at least read out the line counter, we should be far accurater that 1% > there). My testing on a Mini Atom Netbook with Intel GMA-950 in 2010 confirmed that. The OML_sync_control timestamps were off by less than 0.2 msecs wrt. photo-diode measurements on a fast CRT monitor and jitter in the low microsecond range < 1 scanline duration. Basically perfect within measurement accuracy of the external equipment. Here's a poster from 2010, for a vision scientist audience, if you are interested in the procedure i used and some results: https://github.com/kleinerm/Psychtoolbox-3/blob/master/Psychtoolbox/PsychDocumentation/ECVP2010Poster_VisualTimingPrecision.pdf > - if you have any ideas for further test, their' highly welcome. One > thing we unfortunately can't really test is whether the timestamp is > really when the hw starts scanning out the first line of the new > frame. But extremely low jitter and whether we pick the right frame > (or accidentally overcorrect by an entire frame like this code > sometimes managed to do) should able be covered. > See above: I have the equipment and methods to test that, but unfortunately difficulty getting my hands on Intel gpus in our lab, but at least during testing at the end of 2010, when i could borrow a few, the kms timestamping was basically perfect. > Eventually I want to also get rid of the delayed vblank disabling and > make the frame counter readout race-free (we probably need to move a > bit of drm core logic into the driver for that). Again, I'll only > merge patches once I'm happy with the evilness of the tests ... > Happy to review that once you get at it. I spent a lot of time staring at that code after Matthew Garret proposed some patches for early vblank disabling a while ago. If i'm not mistaken, the vblank disable/enable code in the core should be almost race-free. If you could guarantee that the driver->get_vblank_count() function returns a count "as if" the hw counter increments always at a fixed point in the scanout, identical for all kms drivers, e.g., defined to increment always at the 1st or last scanline of vblank, it should be race free. The enable/disable code in the core makes the implicit assumption that the hw counter increments either at beginning or end of vblank, can't remember atm. which one it was. Maybe there are better ways on intel hardware, but my sketchy idea was to do something like read scanline, read hw vblank counter, read scanline, do some comparisons and find out if either +1 needs to be added to the returned counter value to fudge it, or if the query needs to be repeated once in very rare cases. It worked at least on paper. I should really stop making notes only on scrap paper ;-) -- maybe i can find it somewhere in my paper pile. >> Btw. if you remove the hack in the followup patch, you can also remove the >> do_gettimeofday(&tnow). > > Indeed, thanks for spotting this. I'll resend v2 of this patch. > Fwiw, you have my Reviewed-by: mario.kleiner@tuebingen.mpg.de thanks, -mario
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 57c1309..87f825c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6203,25 +6203,6 @@ static void do_intel_finish_page_flip(struct drm_device *dev, e = work->event; e->event.sequence = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl); - /* Called before vblank count and timestamps have - * been updated for the vblank interval of flip - * completion? Need to increment vblank count and - * add one videorefresh duration to returned timestamp - * to account for this. We assume this happened if we - * get called over 0.9 frame durations after the last - * timestamped vblank. - * - * This calculation can not be used with vrefresh rates - * below 5Hz (10Hz to be on the safe side) without - * promoting to 64 integers. - */ - if (10 * (timeval_to_ns(&tnow) - timeval_to_ns(&tvbl)) > - 9 * crtc->framedur_ns) { - e->event.sequence++; - tvbl = ns_to_timeval(timeval_to_ns(&tvbl) + - crtc->framedur_ns); - } - e->event.tv_sec = tvbl.tv_sec;