[PATCH 3/3] drm: do not sleep on vblank while holding a mutex

[Mario Kleiner]

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 at 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 at 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)