From: Paulo Zanoni paulo.r.zanoni@intel.com
Hi
--- TL;DR summary: I wrote patches. Help me choose the best implementation and interface. ---
So the i915.ko driver could use some mechanism to run functions after a given number of vblanks. Implementations for this mechanism were already proposed in the past (by Chris Wilson and others), but they were i915-specific instead of a generic drm.ko implementation. We even had patches that make use of this new mechanism.
Since this is very similar to the vblank IOCTL we currently have, but with the caller being a Kernel module instead of user space, I decided to write an implementation that tries to reuse the current IOCTL infrastructure.
In the first patch we add all the necessary code to allow the modules to call the vblank ioctl from Kernel space: they provide a callback function that is going to be called instead of the traditional send_vblank_event(), which means the Kernel callers don't have to deal with the file descriptors and DRM events.
In the second patch we add a simple extension of the feature, to allow the drivers to have their callbacks called in a non-atomic context.
In all the other subsequent patches we rework the underlying code so that the common aspects between the user space vblank IOCTL and the Kernel interface are all stored in a single structure (struct drm_vblank_wait_item), and then anything that is specific to the different users is stored in a structure that contains struct drm_vblank_wait_item. This way, most of the drm.ko code only needs to deal with struct drm_vblank_wait_item, not really knowing how it is wrapped. The advantage of this rework is that it reduces the number of memory allocations needed in some cases (from 2 to 1) and it also reduces the amount of memory used on each allocation.
But while this all sounds nice in the above description, I had the feeling that this adds a lot of complexity and makes the code not-so-beautiful. If we ever need more extensions/options to this feature, we're going to have to untangle the code even more from the IOCTL part. We also have a small "abstraction break" in change introduced in patch 6. And there's the risk that some of the reworks may have added a regression somewhere.
Based on that, I decided to also provide an alternative implementation. In this implementation we add a new dev->vblank_kernel_list instead of reusing dev->vblank_event_list, and add new code to handle that this. This new code is completely based on the code that handles dev->vblank_kernel_list, so there's some duplication here. On the other hand, since the in-Kernel infrastructure is not tied to the IOCTL structure, we can very easily grow and adapt the Kernel code without having to fight against the IOCTL code. And the risk of regressions is greatly reduced.
The second difference of this alternative implementation is the signature of the drm_wait_vblank_kernel() function. While it could be exactly the same as the one used in the other series, I decided to make it different so we can also choose which one to use. In the 7-patch series implementation, all the user needs to do is to allocate the structure, and call the function, properly setting all its arguments. Then the function is responsible for parsing the arguments and populating the structure based on that. On the alternative implementation, the user has to fill the structure with the appropriate arguments, and then call drm_wait_vblank_kernel() passing just the allocated structure as an argument.
If you notice, you will see that these patches add a new debugfs file to i915.ko. This file is just a very simple example user of the new interface, which I used while developing. If you have difficulty understanding the new interface, please also look at the i915/i915_debugfs.c diff. Of course, I plan to exclude this debugfs interface from the final to-be-merged patches.
So, in summary, we have a few things we need to discuss:
1. Which implementation to use?
a. Just the 2 first patches of the 7-patch series? Pros: - the Kernel users basically call the vblank IOCTL - the code changes are minimal Cons: - The amount of memory allocations and memory space consumed is not optimal
b. The full 7-patch series? Pros: - The same backend is used for both the Kernel and IOCTL. Cons: - The code gets more complex. - Extending the Kernel interface can get complicated due to the fact that it shares code with the IOCTL interface. - I didn't really like this solution.
c. The alternative implementation I wrote? Pros: - It's independent from the IOCTL code, making further patches easier. Cons: - There is some duplicated code.
d. Something totally different from these alternatives?
2. How should the driver interface look like?
a. All the possibilities are passed through the function call, so the drm.ko code needs to set the struct members itself. b. The caller already sets the struct members instead of passing them as parameters to the function. c. Something different?
All these patches are based on top of Daniel Vetter's drm-intel-nightly tree. It's all just an RFC, so don't expect something very well tested.
Flamewars and bikeshedding are welcome!
Thanks, Paulo
Paulo Zanoni (7):
- First implementation: drm: allow the drivers to call the vblank IOCTL internally drm: allow drm_wait_vblank_kernel() callback from workqueues drm: introduce struct drm_vblank_wait_item drm: add wanted_seq to drm_vblank_wait_item drm: change the drm vblank callback item type drm: make vblank_event_list handle drm_vblank_wait_item types drm: make the callers of drm_wait_vblank() allocate the memory
- Alternative implementation: drm: add a mechanism for drivers to schedule vblank callbacks
Stats for only the first implementation:
drivers/gpu/drm/drm_fops.c | 15 ++- drivers/gpu/drm/drm_ioctl.c | 2 +- drivers/gpu/drm/drm_irq.c | 178 ++++++++++++++++++++++++++---------- drivers/gpu/drm/i915/i915_debugfs.c | 90 ++++++++++++++++++ include/drm/drmP.h | 35 ++++++- 5 files changed, 264 insertions(+), 56 deletions(-)
From: Paulo Zanoni paulo.r.zanoni@intel.com
The drivers need a way to schedule functions to be ran after a certain number of vblanks. The i915.ko driver has plenty of examples where this could be used, such as for unpinning buffers after a modeset. Since the vblank wait IOCTL does basically what we want, but for the user space, in this patch we add a new list of vblank events (dev->vblank_kernel_list) and handle it exactly like we handle the user space events.
Signed-off-by: Paulo Zanoni paulo.r.zanoni@intel.com --- drivers/gpu/drm/drm_drv.c | 1 + drivers/gpu/drm/drm_irq.c | 106 ++++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/i915_debugfs.c | 81 +++++++++++++++++++++++++++ include/drm/drmP.h | 22 ++++++++ 4 files changed, 206 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 2e5c7d9..b5ae6c8 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -557,6 +557,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, INIT_LIST_HEAD(&dev->vmalist); INIT_LIST_HEAD(&dev->maplist); INIT_LIST_HEAD(&dev->vblank_event_list); + INIT_LIST_HEAD(&dev->vblank_kernel_list);
spin_lock_init(&dev->buf_lock); spin_lock_init(&dev->event_lock); diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 0e47df4..6e04035 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1107,6 +1107,13 @@ void drm_crtc_wait_one_vblank(struct drm_crtc *crtc) } EXPORT_SYMBOL(drm_crtc_wait_one_vblank);
+static void send_kernel_vblank_event(struct drm_kernel_vblank_item *item) +{ + if (item->callback_from_work) + schedule_work(&item->work); + else + item->callback(item); +} /** * drm_vblank_off - disable vblank events on a CRTC * @dev: DRM device @@ -1124,7 +1131,8 @@ EXPORT_SYMBOL(drm_crtc_wait_one_vblank); 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_pending_vblank_event *e, *et; + struct drm_kernel_vblank_item *i, *it; struct timeval now; unsigned long irqflags; unsigned int seq; @@ -1151,7 +1159,7 @@ 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) { + list_for_each_entry_safe(e, et, &dev->vblank_event_list, base.link) { if (e->pipe != crtc) continue; DRM_DEBUG("Sending premature vblank event on disable: \ @@ -1161,6 +1169,21 @@ void drm_vblank_off(struct drm_device *dev, int crtc) drm_vblank_put(dev, e->pipe); send_vblank_event(dev, e, seq, &now); } + + list_for_each_entry_safe(i, it, &dev->vblank_kernel_list, link) { + int pipe = drm_crtc_index(i->crtc); + + if (pipe != crtc) + continue; + + DRM_DEBUG("Sending premature kernel vblank event on disable: \ + wanted %d, current %d\n", + i->target_seq, seq); + i->premature = true; + list_del(&i->link); + drm_vblank_put(dev, pipe); + send_kernel_vblank_event(i); + } spin_unlock_irqrestore(&dev->event_lock, irqflags); } EXPORT_SYMBOL(drm_vblank_off); @@ -1560,9 +1583,68 @@ done: return ret; }
+static void drm_kernel_vblank_work_func(struct work_struct *work) +{ + struct drm_kernel_vblank_item *item = + container_of(work, struct drm_kernel_vblank_item, work); + + item->callback(item); +} + +int drm_wait_vblank_kernel(struct drm_kernel_vblank_item *item) +{ + struct drm_crtc *crtc = item->crtc; + struct drm_device *dev = crtc->dev; + int pipe = drm_crtc_index(crtc); + struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; + unsigned int seq; + unsigned long irqflags; + int ret = 0; + + if (!dev->irq_enabled) + return -EINVAL; + + if (item->callback_from_work) + INIT_WORK(&item->work, drm_kernel_vblank_work_func); + + ret = drm_vblank_get(dev, pipe); + if (ret) { + DRM_DEBUG("failed to acquire vblank counter, %d\n", ret); + return ret; + } + + seq = drm_vblank_count(dev, pipe); + if (!item->absolute) + item->target_seq += seq; + + spin_lock_irqsave(&dev->event_lock, irqflags); + + if (!vblank->enabled) { + ret = -EINVAL; + goto out; + } + + if (seq - item->target_seq <= (1 << 23)) { + drm_vblank_put(dev, pipe); + send_kernel_vblank_event(item); + } else { + list_add_tail(&item->link, &dev->vblank_kernel_list); + } + + spin_unlock_irqrestore(&dev->event_lock, irqflags); + return 0; + +out: + spin_unlock_irqrestore(&dev->event_lock, irqflags); + drm_vblank_put(dev, pipe); + return ret; +} +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_pending_vblank_event *e, *et; + struct drm_kernel_vblank_item *i, *it; struct timeval now; unsigned int seq;
@@ -1570,7 +1652,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) { + list_for_each_entry_safe(e, et, &dev->vblank_event_list, base.link) { if (e->pipe != crtc) continue; if ((seq - e->event.sequence) > (1<<23)) @@ -1584,6 +1666,22 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc) send_vblank_event(dev, e, seq, &now); }
+ list_for_each_entry_safe(i, it, &dev->vblank_kernel_list, link) { + int pipe = drm_crtc_index(i->crtc); + + if (pipe != crtc) + continue; + if ((seq - i->target_seq) > (1<<23)) + continue; + + DRM_DEBUG("kernel vblank event on %d, current %d\n", + i->target_seq, seq); + + list_del(&i->link); + drm_vblank_put(dev, pipe); + send_kernel_vblank_event(i); + } + 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 319da61..e705976 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2715,6 +2715,86 @@ static int i915_ddb_info(struct seq_file *m, void *unused) return 0; }
+struct vblank_data { + int eight; + const char *message; + struct drm_kernel_vblank_item item; +}; + +static void vblank_callback(struct drm_kernel_vblank_item *item) +{ + struct vblank_data *data = container_of(item, struct vblank_data, item); + + WARN_ON(data->eight != 8); + WARN_ON(item->callback_from_work != drm_can_sleep()); + DRM_DEBUG_KMS("vblank callback, premature: %s, message: %s\n", + yesno(item->premature), data->message); + + kfree(data); +} + +static int i915_vblank_queue_test_new(struct seq_file *m, void *unused) +{ + struct drm_info_node *node = m->private; + struct drm_device *dev = node->minor->dev; + struct intel_crtc *crtc = NULL; + int ret = 0; + struct vblank_data *data1, *data2; + + for_each_intel_crtc(dev, crtc) + if (crtc->pipe == PIPE_A) + break; + if (WARN_ON(!crtc)) + return -EINVAL; + + data1 = kzalloc(sizeof *data1, GFP_KERNEL); + if (!data1) + return -ENOMEM; + + data2 = kzalloc(sizeof *data2, GFP_KERNEL); + if (!data2) { + kfree(data1); + return -ENOMEM; + } + + data1->message = "hello world (atomic)"; + data1->eight = 8; + data1->item.crtc = &crtc->base; + data1->item.target_seq = 60; + data1->item.absolute = false; + data1->item.callback = vblank_callback; + data1->item.callback_from_work = false; + + data2->message = "hello world (work)"; + data2->eight = 8; + data2->item.crtc = &crtc->base; + data2->item.target_seq = 60; + data2->item.absolute = false; + data2->item.callback = vblank_callback; + data2->item.callback_from_work = true; + + DRM_DEBUG_KMS("scheduling 60 vblanks (no work)\n"); + ret = drm_wait_vblank_kernel(&data1->item); + 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 work)\n"); + ret = drm_wait_vblank_kernel(&data2->item); + if (ret) { + DRM_DEBUG_KMS("vblank schedule error: %d\n", ret); + kfree(data2); + return ret; + } + DRM_DEBUG_KMS("vblanks scheduled\n"); + + return 0; +} + struct pipe_crc_info { const char *name; struct drm_device *dev; @@ -4289,6 +4369,7 @@ static const struct drm_info_list i915_debugfs_list[] = { {"i915_dp_mst_info", i915_dp_mst_info, 0}, {"i915_wa_registers", i915_wa_registers, 0}, {"i915_ddb_info", i915_ddb_info, 0}, + {"i915_vblank_queue_test_new", i915_vblank_queue_test_new, 0}, }; #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index be776fb..295d0e0 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -667,6 +667,26 @@ struct drm_pending_vblank_event { struct drm_event_vblank event; };
+struct drm_kernel_vblank_item { + /* Internal members, set by drm.ko. */ + struct list_head link; + struct work_struct work; + + /* Parameters: set by the caller of drm_wait_vblank_kernel(). */ + struct drm_crtc *crtc; + + int target_seq; + bool absolute; /* Tells if target_seq is a relative offset to the + * current vblank count, or just an absolute value. */ + + void (*callback)(struct drm_kernel_vblank_item *item); + + bool callback_from_work; + + /* Output: to be used by the callback function. */ + bool premature; +}; + struct drm_vblank_crtc { struct drm_device *dev; /* pointer to the drm_device */ wait_queue_head_t queue; /**< VBLANK wait queue */ @@ -784,6 +804,7 @@ struct drm_device { * List of events */ struct list_head vblank_event_list; + struct list_head vblank_kernel_list; spinlock_t event_lock;
/*@} */ @@ -900,6 +921,7 @@ extern int drm_irq_uninstall(struct drm_device *dev); extern int drm_vblank_init(struct drm_device *dev, int num_crtcs); extern int drm_wait_vblank(struct drm_device *dev, void *data, struct drm_file *filp); +extern int drm_wait_vblank_kernel(struct drm_kernel_vblank_item *item); extern u32 drm_vblank_count(struct drm_device *dev, int crtc); extern u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, struct timeval *vblanktime);
On Wed, Nov 19, 2014 at 05:47:08PM -0200, Paulo Zanoni wrote:
From: Paulo Zanoni paulo.r.zanoni@intel.com
The drivers need a way to schedule functions to be ran after a certain number of vblanks. The i915.ko driver has plenty of examples where this could be used, such as for unpinning buffers after a modeset. Since the vblank wait IOCTL does basically what we want, but for the user space, in this patch we add a new list of vblank events (dev->vblank_kernel_list) and handle it exactly like we handle the user space events.
Signed-off-by: Paulo Zanoni paulo.r.zanoni@intel.com
drivers/gpu/drm/drm_drv.c | 1 + drivers/gpu/drm/drm_irq.c | 106 ++++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/i915_debugfs.c | 81 +++++++++++++++++++++++++++ include/drm/drmP.h | 22 ++++++++ 4 files changed, 206 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 2e5c7d9..b5ae6c8 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -557,6 +557,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, INIT_LIST_HEAD(&dev->vmalist); INIT_LIST_HEAD(&dev->maplist); INIT_LIST_HEAD(&dev->vblank_event_list);
INIT_LIST_HEAD(&dev->vblank_kernel_list);
spin_lock_init(&dev->buf_lock); spin_lock_init(&dev->event_lock);
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 0e47df4..6e04035 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1107,6 +1107,13 @@ void drm_crtc_wait_one_vblank(struct drm_crtc *crtc) } EXPORT_SYMBOL(drm_crtc_wait_one_vblank);
+static void send_kernel_vblank_event(struct drm_kernel_vblank_item *item)
Minor nitpick, but it might be good to avoid the word 'event' here. If I were skimming the code and saw this function name, my first impression would be that this had something to do with drm_event's, which are a userspace fd concept. I think a function name like 'do_vblank_callback()' or something would avoid that potential confusion.
+{
- if (item->callback_from_work)
schedule_work(&item->work);- else
item->callback(item);+}
Would callers potentially want to be able to provide their own workqueue? In i915 we use some private workqueues for unpinning on pageflip completion and such. Maybe rather than a boolean flag here you can just pass the workqueue to do queue_work() on and just call the callback directly if it's NULL?
/**
- drm_vblank_off - disable vblank events on a CRTC
- @dev: DRM device
@@ -1124,7 +1131,8 @@ EXPORT_SYMBOL(drm_crtc_wait_one_vblank); 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_pending_vblank_event *e, *et;
- struct drm_kernel_vblank_item *i, *it; struct timeval now; unsigned long irqflags; unsigned int seq;
@@ -1151,7 +1159,7 @@ 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) {
- list_for_each_entry_safe(e, et, &dev->vblank_event_list, base.link) { if (e->pipe != crtc) continue; DRM_DEBUG("Sending premature vblank event on disable: \
@@ -1161,6 +1169,21 @@ void drm_vblank_off(struct drm_device *dev, int crtc) drm_vblank_put(dev, e->pipe); send_vblank_event(dev, e, seq, &now); }
- list_for_each_entry_safe(i, it, &dev->vblank_kernel_list, link) {
int pipe = drm_crtc_index(i->crtc);if (pipe != crtc)continue;DRM_DEBUG("Sending premature kernel vblank event on disable: \wanted %d, current %d\n",i->target_seq, seq);i->premature = true;list_del(&i->link);drm_vblank_put(dev, pipe);send_kernel_vblank_event(i);- } spin_unlock_irqrestore(&dev->event_lock, irqflags);
} EXPORT_SYMBOL(drm_vblank_off); @@ -1560,9 +1583,68 @@ done: return ret; }
+static void drm_kernel_vblank_work_func(struct work_struct *work) +{
- struct drm_kernel_vblank_item *item =
container_of(work, struct drm_kernel_vblank_item, work);- item->callback(item);
+}
+int drm_wait_vblank_kernel(struct drm_kernel_vblank_item *item) +{
Another naming nitpick...at first glance the name here sounds like a blocking function call. Maybe something like drm_schedule_for_vblank() would make the asynchronous nature more clear?
You mentioned in your cover letter that you were trying to decide between passing a pre-filled struct containing all the task details (as you do in your implementation here) vs passing them as parameters to the function. Personally I find the pre-filled structure approach a little bit less intuitive. It seems like some of the fields you're filling in below are only used by this function (e.g., "absolute"), so it seems like at least those fields could be easily converted to parameters.
Finally, you'll need some kerneldoc here eventually. You should make sure to note that the callback function is responsible for freeing the item structure.
- struct drm_crtc *crtc = item->crtc;
- struct drm_device *dev = crtc->dev;
- int pipe = drm_crtc_index(crtc);
- struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
- unsigned int seq;
- unsigned long irqflags;
- int ret = 0;
- if (!dev->irq_enabled)
return -EINVAL;
It's not immediately clear to me why we need to bail in this case? Obviously you won't actually get any vblank interrupts while irq's are disabled, but if the driver knows it's going to reenable interrupts soon, I don't see why it can't get the callback setup while they're off. We don't call anything here that takes a mutex or can sleep do we?
- if (item->callback_from_work)
INIT_WORK(&item->work, drm_kernel_vblank_work_func);- ret = drm_vblank_get(dev, pipe);
- if (ret) {
DRM_DEBUG("failed to acquire vblank counter, %d\n", ret);return ret;- }
- seq = drm_vblank_count(dev, pipe);
- if (!item->absolute)
As noted above, this seems more appropriate as a parameter. But maybe a general 'flags' parameter would be best so that we can easily add additional bits in the future.
Matt
item->target_seq += seq;- spin_lock_irqsave(&dev->event_lock, irqflags);
- if (!vblank->enabled) {
ret = -EINVAL;goto out;- }
- if (seq - item->target_seq <= (1 << 23)) {
drm_vblank_put(dev, pipe);send_kernel_vblank_event(item);- } else {
list_add_tail(&item->link, &dev->vblank_kernel_list);- }
- spin_unlock_irqrestore(&dev->event_lock, irqflags);
- return 0;
+out:
- spin_unlock_irqrestore(&dev->event_lock, irqflags);
- drm_vblank_put(dev, pipe);
- return ret;
+} +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_pending_vblank_event *e, *et;
- struct drm_kernel_vblank_item *i, *it; struct timeval now; unsigned int seq;
@@ -1570,7 +1652,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) {
- list_for_each_entry_safe(e, et, &dev->vblank_event_list, base.link) { if (e->pipe != crtc) continue; if ((seq - e->event.sequence) > (1<<23))
@@ -1584,6 +1666,22 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc) send_vblank_event(dev, e, seq, &now); }
- list_for_each_entry_safe(i, it, &dev->vblank_kernel_list, link) {
int pipe = drm_crtc_index(i->crtc);if (pipe != crtc)continue;if ((seq - i->target_seq) > (1<<23))continue;DRM_DEBUG("kernel vblank event on %d, current %d\n",i->target_seq, seq);list_del(&i->link);drm_vblank_put(dev, pipe);send_kernel_vblank_event(i);- }
- 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 319da61..e705976 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2715,6 +2715,86 @@ static int i915_ddb_info(struct seq_file *m, void *unused) return 0; }
+struct vblank_data {
- int eight;
- const char *message;
- struct drm_kernel_vblank_item item;
+};
+static void vblank_callback(struct drm_kernel_vblank_item *item) +{
- struct vblank_data *data = container_of(item, struct vblank_data, item);
- WARN_ON(data->eight != 8);
- WARN_ON(item->callback_from_work != drm_can_sleep());
- DRM_DEBUG_KMS("vblank callback, premature: %s, message: %s\n",
yesno(item->premature), data->message);- kfree(data);
+}
+static int i915_vblank_queue_test_new(struct seq_file *m, void *unused) +{
- struct drm_info_node *node = m->private;
- struct drm_device *dev = node->minor->dev;
- struct intel_crtc *crtc = NULL;
- int ret = 0;
- struct vblank_data *data1, *data2;
- for_each_intel_crtc(dev, crtc)
if (crtc->pipe == PIPE_A)break;- if (WARN_ON(!crtc))
return -EINVAL;- data1 = kzalloc(sizeof *data1, GFP_KERNEL);
- if (!data1)
return -ENOMEM;- data2 = kzalloc(sizeof *data2, GFP_KERNEL);
- if (!data2) {
kfree(data1);return -ENOMEM;- }
- data1->message = "hello world (atomic)";
- data1->eight = 8;
- data1->item.crtc = &crtc->base;
- data1->item.target_seq = 60;
- data1->item.absolute = false;
- data1->item.callback = vblank_callback;
- data1->item.callback_from_work = false;
- data2->message = "hello world (work)";
- data2->eight = 8;
- data2->item.crtc = &crtc->base;
- data2->item.target_seq = 60;
- data2->item.absolute = false;
- data2->item.callback = vblank_callback;
- data2->item.callback_from_work = true;
- DRM_DEBUG_KMS("scheduling 60 vblanks (no work)\n");
- ret = drm_wait_vblank_kernel(&data1->item);
- 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 work)\n");
- ret = drm_wait_vblank_kernel(&data2->item);
- if (ret) {
DRM_DEBUG_KMS("vblank schedule error: %d\n", ret);kfree(data2);return ret;- }
- DRM_DEBUG_KMS("vblanks scheduled\n");
- return 0;
+}
struct pipe_crc_info { const char *name; struct drm_device *dev; @@ -4289,6 +4369,7 @@ static const struct drm_info_list i915_debugfs_list[] = { {"i915_dp_mst_info", i915_dp_mst_info, 0}, {"i915_wa_registers", i915_wa_registers, 0}, {"i915_ddb_info", i915_ddb_info, 0},
- {"i915_vblank_queue_test_new", i915_vblank_queue_test_new, 0},
}; #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index be776fb..295d0e0 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -667,6 +667,26 @@ struct drm_pending_vblank_event { struct drm_event_vblank event; };
+struct drm_kernel_vblank_item {
- /* Internal members, set by drm.ko. */
- struct list_head link;
- struct work_struct work;
- /* Parameters: set by the caller of drm_wait_vblank_kernel(). */
- struct drm_crtc *crtc;
- int target_seq;
- bool absolute; /* Tells if target_seq is a relative offset to the
* current vblank count, or just an absolute value. */- void (*callback)(struct drm_kernel_vblank_item *item);
- bool callback_from_work;
- /* Output: to be used by the callback function. */
- bool premature;
+};
struct drm_vblank_crtc { struct drm_device *dev; /* pointer to the drm_device */ wait_queue_head_t queue; /**< VBLANK wait queue */ @@ -784,6 +804,7 @@ struct drm_device { * List of events */ struct list_head vblank_event_list;
struct list_head vblank_kernel_list; spinlock_t event_lock;
/*@} */
@@ -900,6 +921,7 @@ extern int drm_irq_uninstall(struct drm_device *dev); extern int drm_vblank_init(struct drm_device *dev, int num_crtcs); extern int drm_wait_vblank(struct drm_device *dev, void *data, struct drm_file *filp); +extern int drm_wait_vblank_kernel(struct drm_kernel_vblank_item *item); extern u32 drm_vblank_count(struct drm_device *dev, int crtc); extern u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, struct timeval *vblanktime); -- 2.1.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
From: Paulo Zanoni paulo.r.zanoni@intel.com
The i915.ko driver needs a way to schedule certain functions to run after some amount of vblanks. There are many different pieces of the driver which could benefit from that.
Since what we want is essentially the vblank ioctl, this patch does the minimum change required to allow this ioctl to be called internally. The noticeable thing here is that the drivers pass a callback function, which is called by drm.ko after the specified amount of vblanks passes.
The great benefit of this minimal change is that all the code responsible for taking care of properly emptying the queues (e.g., when the CRTC is disabled) is already there, so we don't need to rewrite it.
The current wait vblank IOCTL is now implemented on top of these changes, and it provides its own callback: send_vblank_event().
Signed-off-by: Paulo Zanoni paulo.r.zanoni@intel.com --- drivers/gpu/drm/drm_ioctl.c | 2 +- drivers/gpu/drm/drm_irq.c | 65 +++++++++++++++++++++++++++++-------- drivers/gpu/drm/i915/i915_debugfs.c | 62 +++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 13 ++++++-- 4 files changed, 125 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 00587a1..fb35173 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -578,7 +578,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_SG_ALLOC, drm_legacy_sg_alloc, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_SG_FREE, drm_legacy_sg_free, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
- DRM_IOCTL_DEF(DRM_IOCTL_WAIT_VBLANK, drm_wait_vblank, DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_WAIT_VBLANK, drm_wait_vblank_ioctl, DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODESET_CTL, drm_modeset_ctl, 0),
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 0e47df4..f031f77 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -882,7 +882,8 @@ EXPORT_SYMBOL(drm_vblank_count_and_time);
static void send_vblank_event(struct drm_device *dev, struct drm_pending_vblank_event *e, - unsigned long seq, struct timeval *now) + unsigned long seq, struct timeval *now, + bool premature) { WARN_ON_SMP(!spin_is_locked(&dev->event_lock)); e->event.sequence = seq; @@ -918,7 +919,7 @@ void drm_send_vblank_event(struct drm_device *dev, int crtc, now = get_drm_timestamp(); } e->pipe = crtc; - send_vblank_event(dev, e, seq, &now); + send_vblank_event(dev, e, seq, &now, false); } EXPORT_SYMBOL(drm_send_vblank_event);
@@ -1159,7 +1160,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); - send_vblank_event(dev, e, seq, &now); + e->callback(dev, e, seq, &now, true); } spin_unlock_irqrestore(&dev->event_lock, irqflags); } @@ -1373,7 +1374,8 @@ int drm_modeset_ctl(struct drm_device *dev, void *data,
static int drm_queue_vblank_event(struct drm_device *dev, int pipe, union drm_wait_vblank *vblwait, - struct drm_file *file_priv) + struct drm_file *file_priv, + drm_vblank_callback_t callback) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; struct drm_pending_vblank_event *e; @@ -1396,6 +1398,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->callback = callback;
spin_lock_irqsave(&dev->event_lock, flags);
@@ -1411,12 +1414,15 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe, goto err_unlock; }
- if (file_priv->event_space < sizeof e->event) { - ret = -EBUSY; - goto err_unlock; + if (file_priv) { + if (file_priv->event_space < sizeof e->event) { + ret = -EBUSY; + goto err_unlock; + } + + file_priv->event_space -= sizeof e->event; }
- file_priv->event_space -= sizeof e->event; seq = drm_vblank_count_and_time(dev, pipe, &now);
if ((vblwait->request.type & _DRM_VBLANK_NEXTONMISS) && @@ -1434,7 +1440,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); - send_vblank_event(dev, e, seq, &now); + e->callback(dev, e, seq, &now, false); vblwait->reply.sequence = seq; } else { /* drm_handle_vblank_events will call drm_vblank_put */ @@ -1468,11 +1474,12 @@ err_put: * the vblank interrupt refcount afterwards. (vblank IRQ disable follows that * after a timeout with no further vblank waits scheduled). */ -int drm_wait_vblank(struct drm_device *dev, void *data, - struct drm_file *file_priv) +static int __drm_wait_vblank(struct drm_device *dev, + union drm_wait_vblank *vblwait, + struct drm_file *file_priv, + drm_vblank_callback_t callback) { struct drm_vblank_crtc *vblank; - union drm_wait_vblank *vblwait = data; int ret; unsigned int flags, seq, crtc, high_crtc;
@@ -1525,7 +1532,8 @@ int drm_wait_vblank(struct drm_device *dev, void *data, /* must hold on to the vblank ref until the event fires * drm_vblank_put will be called asynchronously */ - return drm_queue_vblank_event(dev, crtc, vblwait, file_priv); + return drm_queue_vblank_event(dev, crtc, vblwait, file_priv, + callback); }
if ((flags & _DRM_VBLANK_NEXTONMISS) && @@ -1560,6 +1568,35 @@ done: return ret; }
+int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv) +{ + union drm_wait_vblank *vblwait = data; + + return __drm_wait_vblank(dev, vblwait, file_priv, send_vblank_event); +} + +int drm_wait_vblank_kernel(struct drm_crtc *crtc, int count, bool absolute, + drm_vblank_callback_t callback, + unsigned long user_data) +{ + struct drm_device *dev = crtc->dev; + union drm_wait_vblank vblwait; + int type = 0; + + type |= absolute ? _DRM_VBLANK_ABSOLUTE : _DRM_VBLANK_RELATIVE; + type |= drm_crtc_index(crtc) << _DRM_VBLANK_HIGH_CRTC_SHIFT; + if (callback) + type |= _DRM_VBLANK_EVENT; + + vblwait.request.type = type; + vblwait.request.sequence = count; + vblwait.request.signal = user_data; + + return __drm_wait_vblank(dev, &vblwait, NULL, callback); +} +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; @@ -1581,7 +1618,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
list_del(&e->base.link); drm_vblank_put(dev, e->pipe); - send_vblank_event(dev, e, seq, &now); + e->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 319da61..f989587 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2715,6 +2715,67 @@ static int i915_ddb_info(struct seq_file *m, void *unused) return 0; }
+struct vblank_data { + int eight; + const char *message; +}; + +static void vblank_callback(struct drm_device *dev, + struct drm_pending_vblank_event *e, + unsigned long seq, struct timeval *now, + bool premature) +{ + struct vblank_data *data = (struct vblank_data *)e->event.user_data; + + WARN_ON(data->eight != 8); + DRM_DEBUG_KMS("vblank callback, seq: %lu, premature: %s message:%s\n", + seq, yesno(premature), data->message); + + e->base.destroy(&e->base); + kfree(data); +} + +static int i915_vblank_queue_test_new(struct seq_file *m, void *unused) +{ + struct drm_info_node *node = m->private; + struct drm_device *dev = node->minor->dev; + struct intel_crtc *crtc = NULL; + int ret = 0; + struct vblank_data *data; + + for_each_intel_crtc(dev, crtc) + if (crtc->pipe == PIPE_A) + break; + if (WARN_ON(!crtc)) + return -EINVAL; + + data = kzalloc(sizeof *data, GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->message = "hello world"; + data->eight = 8; + + DRM_DEBUG_KMS("waiting 60 vblanks (no callback)\n"); + ret = drm_wait_vblank_kernel(&crtc->base, 60, false, NULL, 0); + if (ret) { + DRM_DEBUG_KMS("vblank wait error: %d\n", ret); + 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); + if (ret) { + DRM_DEBUG_KMS("vblank schedule error: %d\n", ret); + return ret; + } + DRM_DEBUG_KMS("vblanks scheduled\n"); + + return 0; +} + struct pipe_crc_info { const char *name; struct drm_device *dev; @@ -4289,6 +4350,7 @@ static const struct drm_info_list i915_debugfs_list[] = { {"i915_dp_mst_info", i915_dp_mst_info, 0}, {"i915_wa_registers", i915_wa_registers, 0}, {"i915_ddb_info", i915_ddb_info, 0}, + {"i915_vblank_queue_test_new", i915_vblank_queue_test_new, 0}, }; #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index be776fb..39d0d87 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -660,11 +660,16 @@ struct drm_minor { struct drm_mode_group mode_group; };
+typedef void (*drm_vblank_callback_t)(struct drm_device *dev, + struct drm_pending_vblank_event *e, + unsigned long seq, struct timeval *now, + bool premature);
struct drm_pending_vblank_event { struct drm_pending_event base; int pipe; struct drm_event_vblank event; + drm_vblank_callback_t callback; };
struct drm_vblank_crtc { @@ -898,8 +903,12 @@ extern int drm_irq_install(struct drm_device *dev, int irq); extern int drm_irq_uninstall(struct drm_device *dev);
extern int drm_vblank_init(struct drm_device *dev, int num_crtcs); -extern int drm_wait_vblank(struct drm_device *dev, void *data, - struct drm_file *filp); +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, + drm_vblank_callback_t callback, + unsigned long user_data); extern u32 drm_vblank_count(struct drm_device *dev, int crtc); extern u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, struct timeval *vblanktime);
On Wed, Nov 19, 2014 at 05:47:09PM -0200, Paulo Zanoni wrote:
From: Paulo Zanoni paulo.r.zanoni@intel.com
The i915.ko driver needs a way to schedule certain functions to run after some amount of vblanks. There are many different pieces of the driver which could benefit from that.
Since what we want is essentially the vblank ioctl, this patch does the minimum change required to allow this ioctl to be called internally. The noticeable thing here is that the drivers pass a callback function, which is called by drm.ko after the specified amount of vblanks passes.
The great benefit of this minimal change is that all the code responsible for taking care of properly emptying the queues (e.g., when the CRTC is disabled) is already there, so we don't need to rewrite it.
The current wait vblank IOCTL is now implemented on top of these changes, and it provides its own callback: send_vblank_event().
Signed-off-by: Paulo Zanoni paulo.r.zanoni@intel.com
...
+int drm_wait_vblank_kernel(struct drm_crtc *crtc, int count, bool absolute,
drm_vblank_callback_t callback,unsigned long user_data)+{
- struct drm_device *dev = crtc->dev;
- union drm_wait_vblank vblwait;
- int type = 0;
- type |= absolute ? _DRM_VBLANK_ABSOLUTE : _DRM_VBLANK_RELATIVE;
- type |= drm_crtc_index(crtc) << _DRM_VBLANK_HIGH_CRTC_SHIFT;
- if (callback)
type |= _DRM_VBLANK_EVENT;
Need some kerneldoc on this function. It looks like if we have a NULL callback this turns into a more general version of drm_wait_one_vblank() that can handle arbitrary delay counts, right? Is there a case where a multi-vblank wait would be useful internal to the kernel? If not, it might be worth just returning failure on that case for now until we have an actual caller.
Matt
From: Paulo Zanoni paulo.r.zanoni@intel.com
This is going to be needed by i915.ko, and I guess other drivers could use it too.
Signed-off-by: Paulo Zanoni paulo.r.zanoni@intel.com --- 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); + } 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);
On Wed, Nov 19, 2014 at 05:47:10PM -0200, Paulo Zanoni wrote:
From: Paulo Zanoni paulo.r.zanoni@intel.com
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 paulo.r.zanoni@intel.com
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 Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
From: Paulo Zanoni paulo.r.zanoni@intel.com
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 paulo.r.zanoni@intel.com --- 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(-)
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 */
On Wed, Nov 19, 2014 at 05:47:11PM -0200, Paulo Zanoni wrote:
From: Paulo Zanoni paulo.r.zanoni@intel.com
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 paulo.r.zanoni@intel.com
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 dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
From: Paulo Zanoni paulo.r.zanoni@intel.com
Store the wanted sequence in the wait_item instead of storing it in the event structure that is eventually going to be sent to user space. The plan is to make Kernel vblank wait items not have the user space event, so we need to store the wanted sequence number somewhere.
It is not a problem that we're not filling e->event.sequence inside drm_queue_vblank_event: we set the value again inside send_vblank_event().
Signed-off-by: Paulo Zanoni paulo.r.zanoni@intel.com --- drivers/gpu/drm/drm_irq.c | 8 ++++---- include/drm/drmP.h | 1 + 2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 7dcbbdb..a82e5ca 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1173,7 +1173,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) continue; DRM_DEBUG("Sending premature vblank event on disable: \ wanted %d, current %d\n", - e->event.sequence, seq); + e->item.wanted_seq, seq); list_del(&e->base.link); drm_vblank_put(dev, e->item.pipe); drm_wait_vblank_callback(dev, e, seq, &now, true); @@ -1469,7 +1469,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe, trace_drm_vblank_event_queued(current->pid, pipe, vblwait->request.sequence);
- e->event.sequence = vblwait->request.sequence; + e->item.wanted_seq = vblwait->request.sequence; if ((seq - vblwait->request.sequence) <= (1 << 23)) { drm_vblank_put(dev, pipe); drm_wait_vblank_callback(dev, e, seq, &now, false); @@ -1646,11 +1646,11 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc) list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) { if (e->item.pipe != crtc) continue; - if ((seq - e->event.sequence) > (1<<23)) + if ((seq - e->item.wanted_seq) > (1<<23)) continue;
DRM_DEBUG("vblank event on %d, current %d\n", - e->event.sequence, seq); + e->item.wanted_seq, seq);
list_del(&e->base.link); drm_vblank_put(dev, e->item.pipe); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index b8bc55a..dcec05b 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -667,6 +667,7 @@ typedef void (*drm_vblank_callback_t)(struct drm_device *dev,
struct drm_vblank_wait_item { int pipe; + unsigned int wanted_seq;
drm_vblank_callback_t callback; bool callback_from_work;
From: Paulo Zanoni paulo.r.zanoni@intel.com
Now that we have created drm_vblank_wait_item, let's use it as the type passed. In the future, callers will have to use container_of to find our their original allocated structure, just like we're doing with the send_vblank_event() callback.
Signed-off-by: Paulo Zanoni paulo.r.zanoni@intel.com --- drivers/gpu/drm/drm_irq.c | 40 ++++++++++++++++++------------------- drivers/gpu/drm/i915/i915_debugfs.c | 4 +++- include/drm/drmP.h | 4 +++- 3 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a82e5ca..4c03240 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -881,10 +881,13 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, EXPORT_SYMBOL(drm_vblank_count_and_time);
static void send_vblank_event(struct drm_device *dev, - struct drm_pending_vblank_event *e, + struct drm_vblank_wait_item *item, unsigned long seq, struct timeval *now, bool premature) { + struct drm_pending_vblank_event *e = + container_of(item, struct drm_pending_vblank_event, item); + WARN_ON_SMP(!spin_is_locked(&dev->event_lock)); e->event.sequence = seq; e->event.tv_sec = now->tv_sec; @@ -919,7 +922,7 @@ void drm_send_vblank_event(struct drm_device *dev, int crtc, now = get_drm_timestamp(); } e->item.pipe = crtc; - send_vblank_event(dev, e, seq, &now, false); + send_vblank_event(dev, &e->item, seq, &now, false); } EXPORT_SYMBOL(drm_send_vblank_event);
@@ -1109,18 +1112,18 @@ 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, + struct drm_vblank_wait_item *item, unsigned long seq, struct timeval *now, bool premature) { - 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); + if (item->callback_from_work) { + item->callback_args.dev = dev; + item->callback_args.seq = seq; + item->callback_args.now = now; + item->callback_args.premature = premature; + schedule_work(&item->callback_work); } else { - e->item.callback(dev, e, seq, now, premature); + item->callback(dev, item, seq, now, premature); } }
@@ -1176,7 +1179,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) e->item.wanted_seq, seq); list_del(&e->base.link); drm_vblank_put(dev, e->item.pipe); - drm_wait_vblank_callback(dev, e, seq, &now, true); + drm_wait_vblank_callback(dev, &e->item, seq, &now, true); } spin_unlock_irqrestore(&dev->event_lock, irqflags); } @@ -1390,14 +1393,11 @@ int drm_modeset_ctl(struct drm_device *dev, void *data,
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, - item.callback_work); + struct drm_vblank_wait_item *item = + container_of(work, struct drm_vblank_wait_item, callback_work);
- e->item.callback(e->item.callback_args.dev, e, - e->item.callback_args.seq, - e->item.callback_args.now, - e->item.callback_args.premature); + item->callback(item->callback_args.dev, item, item->callback_args.seq, + item->callback_args.now, item->callback_args.premature); }
static int drm_queue_vblank_event(struct drm_device *dev, int pipe, @@ -1472,7 +1472,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe, e->item.wanted_seq = vblwait->request.sequence; if ((seq - vblwait->request.sequence) <= (1 << 23)) { drm_vblank_put(dev, pipe); - drm_wait_vblank_callback(dev, e, seq, &now, false); + drm_wait_vblank_callback(dev, &e->item, seq, &now, false); vblwait->reply.sequence = seq; } else { /* drm_handle_vblank_events will call drm_vblank_put */ @@ -1654,7 +1654,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
list_del(&e->base.link); drm_vblank_put(dev, e->item.pipe); - drm_wait_vblank_callback(dev, e, seq, &now, false); + drm_wait_vblank_callback(dev, &e->item, 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 95cf6d3..b5c3f81 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2722,10 +2722,12 @@ struct vblank_data { };
static void vblank_callback(struct drm_device *dev, - struct drm_pending_vblank_event *e, + struct drm_vblank_wait_item *item, unsigned long seq, struct timeval *now, bool premature) { + struct drm_pending_vblank_event *e = + container_of(item, struct drm_pending_vblank_event, item); struct vblank_data *data = (struct vblank_data *)e->event.user_data;
WARN_ON(data->can_sleep != drm_can_sleep()); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index dcec05b..474c892 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -660,8 +660,10 @@ struct drm_minor { struct drm_mode_group mode_group; };
+struct drm_vblank_wait_item; + typedef void (*drm_vblank_callback_t)(struct drm_device *dev, - struct drm_pending_vblank_event *e, + struct drm_vblank_wait_item *item, unsigned long seq, struct timeval *now, bool premature);
From: Paulo Zanoni paulo.r.zanoni@intel.com
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 paulo.r.zanoni@intel.com --- 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;
On Wed, Nov 19, 2014 at 05:47:14PM -0200, Paulo Zanoni wrote:
From: Paulo Zanoni paulo.r.zanoni@intel.com
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 paulo.r.zanoni@intel.com
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);
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;
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);} @@ -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);@@ -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 Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
From: Paulo Zanoni paulo.r.zanoni@intel.com
This way, the Kernel users will be able to fully control the lifetime of struct drm_vblank_wait_item, and they will also be able to wrap it to store their own information. As a result, one less memory allocation will happen, and the Kernel codepath will not set all those drm_pending_vblank_event struct fields.
Signed-off-by: Paulo Zanoni paulo.r.zanoni@intel.com --- drivers/gpu/drm/drm_irq.c | 92 ++++++++++++++++++++----------------- drivers/gpu/drm/i915/i915_debugfs.c | 11 ++--- include/drm/drmP.h | 2 +- 3 files changed, 57 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index dd091c3..5fa5431 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1402,36 +1402,26 @@ static void drm_vblank_event_work_func(struct work_struct *work)
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) + drm_vblank_callback_t callback, + struct drm_vblank_wait_item *item) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; - struct drm_pending_vblank_event *e; struct timeval now; unsigned long flags; unsigned int seq; int ret;
- e = kzalloc(sizeof *e, GFP_KERNEL); - if (e == NULL) { - ret = -ENOMEM; + if (WARN_ON(!item)) { + ret = -EINVAL; goto err_put; }
- e->item.pipe = pipe; - e->base.pid = current->pid; - 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; - e->item.from_user_space = true; - e->item.callback = callback; - e->item.callback_from_work = callback_from_work; + item->pipe = pipe; + item->callback = callback; + item->callback_from_work = callback_from_work; if (callback_from_work) - INIT_WORK(&e->item.callback_work, drm_vblank_event_work_func); + INIT_WORK(&item->callback_work, drm_vblank_event_work_func);
spin_lock_irqsave(&dev->event_lock, flags);
@@ -1447,15 +1437,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe, goto err_unlock; }
- if (file_priv) { - if (file_priv->event_space < sizeof e->event) { - ret = -EBUSY; - 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) && @@ -1470,14 +1451,14 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe, trace_drm_vblank_event_queued(current->pid, pipe, vblwait->request.sequence);
- e->item.wanted_seq = vblwait->request.sequence; + item->wanted_seq = vblwait->request.sequence; if ((seq - vblwait->request.sequence) <= (1 << 23)) { drm_vblank_put(dev, pipe); - drm_wait_vblank_callback(dev, &e->item, seq, &now, false); + drm_wait_vblank_callback(dev, item, seq, &now, false); vblwait->reply.sequence = seq; } else { /* drm_handle_vblank_events will call drm_vblank_put */ - list_add_tail(&e->item.link, &dev->vblank_event_list); + list_add_tail(&item->link, &dev->vblank_event_list); vblwait->reply.sequence = vblwait->request.sequence; }
@@ -1487,7 +1468,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
err_unlock: spin_unlock_irqrestore(&dev->event_lock, flags); - kfree(e); err_put: drm_vblank_put(dev, pipe); return ret; @@ -1509,9 +1489,9 @@ 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) + drm_vblank_callback_t callback, + struct drm_vblank_wait_item *item) { struct drm_vblank_crtc *vblank; int ret; @@ -1566,8 +1546,9 @@ static int __drm_wait_vblank(struct drm_device *dev, /* must hold on to the vblank ref until the event fires * drm_vblank_put will be called asynchronously */ - return drm_queue_vblank_event(dev, crtc, vblwait, file_priv, - callback_from_work, callback); + return drm_queue_vblank_event(dev, crtc, vblwait, + callback_from_work, callback, + item); }
if ((flags & _DRM_VBLANK_NEXTONMISS) && @@ -1606,15 +1587,44 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { union drm_wait_vblank *vblwait = data; + struct drm_pending_vblank_event *e = NULL; + unsigned int flags = vblwait->request.type & _DRM_VBLANK_FLAGS_MASK; + int ret; + + if (flags & _DRM_VBLANK_EVENT) { + e = kzalloc(sizeof *e, GFP_KERNEL); + if (e == NULL) + return -ENOMEM; + + e->base.pid = current->pid; + 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; + e->item.from_user_space = true;
- return __drm_wait_vblank(dev, vblwait, file_priv, false, - send_vblank_event); + if (file_priv->event_space < sizeof e->event) { + kfree(e); + return -EBUSY; + } + file_priv->event_space -= sizeof e->event; + } + + ret = __drm_wait_vblank(dev, vblwait, false, send_vblank_event, + &e->item); + + if (ret && e) + kfree(e); + + return ret; }
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) + struct drm_vblank_wait_item *item) { struct drm_device *dev = crtc->dev; union drm_wait_vblank vblwait; @@ -1627,10 +1637,10 @@ int drm_wait_vblank_kernel(struct drm_crtc *crtc, int count, bool absolute,
vblwait.request.type = type; vblwait.request.sequence = count; - vblwait.request.signal = user_data; + vblwait.request.signal = 0;
- return __drm_wait_vblank(dev, &vblwait, NULL, callback_from_work, - callback); + return __drm_wait_vblank(dev, &vblwait, callback_from_work, callback, + item); } EXPORT_SYMBOL(drm_wait_vblank_kernel);
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index b5c3f81..d2f3ca9 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2719,6 +2719,7 @@ struct vblank_data { int eight; const char *message; bool can_sleep; + struct drm_vblank_wait_item item; };
static void vblank_callback(struct drm_device *dev, @@ -2726,16 +2727,14 @@ static void vblank_callback(struct drm_device *dev, unsigned long seq, struct timeval *now, bool premature) { - struct drm_pending_vblank_event *e = - container_of(item, struct drm_pending_vblank_event, item); - struct vblank_data *data = (struct vblank_data *)e->event.user_data; + struct vblank_data *data = + container_of(item, struct vblank_data, item);
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);
- e->base.destroy(&e->base); kfree(data); }
@@ -2783,7 +2782,7 @@ static int i915_vblank_queue_test_new(struct seq_file *m, void *unused)
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); + vblank_callback, &data1->item); if (ret) { DRM_DEBUG_KMS("vblank schedule error: %d\n", ret); kfree(data1); @@ -2794,7 +2793,7 @@ static int i915_vblank_queue_test_new(struct seq_file *m, void *unused)
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); + vblank_callback, &data2->item); if (ret) { DRM_DEBUG_KMS("vblank schedule error: %d\n", ret); kfree(data2); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 46724d9..55d73d0 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -926,7 +926,7 @@ extern int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, extern 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); + struct drm_vblank_wait_item *item); extern u32 drm_vblank_count(struct drm_device *dev, int crtc); extern u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, struct timeval *vblanktime);
On Wed, Nov 19, 2014 at 05:47:07PM -0200, Paulo Zanoni wrote:
From: Paulo Zanoni paulo.r.zanoni@intel.com
Hi
TL;DR summary: I wrote patches. Help me choose the best implementation and interface.
So the i915.ko driver could use some mechanism to run functions after a given number of vblanks. Implementations for this mechanism were already proposed in the past (by Chris Wilson and others), but they were i915-specific instead of a generic drm.ko implementation. We even had patches that make use of this new mechanism.
Since this is very similar to the vblank IOCTL we currently have, but with the caller being a Kernel module instead of user space, I decided to write an implementation that tries to reuse the current IOCTL infrastructure.
In the first patch we add all the necessary code to allow the modules to call the vblank ioctl from Kernel space: they provide a callback function that is going to be called instead of the traditional send_vblank_event(), which means the Kernel callers don't have to deal with the file descriptors and DRM events.
In the second patch we add a simple extension of the feature, to allow the drivers to have their callbacks called in a non-atomic context.
In all the other subsequent patches we rework the underlying code so that the common aspects between the user space vblank IOCTL and the Kernel interface are all stored in a single structure (struct drm_vblank_wait_item), and then anything that is specific to the different users is stored in a structure that contains struct drm_vblank_wait_item. This way, most of the drm.ko code only needs to deal with struct drm_vblank_wait_item, not really knowing how it is wrapped. The advantage of this rework is that it reduces the number of memory allocations needed in some cases (from 2 to 1) and it also reduces the amount of memory used on each allocation.
But while this all sounds nice in the above description, I had the feeling that this adds a lot of complexity and makes the code not-so-beautiful. If we ever need more extensions/options to this feature, we're going to have to untangle the code even more from the IOCTL part. We also have a small "abstraction break" in change introduced in patch 6. And there's the risk that some of the reworks may have added a regression somewhere.
Based on that, I decided to also provide an alternative implementation. In this implementation we add a new dev->vblank_kernel_list instead of reusing dev->vblank_event_list, and add new code to handle that this. This new code is completely based on the code that handles dev->vblank_kernel_list, so there's some duplication here. On the other hand, since the in-Kernel infrastructure is not tied to the IOCTL structure, we can very easily grow and adapt the Kernel code without having to fight against the IOCTL code. And the risk of regressions is greatly reduced.
The second difference of this alternative implementation is the signature of the drm_wait_vblank_kernel() function. While it could be exactly the same as the one used in the other series, I decided to make it different so we can also choose which one to use. In the 7-patch series implementation, all the user needs to do is to allocate the structure, and call the function, properly setting all its arguments. Then the function is responsible for parsing the arguments and populating the structure based on that. On the alternative implementation, the user has to fill the structure with the appropriate arguments, and then call drm_wait_vblank_kernel() passing just the allocated structure as an argument.
If you notice, you will see that these patches add a new debugfs file to i915.ko. This file is just a very simple example user of the new interface, which I used while developing. If you have difficulty understanding the new interface, please also look at the i915/i915_debugfs.c diff. Of course, I plan to exclude this debugfs interface from the final to-be-merged patches.
So, in summary, we have a few things we need to discuss:
- Which implementation to use?
a. Just the 2 first patches of the 7-patch series? Pros: - the Kernel users basically call the vblank IOCTL - the code changes are minimal Cons: - The amount of memory allocations and memory space consumed is not optimal
b. The full 7-patch series? Pros: - The same backend is used for both the Kernel and IOCTL. Cons: - The code gets more complex. - Extending the Kernel interface can get complicated due to the fact that it shares code with the IOCTL interface. - I didn't really like this solution.
c. The alternative implementation I wrote? Pros: - It's independent from the IOCTL code, making further patches easier. Cons: - There is some duplicated code.
d. Something totally different from these alternatives?
- How should the driver interface look like?
a. All the possibilities are passed through the function call, so the drm.ko code needs to set the struct members itself. b. The caller already sets the struct members instead of passing them as parameters to the function. c. Something different?
All these patches are based on top of Daniel Vetter's drm-intel-nightly tree. It's all just an RFC, so don't expect something very well tested.
Flamewars and bikeshedding are welcome!
As we've discussed on irc a bit I still think cleaning up this rathole is beneficial. Getting the event handling and vblank callbacks right just in drivers seems to be really hard, so better to clean this up and unify things.
But like you've said patches are a lot of churn, so do you have a git branch somewhere for the lazy to look at?
Thanks, Daniel
Thanks, Paulo
Paulo Zanoni (7):
- First implementation:
drm: allow the drivers to call the vblank IOCTL internally drm: allow drm_wait_vblank_kernel() callback from workqueues drm: introduce struct drm_vblank_wait_item drm: add wanted_seq to drm_vblank_wait_item drm: change the drm vblank callback item type drm: make vblank_event_list handle drm_vblank_wait_item types drm: make the callers of drm_wait_vblank() allocate the memory
- Alternative implementation:
drm: add a mechanism for drivers to schedule vblank callbacks
Stats for only the first implementation:
drivers/gpu/drm/drm_fops.c | 15 ++- drivers/gpu/drm/drm_ioctl.c | 2 +- drivers/gpu/drm/drm_irq.c | 178 ++++++++++++++++++++++++++---------- drivers/gpu/drm/i915/i915_debugfs.c | 90 ++++++++++++++++++ include/drm/drmP.h | 35 ++++++- 5 files changed, 264 insertions(+), 56 deletions(-)
-- 2.1.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
TL;DR summary: I wrote patches. Help me choose the best implementation and interface.
So the i915.ko driver could use some mechanism to run functions after a given number of vblanks. Implementations for this mechanism were already proposed in the past (by Chris Wilson and others), but they were i915-specific instead of a generic drm.ko implementation. We even had patches that make use of this new mechanism.
Since this is very similar to the vblank IOCTL we currently have, but with the caller being a Kernel module instead of user space, I decided to write an implementation that tries to reuse the current IOCTL infrastructure.
In the first patch we add all the necessary code to allow the modules to call the vblank ioctl from Kernel space: they provide a callback function that is going to be called instead of the traditional send_vblank_event(), which means the Kernel callers don't have to deal with the file descriptors and DRM events.
In the second patch we add a simple extension of the feature, to allow the drivers to have their callbacks called in a non-atomic context.
In all the other subsequent patches we rework the underlying code so that the common aspects between the user space vblank IOCTL and the Kernel interface are all stored in a single structure (struct drm_vblank_wait_item), and then anything that is specific to the different users is stored in a structure that contains struct drm_vblank_wait_item. This way, most of the drm.ko code only needs to deal with struct drm_vblank_wait_item, not really knowing how it is wrapped. The advantage of this rework is that it reduces the number of memory allocations needed in some cases (from 2 to 1) and it also reduces the amount of memory used on each allocation.
But while this all sounds nice in the above description, I had the feeling that this adds a lot of complexity and makes the code not-so-beautiful. If we ever need more extensions/options to this feature, we're going to have to untangle the code even more from the IOCTL part. We also have a small "abstraction break" in change introduced in patch 6. And there's the risk that some of the reworks may have added a regression somewhere.
Based on that, I decided to also provide an alternative implementation. In this implementation we add a new dev->vblank_kernel_list instead of reusing dev->vblank_event_list, and add new code to handle that this. This new code is completely based on the code that handles dev->vblank_kernel_list, so there's some duplication here. On the other hand, since the in-Kernel infrastructure is not tied to the IOCTL structure, we can very easily grow and adapt the Kernel code without having to fight against the IOCTL code. And the risk of regressions is greatly reduced.
The second difference of this alternative implementation is the signature of the drm_wait_vblank_kernel() function. While it could be exactly the same as the one used in the other series, I decided to make it different so we can also choose which one to use. In the 7-patch series implementation, all the user needs to do is to allocate the structure, and call the function, properly setting all its arguments. Then the function is responsible for parsing the arguments and populating the structure based on that. On the alternative implementation, the user has to fill the structure with the appropriate arguments, and then call drm_wait_vblank_kernel() passing just the allocated structure as an argument.
If you notice, you will see that these patches add a new debugfs file to i915.ko. This file is just a very simple example user of the new interface, which I used while developing. If you have difficulty understanding the new interface, please also look at the i915/i915_debugfs.c diff. Of course, I plan to exclude this debugfs interface from the final to-be-merged patches.
So, in summary, we have a few things we need to discuss:
- Which implementation to use?
a. Just the 2 first patches of the 7-patch series? Pros: - the Kernel users basically call the vblank IOCTL - the code changes are minimal Cons: - The amount of memory allocations and memory space consumed is not optimal
b. The full 7-patch series? Pros: - The same backend is used for both the Kernel and IOCTL. Cons: - The code gets more complex. - Extending the Kernel interface can get complicated due to the fact that it shares code with the IOCTL interface. - I didn't really like this solution.
c. The alternative implementation I wrote? Pros: - It's independent from the IOCTL code, making further patches easier. Cons: - There is some duplicated code.
d. Something totally different from these alternatives?
- How should the driver interface look like?
a. All the possibilities are passed through the function call, so the drm.ko code needs to set the struct members itself. b. The caller already sets the struct members instead of passing them as parameters to the function. c. Something different?
All these patches are based on top of Daniel Vetter's drm-intel-nightly tree. It's all just an RFC, so don't expect something very well tested.
Flamewars and bikeshedding are welcome!
As we've discussed on irc a bit I still think cleaning up this rathole is beneficial. Getting the event handling and vblank callbacks right just in drivers seems to be really hard, so better to clean this up and unify things.
But like you've said patches are a lot of churn, so do you have a git branch somewhere for the lazy to look at?
Thanks, Daniel
I also think vmwgfx might already have something like this, maybe take a look and see if this is the same use case and can cover that as well.
Dave.
On Thu, Nov 27, 2014 at 09:25:15AM +1000, Dave Airlie wrote:
TL;DR summary: I wrote patches. Help me choose the best implementation and interface.
So the i915.ko driver could use some mechanism to run functions after a given number of vblanks. Implementations for this mechanism were already proposed in the past (by Chris Wilson and others), but they were i915-specific instead of a generic drm.ko implementation. We even had patches that make use of this new mechanism.
Since this is very similar to the vblank IOCTL we currently have, but with the caller being a Kernel module instead of user space, I decided to write an implementation that tries to reuse the current IOCTL infrastructure.
In the first patch we add all the necessary code to allow the modules to call the vblank ioctl from Kernel space: they provide a callback function that is going to be called instead of the traditional send_vblank_event(), which means the Kernel callers don't have to deal with the file descriptors and DRM events.
In the second patch we add a simple extension of the feature, to allow the drivers to have their callbacks called in a non-atomic context.
In all the other subsequent patches we rework the underlying code so that the common aspects between the user space vblank IOCTL and the Kernel interface are all stored in a single structure (struct drm_vblank_wait_item), and then anything that is specific to the different users is stored in a structure that contains struct drm_vblank_wait_item. This way, most of the drm.ko code only needs to deal with struct drm_vblank_wait_item, not really knowing how it is wrapped. The advantage of this rework is that it reduces the number of memory allocations needed in some cases (from 2 to 1) and it also reduces the amount of memory used on each allocation.
But while this all sounds nice in the above description, I had the feeling that this adds a lot of complexity and makes the code not-so-beautiful. If we ever need more extensions/options to this feature, we're going to have to untangle the code even more from the IOCTL part. We also have a small "abstraction break" in change introduced in patch 6. And there's the risk that some of the reworks may have added a regression somewhere.
Based on that, I decided to also provide an alternative implementation. In this implementation we add a new dev->vblank_kernel_list instead of reusing dev->vblank_event_list, and add new code to handle that this. This new code is completely based on the code that handles dev->vblank_kernel_list, so there's some duplication here. On the other hand, since the in-Kernel infrastructure is not tied to the IOCTL structure, we can very easily grow and adapt the Kernel code without having to fight against the IOCTL code. And the risk of regressions is greatly reduced.
The second difference of this alternative implementation is the signature of the drm_wait_vblank_kernel() function. While it could be exactly the same as the one used in the other series, I decided to make it different so we can also choose which one to use. In the 7-patch series implementation, all the user needs to do is to allocate the structure, and call the function, properly setting all its arguments. Then the function is responsible for parsing the arguments and populating the structure based on that. On the alternative implementation, the user has to fill the structure with the appropriate arguments, and then call drm_wait_vblank_kernel() passing just the allocated structure as an argument.
If you notice, you will see that these patches add a new debugfs file to i915.ko. This file is just a very simple example user of the new interface, which I used while developing. If you have difficulty understanding the new interface, please also look at the i915/i915_debugfs.c diff. Of course, I plan to exclude this debugfs interface from the final to-be-merged patches.
So, in summary, we have a few things we need to discuss:
- Which implementation to use?
a. Just the 2 first patches of the 7-patch series? Pros: - the Kernel users basically call the vblank IOCTL - the code changes are minimal Cons: - The amount of memory allocations and memory space consumed is not optimal
b. The full 7-patch series? Pros: - The same backend is used for both the Kernel and IOCTL. Cons: - The code gets more complex. - Extending the Kernel interface can get complicated due to the fact that it shares code with the IOCTL interface. - I didn't really like this solution.
c. The alternative implementation I wrote? Pros: - It's independent from the IOCTL code, making further patches easier. Cons: - There is some duplicated code.
d. Something totally different from these alternatives?
- How should the driver interface look like?
a. All the possibilities are passed through the function call, so the drm.ko code needs to set the struct members itself. b. The caller already sets the struct members instead of passing them as parameters to the function. c. Something different?
All these patches are based on top of Daniel Vetter's drm-intel-nightly tree. It's all just an RFC, so don't expect something very well tested.
Flamewars and bikeshedding are welcome!
As we've discussed on irc a bit I still think cleaning up this rathole is beneficial. Getting the event handling and vblank callbacks right just in drivers seems to be really hard, so better to clean this up and unify things.
But like you've said patches are a lot of churn, so do you have a git branch somewhere for the lazy to look at?
Thanks, Daniel
I also think vmwgfx might already have something like this, maybe take a look and see if this is the same use case and can cover that as well.
vmwgfx reuses the drm event logic for fences, so definitely interacts with all this. And Rob has some rfc patches somewhere (as part of atomic) to clean up the drm event handling code and extract the commen prepare/release/send and preclose handling code a bit so that not every driver needs to reinvent that whell (and with bugs).
But I did not spot any vblank callback/worker support in vmwgfx. So I think Paulo's series here is orthogonal. Ofc it would also benefit from an overhaul of the drm event handling code. -Daniel
On Wed, Nov 19, 2014 at 8:47 PM, Paulo Zanoni przanoni@gmail.com wrote:
- How should the driver interface look like?
a. All the possibilities are passed through the function call, so the drm.ko code needs to set the struct members itself. b. The caller already sets the struct members instead of passing them as parameters to the function. c. Something different?
We need b because the caller must allocate the structure (the point where we can return -ENOMEM to userspace might be much after the point where we need to schedule a vblank callback for e.g. async flips). But for simple interfaces we need a few things passed directly I think (since I expect that we'll reuse vblank callback structures similar to how we reuse timers/work items).
I'll follow up with a detailed review of the new interface exposed to drivers and what I think it should look like, need to head off now. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Dave Airlie -
Matt Roper -
Paulo Zanoni