Message: 5
Date: Fri, 28 Oct 2011 11:30:38 +0200
From: Daniel Vetter <daniel@xxxxxxxx>
Subject: Re: [PATCH 3/3] drm: do not sleep on vblank while holding a
mutex
To: Ilija Hadzic <ihadzic@xxxxxxxxxxxxxxxxxxxxxx>
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxxx
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)
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
