On Tue, Aug 08, 2017 at 10:11:13AM +0200, Hans Verkuil wrote:
> On 07/08/17 22:58, Sean Young wrote:
> > On Mon, Aug 07, 2017 at 03:31:24PM +0200, Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >>
> >> The 'Press and Hold' operation was not correctly implemented, in
> >> particular the requirement that the repeat doesn't start until
> >> the second identical keypress arrives. The REP_DELAY value also
> >> had to be adjusted (see the comment in the code) to achieve the
> >> desired behavior.
> >
> > I'm afraid I've caused some confusion; I had not read your last message
> > about autorepeat on irc correctly, when I said "exactly".
> >
> > So if the input layer has not received a key up event after a key down
> > event, after REP_DELAY it will generate another key down event every
> > REP_PERIOD. So for example, here I'm holding a button on a rc-5 device
> > for some time. Comments on lines with parentheses.
> >
> > # ir-keytable -t
> > Testing events. Please, press CTRL-C to abort.
> > 1502138577.703695: event type EV_MSC(0x04): scancode = 0x1e11
> > (each time a driver receives something, scancode is reported.)
> > 1502138577.703695: event type EV_KEY(0x01) key_down: KEY_VOLUMEDOWN(0x0072)
> > 1502138577.703695: event type EV_SYN(0x00).
> > 1502138577.817682: event type EV_MSC(0x04): scancode = 0x1e11
> > 1502138577.817682: event type EV_SYN(0x00).
> > (rc-5 repeats the command after 115ms).
> > 1502138577.930676: event type EV_MSC(0x04): scancode = 0x1e11
> > 1502138577.930676: event type EV_SYN(0x00).
> > 1502138578.044682: event type EV_MSC(0x04): scancode = 0x1e11
> > 1502138578.044682: event type EV_SYN(0x00).
> > 1502138578.181690: event type EV_MSC(0x04): scancode = 0x1e11
> > 1502138578.181690: event type EV_SYN(0x00).
> > 1502138578.205667: event type EV_KEY(0x01) key_down: KEY_VOLUMEDOWN(0x0072)
> > (this is 500ms after the initial key down, so this key down is generated
> > by the input layer).
> > 1502138578.205667: event type EV_SYN(0x00).
> > 1502138578.333667: event type EV_KEY(0x01) key_down: KEY_VOLUMEDOWN(0x0072)
> > (this is 500 + 125 ms, so another key down event generated by input layer).
> > 1502138578.333667: event type EV_SYN(0x00).
> > 1502138578.437662: event type EV_KEY(0x01) key_up: KEY_VOLUMEDOWN(0x0072)
> > 1502138578.437662: event type EV_SYN(0x00).
> > (key up generated by rc-core after 250ms after last scancode received)
> >
> > So I think the autorepeat can do exactly what you want, without cec
> > having any special code for it.
>
> It comes close, but not quite, to what I need. It has more to do with the
> CEC peculiarities than the rc code.
>
> Specifically the CEC spec strongly recommends that the first reported key
> press is always handled as a single non-repeating key press. Only if a second
> identical key press is received within 550 ms will the 'Press and Hold' feature
> kick in and will the key start repeating. This until a Release message is
> received, a different key press is received or nothing is received for 550 ms.
Right, that make sense.
> Effectively the REP_DELAY is equal to the time between the first and second
> key press message, and it immediately switches to repeat mode once the second
> key press is received.
>
> This code models this behavior exactly.
Ok, although you're sending a keyup directly after the first keydown.
Another way of doing this is to set REP_DELAY/REP_PERIOD to 0 and
sending the repeats yourself.
I'm not sure it's worth it just to prevent the keyup.
Sean
>
> Regards,
>
> Hans
>
> >
> > On a side note, here you can see that for rc-5 the IR_KEYPRESS_TIMEOUT
> > should be 115ms (with a little extra margin).
> >
> > My apologies.
> >
> > Sean
> >
> >>
> >> The 'enabled_protocols' field was also never set, fix that too. Since
> >> CEC is a fixed protocol the driver has to set this field.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >> ---
> >> drivers/media/cec/cec-adap.c | 56 ++++++++++++++++++++++++++++++++++++++++----
> >> drivers/media/cec/cec-core.c | 13 ++++++++++
> >> include/media/cec.h | 5 ++++
> >> 3 files changed, 69 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
> >> index 1a021828c8d4..6a2f38f000e8 100644
> >> --- a/drivers/media/cec/cec-adap.c
> >> +++ b/drivers/media/cec/cec-adap.c
> >> @@ -1766,6 +1766,9 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
> >> int la_idx = cec_log_addr2idx(adap, dest_laddr);
> >> bool from_unregistered = init_laddr == 0xf;
> >> struct cec_msg tx_cec_msg = { };
> >> +#ifdef CONFIG_MEDIA_CEC_RC
> >> + int scancode;
> >> +#endif
> >>
> >> dprintk(2, "%s: %*ph\n", __func__, msg->len, msg->msg);
> >>
> >> @@ -1854,11 +1857,9 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
> >> */
> >> case 0x60:
> >> if (msg->len == 2)
> >> - rc_keydown(adap->rc, RC_TYPE_CEC,
> >> - msg->msg[2], 0);
> >> + scancode = msg->msg[2];
> >> else
> >> - rc_keydown(adap->rc, RC_TYPE_CEC,
> >> - msg->msg[2] << 8 | msg->msg[3], 0);
> >> + scancode = msg->msg[2] << 8 | msg->msg[3];
> >> break;
> >> /*
> >> * Other function messages that are not handled.
> >> @@ -1871,11 +1872,54 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
> >> */
> >> case 0x56: case 0x57:
> >> case 0x67: case 0x68: case 0x69: case 0x6a:
> >> + scancode = -1;
> >> break;
> >> default:
> >> - rc_keydown(adap->rc, RC_TYPE_CEC, msg->msg[2], 0);
> >> + scancode = msg->msg[2];
> >> + break;
> >> + }
> >> +
> >> + /* Was repeating, but keypress timed out */
> >> + if (adap->rc_repeating && !adap->rc->keypressed) {
> >> + adap->rc_repeating = false;
> >> + adap->rc_last_scancode = -1;
> >> + }
> >> + /* Different keypress from last time, ends repeat mode */
> >> + if (adap->rc_last_scancode != scancode) {
> >> + rc_keyup(adap->rc);
> >> + adap->rc_repeating = false;
> >> + }
> >> + /* We can't handle this scancode */
> >> + if (scancode < 0) {
> >> + adap->rc_last_scancode = scancode;
> >> + break;
> >> + }
> >> +
> >> + /* Send key press */
> >> + rc_keydown(adap->rc, RC_TYPE_CEC, scancode, 0);
> >> +
> >> + /* When in repeating mode, we're done */
> >> + if (adap->rc_repeating)
> >> + break;
> >> +
> >> + /*
> >> + * We are not repeating, but the new scancode is
> >> + * the same as the last one, and this second key press is
> >> + * within 550 ms (the 'Follower Safety Timeout') from the
> >> + * previous key press, so we now enable the repeating mode.
> >> + */
> >> + if (adap->rc_last_scancode == scancode &&
> >> + msg->rx_ts - adap->rc_last_keypress < 550 * NSEC_PER_MSEC) {
> >> + adap->rc_repeating = true;
> >> break;
> >> }
> >> + /*
> >> + * Not in repeating mode, so avoid triggering repeat mode
> >> + * by calling keyup.
> >> + */
> >> + rc_keyup(adap->rc);
> >> + adap->rc_last_scancode = scancode;
> >> + adap->rc_last_keypress = msg->rx_ts;
> >> #endif
> >> break;
> >>
> >> @@ -1885,6 +1929,8 @@ static int cec_receive_notify(struct cec_adapter *adap, struct cec_msg *msg,
> >> break;
> >> #ifdef CONFIG_MEDIA_CEC_RC
> >> rc_keyup(adap->rc);
> >> + adap->rc_repeating = false;
> >> + adap->rc_last_scancode = -1;
> >> #endif
> >> break;
> >>
> >> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> >> index 52f085ba104a..018a95cae6b0 100644
> >> --- a/drivers/media/cec/cec-core.c
> >> +++ b/drivers/media/cec/cec-core.c
> >> @@ -276,9 +276,11 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
> >> adap->rc->input_id.version = 1;
> >> adap->rc->driver_name = CEC_NAME;
> >> adap->rc->allowed_protocols = RC_BIT_CEC;
> >> + adap->rc->enabled_protocols = RC_BIT_CEC;
> >> adap->rc->priv = adap;
> >> adap->rc->map_name = RC_MAP_CEC;
> >> adap->rc->timeout = MS_TO_NS(100);
> >> + adap->rc_last_scancode = -1;
> >> #endif
> >> return adap;
> >> }
> >> @@ -310,6 +312,17 @@ int cec_register_adapter(struct cec_adapter *adap,
> >> adap->rc = NULL;
> >> return res;
> >> }
> >> + /*
> >> + * The REP_DELAY for CEC is really the time between the initial
> >> + * 'User Control Pressed' message and the second. The first
> >> + * keypress is always seen as non-repeating, the second
> >> + * (provided it has the same UI Command) will start the 'Press
> >> + * and Hold' (aka repeat) behavior. By setting REP_DELAY to the
> >> + * same value as REP_PERIOD the expected CEC behavior is
> >> + * reproduced.
> >> + */
> >> + adap->rc->input_dev->rep[REP_DELAY] =
> >> + adap->rc->input_dev->rep[REP_PERIOD];
> >> }
> >> #endif
> >>
> >> diff --git a/include/media/cec.h b/include/media/cec.h
> >> index 224a6e225c52..be3b243a0d5e 100644
> >> --- a/include/media/cec.h
> >> +++ b/include/media/cec.h
> >> @@ -187,6 +187,11 @@ struct cec_adapter {
> >>
> >> u32 tx_timeouts;
> >>
> >> +#ifdef CONFIG_MEDIA_CEC_RC
> >> + bool rc_repeating;
> >> + int rc_last_scancode;
> >> + u64 rc_last_keypress;
> >> +#endif
> >> #ifdef CONFIG_CEC_NOTIFIER
> >> struct cec_notifier *notifier;
> >> #endif
> >> --
> >> 2.13.2
