[PATCH v2 0/2] media: rc: gpio-ir-recv: add timeout property

[PATCH v2 0/2] media: rc: gpio-ir-recv: add timeout property

[Michael Klein]

The default recorder timeout of 125ms is too high for some BPF protocol 
decoders when a remote sends repeat codes at high rates. This makes the 
timeout configurable via the devicetree.

Changes in v2:
  fix checkpatch.pl warnings

Michael Klein (2):
  media: rc: gpio-ir-recv: add recorder timeout property
  media: bindings: media: gpio-ir-receiver: add linux,timeout-us
    property

 .../devicetree/bindings/media/gpio-ir-receiver.txt          | 3 +++
 Documentation/devicetree/bindings/media/rc.yaml             | 6 ++++++
 drivers/media/rc/gpio-ir-recv.c                             | 3 ++-
 3 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.28.0

[PATCH v2 1/2] media: rc: gpio-ir-recv: add recorder timeout property

[Michael Klein]

This optional property allows to set the recorder timeout via the
devicetree:

- linux,timeout-us: set the length of a space at which
  the recorder goes idle, specified in microseconds.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 drivers/media/rc/gpio-ir-recv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index 22e524b69806..eb3e56c27c29 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -99,7 +99,8 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 	rcdev->dev.parent = dev;
 	rcdev->driver_name = KBUILD_MODNAME;
 	rcdev->min_timeout = 1;
-	rcdev->timeout = IR_DEFAULT_TIMEOUT;
+	if (of_property_read_u32(np, "linux,timeout-us", &rcdev->timeout))
+		rcdev->timeout = IR_DEFAULT_TIMEOUT;
 	rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
 	rcdev->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
 	rcdev->map_name = of_get_property(np, "linux,rc-map-name", NULL);
-- 
2.28.0

[PATCH v2 2/2] media: bindings: media: gpio-ir-receiver: add linux,timeout-us property

[Michael Klein]

Add linux,timeout-us for gpio ir receiver.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 .../devicetree/bindings/media/gpio-ir-receiver.txt          | 3 +++
 Documentation/devicetree/bindings/media/rc.yaml             | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
index 108bf435b933..7aef3fe78322 100644
--- a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
+++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
@@ -9,6 +9,8 @@ Optional properties:
 	  directory.
 	- linux,autosuspend-period: autosuspend delay time,
 	  the unit is milisecond.
+	- linux,timeout-us: set the length of a space at which
+	  the recorder goes idle, specified in microseconds.
 
 Example node:
 
@@ -17,4 +19,5 @@ Example node:
 		gpios = <&gpio0 19 1>;
 		linux,rc-map-name = "rc-rc6-mce";
 		linux,autosuspend-period = <125>;
+		linux,timeout-us = <125000>;
 	};
diff --git a/Documentation/devicetree/bindings/media/rc.yaml b/Documentation/devicetree/bindings/media/rc.yaml
index 8ad2cba5f61f..1f3e208a50a1 100644
--- a/Documentation/devicetree/bindings/media/rc.yaml
+++ b/Documentation/devicetree/bindings/media/rc.yaml
@@ -151,4 +151,10 @@ properties:
       - rc-xbox-dvd
       - rc-zx-irdec
 
+  linux,timeout-us:
+    description:
+      Set the length of a space at which the recorder goes idle, specified in
+      microseconds.
+    $ref: '/schemas/types.yaml#/definitions/uint32'
+
 additionalProperties: true
-- 
2.28.0

[PATCH RESEND v2 0/2] media: rc: gpio-ir-recv: add timeout property

[Michael Klein]

The default recorder timeout of 125ms is too high for some BPF protocol 
decoders when a remote sends repeat codes at high rates. This makes the 
timeout configurable via the devicetree.

Changes in v2:
  fix checkpatch.pl warnings

Michael Klein (2):
  media: rc: gpio-ir-recv: add recorder timeout property
  media: bindings: media: gpio-ir-receiver: add linux,timeout-us
    property

 .../devicetree/bindings/media/gpio-ir-receiver.txt          | 3 +++
 Documentation/devicetree/bindings/media/rc.yaml             | 6 ++++++
 drivers/media/rc/gpio-ir-recv.c                             | 3 ++-
 3 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.28.0

[PATCH RESEND v2 1/2] media: rc: gpio-ir-recv: add recorder timeout property

[Michael Klein]

This optional property allows to set the recorder timeout via the
devicetree:

- linux,timeout-us: set the length of a space at which
  the recorder goes idle, specified in microseconds.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 drivers/media/rc/gpio-ir-recv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index 22e524b69806..eb3e56c27c29 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -99,7 +99,8 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
 	rcdev->dev.parent = dev;
 	rcdev->driver_name = KBUILD_MODNAME;
 	rcdev->min_timeout = 1;
-	rcdev->timeout = IR_DEFAULT_TIMEOUT;
+	if (of_property_read_u32(np, "linux,timeout-us", &rcdev->timeout))
+		rcdev->timeout = IR_DEFAULT_TIMEOUT;
 	rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
 	rcdev->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
 	rcdev->map_name = of_get_property(np, "linux,rc-map-name", NULL);
-- 
2.28.0

[PATCH RESEND v2 2/2] media: bindings: media: gpio-ir-receiver: add linux,timeout-us property

[Michael Klein]

Add linux,timeout-us for gpio ir receiver.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 .../devicetree/bindings/media/gpio-ir-receiver.txt          | 3 +++
 Documentation/devicetree/bindings/media/rc.yaml             | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
index 108bf435b933..7aef3fe78322 100644
--- a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
+++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt
@@ -9,6 +9,8 @@ Optional properties:
 	  directory.
 	- linux,autosuspend-period: autosuspend delay time,
 	  the unit is milisecond.
+	- linux,timeout-us: set the length of a space at which
+	  the recorder goes idle, specified in microseconds.
 
 Example node:
 
@@ -17,4 +19,5 @@ Example node:
 		gpios = <&gpio0 19 1>;
 		linux,rc-map-name = "rc-rc6-mce";
 		linux,autosuspend-period = <125>;
+		linux,timeout-us = <125000>;
 	};
diff --git a/Documentation/devicetree/bindings/media/rc.yaml b/Documentation/devicetree/bindings/media/rc.yaml
index 8ad2cba5f61f..1f3e208a50a1 100644
--- a/Documentation/devicetree/bindings/media/rc.yaml
+++ b/Documentation/devicetree/bindings/media/rc.yaml
@@ -151,4 +151,10 @@ properties:
       - rc-xbox-dvd
       - rc-zx-irdec
 
+  linux,timeout-us:
+    description:
+      Set the length of a space at which the recorder goes idle, specified in
+      microseconds.
+    $ref: '/schemas/types.yaml#/definitions/uint32'
+
 additionalProperties: true
-- 
2.28.0

Re: [PATCH RESEND v2 0/2] media: rc: gpio-ir-recv: add timeout property

[Sean Young]

On Mon, Nov 09, 2020 at 04:23:09PM +0100, Michael Klein wrote:
> The default recorder timeout of 125ms is too high for some BPF protocol 
> decoders when a remote sends repeat codes at high rates. This makes the 
> timeout configurable via the devicetree.

To be honest, 125ms is too much by any measurement. The longest space
in any protocol I'm aware of is 40ms in the sharp ir protocol. I think
changing IR_DEFAUL_TIMEOUT to something like 50ms would make sense.

Also, when an BPF protocol is loaded, user-space can set the timeout
with the LIRC_SET_REC_TIMEOUT ioctl which can depend on the protocol
(set to longest space + 10ms error margin). This would mean that the
bare minimum timeout can be set, which means decoding is as responsive
as can be.

I'm not sure that device tree is really the place for this.

Thanks,

Sean


> 
> Changes in v2:
>   fix checkpatch.pl warnings
> 
> Michael Klein (2):
>   media: rc: gpio-ir-recv: add recorder timeout property
>   media: bindings: media: gpio-ir-receiver: add linux,timeout-us
>     property
> 
>  .../devicetree/bindings/media/gpio-ir-receiver.txt          | 3 +++
>  Documentation/devicetree/bindings/media/rc.yaml             | 6 ++++++
>  drivers/media/rc/gpio-ir-recv.c                             | 3 ++-
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> -- 
> 2.28.0

Re: [PATCH RESEND v2 0/2] media: rc: gpio-ir-recv: add timeout property

[Michael Klein]

On Tue, Nov 10, 2020 at 10:17:27AM +0000, Sean Young wrote:
>On Mon, Nov 09, 2020 at 04:23:09PM +0100, Michael Klein wrote:
>> The default recorder timeout of 125ms is too high for some BPF protocol
>> decoders when a remote sends repeat codes at high rates. This makes the
>> timeout configurable via the devicetree.
>
>To be honest, 125ms is too much by any measurement. The longest space
>in any protocol I'm aware of is 40ms in the sharp ir protocol. I think
>changing IR_DEFAUL_TIMEOUT to something like 50ms would make sense.

Seconded. I'm happy to prepare a patch if changing the default value is 
acceptable.

>Also, when an BPF protocol is loaded, user-space can set the timeout
>with the LIRC_SET_REC_TIMEOUT ioctl which can depend on the protocol
>(set to longest space + 10ms error margin).

Right, although this is a bit cumbersome with current user-space tools. 
The BPF is loaded with ir-keytable, while the recorder timeout needs to 
be set with it-ctl. In the Debian world, those tools are even in 
different packages.

>This would mean that the
>bare minimum timeout can be set, which means decoding is as responsive
>as can be.
>
>I'm not sure that device tree is really the place for this.

Not arguing about this, but IMHO no less than for rc-map-name. So this 
seems to be at least consistent.

Thanks,

Michael

Re: [PATCH RESEND v2 0/2] media: rc: gpio-ir-recv: add timeout property

[Sean Young]

On Tue, Nov 10, 2020 at 01:48:05PM +0100, Michael Klein wrote:
> On Tue, Nov 10, 2020 at 10:17:27AM +0000, Sean Young wrote:
> > On Mon, Nov 09, 2020 at 04:23:09PM +0100, Michael Klein wrote:
> > > The default recorder timeout of 125ms is too high for some BPF protocol
> > > decoders when a remote sends repeat codes at high rates. This makes the
> > > timeout configurable via the devicetree.
> > 
> > To be honest, 125ms is too much by any measurement. The longest space
> > in any protocol I'm aware of is 40ms in the sharp ir protocol. I think
> > changing IR_DEFAUL_TIMEOUT to something like 50ms would make sense.
> 
> Seconded. I'm happy to prepare a patch if changing the default value is
> acceptable.

Actually I don't understand why the high timeout is an issue. It means that
between ir messages you don't get a LIRC_TIMEOUT, just a LIRC_SPACE. Why is
this a problem?

I'm not opposed to such a patch, but we would need to know if it really
solves the problem you are having and it would need to sit in linux-next
for some time.

> > Also, when an BPF protocol is loaded, user-space can set the timeout
> > with the LIRC_SET_REC_TIMEOUT ioctl which can depend on the protocol
> > (set to longest space + 10ms error margin).
> 
> Right, although this is a bit cumbersome with current user-space tools. The
> BPF is loaded with ir-keytable, while the recorder timeout needs to be set
> with it-ctl. In the Debian world, those tools are even in different
> packages.

ir-keytable can use the LIRC_SET_REC_TIMEOUT ioctl to adjust the timeout.
It has opened the lirc device already to load the bpf program. ir-keytable
would need to calculate the minimum timeout needed for all enabled protocols
(bpf and non-bpf). Then it can simply do the ioctl.

> > This would mean that the
> > bare minimum timeout can be set, which means decoding is as responsive
> > as can be.
> > 
> > I'm not sure that device tree is really the place for this.
> 
> Not arguing about this, but IMHO no less than for rc-map-name. So this seems
> to be at least consistent.

Well, I guess it can be argued. However, it can also be argued that
it is not the best solution for this problem.


Sean

Re: [PATCH RESEND v2 0/2] media: rc: gpio-ir-recv: add timeout property

[Michael Klein]

On Tue, Nov 10, 2020 at 01:19:18PM +0000, Sean Young wrote:
>On Tue, Nov 10, 2020 at 01:48:05PM +0100, Michael Klein wrote:
>> On Tue, Nov 10, 2020 at 10:17:27AM +0000, Sean Young wrote:
>> > On Mon, Nov 09, 2020 at 04:23:09PM +0100, Michael Klein wrote:
>> > > The default recorder timeout of 125ms is too high for some BPF protocol
>> > > decoders when a remote sends repeat codes at high rates. This makes the
>> > > timeout configurable via the devicetree.
>> >
>> > To be honest, 125ms is too much by any measurement. The longest space
>> > in any protocol I'm aware of is 40ms in the sharp ir protocol. I think
>> > changing IR_DEFAUL_TIMEOUT to something like 50ms would make sense.
>>
>> Seconded. I'm happy to prepare a patch if changing the default value is
>> acceptable.
>
>Actually I don't understand why the high timeout is an issue. It means that
>between ir messages you don't get a LIRC_TIMEOUT, just a LIRC_SPACE. Why is
>this a problem?

Never mind; this turned out do be a problem of the BPF protocol decoder, 
which relied on LIRC_TIMEOUT to terminate each IR message. After 
overhaul it is quite a bit simpler now and works fine with the long 
timeout.

Thank you for your insights.

Michael