[PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Troy Kisky]

Poulsbo(US15W) cannot have any corb registers initialized
when using single_cmd mode.
When send_cmd timeout occur, note error.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 sound/pci/hda/hda_intel.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c9ad182..c2ff585 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -524,6 +524,16 @@ static void azx_init_cmd_io(struct azx *chip)
 	/* CORB set up */
 	chip->corb.addr = chip->rb.addr;
 	chip->corb.buf = (u32 *)chip->rb.area;
+
+	/* RIRB set up */
+	chip->rirb.addr = chip->rb.addr + 2048;
+	chip->rirb.buf = (u32 *)(chip->rb.area + 2048);
+	chip->rirb.wp = chip->rirb.rp = 0;
+	memset(chip->rirb.cmds, 0, sizeof(chip->rirb.cmds));
+	if (chip->single_cmd) {
+		spin_unlock_irq(&chip->reg_lock);
+		return;
+	}
 	azx_writel(chip, CORBLBASE, (u32)chip->corb.addr);
 	azx_writel(chip, CORBUBASE, upper_32_bits(chip->corb.addr));
 
@@ -536,11 +546,6 @@ static void azx_init_cmd_io(struct azx *chip)
 	/* enable corb dma */
 	azx_writeb(chip, CORBCTL, ICH6_CORBCTL_RUN);
 
-	/* RIRB set up */
-	chip->rirb.addr = chip->rb.addr + 2048;
-	chip->rirb.buf = (u32 *)(chip->rb.area + 2048);
-	chip->rirb.wp = chip->rirb.rp = 0;
-	memset(chip->rirb.cmds, 0, sizeof(chip->rirb.cmds));
 	azx_writel(chip, RIRBLBASE, (u32)chip->rirb.addr);
 	azx_writel(chip, RIRBUBASE, upper_32_bits(chip->rirb.addr));
 
@@ -783,6 +788,7 @@ static int azx_single_send_cmd(struct hda_bus *bus, u32 val)
 	if (printk_ratelimit())
 		snd_printd(SFX "send_cmd timeout: IRS=0x%x, val=0x%x\n",
 			   azx_readw(chip, IRS), val);
+	chip->rirb.res[addr] = -1;
 	return -EIO;
 }
 
-- 
1.5.6.3

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Takashi Iwai]

At Tue,  3 Nov 2009 12:22:37 -0700,
Troy Kisky wrote:
> 
> Poulsbo(US15W) cannot have any corb registers initialized
> when using single_cmd mode.
> When send_cmd timeout occur, note error.

Could you be more specific?  What errors do you get?

And, how it goes to single_cmd mode?  The single_cmd mode is very last
resort, and reaching there means already a serious problem.


thanks,

Takashi

> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  sound/pci/hda/hda_intel.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index c9ad182..c2ff585 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -524,6 +524,16 @@ static void azx_init_cmd_io(struct azx *chip)
>  	/* CORB set up */
>  	chip->corb.addr = chip->rb.addr;
>  	chip->corb.buf = (u32 *)chip->rb.area;
> +
> +	/* RIRB set up */
> +	chip->rirb.addr = chip->rb.addr + 2048;
> +	chip->rirb.buf = (u32 *)(chip->rb.area + 2048);
> +	chip->rirb.wp = chip->rirb.rp = 0;
> +	memset(chip->rirb.cmds, 0, sizeof(chip->rirb.cmds));
> +	if (chip->single_cmd) {
> +		spin_unlock_irq(&chip->reg_lock);
> +		return;
> +	}
>  	azx_writel(chip, CORBLBASE, (u32)chip->corb.addr);
>  	azx_writel(chip, CORBUBASE, upper_32_bits(chip->corb.addr));
>  
> @@ -536,11 +546,6 @@ static void azx_init_cmd_io(struct azx *chip)
>  	/* enable corb dma */
>  	azx_writeb(chip, CORBCTL, ICH6_CORBCTL_RUN);
>  
> -	/* RIRB set up */
> -	chip->rirb.addr = chip->rb.addr + 2048;
> -	chip->rirb.buf = (u32 *)(chip->rb.area + 2048);
> -	chip->rirb.wp = chip->rirb.rp = 0;
> -	memset(chip->rirb.cmds, 0, sizeof(chip->rirb.cmds));
>  	azx_writel(chip, RIRBLBASE, (u32)chip->rirb.addr);
>  	azx_writel(chip, RIRBUBASE, upper_32_bits(chip->rirb.addr));
>  
> @@ -783,6 +788,7 @@ static int azx_single_send_cmd(struct hda_bus *bus, u32 val)
>  	if (printk_ratelimit())
>  		snd_printd(SFX "send_cmd timeout: IRS=0x%x, val=0x%x\n",
>  			   azx_readw(chip, IRS), val);
> +	chip->rirb.res[addr] = -1;
>  	return -EIO;
>  }
>  
> -- 
> 1.5.6.3
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Troy Kisky]

Takashi Iwai wrote:
> At Tue,  3 Nov 2009 12:22:37 -0700,
> Troy Kisky wrote:
>> Poulsbo(US15W) cannot have any corb registers initialized
>> when using single_cmd mode.
>> When send_cmd timeout occur, note error.
> 
> Could you be more specific?  What errors do you get?
> 
> And, how it goes to single_cmd mode?  The single_cmd mode is very last
> resort, and reaching there means already a serious problem.
> 
> 
> thanks,
> 
> Takashi
> 
No error messages, but the response read is always 0.
For testing, I passed single_cmd=1 as a modules option.

HDAudio_03.pdf says, "If implemented, these registers must not be used
at the same time as the CORB and RIRB command/response mechanisms, as the operations
will conflict."

Plus, if the RIRB irq is enabled, the interrupt routine will print out a
spurious interrupt message.

That said, my hardware is switching to single_cmd eventually, even if not
passed as a module option. But at least now, when that happens my audio
isn't dead.

Troy

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Takashi Iwai]

At Wed, 04 Nov 2009 12:45:20 -0700,
Troy Kisky wrote:
> 
> Takashi Iwai wrote:
> > At Tue,  3 Nov 2009 12:22:37 -0700,
> > Troy Kisky wrote:
> >> Poulsbo(US15W) cannot have any corb registers initialized
> >> when using single_cmd mode.
> >> When send_cmd timeout occur, note error.
> > 
> > Could you be more specific?  What errors do you get?
> > 
> > And, how it goes to single_cmd mode?  The single_cmd mode is very last
> > resort, and reaching there means already a serious problem.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> No error messages, but the response read is always 0.

This is odd.  Something is already wrong, then.

> For testing, I passed single_cmd=1 as a modules option.
> 
> HDAudio_03.pdf says, "If implemented, these registers must not be used
> at the same time as the CORB and RIRB command/response mechanisms, as the operations
> will conflict."

I know this, but actually this never be a problem in the real
hardware.  So, it's left there.

> Plus, if the RIRB irq is enabled, the interrupt routine will print out a
> spurious interrupt message.
> 
> That said, my hardware is switching to single_cmd eventually, even if not
> passed as a module option. But at least now, when that happens my audio
> isn't dead.

It's not dead but living-dead :)
The single_cmd mode is really only for debugging.  It's never for any
running system.


thanks,

Takashi

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Troy Kisky]

Takashi Iwai wrote:
> At Wed, 04 Nov 2009 12:45:20 -0700,
> Troy Kisky wrote:
>> Takashi Iwai wrote:
>>> At Tue,  3 Nov 2009 12:22:37 -0700,
>>> Troy Kisky wrote:
>>>> Poulsbo(US15W) cannot have any corb registers initialized
>>>> when using single_cmd mode.
>>>> When send_cmd timeout occur, note error.
>>> Could you be more specific?  What errors do you get?
>>>
>>> And, how it goes to single_cmd mode?  The single_cmd mode is very last
>>> resort, and reaching there means already a serious problem.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>> No error messages, but the response read is always 0.
> 
> This is odd.  Something is already wrong, then.
> 
>> For testing, I passed single_cmd=1 as a modules option.
>>
>> HDAudio_03.pdf says, "If implemented, these registers must not be used
>> at the same time as the CORB and RIRB command/response mechanisms, as the operations
>> will conflict."
> 
> I know this, but actually this never be a problem in the real
> hardware.  So, it's left there.

My hardware is real.

> 
>> Plus, if the RIRB irq is enabled, the interrupt routine will print out a
>> spurious interrupt message.

I meant "spurious response"

>>
>> That said, my hardware is switching to single_cmd eventually, even if not
>> passed as a module option. But at least now, when that happens my audio
>> isn't dead.
> 
> It's not dead but living-dead :)
> The single_cmd mode is really only for debugging.  It's never for any
> running system.
> 
> 
> thanks,
> 
> Takashi



I'm all in favor of totally yanking out all the single_cmd related code,
but if you're leaving it in, you should apply this patch.

If you want someone else to verify the bug, can you ask someone with a
US15W to add the module parameter single_cmd=1 ???


I really do not understand your reluctance to change code that is "really only for
debugging." That should make changes less dangerous, not more. Especially
as the change seems like an obvious fix to a minor oversight.

My only hesitation would be that unsolicited responses are not handled when
in single_cmd mode. Should they be disabled as well?

Troy

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Takashi Iwai]

At Thu, 05 Nov 2009 12:42:03 -0700,
Troy Kisky wrote:
> 
> Takashi Iwai wrote:
> > At Wed, 04 Nov 2009 12:45:20 -0700,
> > Troy Kisky wrote:
> >> Takashi Iwai wrote:
> >>> At Tue,  3 Nov 2009 12:22:37 -0700,
> >>> Troy Kisky wrote:
> >>>> Poulsbo(US15W) cannot have any corb registers initialized
> >>>> when using single_cmd mode.
> >>>> When send_cmd timeout occur, note error.
> >>> Could you be more specific?  What errors do you get?
> >>>
> >>> And, how it goes to single_cmd mode?  The single_cmd mode is very last
> >>> resort, and reaching there means already a serious problem.
> >>>
> >>>
> >>> thanks,
> >>>
> >>> Takashi
> >>>
> >> No error messages, but the response read is always 0.
> > 
> > This is odd.  Something is already wrong, then.
> > 
> >> For testing, I passed single_cmd=1 as a modules option.
> >>
> >> HDAudio_03.pdf says, "If implemented, these registers must not be used
> >> at the same time as the CORB and RIRB command/response mechanisms, as the operations
> >> will conflict."
> > 
> > I know this, but actually this never be a problem in the real
> > hardware.  So, it's left there.
> 
> My hardware is real.

Yeah, the first hit it seems.

> >> Plus, if the RIRB irq is enabled, the interrupt routine will print out a
> >> spurious interrupt message.
> 
> I meant "spurious response"

So your patch alone isn't enough?

> >> That said, my hardware is switching to single_cmd eventually, even if not
> >> passed as a module option. But at least now, when that happens my audio
> >> isn't dead.
> > 
> > It's not dead but living-dead :)
> > The single_cmd mode is really only for debugging.  It's never for any
> > running system.
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> 
> 
> I'm all in favor of totally yanking out all the single_cmd related code,
> but if you're leaving it in, you should apply this patch.

Well, it's no right fix in a strict manner.
The right fix would be to remove the reinitialization or the first
call of azx_init_cmd_io() (and disable the unsolicited events, too).

> If you want someone else to verify the bug, can you ask someone with a
> US15W to add the module parameter single_cmd=1 ???
> 
> 
> I really do not understand your reluctance to change code that is "really only for
> debugging." That should make changes less dangerous, not more. Especially
> as the change seems like an obvious fix to a minor oversight.

This was no oversight but a sort of intention.
The point is that the single_cmd mode could be used as the fallback
when the serious communication error occurs.  But the unsolicited
handling can't be disabled easily from the codec side (and with a hope
that RIRB still works), so I left the CORB/RIRB code as is.

> My only hesitation would be that unsolicited responses are not handled when
> in single_cmd mode. Should they be disabled as well?

Yes.  Strictly speaking, it can't be handled with single_cmd mode.
Maybe resetting GCTL_UNSOL register bit might be enough, but I'm not
sure.

In anyway, your patch is no real fix for your hardware.  Fixing the
behavior of single_cmd mode is nice, but this is no urgent issue.
The fact that the driver falls into this mode means something very
wrong elsewhere, and this is more important thing to fix right now.
That's why I wrote it's living-dead.  You don't see it's dead but it
is :)

So... this obviously isn't a topic that we focus on and waste time.


thanks,

Takashi

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Takashi Iwai]

At Fri, 06 Nov 2009 08:19:08 +0100,
I wrote:
> 
> At Thu, 05 Nov 2009 12:42:03 -0700,
> Troy Kisky wrote:
> > 
> > Takashi Iwai wrote:
> > > At Wed, 04 Nov 2009 12:45:20 -0700,
> > > Troy Kisky wrote:
> > >> Takashi Iwai wrote:
> > >>> At Tue,  3 Nov 2009 12:22:37 -0700,
> > >>> Troy Kisky wrote:
> > >>>> Poulsbo(US15W) cannot have any corb registers initialized
> > >>>> when using single_cmd mode.
> > >>>> When send_cmd timeout occur, note error.
> > >>> Could you be more specific?  What errors do you get?
> > >>>
> > >>> And, how it goes to single_cmd mode?  The single_cmd mode is very last
> > >>> resort, and reaching there means already a serious problem.
> > >>>
> > >>>
> > >>> thanks,
> > >>>
> > >>> Takashi
> > >>>
> > >> No error messages, but the response read is always 0.
> > > 
> > > This is odd.  Something is already wrong, then.
> > > 
> > >> For testing, I passed single_cmd=1 as a modules option.
> > >>
> > >> HDAudio_03.pdf says, "If implemented, these registers must not be used
> > >> at the same time as the CORB and RIRB command/response mechanisms, as the operations
> > >> will conflict."
> > > 
> > > I know this, but actually this never be a problem in the real
> > > hardware.  So, it's left there.
> > 
> > My hardware is real.
> 
> Yeah, the first hit it seems.
> 
> > >> Plus, if the RIRB irq is enabled, the interrupt routine will print out a
> > >> spurious interrupt message.
> > 
> > I meant "spurious response"
> 
> So your patch alone isn't enough?
> 
> > >> That said, my hardware is switching to single_cmd eventually, even if not
> > >> passed as a module option. But at least now, when that happens my audio
> > >> isn't dead.
> > > 
> > > It's not dead but living-dead :)
> > > The single_cmd mode is really only for debugging.  It's never for any
> > > running system.
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > 
> > 
> > 
> > I'm all in favor of totally yanking out all the single_cmd related code,
> > but if you're leaving it in, you should apply this patch.
> 
> Well, it's no right fix in a strict manner.
> The right fix would be to remove the reinitialization or the first
> call of azx_init_cmd_io() (and disable the unsolicited events, too).

... like the patch below (untested!)


Takashi

---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 55c7da3..cce837a 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -722,9 +722,10 @@ static unsigned int azx_rirb_get_response(struct hda_bus *bus,
 		   chip->last_cmd[addr]);
 	chip->single_cmd = 1;
 	bus->response_reset = 0;
-	/* re-initialize CORB/RIRB */
+	/* release CORB/RIRB */
 	azx_free_cmd_io(chip);
-	azx_init_cmd_io(chip);
+	/* disable unsolicited responses */
+	azx_writel(chip, GCTL, azx_readl(chip, GCTL) & ~ICH6_GCTL_UNSOL);
 	return -1;
 }
 
@@ -865,7 +866,9 @@ static int azx_reset(struct azx *chip)
 	}
 
 	/* Accept unsolicited responses */
-	azx_writel(chip, GCTL, azx_readl(chip, GCTL) | ICH6_GCTL_UNSOL);
+	if (!chip->single_cmd)
+		azx_writel(chip, GCTL, azx_readl(chip, GCTL) |
+			   ICH6_GCTL_UNSOL);
 
 	/* detect codecs */
 	if (!chip->codec_mask) {
@@ -980,7 +983,8 @@ static void azx_init_chip(struct azx *chip)
 	azx_int_enable(chip);
 
 	/* initialize the codec command I/O */
-	azx_init_cmd_io(chip);
+	if (!chip->single_cmd)
+		azx_init_cmd_io(chip);
 
 	/* program the position buffer */
 	azx_writel(chip, DPLBASE, (u32)chip->posbuf.addr);

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Troy Kisky]

Takashi Iwai wrote:
> At Fri, 06 Nov 2009 08:19:08 +0100,
> I wrote:
>> At Thu, 05 Nov 2009 12:42:03 -0700,
>> Troy Kisky wrote:
>>> Takashi Iwai wrote:
>>>> At Wed, 04 Nov 2009 12:45:20 -0700,
>>>> Troy Kisky wrote:
>>>>> Takashi Iwai wrote:
>>>>>> At Tue,  3 Nov 2009 12:22:37 -0700,
>>>>>> Troy Kisky wrote:
>>>>>>> Poulsbo(US15W) cannot have any corb registers initialized
>>>>>>> when using single_cmd mode.
>>>>>>> When send_cmd timeout occur, note error.
>>>>>> Could you be more specific?  What errors do you get?
>>>>>>
>>>>>> And, how it goes to single_cmd mode?  The single_cmd mode is very last
>>>>>> resort, and reaching there means already a serious problem.
>>>>>>
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Takashi
>>>>>>
>>>>> No error messages, but the response read is always 0.
>>>> This is odd.  Something is already wrong, then.
>>>>
>>>>> For testing, I passed single_cmd=1 as a modules option.
>>>>>
>>>>> HDAudio_03.pdf says, "If implemented, these registers must not be used
>>>>> at the same time as the CORB and RIRB command/response mechanisms, as the operations
>>>>> will conflict."
>>>> I know this, but actually this never be a problem in the real
>>>> hardware.  So, it's left there.
>>> My hardware is real.
>> Yeah, the first hit it seems.
>>
>>>>> Plus, if the RIRB irq is enabled, the interrupt routine will print out a
>>>>> spurious interrupt message.
>>> I meant "spurious response"
>> So your patch alone isn't enough?
>>
>>>>> That said, my hardware is switching to single_cmd eventually, even if not
>>>>> passed as a module option. But at least now, when that happens my audio
>>>>> isn't dead.
>>>> It's not dead but living-dead :)
>>>> The single_cmd mode is really only for debugging.  It's never for any
>>>> running system.
>>>>
>>>>
>>>> thanks,
>>>>
>>>> Takashi
>>>
>>>
>>> I'm all in favor of totally yanking out all the single_cmd related code,
>>> but if you're leaving it in, you should apply this patch.
>> Well, it's no right fix in a strict manner.
>> The right fix would be to remove the reinitialization or the first
>> call of azx_init_cmd_io() (and disable the unsolicited events, too).
> 
> ... like the patch below (untested!)
> 
> 
> Takashi
> 
> ---
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 55c7da3..cce837a 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -722,9 +722,10 @@ static unsigned int azx_rirb_get_response(struct hda_bus *bus,
>  		   chip->last_cmd[addr]);
>  	chip->single_cmd = 1;
>  	bus->response_reset = 0;
> -	/* re-initialize CORB/RIRB */
> +	/* release CORB/RIRB */
>  	azx_free_cmd_io(chip);
> -	azx_init_cmd_io(chip);
> +	/* disable unsolicited responses */
> +	azx_writel(chip, GCTL, azx_readl(chip, GCTL) & ~ICH6_GCTL_UNSOL);
>  	return -1;
>  }
>  
> @@ -865,7 +866,9 @@ static int azx_reset(struct azx *chip)
>  	}
>  
>  	/* Accept unsolicited responses */
> -	azx_writel(chip, GCTL, azx_readl(chip, GCTL) | ICH6_GCTL_UNSOL);
> +	if (!chip->single_cmd)
> +		azx_writel(chip, GCTL, azx_readl(chip, GCTL) |
> +			   ICH6_GCTL_UNSOL);
>  
>  	/* detect codecs */
>  	if (!chip->codec_mask) {
> @@ -980,7 +983,8 @@ static void azx_init_chip(struct azx *chip)
>  	azx_int_enable(chip);
>  
>  	/* initialize the codec command I/O */
> -	azx_init_cmd_io(chip);
> +	if (!chip->single_cmd)
> +		azx_init_cmd_io(chip);
>  
>  	/* program the position buffer */
>  	azx_writel(chip, DPLBASE, (u32)chip->posbuf.addr);
> _______________________________________________


This patch works fine for me when passing single_cmd=1.

Thanks for spending your time on something you'd rather not.

Troy

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Takashi Iwai]

At Fri, 06 Nov 2009 18:01:27 -0700,
Troy Kisky wrote:
> 
> Takashi Iwai wrote:
> > At Fri, 06 Nov 2009 08:19:08 +0100,
> > I wrote:
> >> At Thu, 05 Nov 2009 12:42:03 -0700,
> >> Troy Kisky wrote:
> >>> Takashi Iwai wrote:
> >>>> At Wed, 04 Nov 2009 12:45:20 -0700,
> >>>> Troy Kisky wrote:
> >>>>> Takashi Iwai wrote:
> >>>>>> At Tue,  3 Nov 2009 12:22:37 -0700,
> >>>>>> Troy Kisky wrote:
> >>>>>>> Poulsbo(US15W) cannot have any corb registers initialized
> >>>>>>> when using single_cmd mode.
> >>>>>>> When send_cmd timeout occur, note error.
> >>>>>> Could you be more specific?  What errors do you get?
> >>>>>>
> >>>>>> And, how it goes to single_cmd mode?  The single_cmd mode is very last
> >>>>>> resort, and reaching there means already a serious problem.
> >>>>>>
> >>>>>>
> >>>>>> thanks,
> >>>>>>
> >>>>>> Takashi
> >>>>>>
> >>>>> No error messages, but the response read is always 0.
> >>>> This is odd.  Something is already wrong, then.
> >>>>
> >>>>> For testing, I passed single_cmd=1 as a modules option.
> >>>>>
> >>>>> HDAudio_03.pdf says, "If implemented, these registers must not be used
> >>>>> at the same time as the CORB and RIRB command/response mechanisms, as the operations
> >>>>> will conflict."
> >>>> I know this, but actually this never be a problem in the real
> >>>> hardware.  So, it's left there.
> >>> My hardware is real.
> >> Yeah, the first hit it seems.
> >>
> >>>>> Plus, if the RIRB irq is enabled, the interrupt routine will print out a
> >>>>> spurious interrupt message.
> >>> I meant "spurious response"
> >> So your patch alone isn't enough?
> >>
> >>>>> That said, my hardware is switching to single_cmd eventually, even if not
> >>>>> passed as a module option. But at least now, when that happens my audio
> >>>>> isn't dead.
> >>>> It's not dead but living-dead :)
> >>>> The single_cmd mode is really only for debugging.  It's never for any
> >>>> running system.
> >>>>
> >>>>
> >>>> thanks,
> >>>>
> >>>> Takashi
> >>>
> >>>
> >>> I'm all in favor of totally yanking out all the single_cmd related code,
> >>> but if you're leaving it in, you should apply this patch.
> >> Well, it's no right fix in a strict manner.
> >> The right fix would be to remove the reinitialization or the first
> >> call of azx_init_cmd_io() (and disable the unsolicited events, too).
> > 
> > ... like the patch below (untested!)
> > 
> > 
> > Takashi
> > 
> > ---
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 55c7da3..cce837a 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -722,9 +722,10 @@ static unsigned int azx_rirb_get_response(struct hda_bus *bus,
> >  		   chip->last_cmd[addr]);
> >  	chip->single_cmd = 1;
> >  	bus->response_reset = 0;
> > -	/* re-initialize CORB/RIRB */
> > +	/* release CORB/RIRB */
> >  	azx_free_cmd_io(chip);
> > -	azx_init_cmd_io(chip);
> > +	/* disable unsolicited responses */
> > +	azx_writel(chip, GCTL, azx_readl(chip, GCTL) & ~ICH6_GCTL_UNSOL);
> >  	return -1;
> >  }
> >  
> > @@ -865,7 +866,9 @@ static int azx_reset(struct azx *chip)
> >  	}
> >  
> >  	/* Accept unsolicited responses */
> > -	azx_writel(chip, GCTL, azx_readl(chip, GCTL) | ICH6_GCTL_UNSOL);
> > +	if (!chip->single_cmd)
> > +		azx_writel(chip, GCTL, azx_readl(chip, GCTL) |
> > +			   ICH6_GCTL_UNSOL);
> >  
> >  	/* detect codecs */
> >  	if (!chip->codec_mask) {
> > @@ -980,7 +983,8 @@ static void azx_init_chip(struct azx *chip)
> >  	azx_int_enable(chip);
> >  
> >  	/* initialize the codec command I/O */
> > -	azx_init_cmd_io(chip);
> > +	if (!chip->single_cmd)
> > +		azx_init_cmd_io(chip);
> >  
> >  	/* program the position buffer */
> >  	azx_writel(chip, DPLBASE, (u32)chip->posbuf.addr);
> > _______________________________________________
> 
> 
> This patch works fine for me when passing single_cmd=1.

Thanks for a quick test!  I'll merge it soon.

> Thanks for spending your time on something you'd rather not.

Well, I'm willing to fix any bugs, of course ;)  So I really
appreciated your report and patch.

But, more important bug still remains -- why single_cmd mode is
activated.  Let's track it down.


thanks,

Takashi

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Troy Kisky]

Takashi Iwai wrote:
> 
> Thanks for a quick test!  I'll merge it soon.
> 
>> Thanks for spending your time on something you'd rather not.
> 
> Well, I'm willing to fix any bugs, of course ;)  So I really
> appreciated your report and patch.
> 
> But, more important bug still remains -- why single_cmd mode is
> activated.  Let's track it down.
> 
I have a little more information on that. The RIRB engine is fine
but the CORB engine is stopping at seemingly random times. Usually
CORB has about 7 commands queued when it switches to single_cmd mode.
After the engine dies, even a rmmod/insmod sequence won't revive it.
It seems to require a power down/up. I have noticed a couple of things
that I thought might be related, but testing didn't show much difference.

1. The Poulsbo manual says that CORB READ Pointer Reset must be cleared
back to 0 and read back as 0 to verify that the clear completed correctly.

2. azx_corb_send_cmd doesn't compare CORBWP with CORBRP to see if adding
an entry will result in an empty queue.

Do you have a suggestion on a more thorough reset? I hate having to reboot
all the time.

Thanks
Troy

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Takashi Iwai]

At Mon, 09 Nov 2009 12:36:31 -0700,
Troy Kisky wrote:
> 
> Takashi Iwai wrote:
> > 
> > Thanks for a quick test!  I'll merge it soon.
> > 
> >> Thanks for spending your time on something you'd rather not.
> > 
> > Well, I'm willing to fix any bugs, of course ;)  So I really
> > appreciated your report and patch.
> > 
> > But, more important bug still remains -- why single_cmd mode is
> > activated.  Let's track it down.
> > 
> I have a little more information on that. The RIRB engine is fine
> but the CORB engine is stopping at seemingly random times. Usually
> CORB has about 7 commands queued when it switches to single_cmd mode.
> After the engine dies, even a rmmod/insmod sequence won't revive it.
> It seems to require a power down/up. I have noticed a couple of things
> that I thought might be related, but testing didn't show much difference.
> 
> 1. The Poulsbo manual says that CORB READ Pointer Reset must be cleared
> back to 0 and read back as 0 to verify that the clear completed correctly.

At which timing?

> 2. azx_corb_send_cmd doesn't compare CORBWP with CORBRP to see if adding
> an entry will result in an empty queue.

It's because azx_corb_send_cmd is asynchronous.  It doesn't wait.
That's the purpose of CORB being a ring buffer...

> Do you have a suggestion on a more thorough reset? I hate having to reboot
> all the time.

Try to set codec->bus->sync_write = 1 somewhere in the initialization.
This will make the driver to wait and synchronize for each verb
response.


thanks,

Takashi

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Troy Kisky]

Takashi Iwai wrote:
> At Mon, 09 Nov 2009 12:36:31 -0700,
> Troy Kisky wrote:
>> Takashi Iwai wrote:
>>> Thanks for a quick test!  I'll merge it soon.
>>>
>>>> Thanks for spending your time on something you'd rather not.
>>> Well, I'm willing to fix any bugs, of course ;)  So I really
>>> appreciated your report and patch.
>>>
>>> But, more important bug still remains -- why single_cmd mode is
>>> activated.  Let's track it down.
>>>
>> I have a little more information on that. The RIRB engine is fine
>> but the CORB engine is stopping at seemingly random times. Usually
>> CORB has about 7 commands queued when it switches to single_cmd mode.
>> After the engine dies, even a rmmod/insmod sequence won't revive it.
>> It seems to require a power down/up. I have noticed a couple of things
>> that I thought might be related, but testing didn't show much difference.
>>
>> 1. The Poulsbo manual says that CORB READ Pointer Reset must be cleared
>> back to 0 and read back as 0 to verify that the clear completed correctly.
> 
> At which timing?

The manual was talking about the CORB read pointer reset sequence. It says
to set the bit, verify set, clear the bit, verify clear. Whereas the
normal hda spec says to just set it and forget it, which is what the code does.

> 
>> 2. azx_corb_send_cmd doesn't compare CORBWP with CORBRP to see if adding
>> an entry will result in an empty queue.
> 
> It's because azx_corb_send_cmd is asynchronous.  It doesn't wait.
> That's the purpose of CORB being a ring buffer...

Still, the circular buffer is a fixed size (256 entries in this case).
The code does not detect if you try to stuff 257 entries into the queue.
Actually, 255 entries is the max you should stuff. The next entry being
stuffed will make the read pointer equal the write pointer and the queue
will be empty. I'm not saying this is likely to ever happen. But if it did
happen, you would end up in single_cmd mode.

> 
>> Do you have a suggestion on a more thorough reset? I hate having to reboot
>> all the time.
> 
> Try to set codec->bus->sync_write = 1 somewhere in the initialization.
> This will make the driver to wait and synchronize for each verb
> response.

OK. I'll give a try.

Thanks
Troy

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Takashi Iwai]

At Tue, 10 Nov 2009 14:15:27 -0700,
Troy Kisky wrote:
> 
> Takashi Iwai wrote:
> > At Mon, 09 Nov 2009 12:36:31 -0700,
> > Troy Kisky wrote:
> >> Takashi Iwai wrote:
> >>> Thanks for a quick test!  I'll merge it soon.
> >>>
> >>>> Thanks for spending your time on something you'd rather not.
> >>> Well, I'm willing to fix any bugs, of course ;)  So I really
> >>> appreciated your report and patch.
> >>>
> >>> But, more important bug still remains -- why single_cmd mode is
> >>> activated.  Let's track it down.
> >>>
> >> I have a little more information on that. The RIRB engine is fine
> >> but the CORB engine is stopping at seemingly random times. Usually
> >> CORB has about 7 commands queued when it switches to single_cmd mode.
> >> After the engine dies, even a rmmod/insmod sequence won't revive it.
> >> It seems to require a power down/up. I have noticed a couple of things
> >> that I thought might be related, but testing didn't show much difference.
> >>
> >> 1. The Poulsbo manual says that CORB READ Pointer Reset must be cleared
> >> back to 0 and read back as 0 to verify that the clear completed correctly.
> > 
> > At which timing?
> 
> The manual was talking about the CORB read pointer reset sequence. It says
> to set the bit, verify set, clear the bit, verify clear. Whereas the
> normal hda spec says to just set it and forget it, which is what the code does.

Hm, CORB read pointer might play actually something for your device,
then.

> >> 2. azx_corb_send_cmd doesn't compare CORBWP with CORBRP to see if adding
> >> an entry will result in an empty queue.
> > 
> > It's because azx_corb_send_cmd is asynchronous.  It doesn't wait.
> > That's the purpose of CORB being a ring buffer...
> 
> Still, the circular buffer is a fixed size (256 entries in this case).
> The code does not detect if you try to stuff 257 entries into the queue.
> Actually, 255 entries is the max you should stuff. The next entry being
> stuffed will make the read pointer equal the write pointer and the queue
> will be empty. I'm not saying this is likely to ever happen. But if it did
> happen, you would end up in single_cmd mode.

Yes, in theory.  But this doesn't happen actually because you usually
read the response often.  The read sequence synchronizes.


thanks,

Takashi

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Troy Kisky]

Takashi Iwai wrote:
> Yes, in theory.  But this doesn't happen actually because you usually
> read the response often.  The read sequence synchronizes.
> 
> 
Can resume type processing send a lot of verbs without waiting for a response?

Speaking of synchronizing, if 2 different processes simultaneously send
different command verbs to the same codec, won't they both read the response
of the last verb sent? Or is there locking somewhere to prevent this?


Troy

Re: [PATCH] ALSA: hda_intel: disable corb rirb when single_cmd active

[Takashi Iwai]

At Wed, 11 Nov 2009 12:05:39 -0700,
Troy Kisky wrote:
> 
> Takashi Iwai wrote:
> > Yes, in theory.  But this doesn't happen actually because you usually
> > read the response often.  The read sequence synchronizes.
> > 
> > 
> Can resume type processing send a lot of verbs without waiting for a response?

Usually not many like 256, I guess.  In doubt, give some printks and
count by yourself...

> Speaking of synchronizing, if 2 different processes simultaneously send
> different command verbs to the same codec, won't they both read the response
> of the last verb sent? Or is there locking somewhere to prevent this?

A mutex protection around the send/response pair.


Takashi