On 23.03.2016 11:50, Tanu Kaskinen wrote: > On Wed, 2016-03-23 at 11:18 +0100, Georg Chini wrote: >> On 23.03.2016 10:55, Tanu Kaskinen wrote: >>> On Tue, 2016-03-22 at 12:12 +0100, Georg Chini wrote: >>>> On 06.12.2015 22:38, Tanu Kaskinen wrote: >>>>> This seems buggy (also without your patch). This code is guarded by "if >>>>> (!in_pop)". The message queue is drained in the beginning of the pop >>>>> callback, and if the POST message that ends an underrun is processed in >>>>> the pop callback, then the rewind is not done, and the underrun won't >>>>> be counted in u->underruns. >>>>> >>>>> Neither of the problems is particularly serious, so we could ignore >>>>> them, but the fix seems simple to me: it should be safe to just remove >>>>> the message queue draining from the pop callback. It would mean that if >>>>> the memblockq is empty, and a POST message is ready for processing when >>>>> the pop callback is called, the audio in the message won't be used, and >>>>> an unnecessary underrun will occur. I believe that's not a real >>>>> problem, however. The memblockq should be empty only when the sink and >>>>> source buffers are full, and if the sink buffer is full, the pop >>>>> callback won't be called. >>>>> >>>> Hi Tanu, >>>> >>>> it looks like this _is_ relevant when running very short latencies. >>>> Those "unnecessary underruns" occur for example when you >>>> try to run HDA -> HDA at 5 ms latency. So I would suggest >>>> to keep it mostly like it is. >>>> Instead of masking the whole block, I would suggest to only mask >>>> the rewind with "if (!in_pop)", so that at least the underruns are still >>>> counted. >>> What are the configured sink and source latencies in that "5 ms >>> latency" setup, and is timer-based scheduling enabled or not? My >>> recollection of the past discussion is that you never agreed to >>> configure the target loopback latency high enough to cover the case >>> where both the sink and source buffers are full. If the target latency >>> is too low in relation to the sink and source latencies, that would >>> explain the underruns. >> Sink & source latency are at 2.33 ms, which seems to be the lowest >> latency that HDA devices support without producing underruns >> or "memblock pool full" messages. Timer based scheduling is enabled. > Ok, 2 x 2.33 ms seems fine. > >>> If it's impossible for me to convince you to avoid configuring >>> inherently unsafe target latencies, >> 5ms should not be inherently unsafe and it is working fine with >> the current code. You can run it for hours without issues, tried >> it often enough meanwhile. Just with your proposed change I am >> hitting underruns nearly immediately and I think the code was put >> there in the first place to avoid this situation. It was not me who >> coded this part. > Ok, so it's a mystery why checking the message queue is necessary. This > mystery should at least be noted in a comment in the code. > > Maybe the reason is that the measured latency contains the full latency > as reported by snd_pcm_delay, while the target latency configuration > only considers the buffer sizes? If the extra latency that is not > included in the buffer size is significant, that makes the latency > controller reduce the amount that is kept in the loopback memblockq. > > What to do about this? I suggest just adding a comment. Feel free to > use the above speculation, if you don't have a better theory. > > -- > Thanks, I'll add some comment. Anyway this was a valuable hint. I can stop to track the "no peek" events now, it looks like this was only necessary because I missed the underuns that were masked by the "if (!in_pop)" condition.
