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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 25, 2011 at 10:20:14PM -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).
> 
> Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>

While you mess around with this code, please use the standard linux
wait_event_* infrastructure. I want to stop that DRM_FOO yelling from
spreading around - the idea of the drm core being os agnostic died quite a
while ago.

Again, have you carefully checked whether this is safe and how the
relevant data structures (vblank counting) are proctected?

One comment below, mind though that I've only glanced over the patch.
-Daniel

> ---
>  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);			\

Hiding a lock-dropping in this fashion is horrible.

> +								\
> +	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.7
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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