[PATCH v2 0/7] Improve latency of IR decoding

[Matthias Reichl]

On Tue, Apr 10, 2018 at 07:39:43PM +0100, Sean Young wrote:
> On Tue, Apr 10, 2018 at 07:53:43PM +0200, Matthias Reichl wrote:
> > Hi Sean,
> > 
> > On Sun, Apr 08, 2018 at 10:19:35PM +0100, Sean Young wrote:
> > > The current IR decoding is much too slow. Many IR protocols rely on
> > > a trailing space for decoding (e.g. rc-6 needs to know when the bits
> > > end). The trailing space is generated by the IR timeout, and if this
> > > is longer than required, buttons can feel slow to respond.
> > > 
> > > The other issue is the keyup timer. IR has no concept of a keyup message,
> > > this is implied by the absence of IR. So, minimising the timeout for
> > > this makes buttons less "sticky"; the are released much quicker.
> > > 
> > > With these patches in place, using IR with the builtin decoders is much
> > > improved and feels very snappy.
> > > 
> > > Changes since v1:
> > >  - lost more testing
> > >  - fixed various issues with mce decoder
> > >  - fixed mceusb so it can use better timeout too
> > 
> > thanks, this version is working fine with meson-ir and gpio-rc-recv
> > (latter on RPi). I mainly tested it with rc-5 remotes so far, more
> > will follow and I'll update LibreELEC in a day or two to include
> > the v2 series.
> 
> Thanks again for testing!
> 
> > Also thanks a lot for the mce_kbd fixes, I was just going to dig
> > into the decoder (we received a bug report about stuck keys with
> > mce_kbd last week), your patches can in just at the right time :)
> > 
> > I had a closer look at ir-mce_kbd-decoder.c and noticed 2 things
> > (which can be handled separately):
> > 
> > It looks like the input_sync call in the state machine error
> > path is not necessary:
> > 
> > out:
> > 	dev_dbg(&dev->dev, "failed at state %i (%uus %s)\n",
> > 		data->state, TO_US(ev.duration), TO_STR(ev.pulse));
> > 	data->state = STATE_INACTIVE;
> > 	input_sync(data->idev);
> > 	return -EINVAL;
> > 
> > If I followed the code paths correctly states that generate
> > input events won't go to out, only paths were no events are
> > generated jump to out - but better verify that, I could have
> > missed something.
> 
> The input_sync() patch is in mce_kbd_rx_timeout() code path, which
> doesn't go through this. Hopefully it should fix the keys stuck
> issue.

Yes, I saw that and the input_sync() in mce_kbd_rx_timeout() fixed
this (I could verify that locally).

What I meant is that the input_sync() in the snippet above, which
isn't touched by your patches, doesn't seem necessary, as the
code paths that lead to there don't submit input events. The
happy paths (successful key/mouse event and keyup) call input_sync()
at the end of STATE_FINISHED:

		lsc.scancode = scancode;
		ir_lirc_scancode_event(dev, &lsc);
		data->state = STATE_INACTIVE;
		input_event(data->idev, EV_MSC, MSC_SCAN, scancode);
		input_sync(data->idev);
		return 0;

> > The other thing I noticed is that there's no spinlock synchronizing
> > the events from the timeout callback with the ones from the state
> > machine - like keylock in rc-main. So the code could potentially
> > be racy (if the timeout fires while the state machine is outputting
> > events).
> 
> This timeout is for sending keyups in case no keyup is received from
> the keyboard via input_report_key(). The input system does locking
> for us (see drivers/input/input.c:436).

The synchronization code in rc-main looks like it's used to make
code blocks that eg call input_report_key and input_sync do that
in an atomic way (plus there's the special handling of keyup_jiffies
to prevent a race betwen the keyup timeout callback and new
decoded scancodes.

As both the timeout callback and the state machine send out
multiple key events and end them with input_sync it looks to me
like we'd need to make that atomic as well. Otherwise we could
have eg timeout running, starting to report a bunch of shift
and other keys as up, then interruption by a new key/scancode
calling input_report_key down plus input_sync and then timeout
resuming and reporting the rest of the keys down and again
do input_sync().

so long,

Hias