Re: [PATCH 01/15] drm/vblank: Use drm_event_reserve_init

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jan 25, 2016 at 10:16:42PM +0100, Daniel Vetter wrote:
> Well we can't use that directly since that code must hold
> dev->event_lock already. Extract an _unlocked version.
> 
> Embarrassingly I've totally forgotten about this patch and any kind of
> event-based vblank wait totally blew up, killing the kernel.
> 
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Forgot cover letter: Just this patch is new, fixes an embarrassing BUG
when using any kind of drm_event for vblanks :( Everything else in the
patch series is unchanged. Review on this one highly welcome.

To make sure I'm not that silly again resending the entire pile, so that
our intel CI bot can pick it up again and test it for real.
-Daniel

> ---
>  drivers/gpu/drm/drm_fops.c | 64 +++++++++++++++++++++++++++++++++++-----------
>  drivers/gpu/drm/drm_irq.c  | 10 +++-----
>  include/drm/drmP.h         |  4 +++
>  3 files changed, 56 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index e13501e3606e..eb6a02f78697 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -678,7 +678,7 @@ unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait)
>  EXPORT_SYMBOL(drm_poll);
>  
>  /**
> - * drm_event_reserve_init - init a DRM event and reserve space for it
> + * drm_event_reserve_init_locked - init a DRM event and reserve space for it
>   * @dev: DRM device
>   * @file_priv: DRM file private data
>   * @p: tracking structure for the pending event
> @@ -694,24 +694,20 @@ EXPORT_SYMBOL(drm_poll);
>   * If callers embedded @p into a larger structure it must be allocated with
>   * kmalloc and @p must be the first member element.
>   *
> + * This is the locked version of drm_event_reserve_init() for callers which
> + * already hold dev->event_lock.
> + *
>   * RETURNS:
>   *
>   * 0 on success or a negative error code on failure.
>   */
> -int drm_event_reserve_init(struct drm_device *dev,
> -			   struct drm_file *file_priv,
> -			   struct drm_pending_event *p,
> -			   struct drm_event *e)
> +int drm_event_reserve_init_locked(struct drm_device *dev,
> +				  struct drm_file *file_priv,
> +				  struct drm_pending_event *p,
> +				  struct drm_event *e)
>  {
> -	unsigned long flags;
> -	int ret = 0;
> -
> -	spin_lock_irqsave(&dev->event_lock, flags);
> -
> -	if (file_priv->event_space < e->length) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> +	if (file_priv->event_space < e->length)
> +		return -ENOMEM;
>  
>  	file_priv->event_space -= e->length;
>  
> @@ -721,8 +717,46 @@ int drm_event_reserve_init(struct drm_device *dev,
>  	/* we *could* pass this in as arg, but everyone uses kfree: */
>  	p->destroy = (void (*) (struct drm_pending_event *)) kfree;
>  
> -out:
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_event_reserve_init_locked);
> +
> +/**
> + * drm_event_reserve_init - init a DRM event and reserve space for it
> + * @dev: DRM device
> + * @file_priv: DRM file private data
> + * @p: tracking structure for the pending event
> + * @e: actual event data to deliver to userspace
> + *
> + * This function prepares the passed in event for eventual delivery. If the event
> + * doesn't get delivered (because the IOCTL fails later on, before queuing up
> + * anything) then the even must be cancelled and freed using
> + * drm_event_cancel_free(). Successfully initialized events should be sent out
> + * using drm_send_event() or drm_send_event_locked() to signal completion of the
> + * asynchronous event to userspace.
> + *
> + * If callers embedded @p into a larger structure it must be allocated with
> + * kmalloc and @p must be the first member element.
> + *
> + * Callers which already hold dev->event_lock should use
> + * drm_event_reserve_init() instead.
> + *
> + * RETURNS:
> + *
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_event_reserve_init(struct drm_device *dev,
> +			   struct drm_file *file_priv,
> +			   struct drm_pending_event *p,
> +			   struct drm_event *e)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	ret = drm_event_reserve_init_locked(dev, file_priv, p, e);
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_event_reserve_init);
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 4ec8bca643ac..aa602199cb5d 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1598,9 +1598,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  	e->event.base.type = DRM_EVENT_VBLANK;
>  	e->event.base.length = sizeof(e->event);
>  	e->event.user_data = vblwait->request.signal;
> -	e->base.event = &e->event.base;
> -	e->base.file_priv = file_priv;
> -	e->base.destroy = (void (*) (struct drm_pending_event *)) kfree;
>  
>  	spin_lock_irqsave(&dev->event_lock, flags);
>  
> @@ -1616,12 +1613,11 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  		goto err_unlock;
>  	}
>  
> -	if (file_priv->event_space < sizeof(e->event)) {
> -		ret = -EBUSY;
> +	ret = drm_event_reserve_init_locked(dev, file_priv, &e->base, &e->event);
> +
> +	if (ret)
>  		goto err_unlock;
> -	}
>  
> -	file_priv->event_space -= sizeof(e->event);
>  	seq = drm_vblank_count_and_time(dev, pipe, &now);
>  
>  	if ((vblwait->request.type & _DRM_VBLANK_NEXTONMISS) &&
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 306ef32ec086..1b71852d0a55 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -926,6 +926,10 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
>  int drm_release(struct inode *inode, struct file *filp);
>  int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv);
>  unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);
> +int drm_event_reserve_init_locked(struct drm_device *dev,
> +				  struct drm_file *file_priv,
> +				  struct drm_pending_event *p,
> +				  struct drm_event *e);
>  int drm_event_reserve_init(struct drm_device *dev,
>  			   struct drm_file *file_priv,
>  			   struct drm_pending_event *p,
> -- 
> 2.7.0.rc3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx