bug: audiohda broken in qemu

bug: audiohda broken in qemu

[Nick Owens]

there have been multiple reports of audio not working in qemu.

https://9fans.topicbox.com/groups/9fans/T3f4bbef90063aae9-Maeb867180b0aa1fbf97ea7f7
https://www.reddit.com/r/plan9/comments/jipk3b/plan_9_qemu_and_audio/

indeed i tested it and it does not work, so i poked around in qemu:

qemu-system-x86_64 -m 512 -enable-kvm -device intel-hda,debug=2
-device hda-duplex,debug=2 -kernel /tmp/9pc64 -initrd /tmp/plan9.ini
-serial mon:stdio -vnc :1

in enumcodec() we try to get the vendor id:

cmderr(id, Getparm, Vendorid, &vid)

but that fails:

intel-hda: read  RIRBWP          : 0x0 (ffff)
intel-hda: read  CORBRP          : 0x0 (ffff)
intel-hda: read  CORBWP          : 0x0 (ffff)
intel-hda: write CORBWP          : 0x1 (ffff)
intel-hda: intel_hda_corb_run: rirb count reached
intel-hda: read  RIRBWP          : 0x0 (ffff)
#A0: no audio codecs found

i looked a bit at this 'rirb count reached' message and surmised that
we needed to change RINTCNT. i looked at the linux driver and they set
it to 1, and to 0xC0 for some 'quirky' cards. i tried 1 but that just
makes subsequent commands after Getparam/Vendorid fail, but setting it
to 0xC0 made all commands work and the device is enumerated, and
#A/audio works.

diff --git a/sys/src/9/pc/audiohda.c b/sys/src/9/pc/audiohda.c
--- a/sys/src/9/pc/audiohda.c
+++ b/sys/src/9/pc/audiohda.c
@@ -1732,6 +1732,7 @@ hdastart(Ctlr *ctlr)
     csr32(ctlr, Rirblbase) = pa;
     csr32(ctlr, Rirbubase) = pa >> 32;
     csr16(ctlr, Rirbwp) = Rirbptrrst;
+    csr16(ctlr, Rintcnt) = 0xC0;
     csr8(ctlr, Rirbctl) = Rirbdma;
     waitup8(ctlr, Rirbctl, Rirbdma, Rirbdma);

note that i have no idea what i am doing.

Re: bug: audiohda broken in qemu

[Anthony Martin]

Nick Owens <mischief@offblast.org> once said:
> i looked a bit at this 'rirb count reached' message and surmised that
> we needed to change RINTCNT. i looked at the linux driver and they set
> it to 1, and to 0xC0 for some 'quirky' cards. i tried 1 but that just
> makes subsequent commands after Getparam/Vendorid fail, but setting it
> to 0xC0 made all commands work and the device is enumerated, and
> #A/audio works.
>
> diff --git a/sys/src/9/pc/audiohda.c b/sys/src/9/pc/audiohda.c
> --- a/sys/src/9/pc/audiohda.c
> +++ b/sys/src/9/pc/audiohda.c
> @@ -1732,6 +1732,7 @@ hdastart(Ctlr *ctlr)
>      csr32(ctlr, Rirblbase) = pa;
>      csr32(ctlr, Rirbubase) = pa >> 32;
>      csr16(ctlr, Rirbwp) = Rirbptrrst;
> +    csr16(ctlr, Rintcnt) = 0xC0;
>      csr8(ctlr, Rirbctl) = Rirbdma;
>      waitup8(ctlr, Rirbctl, Rirbdma, Rirbdma);
>
> note that i have no idea what i am doing.

That's weird. There are two ways to figure out if there are responses
in the response input ring buffer (RIRB): either enable interrupts or
poll the write pointer. We do the latter and set the Rirbint field to
zero in the Rirbctl register. The value of Rintcnt should not matter
at all if RIRB interrupts are disabled.

Which method are they using in Linux?

  Anthony

Re: bug: audiohda broken in qemu

[Anthony Martin]

When the driver sets the CORB write pointer, QEMU doesn't check to see
if RIRB interrupts are enabled before checking the interrupt count and
returning early from it's CORB loop:

	https://github.com/qemu/qemu/blob/master/hw/audio/intel-hda.c#L331

This isn't compatible with drivers that poll the write pointer.

Interestingly, that code in QEMU hasn't changed since it was added
in 2010.

  Anthony

Re: [9front] Re: bug: audiohda broken in qemu

[Nick Owens]

you are in a maze of twisty little passages, all alike.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/hda

it looks like snd_hdac_bus_get_response() in hdac_controller.c
conditionally supports polling.

On Tue, Oct 27, 2020 at 1:17 AM Anthony Martin <ality@pbrane.org> wrote:
>
> Nick Owens <mischief@offblast.org> once said:
> > i looked a bit at this 'rirb count reached' message and surmised that
> > we needed to change RINTCNT. i looked at the linux driver and they set
> > it to 1, and to 0xC0 for some 'quirky' cards. i tried 1 but that just
> > makes subsequent commands after Getparam/Vendorid fail, but setting it
> > to 0xC0 made all commands work and the device is enumerated, and
> > #A/audio works.
> >
> > diff --git a/sys/src/9/pc/audiohda.c b/sys/src/9/pc/audiohda.c
> > --- a/sys/src/9/pc/audiohda.c
> > +++ b/sys/src/9/pc/audiohda.c
> > @@ -1732,6 +1732,7 @@ hdastart(Ctlr *ctlr)
> >      csr32(ctlr, Rirblbase) = pa;
> >      csr32(ctlr, Rirbubase) = pa >> 32;
> >      csr16(ctlr, Rirbwp) = Rirbptrrst;
> > +    csr16(ctlr, Rintcnt) = 0xC0;
> >      csr8(ctlr, Rirbctl) = Rirbdma;
> >      waitup8(ctlr, Rirbctl, Rirbdma, Rirbdma);
> >
> > note that i have no idea what i am doing.
>
> That's weird. There are two ways to figure out if there are responses
> in the response input ring buffer (RIRB): either enable interrupts or
> poll the write pointer. We do the latter and set the Rirbint field to
> zero in the Rirbctl register. The value of Rintcnt should not matter
> at all if RIRB interrupts are disabled.
>
> Which method are they using in Linux?
>
>   Anthony

Re: [9front] bug: audiohda broken in qemu

[cinap_lenrek]

very odd, because we dont even use IRB interrupts at all.
in hdacmd(), you can see we just poll the irb write
pointer instead...

note also that the irb ring has a variable size, it
can go from 2, 16 to 256 entries. so programming that
register with a out of range value might be a bad idea.

i wonder why it fails with intcnt = 1, it seems the response
counter is never reset in qemu so just bumping the count
will just then make it fail after 0xC0 responses?

maybe you can try running the vendor command in a loop
with your change and see if it starts failing once we
reach the count?

the fix might be that we need to clear the response
interrupt status flag in rirbsts (bit 0) after or
before each command?

--
cinap

Re: [9front] bug: audiohda broken in qemu

[Nick Owens]

 On Tue, Oct 27, 2020 at 2:48 AM <cinap_lenrek@felloff.net> wrote:
>
> very odd, because we dont even use IRB interrupts at all.
> in hdacmd(), you can see we just poll the irb write
> pointer instead...
>
> note also that the irb ring has a variable size, it
> can go from 2, 16 to 256 entries. so programming that
> register with a out of range value might be a bad idea.
>
> i wonder why it fails with intcnt = 1, it seems the response
> counter is never reset in qemu so just bumping the count
> will just then make it fail after 0xC0 responses?
>
> maybe you can try running the vendor command in a loop
> with your change and see if it starts failing once we
> reach the count?

yes, running Getparm in enumcodec() in a loop (0xC0+n times) leads to
driver probing failure:

intel-hda: read  CORBWP          : 0xbf (ffff)
intel-hda: write CORBWP          : 0xc0 (ffff)
intel-hda: intel_hda_corb_run: [rp 0xc0] verb 0x000f0000
hda-duplex: hda_audio_command: nid 0 (root), verb 0xf00, payload 0x0
intel-hda: intel_hda_response: [wp 0xc0] response 0x1af40022, extra
0x0
intel-hda: intel_hda_response: rirb count reached (192)
intel-hda: intel_hda_corb_run: corb ring empty
intel-hda: read  RIRBWP          : 0xc0 (ffff)
intel-hda: previous register op repeated 1 times
intel-hda: read  CORBRP          : 0xc0 (ffff)
intel-hda: read  CORBWP          : 0xc0 (ffff)
intel-hda: write CORBWP          : 0xc1 (ffff)
intel-hda: intel_hda_corb_run: rirb count reached
intel-hda: read  RIRBWP          : 0xc0 (ffff)
#A0: no audio codecs found

>
> the fix might be that we need to clear the response
> interrupt status flag in rirbsts (bit 0) after or
> before each command?

FWICT qemu only resets the RIRB counter *if* you had RIRB interrupts
on previously, and we arent using RIRB interrupts:

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/audio/intel-hda.c;h=4330213fff167071800216c49607c5f18022cc20;hb=HEAD#l555

so what is the solution here? use RIRB interrupts?

>
> --
> cinap

Re: [9front] bug: audiohda broken in qemu

[Nick Owens]

following up on my previous idea, i enabled RIRB interrupts but clear
them in hdacmd, with RINTCNT set to 1. this seems to work.

http://okturing.com/src/9629/body

is there any way to improve this?

On Tue, Oct 27, 2020 at 3:23 AM Nick Owens <mischief@offblast.org> wrote:
>
>  On Tue, Oct 27, 2020 at 2:48 AM <cinap_lenrek@felloff.net> wrote:
> >
> > very odd, because we dont even use IRB interrupts at all.
> > in hdacmd(), you can see we just poll the irb write
> > pointer instead...
> >
> > note also that the irb ring has a variable size, it
> > can go from 2, 16 to 256 entries. so programming that
> > register with a out of range value might be a bad idea.
> >
> > i wonder why it fails with intcnt = 1, it seems the response
> > counter is never reset in qemu so just bumping the count
> > will just then make it fail after 0xC0 responses?
> >
> > maybe you can try running the vendor command in a loop
> > with your change and see if it starts failing once we
> > reach the count?
>
> yes, running Getparm in enumcodec() in a loop (0xC0+n times) leads to
> driver probing failure:
>
> intel-hda: read  CORBWP          : 0xbf (ffff)
> intel-hda: write CORBWP          : 0xc0 (ffff)
> intel-hda: intel_hda_corb_run: [rp 0xc0] verb 0x000f0000
> hda-duplex: hda_audio_command: nid 0 (root), verb 0xf00, payload 0x0
> intel-hda: intel_hda_response: [wp 0xc0] response 0x1af40022, extra
> 0x0
> intel-hda: intel_hda_response: rirb count reached (192)
> intel-hda: intel_hda_corb_run: corb ring empty
> intel-hda: read  RIRBWP          : 0xc0 (ffff)
> intel-hda: previous register op repeated 1 times
> intel-hda: read  CORBRP          : 0xc0 (ffff)
> intel-hda: read  CORBWP          : 0xc0 (ffff)
> intel-hda: write CORBWP          : 0xc1 (ffff)
> intel-hda: intel_hda_corb_run: rirb count reached
> intel-hda: read  RIRBWP          : 0xc0 (ffff)
> #A0: no audio codecs found
>
> >
> > the fix might be that we need to clear the response
> > interrupt status flag in rirbsts (bit 0) after or
> > before each command?
>
> FWICT qemu only resets the RIRB counter *if* you had RIRB interrupts
> on previously, and we arent using RIRB interrupts:
>
> https://git.qemu.org/?p=qemu.git;a=blob;f=hw/audio/intel-hda.c;h=4330213fff167071800216c49607c5f18022cc20;hb=HEAD#l555
>
> so what is the solution here? use RIRB interrupts?
>
> >
> > --
> > cinap

Re: [9front] bug: audiohda broken in qemu

[Nick Owens]

should be fixed in http://code.9front.org/hg/plan9front/rev/0e47f4a0c4d6.

On Tue, Oct 27, 2020 at 3:53 AM Nick Owens <mischief@offblast.org> wrote:
>
> following up on my previous idea, i enabled RIRB interrupts but clear
> them in hdacmd, with RINTCNT set to 1. this seems to work.
>
> http://okturing.com/src/9629/body
>
> is there any way to improve this?
>
> On Tue, Oct 27, 2020 at 3:23 AM Nick Owens <mischief@offblast.org> wrote:
> >
> >  On Tue, Oct 27, 2020 at 2:48 AM <cinap_lenrek@felloff.net> wrote:
> > >
> > > very odd, because we dont even use IRB interrupts at all.
> > > in hdacmd(), you can see we just poll the irb write
> > > pointer instead...
> > >
> > > note also that the irb ring has a variable size, it
> > > can go from 2, 16 to 256 entries. so programming that
> > > register with a out of range value might be a bad idea.
> > >
> > > i wonder why it fails with intcnt = 1, it seems the response
> > > counter is never reset in qemu so just bumping the count
> > > will just then make it fail after 0xC0 responses?
> > >
> > > maybe you can try running the vendor command in a loop
> > > with your change and see if it starts failing once we
> > > reach the count?
> >
> > yes, running Getparm in enumcodec() in a loop (0xC0+n times) leads to
> > driver probing failure:
> >
> > intel-hda: read  CORBWP          : 0xbf (ffff)
> > intel-hda: write CORBWP          : 0xc0 (ffff)
> > intel-hda: intel_hda_corb_run: [rp 0xc0] verb 0x000f0000
> > hda-duplex: hda_audio_command: nid 0 (root), verb 0xf00, payload 0x0
> > intel-hda: intel_hda_response: [wp 0xc0] response 0x1af40022, extra
> > 0x0
> > intel-hda: intel_hda_response: rirb count reached (192)
> > intel-hda: intel_hda_corb_run: corb ring empty
> > intel-hda: read  RIRBWP          : 0xc0 (ffff)
> > intel-hda: previous register op repeated 1 times
> > intel-hda: read  CORBRP          : 0xc0 (ffff)
> > intel-hda: read  CORBWP          : 0xc0 (ffff)
> > intel-hda: write CORBWP          : 0xc1 (ffff)
> > intel-hda: intel_hda_corb_run: rirb count reached
> > intel-hda: read  RIRBWP          : 0xc0 (ffff)
> > #A0: no audio codecs found
> >
> > >
> > > the fix might be that we need to clear the response
> > > interrupt status flag in rirbsts (bit 0) after or
> > > before each command?
> >
> > FWICT qemu only resets the RIRB counter *if* you had RIRB interrupts
> > on previously, and we arent using RIRB interrupts:
> >
> > https://git.qemu.org/?p=qemu.git;a=blob;f=hw/audio/intel-hda.c;h=4330213fff167071800216c49607c5f18022cc20;hb=HEAD#l555
> >
> > so what is the solution here? use RIRB interrupts?
> >
> > >
> > > --
> > > cinap