USB input ati_remote autorepeat problem

USB input ati_remote autorepeat problem

[Marko Macek]

Hello!

I have problems with autorepeat in ati_remote (drivers/usb/input) driver 
in "recent" kernels: all keys start repeating immediately without some 
delay.

This makes some things, like changing the channel prev/next or toggling 
fullscreen, etc... impossible/hard.

The problem seems to be related to FILTER_TIME and HZ=250 (which I 
forgot to change).

FILTER_TIME is defined to HZ / 20, and since 250 is not divisible by 20, 
the time will be too short to ignore enough events.

Defining FILTER_TIME to HZ / 20 + 1 seems to fix things, but I'm not 
sure if there are any bad side effects.

    Mark

Re: USB input ati_remote autorepeat problem

[John Daiker]

I have also experience this problem with the ati_remote driver, but 
didn't know the fix.  If I am correct, the HZ always used to default to 
1000HZ, but not it is configurable in the kernel as 1000HZ, 250HZ, or 
100HZ.  By my calculations, the FILTER_TIME will be at most 20% longer 
than before, but as little as 2% bigger.

        HZ       Old        New        % Difference
       100       5          6          20%
       250       12.5       13.5       8%
      1000       50         51         2%

Those are my calculations based on your previously stated definitions.  
The FILTER_TIME const is only used 1 time in ati_remote.c, so I doubt 
there would be a problem redefining it.  Would a redefinition to 50 be 
more appropriate (so keep the repeat delay the same across all platforms)?

Tonight I will recompile and test the driver with the 2 new definitions 
and report on my findings.

John



Marko Macek wrote:
> Hello!
>
> I have problems with autorepeat in ati_remote (drivers/usb/input) 
> driver in "recent" kernels: all keys start repeating immediately 
> without some delay.
>
> This makes some things, like changing the channel prev/next or 
> toggling fullscreen, etc... impossible/hard.
>
> The problem seems to be related to FILTER_TIME and HZ=250 (which I 
> forgot to change).
>
> FILTER_TIME is defined to HZ / 20, and since 250 is not divisible by 
> 20, the time will be too short to ignore enough events.
>
> Defining FILTER_TIME to HZ / 20 + 1 seems to fix things, but I'm not 
> sure if there are any bad side effects.
>
>    Mark
> -
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: USB input ati_remote autorepeat problem

[Nish Aravamudan]

On 6/27/06, Marko Macek <Marko.Macek@gmx.net> wrote:
> Hello!
>
> I have problems with autorepeat in ati_remote (drivers/usb/input) driver
> in "recent" kernels: all keys start repeating immediately without some
> delay.
>
> This makes some things, like changing the channel prev/next or toggling
> fullscreen, etc... impossible/hard.
>
> The problem seems to be related to FILTER_TIME and HZ=250 (which I
> forgot to change).
>
> FILTER_TIME is defined to HZ / 20, and since 250 is not divisible by 20,
> the time will be too short to ignore enough events.
>
> Defining FILTER_TIME to HZ / 20 + 1 seems to fix things, but I'm not
> sure if there are any bad side effects.

Can you try just defining it to msecs_to_jiffies(50)? That should
handle the various HZ cases just fine.

Thanks,
Nish

Re: USB input ati_remote autorepeat problem

[Vojtech Pavlik]

On Tue, Jun 27, 2006 at 02:46:39PM -0700, Nish Aravamudan wrote:
> On 6/27/06, Marko Macek <Marko.Macek@gmx.net> wrote:
> >Hello!
> >
> >I have problems with autorepeat in ati_remote (drivers/usb/input) driver
> >in "recent" kernels: all keys start repeating immediately without some
> >delay.
> >
> >This makes some things, like changing the channel prev/next or toggling
> >fullscreen, etc... impossible/hard.
> >
> >The problem seems to be related to FILTER_TIME and HZ=250 (which I
> >forgot to change).
> >
> >FILTER_TIME is defined to HZ / 20, and since 250 is not divisible by 20,
> >the time will be too short to ignore enough events.
> >
> >Defining FILTER_TIME to HZ / 20 + 1 seems to fix things, but I'm not
> >sure if there are any bad side effects.
> 
> Can you try just defining it to msecs_to_jiffies(50)? That should
> handle the various HZ cases just fine.
 
Indeed, that would be thr right solution. Even better would be to 

	#define	FILTER_TIME	50	/* 50 msec */

and later use

	msecs_to_jiffies(FILTER_TIME)

in the code.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: USB input ati_remote autorepeat problem

[Marko Macek]

Vojtech Pavlik wrote:
> On Tue, Jun 27, 2006 at 02:46:39PM -0700, Nish Aravamudan wrote:
>   
>> On 6/27/06, Marko Macek <Marko.Macek@gmx.net> wrote:
>>     
>>> Hello!
>>>
>>> I have problems with autorepeat in ati_remote (drivers/usb/input) driver
>>> in "recent" kernels: all keys start repeating immediately without some
>>> delay.
>>>
>>> This makes some things, like changing the channel prev/next or toggling
>>> fullscreen, etc... impossible/hard.
>>>
>>> The problem seems to be related to FILTER_TIME and HZ=250 (which I
>>> forgot to change).
>>>
>>> FILTER_TIME is defined to HZ / 20, and since 250 is not divisible by 20,
>>> the time will be too short to ignore enough events.
>>>
>>> Defining FILTER_TIME to HZ / 20 + 1 seems to fix things, but I'm not
>>> sure if there are any bad side effects.
>>>       
>> Can you try just defining it to msecs_to_jiffies(50)? That should
>> handle the various HZ cases just fine.
>>     
>  
> Indeed, that would be thr right solution. Even better would be to 
>
> 	#define	FILTER_TIME	50	/* 50 msec */
>
> and later use
>
> 	msecs_to_jiffies(FILTER_TIME)
>
> in the code.
There is still a problem (reproducible in HZ=100, at least), because 
msec_to_jiffies

#if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
        return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);

Calculates 5 ticks for 50ms, which might seem to be correct, but it 
really isn't,  since 5 ticks can happen in as little as 40 (+eps) ms.

I wonder if this usage of msec_to_jiffies is correct (seems wrong to me).

A working (but not clean) patch might look like this:

--- linux-2.6.17.orig/drivers/usb/input/ati_remote.c    2006-06-29 21:18:15.000000000 +0200

+++ linux-2.6.17/drivers/usb/input/ati_remote.c    2006-07-02 20:10:17.000000000 +0200

@@ -155,9 +155,8 @@

  * events. The hardware generates 5 events for the first keypress

  * and we have to take this into account for an accurate repeat

  * behaviour.

- * (HZ / 20) == 50 ms and works well for me.

  */

-#define FILTER_TIME (HZ / 20)

+#define FILTER_TIME 51 /* msec */

 

 struct ati_remote {

     struct input_dev *idev;

@@ -470,7 +469,7 @@

         /* Filter duplicate events which happen "too close" together. */

         if ((ati_remote->old_data[0] == data[1]) &&

             (ati_remote->old_data[1] == data[2]) &&

-            time_before(jiffies, ati_remote->old_jiffies + FILTER_TIME)) {

+            time_before(jiffies, ati_remote->old_jiffies + msecs_to_jiffies(FILTER_TIME))) {

             ati_remote->repeat_count++;

         } else {

             ati_remote->repeat_count = 0;


Some googling reveals that an old patch used HZ >> 4 (HZ / 16) instead 
of HZ / 20;

Perhaps using msec_to_jiffies(50) + 1 would be the correct fix?

    Mark

Re: USB input ati_remote autorepeat problem

[Vojtech Pavlik]

On Sun, Jul 02, 2006 at 08:37:45PM +0200, Marko Macek wrote:
> Vojtech Pavlik wrote:
> >On Tue, Jun 27, 2006 at 02:46:39PM -0700, Nish Aravamudan wrote:
> >  
> >>On 6/27/06, Marko Macek <Marko.Macek@gmx.net> wrote:
> >>    
> >>>Hello!
> >>>
> >>>I have problems with autorepeat in ati_remote (drivers/usb/input) driver
> >>>in "recent" kernels: all keys start repeating immediately without some
> >>>delay.
> >>>
> >>>This makes some things, like changing the channel prev/next or toggling
> >>>fullscreen, etc... impossible/hard.
> >>>
> >>>The problem seems to be related to FILTER_TIME and HZ=250 (which I
> >>>forgot to change).
> >>>
> >>>FILTER_TIME is defined to HZ / 20, and since 250 is not divisible by 20,
> >>>the time will be too short to ignore enough events.
> >>>
> >>>Defining FILTER_TIME to HZ / 20 + 1 seems to fix things, but I'm not
> >>>sure if there are any bad side effects.
> >>>      
> >>Can you try just defining it to msecs_to_jiffies(50)? That should
> >>handle the various HZ cases just fine.
> >>    
> > 
> >Indeed, that would be thr right solution. Even better would be to 
> >
> >	#define	FILTER_TIME	50	/* 50 msec */
> >
> >and later use
> >
> >	msecs_to_jiffies(FILTER_TIME)
> >
> >in the code.
> There is still a problem (reproducible in HZ=100, at least), because 
> msec_to_jiffies
> 
> #if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
>        return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);
> 
> Calculates 5 ticks for 50ms, which might seem to be correct, but it 
> really isn't,  since 5 ticks can happen in as little as 40 (+eps) ms.
> 
> I wonder if this usage of msec_to_jiffies is correct (seems wrong to me).
> 
> A working (but not clean) patch might look like this:

The patch looks OK to me. You could as well use 60 msec to be on the
safe side, and closer to the "/ 16" version.

> --- linux-2.6.17.orig/drivers/usb/input/ati_remote.c    2006-06-29 
> 21:18:15.000000000 +0200
> 
> +++ linux-2.6.17/drivers/usb/input/ati_remote.c    2006-07-02 
> 20:10:17.000000000 +0200
> 
> @@ -155,9 +155,8 @@
> 
>  * events. The hardware generates 5 events for the first keypress
> 
>  * and we have to take this into account for an accurate repeat
> 
>  * behaviour.
> 
> - * (HZ / 20) == 50 ms and works well for me.
> 
>  */
> 
> -#define FILTER_TIME (HZ / 20)
> 
> +#define FILTER_TIME 51 /* msec */
> 
> 
> 
> struct ati_remote {
> 
>     struct input_dev *idev;
> 
> @@ -470,7 +469,7 @@
> 
>         /* Filter duplicate events which happen "too close" together. */
> 
>         if ((ati_remote->old_data[0] == data[1]) &&
> 
>             (ati_remote->old_data[1] == data[2]) &&
> 
> -            time_before(jiffies, ati_remote->old_jiffies + FILTER_TIME)) {
> 
> +            time_before(jiffies, ati_remote->old_jiffies + 
> msecs_to_jiffies(FILTER_TIME))) {
> 
>             ati_remote->repeat_count++;
> 
>         } else {
> 
>             ati_remote->repeat_count = 0;
> 
> 
> Some googling reveals that an old patch used HZ >> 4 (HZ / 16) instead 
> of HZ / 20;
> 
> Perhaps using msec_to_jiffies(50) + 1 would be the correct fix?
> 
>    Mark
> 
> 
> 

-- 
Vojtech Pavlik
Director SuSE Labs