[PATCH] [media] rc: do not sleep when the driver blocks on IR completion

[PATCH] [media] rc: do not sleep when the driver blocks on IR completion

[Sean Young]

Some drivers wait for the IR device to complete sending before
returning, so sleeping should not be done.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/iguanair.c      | 1 +
 drivers/media/rc/ir-lirc-codec.c | 5 +++++
 drivers/media/rc/winbond-cir.c   | 1 +
 include/media/rc-core.h          | 2 ++
 4 files changed, 9 insertions(+)

diff --git a/drivers/media/rc/iguanair.c b/drivers/media/rc/iguanair.c
index 66ba237..7f1941d 100644
--- a/drivers/media/rc/iguanair.c
+++ b/drivers/media/rc/iguanair.c
@@ -519,6 +519,7 @@ static int __devinit iguanair_probe(struct usb_interface *intf,
 	rc->s_tx_mask = iguanair_set_tx_mask;
 	rc->s_tx_carrier = iguanair_set_tx_carrier;
 	rc->tx_ir = iguanair_tx;
+	rc->tx_ir_drains = 1;
 	rc->driver_name = DRIVER_NAME;
 	rc->map_name = RC_MAP_RC6_MCE;
 	rc->timeout = MS_TO_NS(100);
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 569124b..dd21917 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -144,6 +144,11 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
 	if (ret < 0)
 		goto out;
 
+	if (dev->tx_ir_drains) {
+		ret *= sizeof(unsigned int);
+		goto out;
+	}
+
 	for (i = 0; i < ret; i++)
 		duration += txbuf[i];
 
diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 54ee348..b1b6d34 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -1029,6 +1029,7 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
 	data->dev->s_idle = wbcir_idle_rx;
 	data->dev->s_tx_mask = wbcir_txmask;
 	data->dev->s_tx_carrier = wbcir_txcarrier;
+	data->dev->tx_ir_drains = 1;
 	data->dev->tx_ir = wbcir_tx;
 	data->dev->priv = data;
 	data->dev->dev.parent = &device->dev;
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index b0c494a..fc2318c 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -64,6 +64,7 @@ enum rc_driver_type {
  * @last_keycode: keycode of last keypress
  * @last_scancode: scancode of last keypress
  * @last_toggle: toggle value of last command
+ * @tx_ir_drains: tx_ir returns after IR has been sent
  * @timeout: optional time after which device stops sending data
  * @min_timeout: minimum timeout supported by device
  * @max_timeout: maximum timeout supported by device
@@ -108,6 +109,7 @@ struct rc_dev {
 	u32				last_keycode;
 	u32				last_scancode;
 	u8				last_toggle;
+	unsigned			tx_ir_drains:1;
 	u32				timeout;
 	u32				min_timeout;
 	u32				max_timeout;
-- 
1.7.11.4

Re: [PATCH] [media] rc: do not sleep when the driver blocks on IR completion

[David Härdeman]

On Thu, Aug 23, 2012 at 10:18:35PM +0100, Sean Young wrote:
>Some drivers wait for the IR device to complete sending before
>returning, so sleeping should not be done.

I'm not quite sure what the purpose is. Even if a driver waits for TX to
finish, the lirc imposed sleep isn't harmful in any way.

As far as I can tell, the iguanair driver waits for the usb packet to be
submitted, not for IR TX to finish.

As for winbond-cir, it would be simple enough to change it so that it
doesn't wait for TX to finish (which seems to be a better solution).

Having the TX methods as asynchronous as possible is probably a better
way to go...as I expect a future TX API to be asynchronous.

>
>Signed-off-by: Sean Young <sean@mess.org>
>---
> drivers/media/rc/iguanair.c      | 1 +
> drivers/media/rc/ir-lirc-codec.c | 5 +++++
> drivers/media/rc/winbond-cir.c   | 1 +
> include/media/rc-core.h          | 2 ++
> 4 files changed, 9 insertions(+)
>
>diff --git a/drivers/media/rc/iguanair.c b/drivers/media/rc/iguanair.c
>index 66ba237..7f1941d 100644
>--- a/drivers/media/rc/iguanair.c
>+++ b/drivers/media/rc/iguanair.c
>@@ -519,6 +519,7 @@ static int __devinit iguanair_probe(struct usb_interface *intf,
> 	rc->s_tx_mask = iguanair_set_tx_mask;
> 	rc->s_tx_carrier = iguanair_set_tx_carrier;
> 	rc->tx_ir = iguanair_tx;
>+	rc->tx_ir_drains = 1;
> 	rc->driver_name = DRIVER_NAME;
> 	rc->map_name = RC_MAP_RC6_MCE;
> 	rc->timeout = MS_TO_NS(100);
>diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
>index 569124b..dd21917 100644
>--- a/drivers/media/rc/ir-lirc-codec.c
>+++ b/drivers/media/rc/ir-lirc-codec.c
>@@ -144,6 +144,11 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
> 	if (ret < 0)
> 		goto out;
> 
>+	if (dev->tx_ir_drains) {
>+		ret *= sizeof(unsigned int);
>+		goto out;
>+	}
>+
> 	for (i = 0; i < ret; i++)
> 		duration += txbuf[i];
> 
>diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
>index 54ee348..b1b6d34 100644
>--- a/drivers/media/rc/winbond-cir.c
>+++ b/drivers/media/rc/winbond-cir.c
>@@ -1029,6 +1029,7 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
> 	data->dev->s_idle = wbcir_idle_rx;
> 	data->dev->s_tx_mask = wbcir_txmask;
> 	data->dev->s_tx_carrier = wbcir_txcarrier;
>+	data->dev->tx_ir_drains = 1;
> 	data->dev->tx_ir = wbcir_tx;
> 	data->dev->priv = data;
> 	data->dev->dev.parent = &device->dev;
>diff --git a/include/media/rc-core.h b/include/media/rc-core.h
>index b0c494a..fc2318c 100644
>--- a/include/media/rc-core.h
>+++ b/include/media/rc-core.h
>@@ -64,6 +64,7 @@ enum rc_driver_type {
>  * @last_keycode: keycode of last keypress
>  * @last_scancode: scancode of last keypress
>  * @last_toggle: toggle value of last command
>+ * @tx_ir_drains: tx_ir returns after IR has been sent
>  * @timeout: optional time after which device stops sending data
>  * @min_timeout: minimum timeout supported by device
>  * @max_timeout: maximum timeout supported by device
>@@ -108,6 +109,7 @@ struct rc_dev {
> 	u32				last_keycode;
> 	u32				last_scancode;
> 	u8				last_toggle;
>+	unsigned			tx_ir_drains:1;
> 	u32				timeout;
> 	u32				min_timeout;
> 	u32				max_timeout;
>-- 
>1.7.11.4
>

-- 
David Härdeman

Re: [PATCH] [media] rc: do not sleep when the driver blocks on IR completion

[Sean Young]

On Sat, Aug 25, 2012 at 12:05:18AM +0200, David Härdeman wrote:
> On Thu, Aug 23, 2012 at 10:18:35PM +0100, Sean Young wrote:
> >Some drivers wait for the IR device to complete sending before
> >returning, so sleeping should not be done.
> 
> I'm not quite sure what the purpose is. Even if a driver waits for TX to
> finish, the lirc imposed sleep isn't harmful in any way.

Due to rounding errors, clock skew and different start times, the sleep 
might be waiting for a different amount of time than the hardware took 
to send it. The sleep is a bit of a kludge, let alone if the driver
can wait for the hardware to tell you when it's done.

Also, your change calculates the amount of us to sleep after transmission, 
so if the transmission buffer was modified by the driver, the calculated 
sleep might not make sense. Both winbond-cir and iguanair do this.

> As far as I can tell, the iguanair driver waits for the usb packet to be
> submitted, not for IR TX to finish.

The iguanair driver waits for the ack from the device (which is an urb
you receive from the device), which is sent once the IR has been 
transmitted. The firmware source code is available.

> As for winbond-cir, it would be simple enough to change it so that it
> doesn't wait for TX to finish (which seems to be a better solution).
>
> Having the TX methods as asynchronous as possible is probably a better
> way to go...as I expect a future TX API to be asynchronous.

I agree that a future TX API should be asynchronous and I like your
ideas around that; however that won't be ready for v3.7.


Sean

Re: [PATCH] [media] rc: do not sleep when the driver blocks on IR completion

[David Härdeman]

On Sat, Aug 25, 2012 at 12:26:25AM +0100, Sean Young wrote:
>On Sat, Aug 25, 2012 at 12:05:18AM +0200, David Härdeman wrote:
>> On Thu, Aug 23, 2012 at 10:18:35PM +0100, Sean Young wrote:
>> >Some drivers wait for the IR device to complete sending before
>> >returning, so sleeping should not be done.
>> 
>> I'm not quite sure what the purpose is. Even if a driver waits for TX to
>> finish, the lirc imposed sleep isn't harmful in any way.
>
>Due to rounding errors, clock skew and different start times, the sleep 
>might be waiting for a different amount of time than the hardware took 
>to send it. The sleep is a bit of a kludge, let alone if the driver
>can wait for the hardware to tell you when it's done.

I don't see the sleep as much of a problem right now. Whether the
hardware says its done or if we simulate the same thing in the lirc
layer, the entire concept is a bit of a kludge :)

>Also, your change calculates the amount of us to sleep after transmission, 
>so if the transmission buffer was modified by the driver, the calculated 
>sleep might not make sense. Both winbond-cir and iguanair do this.

Oh, right, I'd overlooked this. I have written patches for winbond-cir
(which makes it asynchronous and leaves the txbuffer alone) and iguanair
(to leave the txbuffer alone). I'll post them sometime today when I've
done some more tests.

>> As far as I can tell, the iguanair driver waits for the usb packet to be
>> submitted, not for IR TX to finish.
>
>The iguanair driver waits for the ack from the device (which is an urb
>you receive from the device), which is sent once the IR has been 
>transmitted. The firmware source code is available.

Ah, I see.


-- 
David Härdeman

Re: [PATCH] [media] rc: do not sleep when the driver blocks on IR completion

[Sean Young]

On Sat, Aug 25, 2012 at 11:25:26AM +0200, David Härdeman wrote:
> On Sat, Aug 25, 2012 at 12:26:25AM +0100, Sean Young wrote:
> >On Sat, Aug 25, 2012 at 12:05:18AM +0200, David Härdeman wrote:
> >> On Thu, Aug 23, 2012 at 10:18:35PM +0100, Sean Young wrote:
> >> >Some drivers wait for the IR device to complete sending before
> >> >returning, so sleeping should not be done.
> >> 
> >> I'm not quite sure what the purpose is. Even if a driver waits for TX to
> >> finish, the lirc imposed sleep isn't harmful in any way.
> >
> >Due to rounding errors, clock skew and different start times, the sleep 
> >might be waiting for a different amount of time than the hardware took 
> >to send it. The sleep is a bit of a kludge, let alone if the driver
> >can wait for the hardware to tell you when it's done.
> 
> I don't see the sleep as much of a problem right now. Whether the
> hardware says its done or if we simulate the same thing in the lirc
> layer, the entire concept is a bit of a kludge :)

It's not making it any better.

> >Also, your change calculates the amount of us to sleep after transmission, 
> >so if the transmission buffer was modified by the driver, the calculated 
> >sleep might not make sense. Both winbond-cir and iguanair do this.
> 
> Oh, right, I'd overlooked this. I have written patches for winbond-cir
> (which makes it asynchronous and leaves the txbuffer alone) and iguanair
> (to leave the txbuffer alone). I'll post them sometime today when I've
> done some more tests.

If this is the solution we're going for, then I've got a patch for iguanair
which leaves the txbuffer alone and is ready and tested. I'll send it out
in the next half hour.


Sean