[PATCH v3 00/19] audio: deprecate -soundhw
v2: - use g_assert_not_reached() for stubs. - add deprecation notice. v3: - rebase to latest master. - adapt to armbru's device initialization changes. Gerd Hoffmann (19): stubs: add isa_create_simple stubs: add pci_create_simple audio: add deprecated_register_soundhw audio: deprecate -soundhw ac97 audio: deprecate -soundhw es1370 audio: deprecate -soundhw adlib audio: deprecate -soundhw cs4231a audio: deprecate -soundhw gus audio: deprecate -soundhw sb16 audio: deprecate -soundhw hda audio: deprecate -soundhw pcspk audio: add soundhw deprecation notice pc_basic_device_init: pass PCMachineState pc_basic_device_init: drop has_pit arg pc_basic_device_init: drop no_vmport arg softmmu: initialize spice and audio earlier audio: rework pcspk_init() audio: create pcspk device early, add audiodev alias. audio: set default value for pcspk.iobase property include/hw/audio/pcspk.h | 12 ++---------- include/hw/audio/soundhw.h | 2 ++ include/hw/i386/pc.h | 6 +++--- hw/audio/ac97.c | 9 ++------- hw/audio/adlib.c | 8 +------- hw/audio/cs4231a.c | 8 +------- hw/audio/es1370.c | 9 ++------- hw/audio/gus.c | 8 +------- hw/audio/intel-hda.c | 3 +++ hw/audio/pcspk.c | 26 ++++++++++++++++++++++---- hw/audio/sb16.c | 9 ++------- hw/audio/soundhw.c | 24 +++++++++++++++++++++++- hw/i386/pc.c | 14 ++++++++------ hw/i386/pc_piix.c | 3 +-- hw/i386/pc_q35.c | 3 +-- hw/isa/i82378.c | 2 +- qdev-monitor.c | 2 ++ softmmu/vl.c | 14 ++++++++------ stubs/isa-bus.c | 7 +++++++ stubs/pci-bus.c | 7 +++++++ docs/system/deprecated.rst | 9 +++++++++ stubs/Makefile.objs | 2 ++ 22 files changed, 110 insertions(+), 77 deletions(-) create mode 100644 stubs/isa-bus.c create mode 100644 stubs/pci-bus.c -- 2.18.4
Needed for -soundhw cleanup. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- stubs/isa-bus.c | 7 +++++++ stubs/Makefile.objs | 1 + 2 files changed, 8 insertions(+) create mode 100644 stubs/isa-bus.c diff --git a/stubs/isa-bus.c b/stubs/isa-bus.c new file mode 100644 index 000000000000..522f448997d4 --- /dev/null +++ b/stubs/isa-bus.c @@ -0,0 +1,7 @@ +#include "qemu/osdep.h" +#include "hw/isa/isa.h" + +ISADevice *isa_create_simple(ISABus *bus, const char *name) +{ + g_assert_not_reached(); +} diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index 28e48171d1f3..869204668231 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -7,6 +7,7 @@ stub-obj-y += fdset.o stub-obj-y += gdbstub.o stub-obj-y += iothread-lock.o stub-obj-y += is-daemonized.o +stub-obj-y += isa-bus.o stub-obj-$(CONFIG_LINUX_AIO) += linux-aio.o stub-obj-$(CONFIG_LINUX_IO_URING) += io_uring.o stub-obj-y += monitor-core.o -- 2.18.4
Needed for -soundhw cleanup. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- stubs/pci-bus.c | 7 +++++++ stubs/Makefile.objs | 1 + 2 files changed, 8 insertions(+) create mode 100644 stubs/pci-bus.c diff --git a/stubs/pci-bus.c b/stubs/pci-bus.c new file mode 100644 index 000000000000..a8932fa93250 --- /dev/null +++ b/stubs/pci-bus.c @@ -0,0 +1,7 @@ +#include "qemu/osdep.h" +#include "hw/pci/pci.h" + +PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name) +{ + g_assert_not_reached(); +} diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index 869204668231..5d61e0d0da5b 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -12,6 +12,7 @@ stub-obj-$(CONFIG_LINUX_AIO) += linux-aio.o stub-obj-$(CONFIG_LINUX_IO_URING) += io_uring.o stub-obj-y += monitor-core.o stub-obj-y += notify-event.o +stub-obj-y += pci-bus.o stub-obj-y += qmp_memory_device.o stub-obj-y += qtest.o stub-obj-y += ramfb.o -- 2.18.4
Add helper function for -soundhw deprecation. It can replace the simple init functions which just call {isa,pci}_create_simple() with a hardcoded type. It also prints a deprecation message. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/hw/audio/soundhw.h | 2 ++ hw/audio/soundhw.c | 24 +++++++++++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/include/hw/audio/soundhw.h b/include/hw/audio/soundhw.h index c8eef8241846..f09a297854af 100644 --- a/include/hw/audio/soundhw.h +++ b/include/hw/audio/soundhw.h @@ -6,6 +6,8 @@ void isa_register_soundhw(const char *name, const char *descr, void pci_register_soundhw(const char *name, const char *descr, int (*init_pci)(PCIBus *bus)); +void deprecated_register_soundhw(const char *name, const char *descr, + int isa, const char *typename); void soundhw_init(void); void select_soundhw(const char *optarg); diff --git a/hw/audio/soundhw.c b/hw/audio/soundhw.c index c750473c8f0c..173b674ff53a 100644 --- a/hw/audio/soundhw.c +++ b/hw/audio/soundhw.c @@ -22,6 +22,7 @@ * THE SOFTWARE. */ #include "qemu/osdep.h" +#include "qemu/option.h" #include "qemu/help_option.h" #include "qemu/error-report.h" #include "qom/object.h" @@ -32,6 +33,7 @@ struct soundhw { const char *name; const char *descr; + const char *typename; int enabled; int isa; union { @@ -65,6 +67,17 @@ void pci_register_soundhw(const char *name, const char *descr, soundhw_count++; } +void deprecated_register_soundhw(const char *name, const char *descr, + int isa, const char *typename) +{ + assert(soundhw_count < ARRAY_SIZE(soundhw) - 1); + soundhw[soundhw_count].name = name; + soundhw[soundhw_count].descr = descr; + soundhw[soundhw_count].isa = isa; + soundhw[soundhw_count].typename = typename; + soundhw_count++; +} + void select_soundhw(const char *optarg) { struct soundhw *c; @@ -136,7 +149,16 @@ void soundhw_init(void) for (c = soundhw; c->name; ++c) { if (c->enabled) { - if (c->isa) { + if (c->typename) { + warn_report("'-soundhw %s' is deprecated, " + "please use '-device %s' instead", + c->name, c->typename); + if (c->isa) { + isa_create_simple(isa_bus, c->typename); + } else { + pci_create_simple(pci_bus, -1, c->typename); + } + } else if (c->isa) { if (!isa_bus) { error_report("ISA bus not available for %s", c->name); exit(1); -- 2.18.4
Switch to deprecated_register_soundhw(). Remove the now obsolete init function. Add an alias so both ac97 and AC97 are working with -device. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/ac97.c | 9 ++------- qdev-monitor.c | 1 + 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/hw/audio/ac97.c b/hw/audio/ac97.c index 8a9b9924c495..38522cf0ba44 100644 --- a/hw/audio/ac97.c +++ b/hw/audio/ac97.c @@ -1393,12 +1393,6 @@ static void ac97_exit(PCIDevice *dev) AUD_remove_card(&s->card); } -static int ac97_init (PCIBus *bus) -{ - pci_create_simple(bus, -1, TYPE_AC97); - return 0; -} - static Property ac97_properties[] = { DEFINE_AUDIO_PROPERTIES(AC97LinkState, card), DEFINE_PROP_END_OF_LIST (), @@ -1436,7 +1430,8 @@ static const TypeInfo ac97_info = { static void ac97_register_types (void) { type_register_static (&ac97_info); - pci_register_soundhw("ac97", "Intel 82801AA AC97 Audio", ac97_init); + deprecated_register_soundhw("ac97", "Intel 82801AA AC97 Audio", + 0, TYPE_AC97); } type_init (ac97_register_types) diff --git a/qdev-monitor.c b/qdev-monitor.c index 22da107484c5..105d9792ecdf 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -53,6 +53,7 @@ typedef struct QDevAlias /* Please keep this table sorted by typename. */ static const QDevAlias qdev_alias_table[] = { + { "AC97", "ac97" }, /* -soundhw name */ { "e1000", "e1000-82540em" }, { "ich9-ahci", "ahci" }, { "lsi53c895a", "lsi" }, -- 2.18.4
Switch to deprecated_register_soundhw(). Remove the now obsolete init function. Add an alias so both es1370 and ES1370 are working with -device. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/es1370.c | 9 ++------- qdev-monitor.c | 1 + 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/hw/audio/es1370.c b/hw/audio/es1370.c index 5f8a83ff5624..4255463a49ff 100644 --- a/hw/audio/es1370.c +++ b/hw/audio/es1370.c @@ -884,12 +884,6 @@ static void es1370_exit(PCIDevice *dev) AUD_remove_card(&s->card); } -static int es1370_init (PCIBus *bus) -{ - pci_create_simple (bus, -1, TYPE_ES1370); - return 0; -} - static Property es1370_properties[] = { DEFINE_AUDIO_PROPERTIES(ES1370State, card), DEFINE_PROP_END_OF_LIST(), @@ -928,7 +922,8 @@ static const TypeInfo es1370_info = { static void es1370_register_types (void) { type_register_static (&es1370_info); - pci_register_soundhw("es1370", "ENSONIQ AudioPCI ES1370", es1370_init); + deprecated_register_soundhw("es1370", "ENSONIQ AudioPCI ES1370", + 0, TYPE_ES1370); } type_init (es1370_register_types) diff --git a/qdev-monitor.c b/qdev-monitor.c index 105d9792ecdf..e3083fae394b 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -55,6 +55,7 @@ typedef struct QDevAlias static const QDevAlias qdev_alias_table[] = { { "AC97", "ac97" }, /* -soundhw name */ { "e1000", "e1000-82540em" }, + { "ES1370", "es1370" }, /* -soundhw name */ { "ich9-ahci", "ahci" }, { "lsi53c895a", "lsi" }, { "virtio-9p-ccw", "virtio-9p", QEMU_ARCH_S390X }, -- 2.18.4
Switch to deprecated_register_soundhw(). Remove the now obsolete init function. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/adlib.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c index 7c3b67dcfb8c..65dff5b6fca4 100644 --- a/hw/audio/adlib.c +++ b/hw/audio/adlib.c @@ -319,16 +319,10 @@ static const TypeInfo adlib_info = { .class_init = adlib_class_initfn, }; -static int Adlib_init (ISABus *bus) -{ - isa_create_simple (bus, TYPE_ADLIB); - return 0; -} - static void adlib_register_types (void) { type_register_static (&adlib_info); - isa_register_soundhw("adlib", ADLIB_DESC, Adlib_init); + deprecated_register_soundhw("adlib", ADLIB_DESC, 1, TYPE_ADLIB); } type_init (adlib_register_types) -- 2.18.4
Switch to deprecated_register_soundhw(). Remove the now obsolete init function. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/cs4231a.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/hw/audio/cs4231a.c b/hw/audio/cs4231a.c index ffdbb58d6a11..59705a8d4701 100644 --- a/hw/audio/cs4231a.c +++ b/hw/audio/cs4231a.c @@ -683,12 +683,6 @@ static void cs4231a_realizefn (DeviceState *dev, Error **errp) AUD_register_card ("cs4231a", &s->card); } -static int cs4231a_init (ISABus *bus) -{ - isa_create_simple (bus, TYPE_CS4231A); - return 0; -} - static Property cs4231a_properties[] = { DEFINE_AUDIO_PROPERTIES(CSState, card), DEFINE_PROP_UINT32 ("iobase", CSState, port, 0x534), @@ -720,7 +714,7 @@ static const TypeInfo cs4231a_info = { static void cs4231a_register_types (void) { type_register_static (&cs4231a_info); - isa_register_soundhw("cs4231a", "CS4231A", cs4231a_init); + deprecated_register_soundhw("cs4231a", "CS4231A", 1, TYPE_CS4231A); } type_init (cs4231a_register_types) -- 2.18.4
Switch to deprecated_register_soundhw(). Remove the now obsolete init function. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/gus.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/hw/audio/gus.c b/hw/audio/gus.c index c8df2bde6b32..7e4a8cadad6f 100644 --- a/hw/audio/gus.c +++ b/hw/audio/gus.c @@ -286,12 +286,6 @@ static void gus_realizefn (DeviceState *dev, Error **errp) AUD_set_active_out (s->voice, 1); } -static int GUS_init (ISABus *bus) -{ - isa_create_simple (bus, TYPE_GUS); - return 0; -} - static Property gus_properties[] = { DEFINE_AUDIO_PROPERTIES(GUSState, card), DEFINE_PROP_UINT32 ("freq", GUSState, freq, 44100), @@ -322,7 +316,7 @@ static const TypeInfo gus_info = { static void gus_register_types (void) { type_register_static (&gus_info); - isa_register_soundhw("gus", "Gravis Ultrasound GF1", GUS_init); + deprecated_register_soundhw("gus", "Gravis Ultrasound GF1", 1, TYPE_GUS); } type_init (gus_register_types) -- 2.18.4
Switch to deprecated_register_soundhw(). Remove the now obsolete init function. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/sb16.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c index df6f755a37f8..2d9e50f99b5d 100644 --- a/hw/audio/sb16.c +++ b/hw/audio/sb16.c @@ -1415,12 +1415,6 @@ static void sb16_realizefn (DeviceState *dev, Error **errp) AUD_register_card ("sb16", &s->card); } -static int SB16_init (ISABus *bus) -{ - isa_create_simple (bus, TYPE_SB16); - return 0; -} - static Property sb16_properties[] = { DEFINE_AUDIO_PROPERTIES(SB16State, card), DEFINE_PROP_UINT32 ("version", SB16State, ver, 0x0405), /* 4.5 */ @@ -1453,7 +1447,8 @@ static const TypeInfo sb16_info = { static void sb16_register_types (void) { type_register_static (&sb16_info); - isa_register_soundhw("sb16", "Creative Sound Blaster 16", SB16_init); + deprecated_register_soundhw("sb16", "Creative Sound Blaster 16", + 1, TYPE_SB16); } type_init (sb16_register_types) -- 2.18.4
Add deprecation message to the audio init function. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/intel-hda.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c index f673b8317a84..f6cea49686d7 100644 --- a/hw/audio/intel-hda.c +++ b/hw/audio/intel-hda.c @@ -25,6 +25,7 @@ #include "qemu/bitops.h" #include "qemu/log.h" #include "qemu/module.h" +#include "qemu/error-report.h" #include "hw/audio/soundhw.h" #include "intel-hda.h" #include "migration/vmstate.h" @@ -1307,6 +1308,8 @@ static int intel_hda_and_codec_init(PCIBus *bus) BusState *hdabus; DeviceState *codec; + warn_report("'-soundhw hda' is deprecated, " + "please use '-device intel-hda -device hda-duplex' instead"); controller = DEVICE(pci_create_simple(bus, -1, "intel-hda")); hdabus = QLIST_FIRST(&controller->child_bus); codec = qdev_new("hda-duplex"); -- 2.18.4
Add deprecation message to the audio init function. Factor out audio initialization and call that from both audio init and realize, so setting audiodev via -global is enough to properly initialize pcspk. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/pcspk.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c index c37a3878612e..ab290e686783 100644 --- a/hw/audio/pcspk.c +++ b/hw/audio/pcspk.c @@ -28,6 +28,7 @@ #include "audio/audio.h" #include "qemu/module.h" #include "qemu/timer.h" +#include "qemu/error-report.h" #include "hw/timer/i8254.h" #include "migration/vmstate.h" #include "hw/audio/pcspk.h" @@ -112,11 +113,15 @@ static void pcspk_callback(void *opaque, int free) } } -static int pcspk_audio_init(ISABus *bus) +static int pcspk_audio_init(PCSpkState *s) { - PCSpkState *s = pcspk_state; struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUDIO_FORMAT_U8, 0}; + if (s->voice) { + /* already initialized */ + return 0; + } + AUD_register_card(s_spk, &s->card); s->voice = AUD_open_out(&s->card, s->voice, s_spk, s, pcspk_callback, &as); @@ -185,6 +190,10 @@ static void pcspk_realizefn(DeviceState *dev, Error **errp) isa_register_ioport(isadev, &s->ioport, s->iobase); + if (s->card.state) { + pcspk_audio_init(s); + } + pcspk_state = s; } @@ -236,9 +245,18 @@ static const TypeInfo pcspk_info = { .class_init = pcspk_class_initfn, }; +static int pcspk_audio_init_soundhw(ISABus *bus) +{ + PCSpkState *s = pcspk_state; + + warn_report("'-soundhw pcspk' is deprecated, " + "please set a backend using '-global isa-pcspk.audiodev=<name>' instead"); + return pcspk_audio_init(s); +} + static void pcspk_register(void) { type_register_static(&pcspk_info); - isa_register_soundhw("pcspk", "PC speaker", pcspk_audio_init); + isa_register_soundhw("pcspk", "PC speaker", pcspk_audio_init_soundhw); } type_init(pcspk_register) -- 2.18.4
On 22/06/2020 13.29, Gerd Hoffmann wrote:
Add deprecation message to the audio init function.
Factor out audio initialization and call that from both audio init and realize, so setting audiodev via -global is enough to properly initialize pcspk.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/pcspk.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) [...] +static int pcspk_audio_init_soundhw(ISABus *bus) +{ + PCSpkState *s = pcspk_state; + + warn_report("'-soundhw pcspk' is deprecated, " + "please set a backend using '-global isa-pcspk.audiodev=<name>' instead"); + return pcspk_audio_init(s); +}
While "-soundhw pcspk" is quite easy to use for the average user, I think the "-global" options will be quite hard to figure out, especially once this deprecation message got removed again when -soundhw has been deleted. Could you maybe add a description how to configure the pc-speaker to docs/system/target-i386-desc.rst.inc, too? Thanks, Thomas
On 6/22/20 2:59 PM, Thomas Huth wrote:
On 22/06/2020 13.29, Gerd Hoffmann wrote:
Add deprecation message to the audio init function.
Factor out audio initialization and call that from both audio init and realize, so setting audiodev via -global is enough to properly initialize pcspk.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/pcspk.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) [...] +static int pcspk_audio_init_soundhw(ISABus *bus) +{ + PCSpkState *s = pcspk_state; + + warn_report("'-soundhw pcspk' is deprecated, " + "please set a backend using '-global isa-pcspk.audiodev=<name>' instead");
Markus's "Crazy shit around -global (pardon my french)" series instead suggest to use '-device ...': https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg06667.html Could that work here?
+ return pcspk_audio_init(s); +}
While "-soundhw pcspk" is quite easy to use for the average user, I think the "-global" options will be quite hard to figure out, especially once this deprecation message got removed again when -soundhw has been deleted. Could you maybe add a description how to configure the pc-speaker to docs/system/target-i386-desc.rst.inc, too?
Thanks, Thomas
On 22/06/20 15:07, Philippe Mathieu-Daudé wrote:
+static int pcspk_audio_init_soundhw(ISABus *bus) +{ + PCSpkState *s = pcspk_state; + + warn_report("'-soundhw pcspk' is deprecated, " + "please set a backend using '-global isa-pcspk.audiodev=<name>' instead"); Markus's "Crazy shit around -global (pardon my french)" series instead suggest to use '-device ...': https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg06667.html Could that work here?
No, this is a different issue. The problem with the floppy is that it conflated two devices in one (controller and drive), while here the device is builtin. In this case -global could be replaced by a machine property, similar to what is done for the block devices that back parallel flash. That however is orthogonal to providing a good CLI for configuring audio. Paolo
On 22/06/20 14:59, Thomas Huth wrote:
+static int pcspk_audio_init_soundhw(ISABus *bus) +{ + PCSpkState *s = pcspk_state; + + warn_report("'-soundhw pcspk' is deprecated, " + "please set a backend using '-global isa-pcspk.audiodev=<name>' instead"); + return pcspk_audio_init(s); +} While "-soundhw pcspk" is quite easy to use for the average user, I think the "-global" options will be quite hard to figure out, especially once this deprecation message got removed again when -soundhw has been deleted. Could you maybe add a description how to configure the pc-speaker to docs/system/target-i386-desc.rst.inc, too?
-nic is an example of how a single option can work great for both built-in and custom devices, and perhaps we could design something like that for audio. For example you could have # configure a builtin or default audio device # and an ALSA audiodev -sound alsa # configure an external audio device and a PA audiodev # options other than "model" go to audiodev -sound pa,model=sb16,fixed-settings=off For more information: https://www.qemu.org/2018/05/31/nic-parameter/ I think we can go ahead with the deprecation, but -global and -device are probably not good enough a replacement for actual removal. Paolo
On Mon, Jun 22, 2020 at 03:22:05PM +0200, Paolo Bonzini wrote:
On 22/06/20 14:59, Thomas Huth wrote:
+static int pcspk_audio_init_soundhw(ISABus *bus) +{ + PCSpkState *s = pcspk_state; + + warn_report("'-soundhw pcspk' is deprecated, " + "please set a backend using '-global isa-pcspk.audiodev=<name>' instead"); + return pcspk_audio_init(s); +} While "-soundhw pcspk" is quite easy to use for the average user, I think the "-global" options will be quite hard to figure out, especially once this deprecation message got removed again when -soundhw has been deleted. Could you maybe add a description how to configure the pc-speaker to docs/system/target-i386-desc.rst.inc, too?
-nic is an example of how a single option can work great for both built-in and custom devices, and perhaps we could design something like that for audio. For example you could have
# configure a builtin or default audio device # and an ALSA audiodev -sound alsa
# configure an external audio device and a PA audiodev # options other than "model" go to audiodev -sound pa,model=sb16,fixed-settings=off
For more information: https://www.qemu.org/2018/05/31/nic-parameter/
I think we can go ahead with the deprecation, but -global and -device are probably not good enough a replacement for actual removal.
-global is temporary, see patch 19/19 (guess I should reorder the series and squash 19/19 to fix that confusion). The plan is to have audio work simliar to block. -audiodev creates a backend (like -blockdev). Properties are used to reference them. Device properties in most cases, machine properties for builtin devices (i.e. pcspk works like pflash then). take care, Gerd
On 22/06/20 16:03, Gerd Hoffmann wrote:
The plan is to have audio work simliar to block. -audiodev creates a backend (like -blockdev). Properties are used to reference them. Device properties in most cases, machine properties for builtin devices (i.e. pcspk works like pflash then).
Block still has a "-drive" option to configure both frontend and backend, though. Audio should have the same (and I modeled my example on -nic because that's much better than -drive). Paolo
On Mon, Jun 22, 2020 at 02:59:10PM +0200, Thomas Huth wrote:
On 22/06/2020 13.29, Gerd Hoffmann wrote:
Add deprecation message to the audio init function.
Factor out audio initialization and call that from both audio init and realize, so setting audiodev via -global is enough to properly initialize pcspk.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/audio/pcspk.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) [...] +static int pcspk_audio_init_soundhw(ISABus *bus) +{ + PCSpkState *s = pcspk_state; + + warn_report("'-soundhw pcspk' is deprecated, " + "please set a backend using '-global isa-pcspk.audiodev=<name>' instead"); + return pcspk_audio_init(s); +}
While "-soundhw pcspk" is quite easy to use for the average user, I think the "-global" options will be quite hard to figure out, especially once this deprecation message got removed again when -soundhw has been deleted.
See also patch 19/19.
Could you maybe add a description how to configure the pc-speaker to docs/system/target-i386-desc.rst.inc, too?
Makes sense indeed. take care, Gerd
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- docs/system/deprecated.rst | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 3a255591c341..7ddd9e400745 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -82,6 +82,15 @@ should specify an ``audiodev=`` property. Additionally, when using vnc, you should specify an ``audiodev=`` propery if you plan to transmit audio through the VNC protocol. +Creating sound card devices using ``-soundhw`` (since 5.1) +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''' + +Sound card devices should be created using ``-device`` instead. The +names are the same for most devices. The exceptions are ``hda`` which +needs two devices (``-device intel-hda --device hda-duplex``) and +``pcspk`` which can be activated using ``-global +pcspk.audiodev=<name>``. + ``-mon ...,control=readline,pretty=on|off`` (since 4.1) ''''''''''''''''''''''''''''''''''''''''''''''''''''''' -- 2.18.4
On Mon, Jun 22, 2020 at 01:29:07PM +0200, Gerd Hoffmann wrote:
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- docs/system/deprecated.rst | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 3a255591c341..7ddd9e400745 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -82,6 +82,15 @@ should specify an ``audiodev=`` property. Additionally, when using vnc, you should specify an ``audiodev=`` propery if you plan to transmit audio through the VNC protocol.
+Creating sound card devices using ``-soundhw`` (since 5.1) +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''' + +Sound card devices should be created using ``-device`` instead. The +names are the same for most devices. The exceptions are ``hda`` which +needs two devices (``-device intel-hda --device hda-duplex``) and +``pcspk`` which can be activated using ``-global +pcspk.audiodev=<name>``.
The previous "-soundhw pcspk" didn't include any audiodev specification, so I presume it activated some kind of default ? What would be the equivalent audiodev to specify for the new arg to match the old default ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Jun 22, 2020 at 12:42:54PM +0100, Daniel P. Berrangé wrote:
On Mon, Jun 22, 2020 at 01:29:07PM +0200, Gerd Hoffmann wrote:
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- docs/system/deprecated.rst | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 3a255591c341..7ddd9e400745 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -82,6 +82,15 @@ should specify an ``audiodev=`` property. Additionally, when using vnc, you should specify an ``audiodev=`` propery if you plan to transmit audio through the VNC protocol.
+Creating sound card devices using ``-soundhw`` (since 5.1) +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''' + +Sound card devices should be created using ``-device`` instead. The +names are the same for most devices. The exceptions are ``hda`` which +needs two devices (``-device intel-hda --device hda-duplex``) and +``pcspk`` which can be activated using ``-global +pcspk.audiodev=<name>``.
The previous "-soundhw pcspk" didn't include any audiodev specification, so I presume it activated some kind of default ?
Yes. First audiodev it finds. Before -audiodev arrived you could not have multiple backends in the first place, so this covers backward compatibility with existing configurations.
What would be the equivalent audiodev to specify for the new arg to match the old default ?
Doesn't exist. You have to explicitly create an audiodev backend. See also patch 19/19 which adds "-machine pcspk-audidev=<name>", that'll probably more useful for libvirt because (a) -global should be avoided anyway and (b) you can see the new machine property in qapi introspection and pick pcspk init code path based on that. take care, Gerd
Need access to pcms for pcspk initialization. Just preparation, no functional change. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/hw/i386/pc.h | 3 ++- hw/i386/pc.c | 3 ++- hw/i386/pc_piix.c | 2 +- hw/i386/pc_q35.c | 2 +- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index e6135c34d656..9b94926410c0 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -160,7 +160,8 @@ void pc_memory_init(PCMachineState *pcms, MemoryRegion **ram_memory); uint64_t pc_pci_hole64_start(void); DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus); -void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, +void pc_basic_device_init(struct PCMachineState *pcms, + ISABus *isa_bus, qemu_irq *gsi, ISADevice **rtc_state, bool create_fdctrl, bool no_vmport, diff --git a/hw/i386/pc.c b/hw/i386/pc.c index d103b8c0ab82..9afb003dfc21 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1176,7 +1176,8 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl, bool no_vmport) g_free(a20_line); } -void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, +void pc_basic_device_init(struct PCMachineState *pcms, + ISABus *isa_bus, qemu_irq *gsi, ISADevice **rtc_state, bool create_fdctrl, bool no_vmport, diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 1497d0e4ae94..605252fe7936 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -235,7 +235,7 @@ static void pc_init1(MachineState *machine, } /* init basic PC hardware */ - pc_basic_device_init(isa_bus, x86ms->gsi, &rtc_state, true, + pc_basic_device_init(pcms, isa_bus, x86ms->gsi, &rtc_state, true, (pcms->vmport != ON_OFF_AUTO_ON), pcms->pit_enabled, 0x4); diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 46cd06524c68..e4b15f27227a 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -275,7 +275,7 @@ static void pc_q35_init(MachineState *machine) } /* init basic PC hardware */ - pc_basic_device_init(isa_bus, x86ms->gsi, &rtc_state, !mc->no_floppy, + pc_basic_device_init(pcms, isa_bus, x86ms->gsi, &rtc_state, !mc->no_floppy, (pcms->vmport != ON_OFF_AUTO_ON), pcms->pit_enabled, 0xff0104); -- 2.18.4
Now that we pass pcms anyway, we don't need the has_pit arg any more. No functional change. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/hw/i386/pc.h | 1 - hw/i386/pc.c | 3 +-- hw/i386/pc_piix.c | 2 +- hw/i386/pc_q35.c | 2 +- 4 files changed, 3 insertions(+), 5 deletions(-) diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 9b94926410c0..3512dcfd3d27 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -165,7 +165,6 @@ void pc_basic_device_init(struct PCMachineState *pcms, ISADevice **rtc_state, bool create_fdctrl, bool no_vmport, - bool has_pit, uint32_t hpet_irqs); void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd); void pc_cmos_init(PCMachineState *pcms, diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 9afb003dfc21..dfe8ba07e982 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1181,7 +1181,6 @@ void pc_basic_device_init(struct PCMachineState *pcms, ISADevice **rtc_state, bool create_fdctrl, bool no_vmport, - bool has_pit, uint32_t hpet_irqs) { int i; @@ -1232,7 +1231,7 @@ void pc_basic_device_init(struct PCMachineState *pcms, qemu_register_boot_set(pc_boot_set, *rtc_state); - if (!xen_enabled() && has_pit) { + if (!xen_enabled() && pcms->pit_enabled) { if (kvm_pit_in_kernel()) { pit = kvm_pit_init(isa_bus, 0x40); } else { diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 605252fe7936..1242ed811804 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -236,7 +236,7 @@ static void pc_init1(MachineState *machine, /* init basic PC hardware */ pc_basic_device_init(pcms, isa_bus, x86ms->gsi, &rtc_state, true, - (pcms->vmport != ON_OFF_AUTO_ON), pcms->pit_enabled, + (pcms->vmport != ON_OFF_AUTO_ON), 0x4); pc_nic_init(pcmc, isa_bus, pci_bus); diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index e4b15f27227a..25dc40896673 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -276,7 +276,7 @@ static void pc_q35_init(MachineState *machine) /* init basic PC hardware */ pc_basic_device_init(pcms, isa_bus, x86ms->gsi, &rtc_state, !mc->no_floppy, - (pcms->vmport != ON_OFF_AUTO_ON), pcms->pit_enabled, + (pcms->vmport != ON_OFF_AUTO_ON), 0xff0104); /* connect pm stuff to lpc */ -- 2.18.4
Now that we pass pcms anyway, we don't need the no_vmport arg any more. No functional change. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/hw/i386/pc.h | 1 - hw/i386/pc.c | 3 +-- hw/i386/pc_piix.c | 1 - hw/i386/pc_q35.c | 1 - 4 files changed, 1 insertion(+), 5 deletions(-) diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 3512dcfd3d27..283754f42bd5 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -164,7 +164,6 @@ void pc_basic_device_init(struct PCMachineState *pcms, ISABus *isa_bus, qemu_irq *gsi, ISADevice **rtc_state, bool create_fdctrl, - bool no_vmport, uint32_t hpet_irqs); void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd); void pc_cmos_init(PCMachineState *pcms, diff --git a/hw/i386/pc.c b/hw/i386/pc.c index dfe8ba07e982..ba88e1dfb80f 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1180,7 +1180,6 @@ void pc_basic_device_init(struct PCMachineState *pcms, ISABus *isa_bus, qemu_irq *gsi, ISADevice **rtc_state, bool create_fdctrl, - bool no_vmport, uint32_t hpet_irqs) { int i; @@ -1247,7 +1246,7 @@ void pc_basic_device_init(struct PCMachineState *pcms, i8257_dma_init(isa_bus, 0); /* Super I/O */ - pc_superio_init(isa_bus, create_fdctrl, no_vmport); + pc_superio_init(isa_bus, create_fdctrl, pcms->vmport != ON_OFF_AUTO_ON); } void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus) diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 1242ed811804..2752b108001c 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -236,7 +236,6 @@ static void pc_init1(MachineState *machine, /* init basic PC hardware */ pc_basic_device_init(pcms, isa_bus, x86ms->gsi, &rtc_state, true, - (pcms->vmport != ON_OFF_AUTO_ON), 0x4); pc_nic_init(pcmc, isa_bus, pci_bus); diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 25dc40896673..b8c8eaa691f6 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -276,7 +276,6 @@ static void pc_q35_init(MachineState *machine) /* init basic PC hardware */ pc_basic_device_init(pcms, isa_bus, x86ms->gsi, &rtc_state, !mc->no_floppy, - (pcms->vmport != ON_OFF_AUTO_ON), 0xff0104); /* connect pm stuff to lpc */ -- 2.18.4
audiodev must be initialized before machine_set_property so the machine can have auiodev property aliases. spice must initialize before audiodev because the default audiodev is spice only in case spice is actually enabled. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- softmmu/vl.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/softmmu/vl.c b/softmmu/vl.c index f669c06ede4a..b8dc1f5a0e86 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -4131,12 +4131,17 @@ void qemu_init(int argc, char **argv, char **envp) fsdev_init_func, NULL, &error_fatal); #endif + /* spice needs the timers to be initialized by this point */ + /* spice must initialize before audio as it changes the default auiodev */ + qemu_spice_init(); + /* - * Note: we need to create block backends before + * Note: we need to create audio and block backends before * machine_set_property(), so machine properties can refer to * them. */ configure_blockdev(&bdo_queue, machine_class, snapshot); + audio_init_audiodevs(); machine_opts = qemu_get_machine_opts(); qemu_opt_foreach(machine_opts, machine_set_property, current_machine, @@ -4230,9 +4235,6 @@ void qemu_init(int argc, char **argv, char **envp) semihosting_arg_fallback(kernel_filename, kernel_cmdline); } - /* spice needs the timers to be initialized by this point */ - qemu_spice_init(); - cpu_ticks_init(); if (default_net) { -- 2.18.4
Instead of creating and returning the pc speaker accept it as argument. That allows to rework the initialization workflow in followup patches. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/hw/audio/pcspk.h | 6 +----- hw/i386/pc.c | 2 +- hw/isa/i82378.c | 2 +- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/include/hw/audio/pcspk.h b/include/hw/audio/pcspk.h index 7e7f5f49dcb0..8b485602675f 100644 --- a/include/hw/audio/pcspk.h +++ b/include/hw/audio/pcspk.h @@ -31,18 +31,14 @@ #define TYPE_PC_SPEAKER "isa-pcspk" -static inline ISADevice *pcspk_init(ISABus *bus, ISADevice *pit) +static inline void pcspk_init(ISADevice *isadev, ISABus *bus, ISADevice *pit) { DeviceState *dev; - ISADevice *isadev; - isadev = isa_new(TYPE_PC_SPEAKER); dev = DEVICE(isadev); qdev_prop_set_uint32(dev, "iobase", 0x61); object_property_set_link(OBJECT(dev), OBJECT(pit), "pit", NULL); isa_realize_and_unref(isadev, bus, &error_fatal); - - return isadev; } #endif /* HW_PCSPK_H */ diff --git a/hw/i386/pc.c b/hw/i386/pc.c index ba88e1dfb80f..3f1d42b05102 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1240,7 +1240,7 @@ void pc_basic_device_init(struct PCMachineState *pcms, /* connect PIT to output control line of the HPET */ qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(DEVICE(pit), 0)); } - pcspk_init(isa_bus, pit); + pcspk_init(isa_new(TYPE_PC_SPEAKER), isa_bus, pit); } i8257_dma_init(isa_bus, 0); diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c index d9e6c7fa0096..75a2da288157 100644 --- a/hw/isa/i82378.c +++ b/hw/isa/i82378.c @@ -102,7 +102,7 @@ static void i82378_realize(PCIDevice *pci, Error **errp) pit = i8254_pit_init(isabus, 0x40, 0, NULL); /* speaker */ - pcspk_init(isabus, pit); + pcspk_init(isa_new(TYPE_PC_SPEAKER), isabus, pit); /* 2 82C37 (dma) */ isa_create_simple(isabus, "i82374"); -- 2.18.4
Create the pcspk device early enough that we can create an audiodev property alias for the machine type. pcspk init works now with "-audiodev <backend>,id=<name> -M pc.pcspk-audiodev=<name>". Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/hw/i386/pc.h | 1 + hw/i386/pc.c | 5 ++++- softmmu/vl.c | 4 ++-- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 283754f42bd5..3290b0b93097 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -33,6 +33,7 @@ struct PCMachineState { PCIBus *bus; I2CBus *smbus; PFlashCFI01 *flash[2]; + ISADevice *pcspk; /* Configuration options: */ uint64_t max_ram_below_4g; diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 3f1d42b05102..bd393f82377f 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1240,7 +1240,7 @@ void pc_basic_device_init(struct PCMachineState *pcms, /* connect PIT to output control line of the HPET */ qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(DEVICE(pit), 0)); } - pcspk_init(isa_new(TYPE_PC_SPEAKER), isa_bus, pit); + pcspk_init(pcms->pcspk, isa_bus, pit); } i8257_dma_init(isa_bus, 0); @@ -1912,6 +1912,9 @@ static void pc_machine_initfn(Object *obj) pcms->pit_enabled = true; pc_system_flash_create(pcms); + pcms->pcspk = isa_new(TYPE_PC_SPEAKER); + object_property_add_alias(OBJECT(pcms), "pcspk-audiodev", + OBJECT(pcms->pcspk), "audiodev"); } static void pc_machine_reset(MachineState *machine) diff --git a/softmmu/vl.c b/softmmu/vl.c index b8dc1f5a0e86..e8580ccaa2c6 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -4287,6 +4287,8 @@ void qemu_init(int argc, char **argv, char **envp) exit (i == 1 ? 1 : 0); } + audio_init_audiodevs(); + /* This checkpoint is required by replay to separate prior clock reading from the other reads, because timer polling functions query clock values from the log. */ @@ -4344,8 +4346,6 @@ void qemu_init(int argc, char **argv, char **envp) create_default_memdev(current_machine, mem_path); } - audio_init_audiodevs(); - /* from here on runstate is RUN_STATE_PRELAUNCH */ machine_run_board_init(current_machine); -- 2.18.4
Allows to drop the explicit qdev_prop_set_uint32 call in pcspk_init. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/hw/audio/pcspk.h | 6 +----- hw/audio/pcspk.c | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/include/hw/audio/pcspk.h b/include/hw/audio/pcspk.h index 8b485602675f..06cba00b8376 100644 --- a/include/hw/audio/pcspk.h +++ b/include/hw/audio/pcspk.h @@ -33,11 +33,7 @@ static inline void pcspk_init(ISADevice *isadev, ISABus *bus, ISADevice *pit) { - DeviceState *dev; - - dev = DEVICE(isadev); - qdev_prop_set_uint32(dev, "iobase", 0x61); - object_property_set_link(OBJECT(dev), OBJECT(pit), "pit", NULL); + object_property_set_link(OBJECT(isadev), OBJECT(pit), "pit", NULL); isa_realize_and_unref(isadev, bus, &error_fatal); } diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c index ab290e686783..c537ea62d394 100644 --- a/hw/audio/pcspk.c +++ b/hw/audio/pcspk.c @@ -219,7 +219,7 @@ static const VMStateDescription vmstate_spk = { static Property pcspk_properties[] = { DEFINE_AUDIO_PROPERTIES(PCSpkState, card), - DEFINE_PROP_UINT32("iobase", PCSpkState, iobase, -1), + DEFINE_PROP_UINT32("iobase", PCSpkState, iobase, 0x61), DEFINE_PROP_BOOL("migrate", PCSpkState, migrate, true), DEFINE_PROP_END_OF_LIST(), }; -- 2.18.4
participants (5)
-
Daniel P. Berrangé -
Gerd Hoffmann -
Paolo Bonzini -
Philippe Mathieu-Daudé -
Thomas Huth