| Message ID | 20200901150937.150292200@linuxfoundation.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hi! > > [ Upstream commit ea8912b788f8144e7d32ee61e5ccba45424bef83 ] > > usleep_range() may take longer than the max argument due to scheduling, > especially under load. This is causing random errors in the transmitted > IR. Remove the usleep_range() in favour of busy-looping with udelay(). > > Signed-off-by: Sean Young <sean@mess.org> I don't believe this should be in stable. Yes, it probably fixes someone's remote control. It also introduces > half a second (!) with interrupts disabled (according to the code comments), which will break other devices on the system. Less intrusive solutions should be explored, first. Like.. if that part is time-critical, perhaps it should set itself at realtime priority, so that scheduler has motivation to schedule it at the right times? Perhaps usleep_range should be delta, delta+1? Perhaps udelay makes sense to use for more than 10usec? Best regards, Pavel > @@ -87,13 +87,8 @@ static int gpio_ir_tx(struct rc_dev *dev, unsigned int *txbuf, > // space > edge = ktime_add_us(edge, txbuf[i]); > delta = ktime_us_delta(edge, ktime_get()); > - if (delta > 10) { > - spin_unlock_irqrestore(&gpio_ir->lock, flags); > - usleep_range(delta, delta + 10); > - spin_lock_irqsave(&gpio_ir->lock, flags); > - } else if (delta > 0) { > + if (delta > 0) > udelay(delta); > - } > } else { > // pulse > ktime_t last = ktime_add_us(edge, txbuf[i]); > -- > 2.25.1 > >
Hi Sean, On Wed, 2 Sep 2020, Sean Young wrote: > Hi Pavel, > > On Wed, Sep 02, 2020 at 12:25:21PM +0200, Pavel Machek wrote: >> Hi! >>> >>> [ Upstream commit ea8912b788f8144e7d32ee61e5ccba45424bef83 ] >>> >>> usleep_range() may take longer than the max argument due to scheduling, >>> especially under load. This is causing random errors in the transmitted >>> IR. Remove the usleep_range() in favour of busy-looping with udelay(). >>> >>> Signed-off-by: Sean Young <sean@mess.org> >> >> I don't believe this should be in stable. >> >> Yes, it probably fixes someone's remote control. >> >> It also introduces > half a second (!) with interrupts disabled >> (according to the code comments), which will break other devices on >> the system. > > Yes, I've always been uncomfortable with this code, but nothing else I > tried worked. > > BTW the delay has a maximum of half a second, but the point stands of > course. > >> Less intrusive solutions should be explored, first. Like.. if that >> part is time-critical, perhaps it should set itself at realtime >> priority, so that scheduler has motivation to schedule it at the right >> times? > > That is an interesting idea, I'll explore that. > Did you try anything around this? It looks like it might be required for devices like the raspbetty pi zero w (see more details below). Is there a way to temporarily increase the priority of the existing thread? or is the preferred way to do readtime priority with a dedicated thread? Assuming the latter: What's the preferred way to transfer data & control from the user's thread to the dedicated thread and back? I'd guess some sort of queue or mailbox? >> Perhaps usleep_range should be delta, delta+1? > > I'm pretty sure I tried that and it didn't work. I'll give it another go. > Shouldn't max actually be less than delta? Otherwise the code is indicating that it's okay to sleep for longer than delta + rescheduling overhead. I tried your latest patch ("re-introduce sleeping for periods of > 50us") on my Pi Zero W and the post-usleep "remaining" delta is anywhere between -25,500us and -50us (i.e. usleep_range usually oversleeps!). The upstream patch gives very stable results: post-udelay delta is typically <10us (i.e. it's underdelaying just a tiny bit). I tried adding a module_param to tune the sleep threshold buffer but because typical delays are 500us and 1,500us and the worst usleep overruns are way larger than that, effectively I had to set it so that usleep never triggered I even tried usleep_range(0, delta - buffer), but that produced the same behaviour. (I even saw a post-usleep delta of -125,000us once!) I added a call to switch to the FIFO scheduler at priority 50 (the same as the recently proposed sched_set_fifo function would do), and (as long as ir-ctl is run as root) it brings the post-usleep delta to ~500us (or with usleep_range(0, ...) for large deltas the post-usleep delta was between ~500us and ~5000us - still undersleeping and very acceptable). Note that pwm-ir-tx has the same issue and so should probably get the same fixes (when they're figured out what they'll be). >> Perhaps udelay makes sense to use for more than 10usec? > > I don't follow -- what are you suggesting here? > > So any other ideas here would be very welcome. I'm happy to explore options, > so far under load the output is can be total garbage if you're unlucky. > > > Thanks, > > Sean > > >> >> Best regards, >> Pavel >> >>> @@ -87,13 +87,8 @@ static int gpio_ir_tx(struct rc_dev *dev, unsigned int *txbuf, >>> // space >>> edge = ktime_add_us(edge, txbuf[i]); >>> delta = ktime_us_delta(edge, ktime_get()); >>> - if (delta > 10) { >>> - spin_unlock_irqrestore(&gpio_ir->lock, flags); >>> - usleep_range(delta, delta + 10); >>> - spin_lock_irqsave(&gpio_ir->lock, flags); >>> - } else if (delta > 0) { >>> + if (delta > 0) >>> udelay(delta); >>> - } >>> } else { >>> // pulse >>> ktime_t last = ktime_add_us(edge, txbuf[i]); >>> -- >>> 2.25.1 >>> >>> >> >> -- >> (english) http://www.livejournal.com/~pavelmachek >> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html > > > -- - Norman Rasmussen - Email: norman@rasmussen.co.za - Home page: http://norman.rasmussen.co.za/
diff --git a/drivers/media/rc/gpio-ir-tx.c b/drivers/media/rc/gpio-ir-tx.c index cd476cab97820..4e70b67ccd181 100644 --- a/drivers/media/rc/gpio-ir-tx.c +++ b/drivers/media/rc/gpio-ir-tx.c @@ -87,13 +87,8 @@ static int gpio_ir_tx(struct rc_dev *dev, unsigned int *txbuf, // space edge = ktime_add_us(edge, txbuf[i]); delta = ktime_us_delta(edge, ktime_get()); - if (delta > 10) { - spin_unlock_irqrestore(&gpio_ir->lock, flags); - usleep_range(delta, delta + 10); - spin_lock_irqsave(&gpio_ir->lock, flags); - } else if (delta > 0) { + if (delta > 0) udelay(delta); - } } else { // pulse ktime_t last = ktime_add_us(edge, txbuf[i]);