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

[Daniel Vetter <daniel@ffwll.ch>]

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.

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.

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.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48