Message: 5 Date: Fri, 28 Oct 2011 11:30:38 +0200 From: Daniel Vetter daniel@ffwll.ch Subject: Re: [PATCH 3/3] drm: do not sleep on vblank while holding a mutex To: Ilija Hadzic ihadzic@research.bell-labs.com Cc: dri-devel@lists.freedesktop.org Message-ID: 20111028093038.GB2919@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@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)