On Die, 2011-10-25 at 22:20 -0400, Ilija Hadzic wrote:
holding the drm_global_mutex in drm_wait_vblank and then going to sleep (via DRM_WAIT_ON) is a bad idea in general because it can block other processes that may issue ioctls that also grab drm_global_mutex. Blocking can occur until next vblank which is in the tens of microseconds order of magnitude.
fix it, but making drm_wait_vblank DRM_UNLOCKED call and then grabbing the mutex inside the drm_wait_vblank function and only for the duration of the critical section (i.e., from first manipulation of vblank sequence number until putting the task in the wait queue).
Does it really need drm_global_mutex at all, as opposed to e.g. dev->struct_mutex?
diff --git a/include/drm/drm_os_linux.h b/include/drm/drm_os_linux.h index 3933691..fc6aaf4 100644 --- a/include/drm/drm_os_linux.h +++ b/include/drm/drm_os_linux.h @@ -123,5 +123,30 @@ do { \ remove_wait_queue(&(queue), &entry); \ } while (0)
+#define DRM_WAIT_ON_LOCKED( ret, queue, timeout, condition ) \ +do { \
DECLARE_WAITQUEUE(entry, current); \unsigned long end = jiffies + (timeout); \add_wait_queue(&(queue), &entry); \mutex_unlock(&drm_global_mutex); \
I agree with Daniel's sentiment on this. AFAICT add_wait_queue() protects against concurrent access to the wait queue, so I think it would be better to just drop the mutex explicitly before calling DRM_WAIT_ON.