cx23885-input.c does in fact use a workqueue....
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:
Here's where it is scheduled for CX23885 chips:
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
Quoted 43 lines · Andy Walls
Tejun,
I just noticed this commit:
commit
8c71778cbf2c8beaefaa2dee5478aa0622d96682Author: Tejun Heo <[email protected]> Date: Fri Dec 24 16:14:20 2010 +0100media/video: don't use
flush_scheduled_work()This patch converts the remaining users of
flush_scheduled_work()in media/video.*
bttv-input.candcx23885-input.cdon'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:
Here's where it is scheduled for CX23885 chips:
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.
Quoted 3 lines
So what should be done about the
flush_scheduled_work()? I think it belongs there.
Convert to using threaded irq?
Signature · 1 line
Dmitry
Hello,
Quoted 9 lines · Dmitry Torokhov
Quoted 9 lines
The cx23885 driver does in fact schedule work for IR input handling:
Here's where it is scheduled for CX23888 chips:
Here's where it is scheduled for CX23885 chips:
Ah, sorry about missing those.
Quoted 14 lines
Quoted 14 lines
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.
Quoted 5 lines
Quoted 3 lines
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.
drivers/media/video/cx23885/cx23885-input.c +3 -0
@@ -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)
Signature · 1 line
tejun
Andy Walls
Reviewed-byQuoted 7 lines · Tejun Heo
Hello,
Quoted 4 lines · Dmitry Torokhov
Quoted 4 lines
The cx23885 driver does in fact schedule work for IR input handling:
Here's where it is scheduled for CX23888 chips:
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Here's where it is scheduled for CX23885 chips:
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.)
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 3 lines
Or
1. Just flush the work items explicitly using
flush_work_sync().
That will do for now.
Quoted 1 line
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.
Quoted 1 line
The first would look like the following. Does this look correct?
Yes, your patch below looks sane to me.
Thanks,
Andy
Quoted 17 lines
Thanks.
drivers/media/video/cx23885/cx23885-input.c +3
-0@@ -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,
Quoted 11 lines · Andy Walls
Quoted 1 line
1. Just flush the work items explicitly using
flush_work_sync().That will do for now.
Quoted 1 line
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_workerwith 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.
Quoted 5 lines
Quoted 1 line
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.
Signature · 1 line
tejun
PATCH 1/1 cx23885: restore flushes of cx23885_dev work items
In reply to Andy WallsCommit 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.
--- drivers/media/video/cx23885/cx23885-input.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/media/video/cx23885/cx23885-input.c +3 -0
@@ -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)
Re: [PATCH] cx23885: restore flushes of cx23885_dev work items
Em 15-02-2011 07:20, Tejun Heo escreveu:
Quoted 6 lines
Commit
8c71778c(media/video: don't useflush_scheduled_work()) droppedflush_scheduled_work()fromcx23885_input_ir_stop()incorrectly assuming that it didn't use any work item; however,cx23885_devmakes use of three work items -cx25840_workand ir_{r|t}x_work.
Quoted 8 lines
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]>
Quoted 18 lines
--- drivers/media/video/cx23885/cx23885-input.c | 3 +++ 1 file changed, 3 insertions(+)drivers/media/video/cx23885/cx23885-input.c +3
-0@@ -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)