Re: [PATCH 0/3] Improve CEC autorepeat handling

[Hans Verkuil]

On 11/27/2017 10:47 AM, Sean Young wrote:
> Hi Hans,
> 
> On Mon, Nov 27, 2017 at 10:13:51AM +0100, Hans Verkuil wrote:
>> On 11/26/2017 12:47 AM, Dmitry Torokhov wrote:
>>> On Fri, Nov 24, 2017 at 11:43:58AM +, Sean Young wrote:
 Due to the slowness of the CEC bus, autorepeat handling rather special
 on CEC. If the repeated user control pressed message is received, a 
 keydown repeat should be sent immediately.
>>>
>>> This sounds like you want to have hardware autorepeat combined with
>>> software one. This seems fairly specific to CEC and I do not think that
>>> this should be in input core; but stay in the driver.
>>>
>>> Another option just to decide what common delay for CEC autorepeat is
>>> and rely on the standard autorepeat handling. The benefit is that users
>>> can control the delay before autorepeat kicks in.
>>
>> They are not allowed to. Autorepeat is only allowed to start when a second
>> keydown message arrives within 550 ms as per the spec. After that autorepeat
>> continues as long as keydown messages are received within 550ms from the
>> previous one. The actual REP_PERIOD time is unrelated to the frequency of
>> the CEC messages but should be that of the local system.
>>
>> The thing to remember here is that CEC is slooow (400 bits/s) so you cannot
>> send messages at REP_PERIOD rate. You should see it as messages that tell
>> you to enter/stay in autorepeat mode. Not as actual autorepeat messages.
>>
>>>

 By handling this in the input layer, we can remove some ugly code from
 cec, which also sends a keyup event after the first keydown, to prevent
 autorepeat.
>>>
>>> If driver does not want input core to handle autorepeat (but handle
>>> autorepeat by themselves) they should indicate it by setting appropriate
>>> dev->rep[REP_DELAY] and dev->rep[REP_PERIOD] before calling
>>> input_register_device(). This will let input core know that it should
>>> not setup its autorepeat timer.
>>
>> That only means that I have to setup the autorepeat timer myself, there
>> is no benefit in that :-)
>>
>> Sean, I kind of agree with Dmitry here. The way autorepeat works for CEC
>> is pretty specific to that protocol and unlikely to be needed for other
>> protocols.
> 
> That's a valid point, I guess. The only downside is special case handling
> outside the input layer, which would be much simpler in the input layer.
> 
>> It is also no big deal to keep knowledge of that within cec-adap.c.
> 
> So first of all, the sii8620 uses the CEC protocol as well (see commit
> e25f1f7c94e1 drm/bridge/sii8620: add remote control support), so this
> will have to go into rc-core, not cec-adap.c. There was a discussion about
> some time ago.
> 
> The current implementation has an ugly key up event which would be nice
> to do without.
> 
>> The only thing that would be nice to have control over is that with CEC
>> userspace shouldn't be able to change REP_DELAY and that REP_DELAY should
>> always be identical to REP_PERIOD. If this can be done easily, then that
>> would be nice, but it's a nice-to-have in my opinion.
> 
> The REP_DELAY must be equal to REP_PERIOD seems a bit odd to me. In fact,
> I propose something different. If REP_DELAY != 0 then the input layer
> produces autorepeats as normal. If REP_DELAY == 0, then generate repeats
> on the second key down event.
> 
> See patch below.
> 
> Thanks,
> Sean
> 
> From 3f439e326888a0ab8688d73c4276ac87b4225b1c Mon Sep 17 00:00:00 2001
> From: Sean Young 
> Date: Thu, 23 Nov 2017 22:37:10 +
> Subject: [PATCH] media: cec: move cec autorepeat handling to rc-core
> 
> CEC autorepeat is different than other protocols. Autorepeat is triggered
> by the first repeated user control pressed CEC message, rather than a
> fixed REP_DELAY.
> 
> This change also does away with the KEY_UP event directly after the first
> KEY_DOWN event, which was used to stop autorepeat from starting.
> 
> See commit a9a249a2c997 ("media: cec: fix remote control passthrough")
> for the original change.
> 
> Signed-off-by: Sean Young 
> ---
>  drivers/media/cec/cec-adap.c | 56 
> 
>  drivers/media/cec/cec-core.c | 12 --
>  drivers/media/rc/rc-main.c   | 45 +--
>  include/media/cec.h  |  5 
>  include/media/rc-core.h  |  3 +++
>  5 files changed, 51 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
> index 98f88c43f62c..258a91800963 100644
> --- a/drivers/media/cec/cec-adap.c
> +++ b/drivers/media/cec/cec-adap.c
> @@ -1788,9 +1788,6 @@ 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__, 

Re: [PATCH 0/3] Improve CEC autorepeat handling

[Dmitry Torokhov]

On Mon, Nov 27, 2017 at 09:47:24AM +, Sean Young wrote:
> Hi Hans,
> 
> On Mon, Nov 27, 2017 at 10:13:51AM +0100, Hans Verkuil wrote:
> > On 11/26/2017 12:47 AM, Dmitry Torokhov wrote:
> > > On Fri, Nov 24, 2017 at 11:43:58AM +, Sean Young wrote:
> > >> Due to the slowness of the CEC bus, autorepeat handling rather special
> > >> on CEC. If the repeated user control pressed message is received, a 
> > >> keydown repeat should be sent immediately.
> > > 
> > > This sounds like you want to have hardware autorepeat combined with
> > > software one. This seems fairly specific to CEC and I do not think that
> > > this should be in input core; but stay in the driver.
> > > 
> > > Another option just to decide what common delay for CEC autorepeat is
> > > and rely on the standard autorepeat handling. The benefit is that users
> > > can control the delay before autorepeat kicks in.
> > 
> > They are not allowed to. Autorepeat is only allowed to start when a second
> > keydown message arrives within 550 ms as per the spec. After that autorepeat
> > continues as long as keydown messages are received within 550ms from the
> > previous one. The actual REP_PERIOD time is unrelated to the frequency of
> > the CEC messages but should be that of the local system.

Not allowed by whom? If I, as a user, want my remote to start
autorepeating after 400 msec instead of 550, will the police come and
fine me? Please do not confuse the default behavior with allowed one.
The only restriction is that if you have not seen messages for longer
than 550 msecs you should "release" the key.

> > 
> > The thing to remember here is that CEC is slooow (400 bits/s) so you cannot
> > send messages at REP_PERIOD rate. You should see it as messages that tell
> > you to enter/stay in autorepeat mode. Not as actual autorepeat messages.

Right, and they do not have to match autorepeat timings, just control
whether we should continue repeating or generate release event.

> > 
> > > 
> > >>
> > >> By handling this in the input layer, we can remove some ugly code from
> > >> cec, which also sends a keyup event after the first keydown, to prevent
> > >> autorepeat.
> > > 
> > > If driver does not want input core to handle autorepeat (but handle
> > > autorepeat by themselves) they should indicate it by setting appropriate
> > > dev->rep[REP_DELAY] and dev->rep[REP_PERIOD] before calling
> > > input_register_device(). This will let input core know that it should
> > > not setup its autorepeat timer.
> > 
> > That only means that I have to setup the autorepeat timer myself, there
> > is no benefit in that :-)
> > 
> > Sean, I kind of agree with Dmitry here. The way autorepeat works for CEC
> > is pretty specific to that protocol and unlikely to be needed for other
> > protocols.
> 
> That's a valid point, I guess. The only downside is special case handling
> outside the input layer, which would be much simpler in the input layer.
> 
> > It is also no big deal to keep knowledge of that within cec-adap.c.
> 
> So first of all, the sii8620 uses the CEC protocol as well (see commit
> e25f1f7c94e1 drm/bridge/sii8620: add remote control support), so this
> will have to go into rc-core, not cec-adap.c. There was a discussion about
> some time ago.
> 
> The current implementation has an ugly key up event which would be nice
> to do without.
> 
> > The only thing that would be nice to have control over is that with CEC
> > userspace shouldn't be able to change REP_DELAY and that REP_DELAY should
> > always be identical to REP_PERIOD. If this can be done easily, then that
> > would be nice, but it's a nice-to-have in my opinion.

You could do that by catching EV_REP events in your driver, but I do not
see why you want to remove this flexibility from users. Again, if I want
to wait for autorepeat to start for 10 seconds, or 10 msecs, it is my
choice. You are to provide the default, I am to override it if I want.

> 
> The REP_DELAY must be equal to REP_PERIOD seems a bit odd to me. In fact,
> I propose something different. If REP_DELAY != 0 then the input layer
> produces autorepeats as normal. If REP_DELAY == 0, then generate repeats
> on the second key down event.

I have no idea why you simply not rely on current input core autorepeat
handling, set REP_DELAY to be 550 msec and call it a day.

Thanks.

-- 
Dmitry

Re: [PATCH 0/3] Improve CEC autorepeat handling

[Hans Verkuil]

On 11/27/2017 10:47 AM, Sean Young wrote:
> Hi Hans,
> 
> On Mon, Nov 27, 2017 at 10:13:51AM +0100, Hans Verkuil wrote:
>> On 11/26/2017 12:47 AM, Dmitry Torokhov wrote:
>>> On Fri, Nov 24, 2017 at 11:43:58AM +, Sean Young wrote:
 Due to the slowness of the CEC bus, autorepeat handling rather special
 on CEC. If the repeated user control pressed message is received, a 
 keydown repeat should be sent immediately.
>>>
>>> This sounds like you want to have hardware autorepeat combined with
>>> software one. This seems fairly specific to CEC and I do not think that
>>> this should be in input core; but stay in the driver.
>>>
>>> Another option just to decide what common delay for CEC autorepeat is
>>> and rely on the standard autorepeat handling. The benefit is that users
>>> can control the delay before autorepeat kicks in.
>>
>> They are not allowed to. Autorepeat is only allowed to start when a second
>> keydown message arrives within 550 ms as per the spec. After that autorepeat
>> continues as long as keydown messages are received within 550ms from the
>> previous one. The actual REP_PERIOD time is unrelated to the frequency of
>> the CEC messages but should be that of the local system.
>>
>> The thing to remember here is that CEC is slooow (400 bits/s) so you cannot
>> send messages at REP_PERIOD rate. You should see it as messages that tell
>> you to enter/stay in autorepeat mode. Not as actual autorepeat messages.
>>
>>>

 By handling this in the input layer, we can remove some ugly code from
 cec, which also sends a keyup event after the first keydown, to prevent
 autorepeat.
>>>
>>> If driver does not want input core to handle autorepeat (but handle
>>> autorepeat by themselves) they should indicate it by setting appropriate
>>> dev->rep[REP_DELAY] and dev->rep[REP_PERIOD] before calling
>>> input_register_device(). This will let input core know that it should
>>> not setup its autorepeat timer.
>>
>> That only means that I have to setup the autorepeat timer myself, there
>> is no benefit in that :-)
>>
>> Sean, I kind of agree with Dmitry here. The way autorepeat works for CEC
>> is pretty specific to that protocol and unlikely to be needed for other
>> protocols.
> 
> That's a valid point, I guess. The only downside is special case handling
> outside the input layer, which would be much simpler in the input layer.
> 
>> It is also no big deal to keep knowledge of that within cec-adap.c.
> 
> So first of all, the sii8620 uses the CEC protocol as well (see commit
> e25f1f7c94e1 drm/bridge/sii8620: add remote control support), so this
> will have to go into rc-core, not cec-adap.c. There was a discussion about
> some time ago.

Ah yes, that's true. But it still is specific to the CEC protocol, the code
handling that would just move to rc-core from cec-adap.c

> 
> The current implementation has an ugly key up event which would be nice
> to do without.
> 
>> The only thing that would be nice to have control over is that with CEC
>> userspace shouldn't be able to change REP_DELAY and that REP_DELAY should
>> always be identical to REP_PERIOD. If this can be done easily, then that
>> would be nice, but it's a nice-to-have in my opinion.
> 
> The REP_DELAY must be equal to REP_PERIOD seems a bit odd to me. In fact,
> I propose something different. If REP_DELAY != 0 then the input layer
> produces autorepeats as normal. If REP_DELAY == 0, then generate repeats
> on the second key down event.
> 
> See patch below.

That looks nice! I'm fine with that.

Regards,

Hans

> 
> Thanks,
> Sean
> 
> From 3f439e326888a0ab8688d73c4276ac87b4225b1c Mon Sep 17 00:00:00 2001
> From: Sean Young 
> Date: Thu, 23 Nov 2017 22:37:10 +
> Subject: [PATCH] media: cec: move cec autorepeat handling to rc-core
> 
> CEC autorepeat is different than other protocols. Autorepeat is triggered
> by the first repeated user control pressed CEC message, rather than a
> fixed REP_DELAY.
> 
> This change also does away with the KEY_UP event directly after the first
> KEY_DOWN event, which was used to stop autorepeat from starting.
> 
> See commit a9a249a2c997 ("media: cec: fix remote control passthrough")
> for the original change.
> 
> Signed-off-by: Sean Young 
> ---
>  drivers/media/cec/cec-adap.c | 56 
> 
>  drivers/media/cec/cec-core.c | 12 --
>  drivers/media/rc/rc-main.c   | 45 +--
>  include/media/cec.h  |  5 
>  include/media/rc-core.h  |  3 +++
>  5 files changed, 51 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
> index 98f88c43f62c..258a91800963 100644
> --- a/drivers/media/cec/cec-adap.c
> +++ b/drivers/media/cec/cec-adap.c
> @@ -1788,9 +1788,6 @@ static int cec_receive_notify(struct cec_adapter *adap, 
> struct cec_msg *msg,
>   int la_idx = cec_log_addr2idx(adap, dest_laddr);
>   

Re: [PATCH 0/3] Improve CEC autorepeat handling

[Sean Young]

Hi Hans,

On Mon, Nov 27, 2017 at 10:13:51AM +0100, Hans Verkuil wrote:
> On 11/26/2017 12:47 AM, Dmitry Torokhov wrote:
> > On Fri, Nov 24, 2017 at 11:43:58AM +, Sean Young wrote:
> >> Due to the slowness of the CEC bus, autorepeat handling rather special
> >> on CEC. If the repeated user control pressed message is received, a 
> >> keydown repeat should be sent immediately.
> > 
> > This sounds like you want to have hardware autorepeat combined with
> > software one. This seems fairly specific to CEC and I do not think that
> > this should be in input core; but stay in the driver.
> > 
> > Another option just to decide what common delay for CEC autorepeat is
> > and rely on the standard autorepeat handling. The benefit is that users
> > can control the delay before autorepeat kicks in.
> 
> They are not allowed to. Autorepeat is only allowed to start when a second
> keydown message arrives within 550 ms as per the spec. After that autorepeat
> continues as long as keydown messages are received within 550ms from the
> previous one. The actual REP_PERIOD time is unrelated to the frequency of
> the CEC messages but should be that of the local system.
> 
> The thing to remember here is that CEC is slooow (400 bits/s) so you cannot
> send messages at REP_PERIOD rate. You should see it as messages that tell
> you to enter/stay in autorepeat mode. Not as actual autorepeat messages.
> 
> > 
> >>
> >> By handling this in the input layer, we can remove some ugly code from
> >> cec, which also sends a keyup event after the first keydown, to prevent
> >> autorepeat.
> > 
> > If driver does not want input core to handle autorepeat (but handle
> > autorepeat by themselves) they should indicate it by setting appropriate
> > dev->rep[REP_DELAY] and dev->rep[REP_PERIOD] before calling
> > input_register_device(). This will let input core know that it should
> > not setup its autorepeat timer.
> 
> That only means that I have to setup the autorepeat timer myself, there
> is no benefit in that :-)
> 
> Sean, I kind of agree with Dmitry here. The way autorepeat works for CEC
> is pretty specific to that protocol and unlikely to be needed for other
> protocols.

That's a valid point, I guess. The only downside is special case handling
outside the input layer, which would be much simpler in the input layer.

> It is also no big deal to keep knowledge of that within cec-adap.c.

So first of all, the sii8620 uses the CEC protocol as well (see commit
e25f1f7c94e1 drm/bridge/sii8620: add remote control support), so this
will have to go into rc-core, not cec-adap.c. There was a discussion about
some time ago.

The current implementation has an ugly key up event which would be nice
to do without.

> The only thing that would be nice to have control over is that with CEC
> userspace shouldn't be able to change REP_DELAY and that REP_DELAY should
> always be identical to REP_PERIOD. If this can be done easily, then that
> would be nice, but it's a nice-to-have in my opinion.

The REP_DELAY must be equal to REP_PERIOD seems a bit odd to me. In fact,
I propose something different. If REP_DELAY != 0 then the input layer
produces autorepeats as normal. If REP_DELAY == 0, then generate repeats
on the second key down event.

See patch below.

Thanks,
Sean

>From 3f439e326888a0ab8688d73c4276ac87b4225b1c Mon Sep 17 00:00:00 2001
From: Sean Young 
Date: Thu, 23 Nov 2017 22:37:10 +
Subject: [PATCH] media: cec: move cec autorepeat handling to rc-core

CEC autorepeat is different than other protocols. Autorepeat is triggered
by the first repeated user control pressed CEC message, rather than a
fixed REP_DELAY.

This change also does away with the KEY_UP event directly after the first
KEY_DOWN event, which was used to stop autorepeat from starting.

See commit a9a249a2c997 ("media: cec: fix remote control passthrough")
for the original change.

Signed-off-by: Sean Young 
---
 drivers/media/cec/cec-adap.c | 56 
 drivers/media/cec/cec-core.c | 12 --
 drivers/media/rc/rc-main.c   | 45 +--
 include/media/cec.h  |  5 
 include/media/rc-core.h  |  3 +++
 5 files changed, 51 insertions(+), 70 deletions(-)

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index 98f88c43f62c..258a91800963 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -1788,9 +1788,6 @@ 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);
 
@@ -1886,9 +1883,11 @@ static int cec_receive_notify(struct cec_adapter *adap, 
struct cec_msg *msg,
 */
case 0x60:
  

Re: [PATCH 0/3] Improve CEC autorepeat handling

[Hans Verkuil]

On 11/26/2017 12:47 AM, Dmitry Torokhov wrote:
> Hi Sean,
> 
> On Fri, Nov 24, 2017 at 11:43:58AM +, Sean Young wrote:
>> Due to the slowness of the CEC bus, autorepeat handling rather special
>> on CEC. If the repeated user control pressed message is received, a 
>> keydown repeat should be sent immediately.
> 
> This sounds like you want to have hardware autorepeat combined with
> software one. This seems fairly specific to CEC and I do not think that
> this should be in input core; but stay in the driver.
> 
> Another option just to decide what common delay for CEC autorepeat is
> and rely on the standard autorepeat handling. The benefit is that users
> can control the delay before autorepeat kicks in.

They are not allowed to. Autorepeat is only allowed to start when a second
keydown message arrives within 550 ms as per the spec. After that autorepeat
continues as long as keydown messages are received within 550ms from the
previous one. The actual REP_PERIOD time is unrelated to the frequency of
the CEC messages but should be that of the local system.

The thing to remember here is that CEC is slooow (400 bits/s) so you cannot
send messages at REP_PERIOD rate. You should see it as messages that tell
you to enter/stay in autorepeat mode. Not as actual autorepeat messages.

> 
>>
>> By handling this in the input layer, we can remove some ugly code from
>> cec, which also sends a keyup event after the first keydown, to prevent
>> autorepeat.
> 
> If driver does not want input core to handle autorepeat (but handle
> autorepeat by themselves) they should indicate it by setting appropriate
> dev->rep[REP_DELAY] and dev->rep[REP_PERIOD] before calling
> input_register_device(). This will let input core know that it should
> not setup its autorepeat timer.

That only means that I have to setup the autorepeat timer myself, there
is no benefit in that :-)

Sean, I kind of agree with Dmitry here. The way autorepeat works for CEC
is pretty specific to that protocol and unlikely to be needed for other
protocols.

It is also no big deal to keep knowledge of that within cec-adap.c.

The only thing that would be nice to have control over is that with CEC
userspace shouldn't be able to change REP_DELAY and that REP_DELAY should
always be identical to REP_PERIOD. If this can be done easily, then that
would be nice, but it's a nice-to-have in my opinion.

Regards,

Hans

Re: [PATCH 0/3] Improve CEC autorepeat handling

[Dmitry Torokhov]

Hi Sean,

On Fri, Nov 24, 2017 at 11:43:58AM +, Sean Young wrote:
> Due to the slowness of the CEC bus, autorepeat handling rather special
> on CEC. If the repeated user control pressed message is received, a 
> keydown repeat should be sent immediately.

This sounds like you want to have hardware autorepeat combined with
software one. This seems fairly specific to CEC and I do not think that
this should be in input core; but stay in the driver.

Another option just to decide what common delay for CEC autorepeat is
and rely on the standard autorepeat handling. The benefit is that users
can control the delay before autorepeat kicks in.

> 
> By handling this in the input layer, we can remove some ugly code from
> cec, which also sends a keyup event after the first keydown, to prevent
> autorepeat.

If driver does not want input core to handle autorepeat (but handle
autorepeat by themselves) they should indicate it by setting appropriate
dev->rep[REP_DELAY] and dev->rep[REP_PERIOD] before calling
input_register_device(). This will let input core know that it should
not setup its autorepeat timer.

Thanks.

-- 
Dmitry

[PATCH 0/3] Improve CEC autorepeat handling

[Sean Young]

Due to the slowness of the CEC bus, autorepeat handling rather special
on CEC. If the repeated user control pressed message is received, a 
keydown repeat should be sent immediately.

By handling this in the input layer, we can remove some ugly code from
cec, which also sends a keyup event after the first keydown, to prevent
autorepeat.

Sean Young (3):
  input: remove redundant check for EV_REP
  input: handle case whether first repeated key triggers repeat
  media: cec: move cec autorepeat handling to rc-core

 Documentation/input/input.rst |  4 +++-
 drivers/input/input.c | 21 
 drivers/media/cec/cec-adap.c  | 56 ---
 drivers/media/cec/cec-core.c  | 12 --
 drivers/media/rc/rc-main.c| 10 +++-
 include/media/cec.h   |  5 
 6 files changed, 33 insertions(+), 75 deletions(-)

-- 
2.14.3