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

[Colin Guthrie]

'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?


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).

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?


Col



-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mandriva Linux Contributor [http://www.mandriva.com/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]