| Message ID | 20180804124526.46206-31-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Request API | expand |
Em Sat, 4 Aug 2018 14:45:22 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > From: Hans Verkuil <hans.verkuil@cisco.com> > > v4l2_ctrl uses mutexes, so we can't setup a ctrl_handler in > interrupt context. Switch to a workqueue instead and drop the timer. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> Reviewed-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Shouldn't this come earlier at the series (before adding request API support to m2m) in order to avoid regressions? > --- > drivers/media/platform/vim2m.c | 25 ++++++++++--------------- > 1 file changed, 10 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c > index 462099a141e4..6f87ef025ff1 100644 > --- a/drivers/media/platform/vim2m.c > +++ b/drivers/media/platform/vim2m.c > @@ -3,7 +3,8 @@ > * > * This is a virtual device driver for testing mem-to-mem videobuf framework. > * It simulates a device that uses memory buffers for both source and > - * destination, processes the data and issues an "irq" (simulated by a timer). > + * destination, processes the data and issues an "irq" (simulated by a delayed > + * workqueue). > * The device is capable of multi-instance, multi-buffer-per-transaction > * operation (via the mem2mem framework). > * > @@ -19,7 +20,6 @@ > #include <linux/module.h> > #include <linux/delay.h> > #include <linux/fs.h> > -#include <linux/timer.h> > #include <linux/sched.h> > #include <linux/slab.h> > > @@ -148,7 +148,7 @@ struct vim2m_dev { > struct mutex dev_mutex; > spinlock_t irqlock; > > - struct timer_list timer; > + struct delayed_work work_run; > > struct v4l2_m2m_dev *m2m_dev; > }; > @@ -336,12 +336,6 @@ static int device_process(struct vim2m_ctx *ctx, > return 0; > } > > -static void schedule_irq(struct vim2m_dev *dev, int msec_timeout) > -{ > - dprintk(dev, "Scheduling a simulated irq\n"); > - mod_timer(&dev->timer, jiffies + msecs_to_jiffies(msec_timeout)); > -} > - > /* > * mem2mem callbacks > */ > @@ -387,13 +381,14 @@ static void device_run(void *priv) > > device_process(ctx, src_buf, dst_buf); > > - /* Run a timer, which simulates a hardware irq */ > - schedule_irq(dev, ctx->transtime); > + /* Run delayed work, which simulates a hardware irq */ > + schedule_delayed_work(&dev->work_run, msecs_to_jiffies(ctx->transtime)); > } > > -static void device_isr(struct timer_list *t) > +static void device_work(struct work_struct *w) > { > - struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t, timer); > + struct vim2m_dev *vim2m_dev = > + container_of(w, struct vim2m_dev, work_run.work); > struct vim2m_ctx *curr_ctx; > struct vb2_v4l2_buffer *src_vb, *dst_vb; > unsigned long flags; > @@ -805,6 +800,7 @@ static void vim2m_stop_streaming(struct vb2_queue *q) > struct vb2_v4l2_buffer *vbuf; > unsigned long flags; > > + flush_scheduled_work(); > for (;;) { > if (V4L2_TYPE_IS_OUTPUT(q->type)) > vbuf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); > @@ -1015,6 +1011,7 @@ static int vim2m_probe(struct platform_device *pdev) > vfd = &dev->vfd; > vfd->lock = &dev->dev_mutex; > vfd->v4l2_dev = &dev->v4l2_dev; > + INIT_DELAYED_WORK(&dev->work_run, device_work); > > ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0); > if (ret) { > @@ -1026,7 +1023,6 @@ static int vim2m_probe(struct platform_device *pdev) > v4l2_info(&dev->v4l2_dev, > "Device registered as /dev/video%d\n", vfd->num); > > - timer_setup(&dev->timer, device_isr, 0); > platform_set_drvdata(pdev, dev); > > dev->m2m_dev = v4l2_m2m_init(&m2m_ops); > @@ -1083,7 +1079,6 @@ static int vim2m_remove(struct platform_device *pdev) > media_device_cleanup(&dev->mdev); > #endif > v4l2_m2m_release(dev->m2m_dev); > - del_timer_sync(&dev->timer); > video_unregister_device(&dev->vfd); > v4l2_device_unregister(&dev->v4l2_dev); > Thanks, Mauro
On 13/08/18 17:05, Mauro Carvalho Chehab wrote: > Em Sat, 4 Aug 2018 14:45:22 +0200 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> From: Hans Verkuil <hans.verkuil@cisco.com> >> >> v4l2_ctrl uses mutexes, so we can't setup a ctrl_handler in >> interrupt context. Switch to a workqueue instead and drop the timer. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > Reviewed-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> > > Shouldn't this come earlier at the series (before adding request API > support to m2m) in order to avoid regressions? ??? At this stage vim2m doesn't support the request API yet. It's the next patch that adds that (and that's when this patch is needed for it to work). Regards, Hans > >> --- >> drivers/media/platform/vim2m.c | 25 ++++++++++--------------- >> 1 file changed, 10 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c >> index 462099a141e4..6f87ef025ff1 100644 >> --- a/drivers/media/platform/vim2m.c >> +++ b/drivers/media/platform/vim2m.c >> @@ -3,7 +3,8 @@ >> * >> * This is a virtual device driver for testing mem-to-mem videobuf framework. >> * It simulates a device that uses memory buffers for both source and >> - * destination, processes the data and issues an "irq" (simulated by a timer). >> + * destination, processes the data and issues an "irq" (simulated by a delayed >> + * workqueue). >> * The device is capable of multi-instance, multi-buffer-per-transaction >> * operation (via the mem2mem framework). >> * >> @@ -19,7 +20,6 @@ >> #include <linux/module.h> >> #include <linux/delay.h> >> #include <linux/fs.h> >> -#include <linux/timer.h> >> #include <linux/sched.h> >> #include <linux/slab.h> >> >> @@ -148,7 +148,7 @@ struct vim2m_dev { >> struct mutex dev_mutex; >> spinlock_t irqlock; >> >> - struct timer_list timer; >> + struct delayed_work work_run; >> >> struct v4l2_m2m_dev *m2m_dev; >> }; >> @@ -336,12 +336,6 @@ static int device_process(struct vim2m_ctx *ctx, >> return 0; >> } >> >> -static void schedule_irq(struct vim2m_dev *dev, int msec_timeout) >> -{ >> - dprintk(dev, "Scheduling a simulated irq\n"); >> - mod_timer(&dev->timer, jiffies + msecs_to_jiffies(msec_timeout)); >> -} >> - >> /* >> * mem2mem callbacks >> */ >> @@ -387,13 +381,14 @@ static void device_run(void *priv) >> >> device_process(ctx, src_buf, dst_buf); >> >> - /* Run a timer, which simulates a hardware irq */ >> - schedule_irq(dev, ctx->transtime); >> + /* Run delayed work, which simulates a hardware irq */ >> + schedule_delayed_work(&dev->work_run, msecs_to_jiffies(ctx->transtime)); >> } >> >> -static void device_isr(struct timer_list *t) >> +static void device_work(struct work_struct *w) >> { >> - struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t, timer); >> + struct vim2m_dev *vim2m_dev = >> + container_of(w, struct vim2m_dev, work_run.work); >> struct vim2m_ctx *curr_ctx; >> struct vb2_v4l2_buffer *src_vb, *dst_vb; >> unsigned long flags; >> @@ -805,6 +800,7 @@ static void vim2m_stop_streaming(struct vb2_queue *q) >> struct vb2_v4l2_buffer *vbuf; >> unsigned long flags; >> >> + flush_scheduled_work(); >> for (;;) { >> if (V4L2_TYPE_IS_OUTPUT(q->type)) >> vbuf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); >> @@ -1015,6 +1011,7 @@ static int vim2m_probe(struct platform_device *pdev) >> vfd = &dev->vfd; >> vfd->lock = &dev->dev_mutex; >> vfd->v4l2_dev = &dev->v4l2_dev; >> + INIT_DELAYED_WORK(&dev->work_run, device_work); >> >> ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0); >> if (ret) { >> @@ -1026,7 +1023,6 @@ static int vim2m_probe(struct platform_device *pdev) >> v4l2_info(&dev->v4l2_dev, >> "Device registered as /dev/video%d\n", vfd->num); >> >> - timer_setup(&dev->timer, device_isr, 0); >> platform_set_drvdata(pdev, dev); >> >> dev->m2m_dev = v4l2_m2m_init(&m2m_ops); >> @@ -1083,7 +1079,6 @@ static int vim2m_remove(struct platform_device *pdev) >> media_device_cleanup(&dev->mdev); >> #endif >> v4l2_m2m_release(dev->m2m_dev); >> - del_timer_sync(&dev->timer); >> video_unregister_device(&dev->vfd); >> v4l2_device_unregister(&dev->v4l2_dev); >> > > > > Thanks, > Mauro >
Em Tue, 14 Aug 2018 09:28:42 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 13/08/18 17:05, Mauro Carvalho Chehab wrote: > > Em Sat, 4 Aug 2018 14:45:22 +0200 > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > >> From: Hans Verkuil <hans.verkuil@cisco.com> > >> > >> v4l2_ctrl uses mutexes, so we can't setup a ctrl_handler in > >> interrupt context. Switch to a workqueue instead and drop the timer. > >> > >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > > > Reviewed-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> > > > > Shouldn't this come earlier at the series (before adding request API > > support to m2m) in order to avoid regressions? > > ??? At this stage vim2m doesn't support the request API yet. It's the next > patch that adds that (and that's when this patch is needed for it to work). Ah, OK! > > Regards, > > Hans > > > > >> --- > >> drivers/media/platform/vim2m.c | 25 ++++++++++--------------- > >> 1 file changed, 10 insertions(+), 15 deletions(-) > >> > >> diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c > >> index 462099a141e4..6f87ef025ff1 100644 > >> --- a/drivers/media/platform/vim2m.c > >> +++ b/drivers/media/platform/vim2m.c > >> @@ -3,7 +3,8 @@ > >> * > >> * This is a virtual device driver for testing mem-to-mem videobuf framework. > >> * It simulates a device that uses memory buffers for both source and > >> - * destination, processes the data and issues an "irq" (simulated by a timer). > >> + * destination, processes the data and issues an "irq" (simulated by a delayed > >> + * workqueue). > >> * The device is capable of multi-instance, multi-buffer-per-transaction > >> * operation (via the mem2mem framework). > >> * > >> @@ -19,7 +20,6 @@ > >> #include <linux/module.h> > >> #include <linux/delay.h> > >> #include <linux/fs.h> > >> -#include <linux/timer.h> > >> #include <linux/sched.h> > >> #include <linux/slab.h> > >> > >> @@ -148,7 +148,7 @@ struct vim2m_dev { > >> struct mutex dev_mutex; > >> spinlock_t irqlock; > >> > >> - struct timer_list timer; > >> + struct delayed_work work_run; > >> > >> struct v4l2_m2m_dev *m2m_dev; > >> }; > >> @@ -336,12 +336,6 @@ static int device_process(struct vim2m_ctx *ctx, > >> return 0; > >> } > >> > >> -static void schedule_irq(struct vim2m_dev *dev, int msec_timeout) > >> -{ > >> - dprintk(dev, "Scheduling a simulated irq\n"); > >> - mod_timer(&dev->timer, jiffies + msecs_to_jiffies(msec_timeout)); > >> -} > >> - > >> /* > >> * mem2mem callbacks > >> */ > >> @@ -387,13 +381,14 @@ static void device_run(void *priv) > >> > >> device_process(ctx, src_buf, dst_buf); > >> > >> - /* Run a timer, which simulates a hardware irq */ > >> - schedule_irq(dev, ctx->transtime); > >> + /* Run delayed work, which simulates a hardware irq */ > >> + schedule_delayed_work(&dev->work_run, msecs_to_jiffies(ctx->transtime)); > >> } > >> > >> -static void device_isr(struct timer_list *t) > >> +static void device_work(struct work_struct *w) > >> { > >> - struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t, timer); > >> + struct vim2m_dev *vim2m_dev = > >> + container_of(w, struct vim2m_dev, work_run.work); > >> struct vim2m_ctx *curr_ctx; > >> struct vb2_v4l2_buffer *src_vb, *dst_vb; > >> unsigned long flags; > >> @@ -805,6 +800,7 @@ static void vim2m_stop_streaming(struct vb2_queue *q) > >> struct vb2_v4l2_buffer *vbuf; > >> unsigned long flags; > >> > >> + flush_scheduled_work(); > >> for (;;) { > >> if (V4L2_TYPE_IS_OUTPUT(q->type)) > >> vbuf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); > >> @@ -1015,6 +1011,7 @@ static int vim2m_probe(struct platform_device *pdev) > >> vfd = &dev->vfd; > >> vfd->lock = &dev->dev_mutex; > >> vfd->v4l2_dev = &dev->v4l2_dev; > >> + INIT_DELAYED_WORK(&dev->work_run, device_work); > >> > >> ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0); > >> if (ret) { > >> @@ -1026,7 +1023,6 @@ static int vim2m_probe(struct platform_device *pdev) > >> v4l2_info(&dev->v4l2_dev, > >> "Device registered as /dev/video%d\n", vfd->num); > >> > >> - timer_setup(&dev->timer, device_isr, 0); > >> platform_set_drvdata(pdev, dev); > >> > >> dev->m2m_dev = v4l2_m2m_init(&m2m_ops); > >> @@ -1083,7 +1079,6 @@ static int vim2m_remove(struct platform_device *pdev) > >> media_device_cleanup(&dev->mdev); > >> #endif > >> v4l2_m2m_release(dev->m2m_dev); > >> - del_timer_sync(&dev->timer); > >> video_unregister_device(&dev->vfd); > >> v4l2_device_unregister(&dev->v4l2_dev); > >> > > > > > > > > Thanks, > > Mauro > > > Thanks, Mauro
diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c index 462099a141e4..6f87ef025ff1 100644 --- a/drivers/media/platform/vim2m.c +++ b/drivers/media/platform/vim2m.c @@ -3,7 +3,8 @@ * * This is a virtual device driver for testing mem-to-mem videobuf framework. * It simulates a device that uses memory buffers for both source and - * destination, processes the data and issues an "irq" (simulated by a timer). + * destination, processes the data and issues an "irq" (simulated by a delayed + * workqueue). * The device is capable of multi-instance, multi-buffer-per-transaction * operation (via the mem2mem framework). * @@ -19,7 +20,6 @@ #include <linux/module.h> #include <linux/delay.h> #include <linux/fs.h> -#include <linux/timer.h> #include <linux/sched.h> #include <linux/slab.h> @@ -148,7 +148,7 @@ struct vim2m_dev { struct mutex dev_mutex; spinlock_t irqlock; - struct timer_list timer; + struct delayed_work work_run; struct v4l2_m2m_dev *m2m_dev; }; @@ -336,12 +336,6 @@ static int device_process(struct vim2m_ctx *ctx, return 0; } -static void schedule_irq(struct vim2m_dev *dev, int msec_timeout) -{ - dprintk(dev, "Scheduling a simulated irq\n"); - mod_timer(&dev->timer, jiffies + msecs_to_jiffies(msec_timeout)); -} - /* * mem2mem callbacks */ @@ -387,13 +381,14 @@ static void device_run(void *priv) device_process(ctx, src_buf, dst_buf); - /* Run a timer, which simulates a hardware irq */ - schedule_irq(dev, ctx->transtime); + /* Run delayed work, which simulates a hardware irq */ + schedule_delayed_work(&dev->work_run, msecs_to_jiffies(ctx->transtime)); } -static void device_isr(struct timer_list *t) +static void device_work(struct work_struct *w) { - struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t, timer); + struct vim2m_dev *vim2m_dev = + container_of(w, struct vim2m_dev, work_run.work); struct vim2m_ctx *curr_ctx; struct vb2_v4l2_buffer *src_vb, *dst_vb; unsigned long flags; @@ -805,6 +800,7 @@ static void vim2m_stop_streaming(struct vb2_queue *q) struct vb2_v4l2_buffer *vbuf; unsigned long flags; + flush_scheduled_work(); for (;;) { if (V4L2_TYPE_IS_OUTPUT(q->type)) vbuf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); @@ -1015,6 +1011,7 @@ static int vim2m_probe(struct platform_device *pdev) vfd = &dev->vfd; vfd->lock = &dev->dev_mutex; vfd->v4l2_dev = &dev->v4l2_dev; + INIT_DELAYED_WORK(&dev->work_run, device_work); ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0); if (ret) { @@ -1026,7 +1023,6 @@ static int vim2m_probe(struct platform_device *pdev) v4l2_info(&dev->v4l2_dev, "Device registered as /dev/video%d\n", vfd->num); - timer_setup(&dev->timer, device_isr, 0); platform_set_drvdata(pdev, dev); dev->m2m_dev = v4l2_m2m_init(&m2m_ops); @@ -1083,7 +1079,6 @@ static int vim2m_remove(struct platform_device *pdev) media_device_cleanup(&dev->mdev); #endif v4l2_m2m_release(dev->m2m_dev); - del_timer_sync(&dev->timer); video_unregister_device(&dev->vfd); v4l2_device_unregister(&dev->v4l2_dev);