[PATCH 3/3] drm: do not sleep on vblank while holding a mutex

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer