| Message ID | 4DCB39AF.2000807@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On May 11, 2011, at 9:36 PM, Mauro Carvalho Chehab wrote: > Em 12-05-2011 03:10, Mauro Carvalho Chehab escreveu: >> Em 12-05-2011 02:37, Anssi Hannula escreveu: > >>> I don't see any other places: >>> $ git grep 'REP_PERIOD' . >>> dvb/dvb-usb/dvb-usb-remote.c: input_dev->rep[REP_PERIOD] = >>> d->props.rc.legacy.rc_interval; >> >> Indeed, the REP_PERIOD is not adjusted on other drivers. I agree that we >> should change it to something like 125ms, for example, as 33ms is too >> short, as it takes up to 114ms for a repeat event to arrive. >> > IMO, the enclosed patch should do a better job with repeat events, without > needing to change rc-core/input/event logic. > > - > > Subject: Use a more consistent value for RC repeat period > From: Mauro Carvalho Chehab <mchehab@redhat.com> > > The default REP_PERIOD is 33 ms. This doesn't make sense for IR's, > as, in general, an IR repeat scancode is provided at every 110/115ms, > depending on the RC protocol. So, increase its default, to do a > better job avoiding ghost repeat events. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> Yeah, I definitely like this change, and I think it should do some good. I've been pointing a number of people at ir-keytable and its ability to tweak these values from userspace. Most people have been bumping REP_PERIOD up a bit, but also REP_DELAY down a bit, so that repeats actually kick in a bit sooner. I'm fine with leaving 500 as the default there though, and letting individual drivers adjust it if they really want. Acked-by: Jarod Wilson <jarod@redhat.com>
On Thu, May 12, 2011 at 03:36:47AM +0200, Mauro Carvalho Chehab wrote: > Em 12-05-2011 03:10, Mauro Carvalho Chehab escreveu: > > Em 12-05-2011 02:37, Anssi Hannula escreveu: > > >> I don't see any other places: > >> $ git grep 'REP_PERIOD' . > >> dvb/dvb-usb/dvb-usb-remote.c: input_dev->rep[REP_PERIOD] = > >> d->props.rc.legacy.rc_interval; > > > > Indeed, the REP_PERIOD is not adjusted on other drivers. I agree that we > > should change it to something like 125ms, for example, as 33ms is too > > short, as it takes up to 114ms for a repeat event to arrive. > > > IMO, the enclosed patch should do a better job with repeat events, without > needing to change rc-core/input/event logic. > > - > > Subject: Use a more consistent value for RC repeat period > From: Mauro Carvalho Chehab <mchehab@redhat.com> > > The default REP_PERIOD is 33 ms. This doesn't make sense for IR's, > as, in general, an IR repeat scancode is provided at every 110/115ms, > depending on the RC protocol. So, increase its default, to do a > better job avoiding ghost repeat events. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> > > diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c > index f53f9c6..ee67169 100644 > --- a/drivers/media/rc/rc-main.c > +++ b/drivers/media/rc/rc-main.c > @@ -1044,6 +1044,13 @@ int rc_register_device(struct rc_dev *dev) > */ > dev->input_dev->rep[REP_DELAY] = 500; > > + /* > + * As a repeat event on protocols like RC-5 and NEC take as long as > + * 110/114ms, using 33ms as a repeat period is not the right thing > + * to do. > + */ > + dev->input_dev->rep[REP_PERIOD] = 125; > + > path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL); > printk(KERN_INFO "%s: %s as %s\n", > dev_name(&dev->dev), so if I get this right, a XkbSetControls(.. XkbRepeatKeysMask ...) by a client to set the repeat rate would provide the same solution - for those clients/devices affected. The interesting question is how clients would identify the devices that are affected by this (other than trial and error). Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Peter Hutterer wrote: > On Thu, May 12, 2011 at 03:36:47AM +0200, Mauro Carvalho Chehab wrote: >> Em 12-05-2011 03:10, Mauro Carvalho Chehab escreveu: >>> Em 12-05-2011 02:37, Anssi Hannula escreveu: >>>> I don't see any other places: >>>> $ git grep 'REP_PERIOD' . >>>> dvb/dvb-usb/dvb-usb-remote.c: input_dev->rep[REP_PERIOD] = >>>> d->props.rc.legacy.rc_interval; >>> Indeed, the REP_PERIOD is not adjusted on other drivers. I agree that we >>> should change it to something like 125ms, for example, as 33ms is too >>> short, as it takes up to 114ms for a repeat event to arrive. >>> >> IMO, the enclosed patch should do a better job with repeat events, without >> needing to change rc-core/input/event logic. >> >> - >> >> Subject: Use a more consistent value for RC repeat period >> From: Mauro Carvalho Chehab<mchehab@redhat.com> >> >> The default REP_PERIOD is 33 ms. This doesn't make sense for IR's, >> as, in general, an IR repeat scancode is provided at every 110/115ms, >> depending on the RC protocol. So, increase its default, to do a >> better job avoiding ghost repeat events. >> >> Signed-off-by: Mauro Carvalho Chehab<mchehab@redhat.com> >> >> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c >> index f53f9c6..ee67169 100644 >> --- a/drivers/media/rc/rc-main.c >> +++ b/drivers/media/rc/rc-main.c >> @@ -1044,6 +1044,13 @@ int rc_register_device(struct rc_dev *dev) >> */ >> dev->input_dev->rep[REP_DELAY] = 500; >> >> + /* >> + * As a repeat event on protocols like RC-5 and NEC take as long as >> + * 110/114ms, using 33ms as a repeat period is not the right thing >> + * to do. >> + */ >> + dev->input_dev->rep[REP_PERIOD] = 125; >> + >> path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL); >> printk(KERN_INFO "%s: %s as %s\n", >> dev_name(&dev->dev), > > so if I get this right, a XkbSetControls(.. XkbRepeatKeysMask ...) by a > client to set the repeat rate would provide the same solution - for those > clients/devices affected. > > The interesting question is how clients would identify the devices that are > affected by this (other than trial and error). ir-keytable in v4l-utils is able to identify rc event devices by way of prodding in /sys/class/rc/, but I'm assuming that means every client would have to grow insight into how to do the same.
On 12.05.2011 04:36, Mauro Carvalho Chehab wrote: > Em 12-05-2011 03:10, Mauro Carvalho Chehab escreveu: >> Em 12-05-2011 02:37, Anssi Hannula escreveu: > >>> I don't see any other places: >>> $ git grep 'REP_PERIOD' . >>> dvb/dvb-usb/dvb-usb-remote.c: input_dev->rep[REP_PERIOD] = >>> d->props.rc.legacy.rc_interval; >> >> Indeed, the REP_PERIOD is not adjusted on other drivers. I agree that we >> should change it to something like 125ms, for example, as 33ms is too >> short, as it takes up to 114ms for a repeat event to arrive. >> > IMO, the enclosed patch should do a better job with repeat events, without > needing to change rc-core/input/event logic. It will indeed reduce the amount of ghost events so it brings us in the right direction. I'd still like to get rid of the ghost repeats entirely, or at least some way for users to do it if we don't do it by default. Maybe we could replace the kernel softrepeat with native repeats (for those protocols/drivers that have them), while making sure that repeat events before REP_DELAY are ignored and repeat events less than REP_PERIOD since the previous event are ignored, so the users can still configure them as they like? Or maybe just a module option that causes rc-core to use native repeat events, for those of us that want accurate repeat events without ghosting? > - > > Subject: Use a more consistent value for RC repeat period > From: Mauro Carvalho Chehab <mchehab@redhat.com> > > The default REP_PERIOD is 33 ms. This doesn't make sense for IR's, > as, in general, an IR repeat scancode is provided at every 110/115ms, > depending on the RC protocol. So, increase its default, to do a > better job avoiding ghost repeat events. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> > > diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c > index f53f9c6..ee67169 100644 > --- a/drivers/media/rc/rc-main.c > +++ b/drivers/media/rc/rc-main.c > @@ -1044,6 +1044,13 @@ int rc_register_device(struct rc_dev *dev) > */ > dev->input_dev->rep[REP_DELAY] = 500; > > + /* > + * As a repeat event on protocols like RC-5 and NEC take as long as > + * 110/114ms, using 33ms as a repeat period is not the right thing > + * to do. > + */ > + dev->input_dev->rep[REP_PERIOD] = 125; > + > path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL); > printk(KERN_INFO "%s: %s as %s\n", > dev_name(&dev->dev), >
Em 13-05-2011 01:48, Anssi Hannula escreveu: > On 12.05.2011 04:36, Mauro Carvalho Chehab wrote: >> Em 12-05-2011 03:10, Mauro Carvalho Chehab escreveu: >>> Em 12-05-2011 02:37, Anssi Hannula escreveu: >> >>>> I don't see any other places: >>>> $ git grep 'REP_PERIOD' . >>>> dvb/dvb-usb/dvb-usb-remote.c: input_dev->rep[REP_PERIOD] = >>>> d->props.rc.legacy.rc_interval; >>> >>> Indeed, the REP_PERIOD is not adjusted on other drivers. I agree that we >>> should change it to something like 125ms, for example, as 33ms is too >>> short, as it takes up to 114ms for a repeat event to arrive. >>> >> IMO, the enclosed patch should do a better job with repeat events, without >> needing to change rc-core/input/event logic. > > It will indeed reduce the amount of ghost events so it brings us in the > right direction. > > I'd still like to get rid of the ghost repeats entirely, or at least > some way for users to do it if we don't do it by default. > Maybe we could replace the kernel softrepeat with native repeats (for > those protocols/drivers that have them), while making sure that repeat > events before REP_DELAY are ignored and repeat events less than > REP_PERIOD since the previous event are ignored, so the users can still > configure them as they like? > This doesn't seem to be the right thing to do. If the kernel will accept 33 ms as the value or REP_PERIOD, but it will internally set the maximum repeat rate is 115 ms (no matter what logic it would use for that), Kernel (or X) shouldn't allow the user to set a smaller value. The thing is that writing a logic to block a small value is not easy, since the max value is protocol-dependent (worse than that, on some cases, it is device-specific). It seems better to add a warning at the userspace tools that delays lower than 115 ms can produce ghost events on IR's. > Or maybe just a module option that causes rc-core to use native repeat > events, for those of us that want accurate repeat events without ghosting? If the user already knows about the possibility to generate ghost effects, with low delays, he can simply not pass a bad value to the kernel, instead of forcing a modprobe parameter that will limit the minimal value. Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14.05.2011 01:39, Mauro Carvalho Chehab wrote: > Em 13-05-2011 01:48, Anssi Hannula escreveu: >> On 12.05.2011 04:36, Mauro Carvalho Chehab wrote: >>> Em 12-05-2011 03:10, Mauro Carvalho Chehab escreveu: >>>> Em 12-05-2011 02:37, Anssi Hannula escreveu: >>> >>>>> I don't see any other places: >>>>> $ git grep 'REP_PERIOD' . >>>>> dvb/dvb-usb/dvb-usb-remote.c: input_dev->rep[REP_PERIOD] = >>>>> d->props.rc.legacy.rc_interval; >>>> >>>> Indeed, the REP_PERIOD is not adjusted on other drivers. I agree that we >>>> should change it to something like 125ms, for example, as 33ms is too >>>> short, as it takes up to 114ms for a repeat event to arrive. >>>> >>> IMO, the enclosed patch should do a better job with repeat events, without >>> needing to change rc-core/input/event logic. >> >> It will indeed reduce the amount of ghost events so it brings us in the >> right direction. >> >> I'd still like to get rid of the ghost repeats entirely, or at least >> some way for users to do it if we don't do it by default. > >> Maybe we could replace the kernel softrepeat with native repeats (for >> those protocols/drivers that have them), while making sure that repeat >> events before REP_DELAY are ignored and repeat events less than >> REP_PERIOD since the previous event are ignored, so the users can still >> configure them as they like? >> > > This doesn't seem to be the right thing to do. If the kernel will > accept 33 ms as the value or REP_PERIOD, but it will internally > set the maximum repeat rate is 115 ms (no matter what logic it would > use for that), Kernel (or X) shouldn't allow the user to set a smaller value. > > The thing is that writing a logic to block a small value is not easy, since > the max value is protocol-dependent (worse than that, on some cases, it is > device-specific). It seems better to add a warning at the userspace tools > that delays lower than 115 ms can produce ghost events on IR's. From what I see, even periods longer than 115 ms can produce ghost events. For example with your patch softrepeat period is 125ms, release timeout 250ms, and a native rate of 110ms: There are 4 native events transmitted at 000 ms 110 ms 220 ms 330 ms (user stops between 330ms and 440ms) This causes these events in the evdev interface: 000: 1 125: 2 250: 2 375: 2 500: 2 550: 0 So we got 1-2 ghost repeat events. >> Or maybe just a module option that causes rc-core to use native repeat >> events, for those of us that want accurate repeat events without ghosting? > > If the user already knows about the possibility to generate ghost effects, > with low delays, he can simply not pass a bad value to the kernel, instead > of forcing a modprobe parameter that will limit the minimal value. There is no "good value" for REP_PERIOD (as in ghost repeats guaranteed gone like with native repeats). Sufficiently large values will make ghost repeats increasingly rare, but the period becomes so long the autorepeat becomes frustratingly slow to use.
Em 14-05-2011 01:07, Anssi Hannula escreveu: > On 14.05.2011 01:39, Mauro Carvalho Chehab wrote: >> Em 13-05-2011 01:48, Anssi Hannula escreveu: >>> On 12.05.2011 04:36, Mauro Carvalho Chehab wrote: >>>> Em 12-05-2011 03:10, Mauro Carvalho Chehab escreveu: >>>>> Em 12-05-2011 02:37, Anssi Hannula escreveu: >>>> >>>>>> I don't see any other places: >>>>>> $ git grep 'REP_PERIOD' . >>>>>> dvb/dvb-usb/dvb-usb-remote.c: input_dev->rep[REP_PERIOD] = >>>>>> d->props.rc.legacy.rc_interval; >>>>> >>>>> Indeed, the REP_PERIOD is not adjusted on other drivers. I agree that we >>>>> should change it to something like 125ms, for example, as 33ms is too >>>>> short, as it takes up to 114ms for a repeat event to arrive. >>>>> >>>> IMO, the enclosed patch should do a better job with repeat events, without >>>> needing to change rc-core/input/event logic. >>> >>> It will indeed reduce the amount of ghost events so it brings us in the >>> right direction. >>> >>> I'd still like to get rid of the ghost repeats entirely, or at least >>> some way for users to do it if we don't do it by default. >> >>> Maybe we could replace the kernel softrepeat with native repeats (for >>> those protocols/drivers that have them), while making sure that repeat >>> events before REP_DELAY are ignored and repeat events less than >>> REP_PERIOD since the previous event are ignored, so the users can still >>> configure them as they like? >>> >> >> This doesn't seem to be the right thing to do. If the kernel will >> accept 33 ms as the value or REP_PERIOD, but it will internally >> set the maximum repeat rate is 115 ms (no matter what logic it would >> use for that), Kernel (or X) shouldn't allow the user to set a smaller value. >> >> The thing is that writing a logic to block a small value is not easy, since >> the max value is protocol-dependent (worse than that, on some cases, it is >> device-specific). It seems better to add a warning at the userspace tools >> that delays lower than 115 ms can produce ghost events on IR's. > > From what I see, even periods longer than 115 ms can produce ghost events. > > For example with your patch softrepeat period is 125ms, release timeout > 250ms, and a native rate of 110ms: > > There are 4 native events transmitted at > 000 ms > 110 ms > 220 ms > 330 ms > (user stops between 330ms and 440ms) > > This causes these events in the evdev interface: > 000: 1 > 125: 2 > 250: 2 > 375: 2 > 500: 2 > 550: 0 > > So we got 1-2 ghost repeat events. > >>> Or maybe just a module option that causes rc-core to use native repeat >>> events, for those of us that want accurate repeat events without ghosting? >> >> If the user already knows about the possibility to generate ghost effects, >> with low delays, he can simply not pass a bad value to the kernel, instead >> of forcing a modprobe parameter that will limit the minimal value. > > There is no "good value" for REP_PERIOD (as in ghost repeats guaranteed > gone like with native repeats). Sufficiently large values will make > ghost repeats increasingly rare, but the period becomes so long the > autorepeat becomes frustratingly slow to use. > The 250 ms delay used internally to wait for a repeat code is there because shorter periods weren't working on one of the first boards we've added to rc core (it was a saa7134 - can't remember much details... too much time ago). I remember that I added it as a per-board timer (or per protocol?), as it seemed to high for me, but later, David sent a series of patches rewriting the entire stuff and proposing to have just one timer, arguing that later this could be changed. As his series were improving rc-core, I ended by acking with the changes. The fact is that REP_PERIODS shorter than that timer makes non-sense, as that timer is used to actually wait for a repeat message. I suspect we should re-work the code, perhaps replacing the 250 ms fixed value by REP_PERIOD. I can't work on it this weekend, as I'm about to leave Hungary to return back home. I suspect that I'll have lots of fun next week, due to a one-week travel, and due to the .40 merge window (I suspect it will be opened next week). Maybe Jarod can find some time to do such patch and test it. Thanks, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15.05.2011 06:41, Mauro Carvalho Chehab wrote: > Em 14-05-2011 01:07, Anssi Hannula escreveu: >> On 14.05.2011 01:39, Mauro Carvalho Chehab wrote: >>> Em 13-05-2011 01:48, Anssi Hannula escreveu: >>>> On 12.05.2011 04:36, Mauro Carvalho Chehab wrote: >>>>> Em 12-05-2011 03:10, Mauro Carvalho Chehab escreveu: >>>>>> Em 12-05-2011 02:37, Anssi Hannula escreveu: >>>>> >>>>>>> I don't see any other places: >>>>>>> $ git grep 'REP_PERIOD' . >>>>>>> dvb/dvb-usb/dvb-usb-remote.c: input_dev->rep[REP_PERIOD] = >>>>>>> d->props.rc.legacy.rc_interval; >>>>>> >>>>>> Indeed, the REP_PERIOD is not adjusted on other drivers. I agree that we >>>>>> should change it to something like 125ms, for example, as 33ms is too >>>>>> short, as it takes up to 114ms for a repeat event to arrive. >>>>>> >>>>> IMO, the enclosed patch should do a better job with repeat events, without >>>>> needing to change rc-core/input/event logic. >>>> >>>> It will indeed reduce the amount of ghost events so it brings us in the >>>> right direction. >>>> >>>> I'd still like to get rid of the ghost repeats entirely, or at least >>>> some way for users to do it if we don't do it by default. >>> >>>> Maybe we could replace the kernel softrepeat with native repeats (for >>>> those protocols/drivers that have them), while making sure that repeat >>>> events before REP_DELAY are ignored and repeat events less than >>>> REP_PERIOD since the previous event are ignored, so the users can still >>>> configure them as they like? >>>> >>> >>> This doesn't seem to be the right thing to do. If the kernel will >>> accept 33 ms as the value or REP_PERIOD, but it will internally >>> set the maximum repeat rate is 115 ms (no matter what logic it would >>> use for that), Kernel (or X) shouldn't allow the user to set a smaller value. >>> >>> The thing is that writing a logic to block a small value is not easy, since >>> the max value is protocol-dependent (worse than that, on some cases, it is >>> device-specific). It seems better to add a warning at the userspace tools >>> that delays lower than 115 ms can produce ghost events on IR's. >> >> From what I see, even periods longer than 115 ms can produce ghost events. >> >> For example with your patch softrepeat period is 125ms, release timeout >> 250ms, and a native rate of 110ms: >> >> There are 4 native events transmitted at >> 000 ms >> 110 ms >> 220 ms >> 330 ms >> (user stops between 330ms and 440ms) >> >> This causes these events in the evdev interface: >> 000: 1 >> 125: 2 >> 250: 2 >> 375: 2 >> 500: 2 >> 550: 0 >> >> So we got 1-2 ghost repeat events. >> >>>> Or maybe just a module option that causes rc-core to use native repeat >>>> events, for those of us that want accurate repeat events without ghosting? >>> >>> If the user already knows about the possibility to generate ghost effects, >>> with low delays, he can simply not pass a bad value to the kernel, instead >>> of forcing a modprobe parameter that will limit the minimal value. >> >> There is no "good value" for REP_PERIOD (as in ghost repeats guaranteed >> gone like with native repeats). Sufficiently large values will make >> ghost repeats increasingly rare, but the period becomes so long the >> autorepeat becomes frustratingly slow to use. >> > The 250 ms delay used internally to wait for a repeat code is there because > shorter periods weren't working on one of the first boards we've added to > rc core (it was a saa7134 - can't remember much details... too much time ago). > > I remember that I added it as a per-board timer (or per protocol?), as it seemed > to high for me, but later, David sent a series of patches rewriting the entire > stuff and proposing to have just one timer, arguing that later this could be > changed. As his series were improving rc-core, I ended by acking with the changes. > > The fact is that REP_PERIODS shorter than that timer makes non-sense, as that > timer is used to actually wait for a repeat message. > > I suspect we should re-work the code, perhaps replacing the 250 ms fixed value > by REP_PERIOD. Well, that still has a 50% chance of a ghost repeat with length 1-125ms (e.g. native rate 110ms, user releases button at 900ms, last native event at 880ms, evdev repeat events at 500,625,750,875,1000ms). It would be significantly better than it was before, though, and I'll have to test it myself to see if it is good enough (though I fear it is not). > I can't work on it this weekend, as I'm about to leave Hungary to return back > home. I suspect that I'll have lots of fun next week, due to a one-week travel, > and due to the .40 merge window (I suspect it will be opened next week). > > Maybe Jarod can find some time to do such patch and test it. > > Thanks, > Mauro. >
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c index f53f9c6..ee67169 100644 --- a/drivers/media/rc/rc-main.c +++ b/drivers/media/rc/rc-main.c @@ -1044,6 +1044,13 @@ int rc_register_device(struct rc_dev *dev) */ dev->input_dev->rep[REP_DELAY] = 500; + /* + * As a repeat event on protocols like RC-5 and NEC take as long as + * 110/114ms, using 33ms as a repeat period is not the right thing + * to do. + */ + dev->input_dev->rep[REP_PERIOD] = 125; + path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL); printk(KERN_INFO "%s: %s as %s\n", dev_name(&dev->dev),