Tejun,
I just noticed this commit:
commit 8c71778cbf2c8beaefaa2dee5478aa0622d96682
Author: Tejun Heo <[email protected]>
Date: Fri Dec 24 16:14:20 2010 +0100
media/video: don't use flush_scheduled_work()
This patch converts the remaining users of flush_scheduled_work() in
media/video.
* bttv-input.c and cx23885-input.c don't use workqueue at all. No
need to flush.
[...]
The cx23885 driver does in fact schedule work for IR input handling:
Here's where it is scheduled for CX23888 chips:
http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/cx23885/cx23885-ir.c;h=7125247dd25558678c823ee3262675570c9aa630;hb=HEAD#l76
Here's where it is scheduled for CX23885 chips:
http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/cx23885/cx23885-core.c;h=359882419b7f588b7c698dbcfb6a39ddb1603301;hb=HEAD#l1861
The two different chips are handled slightly differently because
a. the CX23888 IR unit is accessable via a PCI register block. The IR
IRQ can be acknowledged with direct PCI register accesses in an
interrupt context, and the IR pulse FIFO serviced later in a workqueue
context.
b. the CX23885 IR unit is accessed over an I2C bus. The CX23885 A/V IRQ
has to be masked in an interrupt context (with PCI registers accesses).
Then the CX23885 A/V unit's IR IRQ is ack'ed over I2C in a workqueue
context and the IR pulse FIFO is also serviced over I2C in a workqueue
context.
So what should be done about the flush_scheduled_work()? I think it
belongs there.
Regards,
Andy
On Sun, Feb 13, 2011 at 08:35:22PM -0500, Andy Walls wrote:
> Tejun,
>
> I just noticed this commit:
>
> commit 8c71778cbf2c8beaefaa2dee5478aa0622d96682
> Author: Tejun Heo <[email protected]>
> Date: Fri Dec 24 16:14:20 2010 +0100
>
> media/video: don't use flush_scheduled_work()
>
> This patch converts the remaining users of flush_scheduled_work() in
> media/video.
>
> * bttv-input.c and cx23885-input.c don't use workqueue at all. No
> need to flush.
> [...]
>
>
> The cx23885 driver does in fact schedule work for IR input handling:
>
> Here's where it is scheduled for CX23888 chips:
>
> http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/cx23885/cx23885-ir.c;h=7125247dd25558678c823ee3262675570c9aa630;hb=HEAD#l76
>
> Here's where it is scheduled for CX23885 chips:
>
> http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/cx23885/cx23885-core.c;h=359882419b7f588b7c698dbcfb6a39ddb1603301;hb=HEAD#l1861
>
>
> The two different chips are handled slightly differently because
>
> a. the CX23888 IR unit is accessable via a PCI register block. The IR
> IRQ can be acknowledged with direct PCI register accesses in an
> interrupt context, and the IR pulse FIFO serviced later in a workqueue
> context.
>
> b. the CX23885 IR unit is accessed over an I2C bus. The CX23885 A/V IRQ
> has to be masked in an interrupt context (with PCI registers accesses).
> Then the CX23885 A/V unit's IR IRQ is ack'ed over I2C in a workqueue
> context and the IR pulse FIFO is also serviced over I2C in a workqueue
> context.
>
>
> So what should be done about the flush_scheduled_work()? I think it
> belongs there.
>
Convert to using threaded irq?
--
Dmitry
Hello,
On Sun, Feb 13, 2011 at 08:33:55PM -0800, Dmitry Torokhov wrote:
> > The cx23885 driver does in fact schedule work for IR input handling:
> >
> > Here's where it is scheduled for CX23888 chips:
> >
> > http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/cx23885/cx23885-ir.c;h=7125247dd25558678c823ee3262675570c9aa630;hb=HEAD#l76
> >
> > Here's where it is scheduled for CX23885 chips:
> >
> > http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/cx23885/cx23885-core.c;h=359882419b7f588b7c698dbcfb6a39ddb1603301;hb=HEAD#l1861
Ah, sorry about missing those.
> > The two different chips are handled slightly differently because
> >
> > a. the CX23888 IR unit is accessable via a PCI register block. The IR
> > IRQ can be acknowledged with direct PCI register accesses in an
> > interrupt context, and the IR pulse FIFO serviced later in a workqueue
> > context.
> >
> > b. the CX23885 IR unit is accessed over an I2C bus. The CX23885 A/V IRQ
> > has to be masked in an interrupt context (with PCI registers accesses).
> > Then the CX23885 A/V unit's IR IRQ is ack'ed over I2C in a workqueue
> > context and the IR pulse FIFO is also serviced over I2C in a workqueue
> > context.
> >
> >
> > So what should be done about the flush_scheduled_work()? I think it
> > belongs there.
> >
>
> Convert to using threaded irq?
Or
1. Just flush the work items explicitly using flush_work_sync().
2. Create a dedicated workqueue to serve as flushing domain.
The first would look like the following. Does this look correct?
Thanks.
diff --git a/drivers/media/video/cx23885/cx23885-input.c b/drivers/media/video/cx23885/cx23885-input.c
index 199b996..e27cedb 100644
--- a/drivers/media/video/cx23885/cx23885-input.c
+++ b/drivers/media/video/cx23885/cx23885-input.c
@@ -229,6 +229,9 @@ static void cx23885_input_ir_stop(struct cx23885_dev *dev)
v4l2_subdev_call(dev->sd_ir, ir, rx_s_parameters, ¶ms);
v4l2_subdev_call(dev->sd_ir, ir, rx_g_parameters, ¶ms);
}
+ flush_work_sync(&dev->cx25840_work);
+ flush_work_sync(&dev->ir_rx_work);
+ flush_work_sync(&dev->ir_tx_work);
}
static void cx23885_input_ir_close(struct rc_dev *rc)
--
tejun
On Mon, 2011-02-14 at 12:03 +0100, Tejun Heo wrote:
> Hello,
>
> On Sun, Feb 13, 2011 at 08:33:55PM -0800, Dmitry Torokhov wrote:
> > > The cx23885 driver does in fact schedule work for IR input handling:
> > >
> > > Here's where it is scheduled for CX23888 chips:
> > >
> > > http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/cx23885/cx23885-ir.c;h=7125247dd25558678c823ee3262675570c9aa630;hb=HEAD#l76
> > >
> > > Here's where it is scheduled for CX23885 chips:
> > >
> > > http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/cx23885/cx23885-core.c;h=359882419b7f588b7c698dbcfb6a39ddb1603301;hb=HEAD#l1861
>
> Ah, sorry about missing those.
There's a lot of indirection in drivers under drivers/media/video so it
doesn't surprise me when someone misses something.
(BTW, I only checked cx23885 since I wrote the IR handling and added the
work queue usage for IR that driver.)
> > > So what should be done about the flush_scheduled_work()? I think it
> > > belongs there.
> > >
> >
> > Convert to using threaded irq?
Too big of a regression testing job for me to take on at the moment.
> Or
>
> 1. Just flush the work items explicitly using flush_work_sync().
That will do for now.
> 2. Create a dedicated workqueue to serve as flushing domain.
I have gotten reports of the IR Rx FIFO overflows for the CX23885 IR Rx
unit (the I2C connected one). I eventually should either set the Rx
FIFO service interrupt watermark down from 4 measurments to 1
measurment, or use a kthread_worker with some higher priority to respond
to the IR Rx FIFO service interrupt.
> The first would look like the following. Does this look correct?
Yes, your patch below looks sane to me.
Reviewed-by: Andy Walls <[email protected]>
Thanks,
Andy
> Thanks.
>
> diff --git a/drivers/media/video/cx23885/cx23885-input.c b/drivers/media/video/cx23885/cx23885-input.c
> index 199b996..e27cedb 100644
> --- a/drivers/media/video/cx23885/cx23885-input.c
> +++ b/drivers/media/video/cx23885/cx23885-input.c
> @@ -229,6 +229,9 @@ static void cx23885_input_ir_stop(struct cx23885_dev *dev)
> v4l2_subdev_call(dev->sd_ir, ir, rx_s_parameters, ¶ms);
> v4l2_subdev_call(dev->sd_ir, ir, rx_g_parameters, ¶ms);
> }
> + flush_work_sync(&dev->cx25840_work);
> + flush_work_sync(&dev->ir_rx_work);
> + flush_work_sync(&dev->ir_tx_work);
> }
>
> static void cx23885_input_ir_close(struct rc_dev *rc)
>
Hello,
On Mon, Feb 14, 2011 at 07:54:36PM -0500, Andy Walls wrote:
> > 1. Just flush the work items explicitly using flush_work_sync().
>
> That will do for now.
>
> > 2. Create a dedicated workqueue to serve as flushing domain.
>
> I have gotten reports of the IR Rx FIFO overflows for the CX23885 IR Rx
> unit (the I2C connected one). I eventually should either set the Rx
> FIFO service interrupt watermark down from 4 measurments to 1
> measurment, or use a kthread_worker with some higher priority to respond
> to the IR Rx FIFO service interrupt.
Hmmm... please consider playing with WQ_HIGHPRI before going forward
with dedicated thread.
> > The first would look like the following. Does this look correct?
>
> Yes, your patch below looks sane to me.
>
> Reviewed-by: Andy Walls <[email protected]>
Thanks. Will send patch with proper description soon.
--
tejun
Commit 8c71778c (media/video: don't use flush_scheduled_work())
dropped flush_scheduled_work() from cx23885_input_ir_stop()
incorrectly assuming that it didn't use any work item; however,
cx23885_dev makes use of three work items - cx25840_work and
ir_{r|t}x_work.
Make cx23885_input_ir_stop() sync flush all three work items before
returning.
Signed-off-by: Tejun Heo <[email protected]>
Reported-by: Andy Walls <[email protected]>
Reviewed-by: Andy Walls <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
---
drivers/media/video/cx23885/cx23885-input.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/video/cx23885/cx23885-input.c b/drivers/media/video/cx23885/cx23885-input.c
index 199b996..e27cedb 100644
--- a/drivers/media/video/cx23885/cx23885-input.c
+++ b/drivers/media/video/cx23885/cx23885-input.c
@@ -229,6 +229,9 @@ static void cx23885_input_ir_stop(struct cx23885_dev *dev)
v4l2_subdev_call(dev->sd_ir, ir, rx_s_parameters, ¶ms);
v4l2_subdev_call(dev->sd_ir, ir, rx_g_parameters, ¶ms);
}
+ flush_work_sync(&dev->cx25840_work);
+ flush_work_sync(&dev->ir_rx_work);
+ flush_work_sync(&dev->ir_tx_work);
}
static void cx23885_input_ir_close(struct rc_dev *rc)
Em 15-02-2011 07:20, Tejun Heo escreveu:
> Commit 8c71778c (media/video: don't use flush_scheduled_work())
> dropped flush_scheduled_work() from cx23885_input_ir_stop()
> incorrectly assuming that it didn't use any work item; however,
> cx23885_dev makes use of three work items - cx25840_work and
> ir_{r|t}x_work.
>
> Make cx23885_input_ir_stop() sync flush all three work items before
> returning.
>
> Signed-off-by: Tejun Heo <[email protected]>
> Reported-by: Andy Walls <[email protected]>
> Reviewed-by: Andy Walls <[email protected]>
> Cc: Dmitry Torokhov <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
Acked-by: Mauro Carvalho Chehab <[email protected]>
> ---
> drivers/media/video/cx23885/cx23885-input.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/video/cx23885/cx23885-input.c b/drivers/media/video/cx23885/cx23885-input.c
> index 199b996..e27cedb 100644
> --- a/drivers/media/video/cx23885/cx23885-input.c
> +++ b/drivers/media/video/cx23885/cx23885-input.c
> @@ -229,6 +229,9 @@ static void cx23885_input_ir_stop(struct cx23885_dev *dev)
> v4l2_subdev_call(dev->sd_ir, ir, rx_s_parameters, ¶ms);
> v4l2_subdev_call(dev->sd_ir, ir, rx_g_parameters, ¶ms);
> }
> + flush_work_sync(&dev->cx25840_work);
> + flush_work_sync(&dev->ir_rx_work);
> + flush_work_sync(&dev->ir_tx_work);
> }
>
> static void cx23885_input_ir_close(struct rc_dev *rc)