Attachment 52833 Details for Bug 39202 – provisional patch to try as an experiment

[patch] provisional patch to try as an experiment

0001-drm-do-not-sleep-on-vblank-while-holding-a-mutex.patch (text/plain), 4.30 KB, created by Ilija Hadzic on 2011-10-27 15:27:15 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ilija Hadzic

Created: 2011-10-27 15:27:15 UTC

Size: 4.30 KB

patch

obsolete

>From c16cf5b55ccf20d9b3d57f97181b2e492ae6117e Mon Sep 17 00:00:00 2001
>From: Ilija Hadzic <ihadzic@research.bell-labs.com>
>Date: Sat, 22 Oct 2011 00:04:29 -0400
>Subject: [PATCH] drm: do not sleep on vblank while holding a mutex
>
>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).
>
>Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
>---
> drivers/gpu/drm/drm_drv.c  |    2 +-
> drivers/gpu/drm/drm_irq.c  |   16 +++++++++++-----
> include/drm/drm_os_linux.h |   25 +++++++++++++++++++++++++
> 3 files changed, 37 insertions(+), 6 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),
> 
>diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>index 3830e9e..d85d604 100644
>--- a/drivers/gpu/drm/drm_irq.c
>+++ b/drivers/gpu/drm/drm_irq.c
>@@ -1191,6 +1191,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> 		DRM_DEBUG("failed to acquire vblank counter, %d\n", ret);
> 		return ret;
> 	}
>+	mutex_lock(&drm_global_mutex);
> 	seq = drm_vblank_count(dev, crtc);
> 
> 	switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
>@@ -1200,12 +1201,15 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> 	case _DRM_VBLANK_ABSOLUTE:
> 		break;
> 	default:
>+		mutex_unlock(&drm_global_mutex);
> 		ret = -EINVAL;
> 		goto done;
> 	}
> 
>-	if (flags & _DRM_VBLANK_EVENT)
>+	if (flags & _DRM_VBLANK_EVENT) {
>+		mutex_unlock(&drm_global_mutex);
> 		return drm_queue_vblank_event(dev, crtc, vblwait, file_priv);
>+	}
> 
> 	if ((flags & _DRM_VBLANK_NEXTONMISS) &&
> 	    (seq - vblwait->request.sequence) <= (1<<23)) {
>@@ -1215,10 +1219,12 @@ 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;
>-	DRM_WAIT_ON(ret, dev->vbl_queue[crtc], 3 * DRM_HZ,
>-		    (((drm_vblank_count(dev, crtc) -
>-		       vblwait->request.sequence) <= (1 << 23)) ||
>-		     !dev->irq_enabled));
>+	/* DRM_WAIT_ON_LOCKED will release drm_global_mutex */
>+	/* before going to sleep */
>+	DRM_WAIT_ON_LOCKED(ret, dev->vbl_queue[crtc], 3 * DRM_HZ,
>+			   (((drm_vblank_count(dev, crtc) -
>+			      vblwait->request.sequence) <= (1 << 23)) ||
>+			    !dev->irq_enabled));
> 
> 	if (ret != -EINTR) {
> 		struct timeval now;
>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);			\
>+								\
>+	for (;;) {						\
>+		__set_current_state(TASK_INTERRUPTIBLE);	\
>+		if (condition)					\
>+			break;					\
>+		if (time_after_eq(jiffies, end)) {		\
>+			ret = -EBUSY;				\
>+			break;					\
>+		}						\
>+		schedule_timeout((HZ/100 > 1) ? HZ/100 : 1);	\
>+		if (signal_pending(current)) {			\
>+			ret = -EINTR;				\
>+			break;					\
>+		}						\
>+	}							\
>+	__set_current_state(TASK_RUNNING);			\
>+	remove_wait_queue(&(queue), &entry);			\
>+} while (0)
>+
> #define DRM_WAKEUP( queue ) wake_up( queue )
> #define DRM_INIT_WAITQUEUE( queue ) init_waitqueue_head( queue )
>-- 
>1.7.4.1
>

Actions: View | Splinter Review

Attachments on bug 39202: 49061 | 49062 | 49063 | 52833

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.