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

Mario Kleiner mario.kleiner at tuebingen.mpg.de
Fri Oct 28 11:15:11 PDT 2011

> Message: 5
> Date: Fri, 28 Oct 2011 11:30:38 +0200
> From: Daniel Vetter <daniel at ffwll.ch>
> Subject: Re: [PATCH 3/3] drm: do not sleep on vblank while holding a
> 	mutex
> To: Ilija Hadzic <ihadzic at research.bell-labs.com>
> Cc: dri-devel at lists.freedesktop.org
> Message-ID: <20111028093038.GB2919 at phenom.ffwll.local>
> Content-Type: text/plain; charset=us-ascii
>>> - Imo we also have a few too many atomic ops going on, e.g.
>>> dev->vblank_refcount looks like it's atomic only because or the  
>>> procfs
>>> file, so maybe kill that procfs file entirely.
>> I am not sure about that. dev->vblank_refcount looks critical to me:
>> it is incremented through drm_vblank_get which is called from
>> several different contexts: page-flip functions in several drivers
>> (which can run in the context of multiple user processes), **_pm_**
>> stuff in radeon driver (which looks like it runs in the context of
>> kernel worker thread). Mutexes used at all these different places
>> are not quite consistent. If anything, as the result of a later
>> audit, some other mutexes may be rendered unecessary, but
>> definitely, I would keep vblank_refcount atomic.


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've re-read the code a bit and in _get it's protected by dev- 
> >vbl_lock,
> but not so in _put (because we issue a timer call to actually  
> switch it
> off). I think we could just shove it under the protection of dev- 
> >vbl_lock
> completely. Another fuzzzy area is the relation between dev- 
> >vbl_lock and
> dev->vblank_time_lock. The latter looks a bit rendundant.

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  


Mario Kleiner
Max Planck Institute for Biological Cybernetics
Spemannstr. 38
72076 Tuebingen

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)

More information about the dri-devel mailing list