Re: IR code autorepeat issue?

2010-09-20 Thread Anton Blanchard 

Hi,

   I'm seeing double IR events on 2.6.36-rc2 and a DViCO FusionHDTV DVB-T
   Dual Express.
 
  There's one issue on touching on this constant: it is currently just one
  global timeout value that will be used by all protocols. This timeout
  should be enough to retrieve and proccess the repeat key event on all
  protocols, and on all devices, or we'll need to do a per-protocol (and
  eventually per device) timeout init. From
  http://www.sbprojects.com/knowledge/ir/ir.htm, we see that NEC prococol
  uses 110 ms for repeat code, and we need some aditional time to wake up the
  decoding task. I'd say that anything lower than 150-180ms would risk to not
  decode repeat events with NEC.
  
  I got exactly the same problem when adding RC CORE support at the dib0700
  driver. At that driver, there's an additional time of sending/receiving
  URB's from USB. So, we probably need a higher timeout. Even so, I tried to
  reduce the timeout to 200ms or 150ms (not sure), but it didn't work. So, I
  ended by just patching the dibcom driver to do dev-rep[REP_DELAY] = 500:
 
 Ok, just sent a patch adding it to rc-core, and removing from dib0700 driver.

Thanks, tested and confirmed to work!

I originally hit this on Ubuntu Maverick. Would you be OK if I submit it for
backport to 2.6.35 stable?

Anton
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: IR code autorepeat issue?

2010-09-20 Thread Jarod Wilson 
On Mon, Sep 20, 2010 at 6:55 PM, Anton Blanchard an...@samba.org wrote:

 Hi,

   I'm seeing double IR events on 2.6.36-rc2 and a DViCO FusionHDTV DVB-T
   Dual Express.
 
  There's one issue on touching on this constant: it is currently just one
  global timeout value that will be used by all protocols. This timeout
  should be enough to retrieve and proccess the repeat key event on all
  protocols, and on all devices, or we'll need to do a per-protocol (and
  eventually per device) timeout init. From
  http://www.sbprojects.com/knowledge/ir/ir.htm, we see that NEC prococol
  uses 110 ms for repeat code, and we need some aditional time to wake up the
  decoding task. I'd say that anything lower than 150-180ms would risk to not
  decode repeat events with NEC.
 
  I got exactly the same problem when adding RC CORE support at the dib0700
  driver. At that driver, there's an additional time of sending/receiving
  URB's from USB. So, we probably need a higher timeout. Even so, I tried to
  reduce the timeout to 200ms or 150ms (not sure), but it didn't work. So, I
  ended by just patching the dibcom driver to do dev-rep[REP_DELAY] = 500:

 Ok, just sent a patch adding it to rc-core, and removing from dib0700 driver.

 Thanks, tested and confirmed to work!

 I originally hit this on Ubuntu Maverick. Would you be OK if I submit it for
 backport to 2.6.35 stable?

Nb: both Fedora 14's and Ubuntu 10.10's 2.6.35 kernels have a
considerably newer IR stack than upstream 2.6.35, but this patch
should still be relevant to both, since I don't see it yet in the
Fedora tree, and the Ubuntu patches are more or less identical to at
least the first round of IR stack update patches in the Fedora kernel.
(Hm, I should actually give the Ubuntu folks some updated patches, but
I think they may be pretty well frozen already...) I'll take care of
updating the IR bits in the Fedora kernel, and will point some Ubuntu
folks in this direction as well.

-- 
Jarod Wilson
ja...@wilsonet.com
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: IR code autorepeat issue?

2010-09-08 Thread Mauro Carvalho Chehab 
Em 29-08-2010 12:44, Mauro Carvalho Chehab escreveu:
 Em 29-08-2010 03:40, Anton Blanchard escreveu:

 I'm seeing double IR events on 2.6.36-rc2 and a DViCO FusionHDTV DVB-T Dual
 Express.
 There's one issue on touching on this constant: it is currently just one 
 global 
 timeout value that will be used by all protocols. This timeout should be 
 enough to
 retrieve and proccess the repeat key event on all protocols, and on all 
 devices, or 
 we'll need to do a per-protocol (and eventually per device) timeout init. 
 From 
 http://www.sbprojects.com/knowledge/ir/ir.htm, we see that NEC prococol uses 
 110 ms
 for repeat code, and we need some aditional time to wake up the decoding 
 task. I'd
 say that anything lower than 150-180ms would risk to not decode repeat events 
 with
 NEC.
 
 I got exactly the same problem when adding RC CORE support at the dib0700 
 driver. At
 that driver, there's an additional time of sending/receiving URB's from USB. 
 So, we
 probably need a higher timeout. Even so, I tried to reduce the timeout to 
 200ms or 150ms 
 (not sure), but it didn't work. So, I ended by just patching the dibcom 
 driver to do 
 dev-rep[REP_DELAY] = 500:

Ok, just sent a patch adding it to rc-core, and removing from dib0700 driver.

Cheers,
Mauro.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: IR code autorepeat issue?

2010-08-31 Thread David Härdeman 
On Mon, August 30, 2010 14:50, Mauro Carvalho Chehab wrote:
 Em 29-08-2010 22:26, Andy Walls escreveu:
 How about a keycode sensitive repeat delay?
 A short delay for vol+/-, ch+/-, etc.
 A medium delay for digits, fast forward, rewind, pause, play, stop, etc.
 A long delay for power, mute, etc.

 There are two separate things here:

 1) we need to fix a bug introduced for some remotes;
 2) We may improve the repeat code at rc subsystem.

 For (1), a simple trivial patch is enough/desired. Let's first fix it, and
 then think on improvements.

I agree, and I think setting REP_DELAY = 500 is a good fix for now (note
that it needs to be set after registering the input device). We can tweak
the repeat handling further once rc_core has settled down.

 About a keycode sensitive delay, it might be a good idea, but
 there are some aspects to consider:
*snip*
 IMHO, this would add too much complexity for not much gain.

Agreed, the per-keycode-delay lists would probably not be welcome
in-kernel (as it's basically putting policy in the kernel) and even if we
implemented APIs to let userspace control it, I think it's unlikely that
the vast majority would ever use it (meaning we add useless complexity).

-- 
David Härdeman

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: IR code autorepeat issue?

2010-08-30 Thread Mauro Carvalho Chehab 
Em 29-08-2010 22:26, Andy Walls escreveu:
 How about a keycode sensitive repeat delay?
 A short delay for vol+/-, ch+/-, etc.
 A medium delay for digits, fast forward, rewind, pause, play, stop, etc.
 A long delay for power, mute, etc.

There are two separate things here:

1) we need to fix a bug introduced for some remotes;
2) We may improve the repeat code at rc subsystem.

For (1), a simple trivial patch is enough/desired. Let's first fix it, and then
think on improvements.

About a keycode sensitive delay, it might be a good idea, but there are some
aspects to consider:
- a few remotes already do it. I have one or two remotes here that,
instead of using the remote protocol standard for key repeat for all keys, 
they simply implement their own logic, allowing repeat events only on a few
keys, (like vol and channel keys). So, conflicts may rise with hardware-driven
logic;
- evdev already allows reading/replacing the repeat settings with 
EVIOCGREP/EVIOCSREP;
- to do a per-key repeat events, it is needed to disable the
input repeat handling at the input subsystem and to implement our own logic 
inside
rc-core, or to use some ugly workarounds. It is needed to take some care when 
reinventing 
the wheel. Several old IR codes at drivers/media did some sort of their own 
repeat logic, 
some of them using a very odd logic;
- it is needed to clearly define what's a short/medium/long delay, in 
terms of
two parameters: delay for starting to produce repeat key events and number of
repeat events per second;
- it is needed to define what key will use the each delay timings, and 
what should be done with the other keys that don't match any delay lists;
- a new API set of calls will be needed to set/get the delay timings 
lists,
and to associate a keycode with a specified delay time.

IMHO, this would add too much complexity for not much gain. 
 
 Regards,
 Andy
 
 Mauro Carvalho Chehab mche...@infradead.org wrote:
 
 Em 29-08-2010 03:40, Anton Blanchard escreveu:

 I'm seeing double IR events on 2.6.36-rc2 and a DViCO FusionHDTV DVB-T Dual
 Express. I enabled some debug and it looks like we are only getting one IR
 event from the device as expected:

 [ 1351.032084] ir_keydown: i2c IR (FusionHDTV): key down event, key 0x0067, 
 scancode 0x0051
 [ 1351.281284] ir_keyup: keyup key 0x0067

 ie one key down event and one key up event 250ms later. I wonder if the 
 input
 layer software autorepeat is the culprit. It seems to set autorepeat to 
 start
 at 250ms:

 /*
  * If delay and period are pre-set by the driver, then autorepeating
  * is handled by the driver itself and we don't do it in input.c.
  */
 init_timer(dev-timer);
 if (!dev-rep[REP_DELAY]  !dev-rep[REP_PERIOD]) {
 dev-timer.data = (long) dev;
 dev-timer.function = input_repeat_key;
 dev-rep[REP_DELAY] = 250;
 dev-rep[REP_PERIOD] = 33;
 }

 If I shorten the IR key up events to 100ms via the patch below the problem
 goes away. I guess the other option would be to initialise REP_DELAY and
 REP_PERIOD so the input layer autorepeat doesn't cut in at all. Thoughts?

 Anton
 --

 diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c
 index 7e82a9d..cf44d5a 100644
 --- a/drivers/media/IR/ir-keytable.c
 +++ b/drivers/media/IR/ir-keytable.c
 @@ -22,7 +22,7 @@
  #define IR_TAB_MAX_SIZE8192
  
  /* FIXME: IR_KEYPRESS_TIMEOUT should be protocol specific */
 -#define IR_KEYPRESS_TIMEOUT 250
 +#define IR_KEYPRESS_TIMEOUT 100

 Yes, 250ms is too high, if we want to use REP_DELAY = 250ms.

 There's one issue on touching on this constant: it is currently just one 
 global 
 timeout value that will be used by all protocols. This timeout should be 
 enough to
 retrieve and proccess the repeat key event on all protocols, and on all 
 devices, or 
 we'll need to do a per-protocol (and eventually per device) timeout init. 
 From 
 http://www.sbprojects.com/knowledge/ir/ir.htm, we see that NEC prococol uses 
 110 ms
 for repeat code, and we need some aditional time to wake up the decoding 
 task. I'd
 say that anything lower than 150-180ms would risk to not decode repeat 
 events with
 NEC.

 I got exactly the same problem when adding RC CORE support at the dib0700 
 driver. At
 that driver, there's an additional time of sending/receiving URB's from USB. 
 So, we
 probably need a higher timeout. Even so, I tried to reduce the timeout to 
 200ms or 150ms 
 (not sure), but it didn't work. So, I ended by just patching the dibcom 
 driver to do 
 dev-rep[REP_DELAY] = 500:

 commit 8dc09004978538d211ccc36b5046919489e30a55
 Author: Mauro Carvalho Chehab mche...@redhat.com
 Date:   Sat Jul 31 23:37:19 2010 -0300

V4L/DVB: dib0700: avoid bad repeat

a 250ms delay is too low for this device. It ends by producing false
repeat events. Increase the delay time to 500 ms to

IR code autorepeat issue?

2010-08-29 Thread Anton Blanchard 

I'm seeing double IR events on 2.6.36-rc2 and a DViCO FusionHDTV DVB-T Dual
Express. I enabled some debug and it looks like we are only getting one IR
event from the device as expected:

[ 1351.032084] ir_keydown: i2c IR (FusionHDTV): key down event, key 0x0067, 
scancode 0x0051
[ 1351.281284] ir_keyup: keyup key 0x0067

ie one key down event and one key up event 250ms later. I wonder if the input
layer software autorepeat is the culprit. It seems to set autorepeat to start
at 250ms:

/*
 * If delay and period are pre-set by the driver, then autorepeating
 * is handled by the driver itself and we don't do it in input.c.
 */
init_timer(dev-timer);
if (!dev-rep[REP_DELAY]  !dev-rep[REP_PERIOD]) {
dev-timer.data = (long) dev;
dev-timer.function = input_repeat_key;
dev-rep[REP_DELAY] = 250;
dev-rep[REP_PERIOD] = 33;
}

If I shorten the IR key up events to 100ms via the patch below the problem
goes away. I guess the other option would be to initialise REP_DELAY and
REP_PERIOD so the input layer autorepeat doesn't cut in at all. Thoughts?

Anton
--

diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c
index 7e82a9d..cf44d5a 100644
--- a/drivers/media/IR/ir-keytable.c
+++ b/drivers/media/IR/ir-keytable.c
@@ -22,7 +22,7 @@
 #define IR_TAB_MAX_SIZE8192
 
 /* FIXME: IR_KEYPRESS_TIMEOUT should be protocol specific */
-#define IR_KEYPRESS_TIMEOUT 250
+#define IR_KEYPRESS_TIMEOUT 100
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: IR code autorepeat issue?

2010-08-29 Thread Mauro Carvalho Chehab 
Em 29-08-2010 03:40, Anton Blanchard escreveu:
 
 I'm seeing double IR events on 2.6.36-rc2 and a DViCO FusionHDTV DVB-T Dual
 Express. I enabled some debug and it looks like we are only getting one IR
 event from the device as expected:
 
 [ 1351.032084] ir_keydown: i2c IR (FusionHDTV): key down event, key 0x0067, 
 scancode 0x0051
 [ 1351.281284] ir_keyup: keyup key 0x0067
 
 ie one key down event and one key up event 250ms later. I wonder if the input
 layer software autorepeat is the culprit. It seems to set autorepeat to start
 at 250ms:
 
 /*
  * If delay and period are pre-set by the driver, then autorepeating
  * is handled by the driver itself and we don't do it in input.c.
  */
 init_timer(dev-timer);
 if (!dev-rep[REP_DELAY]  !dev-rep[REP_PERIOD]) {
 dev-timer.data = (long) dev;
 dev-timer.function = input_repeat_key;
 dev-rep[REP_DELAY] = 250;
 dev-rep[REP_PERIOD] = 33;
 }
 
 If I shorten the IR key up events to 100ms via the patch below the problem
 goes away. I guess the other option would be to initialise REP_DELAY and
 REP_PERIOD so the input layer autorepeat doesn't cut in at all. Thoughts?

 Anton
 --
 
 diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c
 index 7e82a9d..cf44d5a 100644
 --- a/drivers/media/IR/ir-keytable.c
 +++ b/drivers/media/IR/ir-keytable.c
 @@ -22,7 +22,7 @@
  #define IR_TAB_MAX_SIZE  8192
  
  /* FIXME: IR_KEYPRESS_TIMEOUT should be protocol specific */
 -#define IR_KEYPRESS_TIMEOUT 250
 +#define IR_KEYPRESS_TIMEOUT 100

Yes, 250ms is too high, if we want to use REP_DELAY = 250ms.

There's one issue on touching on this constant: it is currently just one global 
timeout value that will be used by all protocols. This timeout should be enough 
to
retrieve and proccess the repeat key event on all protocols, and on all 
devices, or 
we'll need to do a per-protocol (and eventually per device) timeout init. From 
http://www.sbprojects.com/knowledge/ir/ir.htm, we see that NEC prococol uses 
110 ms
for repeat code, and we need some aditional time to wake up the decoding task. 
I'd
say that anything lower than 150-180ms would risk to not decode repeat events 
with
NEC.

I got exactly the same problem when adding RC CORE support at the dib0700 
driver. At
that driver, there's an additional time of sending/receiving URB's from USB. 
So, we
probably need a higher timeout. Even so, I tried to reduce the timeout to 200ms 
or 150ms 
(not sure), but it didn't work. So, I ended by just patching the dibcom driver 
to do 
dev-rep[REP_DELAY] = 500:

commit 8dc09004978538d211ccc36b5046919489e30a55
Author: Mauro Carvalho Chehab mche...@redhat.com
Date:   Sat Jul 31 23:37:19 2010 -0300

V4L/DVB: dib0700: avoid bad repeat

a 250ms delay is too low for this device. It ends by producing false
repeat events. Increase the delay time to 500 ms to avoid troubles.

Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com

diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c 
b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 164fa9c..a05d955 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -648,6 +648,9 @@ static int dib0700_probe(struct usb_interface *intf,
else
dev-props.rc.core.bulk_mode = false;
 
+ /* Need a higher delay, to avoid wrong repeat */
+ dev-rc_input_dev-rep[REP_DELAY] = 500;
+
dib0700_rc_setup(dev);

Maybe the better solution is to use, by default:
rc_input_dev-rep[REP_DELAY] = 500;
#define IR_KEYPRESS_TIMEOUT 250

And, eventually, adding a patch to allow changing it per device.

That's said, IMHO, 500ms is a very reasonable time for starting repeat with 
remotes. 
Opinions?

Cheers,
Mauro.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: IR code autorepeat issue?

2010-08-29 Thread Andy Walls 
How about a keycode sensitive repeat delay?
A short delay for vol+/-, ch+/-, etc.
A medium delay for digits, fast forward, rewind, pause, play, stop, etc.
A long delay for power, mute, etc.

Regards,
Andy

Mauro Carvalho Chehab mche...@infradead.org wrote:

Em 29-08-2010 03:40, Anton Blanchard escreveu:
 
 I'm seeing double IR events on 2.6.36-rc2 and a DViCO FusionHDTV DVB-T Dual
 Express. I enabled some debug and it looks like we are only getting one IR
 event from the device as expected:
 
 [ 1351.032084] ir_keydown: i2c IR (FusionHDTV): key down event, key 0x0067, 
 scancode 0x0051
 [ 1351.281284] ir_keyup: keyup key 0x0067
 
 ie one key down event and one key up event 250ms later. I wonder if the input
 layer software autorepeat is the culprit. It seems to set autorepeat to start
 at 250ms:
 
 /*
  * If delay and period are pre-set by the driver, then autorepeating
  * is handled by the driver itself and we don't do it in input.c.
  */
 init_timer(dev-timer);
 if (!dev-rep[REP_DELAY]  !dev-rep[REP_PERIOD]) {
 dev-timer.data = (long) dev;
 dev-timer.function = input_repeat_key;
 dev-rep[REP_DELAY] = 250;
 dev-rep[REP_PERIOD] = 33;
 }
 
 If I shorten the IR key up events to 100ms via the patch below the problem
 goes away. I guess the other option would be to initialise REP_DELAY and
 REP_PERIOD so the input layer autorepeat doesn't cut in at all. Thoughts?

 Anton
 --
 
 diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c
 index 7e82a9d..cf44d5a 100644
 --- a/drivers/media/IR/ir-keytable.c
 +++ b/drivers/media/IR/ir-keytable.c
 @@ -22,7 +22,7 @@
  #define IR_TAB_MAX_SIZE 8192
  
  /* FIXME: IR_KEYPRESS_TIMEOUT should be protocol specific */
 -#define IR_KEYPRESS_TIMEOUT 250
 +#define IR_KEYPRESS_TIMEOUT 100

Yes, 250ms is too high, if we want to use REP_DELAY = 250ms.

There's one issue on touching on this constant: it is currently just one 
global 
timeout value that will be used by all protocols. This timeout should be 
enough to
retrieve and proccess the repeat key event on all protocols, and on all 
devices, or 
we'll need to do a per-protocol (and eventually per device) timeout init. From 
http://www.sbprojects.com/knowledge/ir/ir.htm, we see that NEC prococol uses 
110 ms
for repeat code, and we need some aditional time to wake up the decoding task. 
I'd
say that anything lower than 150-180ms would risk to not decode repeat events 
with
NEC.

I got exactly the same problem when adding RC CORE support at the dib0700 
driver. At
that driver, there's an additional time of sending/receiving URB's from USB. 
So, we
probably need a higher timeout. Even so, I tried to reduce the timeout to 
200ms or 150ms 
(not sure), but it didn't work. So, I ended by just patching the dibcom driver 
to do 
dev-rep[REP_DELAY] = 500:

commit 8dc09004978538d211ccc36b5046919489e30a55
Author: Mauro Carvalho Chehab mche...@redhat.com
Date:   Sat Jul 31 23:37:19 2010 -0300

V4L/DVB: dib0700: avoid bad repeat

a 250ms delay is too low for this device. It ends by producing false
repeat events. Increase the delay time to 500 ms to avoid troubles.

Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com

diff --git a/drivers/media/dvb/dvb-usb/dib0700_core.c 
b/drivers/media/dvb/dvb-usb/dib0700_core.c
index 164fa9c..a05d955 100644
--- a/drivers/media/dvb/dvb-usb/dib0700_core.c
+++ b/drivers/media/dvb/dvb-usb/dib0700_core.c
@@ -648,6 +648,9 @@ static int dib0700_probe(struct usb_interface *intf,
else
dev-props.rc.core.bulk_mode = false;
 
+ /* Need a higher delay, to avoid wrong repeat */
+ dev-rc_input_dev-rep[REP_DELAY] = 500;
+
dib0700_rc_setup(dev);

Maybe the better solution is to use, by default:
   rc_input_dev-rep[REP_DELAY] = 500;
   #define IR_KEYPRESS_TIMEOUT 250

And, eventually, adding a patch to allow changing it per device.

That's said, IMHO, 500ms is a very reasonable time for starting repeat with 
remotes. 
Opinions?

Cheers,
Mauro.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html