[6/9] vt82c686: Support machine-default audiodev with fallback

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/isa/vt82c686.c   |  2 ++
 hw/mips/fuloong2e.c | 12 +++++++++---
 hw/ppc/pegasos2.c   | 10 ++++++++--
 hw/ppc/sam460ex.c   |  2 ++
 4 files changed, 21 insertions(+), 5 deletions(-)

On Fri, 22 Sep 2023, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/isa/vt82c686.c   |  2 ++
> hw/mips/fuloong2e.c | 12 +++++++++---
> hw/ppc/pegasos2.c   | 10 ++++++++--
> hw/ppc/sam460ex.c   |  2 ++
> 4 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 57bdfb4e78c..3ec8e43708a 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -578,6 +578,8 @@ static void via_isa_init(Object *obj)
>     object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI);
>     object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
>     object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
> +
> +    object_property_add_alias(obj, "audiodev", OBJECT(&s->ac97), "audiodev");
> }
>
> static const TypeInfo via_isa_info = {
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index c827f615f3b..d0671824f94 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -41,6 +41,7 @@
> #include "sysemu/reset.h"
> #include "sysemu/sysemu.h"
> #include "qemu/error-report.h"
> +#include "audio/audio.h"
>
> #define ENVP_PADDR              0x2000
> #define ENVP_VADDR              cpu_mips_phys_to_kseg0(NULL, ENVP_PADDR)
> @@ -295,9 +296,12 @@ static void mips_fuloong2e_init(MachineState *machine)
>     pci_bus = bonito_init((qemu_irq *)&(env->irq[2]));
>
>     /* South bridge -> IP5 */
> -    pci_dev = pci_create_simple_multifunction(pci_bus,
> -                                              PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
> -                                              TYPE_VT82C686B_ISA);
> +    pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
> +                                    TYPE_VT82C686B_ISA);
> +    qdev_prop_set_string(DEVICE(pci_dev), "audiodev",
> +                         audio_maybe_init_dummy("fuloong2e.defaudio"));
> +    pci_realize_and_unref(pci_dev, pci_bus, &error_abort);
> +
>     object_property_add_alias(OBJECT(machine), "rtc-time",
>                               object_resolve_path_component(OBJECT(pci_dev),
>                                                             "rtc"),
> @@ -337,6 +341,8 @@ static void mips_fuloong2e_machine_init(MachineClass *mc)
>     mc->default_ram_size = 256 * MiB;
>     mc->default_ram_id = "fuloong2e.ram";
>     mc->minimum_page_bits = 14;
> +
> +    machine_add_audiodev_property(mc);
> }
>
> DEFINE_MACHINE("fuloong2e", mips_fuloong2e_machine_init)
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index bd397cf2b5c..45b94cab73a 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -37,6 +37,7 @@
> #include "qemu/datadir.h"
> #include "sysemu/device_tree.h"
> #include "hw/ppc/vof.h"
> +#include "audio/audio.h"
>
> #include <libfdt.h>
>
> @@ -180,8 +181,11 @@ static void pegasos2_init(MachineState *machine)
>     pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS);
>
>     /* VIA VT8231 South Bridge (multifunction PCI device) */
> -    via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0),
> -                                                 TYPE_VT8231_ISA));
> +    via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), TYPE_VT8231_ISA));
> +    qdev_prop_set_string(DEVICE(via), "audiodev",
> +                         audio_maybe_init_dummy("pegasos2.defaudio"));
> +    pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_abort);
> +

Do not add blank line here, the rest is still part of via init.

>     for (i = 0; i < PCI_NUM_PINS; i++) {
>         pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i);
>     }
> @@ -564,6 +568,8 @@ static void pegasos2_machine_class_init(ObjectClass *oc, void *data)
>     vhc->encode_hpt_for_kvm_pr = vhyp_encode_hpt_for_kvm_pr;
>
>     vmc->setprop = pegasos2_setprop;
> +
> +    machine_add_audiodev_property(mc);
> }
>
> static const TypeInfo pegasos2_machine_info = {
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 1e615b8d355..8c3abc67131 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -530,6 +530,8 @@ static void sam460ex_machine_init(MachineClass *mc)
>     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("460exb");
>     mc->default_ram_size = 512 * MiB;
>     mc->default_ram_id = "ppc4xx.sdram";
> +
> +    machine_add_audiodev_property(mc);

This hunk has nothing to do with vt82686 so probably should be in 
previoius patch. Also sam460ex embedded sound part is not emulated and can 
only use PCI sound cards. What this line is needed for? If every machine 
now needs this call, can it be added in some generic machine init func in 
hw/core/machine.c instead?

I'm not sure about this whole series. Looks like it gets rid of 71 lines 
but adding 158 and makes the users' life harder by requireing them to 
specify drivers they may not even know about. What's the point? Is there 
still a default to use the normally used audiodev for the platform or 
people will now get errors and no sound unless they change their command 
lines?

Regards,
BALATON Zoltan

> }
>
> DEFINE_MACHINE("sam460ex", sam460ex_machine_init)
>

On Fri, Sep 22, 2023 at 2:17 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("460exb");
> >     mc->default_ram_size = 512 * MiB;
> >     mc->default_ram_id = "ppc4xx.sdram";
> > +
> > +    machine_add_audiodev_property(mc);
>
> This hunk has nothing to do with vt82686 so probably should be in
> previoius patch. Also sam460ex embedded sound part is not emulated and can
> only use PCI sound cards. What this line is needed for?

No, this line shouldn't be there.

> If every machine
> now needs this call, can it be added in some generic machine init func in
> hw/core/machine.c instead?

It is not needed by every machine, only by every machine that has
embedded sound.

> I'm not sure about this whole series. Looks like it gets rid of 71 lines
> but adding 158 and makes the users' life harder by requireing them to
> specify drivers they may not even know about. What's the point? Is there
> still a default to use the normally used audiodev for the platform or
> people will now get errors and no sound unless they change their command
> lines?

I think you're right, I should have sent this series without the last
two patches.

The first seven add more functionality, because they let you use
-audiodev for configuration of embedded boards. This is why they add
some lines of code.

The last two patches instead are the ones that make -audio or
-audiodev mandatory. They should be separate and they may not be a
good idea without something like "-audio default". And if no "-audio
default" is added, there is more code that can go (for example the
--audio-drv-list option to configure and CONFIG_AUDIO_DRIVERS).

Paolo

On Fri, 22 Sep 2023, Paolo Bonzini wrote:
> On Fri, Sep 22, 2023 at 2:17 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("460exb");
>>>     mc->default_ram_size = 512 * MiB;
>>>     mc->default_ram_id = "ppc4xx.sdram";
>>> +
>>> +    machine_add_audiodev_property(mc);
>>
>> This hunk has nothing to do with vt82686 so probably should be in
>> previoius patch. Also sam460ex embedded sound part is not emulated and can
>> only use PCI sound cards. What this line is needed for?
>
> No, this line shouldn't be there.
>
>> If every machine
>> now needs this call, can it be added in some generic machine init func in
>> hw/core/machine.c instead?
>
> It is not needed by every machine, only by every machine that has
> embedded sound.
>
>> I'm not sure about this whole series. Looks like it gets rid of 71 lines
>> but adding 158 and makes the users' life harder by requireing them to
>> specify drivers they may not even know about. What's the point? Is there
>> still a default to use the normally used audiodev for the platform or
>> people will now get errors and no sound unless they change their command
>> lines?
>
> I think you're right, I should have sent this series without the last
> two patches.
>
> The first seven add more functionality, because they let you use
> -audiodev for configuration of embedded boards. This is why they add
> some lines of code.
>
> The last two patches instead are the ones that make -audio or
> -audiodev mandatory. They should be separate and they may not be a
> good idea without something like "-audio default". And if no "-audio
> default" is added, there is more code that can go (for example the
> --audio-drv-list option to configure and CONFIG_AUDIO_DRIVERS).

I still don't see the point, because it already works without these 
changes. With current master one can specify -audiodev for -M paegasos2 
and it gives a warning but does the right thing and sets the audiodev for 
via-ac97. I think the warning can be avoided by using -global to set the 
via-ac97 audiodev property but since it picks up the -audiodev there's no 
need to. Apart from the warning this is convenient for the user, what's 
proposed in this series seems less so. What is the issue this series tries 
to solve?

Regards,
BALATON Zoltan