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

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

[Ilija Hadzic]

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.

v2: incorporate comments received from Daniel Vetter and
    Michel Daenzer.

v3: after a lengty discussion with Daniel Vetter, it was concluded
    we should not worry about any locking, within drm_wait_vblank
    function so this patch becomes a rather trivial removal
    of drm_global_mutex from drm_wait_vblank

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_drv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index dbabcb0..dc0eb0b 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),
 
-- 
1.7.7

Re: [PATCH v3] drm: do not sleep on vblank while holding a mutex

[Daniel Vetter]

On Mon, Oct 31, 2011 at 01:10:21PM -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.
> 
> v2: incorporate comments received from Daniel Vetter and
>     Michel Daenzer.
> 
> v3: after a lengty discussion with Daniel Vetter, it was concluded
>     we should not worry about any locking, within drm_wait_vblank
>     function so this patch becomes a rather trivial removal
>     of drm_global_mutex from drm_wait_vblank

That commit message is a bit garbage. What about ...

"... it was concluded that the only think not yet protected with locks and
atomic ops is the write to dev->last_vblank_wait. It's only used in a
debug file in proc, and the current code already employs no correct
locking: the proc file only takes dev->struct_mutex, whereas
drm_wait_vblank implicitly took the drm_global_mutex. Given all this, it's
not worth bothering to try to fix this at this time."

I think it's important to correctly document the conclusion of this
discussion, because we've worried quite a bit about correct locking.

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v3] drm: do not sleep on vblank while holding a mutex

[Ilija Hadzic]

OK, v4 coming up and your suggestion will be copied verbatim. Hopefully 
that does it.

This patch is probably going to become a record-holder in comment/code 
lines ratio ;-)

-- Ilija


On Mon, 31 Oct 2011, Daniel Vetter wrote:

> On Mon, Oct 31, 2011 at 01:10:21PM -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.
>>
>> v2: incorporate comments received from Daniel Vetter and
>>     Michel Daenzer.
>>
>> v3: after a lengty discussion with Daniel Vetter, it was concluded
>>     we should not worry about any locking, within drm_wait_vblank
>>     function so this patch becomes a rather trivial removal
>>     of drm_global_mutex from drm_wait_vblank
>
> That commit message is a bit garbage. What about ...
>
> "... it was concluded that the only think not yet protected with locks and
> atomic ops is the write to dev->last_vblank_wait. It's only used in a
> debug file in proc, and the current code already employs no correct
> locking: the proc file only takes dev->struct_mutex, whereas
> drm_wait_vblank implicitly took the drm_global_mutex. Given all this, it's
> not worth bothering to try to fix this at this time."
>
> I think it's important to correctly document the conclusion of this
> discussion, because we've worried quite a bit about correct locking.
>
> Yours, Daniel
> -- 
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48
>