Date 15.5.2013 15:26, Takashi Iwai wrote: > At Wed, 15 May 2013 15:12:17 +0200, > Jaroslav Kysela wrote: >> >> 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. > > Well, alsactl would just restore what's saved. So, if the saved data > already contains the softvol ctl element with the old TLV, it's simply > restored as is. It's enough. > You may think of adding the code to softvol plugin to automatically > rewrite TLV of the existing ctl element if it contains no new TLV > type. But, PA shall skip softvol. Thus, it won't be touched. And > yet, PA would like to skip the control elements that have been created > beforehand. The alsa-lib code can be modified to create or modify the user space control also in the SND_PCM_NO_SOFTVOL case, so the mixer API will be informed that the PCM controls belongs to softvol. I don't see any other problems. Jaroslav -- Jaroslav Kysela <perex at perex.cz> Linux Kernel Sound Maintainer ALSA Project; Red Hat, Inc.
