| Message ID | 1375313000-10672-1-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 1, 2013 at 1:23 AM, Rob Clark <robdclark@gmail.com> wrote: > A small helper to queue up work to do, from workqueue context, after a > flip. Typically useful to defer unreffing buffers that may be read by > the display controller until vblank. > > Signed-off-by: Rob Clark <robdclark@gmail.com> > --- > I've re-inventing the same wheel three times in as many drivers (omapdrm, > tilcdc, and in upcoming msm driver I need two of 'em). I guess it is > time to helper-up. > > I'll update omapdrm and tilcdc to use this as well, but I figured I'd > send an RFC first in case anyone wants to get their bikeshed on. If > there are other drivers that could use this, and are straightforward > to convert over, let me know and I can update them as well. One thing drm/i915 needs is to be able to flush the workqueue (to make sure we don't pile up giant amounts of buffers waiting to be unpinned and so temporarily leak a bit of memory). So some way to synchronously flush out flip functions would be required (and make sure all that have been queued up to that point are really completed). But at that point a separate workqueue sounds simpler, so I wonder a bit what this gains us? At roughly 50Hz flip work functions aren't really that performance critical imo ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Sun, Aug 4, 2013 at 1:34 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Aug 1, 2013 at 1:23 AM, Rob Clark <robdclark@gmail.com> wrote: >> A small helper to queue up work to do, from workqueue context, after a >> flip. Typically useful to defer unreffing buffers that may be read by >> the display controller until vblank. >> >> Signed-off-by: Rob Clark <robdclark@gmail.com> >> --- >> I've re-inventing the same wheel three times in as many drivers (omapdrm, >> tilcdc, and in upcoming msm driver I need two of 'em). I guess it is >> time to helper-up. >> >> I'll update omapdrm and tilcdc to use this as well, but I figured I'd >> send an RFC first in case anyone wants to get their bikeshed on. If >> there are other drivers that could use this, and are straightforward >> to convert over, let me know and I can update them as well. > > > One thing drm/i915 needs is to be able to flush the workqueue (to make > sure we don't pile up giant amounts of buffers waiting to be unpinned > and so temporarily leak a bit of memory). So some way to synchronously > flush out flip functions would be required (and make sure all that > have been queued up to that point are really completed). But at that > point a separate workqueue sounds simpler, so I wonder a bit what this > gains us? At roughly 50Hz flip work functions aren't really that > performance critical imo ... you could of course do separate or same workqueue from your main render-complete workqueue, or flush the workqueue at whatever point you want.. I suppose if you are queuing from a context that is safe to sleep you could have a blocking-queue function. I haven't really needed anything like that so far, but feel free to extend the flip-work helper however you need to be useful for i915. (Or point me at the bits in i915 that do something similar and I'll have a think about how to convert that) BR, -R > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Sun, Aug 04, 2013 at 10:20:54PM -0400, Rob Clark wrote: > On Sun, Aug 4, 2013 at 1:34 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Aug 1, 2013 at 1:23 AM, Rob Clark <robdclark@gmail.com> wrote: > >> A small helper to queue up work to do, from workqueue context, after a > >> flip. Typically useful to defer unreffing buffers that may be read by > >> the display controller until vblank. > >> > >> Signed-off-by: Rob Clark <robdclark@gmail.com> > >> --- > >> I've re-inventing the same wheel three times in as many drivers (omapdrm, > >> tilcdc, and in upcoming msm driver I need two of 'em). I guess it is > >> time to helper-up. > >> > >> I'll update omapdrm and tilcdc to use this as well, but I figured I'd > >> send an RFC first in case anyone wants to get their bikeshed on. If > >> there are other drivers that could use this, and are straightforward > >> to convert over, let me know and I can update them as well. > > > > > > One thing drm/i915 needs is to be able to flush the workqueue (to make > > sure we don't pile up giant amounts of buffers waiting to be unpinned > > and so temporarily leak a bit of memory). So some way to synchronously > > flush out flip functions would be required (and make sure all that > > have been queued up to that point are really completed). But at that > > point a separate workqueue sounds simpler, so I wonder a bit what this > > gains us? At roughly 50Hz flip work functions aren't really that > > performance critical imo ... > > you could of course do separate or same workqueue from your main > render-complete workqueue, or flush the workqueue at whatever point > you want.. I suppose if you are queuing from a context that is safe > to sleep you could have a blocking-queue function. I haven't really > needed anything like that so far, but feel free to extend the > flip-work helper however you need to be useful for i915. (Or point me > at the bits in i915 that do something similar and I'll have a think > about how to convert that) queue_work in do_intel_finish_page_flip and flush_work in intel_crtc_page_flip is the relevant code. The former is called from irq context. -Daniel
On Mon, Aug 5, 2013 at 4:00 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Sun, Aug 04, 2013 at 10:20:54PM -0400, Rob Clark wrote: >> On Sun, Aug 4, 2013 at 1:34 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> > On Thu, Aug 1, 2013 at 1:23 AM, Rob Clark <robdclark@gmail.com> wrote: >> >> A small helper to queue up work to do, from workqueue context, after a >> >> flip. Typically useful to defer unreffing buffers that may be read by >> >> the display controller until vblank. >> >> >> >> Signed-off-by: Rob Clark <robdclark@gmail.com> >> >> --- >> >> I've re-inventing the same wheel three times in as many drivers (omapdrm, >> >> tilcdc, and in upcoming msm driver I need two of 'em). I guess it is >> >> time to helper-up. >> >> >> >> I'll update omapdrm and tilcdc to use this as well, but I figured I'd >> >> send an RFC first in case anyone wants to get their bikeshed on. If >> >> there are other drivers that could use this, and are straightforward >> >> to convert over, let me know and I can update them as well. >> > >> > >> > One thing drm/i915 needs is to be able to flush the workqueue (to make >> > sure we don't pile up giant amounts of buffers waiting to be unpinned >> > and so temporarily leak a bit of memory). So some way to synchronously >> > flush out flip functions would be required (and make sure all that >> > have been queued up to that point are really completed). But at that >> > point a separate workqueue sounds simpler, so I wonder a bit what this >> > gains us? At roughly 50Hz flip work functions aren't really that >> > performance critical imo ... >> >> you could of course do separate or same workqueue from your main >> render-complete workqueue, or flush the workqueue at whatever point >> you want.. I suppose if you are queuing from a context that is safe >> to sleep you could have a blocking-queue function. I haven't really >> needed anything like that so far, but feel free to extend the >> flip-work helper however you need to be useful for i915. (Or point me >> at the bits in i915 that do something similar and I'll have a think >> about how to convert that) > > queue_work in do_intel_finish_page_flip and flush_work in > intel_crtc_page_flip is the relevant code. The former is called from irq > context. ok, it looks like intel_crtc_page_flip() would call drm_flip_work_queue() and then something like: if (kfifo_len(&flipwork->fifo) >= 2) flush_workqueue(wq) maybe a static inline fxn to wrap the kfifo_len().. but that is the basic idea. Although seems like you have a few other things wrapped up in intel_unpin_work. I guess you could move the pending event to the crtc because you can only have one pending flip at a time (and maybe enable_stall_check I didn't really look at the stall-check stuff..) BR, -R > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 801bcaf..9781469 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -13,7 +13,7 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \ drm_crtc.o drm_modes.o drm_edid.o \ drm_info.o drm_debugfs.o drm_encoder_slave.o \ drm_trace_points.o drm_global.o drm_prime.o \ - drm_rect.o + drm_rect.o drm_flip_work.o drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o diff --git a/drivers/gpu/drm/drm_flip_work.c b/drivers/gpu/drm/drm_flip_work.c new file mode 100644 index 0000000..e788882 --- /dev/null +++ b/drivers/gpu/drm/drm_flip_work.c @@ -0,0 +1,124 @@ +/* + * Copyright (C) 2013 Red Hat + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +#include "drmP.h" +#include "drm_flip_work.h" + +/** + * drm_flip_work_queue - queue work + * @work: the flip-work + * @val: the value to queue + * + * Queues work, 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(struct drm_flip_work *work, void *val) +{ + if (kfifo_put(&work->fifo, (const void **)&val)) { + atomic_inc(&work->pending); + } else { + DRM_ERROR("%s fifo full!\n", work->name); + work->func(work, val); + } +} +EXPORT_SYMBOL(drm_flip_work_queue); + +/** + * drm_flip_work_commit - commit queued work + * @work: the flip-work + * @wq: the work-queue to run the queued work on + * + * Trigger work previously queued by drm_flip_work_queue() to run + * on a workqueue. The typical usage would be to queue work (via + * drm_flip_work_queue()) at any point (from vblank irq and/or + * prior), and then from vblank irq commit the queued work. + */ +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); + queue_work(wq, &work->worker); +} +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; + + atomic_sub(count, &work->count); + + while(count--) + if (!WARN_ON(!kfifo_get(&work->fifo, &val))) + work->func(work, val); +} + +/** + * 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, + const char *name, drm_flip_func_t func) +{ + int ret; + + work->name = name; + atomic_set(&work->count, 0); + atomic_set(&work->pending, 0); + 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; +} +EXPORT_SYMBOL(drm_flip_work_init); + +/** + * drm_flip_work_cleanup - cleans up flip-work + * @work: the flip-work to cleanup + * + * Destroy resources allocated for the flip-work + */ +void drm_flip_work_cleanup(struct drm_flip_work *work) +{ + WARN_ON(!kfifo_is_empty(&work->fifo)); + kfifo_free(&work->fifo); +} +EXPORT_SYMBOL(drm_flip_work_cleanup); diff --git a/include/drm/drm_flip_work.h b/include/drm/drm_flip_work.h new file mode 100644 index 0000000..7d3840b --- /dev/null +++ b/include/drm/drm_flip_work.h @@ -0,0 +1,71 @@ +/* + * Copyright (C) 2013 Red Hat + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +#ifndef DRM_FLIP_WORK_H +#define DRM_FLIP_WORK_H + +#include <linux/kfifo.h> +#include <linux/workqueue.h> + +/** + * DOC: flip utils + * + * 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. + */ + +struct drm_flip_work; + +/** + * drm_flip_func_t - callback function + * + * @work: the flip work + * @val: value queued via drm_flip_work_queue() + * + * Callback function to be called for each of the queue'd work items after + * drm_flip_work_commit() is called. + */ +typedef void (*drm_flip_func_t)(struct drm_flip_work *work, void *val); + +/** + * struct drm_flip_work - flip work queue + */ +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 *); +}; + +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, + const char *name, drm_flip_func_t func); +void drm_flip_work_cleanup(struct drm_flip_work *work); + +#endif /* DRM_FLIP_WORK_H */
A small helper to queue up work to do, from workqueue context, after a flip. Typically useful to defer unreffing buffers that may be read by the display controller until vblank. Signed-off-by: Rob Clark <robdclark@gmail.com> --- I've re-inventing the same wheel three times in as many drivers (omapdrm, tilcdc, and in upcoming msm driver I need two of 'em). I guess it is time to helper-up. I'll update omapdrm and tilcdc to use this as well, but I figured I'd send an RFC first in case anyone wants to get their bikeshed on. If there are other drivers that could use this, and are straightforward to convert over, let me know and I can update them as well. drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/drm_flip_work.c | 124 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_flip_work.h | 71 +++++++++++++++++++++++ 3 files changed, 196 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_flip_work.c create mode 100644 include/drm/drm_flip_work.h