| Message ID | 20180308164327.ihhmvm6ntzvnsjy7@gofer.mess.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Sean, On Thu, Mar 08, 2018 at 04:43:27PM +0000, Sean Young wrote: > On Tue, Mar 06, 2018 at 06:41:22PM +0100, Matthias Reichl wrote: > > Meson doesn't seem to be able to generate timeout events > > in hardware. So install a software timer to generate the > > timeout events required by the decoders to prevent > > "ghost keypresses". > > > > Signed-off-by: Matthias Reichl <hias@horus.com> > > --- > > drivers/media/rc/meson-ir.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c > > index f2204eb77e2a..f34c5836412b 100644 > > --- a/drivers/media/rc/meson-ir.c > > +++ b/drivers/media/rc/meson-ir.c > > @@ -69,6 +69,7 @@ struct meson_ir { > > void __iomem *reg; > > struct rc_dev *rc; > > spinlock_t lock; > > + struct timer_list timeout_timer; > > }; > > > > static void meson_ir_set_mask(struct meson_ir *ir, unsigned int reg, > > @@ -98,6 +99,10 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id) > > rawir.pulse = !!(status & STATUS_IR_DEC_IN); > > > > ir_raw_event_store(ir->rc, &rawir); > > + > > + mod_timer(&ir->timeout_timer, > > + jiffies + nsecs_to_jiffies(ir->rc->timeout)); > > + > > ir_raw_event_handle(ir->rc); > > > > spin_unlock(&ir->lock); > > @@ -105,6 +110,17 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id) > > return IRQ_HANDLED; > > } > > > > +static void meson_ir_timeout_timer(struct timer_list *t) > > +{ > > + struct meson_ir *ir = from_timer(ir, t, timeout_timer); > > + DEFINE_IR_RAW_EVENT(rawir); > > + > > + rawir.timeout = true; > > + rawir.duration = ir->rc->timeout; > > + ir_raw_event_store(ir->rc, &rawir); > > + ir_raw_event_handle(ir->rc); > > +} > > Now there can be concurrent access to the raw IR kfifo from the interrupt > handler and the timer. As there is a race condition between the timeout > timer and new IR arriving from the interrupt handler, the timeout could > end being generated after new IR and corrupting a message. There is very > similar functionality in rc-ir-raw.c (with a spinlock). Ah, thanks for the hint! Now I also noticed your commit a few weeks ago - must have missed that before. > > + > > static int meson_ir_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > @@ -145,7 +161,9 @@ static int meson_ir_probe(struct platform_device *pdev) > > ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY; > > ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; > > ir->rc->rx_resolution = US_TO_NS(MESON_TRATE); > > + ir->rc->min_timeout = 1; > > ir->rc->timeout = MS_TO_NS(200); > > + ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT; > > Any idea why the default timeout is to 200ms? It seems very high. Indeed it is very high, but I have no idea where that might be coming from - so I didn't touch it. I've been testing rc-5 and NEC remotes with 20-50ms timeouts on meson-ir/upstream kernel and a couple of LibreELEC users are using 30-50ms timeouts without issues on Amlogic devices as well (on 3.14 vendor kernel with meson-ir timeout patch): https://forum.libreelec.tv/thread/11643-le9-0-remote-configs-ir-keytable-amlogic-devices/?postID=83124#post83124 Out of curiosity: where does the 125ms IR_DEFAULT_TIMEOUT value come from? For raw IR signals processed by the decoders this seems rather high to me as well. On my RPi3 with gpio-ir-recv I'm using 30ms timeout (with an rc-5 remote) without issues. > > ir->rc->driver_name = DRIVER_NAME; > > > > spin_lock_init(&ir->lock); > > @@ -157,6 +175,8 @@ static int meson_ir_probe(struct platform_device *pdev) > > return ret; > > } > > > > + timer_setup(&ir->timeout_timer, meson_ir_timeout_timer, 0); > > + > > ret = devm_request_irq(dev, irq, meson_ir_irq, 0, NULL, ir); > > if (ret) { > > dev_err(dev, "failed to request irq\n"); > > @@ -198,6 +218,8 @@ static int meson_ir_remove(struct platform_device *pdev) > > meson_ir_set_mask(ir, IR_DEC_REG1, REG1_ENABLE, 0); > > spin_unlock_irqrestore(&ir->lock, flags); > > > > + del_timer_sync(&ir->timeout_timer); > > + > > return 0; > > } > > > > -- > > 2.11.0 > > Would you mind trying this patch? Tested-by: Matthias Reichl <hias@horus.com> Thanks a lot, this patch works fine! And having a common function in rc-core looks like a very good idea to me as well. Only thing I'd like to have added is min/max timeout values set in meson-ir so it's configurable via ir-ctl. A separate patch for that would make sense, though, I guess. so long & thanks, Hias > > Thanks > > Sean > --- > >>From f98f4fc05d743ac48a95694996985b2c1f0c4a4b Mon Sep 17 00:00:00 2001 > From: Sean Young <sean@mess.org> > Date: Thu, 8 Mar 2018 14:42:44 +0000 > Subject: [PATCH] media: rc: meson-ir: add timeout on idle > > Meson doesn't seem to be able to generate timeout events in hardware. So > install a software timer to generate the timeout events required by the > decoders to prevent "ghost keypresses". > > Signed-off-by: Sean Young <sean@mess.org> > --- > drivers/media/rc/meson-ir.c | 3 +-- > drivers/media/rc/rc-ir-raw.c | 30 +++++++++++++++++++++++++++--- > include/media/rc-core.h | 4 +++- > 3 files changed, 31 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c > index f2204eb77e2a..64b0aa4f4db7 100644 > --- a/drivers/media/rc/meson-ir.c > +++ b/drivers/media/rc/meson-ir.c > @@ -97,8 +97,7 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id) > status = readl_relaxed(ir->reg + IR_DEC_STATUS); > rawir.pulse = !!(status & STATUS_IR_DEC_IN); > > - ir_raw_event_store(ir->rc, &rawir); > - ir_raw_event_handle(ir->rc); > + ir_raw_event_store_with_timeout(ir->rc, &rawir); > > spin_unlock(&ir->lock); > > diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c > index 984bb82851f9..374f83105a23 100644 > --- a/drivers/media/rc/rc-ir-raw.c > +++ b/drivers/media/rc/rc-ir-raw.c > @@ -92,7 +92,6 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse) > { > ktime_t now; > DEFINE_IR_RAW_EVENT(ev); > - int rc = 0; > > if (!dev->raw) > return -EINVAL; > @@ -101,8 +100,33 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse) > ev.duration = ktime_to_ns(ktime_sub(now, dev->raw->last_event)); > ev.pulse = !pulse; > > + return ir_raw_event_store_with_timeout(dev, &ev); > +} > +EXPORT_SYMBOL_GPL(ir_raw_event_store_edge); > + > +/* > + * ir_raw_event_store_with_timeout() - pass a pulse/space duration to the raw > + * ir decoders, schedule decoding and > + * timeout > + * @dev: the struct rc_dev device descriptor > + * @ev: the struct ir_raw_event descriptor of the pulse/space > + * > + * This routine (which may be called from an interrupt context) stores a > + * pulse/space duration for the raw ir decoding state machines, schedules > + * decoding and generates a timeout. > + */ > +int ir_raw_event_store_with_timeout(struct rc_dev *dev, struct ir_raw_event *ev) > +{ > + ktime_t now; > + int rc = 0; > + > + if (!dev->raw) > + return -EINVAL; > + > + now = ktime_get(); > + > spin_lock(&dev->raw->edge_spinlock); > - rc = ir_raw_event_store(dev, &ev); > + rc = ir_raw_event_store(dev, ev); > > dev->raw->last_event = now; > > @@ -117,7 +141,7 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse) > > return rc; > } > -EXPORT_SYMBOL_GPL(ir_raw_event_store_edge); > +EXPORT_SYMBOL_GPL(ir_raw_event_store_with_timeout); > > /** > * ir_raw_event_store_with_filter() - pass next pulse/space to decoders with some processing > diff --git a/include/media/rc-core.h b/include/media/rc-core.h > index fc3a92668bab..6742fd86ff65 100644 > --- a/include/media/rc-core.h > +++ b/include/media/rc-core.h > @@ -334,7 +334,9 @@ void ir_raw_event_handle(struct rc_dev *dev); > int ir_raw_event_store(struct rc_dev *dev, struct ir_raw_event *ev); > int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse); > int ir_raw_event_store_with_filter(struct rc_dev *dev, > - struct ir_raw_event *ev); > + struct ir_raw_event *ev); > +int ir_raw_event_store_with_timeout(struct rc_dev *dev, > + struct ir_raw_event *ev); > void ir_raw_event_set_idle(struct rc_dev *dev, bool idle); > int ir_raw_encode_scancode(enum rc_proto protocol, u32 scancode, > struct ir_raw_event *events, unsigned int max); > -- > 2.14.3 > >
Hi Matthias, On Fri, Mar 09, 2018 at 04:54:51PM +0100, Matthias Reichl wrote: > On Thu, Mar 08, 2018 at 04:43:27PM +0000, Sean Young wrote: > > On Tue, Mar 06, 2018 at 06:41:22PM +0100, Matthias Reichl wrote: > > > Meson doesn't seem to be able to generate timeout events > > > in hardware. So install a software timer to generate the > > > timeout events required by the decoders to prevent > > > "ghost keypresses". > > > > > > Signed-off-by: Matthias Reichl <hias@horus.com> > > > --- > > > drivers/media/rc/meson-ir.c | 22 ++++++++++++++++++++++ > > > 1 file changed, 22 insertions(+) > > > > > > diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c > > > index f2204eb77e2a..f34c5836412b 100644 > > > --- a/drivers/media/rc/meson-ir.c > > > +++ b/drivers/media/rc/meson-ir.c > > > @@ -69,6 +69,7 @@ struct meson_ir { > > > void __iomem *reg; > > > struct rc_dev *rc; > > > spinlock_t lock; > > > + struct timer_list timeout_timer; > > > }; > > > > > > static void meson_ir_set_mask(struct meson_ir *ir, unsigned int reg, > > > @@ -98,6 +99,10 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id) > > > rawir.pulse = !!(status & STATUS_IR_DEC_IN); > > > > > > ir_raw_event_store(ir->rc, &rawir); > > > + > > > + mod_timer(&ir->timeout_timer, > > > + jiffies + nsecs_to_jiffies(ir->rc->timeout)); > > > + > > > ir_raw_event_handle(ir->rc); > > > > > > spin_unlock(&ir->lock); > > > @@ -105,6 +110,17 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id) > > > return IRQ_HANDLED; > > > } > > > > > > +static void meson_ir_timeout_timer(struct timer_list *t) > > > +{ > > > + struct meson_ir *ir = from_timer(ir, t, timeout_timer); > > > + DEFINE_IR_RAW_EVENT(rawir); > > > + > > > + rawir.timeout = true; > > > + rawir.duration = ir->rc->timeout; > > > + ir_raw_event_store(ir->rc, &rawir); > > > + ir_raw_event_handle(ir->rc); > > > +} > > > > Now there can be concurrent access to the raw IR kfifo from the interrupt > > handler and the timer. As there is a race condition between the timeout > > timer and new IR arriving from the interrupt handler, the timeout could > > end being generated after new IR and corrupting a message. There is very > > similar functionality in rc-ir-raw.c (with a spinlock). > > Ah, thanks for the hint! Now I also noticed your commit a few > weeks ago - must have missed that before. > > > > + > > > static int meson_ir_probe(struct platform_device *pdev) > > > { > > > struct device *dev = &pdev->dev; > > > @@ -145,7 +161,9 @@ static int meson_ir_probe(struct platform_device *pdev) > > > ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY; > > > ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; > > > ir->rc->rx_resolution = US_TO_NS(MESON_TRATE); > > > + ir->rc->min_timeout = 1; > > > ir->rc->timeout = MS_TO_NS(200); > > > + ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT; > > > > Any idea why the default timeout is to 200ms? It seems very high. > > Indeed it is very high, but I have no idea where that might be > coming from - so I didn't touch it. > > I've been testing rc-5 and NEC remotes with 20-50ms timeouts > on meson-ir/upstream kernel and a couple of LibreELEC users are > using 30-50ms timeouts without issues on Amlogic devices as well > (on 3.14 vendor kernel with meson-ir timeout patch): > > https://forum.libreelec.tv/thread/11643-le9-0-remote-configs-ir-keytable-amlogic-devices/?postID=83124#post83124 > > Out of curiosity: where does the 125ms IR_DEFAULT_TIMEOUT value > come from? For raw IR signals processed by the decoders this seems > rather high to me as well. On my RPi3 with gpio-ir-recv I'm > using 30ms timeout (with an rc-5 remote) without issues. So if the timeout is below N then you will never get a space of N or larger; the largest space I know of in an IR message is 40ms in the sanyo protocol: https://www.sbprojects.net/knowledge/ir/sharp.php So if timeout is set to less than 40ms, we would have trouble decoding the sharp protocol. The space between nec repeats is a little less than 100ms. I'm trying to remember what would could go wrong if the space between them would be timeouts instead. Mauro do you remember? I can imagine some IR hardware (e.g. winbond) queuing up IR after generating a timeout, thus delaying delivering IR to the kernel and we ending up generating a key up. The problem with a higher timeout is that the trailing space (=timeout) is sometimes needed for decoding, and decoding of the last message is delayed until the timeout is received. That means the keyup message is delayed until that time, making keys a bit "sticky" and more likely to generate repeats. I'm pretty sure that is needed for rc-5 and nec. > > > ir->rc->driver_name = DRIVER_NAME; > > > > > > spin_lock_init(&ir->lock); > > > @@ -157,6 +175,8 @@ static int meson_ir_probe(struct platform_device *pdev) > > > return ret; > > > } > > > > > > + timer_setup(&ir->timeout_timer, meson_ir_timeout_timer, 0); > > > + > > > ret = devm_request_irq(dev, irq, meson_ir_irq, 0, NULL, ir); > > > if (ret) { > > > dev_err(dev, "failed to request irq\n"); > > > @@ -198,6 +218,8 @@ static int meson_ir_remove(struct platform_device *pdev) > > > meson_ir_set_mask(ir, IR_DEC_REG1, REG1_ENABLE, 0); > > > spin_unlock_irqrestore(&ir->lock, flags); > > > > > > + del_timer_sync(&ir->timeout_timer); > > > + > > > return 0; > > > } > > > > > > -- > > > 2.11.0 > > > > Would you mind trying this patch? > > Tested-by: Matthias Reichl <hias@horus.com> > > Thanks a lot, this patch works fine! And having a common function > in rc-core looks like a very good idea to me as well. > > Only thing I'd like to have added is min/max timeout values set > in meson-ir so it's configurable via ir-ctl. A separate patch for > that would make sense, though, I guess. Yes, that would be good. That should go into a separate commit. Thanks for testing! Sean
Hi Sean, On Sat, Mar 10, 2018 at 11:27:45AM +0000, Sean Young wrote: > Hi Matthias, > > On Fri, Mar 09, 2018 at 04:54:51PM +0100, Matthias Reichl wrote: > > On Thu, Mar 08, 2018 at 04:43:27PM +0000, Sean Young wrote: > > > On Tue, Mar 06, 2018 at 06:41:22PM +0100, Matthias Reichl wrote: > > > > + > > > > static int meson_ir_probe(struct platform_device *pdev) > > > > { > > > > struct device *dev = &pdev->dev; > > > > @@ -145,7 +161,9 @@ static int meson_ir_probe(struct platform_device *pdev) > > > > ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY; > > > > ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; > > > > ir->rc->rx_resolution = US_TO_NS(MESON_TRATE); > > > > + ir->rc->min_timeout = 1; > > > > ir->rc->timeout = MS_TO_NS(200); > > > > + ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT; > > > > > > Any idea why the default timeout is to 200ms? It seems very high. > > > > Indeed it is very high, but I have no idea where that might be > > coming from - so I didn't touch it. > > > > I've been testing rc-5 and NEC remotes with 20-50ms timeouts > > on meson-ir/upstream kernel and a couple of LibreELEC users are > > using 30-50ms timeouts without issues on Amlogic devices as well > > (on 3.14 vendor kernel with meson-ir timeout patch): > > > > https://forum.libreelec.tv/thread/11643-le9-0-remote-configs-ir-keytable-amlogic-devices/?postID=83124#post83124 > > > > Out of curiosity: where does the 125ms IR_DEFAULT_TIMEOUT value > > come from? For raw IR signals processed by the decoders this seems > > rather high to me as well. On my RPi3 with gpio-ir-recv I'm > > using 30ms timeout (with an rc-5 remote) without issues. > > So if the timeout is below N then you will never get a space of N or larger; > the largest space I know of in an IR message is 40ms in the sanyo protocol: > > https://www.sbprojects.net/knowledge/ir/sharp.php > > So if timeout is set to less than 40ms, we would have trouble decoding the > sharp protocol. > > The space between nec repeats is a little less than 100ms. I'm trying to > remember what would could go wrong if the space between them would be > timeouts instead. Mauro do you remember? I can imagine some IR hardware > (e.g. winbond) queuing up IR after generating a timeout, thus delaying > delivering IR to the kernel and we ending up generating a key up. > > The problem with a higher timeout is that the trailing space (=timeout) > is sometimes needed for decoding, and decoding of the last message is > delayed until the timeout is received. That means the keyup message is > delayed until that time, making keys a bit "sticky" and more likely to > generate repeats. I'm pretty sure that is needed for rc-5 and nec. Another issue with high timeouts is the response to very short button presses where the remote only transmits a single scancode. It then takes signal transmission time plus timeout, so roughly a quarter of a second on meson-ir and ite-cir with 200ms timeout, until the scancode is decoded and the keydown event is generated. On longer button presses this is less of an issue as we get the space signal when the first pulse of the repeated scancode is received. So the delay between button press and keydown is determined by the remote scancode repeat interval and with typically ~110ms on nec/rc-5 a lot lower. This affects both "quick fingers" using a standard remote and users of programmable remotes like the Logitech Harmony where the number of scancodes transmitted on a short press can be configured. With a single scancode transmission we run into the long keydown delay, 2 scancodes is fine, and at 3 or 4 we start running into the key repeat issue. We received several reports from users that their remote felt "sluggish" when we switched from the downstream "amremote" driver (which IIRC decoded the nec protocol in hardware) to meson-ir. Lowering the timeout to 125ms or even significantly lower (depending on what the protocol and IR receiver permits) removes this "sluggishness", users report that their remote is more "responsive". so long, Hias
Hi Matthias, On Sat, Mar 10, 2018 at 06:38:28PM +0100, Matthias Reichl wrote: > On Sat, Mar 10, 2018 at 11:27:45AM +0000, Sean Young wrote: > > So if the timeout is below N then you will never get a space of N or larger; > > the largest space I know of in an IR message is 40ms in the sanyo protocol: > > > > https://www.sbprojects.net/knowledge/ir/sharp.php > > > > So if timeout is set to less than 40ms, we would have trouble decoding the > > sharp protocol. > > > > The space between nec repeats is a little less than 100ms. I'm trying to > > remember what would could go wrong if the space between them would be > > timeouts instead. Mauro do you remember? I can imagine some IR hardware > > (e.g. winbond) queuing up IR after generating a timeout, thus delaying > > delivering IR to the kernel and we ending up generating a key up. > > > > The problem with a higher timeout is that the trailing space (=timeout) > > is sometimes needed for decoding, and decoding of the last message is > > delayed until the timeout is received. That means the keyup message is > > delayed until that time, making keys a bit "sticky" and more likely to > > generate repeats. I'm pretty sure that is needed for rc-5 and nec. > > Another issue with high timeouts is the response to very short button > presses where the remote only transmits a single scancode. It then > takes signal transmission time plus timeout, so roughly a quarter > of a second on meson-ir and ite-cir with 200ms timeout, until the > scancode is decoded and the keydown event is generated. > > On longer button presses this is less of an issue as we get the > space signal when the first pulse of the repeated scancode is > received. So the delay between button press and keydown is determined > by the remote scancode repeat interval and with typically ~110ms > on nec/rc-5 a lot lower. > > This affects both "quick fingers" using a standard remote and > users of programmable remotes like the Logitech Harmony where > the number of scancodes transmitted on a short press can be > configured. With a single scancode transmission we run into > the long keydown delay, 2 scancodes is fine, and at 3 or 4 we > start running into the key repeat issue. > > We received several reports from users that their remote felt > "sluggish" when we switched from the downstream "amremote" driver > (which IIRC decoded the nec protocol in hardware) to meson-ir. > > Lowering the timeout to 125ms or even significantly lower > (depending on what the protocol and IR receiver permits) > removes this "sluggishness", users report that their remote > is more "responsive". That makes complete sense. I'm actually keen to get this lowered, since this makes it possible to lower the repeat period per-protocol, see commit d57ea877af38 which had to be reverted (the ite driver will need fixing up as well before this can happen). Lowering to below 125ms does increase the risk of regressions, so I am weary of that. Do you think there is benefit in doing this? Thanks Sean
Hi Sean, On Sun, Mar 11, 2018 at 12:55:19PM +0000, Sean Young wrote: > Hi Matthias, > > On Sat, Mar 10, 2018 at 06:38:28PM +0100, Matthias Reichl wrote: > > On Sat, Mar 10, 2018 at 11:27:45AM +0000, Sean Young wrote: > > > So if the timeout is below N then you will never get a space of N or larger; > > > the largest space I know of in an IR message is 40ms in the sanyo protocol: > > > > > > https://www.sbprojects.net/knowledge/ir/sharp.php > > > > > > So if timeout is set to less than 40ms, we would have trouble decoding the > > > sharp protocol. > > > > > > The space between nec repeats is a little less than 100ms. I'm trying to > > > remember what would could go wrong if the space between them would be > > > timeouts instead. Mauro do you remember? I can imagine some IR hardware > > > (e.g. winbond) queuing up IR after generating a timeout, thus delaying > > > delivering IR to the kernel and we ending up generating a key up. > > > > > > The problem with a higher timeout is that the trailing space (=timeout) > > > is sometimes needed for decoding, and decoding of the last message is > > > delayed until the timeout is received. That means the keyup message is > > > delayed until that time, making keys a bit "sticky" and more likely to > > > generate repeats. I'm pretty sure that is needed for rc-5 and nec. > > > > Another issue with high timeouts is the response to very short button > > presses where the remote only transmits a single scancode. It then > > takes signal transmission time plus timeout, so roughly a quarter > > of a second on meson-ir and ite-cir with 200ms timeout, until the > > scancode is decoded and the keydown event is generated. > > > > On longer button presses this is less of an issue as we get the > > space signal when the first pulse of the repeated scancode is > > received. So the delay between button press and keydown is determined > > by the remote scancode repeat interval and with typically ~110ms > > on nec/rc-5 a lot lower. > > > > This affects both "quick fingers" using a standard remote and > > users of programmable remotes like the Logitech Harmony where > > the number of scancodes transmitted on a short press can be > > configured. With a single scancode transmission we run into > > the long keydown delay, 2 scancodes is fine, and at 3 or 4 we > > start running into the key repeat issue. > > > > We received several reports from users that their remote felt > > "sluggish" when we switched from the downstream "amremote" driver > > (which IIRC decoded the nec protocol in hardware) to meson-ir. > > > > Lowering the timeout to 125ms or even significantly lower > > (depending on what the protocol and IR receiver permits) > > removes this "sluggishness", users report that their remote > > is more "responsive". > > That makes complete sense. I'm actually keen to get this lowered, since > this makes it possible to lower the repeat period per-protocol, see > commit d57ea877af38 which had to be reverted (the ite driver will > need fixing up as well before this can happen). I remember the commit, this issue hit us in LibreELEC testbuilds as well :-) > Lowering to below 125ms does increase the risk of regressions, so I > am weary of that. Do you think there is benefit in doing this? I'd also say stick to the 125ms default. The default settings should always be safe ones IMO. People who want to optimize for the last bit of performance can easily do that on their own, at their own risk. Personally I've been using gpio-ir-recv on RPi with the default 125ms timeout and a Hauppauge rc-5 remote for about 2 years now and I've always been happy with that. I have to acknowledge though that the responsiveness of a remote with short messages, like rc-5, in combination with a low timeout (I tested down to 10ms) is pretty impressive. so long, Hias
On Mon, Mar 12, 2018 at 02:20:00PM +0100, Matthias Reichl wrote: > On Sun, Mar 11, 2018 at 12:55:19PM +0000, Sean Young wrote: > > That makes complete sense. I'm actually keen to get this lowered, since > > this makes it possible to lower the repeat period per-protocol, see > > commit d57ea877af38 which had to be reverted (the ite driver will > > need fixing up as well before this can happen). > > I remember the commit, this issue hit us in LibreELEC testbuilds > as well :-) > > > Lowering to below 125ms does increase the risk of regressions, so I > > am weary of that. Do you think there is benefit in doing this? > > I'd also say stick to the 125ms default. The default settings > should always be safe ones IMO. Well, yes. I just wanted to explore the ideal situation before making up our minds. > People who want to optimize for the last bit of performance can > easily do that on their own, at their own risk. > > > Personally I've been using gpio-ir-recv on RPi with the default 125ms > timeout and a Hauppauge rc-5 remote for about 2 years now and I've > always been happy with that. Ok. We should try to get this change for meson-ir ready for v4.17. I can write a patch later. > I have to acknowledge though that the responsiveness of a remote > with short messages, like rc-5, in combination with a low timeout > (I tested down to 10ms) is pretty impressive. I've been thinking about this problem. What we could do is have a per-protocol maximum space length, and repeat period. The timeout can then be set to a maximum space length (+safety margin), and the keyup timer can be set to timeout + repeat period (+safety margin). If memory serves, the lirc daemon always sets the timeout with LIRC_SET_REC_TIMEOUT, so it would not affect lirc daemon decoding. Anyway, just an idea. Not something for v4.17. Thanks, Sean
Hi Sean, On Mon, Mar 12, 2018 at 01:58:11PM +0000, Sean Young wrote: > On Mon, Mar 12, 2018 at 02:20:00PM +0100, Matthias Reichl wrote: > > On Sun, Mar 11, 2018 at 12:55:19PM +0000, Sean Young wrote: > > > That makes complete sense. I'm actually keen to get this lowered, since > > > this makes it possible to lower the repeat period per-protocol, see > > > commit d57ea877af38 which had to be reverted (the ite driver will > > > need fixing up as well before this can happen). > > > > I remember the commit, this issue hit us in LibreELEC testbuilds > > as well :-) > > > > > Lowering to below 125ms does increase the risk of regressions, so I > > > am weary of that. Do you think there is benefit in doing this? > > > > I'd also say stick to the 125ms default. The default settings > > should always be safe ones IMO. > > Well, yes. I just wanted to explore the ideal situation before making > up our minds. > > > People who want to optimize for the last bit of performance can > > easily do that on their own, at their own risk. > > > > > > Personally I've been using gpio-ir-recv on RPi with the default 125ms > > timeout and a Hauppauge rc-5 remote for about 2 years now and I've > > always been happy with that. > > Ok. We should try to get this change for meson-ir ready for v4.17. I can > write a patch later. Thanks, it worked fine! > > I have to acknowledge though that the responsiveness of a remote > > with short messages, like rc-5, in combination with a low timeout > > (I tested down to 10ms) is pretty impressive. > > I've been thinking about this problem. What we could do is have a > per-protocol maximum space length, and repeat period. The timeout > can then be set to a maximum space length (+safety margin), and the > keyup timer can be set to timeout + repeat period (+safety margin). This sounds like a very good idea. It won't help much with IR receivers that have no configurable timeout or a large minimum timeout (ite-cir has 100ms min, probably a hardware limitation?), but for other receivers this'll be a nice improvement. > If memory serves, the lirc daemon always sets the timeout with > LIRC_SET_REC_TIMEOUT, so it would not affect lirc daemon decoding. Current versions of Lirc (0.9.4 and newer) don't seem to use LIRC_SET_REC_TIMEOUT but handle timeouts on it's own via a timeout value in poll(). There's still some generic code in lircd.cpp that supports setting timeouts via LIRC_SET_REC_TIMEOUT but the default plugin (which handles /dev/lircX) doesn't implement any of the required get/set timeout ioctls. strace on lircd 0.10.0 also shows that only LIRC_GET_FEATURES is used. Older Lirc versions (checked with 0.9.1 source I had here) seem to be using LIRC_SET_REC_TIMEOUT. So I think we should be fine here. Not sure if there are other users of the /dev/lirc interface that could be affected, I'm only familiar with lirc and the tools from v4l-utils. > Anyway, just an idea. Not something for v4.17. No need to rush things, your idea looks good to me but better test it thoroughly. Drop me a line if you have a first implementation, I'd be happy to help with testing. so long, Hias
diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c index f2204eb77e2a..64b0aa4f4db7 100644 --- a/drivers/media/rc/meson-ir.c +++ b/drivers/media/rc/meson-ir.c @@ -97,8 +97,7 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id) status = readl_relaxed(ir->reg + IR_DEC_STATUS); rawir.pulse = !!(status & STATUS_IR_DEC_IN); - ir_raw_event_store(ir->rc, &rawir); - ir_raw_event_handle(ir->rc); + ir_raw_event_store_with_timeout(ir->rc, &rawir); spin_unlock(&ir->lock); diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c index 984bb82851f9..374f83105a23 100644 --- a/drivers/media/rc/rc-ir-raw.c +++ b/drivers/media/rc/rc-ir-raw.c @@ -92,7 +92,6 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse) { ktime_t now; DEFINE_IR_RAW_EVENT(ev); - int rc = 0; if (!dev->raw) return -EINVAL; @@ -101,8 +100,33 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse) ev.duration = ktime_to_ns(ktime_sub(now, dev->raw->last_event)); ev.pulse = !pulse; + return ir_raw_event_store_with_timeout(dev, &ev); +} +EXPORT_SYMBOL_GPL(ir_raw_event_store_edge); + +/* + * ir_raw_event_store_with_timeout() - pass a pulse/space duration to the raw + * ir decoders, schedule decoding and + * timeout + * @dev: the struct rc_dev device descriptor + * @ev: the struct ir_raw_event descriptor of the pulse/space + * + * This routine (which may be called from an interrupt context) stores a + * pulse/space duration for the raw ir decoding state machines, schedules + * decoding and generates a timeout. + */ +int ir_raw_event_store_with_timeout(struct rc_dev *dev, struct ir_raw_event *ev) +{ + ktime_t now; + int rc = 0; + + if (!dev->raw) + return -EINVAL; + + now = ktime_get(); + spin_lock(&dev->raw->edge_spinlock); - rc = ir_raw_event_store(dev, &ev); + rc = ir_raw_event_store(dev, ev); dev->raw->last_event = now; @@ -117,7 +141,7 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse) return rc; } -EXPORT_SYMBOL_GPL(ir_raw_event_store_edge); +EXPORT_SYMBOL_GPL(ir_raw_event_store_with_timeout); /** * ir_raw_event_store_with_filter() - pass next pulse/space to decoders with some processing diff --git a/include/media/rc-core.h b/include/media/rc-core.h index fc3a92668bab..6742fd86ff65 100644 --- a/include/media/rc-core.h +++ b/include/media/rc-core.h @@ -334,7 +334,9 @@ void ir_raw_event_handle(struct rc_dev *dev); int ir_raw_event_store(struct rc_dev *dev, struct ir_raw_event *ev); int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse); int ir_raw_event_store_with_filter(struct rc_dev *dev, - struct ir_raw_event *ev); + struct ir_raw_event *ev); +int ir_raw_event_store_with_timeout(struct rc_dev *dev, + struct ir_raw_event *ev); void ir_raw_event_set_idle(struct rc_dev *dev, bool idle); int ir_raw_encode_scancode(enum rc_proto protocol, u32 scancode, struct ir_raw_event *events, unsigned int max);