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