Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices

[Andi Shyti]

Hi Sean,

> > > Andi, it would be good to know what the use-case for the original change 
> > > is.
> > 
> > the use case is the ir-spi itself which doesn't need the lirc to
> > perform any waiting on its behalf.
> 
> Here is the crux of the problem: in the ir-spi case no wait will actually 
> happen here, and certainly no "over-wait". The patch below will not change
> behaviour at all.
> 
> In the ir-spi case, "towait" will be 0 and no wait happens.
> 
> I think the code is already in good shape but somehow there is a 
> misunderstanding. Did I miss something?

We can just drop this patch, it's just something small that is
bothering me.

I will send a new patchset without this one.

Thanks,
Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices

[Sean Young]

Hi Andi,

On Tue, Nov 01, 2016 at 03:51:11PM +0900, Andi Shyti wrote:
> > Andi, it would be good to know what the use-case for the original change is.
> 
> the use case is the ir-spi itself which doesn't need the lirc to
> perform any waiting on its behalf.

Here is the crux of the problem: in the ir-spi case no wait will actually 
happen here, and certainly no "over-wait". The patch below will not change
behaviour at all.

In the ir-spi case, "towait" will be 0 and no wait happens.

I think the code is already in good shape but somehow there is a 
misunderstanding. Did I miss something?


Sean

> To me it just doesn't look right to simulate a fake transmission
> period and wait unnecessary time. Of course, the "over-wait" is not
> a big deal and at the end we can decide to drop it.
> 
> Otherwise, an alternative could be to add the boolean
> 'tx_no_wait' in the rc_dev structure. It could be set by the
> device driver during the initialization and the we can follow
> your approach.
> 
> Something like this:
> 
> diff --git a/drivers/media/rc/ir-lirc-codec.c 
> b/drivers/media/rc/ir-lirc-codec.c
> index c327730..4553d04 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -161,15 +161,19 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, 
> const char __user *buf,
>  
> ret *= sizeof(unsigned int);
>  
> -   /*
> -* The lircd gap calculation expects the write function to
> -* wait for the actual IR signal to be transmitted before
> -* returning.
> -*/
> -   towait = ktime_us_delta(ktime_add_us(start, duration), ktime_get());
> -   if (towait > 0) {
> -   set_current_state(TASK_INTERRUPTIBLE);
> -   schedule_timeout(usecs_to_jiffies(towait));
> +   if (!dev->tx_no_wait) {
> +   /*
> +* The lircd gap calculation expects the write function to
> +* wait for the actual IR signal to be transmitted before
> +* returning.
> +*/
> +   towait = ktime_us_delta(ktime_add_us(start, duration),
> +   ktime_get());
> +   if (towait > 0) {
> +   set_current_state(TASK_INTERRUPTIBLE);
> +   schedule_timeout(usecs_to_jiffies(towait));
> +   }
> +
> }
>  
>  out:
> diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
> index fcda1e4..e44abfa 100644
> --- a/drivers/media/rc/ir-spi.c
> +++ b/drivers/media/rc/ir-spi.c
> @@ -149,6 +149,7 @@ static int ir_spi_probe(struct spi_device *spi)
> if (!idata->rc)
> return -ENOMEM;
>  
> +   idata->rc->tx_no_wait  = true;
> idata->rc->tx_ir   = ir_spi_tx;
> idata->rc->s_tx_carrier= ir_spi_set_tx_carrier;
> idata->rc->s_tx_duty_cycle = ir_spi_set_duty_cycle;
> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
> index fe0c9c4..c3ced9b 100644
> --- a/include/media/rc-core.h
> +++ b/include/media/rc-core.h
> @@ -85,6 +85,9 @@ enum rc_filter_type {
>   * @input_dev: the input child device used to communicate events to userspace
>   * @driver_type: specifies if protocol decoding is done in hardware or 
> software
>   * @idle: used to keep track of RX state
> + * @tx_no_wait: decides whether to perform or not a sync write or not. The
> + *  device driver setting it to true must make sure to not break the ABI
> + *  which requires a sync transfer.
>   * @allowed_protocols: bitmask with the supported RC_BIT_* protocols
>   * @enabled_protocols: bitmask with the enabled RC_BIT_* protocols
>   * @allowed_wakeup_protocols: bitmask with the supported RC_BIT_* wakeup 
> protocols
> @@ -147,6 +150,7 @@ struct rc_dev {
> struct input_dev*input_dev;
> enum rc_driver_type driver_type;
> boolidle;
> +   booltx_no_wait;
> u64 allowed_protocols;
> u64 enabled_protocols;
> u64 allowed_wakeup_protocols;
> 
> Thanks,
> Andi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices

[Andi Shyti]

Hi Sean,

> Andi, it would be good to know what the use-case for the original change is.

the use case is the ir-spi itself which doesn't need the lirc to
perform any waiting on its behalf.

To me it just doesn't look right to simulate a fake transmission
period and wait unnecessary time. Of course, the "over-wait" is not
a big deal and at the end we can decide to drop it.

Otherwise, an alternative could be to add the boolean
'tx_no_wait' in the rc_dev structure. It could be set by the
device driver during the initialization and the we can follow
your approach.

Something like this:

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index c327730..4553d04 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -161,15 +161,19 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, 
const char __user *buf,
 
ret *= sizeof(unsigned int);
 
-   /*
-* The lircd gap calculation expects the write function to
-* wait for the actual IR signal to be transmitted before
-* returning.
-*/
-   towait = ktime_us_delta(ktime_add_us(start, duration), ktime_get());
-   if (towait > 0) {
-   set_current_state(TASK_INTERRUPTIBLE);
-   schedule_timeout(usecs_to_jiffies(towait));
+   if (!dev->tx_no_wait) {
+   /*
+* The lircd gap calculation expects the write function to
+* wait for the actual IR signal to be transmitted before
+* returning.
+*/
+   towait = ktime_us_delta(ktime_add_us(start, duration),
+   ktime_get());
+   if (towait > 0) {
+   set_current_state(TASK_INTERRUPTIBLE);
+   schedule_timeout(usecs_to_jiffies(towait));
+   }
+
}
 
 out:
diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
index fcda1e4..e44abfa 100644
--- a/drivers/media/rc/ir-spi.c
+++ b/drivers/media/rc/ir-spi.c
@@ -149,6 +149,7 @@ static int ir_spi_probe(struct spi_device *spi)
if (!idata->rc)
return -ENOMEM;
 
+   idata->rc->tx_no_wait  = true;
idata->rc->tx_ir   = ir_spi_tx;
idata->rc->s_tx_carrier= ir_spi_set_tx_carrier;
idata->rc->s_tx_duty_cycle = ir_spi_set_duty_cycle;
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index fe0c9c4..c3ced9b 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -85,6 +85,9 @@ enum rc_filter_type {
  * @input_dev: the input child device used to communicate events to userspace
  * @driver_type: specifies if protocol decoding is done in hardware or software
  * @idle: used to keep track of RX state
+ * @tx_no_wait: decides whether to perform or not a sync write or not. The
+ *  device driver setting it to true must make sure to not break the ABI
+ *  which requires a sync transfer.
  * @allowed_protocols: bitmask with the supported RC_BIT_* protocols
  * @enabled_protocols: bitmask with the enabled RC_BIT_* protocols
  * @allowed_wakeup_protocols: bitmask with the supported RC_BIT_* wakeup 
protocols
@@ -147,6 +150,7 @@ struct rc_dev {
struct input_dev*input_dev;
enum rc_driver_type driver_type;
boolidle;
+   booltx_no_wait;
u64 allowed_protocols;
u64 enabled_protocols;
u64 allowed_wakeup_protocols;

Thanks,
Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices

[Sean Young]

Hi David, Andi,

On Mon, Oct 31, 2016 at 02:31:52PM +, David Härdeman wrote:
> October 27, 2016 4:36 PM, "Sean Young"  wrote:
> > Since we have to be able to switch between waiting and not waiting,
> > we need some sort of ABI for this. I think this warrants a new ioctl;
> > I'm not sure how else it can be done. I'll be sending out a patch
> > shortly.
> 
> Hi Sean,
> 
> have you considered using a module param for the LIRC bridge module instead? 
> As far as I understand it, this is an issue which is entirely internal to 
> LIRC, and it's also not something which really needs to be changed on a 
> per-device level (either you have a modern LIRC daemon or you don't, and all 
> drivers should behave the same, no?).

I still don't see how any of this would change anything for the ir-spi case:
since it uses sync spi transfer, it's going to block anyway.

> Another advantage is that the parameter would then go away if and when the 
> lirc bridge ever goes away (yes, I can still dream, can't I?).

The suggested ioctl is unique to lirc too and would disappear if the
lirc ABI goes away. With a module parameter you would change the lirc ABI
into an incompatible one. Not sure that is what module parameters should
be used for.

Andi, it would be good to know what the use-case for the original change is.


Thanks
Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices

[David Härdeman]

October 27, 2016 4:36 PM, "Sean Young"  wrote:
> Since we have to be able to switch between waiting and not waiting,
> we need some sort of ABI for this. I think this warrants a new ioctl;
> I'm not sure how else it can be done. I'll be sending out a patch
> shortly.

Hi Sean,

have you considered using a module param for the LIRC bridge module instead? As 
far as I understand it, this is an issue which is entirely internal to LIRC, 
and it's also not something which really needs to be changed on a per-device 
level (either you have a modern LIRC daemon or you don't, and all drivers 
should behave the same, no?).

Another advantage is that the parameter would then go away if and when the lirc 
bridge ever goes away (yes, I can still dream, can't I?).

Regards,
David
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices

[Sean Young]

On Thu, Oct 27, 2016 at 04:44:01PM +0900, Andi Shyti wrote:
> Hi Sean,
> 
> it's been a while :)
> 
> I was going through your review fixing what needs to be fixed,
> but...
> 
> > > @@ -153,7 +153,7 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, 
> > > const char __user *buf,
> > >   }
> > >  
> > >   ret = dev->tx_ir(dev, txbuf, count);
> > > - if (ret < 0)
> > > + if (ret < 0 || dev->driver_type == RC_DRIVER_IR_RAW_TX)
> > 
> > Just because a driver only does transmit doesn't mean its transmit ABI
> > should change.
> > 
> > Now this bit of code is pretty horrible. It ensures that the call to write()
> > takes at least as long as the length of the transmit IR by sleeping. That's
> > not much of a guarantee that the IR has been sent.
> > 
> > Note that in the case of ir-spi, since your spi transfer is sync no sleep
> > should be introduced here.
> > 
> > The gap calculation in lirc checks that if the call to write() took _longer_
> > than expected wait before sending the next IR code (when either multiple
> > IR codes or repeats are specified). Introducing the sleep in the kernel
> > here does not help at all, lirc already ensures that it waits as long as
> > the IR is long (see schedule_repeat_timer in lirc).
> > 
> > This change was introduced in 3.10, commit f8e00d5. 
> 
> ... I'm not sure what can be done here. I get your point and I
> understand that this indeed is a kind of fake sync point and by
> doing this I 

My original plan was to send a patch which just removes the silly wait,
but on further investigating debian stable and testing still carry a
lirc version that depend on it, so that's not going to fly.

> How about creating two different functions:
> 
> - ir_lirc_transmit_ir where we actually do what the function
>   already does
> - ir_lirc_transmit_no_sync where the function we don't wait
>   because the the sync is done on a different level (for example
>   in the SPI case).
> 
> SPI does approximately the same thing.

Since we have to be able to switch between waiting and not waiting, 
we need some sort of ABI for this. I think this warrants a new ioctl;
I'm not sure how else it can be done. I'll be sending out a patch
shortly.


Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices

[Andi Shyti]

Hi Sean,

it's been a while :)

I was going through your review fixing what needs to be fixed,
but...

> > @@ -153,7 +153,7 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, 
> > const char __user *buf,
> > }
> >  
> > ret = dev->tx_ir(dev, txbuf, count);
> > -   if (ret < 0)
> > +   if (ret < 0 || dev->driver_type == RC_DRIVER_IR_RAW_TX)
> 
> Just because a driver only does transmit doesn't mean its transmit ABI
> should change.
> 
> Now this bit of code is pretty horrible. It ensures that the call to write()
> takes at least as long as the length of the transmit IR by sleeping. That's
> not much of a guarantee that the IR has been sent.
> 
> Note that in the case of ir-spi, since your spi transfer is sync no sleep
> should be introduced here.
> 
> The gap calculation in lirc checks that if the call to write() took _longer_
> than expected wait before sending the next IR code (when either multiple
> IR codes or repeats are specified). Introducing the sleep in the kernel
> here does not help at all, lirc already ensures that it waits as long as
> the IR is long (see schedule_repeat_timer in lirc).
> 
> This change was introduced in 3.10, commit f8e00d5. 

... I'm not sure what can be done here. I get your point and I
understand that this indeed is a kind of fake sync point and by
doing this I 

How about creating two different functions:

- ir_lirc_transmit_ir where we actually do what the function
  already does
- ir_lirc_transmit_no_sync where the function we don't wait
  because the the sync is done on a different level (for example
  in the SPI case).

SPI does approximately the same thing.

Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices

[Sean Young]

On Fri, Sep 02, 2016 at 02:16:27AM +0900, Andi Shyti wrote:
> Transmitters do not need to wait until the data has been sent
> (and of course received). Return before waiting.
> 
> Signed-off-by: Andi Shyti 
> ---
>  drivers/media/rc/ir-lirc-codec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/ir-lirc-codec.c 
> b/drivers/media/rc/ir-lirc-codec.c
> index c327730..d8953fb 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -153,7 +153,7 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, 
> const char __user *buf,
>   }
>  
>   ret = dev->tx_ir(dev, txbuf, count);
> - if (ret < 0)
> + if (ret < 0 || dev->driver_type == RC_DRIVER_IR_RAW_TX)

Just because a driver only does transmit doesn't mean its transmit ABI
should change.

Now this bit of code is pretty horrible. It ensures that the call to write()
takes at least as long as the length of the transmit IR by sleeping. That's
not much of a guarantee that the IR has been sent.

Note that in the case of ir-spi, since your spi transfer is sync no sleep
should be introduced here.

The gap calculation in lirc checks that if the call to write() took _longer_
than expected wait before sending the next IR code (when either multiple
IR codes or repeats are specified). Introducing the sleep in the kernel
here does not help at all, lirc already ensures that it waits as long as
the IR is long (see schedule_repeat_timer in lirc).

This change was introduced in 3.10, commit f8e00d5. 


Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices

[Andi Shyti]

Transmitters do not need to wait until the data has been sent
(and of course received). Return before waiting.

Signed-off-by: Andi Shyti 
---
 drivers/media/rc/ir-lirc-codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index c327730..d8953fb 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -153,7 +153,7 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const 
char __user *buf,
}
 
ret = dev->tx_ir(dev, txbuf, count);
-   if (ret < 0)
+   if (ret < 0 || dev->driver_type == RC_DRIVER_IR_RAW_TX)
goto out;
 
for (duration = i = 0; i < ret; i++)
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html