At Wed, 15 May 2013 14:47:15 +0200,
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)
Let's hope that no one sets the mode value ever... :)
But yes, other than that, it looks feasible indeed.
Takashi
> {
> snd_mixer_t *mixer;
> assert(mixerp);
> mixer = calloc(1, sizeof(*mixer));
> if (mixer == NULL)
> return -ENOMEM;
> + mixer->attach_mode = mode;
> INIT_LIST_HEAD(&mixer->slaves);
> INIT_LIST_HEAD(&mixer->classes);
> INIT_LIST_HEAD(&mixer->elems);
> @@ -200,7 +201,7 @@ int snd_mixer_attach(snd_mixer_t *mixer, const char
> *name)
> snd_hctl_t *hctl;
> int err;
>
> - err = snd_hctl_open(&hctl, name, 0);
> + err = snd_hctl_open(&hctl, name, mixer->attach_mode);
> if (err < 0)
> return err;
> err = snd_mixer_attach_hctl(mixer, hctl);
> diff --git a/src/mixer/mixer_local.h b/src/mixer/mixer_local.h
> index 27b4a3b..2d1866e 100644
> --- a/src/mixer/mixer_local.h
> +++ b/src/mixer/mixer_local.h
> @@ -71,6 +71,7 @@ struct _snd_mixer {
> unsigned int count;
> unsigned int alloc;
> unsigned int events;
> + int attach_mode;
> snd_mixer_callback_t callback;
> void *callback_private;
> snd_mixer_compare_t compare;
>
>
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
>
