On Thu, Aug 10, 2017 at 12:45:42PM +0200, Hans Verkuil wrote:
> On 09/08/17 23:17, Sean Young wrote:
> > 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.verk...@cisco.com>
> >>>>
> >>>> 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.
>
> Yes, that's to prevent it from going into repeat mode. It shouldn't for
> the first CEC key press message. Remember that CEC is slow, so it may
> well take 500ms before you get the next message. And if REP_DELAY < 500ms
> it will already start repeating which is not what you want for the first
> key press. Calling keyup immediately will prevent this from happening.
>
> > Another way of doing this is to set REP_DELAY/REP_PERIOD to 0 and
> > sending the repeats yourself.
>
> No, because you want the user to be able to influence the repeat speed.
> And the repeat speed is supposed to be that of linux, the repeated CEC
> messages are just to tell linux that it has to remain in key repeating
> mode. I.e. if you receive 5 CEC messages while in Press and Hold mode,
> then that can translate to 20 actual repeats (depending on the REP_PERIOD).
>
> Besides, I don't want to have to write the timer code to repeat the keys
> myself, after all, it's already there.
Yes, agreed. I don't think the key up is ideal, but the alternatives
are worse.
> > I'm not sure it's worth it just to prevent the keyup.
Sean
> >
> >
> > 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.verk...@cisco.com>
> >>>> ---
> >>>> 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
