[alsa-devel] PulseAudio and softvol

[Jaroslav Kysela]

Date 15.5.2013 15:05, Takashi Iwai wrote:
> At Wed, 15 May 2013 14:52:53 +0200,
> Jaroslav Kysela wrote:
>>
>> Date 15.5.2013 14:47, David Henningsson wrote:
>>> On 05/15/2013 02:42 PM, Takashi Iwai wrote:
>>>> At Wed, 15 May 2013 13:22:03 +0200,
>>>> Jaroslav Kysela wrote:
>>>>>
>>>>> Date 15.5.2013 13:03, David Henningsson wrote:
>>>>>> On 05/15/2013 12:53 PM, Jaroslav Kysela wrote:
>>>>>>> Date 15.5.2013 12:48, Takashi Iwai wrote:
>>>>>>>> At Wed, 15 May 2013 12:26:51 +0200,
>>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>>
>>>>>>>>> Date 15.5.2013 11:55, Arun Raghavan wrote:
>>>>>>>>>> Hello,
>>>>>>>>>> A number of users have intermittently(?) been hitting a crash in
>>>>>>>>>> alsa-lib 1.0.27 [1, 2] related to the softvol plugin. I'm not able to
>>>>>>>>>> reproduce this reliably, so can't find an easy way to debug/fix.
>>>>>>>>>
>>>>>>>>> The problem is that the offsets are not in sync in this case [1]:
>>>>>>>>>
>>>>>>>>> src_offset = 38560
>>>>>>>>> dst_offset = 38568
>>>>>>>>> frames = 16374
>>>>>>>>>
>>>>>>>>> Could you reproduce this bug in any way? At least snd_pcm_dump() before
>>>>>>>>> the failing snd_pcm_mmap_commit() call might help to determine what was
>>>>>>>>> the status before the assert() was entered.
>>>>>>>>
>>>>>>>> Yep.  And this path is actually with volume 0dB, that is, a simply
>>>>>>>> passthrough in softvol.  Thus the bug may hit essentially any
>>>>>>>> plugins, not specifically softvol.
>>>>>>>>
>>>>>>>>
>>>>>>>>>> However, this raises a tangential question - why do we need softvol to
>>>>>>>>>> be plugged for 'front' at all? David explained to me that this is to
>>>>>>>>>> guarantee the existence of a PCM control. Perhaps I don't fully
>>>>>>>>>> understand this, because I'm unconvinced by the reason. Could someone
>>>>>>>>>> explain/refute?
>>>>>>>>>>
>>>>>>>>>> This is especially bad for us, from PulseAudio's perspective, because we
>>>>>>>>>> aren't getting a zero-copy path.
>>>>>>>>>
>>>>>>>>> If the softvol is set to 0dB (no attenuation or gain), then the ring
>>>>>>>>> buffer pointers are moved without any sample processing, so the
>>>>>>>>> zero-copy functionality is kept.
>>>>>>>>
>>>>>>>> Yeah, a sort of.  The mmap is cleared in the slave PCM, so actually
>>>>>>>> there will be copy operations in underlying layers even though softvol
>>>>>>>> itself does zero copy.
>>>>>>>>
>>>>>>>> Actually it makes no sense to keep softvol for PA, but the problem is
>>>>>>>> always the regression.  There are certainly users without PA, which
>>>>>>>> might still rely on the softvol for such hardware without the amp
>>>>>>>> control.
>>>>>>>>
>>>>>>>> Maybe We can add some flag to indicate whether to handle softvol or
>>>>>>>> not, e.g. defaults.pcm.skip_softvol, and let PA set this in its config
>>>>>>>> space.  Setting a config item itself would break anything, so it'll
>>>>>>>> still work with old alsa-lib (but with softvol).
>>>>>>>
>>>>>>> We have already SND_PCM_NO_SOFTVOL open mode for this purpose, so I
>>>>>>> wonder, why PA does not use it..
>>>>>>
>>>>>> The problem is knowing whether PCM is a softvol or not. In some cases,
>>>>>> we need to set PCM to control hardware volume.
>>>>>>
>>>>>> Maybe, if we could figure this out somehow, we could ignore the PCM
>>>>>> mixer control (or possibly set it to zero) in case PCM is a softvol,
>>>>>> and actually use it if PCM is not a softvol.
>>>>>>
>>>>>> It does not look like this is currently possible from the simple mixer
>>>>>> interface, but I might be missing something?
>>>>>
>>>>> It is not possible. Perhaps, we may create a new dummy mixer control (in
>>>>> an inactive state) which will identify the presence of the softvol
>>>>> plugin, like:
>>>>>
>>>>> "Softvol PCM Playback Volume" - full name for the raw control API
>>>>> "Softvol PCM" - simple mixer name
>>>>
>>>> Well, if changing in such a way, I'd rather drop softvol from
>>>> HDA-Intel.conf.
>>>>
>>>> If we could give some flag in mixer API, we could add a code to filter
>>>> out the user controls from the mixer's hctl.  But snd_mixer_attach()
>>>> takes only the string, and the string modifier may lead to the
>>>> incompatibility when used with an older version.  Hmm.
>>>
>>> That seems solvable to me, something like this:
>>>
>>> diff --git a/src/mixer/mixer.c b/src/mixer/mixer.c
>>> index 56e023d..4afa979 100644
>>> --- a/src/mixer/mixer.c
>>> +++ b/src/mixer/mixer.c
>>> @@ -65,13 +65,14 @@ static int snd_mixer_compare_default(const 
>>> snd_mixer_elem_t *c1,
>>>    * \param mode Open mode
>>>    * \return 0 on success otherwise a negative error code
>>>    */
>>> -int snd_mixer_open(snd_mixer_t **mixerp, int mode ATTRIBUTE_UNUSED)
>>> +int snd_mixer_open(snd_mixer_t **mixerp, int mode)
>>
>> Yes, it could be implemented in this way. A special TLV entry may be
>> introduced to detect, if the control is created by softvol.
> 
> The additional TLV won't work if a control is restored by alsactl, for
> example, unfortunately.

This looks like a bug, doesn't? Anyway, I see some TLV restore code in
alsactl, but the support for all control types should be added not only
for SND_CTL_ELEM_TYPE_INTEGER.

					Jaroslav

-- 
Jaroslav Kysela <perex at perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.