On Oct 28, 2011, at 9:15 PM, Daniel Vetter wrote:
On Fri, Oct 28, 2011 at 08:15:11PM +0200, Mario Kleiner wrote:
be careful with vblank_refcount. I think it probably should stay atomic. The drm_vblank_put() is often used in interrupt handlers of the kms drivers where you don't want to uneccessary wait on a lock, but be as quick as possible.
I don't think that's a real issue - if we have lock contention anywhere with the current code, we already have a problem - and it looks like we have ;-) And the spinlocks are all already switching off irqs locally, so we should already strive for the shortest possible lock hold times.
[snip]
The vblank_time_lock serves a different purpose from vbl_lock. It is used in the vblank irq handler drm_handle_vblank() and in the vblank irq on/off functions to protect the vblank timestamping and counting against races between the irq handler and the on/off code, and some funny races between the cpu reinitializing vblank counts and timestamps in non-irq path and the gpu firing vblank interrupts and/ or updating its hardware vblank counter at the "wrong" moment. vblank_time_lock basically blocks/delays gpu vblank irq processing during such problematic periods. Timing there is criticial for reliable vblank timestamping, kms page flipping and artifact free dynamic gpu power-management. It should be locked as seldomly and held as short as possible. I initially tried to get away only with vbl_lock, but there were uses of vbl_lock that looked as if using it in the interrupt handler as well could cause long delays in irq processing.
I've gone through the code, and in almost all case where the vbl_lock is hold for a bit more than just a few load and stores we are immediately taking the vblank_time_lock and hold it almost as long. Which is why I think this is redundant. The only exeption is the irq uninstall code, which isn't really a fast path anyway.
Ok, if that is the case in the current code, fair enough. We would have to stress the importance of *very low* vbl_lock hold times in the documentation of the code if we want to get rid of the vblank_time_lock. The distinctive lock for that purpose is also meant as a marker that you should try twice as hard to be quick while you hold that lock than with other spinlocks. A few dozen usecs extra delay there could make a difference between met and missed pageflip deadlines and well working or not well working dynamic gpu reclocking, e.g., on radeon, where this stuff is triggered by vblank irq's and quite timing sensitive.
Also, all these paths with longer lock-hold times are in the initial vblank_get/final vblank_put. And because we delay the vblank disabling by a full second, we should never hit that in steady state.
5 seconds by default, i think. We wanted to lower that to something like < 100 msecs at some point, but there's still some work to do to remove some potential race with the gpu in the on/off path which could cause lost/wrong vblank counts, and the nouveau kms driver doesn't implement hardware vblank counter at all atm, which makes small values on nouveau unsafe atm. With higher on/off frequencies the impact of too long lock hold times would become worse.
But I think you're one of the most timing-sensitive users of this vblank stuff, so when I get around to clean this up I'll certainly put you on cc so you can test for any adverse effects. Hopefully ripping out unecessary atomic ops makes stuff faster, not slower.
Yes, the current timing/timestamp precision and some of the related dri2 features of the free graphics stack is a very strong selling point to lure quite many timing sensitive users of my userspace toolkit away from other proprietary solutions.
Ok, cc'ing me sounds like a plan.
Thanks, -mario
-Daniel
Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48
********************************************************************* Mario Kleiner Max Planck Institute for Biological Cybernetics Spemannstr. 38 72076 Tuebingen Germany
e-mail: mario.kleiner@tuebingen.mpg.de office: +49 (0)7071/601-1623 fax: +49 (0)7071/601-616 www: http://www.kyb.tuebingen.mpg.de/~kleinerm ********************************************************************* "For a successful technology, reality must take precedence over public relations, for Nature cannot be fooled." (Richard Feynman)