Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Thadeu Lima de Souza Cascardo]

On Wed, Jul 16, 2008 at 01:47:03AM +0200, Takashi Iwai wrote:
> At Wed, 9 Jul 2008 14:26:38 -0300,
> Thadeu Lima de Souza Cascardo wrote:
> > 
> > On Wed, Jul 09, 2008 at 08:23:12PM +0200, Takashi Iwai wrote:
> > > At Tue, 8 Jul 2008 14:31:22 -0300,
> > > Thadeu Lima de Souza Cascardo wrote:
> > > > 
> > > > On Wed, Jul 09, 2008 at 05:07:17PM +0200, Takashi Iwai wrote:
> > > > > At Tue, 8 Jul 2008 13:59:32 -0300,
> > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > 
> > > > > > On Tue, Jul 08, 2008 at 12:16:35PM +0200, Takashi Iwai wrote:
> > > > > > > At Mon, 7 Jul 2008 14:36:55 -0300,
> > > > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > > > 
> > > > > > > > On Mon, Jul 07, 2008 at 06:39:09PM +0200, Takashi Iwai wrote:
> > > > > > > > > At Sun, 6 Jul 2008 14:15:56 -0300,
> > > > > > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > > > > > 
> > > > > > > > > > If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
> > > > > > > > > > initializing the codecs. While resuming, it does not wait for all codecs
> > > > > > > > > > to be active. Sound card does not work after a resume without it,
> > > > > > > > > > however. This patch fixes it.
> > > > > > > > > 
> > > > > > > > > Thanks for the patch.
> > > > > > > > > 
> > > > > > > > > But, I still don't figure out why this is needed.
> > > > > > > > > In the else block (with the comment "resume phase"), you can find the
> > > > > > > > > loop to wait for the all *probed* codecs.  The difference with the
> > > > > > > > > code you moved is that it checks only the bits corresponding to the
> > > > > > > > > already probed codecs.  In other words, the other bits should be
> > > > > > > > > irrelevant with the hardware.
> > > > > > > > > 
> > > > > > > > > I guess it's not about the loop but the additional 1/4 sec delay that
> > > > > > > > > did fix the resume on your device.  Please check how is the status
> > > > > > > > > bits and whether it passed the loop in the middle.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Takashi
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > The 1/4 sec delay came in my mind as one of the possible reasons, and
> > > > > > > > that's why I've made some tests. status and nstatus are 0x200, while
> > > > > > > > codec_isr_bits is 0x300. The loop waits for the status register give us
> > > > > > > > 0x300 as the active codecs, instead of the only one probed. Since the
> > > > > > > > cold reset in the case of a power saving cleans up all codec registers,
> > > > > > > > it is needed that we wait for all codecs again (like in the probe case).
> > > > > > > 
> > > > > > > You loaded the modem driver as well?
> > > > > > > If so, what happens if you unload modem driver?
> > > > > > > 
> > > > > > > 
> > > > > > > thanks,
> > > > > > > 
> > > > > > > Takashi
> > > > > > > 
> > > > > > 
> > > > > > The modem driver has always been loaded. Unloading it with my patch
> > > > > > works OK.
> > > > > 
> > > > > Well, I meant to unload the modem driver *without* your patch.
> > > > > That is, does it the unmodified version work if you unload the modem
> > > > > driver beforehand (e.g. adding it to blacklist)?
> > > > > 
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > Takashi
> > > > > 
> > > > 
> > > > I blacklisted snd_intel8x0m, rebooted, it was not loaded, sound worked.
> > > > Suspended, resumed, sound didn't work, as usual.
> > > 
> > > Thanks for checking.
> > > 
> > > I still don't understand why you need to wait for all codecs.
> > > Certainly the secondary one is the modem codec, and it has nothing to
> > > do with the audio codec...
> > > 
> > > I don't mind to apply the patch as is since it's harmless except for
> > > the extra delay.  But, the real question is whether it's the codec
> > > probe or just another delay.
> > > 
> > > Note that not all devices have codecs filling all slots like yours.
> > > On the later ICH, it has three slots while usually two of them are
> > > used.  So, on these hardwares, your code results always in just a 25ms
> > > delay.
> > > 
> > > Anyway, could you give your sign-off?
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > 
> > 
> > Maybe we could treat this as a quirk in my device, which is 8086:2485?
> > 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxxxxxx>
> 
> Thanks.
> 
> Meanwhile, I reconsidered about this problem.  I think the check of
> the whole "active" codec slots can be done better in a way like the
> following.  Could you give it a try?
> 
> If this still doesn't work, I suspect it's really the matter of
> additional delay.
> 

It did not work, but it is not really a matter of additional delay
either. Perhaps, when resuming, we are not doing something else. Here is
what I've done:

I printed codec_isr_bits, codec_init_bits and status when probind and
when resuming.

--
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index a282c7c..9ab8383 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -2346,6 +2346,7 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
 				chip->codec_isr_bits;
 		}
 		chip->codec_init_bits = status;
+               snd_printk ("cascardo - resume: %x %x %x\n", chip->codec_isr_bits, chip->codec_init_bits, status);
 	} else {
 		/* resume phase */
 		/* wait until all the probed codecs are ready */
@@ -2357,6 +2358,7 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
 				break;
 			schedule_timeout_uninterruptible(1);
 		} while (time_after_eq(end_time, jiffies));
+               snd_printk ("cascardo - resume: %x %x %x\n", chip->codec_isr_bits, chip->codec_init_bits, status);
 	}
 
 	if (chip->device_type == DEVICE_SIS) {
--

Here is what I've got:

cascardo@vespa:~/code/linux/build/alsa$ dmesg | grep cascardo
[ 1835.512078] cascardo - probe: 300 200 200
[ 1848.511084] cascardo - resume: 300 200 200

Then, I've waited for codec_isr_bits, instead of codec_init_bits when
resuming, and here is what I've got.


[ 2033.909078] cascardo - probe: 300 200 200
[ 2069.692183] cascardo - resume: 300 200 300
cascardo@vespa:~/code/linux/build/alsa$


Considering that status is 0x0200 when probing, which means we spend
those HZ/4 for nothing, and the driver works, there is something the
driver should do when resuming so it could work with status 0x0200.
However, we wait for 0x0300 and we get it in less than one second.

Remember this only happens when there is a cold reset while resuming. If
not cold reseting, here is what I get:

[ 3855.656460] cascardo - probe: 300 300 300
[ 3866.633160] cascardo - resume: 300 300 300

Printing the control register the driver writes in this section of the
code:

--
@@ -2305,3 +2305,3 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
        /* finish cold or do warm reset */
        cnt |= (cnt & ICH_AC97COLD) == 0 ? ICH_AC97COLD : ICH_AC97WARM;
        iputdword(chip, ICHREG(GLOB_CNT), cnt);
--

I've got
[ 4261.231603] cascardo - reset: 0x6
[ 4269.748091] cascardo - reset: 0x2

The first while probing (which means we do a warm reset) and the second
while resuming, which does not mean a cold reset, but finishing a cold
reset. All this is with ac97 power save enabled:

cascardo@vespa:~/code/linux/build/alsa$ cat /sys/module/snd_ac97_codec/parameters/power_save
Y
cascardo@vespa:~/code/linux/build/alsa$

This version I've built does not have your patch for power_save to be an
integer with the timeout in seconds. So this works as a timeout of 2
seconds. I've tried the driver with 4 seconds of timeout, without my
patch, and it didn't work either. So I don't think this timeout is
related.

So, my question is: is there any device (perhaps the later ICH) that
needs a cold reset with power_save enabled? Mine didn't and, perhaps, we
should simply remove that ifdef and do the reset as usual, which will
work out for me.

Thanks for the attention,
Thadeu Cascardo.

> 
> thanks,
> 
> Takashi
> ---
> diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> index 048d99e..a282c7c 100644
> --- a/sound/pci/intel8x0.c
> +++ b/sound/pci/intel8x0.c
> @@ -404,6 +404,7 @@ struct intel8x0 {
>  	unsigned int *codec_bit;
>  	unsigned int codec_isr_bits;
>  	unsigned int codec_ready_bits;
> +	unsigned int codec_init_bits;
>  
>  	spinlock_t reg_lock;
>  	
> @@ -2278,7 +2279,7 @@ static void do_ali_reset(struct intel8x0 *chip)
>  static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
>  {
>  	unsigned long end_time;
> -	unsigned int cnt, status, nstatus;
> +	unsigned int cnt, status;
>  	
>  	/* put logic to right state */
>  	/* first clear status bits */
> @@ -2344,20 +2345,15 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
>  			status |= igetdword(chip, ICHREG(GLOB_STA)) &
>  				chip->codec_isr_bits;
>  		}
> -
> +		chip->codec_init_bits = status;
>  	} else {
>  		/* resume phase */
> -		int i;
> -		status = 0;
> -		for (i = 0; i < chip->ncodecs; i++)
> -			if (chip->ac97[i])
> -				status |= chip->codec_bit[chip->ac97_sdin[i]];
>  		/* wait until all the probed codecs are ready */
>  		end_time = jiffies + HZ;
>  		do {
> -			nstatus = igetdword(chip, ICHREG(GLOB_STA)) &
> +			status = igetdword(chip, ICHREG(GLOB_STA)) &
>  				chip->codec_isr_bits;
> -			if (status == nstatus)
> +			if (status == chip->codec_init_bits)
>  				break;
>  			schedule_timeout_uninterruptible(1);
>  		} while (time_after_eq(end_time, jiffies));

Attachment: signature.asc
Description: Digital signature