[RFC,2/7] drm: allow drm_wait_vblank_kernel() callback from workqueues

Commit Message

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

From: Paulo Zanoni <[e-mail]>

This is going to be needed by i915.ko, and I guess other drivers could
use it too.

Signed-off-by: Paulo Zanoni <[e-mail]>
---
 drivers/gpu/drm/drm_irq.c           | 46 ++++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_debugfs.c | 45 ++++++++++++++++++++++++++++--------
 include/drm/drmP.h                  | 11 ++++++++-
 3 files changed, 86 insertions(+), 16 deletions(-)

Comments

Matt Roper Dec. 5, 2014, 12:34 a.m. UTC | #1

On Wed, Nov 19, 2014 at 05:47:10PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <[e-mail]>
> 
> This is going to be needed by i915.ko, and I guess other drivers could
> use it too.

You may want to explain what we plan to use this for in i915 so that
other driver developers will more easily see where it might apply to
their own drivers.

> 
> Signed-off-by: Paulo Zanoni <[e-mail]>
> ---
>  drivers/gpu/drm/drm_irq.c           | 46 ++++++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/i915_debugfs.c | 45 ++++++++++++++++++++++++++++--------
>  include/drm/drmP.h                  | 11 ++++++++-
>  3 files changed, 86 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index f031f77..099aef1 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1108,6 +1108,22 @@ void drm_crtc_wait_one_vblank(struct drm_crtc *crtc)
>  }
>  EXPORT_SYMBOL(drm_crtc_wait_one_vblank);
>  
> +static void drm_wait_vblank_callback(struct drm_device *dev,
> +				     struct drm_pending_vblank_event *e,
> +				     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);

As I noted on your alternative implementation, do we need to let the
driver choose the workqueue it wants to wait on?  We're always going to
use the system workqueue here, but some places in i915 already use a
private workqueue.


> +	} else {
> +		e->callback(dev, e, seq, now, premature);
> +	}
> +}
> +
...
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 39d0d87..bc114f0 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -669,7 +669,16 @@ struct drm_pending_vblank_event {
>  	struct drm_pending_event base;
>  	int pipe;
>  	struct drm_event_vblank event;
> +
>  	drm_vblank_callback_t callback;
> +	bool callback_from_work;
> +	struct work_struct callback_work;
> +	struct {
> +		struct drm_device *dev;
> +		unsigned long seq;
> +		struct timeval *now;

Do we need seq and now here?  We still have access to the
drm_pending_vblank_event in our callback, so can't we just use the
fields we already have in e->event?

Also, do we need dev for something specific?  Can't the driver just grab
that from its user_data structure?


Matt


> +		bool premature;
> +	} callback_args;
>  };
>  
>  struct drm_vblank_crtc {
> @@ -906,7 +915,7 @@ extern int drm_vblank_init(struct drm_device *dev, int num_crtcs);
>  extern int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  				 struct drm_file *filp);
>  extern int drm_wait_vblank_kernel(struct drm_crtc *crtc, int count,
> -				  bool absolute,
> +				  bool absolute, bool callback_from_work,
>  				  drm_vblank_callback_t callback,
>  				  unsigned long user_data);
>  extern u32 drm_vblank_count(struct drm_device *dev, int crtc);
> -- 
> 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_irq.c b/drivers/gpu/drm/drm_irq.c
index f031f77..099aef1 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1108,6 +1108,22 @@  void drm_crtc_wait_one_vblank(struct drm_crtc *crtc)
 }
 EXPORT_SYMBOL(drm_crtc_wait_one_vblank);
 
+static void drm_wait_vblank_callback(struct drm_device *dev,
+				     struct drm_pending_vblank_event *e,
+				     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);
+	} else {
+		e->callback(dev, e, seq, now, premature);
+	}
+}
+
 /**
  * drm_vblank_off - disable vblank events on a CRTC
  * @dev: DRM device
@@ -1160,7 +1176,7 @@  void drm_vblank_off(struct drm_device *dev, int crtc)
 			  e->event.sequence, seq);
 		list_del(&e->base.link);
 		drm_vblank_put(dev, e->pipe);
-		e->callback(dev, e, seq, &now, true);
+		drm_wait_vblank_callback(dev, e, seq, &now, true);
 	}
 	spin_unlock_irqrestore(&dev->event_lock, irqflags);
 }
@@ -1372,9 +1388,20 @@  int drm_modeset_ctl(struct drm_device *dev, void *data,
 	return 0;
 }
 
+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);
+
+	e->callback(e->callback_args.dev, e, e->callback_args.seq,
+		    e->callback_args.now, e->callback_args.premature);
+}
+
 static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 				  union drm_wait_vblank *vblwait,
 				  struct drm_file *file_priv,
+				  bool callback_from_work,
 				  drm_vblank_callback_t callback)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
@@ -1399,6 +1426,9 @@  static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 	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;
+	if (callback_from_work)
+		INIT_WORK(&e->callback_work, drm_vblank_event_work_func);
 
 	spin_lock_irqsave(&dev->event_lock, flags);
 
@@ -1440,7 +1470,7 @@  static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 	e->event.sequence = vblwait->request.sequence;
 	if ((seq - vblwait->request.sequence) <= (1 << 23)) {
 		drm_vblank_put(dev, pipe);
-		e->callback(dev, e, seq, &now, false);
+		drm_wait_vblank_callback(dev, e, seq, &now, false);
 		vblwait->reply.sequence = seq;
 	} else {
 		/* drm_handle_vblank_events will call drm_vblank_put */
@@ -1477,6 +1507,7 @@  err_put:
 static int __drm_wait_vblank(struct drm_device *dev,
 			     union drm_wait_vblank *vblwait,
 			     struct drm_file *file_priv,
+			     bool callback_from_work,
 			     drm_vblank_callback_t callback)
 {
 	struct drm_vblank_crtc *vblank;
@@ -1533,7 +1564,7 @@  static int __drm_wait_vblank(struct drm_device *dev,
 		 * drm_vblank_put will be called asynchronously
 		 */
 		return drm_queue_vblank_event(dev, crtc, vblwait, file_priv,
-					      callback);
+					      callback_from_work, callback);
 	}
 
 	if ((flags & _DRM_VBLANK_NEXTONMISS) &&
@@ -1573,10 +1604,12 @@  int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 {
 	union drm_wait_vblank *vblwait = data;
 
-	return __drm_wait_vblank(dev, vblwait, file_priv, send_vblank_event);
+	return __drm_wait_vblank(dev, vblwait, file_priv, false,
+				 send_vblank_event);
 }
 
 int drm_wait_vblank_kernel(struct drm_crtc *crtc, int count, bool absolute,
+			   bool callback_from_work,
 			   drm_vblank_callback_t callback,
 			   unsigned long user_data)
 {
@@ -1593,7 +1626,8 @@  int drm_wait_vblank_kernel(struct drm_crtc *crtc, int count, bool absolute,
 	vblwait.request.sequence = count;
 	vblwait.request.signal = user_data;
 
-	return __drm_wait_vblank(dev, &vblwait, NULL, callback);
+	return __drm_wait_vblank(dev, &vblwait, NULL, callback_from_work,
+				 callback);
 }
 EXPORT_SYMBOL(drm_wait_vblank_kernel);
 
@@ -1618,7 +1652,7 @@  static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
 
 		list_del(&e->base.link);
 		drm_vblank_put(dev, e->pipe);
-		e->callback(dev, e, seq, &now, false);
+		drm_wait_vblank_callback(dev, e, seq, &now, false);
 	}
 
 	trace_drm_vblank_event(crtc, seq);
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f989587..95cf6d3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2718,6 +2718,7 @@  static int i915_ddb_info(struct seq_file *m, void *unused)
 struct vblank_data {
 	int eight;
 	const char *message;
+	bool can_sleep;
 };
 
 static void vblank_callback(struct drm_device *dev,
@@ -2727,6 +2728,7 @@  static void vblank_callback(struct drm_device *dev,
 {
 	struct vblank_data *data = (struct vblank_data *)e->event.user_data;
 
+	WARN_ON(data->can_sleep != drm_can_sleep());
 	WARN_ON(data->eight != 8);
 	DRM_DEBUG_KMS("vblank callback, seq: %lu, premature: %s message:%s\n",
 		      seq, yesno(premature), data->message);
@@ -2741,7 +2743,7 @@  static int i915_vblank_queue_test_new(struct seq_file *m, void *unused)
 	struct drm_device *dev = node->minor->dev;
 	struct intel_crtc *crtc = NULL;
 	int ret = 0;
-	struct vblank_data *data;
+	struct vblank_data *data1, *data2;
 
 	for_each_intel_crtc(dev, crtc)
 		if (crtc->pipe == PIPE_A)
@@ -2749,26 +2751,51 @@  static int i915_vblank_queue_test_new(struct seq_file *m, void *unused)
 	if (WARN_ON(!crtc))
 		return -EINVAL;
 
-	data = kzalloc(sizeof *data, GFP_KERNEL);
-	if (!data)
+	data1 = kzalloc(sizeof *data1, GFP_KERNEL);
+	if (!data1)
 		return -ENOMEM;
 
-	data->message = "hello world";
-	data->eight = 8;
+	data1->message = "hello world (atomic)";
+	data1->eight = 8;
+	data1->can_sleep = false;
+
+	data2 = kzalloc(sizeof *data2, GFP_KERNEL);
+	if (!data2) {
+		kfree(data1);
+		return -ENOMEM;
+	}
+
+	data2->message = "hello world (from work)";
+	data2->eight = 8;
+	data2->can_sleep = true;
 
 	DRM_DEBUG_KMS("waiting 60 vblanks (no callback)\n");
-	ret = drm_wait_vblank_kernel(&crtc->base, 60, false, NULL, 0);
+	ret = drm_wait_vblank_kernel(&crtc->base, 60, false, false, NULL, 0);
 	if (ret) {
 		DRM_DEBUG_KMS("vblank wait error: %d\n", ret);
+		kfree(data1);
+		kfree(data2);
 		return ret;
 	}
 	DRM_DEBUG_KMS("vblank wait done\n");
 
-	DRM_DEBUG_KMS("scheduling 60 vblanks (with callback)\n");
-	ret = drm_wait_vblank_kernel(&crtc->base, 60, false, vblank_callback,
-				     (unsigned long) data);
+	DRM_DEBUG_KMS("scheduling 60 vblanks (with callback, can't sleep)\n");
+	ret = drm_wait_vblank_kernel(&crtc->base, 60, false, false,
+				     vblank_callback, (unsigned long) data1);
+	if (ret) {
+		DRM_DEBUG_KMS("vblank schedule error: %d\n", ret);
+		kfree(data1);
+		kfree(data2);
+		return ret;
+	}
+	DRM_DEBUG_KMS("vblanks scheduled\n");
+
+	DRM_DEBUG_KMS("scheduling 60 vblanks (with callback, can sleep)\n");
+	ret = drm_wait_vblank_kernel(&crtc->base, 60, false, true,
+				     vblank_callback, (unsigned long) data2);
 	if (ret) {
 		DRM_DEBUG_KMS("vblank schedule error: %d\n", ret);
+		kfree(data2);
 		return ret;
 	}
 	DRM_DEBUG_KMS("vblanks scheduled\n");
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 39d0d87..bc114f0 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -669,7 +669,16 @@  struct drm_pending_vblank_event {
 	struct drm_pending_event base;
 	int pipe;
 	struct drm_event_vblank event;
+
 	drm_vblank_callback_t callback;
+	bool callback_from_work;
+	struct work_struct callback_work;
+	struct {
+		struct drm_device *dev;
+		unsigned long seq;
+		struct timeval *now;
+		bool premature;
+	} callback_args;
 };
 
 struct drm_vblank_crtc {
@@ -906,7 +915,7 @@  extern int drm_vblank_init(struct drm_device *dev, int num_crtcs);
 extern int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 				 struct drm_file *filp);
 extern int drm_wait_vblank_kernel(struct drm_crtc *crtc, int count,
-				  bool absolute,
+				  bool absolute, bool callback_from_work,
 				  drm_vblank_callback_t callback,
 				  unsigned long user_data);
 extern u32 drm_vblank_count(struct drm_device *dev, int crtc);