[RFC,3/7] drm: introduce struct drm_vblank_wait_item

Commit Message

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

From: Paulo Zanoni <[e-mail]>

It's supposed to contain all the information that is required for both
kernel and user space vblank wait items, but not hold any information
required by just one of them.

For now, we just moved the struct members from one place to another,
but the long term goal is that most of the drm.ko code will only
handle "struct drm_vblank_wait_item", not knowing anything else. This
will allow the callers to wrap this struct in their own private
structures. This will happen in the next patches.

Signed-off-by: Paulo Zanoni <[e-mail]>
---
 drivers/gpu/drm/drm_fops.c |  2 +-
 drivers/gpu/drm/drm_irq.c  | 42 ++++++++++++++++++++++--------------------
 include/drm/drmP.h         | 10 +++++++---
 3 files changed, 30 insertions(+), 24 deletions(-)

Comments

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

On Wed, Nov 19, 2014 at 05:47:11PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <[e-mail]>
> 
> It's supposed to contain all the information that is required for both
> kernel and user space vblank wait items, but not hold any information
> required by just one of them.
> 
> For now, we just moved the struct members from one place to another,
> but the long term goal is that most of the drm.ko code will only
> handle "struct drm_vblank_wait_item", not knowing anything else. This
> will allow the callers to wrap this struct in their own private
> structures. This will happen in the next patches.
> 
> Signed-off-by: Paulo Zanoni <[e-mail]>

I'd clarify the commit message here to say that it's *eventually* going
to contain all of the general data and be subclassed by callers.  On
first read, I was confused here since you still have to use
e->event.user_data until patch #7.

Personally, I'd also just squash patch #4 into this one.

...
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index bc114f0..b8bc55a 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -665,10 +665,8 @@ typedef void (*drm_vblank_callback_t)(struct drm_device *dev,
>  				      unsigned long seq, struct timeval *now,
>  				      bool premature);
>  
> -struct drm_pending_vblank_event {
> -	struct drm_pending_event base;
> +struct drm_vblank_wait_item {
>  	int pipe;
> -	struct drm_event_vblank event;

I know it's nitpicking, but the term 'item' seems a bit vague/ambiguous
to me.  Maybe something like drm_vblank_task or drm_vblank_job would be
slightly more descriptive?  In the same vein, I'd suggest something like
"drm_schedule_vblank_job()" in place of drm_wait_vblank_kernel() and
"drm_trigger_vblank_job()" in place of drm_wait_vblank_callback().  Of
course this is all pretty much personal opinion, so feel free to ignore
this suggestion if you disagree.  :-)


Matt

>  
>  	drm_vblank_callback_t callback;
>  	bool callback_from_work;
> @@ -681,6 +679,12 @@ struct drm_pending_vblank_event {
>  	} callback_args;
>  };
>  
> +struct drm_pending_vblank_event {
> +	struct drm_pending_event base;
> +	struct drm_event_vblank event;
> +	struct drm_vblank_wait_item item;
> +};
> +
>  struct drm_vblank_crtc {
>  	struct drm_device *dev;		/* pointer to the drm_device */
>  	wait_queue_head_t queue;	/**< VBLANK wait queue */
> -- 
> 2.1.1
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 91e1105..47c5e58 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -289,7 +289,7 @@  static void drm_events_release(struct drm_file *file_priv)
 	list_for_each_entry_safe(v, vt, &dev->vblank_event_list, base.link)
 		if (v->base.file_priv == file_priv) {
 			list_del(&v->base.link);
-			drm_vblank_put(dev, v->pipe);
+			drm_vblank_put(dev, v->item.pipe);
 			v->base.destroy(&v->base);
 		}
 
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 099aef1..7dcbbdb 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -893,7 +893,7 @@  static void send_vblank_event(struct drm_device *dev,
 	list_add_tail(&e->base.link,
 		      &e->base.file_priv->event_list);
 	wake_up_interruptible(&e->base.file_priv->event_wait);
-	trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
+	trace_drm_vblank_event_delivered(e->base.pid, e->item.pipe,
 					 e->event.sequence);
 }
 
@@ -918,7 +918,7 @@  void drm_send_vblank_event(struct drm_device *dev, int crtc,
 
 		now = get_drm_timestamp();
 	}
-	e->pipe = crtc;
+	e->item.pipe = crtc;
 	send_vblank_event(dev, e, seq, &now, false);
 }
 EXPORT_SYMBOL(drm_send_vblank_event);
@@ -1113,14 +1113,14 @@  static void drm_wait_vblank_callback(struct drm_device *dev,
 				     unsigned long seq, struct timeval *now,
 				     bool premature)
 {
-	if (e->callback_from_work) {
-		e->callback_args.dev = dev;
-		e->callback_args.seq = seq;
-		e->callback_args.now = now;
-		e->callback_args.premature = premature;
-		schedule_work(&e->callback_work);
+	if (e->item.callback_from_work) {
+		e->item.callback_args.dev = dev;
+		e->item.callback_args.seq = seq;
+		e->item.callback_args.now = now;
+		e->item.callback_args.premature = premature;
+		schedule_work(&e->item.callback_work);
 	} else {
-		e->callback(dev, e, seq, now, premature);
+		e->item.callback(dev, e, seq, now, premature);
 	}
 }
 
@@ -1169,13 +1169,13 @@  void drm_vblank_off(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->pipe != crtc)
+		if (e->item.pipe != crtc)
 			continue;
 		DRM_DEBUG("Sending premature vblank event on disable: \
 			  wanted %d, current %d\n",
 			  e->event.sequence, seq);
 		list_del(&e->base.link);
-		drm_vblank_put(dev, e->pipe);
+		drm_vblank_put(dev, e->item.pipe);
 		drm_wait_vblank_callback(dev, e, seq, &now, true);
 	}
 	spin_unlock_irqrestore(&dev->event_lock, irqflags);
@@ -1392,10 +1392,12 @@  static void drm_vblank_event_work_func(struct work_struct *work)
 {
 	struct drm_pending_vblank_event *e =
 		container_of(work, struct drm_pending_vblank_event,
-			     callback_work);
+			     item.callback_work);
 
-	e->callback(e->callback_args.dev, e, e->callback_args.seq,
-		    e->callback_args.now, e->callback_args.premature);
+	e->item.callback(e->item.callback_args.dev, e,
+			 e->item.callback_args.seq,
+			 e->item.callback_args.now,
+			 e->item.callback_args.premature);
 }
 
 static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
@@ -1417,7 +1419,7 @@  static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 		goto err_put;
 	}
 
-	e->pipe = pipe;
+	e->item.pipe = pipe;
 	e->base.pid = current->pid;
 	e->event.base.type = DRM_EVENT_VBLANK;
 	e->event.base.length = sizeof e->event;
@@ -1425,10 +1427,10 @@  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->callback = callback;
-	e->callback_from_work = callback_from_work;
+	e->item.callback = callback;
+	e->item.callback_from_work = callback_from_work;
 	if (callback_from_work)
-		INIT_WORK(&e->callback_work, drm_vblank_event_work_func);
+		INIT_WORK(&e->item.callback_work, drm_vblank_event_work_func);
 
 	spin_lock_irqsave(&dev->event_lock, flags);
 
@@ -1642,7 +1644,7 @@  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->pipe != crtc)
+		if (e->item.pipe != crtc)
 			continue;
 		if ((seq - e->event.sequence) > (1<<23))
 			continue;
@@ -1651,7 +1653,7 @@  static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
 			  e->event.sequence, seq);
 
 		list_del(&e->base.link);
-		drm_vblank_put(dev, e->pipe);
+		drm_vblank_put(dev, e->item.pipe);
 		drm_wait_vblank_callback(dev, e, seq, &now, false);
 	}
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index bc114f0..b8bc55a 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -665,10 +665,8 @@  typedef void (*drm_vblank_callback_t)(struct drm_device *dev,
 				      unsigned long seq, struct timeval *now,
 				      bool premature);
 
-struct drm_pending_vblank_event {
-	struct drm_pending_event base;
+struct drm_vblank_wait_item {
 	int pipe;
-	struct drm_event_vblank event;
 
 	drm_vblank_callback_t callback;
 	bool callback_from_work;
@@ -681,6 +679,12 @@  struct drm_pending_vblank_event {
 	} callback_args;
 };
 
+struct drm_pending_vblank_event {
+	struct drm_pending_event base;
+	struct drm_event_vblank event;
+	struct drm_vblank_wait_item item;
+};
+
 struct drm_vblank_crtc {
 	struct drm_device *dev;		/* pointer to the drm_device */
 	wait_queue_head_t queue;	/**< VBLANK wait queue */