Re: [PATCH 02/22] drm: Add functions to setup/tear down drm_events.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

Thank you for the patch.

On Monday 11 January 2016 22:40:56 Daniel Vetter wrote:
> An attempt at not spreading out the file_priv->event_space stuff out
> quite so far and wide.  And I think fixes something in ipp_get_event()
> that is broken (or if they are doing something more weird/subtle, then
> breaks it in a fun way).
> 
> Based upon a patch from Rob Clark, rebased and polished.
> 
> v2: Spelling fixes (Alex).
> 
> Cc: Alex Deucher <alexdeucher@gmail.com>
> Acked-by: Daniel Stone <daniels@collabora.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/drm_atomic.c | 44 ++++++++---------------------
>  drivers/gpu/drm/drm_crtc.c   | 36 +++++++-----------------
>  drivers/gpu/drm/drm_fops.c   | 67 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drmP.h           |  7 ++++-
>  4 files changed, 94 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3f74193885f1..8fb469c4e4b8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1347,44 +1347,23 @@ static struct drm_pending_vblank_event
> *create_vblank_event( struct drm_device *dev, struct drm_file *file_priv,
> uint64_t user_data) {
>  	struct drm_pending_vblank_event *e = NULL;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&dev->event_lock, flags);
> -	if (file_priv->event_space < sizeof e->event) {
> -		spin_unlock_irqrestore(&dev->event_lock, flags);
> -		goto out;
> -	}
> -	file_priv->event_space -= sizeof e->event;
> -	spin_unlock_irqrestore(&dev->event_lock, flags);
> +	int ret;
> 
>  	e = kzalloc(sizeof *e, GFP_KERNEL);
> -	if (e == NULL) {
> -		spin_lock_irqsave(&dev->event_lock, flags);
> -		file_priv->event_space += sizeof e->event;
> -		spin_unlock_irqrestore(&dev->event_lock, flags);
> -		goto out;
> -	}
> +	if (!e)
> +		return NULL;
> 
>  	e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
> -	e->event.base.length = sizeof e->event;
> +	e->event.base.length = sizeof(e->event);
>  	e->event.user_data = user_data;
> -	e->base.event = &e->event.base;
> -	e->base.file_priv = file_priv;
> -	e->base.destroy = (void (*) (struct drm_pending_event *)) kfree;
> -
> -out:
> -	return e;
> -}
> 
> -static void destroy_vblank_event(struct drm_device *dev,
> -		struct drm_file *file_priv, struct drm_pending_vblank_event *e)
> -{
> -	unsigned long flags;
> +	ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
> +	if (ret) {
> +		kfree(e);
> +		return NULL;
> +	}
> 
> -	spin_lock_irqsave(&dev->event_lock, flags);
> -	file_priv->event_space += sizeof e->event;
> -	spin_unlock_irqrestore(&dev->event_lock, flags);
> -	kfree(e);
> +	return e;
>  }
> 
>  static int atomic_set_prop(struct drm_atomic_state *state,
> @@ -1646,8 +1625,7 @@ out:
>  			if (!crtc_state->event)
>  				continue;
> 
> -			destroy_vblank_event(dev, file_priv,
> -					     crtc_state->event);
> +			drm_event_cancel_free(dev, &crtc_state->event->base);
>  		}
>  	}
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 1e75a145834a..60a4184d41b7 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5264,7 +5264,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	struct drm_framebuffer *fb = NULL;
>  	struct drm_pending_vblank_event *e = NULL;
> -	unsigned long flags;
>  	int ret = -EINVAL;
> 
>  	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
> @@ -5315,41 +5314,26 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> }
> 
>  	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> -		ret = -ENOMEM;
> -		spin_lock_irqsave(&dev->event_lock, flags);
> -		if (file_priv->event_space < sizeof(e->event)) {
> -			spin_unlock_irqrestore(&dev->event_lock, flags);
> -			goto out;
> -		}
> -		file_priv->event_space -= sizeof(e->event);
> -		spin_unlock_irqrestore(&dev->event_lock, flags);
> -
> -		e = kzalloc(sizeof(*e), GFP_KERNEL);
> -		if (e == NULL) {
> -			spin_lock_irqsave(&dev->event_lock, flags);
> -			file_priv->event_space += sizeof(e->event);
> -			spin_unlock_irqrestore(&dev->event_lock, flags);
> +		e = kzalloc(sizeof *e, GFP_KERNEL);
> +		if (!e) {
> +			ret = -ENOMEM;
>  			goto out;
>  		}
> -
>  		e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
>  		e->event.base.length = sizeof(e->event);
>  		e->event.user_data = page_flip->user_data;
> -		e->base.event = &e->event.base;
> -		e->base.file_priv = file_priv;
> -		e->base.destroy =
> -			(void (*) (struct drm_pending_event *)) kfree;
> +		ret = drm_event_reserve_init(dev, file_priv, &e->base, &e-
>event.base);
> +		if (ret) {
> +			kfree(e);
> +			goto out;
> +		}
>  	}
> 
>  	crtc->primary->old_fb = crtc->primary->fb;
>  	ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
>  	if (ret) {
> -		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> -			spin_lock_irqsave(&dev->event_lock, flags);
> -			file_priv->event_space += sizeof(e->event);
> -			spin_unlock_irqrestore(&dev->event_lock, flags);
> -			kfree(e);
> -		}
> +		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT)
> +			drm_event_cancel_free(dev, &e->base);
>  		/* Keep the old fb, don't unref it. */
>  		crtc->primary->old_fb = NULL;
>  	} else {
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 73075a1fa380..476408b638e3 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -672,3 +672,70 @@ unsigned int drm_poll(struct file *filp, struct
> poll_table_struct *wait) return mask;
>  }
>  EXPORT_SYMBOL(drm_poll);
> +
> +/**
> + * 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().
> + *
> + * If callers embedded @p into a larger structure it must be allocated with
> + * kmalloc and @p must be the first member element.
> + *
> + * 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 = 0;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +
> +	if (file_priv->event_space < e->length) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	file_priv->event_space -= e->length;
> +
> +	p->event = e;
> +	p->file_priv = file_priv;
> +
> +	/* we *could* pass this in as arg, but everyone uses kfree: */
> +	p->destroy = (void (*) (struct drm_pending_event *)) kfree;
> +
> +out:
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_event_reserve_init);
> +
> +/**
> + * drm_event_cancel_free - free a DRM event and release it's space
> + * @dev: DRM device
> + * @p: tracking structure for the pending event
> + *
> + * This function frees the event @p initialized with
> drm_event_reserve_init() + * and releases any allocated space.
> + */
> +void drm_event_cancel_free(struct drm_device *dev,
> +			   struct drm_pending_event *p)
> +{
> +	unsigned long flags;
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	p->file_priv->event_space += p->event->length;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +	p->destroy(p);
> +}
> +EXPORT_SYMBOL(drm_event_cancel_free);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index fc9b9ad5b089..ad4d0a31294d 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -925,8 +925,13 @@ ssize_t drm_read(struct file *filp, char __user
> *buffer, size_t count, loff_t *offset);
>  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(struct drm_device *dev,
> +			   struct drm_file *file_priv,
> +			   struct drm_pending_event *p,
> +			   struct drm_event *e);
> +void drm_event_cancel_free(struct drm_device *dev,
> +			   struct drm_pending_event *p);
> 
>  /* Misc. IOCTL support (drm_ioctl.c) */
>  int drm_noop(struct drm_device *dev, void *data,

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel