* [PATCH v2 0/2] drm: rework flip-work framework
@ 2014-07-12 7:00 Boris BREZILLON
2014-07-12 7:00 ` [PATCH v2 1/2] drm: rework flip-work helpers to avoid calling func when the FIFO is full Boris BREZILLON
2014-07-12 7:00 ` [PATCH v2 2/2] drm: flip-work: change drm_flip_work_init prototype Boris BREZILLON
0 siblings, 2 replies; 5+ messages in thread
From: Boris BREZILLON @ 2014-07-12 7:00 UTC (permalink / raw)
To: Rob Clark; +Cc: David Airlie, dri-devel, linux-kernel, Boris BREZILLON
Hello,
This patch series reworks the flip-work framework to make it safe when
calling drm_flip_work_queue from atomic contexts.
The 2nd patch of this series is optional, as it only reworks
drm_flip_work_init prototype to remove unneeded size argument and
return code (this function cannot fail anymore).
Best Regards,
Boris
Boris BREZILLON (2):
drm: rework flip-work helpers to avoid calling func when the FIFO is
full
drm: flip-work: change drm_flip_work_init prototype
drivers/gpu/drm/drm_flip_work.c | 104 ++++++++++++++++++++++---------
drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 19 ++----
drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 16 +----
drivers/gpu/drm/omapdrm/omap_plane.c | 14 +----
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 6 +-
include/drm/drm_flip_work.h | 31 ++++++---
6 files changed, 105 insertions(+), 85 deletions(-)
--
1.8.3.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] drm: rework flip-work helpers to avoid calling func when the FIFO is full
2014-07-12 7:00 [PATCH v2 0/2] drm: rework flip-work framework Boris BREZILLON
@ 2014-07-12 7:00 ` Boris BREZILLON
2014-07-12 7:29 ` Boris BREZILLON
2014-07-12 11:37 ` Rob Clark
2014-07-12 7:00 ` [PATCH v2 2/2] drm: flip-work: change drm_flip_work_init prototype Boris BREZILLON
1 sibling, 2 replies; 5+ messages in thread
From: Boris BREZILLON @ 2014-07-12 7:00 UTC (permalink / raw)
To: Rob Clark; +Cc: David Airlie, dri-devel, linux-kernel, Boris BREZILLON
Make use of lists instead of kfifo in order to dynamically allocate
task entry when someone require some delayed work, and thus preventing
drm_flip_work_queue from directly calling func instead of queuing this
call.
This allow drm_flip_work_queue to be safely called even within irq
handlers.
Add new helper functions to allocate a flip work task and queue it when
needed. This prevents allocating data within irq context (which might
impact the time spent in the irq handler).
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
drivers/gpu/drm/drm_flip_work.c | 96 ++++++++++++++++++++++++++++++-----------
include/drm/drm_flip_work.h | 29 +++++++++----
2 files changed, 93 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_flip_work.c b/drivers/gpu/drm/drm_flip_work.c
index f9c7fa3..7441aa8 100644
--- a/drivers/gpu/drm/drm_flip_work.c
+++ b/drivers/gpu/drm/drm_flip_work.c
@@ -25,6 +25,44 @@
#include "drm_flip_work.h"
/**
+ * drm_flip_work_allocate_task - allocate a flip-work task
+ * @data: data associated to the task
+ * @flags: allocator flags
+ *
+ * Allocate a drm_flip_task object and attach private data to it.
+ */
+struct drm_flip_task *drm_flip_work_allocate_task(void *data, gfp_t flags)
+{
+ struct drm_flip_task *task;
+
+ task = kzalloc(sizeof(*task), flags);
+ if (task)
+ task->data = data;
+
+ return task;
+}
+EXPORT_SYMBOL(drm_flip_work_allocate_task);
+
+/**
+ * drm_flip_work_queue_task - queue a specific task
+ * @work: the flip-work
+ * @task: the task to handle
+ *
+ * Queues task, that will later be run (passed back to drm_flip_func_t
+ * func) on a work queue after drm_flip_work_commit() is called.
+ */
+void drm_flip_work_queue_task(struct drm_flip_work *work,
+ struct drm_flip_task *task)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&work->lock, flags);
+ list_add_tail(&task->node, &work->queued);
+ spin_unlock_irqrestore(&work->lock, flags);
+}
+EXPORT_SYMBOL(drm_flip_work_queue_task);
+
+/**
* drm_flip_work_queue - queue work
* @work: the flip-work
* @val: the value to queue
@@ -34,10 +72,14 @@
*/
void drm_flip_work_queue(struct drm_flip_work *work, void *val)
{
- if (kfifo_put(&work->fifo, val)) {
- atomic_inc(&work->pending);
+ struct drm_flip_task *task;
+
+ task = drm_flip_work_allocate_task(val,
+ drm_can_sleep() ? GFP_KERNEL : GFP_ATOMIC);
+ if (task) {
+ drm_flip_work_queue_task(work, task);
} else {
- DRM_ERROR("%s fifo full!\n", work->name);
+ DRM_ERROR("%s could not allocate task!\n", work->name);
work->func(work, val);
}
}
@@ -56,9 +98,12 @@ EXPORT_SYMBOL(drm_flip_work_queue);
void drm_flip_work_commit(struct drm_flip_work *work,
struct workqueue_struct *wq)
{
- uint32_t pending = atomic_read(&work->pending);
- atomic_add(pending, &work->count);
- atomic_sub(pending, &work->pending);
+ unsigned long flags;
+
+ spin_lock_irqsave(&work->lock, flags);
+ list_splice_tail(&work->queued, &work->commited);
+ INIT_LIST_HEAD(&work->queued);
+ spin_unlock_irqrestore(&work->lock, flags);
queue_work(wq, &work->worker);
}
EXPORT_SYMBOL(drm_flip_work_commit);
@@ -66,14 +111,26 @@ EXPORT_SYMBOL(drm_flip_work_commit);
static void flip_worker(struct work_struct *w)
{
struct drm_flip_work *work = container_of(w, struct drm_flip_work, worker);
- uint32_t count = atomic_read(&work->count);
- void *val = NULL;
+ struct list_head tasks;
+ unsigned long flags;
- atomic_sub(count, &work->count);
+ while (1) {
+ struct drm_flip_task *task, *tmp;
- while(count--)
- if (!WARN_ON(!kfifo_get(&work->fifo, &val)))
- work->func(work, val);
+ INIT_LIST_HEAD(&tasks);
+ spin_lock_irqsave(&work->lock, flags);
+ list_splice_tail(&work->commited, &tasks);
+ INIT_LIST_HEAD(&work->commited);
+ spin_unlock_irqrestore(&work->lock, flags);
+
+ if (list_empty(&tasks))
+ break;
+
+ list_for_each_entry_safe(task, tmp, &tasks, node) {
+ work->func(work, task->data);
+ kfree(task);
+ }
+ }
}
/**
@@ -91,19 +148,11 @@ static void flip_worker(struct work_struct *w)
int drm_flip_work_init(struct drm_flip_work *work, int size,
const char *name, drm_flip_func_t func)
{
- int ret;
-
work->name = name;
- atomic_set(&work->count, 0);
- atomic_set(&work->pending, 0);
+ INIT_LIST_HEAD(&work->queued);
+ INIT_LIST_HEAD(&work->commited);
work->func = func;
- ret = kfifo_alloc(&work->fifo, size, GFP_KERNEL);
- if (ret) {
- DRM_ERROR("could not allocate %s fifo\n", name);
- return ret;
- }
-
INIT_WORK(&work->worker, flip_worker);
return 0;
@@ -118,7 +167,6 @@ EXPORT_SYMBOL(drm_flip_work_init);
*/
void drm_flip_work_cleanup(struct drm_flip_work *work)
{
- WARN_ON(!kfifo_is_empty(&work->fifo));
- kfifo_free(&work->fifo);
+ WARN_ON(!list_empty(&work->queued) || !list_empty(&work->commited));
}
EXPORT_SYMBOL(drm_flip_work_cleanup);
diff --git a/include/drm/drm_flip_work.h b/include/drm/drm_flip_work.h
index 9eed34d..0a5acff 100644
--- a/include/drm/drm_flip_work.h
+++ b/include/drm/drm_flip_work.h
@@ -25,6 +25,7 @@
#define DRM_FLIP_WORK_H
#include <linux/kfifo.h>
+#include <linux/spinlock.h>
#include <linux/workqueue.h>
/**
@@ -32,9 +33,7 @@
*
* Util to queue up work to run from work-queue context after flip/vblank.
* Typically this can be used to defer unref of framebuffer's, cursor
- * bo's, etc until after vblank. The APIs are all safe (and lockless)
- * for up to one producer and once consumer at a time. The single-consumer
- * aspect is ensured by committing the queued work to a single work-queue.
+ * bo's, etc until after vblank. The APIs are all safe.
*/
struct drm_flip_work;
@@ -51,22 +50,36 @@ struct drm_flip_work;
typedef void (*drm_flip_func_t)(struct drm_flip_work *work, void *val);
/**
+ * struct drm_flip_task - flip work task
+ * @node: list entry element
+ * @data: data to pass to work->func
+ */
+struct drm_flip_task {
+ struct list_head node;
+ void *data;
+};
+
+/**
* struct drm_flip_work - flip work queue
* @name: debug name
- * @pending: number of queued but not committed items
- * @count: number of committed items
* @func: callback fxn called for each committed item
* @worker: worker which calls @func
- * @fifo: queue of committed items
+ * @queued: queued tasks
+ * @commited: commited tasks
+ * @lock: lock to access queued and commited lists
*/
struct drm_flip_work {
const char *name;
- atomic_t pending, count;
drm_flip_func_t func;
struct work_struct worker;
- DECLARE_KFIFO_PTR(fifo, void *);
+ struct list_head queued;
+ struct list_head commited;
+ spinlock_t lock;
};
+struct drm_flip_task *drm_flip_work_allocate_task(void *data, gfp_t flags);
+void drm_flip_work_queue_task(struct drm_flip_work *work,
+ struct drm_flip_task *task);
void drm_flip_work_queue(struct drm_flip_work *work, void *val);
void drm_flip_work_commit(struct drm_flip_work *work,
struct workqueue_struct *wq);
--
1.8.3.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] drm: flip-work: change drm_flip_work_init prototype
2014-07-12 7:00 [PATCH v2 0/2] drm: rework flip-work framework Boris BREZILLON
2014-07-12 7:00 ` [PATCH v2 1/2] drm: rework flip-work helpers to avoid calling func when the FIFO is full Boris BREZILLON
@ 2014-07-12 7:00 ` Boris BREZILLON
1 sibling, 0 replies; 5+ messages in thread
From: Boris BREZILLON @ 2014-07-12 7:00 UTC (permalink / raw)
To: Rob Clark; +Cc: David Airlie, dri-devel, linux-kernel, Boris BREZILLON
Now that we're using lists instead of kfifo to store drm flip-work tasks
we do not need the size parameter passed to drm_flip_work_init function
anymore.
Moreover this function cannot fail anymore, we can thus remove the return
code.
Modify drm_flip_work_init users to take account of these changes.
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
drivers/gpu/drm/drm_flip_work.c | 8 +-------
drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 19 ++++---------------
drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 16 +++-------------
drivers/gpu/drm/omapdrm/omap_plane.c | 14 ++------------
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 6 +-----
include/drm/drm_flip_work.h | 2 +-
6 files changed, 12 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/drm_flip_work.c b/drivers/gpu/drm/drm_flip_work.c
index 7441aa8..2b557f2 100644
--- a/drivers/gpu/drm/drm_flip_work.c
+++ b/drivers/gpu/drm/drm_flip_work.c
@@ -136,16 +136,12 @@ static void flip_worker(struct work_struct *w)
/**
* drm_flip_work_init - initialize flip-work
* @work: the flip-work to initialize
- * @size: the max queue depth
* @name: debug name
* @func: the callback work function
*
* Initializes/allocates resources for the flip-work
- *
- * RETURNS:
- * Zero on success, error code on failure.
*/
-int drm_flip_work_init(struct drm_flip_work *work, int size,
+void drm_flip_work_init(struct drm_flip_work *work,
const char *name, drm_flip_func_t func)
{
work->name = name;
@@ -154,8 +150,6 @@ int drm_flip_work_init(struct drm_flip_work *work, int size,
work->func = func;
INIT_WORK(&work->worker, flip_worker);
-
- return 0;
}
EXPORT_SYMBOL(drm_flip_work_init);
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
index 74cebb5..44d4f93 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
@@ -755,10 +755,8 @@ struct drm_crtc *mdp4_crtc_init(struct drm_device *dev,
int ret;
mdp4_crtc = kzalloc(sizeof(*mdp4_crtc), GFP_KERNEL);
- if (!mdp4_crtc) {
- ret = -ENOMEM;
- goto fail;
- }
+ if (!mdp4_crtc)
+ return ERR_PTR(-ENOMEM);
crtc = &mdp4_crtc->base;
@@ -779,12 +777,9 @@ struct drm_crtc *mdp4_crtc_init(struct drm_device *dev,
spin_lock_init(&mdp4_crtc->cursor.lock);
- ret = drm_flip_work_init(&mdp4_crtc->unref_fb_work, 16,
+ drm_flip_work_init(&mdp4_crtc->unref_fb_work,
"unref fb", unref_fb_worker);
- if (ret)
- goto fail;
-
- ret = drm_flip_work_init(&mdp4_crtc->unref_cursor_work, 64,
+ drm_flip_work_init(&mdp4_crtc->unref_cursor_work,
"unref cursor", unref_cursor_worker);
INIT_FENCE_CB(&mdp4_crtc->pageflip_cb, pageflip_cb);
@@ -795,10 +790,4 @@ struct drm_crtc *mdp4_crtc_init(struct drm_device *dev,
mdp4_plane_install_properties(mdp4_crtc->plane, &crtc->base);
return crtc;
-
-fail:
- if (crtc)
- mdp4_crtc_destroy(crtc);
-
- return ERR_PTR(ret);
}
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
index ebe2e60..a0cb374 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
@@ -537,10 +537,8 @@ struct drm_crtc *mdp5_crtc_init(struct drm_device *dev,
int ret;
mdp5_crtc = kzalloc(sizeof(*mdp5_crtc), GFP_KERNEL);
- if (!mdp5_crtc) {
- ret = -ENOMEM;
- goto fail;
- }
+ if (!mdp5_crtc)
+ return ERR_PTR(-ENOMEM);
crtc = &mdp5_crtc->base;
@@ -553,10 +551,8 @@ struct drm_crtc *mdp5_crtc_init(struct drm_device *dev,
snprintf(mdp5_crtc->name, sizeof(mdp5_crtc->name), "%s:%d",
pipe2name(mdp5_plane_pipe(plane)), id);
- ret = drm_flip_work_init(&mdp5_crtc->unref_fb_work, 16,
+ drm_flip_work_init(&mdp5_crtc->unref_fb_work,
"unref fb", unref_fb_worker);
- if (ret)
- goto fail;
INIT_FENCE_CB(&mdp5_crtc->pageflip_cb, pageflip_cb);
@@ -566,10 +562,4 @@ struct drm_crtc *mdp5_crtc_init(struct drm_device *dev,
mdp5_plane_install_properties(mdp5_crtc->plane, &crtc->base);
return crtc;
-
-fail:
- if (crtc)
- mdp5_crtc_destroy(crtc);
-
- return ERR_PTR(ret);
}
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 3cf31ee..847d1ca 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -397,14 +397,10 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
omap_plane = kzalloc(sizeof(*omap_plane), GFP_KERNEL);
if (!omap_plane)
- goto fail;
+ return NULL;
- ret = drm_flip_work_init(&omap_plane->unpin_work, 16,
+ drm_flip_work_init(&omap_plane->unpin_work,
"unpin", unpin_worker);
- if (ret) {
- dev_err(dev->dev, "could not allocate unpin FIFO\n");
- goto fail;
- }
omap_plane->nformats = omap_framebuffer_get_formats(
omap_plane->formats, ARRAY_SIZE(omap_plane->formats),
@@ -446,10 +442,4 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
omap_plane->info.zorder = id;
return plane;
-
-fail:
- if (plane)
- omap_plane_destroy(plane);
-
- return NULL;
}
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index d642d4a0..7d782ce 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -664,12 +664,8 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
tilcdc_crtc->dpms = DRM_MODE_DPMS_OFF;
init_waitqueue_head(&tilcdc_crtc->frame_done_wq);
- ret = drm_flip_work_init(&tilcdc_crtc->unref_work, 16,
+ drm_flip_work_init(&tilcdc_crtc->unref_work,
"unref", unref_worker);
- if (ret) {
- dev_err(dev->dev, "could not allocate unref FIFO\n");
- goto fail;
- }
ret = drm_crtc_init(dev, crtc, &tilcdc_crtc_funcs);
if (ret < 0)
diff --git a/include/drm/drm_flip_work.h b/include/drm/drm_flip_work.h
index 0a5acff..208e357 100644
--- a/include/drm/drm_flip_work.h
+++ b/include/drm/drm_flip_work.h
@@ -83,7 +83,7 @@ void drm_flip_work_queue_task(struct drm_flip_work *work,
void drm_flip_work_queue(struct drm_flip_work *work, void *val);
void drm_flip_work_commit(struct drm_flip_work *work,
struct workqueue_struct *wq);
-int drm_flip_work_init(struct drm_flip_work *work, int size,
+void drm_flip_work_init(struct drm_flip_work *work,
const char *name, drm_flip_func_t func);
void drm_flip_work_cleanup(struct drm_flip_work *work);
--
1.8.3.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] drm: rework flip-work helpers to avoid calling func when the FIFO is full
2014-07-12 7:00 ` [PATCH v2 1/2] drm: rework flip-work helpers to avoid calling func when the FIFO is full Boris BREZILLON
@ 2014-07-12 7:29 ` Boris BREZILLON
2014-07-12 11:37 ` Rob Clark
1 sibling, 0 replies; 5+ messages in thread
From: Boris BREZILLON @ 2014-07-12 7:29 UTC (permalink / raw)
To: Boris BREZILLON; +Cc: Rob Clark, David Airlie, dri-devel, linux-kernel
On Sat, 12 Jul 2014 09:00:08 +0200
Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:
> Make use of lists instead of kfifo in order to dynamically allocate
> task entry when someone require some delayed work, and thus preventing
> drm_flip_work_queue from directly calling func instead of queuing this
> call.
> This allow drm_flip_work_queue to be safely called even within irq
> handlers.
>
> Add new helper functions to allocate a flip work task and queue it when
> needed. This prevents allocating data within irq context (which might
> impact the time spent in the irq handler).
>
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
> drivers/gpu/drm/drm_flip_work.c | 96 ++++++++++++++++++++++++++++++-----------
> include/drm/drm_flip_work.h | 29 +++++++++----
> 2 files changed, 93 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_flip_work.c b/drivers/gpu/drm/drm_flip_work.c
> index f9c7fa3..7441aa8 100644
> --- a/drivers/gpu/drm/drm_flip_work.c
> +++ b/drivers/gpu/drm/drm_flip_work.c
> @@ -25,6 +25,44 @@
> #include "drm_flip_work.h"
>
> /**
> + * drm_flip_work_allocate_task - allocate a flip-work task
> + * @data: data associated to the task
> + * @flags: allocator flags
> + *
> + * Allocate a drm_flip_task object and attach private data to it.
> + */
> +struct drm_flip_task *drm_flip_work_allocate_task(void *data, gfp_t flags)
> +{
> + struct drm_flip_task *task;
> +
> + task = kzalloc(sizeof(*task), flags);
> + if (task)
> + task->data = data;
> +
> + return task;
> +}
> +EXPORT_SYMBOL(drm_flip_work_allocate_task);
> +
> +/**
> + * drm_flip_work_queue_task - queue a specific task
> + * @work: the flip-work
> + * @task: the task to handle
> + *
> + * Queues task, that will later be run (passed back to drm_flip_func_t
> + * func) on a work queue after drm_flip_work_commit() is called.
> + */
> +void drm_flip_work_queue_task(struct drm_flip_work *work,
> + struct drm_flip_task *task)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&work->lock, flags);
> + list_add_tail(&task->node, &work->queued);
> + spin_unlock_irqrestore(&work->lock, flags);
> +}
> +EXPORT_SYMBOL(drm_flip_work_queue_task);
> +
> +/**
> * drm_flip_work_queue - queue work
> * @work: the flip-work
> * @val: the value to queue
> @@ -34,10 +72,14 @@
> */
> void drm_flip_work_queue(struct drm_flip_work *work, void *val)
> {
> - if (kfifo_put(&work->fifo, val)) {
> - atomic_inc(&work->pending);
> + struct drm_flip_task *task;
> +
> + task = drm_flip_work_allocate_task(val,
> + drm_can_sleep() ? GFP_KERNEL : GFP_ATOMIC);
> + if (task) {
> + drm_flip_work_queue_task(work, task);
> } else {
> - DRM_ERROR("%s fifo full!\n", work->name);
> + DRM_ERROR("%s could not allocate task!\n", work->name);
> work->func(work, val);
> }
> }
> @@ -56,9 +98,12 @@ EXPORT_SYMBOL(drm_flip_work_queue);
> void drm_flip_work_commit(struct drm_flip_work *work,
> struct workqueue_struct *wq)
> {
> - uint32_t pending = atomic_read(&work->pending);
> - atomic_add(pending, &work->count);
> - atomic_sub(pending, &work->pending);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&work->lock, flags);
> + list_splice_tail(&work->queued, &work->commited);
> + INIT_LIST_HEAD(&work->queued);
> + spin_unlock_irqrestore(&work->lock, flags);
> queue_work(wq, &work->worker);
> }
> EXPORT_SYMBOL(drm_flip_work_commit);
> @@ -66,14 +111,26 @@ EXPORT_SYMBOL(drm_flip_work_commit);
> static void flip_worker(struct work_struct *w)
> {
> struct drm_flip_work *work = container_of(w, struct drm_flip_work, worker);
> - uint32_t count = atomic_read(&work->count);
> - void *val = NULL;
> + struct list_head tasks;
> + unsigned long flags;
>
> - atomic_sub(count, &work->count);
> + while (1) {
> + struct drm_flip_task *task, *tmp;
>
> - while(count--)
> - if (!WARN_ON(!kfifo_get(&work->fifo, &val)))
> - work->func(work, val);
> + INIT_LIST_HEAD(&tasks);
> + spin_lock_irqsave(&work->lock, flags);
> + list_splice_tail(&work->commited, &tasks);
> + INIT_LIST_HEAD(&work->commited);
> + spin_unlock_irqrestore(&work->lock, flags);
> +
> + if (list_empty(&tasks))
> + break;
> +
> + list_for_each_entry_safe(task, tmp, &tasks, node) {
> + work->func(work, task->data);
> + kfree(task);
> + }
> + }
> }
>
> /**
> @@ -91,19 +148,11 @@ static void flip_worker(struct work_struct *w)
> int drm_flip_work_init(struct drm_flip_work *work, int size,
> const char *name, drm_flip_func_t func)
> {
> - int ret;
> -
> work->name = name;
> - atomic_set(&work->count, 0);
> - atomic_set(&work->pending, 0);
> + INIT_LIST_HEAD(&work->queued);
> + INIT_LIST_HEAD(&work->commited);
spin_lock_init(&work->lock);
is missing here.
> work->func = func;
>
> - ret = kfifo_alloc(&work->fifo, size, GFP_KERNEL);
> - if (ret) {
> - DRM_ERROR("could not allocate %s fifo\n", name);
> - return ret;
> - }
> -
> INIT_WORK(&work->worker, flip_worker);
>
> return 0;
> @@ -118,7 +167,6 @@ EXPORT_SYMBOL(drm_flip_work_init);
> */
> void drm_flip_work_cleanup(struct drm_flip_work *work)
> {
> - WARN_ON(!kfifo_is_empty(&work->fifo));
> - kfifo_free(&work->fifo);
> + WARN_ON(!list_empty(&work->queued) || !list_empty(&work->commited));
> }
> EXPORT_SYMBOL(drm_flip_work_cleanup);
> diff --git a/include/drm/drm_flip_work.h b/include/drm/drm_flip_work.h
> index 9eed34d..0a5acff 100644
> --- a/include/drm/drm_flip_work.h
> +++ b/include/drm/drm_flip_work.h
> @@ -25,6 +25,7 @@
> #define DRM_FLIP_WORK_H
>
> #include <linux/kfifo.h>
> +#include <linux/spinlock.h>
> #include <linux/workqueue.h>
>
> /**
> @@ -32,9 +33,7 @@
> *
> * Util to queue up work to run from work-queue context after flip/vblank.
> * Typically this can be used to defer unref of framebuffer's, cursor
> - * bo's, etc until after vblank. The APIs are all safe (and lockless)
> - * for up to one producer and once consumer at a time. The single-consumer
> - * aspect is ensured by committing the queued work to a single work-queue.
> + * bo's, etc until after vblank. The APIs are all safe.
> */
>
> struct drm_flip_work;
> @@ -51,22 +50,36 @@ struct drm_flip_work;
> typedef void (*drm_flip_func_t)(struct drm_flip_work *work, void *val);
>
> /**
> + * struct drm_flip_task - flip work task
> + * @node: list entry element
> + * @data: data to pass to work->func
> + */
> +struct drm_flip_task {
> + struct list_head node;
> + void *data;
> +};
> +
> +/**
> * struct drm_flip_work - flip work queue
> * @name: debug name
> - * @pending: number of queued but not committed items
> - * @count: number of committed items
> * @func: callback fxn called for each committed item
> * @worker: worker which calls @func
> - * @fifo: queue of committed items
> + * @queued: queued tasks
> + * @commited: commited tasks
> + * @lock: lock to access queued and commited lists
> */
> struct drm_flip_work {
> const char *name;
> - atomic_t pending, count;
> drm_flip_func_t func;
> struct work_struct worker;
> - DECLARE_KFIFO_PTR(fifo, void *);
> + struct list_head queued;
> + struct list_head commited;
> + spinlock_t lock;
> };
>
> +struct drm_flip_task *drm_flip_work_allocate_task(void *data, gfp_t flags);
> +void drm_flip_work_queue_task(struct drm_flip_work *work,
> + struct drm_flip_task *task);
> void drm_flip_work_queue(struct drm_flip_work *work, void *val);
> void drm_flip_work_commit(struct drm_flip_work *work,
> struct workqueue_struct *wq);
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] drm: rework flip-work helpers to avoid calling func when the FIFO is full
2014-07-12 7:00 ` [PATCH v2 1/2] drm: rework flip-work helpers to avoid calling func when the FIFO is full Boris BREZILLON
2014-07-12 7:29 ` Boris BREZILLON
@ 2014-07-12 11:37 ` Rob Clark
1 sibling, 0 replies; 5+ messages in thread
From: Rob Clark @ 2014-07-12 11:37 UTC (permalink / raw)
To: Boris BREZILLON; +Cc: David Airlie, dri-devel, Linux Kernel Mailing List
On Sat, Jul 12, 2014 at 3:00 AM, Boris BREZILLON
<boris.brezillon@free-electrons.com> wrote:
> Make use of lists instead of kfifo in order to dynamically allocate
> task entry when someone require some delayed work, and thus preventing
> drm_flip_work_queue from directly calling func instead of queuing this
> call.
> This allow drm_flip_work_queue to be safely called even within irq
> handlers.
>
> Add new helper functions to allocate a flip work task and queue it when
> needed. This prevents allocating data within irq context (which might
> impact the time spent in the irq handler).
>
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
> drivers/gpu/drm/drm_flip_work.c | 96 ++++++++++++++++++++++++++++++-----------
> include/drm/drm_flip_work.h | 29 +++++++++----
> 2 files changed, 93 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_flip_work.c b/drivers/gpu/drm/drm_flip_work.c
> index f9c7fa3..7441aa8 100644
> --- a/drivers/gpu/drm/drm_flip_work.c
> +++ b/drivers/gpu/drm/drm_flip_work.c
> @@ -25,6 +25,44 @@
> #include "drm_flip_work.h"
>
> /**
> + * drm_flip_work_allocate_task - allocate a flip-work task
> + * @data: data associated to the task
> + * @flags: allocator flags
> + *
> + * Allocate a drm_flip_task object and attach private data to it.
> + */
> +struct drm_flip_task *drm_flip_work_allocate_task(void *data, gfp_t flags)
> +{
> + struct drm_flip_task *task;
> +
> + task = kzalloc(sizeof(*task), flags);
> + if (task)
> + task->data = data;
> +
> + return task;
> +}
> +EXPORT_SYMBOL(drm_flip_work_allocate_task);
> +
> +/**
> + * drm_flip_work_queue_task - queue a specific task
> + * @work: the flip-work
> + * @task: the task to handle
> + *
> + * Queues task, that will later be run (passed back to drm_flip_func_t
> + * func) on a work queue after drm_flip_work_commit() is called.
> + */
> +void drm_flip_work_queue_task(struct drm_flip_work *work,
> + struct drm_flip_task *task)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&work->lock, flags);
> + list_add_tail(&task->node, &work->queued);
> + spin_unlock_irqrestore(&work->lock, flags);
> +}
> +EXPORT_SYMBOL(drm_flip_work_queue_task);
> +
> +/**
> * drm_flip_work_queue - queue work
> * @work: the flip-work
> * @val: the value to queue
> @@ -34,10 +72,14 @@
> */
> void drm_flip_work_queue(struct drm_flip_work *work, void *val)
> {
> - if (kfifo_put(&work->fifo, val)) {
> - atomic_inc(&work->pending);
> + struct drm_flip_task *task;
> +
> + task = drm_flip_work_allocate_task(val,
> + drm_can_sleep() ? GFP_KERNEL : GFP_ATOMIC);
> + if (task) {
> + drm_flip_work_queue_task(work, task);
> } else {
> - DRM_ERROR("%s fifo full!\n", work->name);
> + DRM_ERROR("%s could not allocate task!\n", work->name);
> work->func(work, val);
> }
> }
> @@ -56,9 +98,12 @@ EXPORT_SYMBOL(drm_flip_work_queue);
> void drm_flip_work_commit(struct drm_flip_work *work,
> struct workqueue_struct *wq)
> {
> - uint32_t pending = atomic_read(&work->pending);
> - atomic_add(pending, &work->count);
> - atomic_sub(pending, &work->pending);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&work->lock, flags);
> + list_splice_tail(&work->queued, &work->commited);
> + INIT_LIST_HEAD(&work->queued);
> + spin_unlock_irqrestore(&work->lock, flags);
> queue_work(wq, &work->worker);
> }
> EXPORT_SYMBOL(drm_flip_work_commit);
> @@ -66,14 +111,26 @@ EXPORT_SYMBOL(drm_flip_work_commit);
> static void flip_worker(struct work_struct *w)
> {
> struct drm_flip_work *work = container_of(w, struct drm_flip_work, worker);
> - uint32_t count = atomic_read(&work->count);
> - void *val = NULL;
> + struct list_head tasks;
> + unsigned long flags;
>
> - atomic_sub(count, &work->count);
> + while (1) {
> + struct drm_flip_task *task, *tmp;
>
> - while(count--)
> - if (!WARN_ON(!kfifo_get(&work->fifo, &val)))
> - work->func(work, val);
> + INIT_LIST_HEAD(&tasks);
> + spin_lock_irqsave(&work->lock, flags);
> + list_splice_tail(&work->commited, &tasks);
> + INIT_LIST_HEAD(&work->commited);
> + spin_unlock_irqrestore(&work->lock, flags);
> +
> + if (list_empty(&tasks))
> + break;
> +
> + list_for_each_entry_safe(task, tmp, &tasks, node) {
> + work->func(work, task->data);
> + kfree(task);
> + }
> + }
> }
>
> /**
> @@ -91,19 +148,11 @@ static void flip_worker(struct work_struct *w)
> int drm_flip_work_init(struct drm_flip_work *work, int size,
> const char *name, drm_flip_func_t func)
> {
> - int ret;
> -
> work->name = name;
> - atomic_set(&work->count, 0);
> - atomic_set(&work->pending, 0);
> + INIT_LIST_HEAD(&work->queued);
> + INIT_LIST_HEAD(&work->commited);
> work->func = func;
>
> - ret = kfifo_alloc(&work->fifo, size, GFP_KERNEL);
> - if (ret) {
> - DRM_ERROR("could not allocate %s fifo\n", name);
> - return ret;
> - }
> -
> INIT_WORK(&work->worker, flip_worker);
>
> return 0;
> @@ -118,7 +167,6 @@ EXPORT_SYMBOL(drm_flip_work_init);
> */
> void drm_flip_work_cleanup(struct drm_flip_work *work)
> {
> - WARN_ON(!kfifo_is_empty(&work->fifo));
> - kfifo_free(&work->fifo);
> + WARN_ON(!list_empty(&work->queued) || !list_empty(&work->commited));
> }
> EXPORT_SYMBOL(drm_flip_work_cleanup);
> diff --git a/include/drm/drm_flip_work.h b/include/drm/drm_flip_work.h
> index 9eed34d..0a5acff 100644
> --- a/include/drm/drm_flip_work.h
> +++ b/include/drm/drm_flip_work.h
> @@ -25,6 +25,7 @@
> #define DRM_FLIP_WORK_H
>
> #include <linux/kfifo.h>
> +#include <linux/spinlock.h>
> #include <linux/workqueue.h>
>
> /**
> @@ -32,9 +33,7 @@
> *
> * Util to queue up work to run from work-queue context after flip/vblank.
> * Typically this can be used to defer unref of framebuffer's, cursor
> - * bo's, etc until after vblank. The APIs are all safe (and lockless)
> - * for up to one producer and once consumer at a time. The single-consumer
> - * aspect is ensured by committing the queued work to a single work-queue.
> + * bo's, etc until after vblank. The APIs are all safe.
maybe when you add the missing spin_lock_init() you could add "irq" or
"atomic" to that last sentence? Otherwise it looks a bit funny, ie.
what are they safe from?
Other than that, for the series:
Reviewed-by: Rob Clark <robdclark@gmail.com>
> */
>
> struct drm_flip_work;
> @@ -51,22 +50,36 @@ struct drm_flip_work;
> typedef void (*drm_flip_func_t)(struct drm_flip_work *work, void *val);
>
> /**
> + * struct drm_flip_task - flip work task
> + * @node: list entry element
> + * @data: data to pass to work->func
> + */
> +struct drm_flip_task {
> + struct list_head node;
> + void *data;
> +};
> +
> +/**
> * struct drm_flip_work - flip work queue
> * @name: debug name
> - * @pending: number of queued but not committed items
> - * @count: number of committed items
> * @func: callback fxn called for each committed item
> * @worker: worker which calls @func
> - * @fifo: queue of committed items
> + * @queued: queued tasks
> + * @commited: commited tasks
> + * @lock: lock to access queued and commited lists
> */
> struct drm_flip_work {
> const char *name;
> - atomic_t pending, count;
> drm_flip_func_t func;
> struct work_struct worker;
> - DECLARE_KFIFO_PTR(fifo, void *);
> + struct list_head queued;
> + struct list_head commited;
> + spinlock_t lock;
> };
>
> +struct drm_flip_task *drm_flip_work_allocate_task(void *data, gfp_t flags);
> +void drm_flip_work_queue_task(struct drm_flip_work *work,
> + struct drm_flip_task *task);
> void drm_flip_work_queue(struct drm_flip_work *work, void *val);
> void drm_flip_work_commit(struct drm_flip_work *work,
> struct workqueue_struct *wq);
> --
> 1.8.3.2
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-07-12 11:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-12 7:00 [PATCH v2 0/2] drm: rework flip-work framework Boris BREZILLON
2014-07-12 7:00 ` [PATCH v2 1/2] drm: rework flip-work helpers to avoid calling func when the FIFO is full Boris BREZILLON
2014-07-12 7:29 ` Boris BREZILLON
2014-07-12 11:37 ` Rob Clark
2014-07-12 7:00 ` [PATCH v2 2/2] drm: flip-work: change drm_flip_work_init prototype Boris BREZILLON
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®