[alsa-devel] [PATCH fixed bug 4032 1/1] Fixed bug 4032.

[Takashi Iwai]

At Tue, 15 Dec 2009 10:35:12 +0100 (CET),
Jaroslav Kysela wrote:
> 
> On Tue, 15 Dec 2009, Takashi Iwai wrote:
> 
> > At Mon, 14 Dec 2009 13:49:33 -0700,
> > Steve Soule wrote:
> >>
> >> On 12/14/2009 12:01 PM, Takashi Iwai wrote:
> >>> At Mon, 14 Dec 2009 11:41:55 -0700,
> >>> Steve Soule wrote:
> >>>>
> >>>>> From c3387a767f827f57a6b1f6b772d3d7a1b6b39942 Mon Sep 17 00:00:00 2001
> >>>> From: Steve Soule <sts11dbxr at gmail.com>
> >>>> Date: Mon, 14 Dec 2009 11:06:03 -0700
> >>>> Subject: [PATCH fixed bug 4032 1/1] Fixed bug 4032.
> >>>>
> >>>>
> >>>> Signed-off-by: Steve Soule <sts11dbxr at gmail.com>
> >>>
> >>> Could you give a bit more changelog text?
> >>> It's not sure what bug 4032 indicates, and more importantly, why this
> >>> change is needed for fixing what.
> >>
> >> Okay.  Here is a thorough changelog:
> >>
> >> This is a fix of a bug in ac97_codec.c. This bug has existed for many
> >> years in many versions of alsa and versions of the kernel.  The bug
> >> appears with a Soundblaster 16PCI sound card.  With this card, sometimes
> >> the driver works correctly, and sometimes it doesn't.  When it doesn't
> >> work correctly, the driver prints the error message:
> >>
> >> AC'97 0 analog subsections not ready
> >>
> >> and fails to detect the complete set of sound controls.  In particular,
> >> the driver fails to detect the "Master Playback Volume" control, and so
> >> the sound card volume level is arbitrary--usually very low.
> >>
> >> As far as I can tell, the problem is in the function snd_ac97_mixer() in
> >> ac97_codec.c, in the lines immediately before the error message is
> >> printed.  These lines appear to be waiting for the card to come out of
> >> powerdown mode.  If the card does not come up within a timeout, the
> >> error message is printed, and the controls are not detected correctly.
> >>
> >> The timeout in the latest version is 120 ms (though it was 100 ms when I
> >> first reported this bug a year and a half ago).  I experimented with
> >> different timeout values, and found that 1 second was not sufficient,
> >> and that 5 seconds is sufficient.
> >
> > Hrm, usually 1 second is really long enough for any operations.
> > And 5 seconds is already a history since genesis.  If it takes so
> > long, something other is fishy.
> >
> > Although extending the timeout would be relatively harmless, I still
> > don't think this is the point to fix.  More likely the problem is in
> > the codec accessor side...
> 
> Maybe, but ens1371 chip is really old and it might be that the initial 
> codec link synchronization is not ideal.
> 
> What about to add the timeout variable to ac97 structure?

Hmm, it's also harmlful, but I don't think it's worth, too.

My point is that we'd need to look at a different place for the real
problem.  Meanwhile, Steve's patch is OK as it shouldn't regress.
(Thus I pulled your tree, too.)


thanks,

Takashi

> 
> Something like this:
> 
> diff --git a/include/sound/ac97_codec.h b/include/sound/ac97_codec.h
> index 4940045..2ff650a 100644
> --- a/include/sound/ac97_codec.h
> +++ b/include/sound/ac97_codec.h
> @@ -381,6 +381,7 @@
>   #define AC97_SCAP_NO_SPDIF	(1<<9)	/* don't build SPDIF controls */
>   #define AC97_SCAP_EAPD_LED	(1<<10)	/* EAPD as mute LED */
>   #define AC97_SCAP_POWER_SAVE	(1<<11)	/* capable for aggresive power-saving */
> +#define AC97_SCAP_LONGANALOGTMO	(1<<12)	/* long timeout for analog subsect */
> 
>   /* ac97->flags */
>   #define AC97_HAS_PC_BEEP	(1<<0)	/* force PC Speaker usage */
> diff --git a/sound/pci/ac97/ac97_codec.c b/sound/pci/ac97/ac97_codec.c
> index c119206..ec22a9a 100644
> --- a/sound/pci/ac97/ac97_codec.c
> +++ b/sound/pci/ac97/ac97_codec.c
> @@ -2122,7 +2122,10 @@ int snd_ac97_mixer(struct snd_ac97_bus *bus, struct snd_ac97_template *template,
>   		}
>   		/* nothing should be in powerdown mode */
>   		snd_ac97_write_cache(ac97, AC97_GENERAL_PURPOSE, 0);
> -		end_time = jiffies + msecs_to_jiffies(5000);
> +		end_time = 120;
> +		if (ac97->scaps & AC97_SCAP_LONGANALOGTMO)
> +			end_time = 5000;
> +		end_time = jiffies + msecs_to_jiffies(end_time);
>   		do {
>   			if ((snd_ac97_read(ac97, AC97_POWERDOWN) & 0x0f) == 0x0f)
>   				goto __ready_ok;
> diff --git a/sound/pci/ens1370.c b/sound/pci/ens1370.c
> index 2b82c5c..8afe173 100644
> --- a/sound/pci/ens1370.c
> +++ b/sound/pci/ens1370.c
> @@ -1631,7 +1631,7 @@ static int __devinit snd_ensoniq_1371_mixer(struct ensoniq *ensoniq,
>   	ac97.private_data = ensoniq;
>   	ac97.private_free = snd_ensoniq_mixer_free_ac97;
>   	ac97.pci = ensoniq->pci;
> -	ac97.scaps = AC97_SCAP_AUDIO;
> +	ac97.scaps = AC97_SCAP_AUDIO|AC97_SCAP_LONGANALOGTMO;
>   	if ((err = snd_ac97_mixer(pbus, &ac97, &ensoniq->u.es1371.ac97)) < 0)
>   		return err;
>   	if (has_spdif > 0 ||
> 
>  						Jaroslav
> 
> -----
> Jaroslav Kysela <perex at perex.cz>
> Linux Kernel Sound Maintainer
> ALSA Project, Red Hat, Inc.
>