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@xxxxxxxx
Mobile: +41 (0)79 365 57 48
*********************************************************************
Mario Kleiner
Max Planck Institute for Biological Cybernetics
Spemannstr. 38
72076 Tuebingen
Germany
e-mail: mario.kleiner@xxxxxxxxxxxxxxxx
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)
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
