| Message ID | 49192ccd62fc98c474bb0b627a22cc28a634e8b8.1650874791.git.mkletzan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: Remove deprecated audio features | expand |
On Mon, Apr 25, 2022 at 10:21:50AM +0200, Martin Kletzander wrote: > Many machine types have default audio devices with no way to set the underlying > audiodev. Instead of adding an option for each and every one of them this new > property can be used as a default during machine initialisation when creating > such devices. > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com> > --- > hw/core/machine.c | 23 +++++++++++++++++++++++ > include/hw/boards.h | 1 + > 2 files changed, 24 insertions(+) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index cb9bbc844d24..d055a126d398 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -596,6 +596,22 @@ static void machine_set_memdev(Object *obj, const char *value, Error **errp) > ms->ram_memdev_id = g_strdup(value); > } > > +static char *machine_get_default_audiodev(Object *obj, Error **errp) > +{ > + MachineState *ms = MACHINE(obj); > + > + return g_strdup(ms->default_audiodev); > +} > + > +static void machine_set_default_audiodev(Object *obj, const char *value, > + Error **errp) > +{ > + MachineState *ms = MACHINE(obj); > + > + g_free(ms->default_audiodev); > + ms->default_audiodev = g_strdup(value); > +} > + > HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine) > { > int i; > @@ -867,6 +883,12 @@ static void machine_class_init(ObjectClass *oc, void *data) > object_class_property_set_description(oc, "confidential-guest-support", > "Set confidential guest scheme to support"); > > + object_class_property_add_str(oc, "default-audiodev", > + machine_get_default_audiodev, > + machine_set_default_audiodev); > + object_class_property_set_description(oc, "default-audiodev", > + "Audiodev to use for default machine devices"); > + Hmm, if we add this, people might reasonably question why this default audiodev can't be used for everything, instead of making other 'audiodev' parameter mandatory. For the x86 machines we have a property tied specifically to the PC speaker. -machine ...,pcspk-audiodev=<name> If we assume that's the desired pattern, then every machine which has a built-in audio device should gain some "{$device}-audiodev" proprerty where '$device' is some reasonable name for the built-in audio device of the machine. This would work better if a machine ended up with two built-in audio devices and needed separate audiodevs for them. > /* For compatibility */ > object_class_property_add_str(oc, "memory-encryption", > machine_get_memory_encryption, machine_set_memory_encryption); > @@ -961,6 +983,7 @@ static void machine_finalize(Object *obj) > g_free(ms->device_memory); > g_free(ms->nvdimms_state); > g_free(ms->numa_state); > + g_free(ms->default_audiodev); > } > > bool machine_usb(MachineState *machine) > diff --git a/include/hw/boards.h b/include/hw/boards.h > index d64b5481e834..5be1de50af03 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -346,6 +346,7 @@ struct MachineState { > */ > MemoryRegion *ram; > DeviceMemoryState *device_memory; > + char *default_audiodev; > > ram_addr_t ram_size; > ram_addr_t maxram_size; > -- > 2.35.1 > With regards, Daniel
On Mon, Apr 25, 2022 at 03:06:14PM +0100, Daniel P. Berrangé wrote: >On Mon, Apr 25, 2022 at 10:21:50AM +0200, Martin Kletzander wrote: >> Many machine types have default audio devices with no way to set the underlying >> audiodev. Instead of adding an option for each and every one of them this new >> property can be used as a default during machine initialisation when creating >> such devices. >> >> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> >> --- >> hw/core/machine.c | 23 +++++++++++++++++++++++ >> include/hw/boards.h | 1 + >> 2 files changed, 24 insertions(+) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index cb9bbc844d24..d055a126d398 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -596,6 +596,22 @@ static void machine_set_memdev(Object *obj, const char *value, Error **errp) >> ms->ram_memdev_id = g_strdup(value); >> } >> >> +static char *machine_get_default_audiodev(Object *obj, Error **errp) >> +{ >> + MachineState *ms = MACHINE(obj); >> + >> + return g_strdup(ms->default_audiodev); >> +} >> + >> +static void machine_set_default_audiodev(Object *obj, const char *value, >> + Error **errp) >> +{ >> + MachineState *ms = MACHINE(obj); >> + >> + g_free(ms->default_audiodev); >> + ms->default_audiodev = g_strdup(value); >> +} >> + >> HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine) >> { >> int i; >> @@ -867,6 +883,12 @@ static void machine_class_init(ObjectClass *oc, void *data) >> object_class_property_set_description(oc, "confidential-guest-support", >> "Set confidential guest scheme to support"); >> >> + object_class_property_add_str(oc, "default-audiodev", >> + machine_get_default_audiodev, >> + machine_set_default_audiodev); >> + object_class_property_set_description(oc, "default-audiodev", >> + "Audiodev to use for default machine devices"); >> + > >Hmm, if we add this, people might reasonably question why this default >audiodev can't be used for everything, instead of making other 'audiodev' >parameter mandatory. > >For the x86 machines we have a property tied specifically to the PC >speaker. > > -machine ...,pcspk-audiodev=<name> > > >If we assume that's the desired pattern, then every machine which has >a built-in audio device should gain some "{$device}-audiodev" proprerty >where '$device' is some reasonable name for the built-in audio device >of the machine. This would work better if a machine ended up with >two built-in audio devices and needed separate audiodevs for them. > Which is what I wanted to avoid because it creates huge amount of names which are not easy to find out. I imagine that the default audio devices which already exist are not much known and mostly used as-is without the users needing to figure out what they are. Especially those that are enabled even with -nodefaults. And I expect people who just want to just emulate an old Palm do not really want to specify the names of both devices which need an audiodev, especially when one of them is a touch display or something weird like that. Moreover you cannot specify anything for these devices currently, so you cannot use two different backends now. I understand that the naming suggests it would be a default audiodev for anything without one explicitly specified, but I could not think of any other suitable name. And I felt like having a default that would be used for all devices defeats the purpose of removing default audiodev. If adding a machine option for every such device is the way to go, then I can try changing that. Hopefully that would not turn into another 18-patch series =) So should I prefer that route? > >> /* For compatibility */ >> object_class_property_add_str(oc, "memory-encryption", >> machine_get_memory_encryption, machine_set_memory_encryption); >> @@ -961,6 +983,7 @@ static void machine_finalize(Object *obj) >> g_free(ms->device_memory); >> g_free(ms->nvdimms_state); >> g_free(ms->numa_state); >> + g_free(ms->default_audiodev); >> } >> >> bool machine_usb(MachineState *machine) >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index d64b5481e834..5be1de50af03 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -346,6 +346,7 @@ struct MachineState { >> */ >> MemoryRegion *ram; >> DeviceMemoryState *device_memory; >> + char *default_audiodev; >> >> ram_addr_t ram_size; >> ram_addr_t maxram_size; >> -- >> 2.35.1 >> > >With 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 :| >
diff --git a/hw/core/machine.c b/hw/core/machine.c index cb9bbc844d24..d055a126d398 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -596,6 +596,22 @@ static void machine_set_memdev(Object *obj, const char *value, Error **errp) ms->ram_memdev_id = g_strdup(value); } +static char *machine_get_default_audiodev(Object *obj, Error **errp) +{ + MachineState *ms = MACHINE(obj); + + return g_strdup(ms->default_audiodev); +} + +static void machine_set_default_audiodev(Object *obj, const char *value, + Error **errp) +{ + MachineState *ms = MACHINE(obj); + + g_free(ms->default_audiodev); + ms->default_audiodev = g_strdup(value); +} + HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine) { int i; @@ -867,6 +883,12 @@ static void machine_class_init(ObjectClass *oc, void *data) object_class_property_set_description(oc, "confidential-guest-support", "Set confidential guest scheme to support"); + object_class_property_add_str(oc, "default-audiodev", + machine_get_default_audiodev, + machine_set_default_audiodev); + object_class_property_set_description(oc, "default-audiodev", + "Audiodev to use for default machine devices"); + /* For compatibility */ object_class_property_add_str(oc, "memory-encryption", machine_get_memory_encryption, machine_set_memory_encryption); @@ -961,6 +983,7 @@ static void machine_finalize(Object *obj) g_free(ms->device_memory); g_free(ms->nvdimms_state); g_free(ms->numa_state); + g_free(ms->default_audiodev); } bool machine_usb(MachineState *machine) diff --git a/include/hw/boards.h b/include/hw/boards.h index d64b5481e834..5be1de50af03 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -346,6 +346,7 @@ struct MachineState { */ MemoryRegion *ram; DeviceMemoryState *device_memory; + char *default_audiodev; ram_addr_t ram_size; ram_addr_t maxram_size;
Many machine types have default audio devices with no way to set the underlying audiodev. Instead of adding an option for each and every one of them this new property can be used as a default during machine initialisation when creating such devices. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- hw/core/machine.c | 23 +++++++++++++++++++++++ include/hw/boards.h | 1 + 2 files changed, 24 insertions(+)