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

[Mario Kleiner]

> 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.
>

Hi,

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  
processing.

thanks,
-mario


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