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