Re: [PATCH 4.19 053/125] media: gpio-ir-tx: improve precision of transmitted signal due to scheduling

Norman Rasmussen
2020-10-26T00:54:45Z

[-- Attachment #1: Type: text/plain, Size: 4305 bytes --]

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/