Re: [PATCH 2/2] cec: fix remote control passthrough

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.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.
> 
> 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.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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux