[v3,11/19] audio: deprecate -soundhw pcspk
diff mbox series

Message ID 20200622112914.30454-12-kraxel@redhat.com
State New
Headers show
Series
  • audio: deprecate -soundhw
Related show

Commit Message

Gerd Hoffmann June 22, 2020, 11:29 a.m. UTC
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(-)

Comments

Thomas Huth June 22, 2020, 12:59 p.m. UTC | #1
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
Philippe Mathieu-Daudé June 22, 2020, 1:07 p.m. UTC | #2
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
> 
>
Paolo Bonzini June 22, 2020, 1:22 p.m. UTC | #3
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
Paolo Bonzini June 22, 2020, 1:25 p.m. UTC | #4
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
Gerd Hoffmann June 22, 2020, 1:38 p.m. UTC | #5
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
Gerd Hoffmann June 22, 2020, 2:03 p.m. UTC | #6
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
Paolo Bonzini June 22, 2020, 2:35 p.m. UTC | #7
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

Patch
diff mbox series

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)