Re: [PATCH 0/5] Pegasos2 fixes and audio output support

[BALATON Zoltan]

On Wed, 22 Feb 2023, Bernhard Beschow wrote:
> On Wed, Feb 22, 2023 at 4:38 PM Bernhard Beschow <shentey@gmail.com> wrote:
> > On Tue, Feb 21, 2023 at 7:44 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > > This series fixes PCI interrupts on the ppc/pegasos2 machine and adds
> > > partial implementation of the via-ac97 sound part enough to get audio
> > > output. I'd like this to be merged for QEMU 8.0.
> > >
> > > Regards,
> > > BALATON Zoltan
> > >
> > > BALATON Zoltan (5):
> > >   hw/isa/vt82c686: Implement interrupt routing in via_isa_set_irq
> > >   hw/isa/vt82c686: Implement PIRQ pins
> > >   hw/ppc/pegasos2: Fix PCI interrupt routing
> > >   hw/audio/ac97: Split off some definitions to a header
> > >   hw/audio/via-ac97: Basic implementation of audio playback
> > >
> > >  hw/audio/ac97.c            |  43 +---
> > >  hw/audio/ac97.h            |  65 ++++++
> > >  hw/audio/trace-events      |   6 +
> > >  hw/audio/via-ac97.c        | 436 ++++++++++++++++++++++++++++++++++++-
> > >  hw/ide/via.c               |   2 +-
> > >  hw/isa/vt82c686.c          |  61 +++++-
> > >  hw/pci-host/mv64361.c      |   4 -
> > >  hw/ppc/pegasos2.c          |  26 ++-
> > >  hw/usb/vt82c686-uhci-pci.c |   5 +-
> > >  include/hw/isa/vt82c686.h  |  39 +++-
> > >  10 files changed, 626 insertions(+), 61 deletions(-)
> > >  create mode 100644 hw/audio/ac97.h
> > >
> > > --
> > > 2.30.7
> > Wow, the MorphOS people paid attention to sound design. Thanks for
> > presenting it to us, Zoltan!
> >
> > I've had a closer look at your series and I think it can be simplified:
> > Patch 2 can be implemented quite straight-forward like I proposed in a
> > private mail: https://github.com/shentok/qemu/commit/via-priq-routing.
> > Then, in order to make patch 3 "hw/ppc/pegasos2: Fix PCI interrupt routing"
> > working, one can expose the PCI interrupts with a single line like you do
> > in patch 2. With this, patch 1 "hw/isa/vt82c686: Implement interrupt
> > routing in via_isa_set_irq" isn't needed any longer and can be omitted.
> >
> > In via-ac97, rather than using via_isa_set_irq(), pci_set_irq() can be
> > used instead. pci_set_irq() internally takes care of all the ISA interrupt
> > level tracking patch 1 attempted to address.
>
> Here is a proof of concept branch to demonstrate that the simplification
> actually works: https://github.com/shentok/qemu/commits/pegasos2 (Tested
> with MorphOS with and without pegasos2.rom).

Does this only work because both the via-ac97 and the PCI interrupts are 
mapped to the same ISA IRQ and you've only tested sound? The guest could 
configure each device to use a different IRQ, also mapping them so they 
share one ISA interrupt. What happens if multiple devices are mapped to 
IRQ 9 (which is the case on pegasos2 where PCI cards, ac97 and USB all 
share this IRQ) and more than one such device wants to raise an interrupt 
at the same time? If you ack the ac97 interrupt but a PCI network card or 
the USB part still wants to get the CPUs attention the ISA IRQ should 
remain raised until all devices are serviced. I don't see a way to track 
the status of all devices in a single qemu_irq which can only be up or 
down so we need something to store the state of each source. My patch adds 
a state register to each ISA IRQ line for all possible sources which could 
probably be stored once but then for each change of ISA IRQ status all the 
mapped devices should be checked and combined so it's easier to store them 
for each IRQ. Does your approach still work if you play sound, and copy 
something from network to a USB device at the same time? (I'm not sure 
mine does not have remaining bugs but I don't think this can be simplified 
that way but if you can prove it would work I don't mind taking an 
alternative version but I'm not convinced yet.)

Regards,
BALATON Zoltan

> > I might have further comments but I think it's enough for now.
> >
> > Thanks again for making via-ac97 work!
> >
> > Best regards,
> > Bernhard