[RFC,6/7] drm: make vblank_event_list handle drm_vblank_wait_item types

Commit Message

Paulo Zanoni Nov. 19, 2014, 7:47 p.m. UTC

From: Paulo Zanoni <[e-mail]>

Which means the list doesn't really need to know if the event is from
user space or kernel space.

The only place here where we have to break the abstraction is at
drm_fops, when we're releasing all the events associated with a
file_priv. Here we introduced item.from_user_space, that needs to be
checked before we transform the item pointer into the appropriate
drm_pending_vblank_event pointer. Other strategies to solve this
problem - instead of adding item->from_user_space - would be to: (i)
store a copy of the file_priv pointer in the drm_vblank_wait_item
structure, but then we'd also need a custom destroy() function; or
(ii) add file_priv->pending_event_list, but then we'd have to keep all
the user space items in sync with dev->vblank_event_list, which could
lead to complicated code.

Signed-off-by: Paulo Zanoni <[e-mail]>
---
 drivers/gpu/drm/drm_fops.c | 15 +++++++++++----
 drivers/gpu/drm/drm_irq.c  | 33 +++++++++++++++++----------------
 include/drm/drmP.h         |  4 +++-
 3 files changed, 31 insertions(+), 21 deletions(-)

Comments

Matt Roper Dec. 5, 2014, 2:27 a.m. UTC | #1

On Wed, Nov 19, 2014 at 05:47:14PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <[e-mail]>
> 
> Which means the list doesn't really need to know if the event is from
> user space or kernel space.
> 
> The only place here where we have to break the abstraction is at
> drm_fops, when we're releasing all the events associated with a
> file_priv. Here we introduced item.from_user_space, that needs to be
> checked before we transform the item pointer into the appropriate
> drm_pending_vblank_event pointer. Other strategies to solve this
> problem - instead of adding item->from_user_space - would be to: (i)
> store a copy of the file_priv pointer in the drm_vblank_wait_item
> structure, but then we'd also need a custom destroy() function; or
> (ii) add file_priv->pending_event_list, but then we'd have to keep all
> the user space items in sync with dev->vblank_event_list, which could
> lead to complicated code.

Intuitively, (ii) seems like the natural choice to me.  The pending
userspace events really are fd-specific, so it seems natural to stick
them in a drm_file list rather than a global one.  You'd have to remove
both the base class and the subclass from their respective lists when
you're done with an event, but since all of this is done under the
event_lock spinlock, it doesn't seem like it would be too complicated.
Am I overlooking something that makes it more challenging?


Matt

> 
> Signed-off-by: Paulo Zanoni <[e-mail]>
> ---
>  drivers/gpu/drm/drm_fops.c | 15 +++++++++++----
>  drivers/gpu/drm/drm_irq.c  | 33 +++++++++++++++++----------------
>  include/drm/drmP.h         |  4 +++-
>  3 files changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 47c5e58..fbdbde2 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -280,18 +280,25 @@ static void drm_events_release(struct drm_file *file_priv)
>  {
>  	struct drm_device *dev = file_priv->minor->dev;
>  	struct drm_pending_event *e, *et;
> -	struct drm_pending_vblank_event *v, *vt;
> +	struct drm_vblank_wait_item *i, *it;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&dev->event_lock, flags);
>  
>  	/* Remove pending flips */
> -	list_for_each_entry_safe(v, vt, &dev->vblank_event_list, base.link)
> +	list_for_each_entry_safe(i, it, &dev->vblank_event_list, link) {
> +		struct drm_pending_vblank_event *v;
> +
> +		if (!i->from_user_space)
> +			continue;
> +
> +		v = container_of(i, struct drm_pending_vblank_event, item);
>  		if (v->base.file_priv == file_priv) {
> -			list_del(&v->base.link);
> -			drm_vblank_put(dev, v->item.pipe);
> +			list_del(&i->link);
> +			drm_vblank_put(dev, i->pipe);
>  			v->base.destroy(&v->base);
>  		}
> +	}
>  
>  	/* Remove unconsumed events */
>  	list_for_each_entry_safe(e, et, &file_priv->event_list, link) {
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 4c03240..dd091c3 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1144,7 +1144,7 @@ static void drm_wait_vblank_callback(struct drm_device *dev,
>  void drm_vblank_off(struct drm_device *dev, int crtc)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> -	struct drm_pending_vblank_event *e, *t;
> +	struct drm_vblank_wait_item *i, *t;
>  	struct timeval now;
>  	unsigned long irqflags;
>  	unsigned int seq;
> @@ -1171,15 +1171,15 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>  	/* Send any queued vblank events, lest the natives grow disquiet */
>  	seq = drm_vblank_count_and_time(dev, crtc, &now);
>  
> -	list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
> -		if (e->item.pipe != crtc)
> +	list_for_each_entry_safe(i, t, &dev->vblank_event_list, link) {
> +		if (i->pipe != crtc)
>  			continue;
>  		DRM_DEBUG("Sending premature vblank event on disable: \
>  			  wanted %d, current %d\n",
> -			  e->item.wanted_seq, seq);
> -		list_del(&e->base.link);
> -		drm_vblank_put(dev, e->item.pipe);
> -		drm_wait_vblank_callback(dev, &e->item, seq, &now, true);
> +			 i->wanted_seq, seq);
> +		list_del(&i->link);
> +		drm_vblank_put(dev, i->pipe);
> +		drm_wait_vblank_callback(dev, i, seq, &now, true);
>  	}
>  	spin_unlock_irqrestore(&dev->event_lock, irqflags);
>  }
> @@ -1427,6 +1427,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
>  	e->base.event = &e->event.base;
>  	e->base.file_priv = file_priv;
>  	e->base.destroy = (void (*) (struct drm_pending_event *)) kfree;
> +	e->item.from_user_space = true;
>  	e->item.callback = callback;
>  	e->item.callback_from_work = callback_from_work;
>  	if (callback_from_work)
> @@ -1476,7 +1477,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
>  		vblwait->reply.sequence = seq;
>  	} else {
>  		/* drm_handle_vblank_events will call drm_vblank_put */
> -		list_add_tail(&e->base.link, &dev->vblank_event_list);
> +		list_add_tail(&e->item.link, &dev->vblank_event_list);
>  		vblwait->reply.sequence = vblwait->request.sequence;
>  	}
>  
> @@ -1635,7 +1636,7 @@ EXPORT_SYMBOL(drm_wait_vblank_kernel);
>  
>  static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
>  {
> -	struct drm_pending_vblank_event *e, *t;
> +	struct drm_vblank_wait_item *i, *t;
>  	struct timeval now;
>  	unsigned int seq;
>  
> @@ -1643,18 +1644,18 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
>  
>  	seq = drm_vblank_count_and_time(dev, crtc, &now);
>  
> -	list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
> -		if (e->item.pipe != crtc)
> +	list_for_each_entry_safe(i, t, &dev->vblank_event_list, link) {
> +		if (i->pipe != crtc)
>  			continue;
> -		if ((seq - e->item.wanted_seq) > (1<<23))
> +		if ((seq - i->wanted_seq) > (1<<23))
>  			continue;
>  
>  		DRM_DEBUG("vblank event on %d, current %d\n",
> -			  e->item.wanted_seq, seq);
> +			  i->wanted_seq, seq);
>  
> -		list_del(&e->base.link);
> -		drm_vblank_put(dev, e->item.pipe);
> -		drm_wait_vblank_callback(dev, &e->item, seq, &now, false);
> +		list_del(&i->link);
> +		drm_vblank_put(dev, i->pipe);
> +		drm_wait_vblank_callback(dev, i, seq, &now, false);
>  	}
>  
>  	trace_drm_vblank_event(crtc, seq);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 474c892..46724d9 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -258,7 +258,7 @@ struct drm_ioctl_desc {
>  /* Event queued up for userspace to read */
>  struct drm_pending_event {
>  	struct drm_event *event;
> -	struct list_head link;
> +	struct list_head link; /* file_priv->event_list */
>  	struct drm_file *file_priv;
>  	pid_t pid; /* pid of requester, no guarantee it's valid by the time
>  		      we deliver the event, for tracing only */
> @@ -668,8 +668,10 @@ typedef void (*drm_vblank_callback_t)(struct drm_device *dev,
>  				      bool premature);
>  
>  struct drm_vblank_wait_item {
> +	struct list_head link; /* dev->vblank_event_list */
>  	int pipe;
>  	unsigned int wanted_seq;
> +	bool from_user_space;
>  
>  	drm_vblank_callback_t callback;
>  	bool callback_from_work;
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 47c5e58..fbdbde2 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -280,18 +280,25 @@  static void drm_events_release(struct drm_file *file_priv)
 {
 	struct drm_device *dev = file_priv->minor->dev;
 	struct drm_pending_event *e, *et;
-	struct drm_pending_vblank_event *v, *vt;
+	struct drm_vblank_wait_item *i, *it;
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev->event_lock, flags);
 
 	/* Remove pending flips */
-	list_for_each_entry_safe(v, vt, &dev->vblank_event_list, base.link)
+	list_for_each_entry_safe(i, it, &dev->vblank_event_list, link) {
+		struct drm_pending_vblank_event *v;
+
+		if (!i->from_user_space)
+			continue;
+
+		v = container_of(i, struct drm_pending_vblank_event, item);
 		if (v->base.file_priv == file_priv) {
-			list_del(&v->base.link);
-			drm_vblank_put(dev, v->item.pipe);
+			list_del(&i->link);
+			drm_vblank_put(dev, i->pipe);
 			v->base.destroy(&v->base);
 		}
+	}
 
 	/* Remove unconsumed events */
 	list_for_each_entry_safe(e, et, &file_priv->event_list, link) {
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 4c03240..dd091c3 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1144,7 +1144,7 @@  static void drm_wait_vblank_callback(struct drm_device *dev,
 void drm_vblank_off(struct drm_device *dev, int crtc)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
-	struct drm_pending_vblank_event *e, *t;
+	struct drm_vblank_wait_item *i, *t;
 	struct timeval now;
 	unsigned long irqflags;
 	unsigned int seq;
@@ -1171,15 +1171,15 @@  void drm_vblank_off(struct drm_device *dev, int crtc)
 	/* Send any queued vblank events, lest the natives grow disquiet */
 	seq = drm_vblank_count_and_time(dev, crtc, &now);
 
-	list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
-		if (e->item.pipe != crtc)
+	list_for_each_entry_safe(i, t, &dev->vblank_event_list, link) {
+		if (i->pipe != crtc)
 			continue;
 		DRM_DEBUG("Sending premature vblank event on disable: \
 			  wanted %d, current %d\n",
-			  e->item.wanted_seq, seq);
-		list_del(&e->base.link);
-		drm_vblank_put(dev, e->item.pipe);
-		drm_wait_vblank_callback(dev, &e->item, seq, &now, true);
+			 i->wanted_seq, seq);
+		list_del(&i->link);
+		drm_vblank_put(dev, i->pipe);
+		drm_wait_vblank_callback(dev, i, seq, &now, true);
 	}
 	spin_unlock_irqrestore(&dev->event_lock, irqflags);
 }
@@ -1427,6 +1427,7 @@  static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 	e->base.event = &e->event.base;
 	e->base.file_priv = file_priv;
 	e->base.destroy = (void (*) (struct drm_pending_event *)) kfree;
+	e->item.from_user_space = true;
 	e->item.callback = callback;
 	e->item.callback_from_work = callback_from_work;
 	if (callback_from_work)
@@ -1476,7 +1477,7 @@  static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 		vblwait->reply.sequence = seq;
 	} else {
 		/* drm_handle_vblank_events will call drm_vblank_put */
-		list_add_tail(&e->base.link, &dev->vblank_event_list);
+		list_add_tail(&e->item.link, &dev->vblank_event_list);
 		vblwait->reply.sequence = vblwait->request.sequence;
 	}
 
@@ -1635,7 +1636,7 @@  EXPORT_SYMBOL(drm_wait_vblank_kernel);
 
 static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
 {
-	struct drm_pending_vblank_event *e, *t;
+	struct drm_vblank_wait_item *i, *t;
 	struct timeval now;
 	unsigned int seq;
 
@@ -1643,18 +1644,18 @@  static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
 
 	seq = drm_vblank_count_and_time(dev, crtc, &now);
 
-	list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
-		if (e->item.pipe != crtc)
+	list_for_each_entry_safe(i, t, &dev->vblank_event_list, link) {
+		if (i->pipe != crtc)
 			continue;
-		if ((seq - e->item.wanted_seq) > (1<<23))
+		if ((seq - i->wanted_seq) > (1<<23))
 			continue;
 
 		DRM_DEBUG("vblank event on %d, current %d\n",
-			  e->item.wanted_seq, seq);
+			  i->wanted_seq, seq);
 
-		list_del(&e->base.link);
-		drm_vblank_put(dev, e->item.pipe);
-		drm_wait_vblank_callback(dev, &e->item, seq, &now, false);
+		list_del(&i->link);
+		drm_vblank_put(dev, i->pipe);
+		drm_wait_vblank_callback(dev, i, seq, &now, false);
 	}
 
 	trace_drm_vblank_event(crtc, seq);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 474c892..46724d9 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -258,7 +258,7 @@  struct drm_ioctl_desc {
 /* Event queued up for userspace to read */
 struct drm_pending_event {
 	struct drm_event *event;
-	struct list_head link;
+	struct list_head link; /* file_priv->event_list */
 	struct drm_file *file_priv;
 	pid_t pid; /* pid of requester, no guarantee it's valid by the time
 		      we deliver the event, for tracing only */
@@ -668,8 +668,10 @@  typedef void (*drm_vblank_callback_t)(struct drm_device *dev,
 				      bool premature);
 
 struct drm_vblank_wait_item {
+	struct list_head link; /* dev->vblank_event_list */
 	int pipe;
 	unsigned int wanted_seq;
+	bool from_user_space;
 
 	drm_vblank_callback_t callback;
 	bool callback_from_work;