[pulseaudio-discuss] [PATCH] Do not use tsched watermark if tsched is disabled

[David Henningsson]

2010-09-02 10:41, Colin Guthrie skrev:
> 'Twas brillig, and David Henningsson at 02/09/10 07:29 did gyre and gimble:
>> 2010-09-01 20:06, pl bossart skrev:
>>>>> Probably either one will work, but if we're about to release 0.9.22
>>>>> (heard something from Lennart yet?), I suggest we go with my version for
>>>>> 0.9.22 as that one is the least invasive (only touches non-tsched
>>>>> devices), and keep Pierre's version in master.
>>>>
>>>> Sounds reasonable. Pierre, what's your take?
>>>
>>> That would mean an additional post-release patch for tsched devices. I
>>> am lazy and would prefer a one-stop fix. I don't care if this is
>>> David's or mine, as long as the solution works for both cases.
>>> The only real difference is the bytes/ms parameter. Although ms are
>>> more intuitive, the bytes makes more sense from a hardware point of
>>> view. If you pass a parameter in ms, there might be cases where the
>>> actual number of bytes is lower than the DMA burst, it'll depend on
>>> what frequency the sink operates at. There are some cases where
>>> alsa-sink works at 8kHz (BT-SCO) or 48kHz (all other cases), a 6x
>>> variability in the rewind behavior is difficult to handle.
>>
>> Fair enough, how about the attached compromise (untested)? If you then
>> would like to turn the define of dma_rewind_margin_bytes into a
>> parameter, that should be fairly simple.
> 
> That patch works fine too with the chordtest test.
> 
> In order to do more testing however, I also tried the following two
> cherry picks to stable-queue _instead_ of your patch:
> 
> commit d11cd33e3aff14fdd66826b3252d90b1b0e38c48
> Author: Lennart Poettering <lennart at poettering.net>
> Date:   Tue Feb 23 03:23:22 2010 +0100
> 
>     alsa: don't make use of tsched related variables when tsched is disabled
> 
> commit 4df443bbe682055a41e7c2248877dcc7682a69b8
> Author: Pierre-Louis Bossart <pierre-louis.bossart at intel.com>
> Date:   Thu Apr 29 10:48:11 2010 -0500
> 
>     add rewind-safeguard parameter
> 
>     Rewinding the ring buffer completely causes audible issues with DMAs.
>     Previous solution didn't work with tsched=0, and used tsched_watermark
>     for guardband, which isn't linked to hardware and could become
> really high
>     if underflows occurred.
> 
>     Added separate parameter that can be tuned to hardware limitations
> and size
>     of DMA bursts.
> 
> 
> This approach also fixed the chordtest test case.
> 
> 
> Obviously to fix that test case, I'd rather cherry-pick those two
> patches than introduce a new patch only on stable-queue.
> 
> David, as it's obvious that I'm not fully up-to-speed with how this alsa
> stuff works in any great depth, what do you think your patch adds on top
> of the above two?

So my only worry, but I might be wrong, is that a fixed limit of 256
bytes in combination with high sample rates (think 8 channels, 192 kHz,
32 bit float, that will make 256 bytes 0,04 ms only), will not be
enough. We'll rewind back and before we have time to fill it up again we
will get an underrun (and possibly DMA failure?). So that's why I want
to see a ms based margin as well as a byte-based one.

For tsched devices, the watermark is a dynamic ms-based margin that
seems to have been working well (AFAIK), but for non-tsched devices, we
have had no margin at all, causing failure.

As a summary, Pierre is afraid a ms-based one will rewind too far if the
sample rate is low, and I'm afraid a byte-based one will rewind too far
if the sample rate is high.

> My primary concern is that with the current git master version, the
> u->tsched_watermark is not used at all to calculate unused_nbytes,
> whereas in your patch it is (the usage of u->tsched_watermark was
> removed in git master in Pierre's patch above).
> 
> Now I can see that the general aim in Peirre's change was to prevent
> rewinding too far, but with your patch it seems to stop it rewinding too
> little (have I interpreted this right?) and there is no upper limit on
> the amount rewound (in tsched mode with a massive watermark at least).

Both patches aim not to rewind too far, if my one isn't, something is
fundamentally wrong with my version.

> So is there still something missing in your patch? Can the rewind now be
> too much for the DMA controller (a problem alluded to in the comments in
> Pierre's fix).
> 
> 
> Right, I hope I've asked the "right" questions here. I'm aware that my
> not groking things fully isn't helping  but I'm also keen to not cock
> things up :D
> 
> In order to keep things sensible and prevent divergence, I think it's
> wise to cherry pick the above two patches to stable-queue and make any
> further change needed on top of that. This way the patch should apply to
> git master and stable-queue and I can prevent patch divergence and thus
> the strange reoccurances of problems post 0.9.22 when we ultimately
> release git master under whatever version it turns out to be. I think
> that makes sense. WDYT?

Agreed: You can pick those two patches, and then we add a third patch to
both branches, which brings back the watermark for tsched devices and 20
ms for non-tsched. Assuming my suspicion is not disproved, of course.
What does Pierre think of that?

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic