drm_wait_vblank must be DRM_UNLOCKED because otherwise it will grab the drm_global_mutex and then go to sleep until the vblank event it is waiting for. That can wreck havoc in the windowing system because if one process issues this ioctl, it will block all other processes for the duration of all vblanks between the current and the one it is waiting for. In some cases it can block the entire windowing system. So, make this ioctl DRM_UNLOCKED, wrap the remaining (very short) critical section with dev->vbl_lock spinlock, and add a comment to the code explaining what we are protecting with the lock.
v2: incorporate comments received from Daniel Vetter and Michel Daenzer.
Signed-off-by: Ilija Hadzic ihadzic@research.bell-labs.com --- drivers/gpu/drm/drm_drv.c | 2 +- drivers/gpu/drm/drm_irq.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index e159f17..c990dab 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -124,7 +124,7 @@ static struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_SG_ALLOC, drm_sg_alloc_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_SG_FREE, drm_sg_free, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
- DRM_IOCTL_DEF(DRM_IOCTL_WAIT_VBLANK, drm_wait_vblank, 0), + DRM_IOCTL_DEF(DRM_IOCTL_WAIT_VBLANK, drm_wait_vblank, DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODESET_CTL, drm_modeset_ctl, 0),
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 3830e9e..c8b4da8 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1160,6 +1160,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, union drm_wait_vblank *vblwait = data; int ret = 0; unsigned int flags, seq, crtc, high_crtc; + unsigned long irqflags;
if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) return -EINVAL; @@ -1193,6 +1194,13 @@ int drm_wait_vblank(struct drm_device *dev, void *data, } seq = drm_vblank_count(dev, crtc);
+ /* the lock protects this section from itself executed in */ + /* the context of another PID, ensuring that the process that */ + /* wrote dev->last_vblank_wait is really the last process */ + /* that was here; processes waiting on different CRTCs */ + /* do not need to be interlocked, but rather than introducing */ + /* a new per-crtc lock, we reuse vbl_lock for simplicity */ + spin_lock_irqsave(&dev->vbl_lock, irqflags); switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) { case _DRM_VBLANK_RELATIVE: vblwait->request.sequence += seq; @@ -1200,12 +1208,15 @@ int drm_wait_vblank(struct drm_device *dev, void *data, case _DRM_VBLANK_ABSOLUTE: break; default: + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); ret = -EINVAL; goto done; }
- if (flags & _DRM_VBLANK_EVENT) + if (flags & _DRM_VBLANK_EVENT) { + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); return drm_queue_vblank_event(dev, crtc, vblwait, file_priv); + }
if ((flags & _DRM_VBLANK_NEXTONMISS) && (seq - vblwait->request.sequence) <= (1<<23)) { @@ -1215,6 +1226,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, DRM_DEBUG("waiting on vblank count %d, crtc %d\n", vblwait->request.sequence, crtc); dev->last_vblank_wait[crtc] = vblwait->request.sequence; + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); DRM_WAIT_ON(ret, dev->vbl_queue[crtc], 3 * DRM_HZ, (((drm_vblank_count(dev, crtc) - vblwait->request.sequence) <= (1 << 23)) ||
On Fri, Oct 28, 2011 at 05:42:23PM -0400, Ilija Hadzic wrote:
drm_wait_vblank must be DRM_UNLOCKED because otherwise it will grab the drm_global_mutex and then go to sleep until the vblank event it is waiting for. That can wreck havoc in the windowing system because if one process issues this ioctl, it will block all other processes for the duration of all vblanks between the current and the one it is waiting for. In some cases it can block the entire windowing system. So, make this ioctl DRM_UNLOCKED, wrap the remaining (very short) critical section with dev->vbl_lock spinlock, and add a comment to the code explaining what we are protecting with the lock.
v2: incorporate comments received from Daniel Vetter and Michel Daenzer.
Signed-off-by: Ilija Hadzic ihadzic@research.bell-labs.com
drivers/gpu/drm/drm_drv.c | 2 +- drivers/gpu/drm/drm_irq.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index e159f17..c990dab 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -124,7 +124,7 @@ static struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_SG_ALLOC, drm_sg_alloc_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_SG_FREE, drm_sg_free, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
- DRM_IOCTL_DEF(DRM_IOCTL_WAIT_VBLANK, drm_wait_vblank, 0),
DRM_IOCTL_DEF(DRM_IOCTL_WAIT_VBLANK, drm_wait_vblank, DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODESET_CTL, drm_modeset_ctl, 0),
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 3830e9e..c8b4da8 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1160,6 +1160,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, union drm_wait_vblank *vblwait = data; int ret = 0; unsigned int flags, seq, crtc, high_crtc;
unsigned long irqflags;
if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) return -EINVAL;
@@ -1193,6 +1194,13 @@ int drm_wait_vblank(struct drm_device *dev, void *data, } seq = drm_vblank_count(dev, crtc);
- /* the lock protects this section from itself executed in */
- /* the context of another PID, ensuring that the process that */
- /* wrote dev->last_vblank_wait is really the last process */
- /* that was here; processes waiting on different CRTCs */
- /* do not need to be interlocked, but rather than introducing */
- /* a new per-crtc lock, we reuse vbl_lock for simplicity */
Multiline comments are done with one pair of /* */, see CodingStyle
- spin_lock_irqsave(&dev->vbl_lock, irqflags); switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) { case _DRM_VBLANK_RELATIVE: vblwait->request.sequence += seq;
@@ -1200,12 +1208,15 @@ int drm_wait_vblank(struct drm_device *dev, void *data, case _DRM_VBLANK_ABSOLUTE: break; default:
spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
- if (flags & _DRM_VBLANK_EVENT)
if (flags & _DRM_VBLANK_EVENT) {
spin_unlock_irqrestore(&dev->vbl_lock, irqflags);return drm_queue_vblank_event(dev, crtc, vblwait, file_priv);
}
if ((flags & _DRM_VBLANK_NEXTONMISS) && (seq - vblwait->request.sequence) <= (1<<23)) {
@@ -1215,6 +1226,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, DRM_DEBUG("waiting on vblank count %d, crtc %d\n", vblwait->request.sequence, crtc); dev->last_vblank_wait[crtc] = vblwait->request.sequence;
- spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
Imo the only thing we need to protect here is dev->last_vblank_wait against concurrent access from drm_vblank_info in drm_info.c
But the locking there is horribly broken: It grabs dev->struct_mutex, but that protects exactly nothing, afaics: - dev->vblank_refcount is an atomic - vblank_count is protected by a the handrolled seqlock in drm_vblank_count - dev->last_vblank_wait was protected by the global lock, but is now protected by the dev->vbl_spinlock on the write side - dev->vblank_inmodeset is protected by the global lock for ums setups (locked ioctl) and by dev->mode_config.mutex for kms
And I just don't see anything else you might protect here - vblwait is just the ioctl data copied in by the drm core ioctl helpers.
So I don't quite see what you protect here with that spinlock. Imo - either make dev->last_vblank_wait into an atomic_t - or just don't care about it, locking of vblank_info is already horribly broken. In both cases you don't need the dev->vbl_lock spinlock.
I personally vote for no additional locking at all here and just drop the global lock. Or maybe I'm blind and we need this. In that case, please clue me up.
DRM_WAIT_ON(ret, dev->vbl_queue[crtc], 3 * DRM_HZ, (((drm_vblank_count(dev, crtc) - vblwait->request.sequence) <= (1 << 23)) ||
One line below there's the curios case of us rechecking dev->irq_enabled. Without any locking in place. But afaics this is only changed by the driver at setup and teardown when we are guaranteed (hopefully) that there's no open fd and hence no way to call an ioctl. So checking once at the beginning should be good enough. Whatever. -Daniel
On Sat, 29 Oct 2011, Daniel Vetter wrote:
- /* the lock protects this section from itself executed in */
- /* the context of another PID, ensuring that the process that */
- /* wrote dev->last_vblank_wait is really the last process */
- /* that was here; processes waiting on different CRTCs */
- /* do not need to be interlocked, but rather than introducing */
- /* a new per-crtc lock, we reuse vbl_lock for simplicity */
Multiline comments are done with one pair of /* */, see CodingStyle
Will fix the multiline comments (though scripts/checkpatch.pl didn't complain so I figured it was OK)
I personally vote for no additional locking at all here and just drop the global lock. Or maybe I'm blind and we need this. In that case, please clue me up.
What I am doing with the lock, is makeing sure that the last vblank wait reported is really last. You said in your previous comment (which I agree with) that the value of having last_vblank_wait in debugfs is in case of hangs. In that case you want to see who really was the last one to issue the vblank.
Now suppose that the drm_wait_vblank is enteret in the context of two different PIDs and suppose that there are no locks. Let's say that the first process wants to wait on vblank N and the second wants to wait on vblank N+1. First process can reach the point just before it wants to write to last_vblank_wait and lose the processor there (so it has N in vblank.request (on its stack) calculated, but it has not written it into last_vblank_wait yet (because it lost the CPU right there). Then the second process gets the CPU, and executes and let's say that it goes all the way through so it writes N+1 into last_vblank_wait and then schedules away (it called DRM_WAIT_ON). Now the first process gets the CPU where it left off and overwrites N+1 in last_vblank_wait with N, which is now stale.
I explained in the comment (and in my note this morning), that the only race condition is against itself in the context of different processes. The above is the scenario.
Race against drm_vblank_info exists in theory, but in practice doesn't matter because the reader of debugfs (or proc file system) runs asynchronously (and typically at slower pace) from processes issuing vblank waits (if it's a human looking at the filesystem then definitely much slower pace). Since processes issuing vblanks are overwriting the same last_vblank_value, drm_vblank_info is already doing a form of downsampling and debugfs is losing information, anyway, so who cares about the lock. In case of a hang, the value of last_vblank_wait doesn't change, so again the lock in drm_vblank_info doesn't change anything. So adding a lock there (which would have to be vbl_lock to make it usable) is only a matter of theoretical correctness, but no practical value. So I decided not to touch drm_vblank_info.
drm_wait_vblank, however, needs a protection from itself as I explained above.
DRM_WAIT_ON(ret, dev->vbl_queue[crtc], 3 * DRM_HZ, (((drm_vblank_count(dev, crtc) - vblwait->request.sequence) <= (1 << 23)) ||
One line below there's the curios case of us rechecking dev->irq_enabled. Without any locking in place. But afaics this is only changed by the driver at setup and teardown when we are guaranteed (hopefully) that there's no open fd and hence no way to call an ioctl. So checking once at the beginning should be good enough. Whatever.
It's probably redundant statement because it won't be reached if irq_enabled is false and there is nothing to change it to true at runtime, but that's a topic for a different patch.
-- Ilija
On Fri, Oct 28, 2011 at 09:44:54PM -0500, Ilija Hadzic wrote:
Now suppose that the drm_wait_vblank is enteret in the context of two different PIDs and suppose that there are no locks. Let's say that the first process wants to wait on vblank N and the second wants to wait on vblank N+1. First process can reach the point just before it wants to write to last_vblank_wait and lose the processor there (so it has N in vblank.request (on its stack) calculated, but it has not written it into last_vblank_wait yet (because it lost the CPU right there). Then the second process gets the CPU, and executes and let's say that it goes all the way through so it writes N+1 into last_vblank_wait and then schedules away (it called DRM_WAIT_ON). Now the first process gets the CPU where it left off and overwrites N+1 in last_vblank_wait with N, which is now stale.
Ok, and here's why your locking (or any locking that drops the lock before calling schedule) won't work: Dropping the lock allows more than one process waiting for a vblank on the same crtc (the hole point of this exerices). So let's say N processes are waiting for vblank on crtc A, but we only have one storage place for last_vblank_wait. So by necessity, it's gonna be stale for N-1 processes. And because the scheduler is free to take away the cpu between the unlock and the schedule, you can freely pick for which process it's gonna be the right one, there's zero ordering guarantees possible.
By throwing in a modeset call that disables vblanks it's pretty easy to come up with a process that waits for vblank x+1 (when x is the last vblank before the vblank counter got stopped before the modeset) and last_vblank_wait == x.
Which is why I think adding decent tracepoints would be much better, but that's a different patch.
[snip]
Agreed on not locking the debug file and postponing other cleanups, that's all material for different patches.
Yours, Daniel
On Sat, 29 Oct 2011, Daniel Vetter wrote:
Ok, and here's why your locking (or any locking that drops the lock before calling schedule) won't work: [SNIP]
You just came full circle. Recall that in my v1 patch I went all the way to enqueuing the process in the wait queue before dropping the lock. That would have guaranteed that if there is a hangup, what last_vblank_wait says is the last is really the last. If there is no hang, then it doesn't matter because last_vlank_wait is constantly overwritten (and is indeed stale for N-1 processes). However, that was disliked in the review and I didn't want to argue.
So in the interest of making progress, it looks that you would be happy if this patch just dropped DRM_UNLOCKED and declared that we don't care about (potentially theoretical) critical section related to last_vblank_wait.
If that's the case, I'll cut a relatvely trivial patch that drops DRM_UNLOCKED from this system call to solve the problem that I pointed earlier in this thread and leave all the rest of the locking discussion for other patches.
Would that be fine by you ?
thanks,
Ilija
On Sat, Oct 29, 2011 at 06:59:42AM -0500, Ilija Hadzic wrote:
On Sat, 29 Oct 2011, Daniel Vetter wrote:
Ok, and here's why your locking (or any locking that drops the lock before calling schedule) won't work: [SNIP]
You just came full circle. Recall that in my v1 patch I went all the way to enqueuing the process in the wait queue before dropping the lock. That would have guaranteed that if there is a hangup, what last_vblank_wait says is the last is really the last. If there is no hang, then it doesn't matter because last_vlank_wait is constantly overwritten (and is indeed stale for N-1 processes). However, that was disliked in the review and I didn't want to argue.
Not quite full circle. Holding the lock until you're on the waitqueue doesn't really change anything - we wake up everybody on every vblank anyway. The thing I was nitpicking over was that vblank_last_wait isn't really protected on the read side, but as you've pointed out, that's not a big problem. Essentially the only thing about vblank_last_wait that we can guarantee is that somewhen somebody tried to wait on this vblank. Nothing else is possible (and the reason why I think we should ditch this all and replace it we some decent tracing - material for a differen patch though).
So in the interest of making progress, it looks that you would be happy if this patch just dropped DRM_UNLOCKED and declared that we don't care about (potentially theoretical) critical section related to last_vblank_wait.
If that's the case, I'll cut a relatvely trivial patch that drops DRM_UNLOCKED from this system call to solve the problem that I pointed earlier in this thread and leave all the rest of the locking discussion for other patches.
Would that be fine by you ?
Yeah.
Cheers, Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Ilija Hadzic