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 a Friday in 2020, 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(-)
>
>@@ -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);

-soundhw pcspk is the only soundhw device present in libvirt git.

Is there a way to probe for this change via QMP?

Jano

>+}
>+
> 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 Fri, May 15, 2020 at 05:08:23PM +0200, Ján Tomko wrote:
> On a Friday in 2020, 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(-)
> > 
> > @@ -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);
> 
> -soundhw pcspk is the only soundhw device present in libvirt git.
> 
> Is there a way to probe for this change via QMP?

Oops.  I'm surprised libvirt actually supports pcspk.

There is no way to see that in qmp, and I can't think of an easy way
to add that.  Does libvirt check for command line switches still?
So it could see -soundhw going away if that happens?

take care,
  Gerd

On Mon, May 18, 2020 at 12:16:28PM +0200, Gerd Hoffmann wrote:
> On Fri, May 15, 2020 at 05:08:23PM +0200, Ján Tomko wrote:
> > On a Friday in 2020, 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(-)
> > > 
> > > @@ -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);
> > 
> > -soundhw pcspk is the only soundhw device present in libvirt git.
> > 
> > Is there a way to probe for this change via QMP?
> 
> Oops.  I'm surprised libvirt actually supports pcspk.
> 
> There is no way to see that in qmp, and I can't think of an easy way
> to add that.  Does libvirt check for command line switches still?
> So it could see -soundhw going away if that happens?

IIUC, instead of probing for whether -soundhw is deprecated, it should
be suffiicent for us to probe if "isa-pcspk.audiodev" exists. Assuming
we always use isa-pcspk.audiodev if it exists, then we'll trivially
avoid using the -soundhw arg.

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 a Monday in 2020, Daniel P. Berrangé wrote:
>On Mon, May 18, 2020 at 12:16:28PM +0200, Gerd Hoffmann wrote:
>> On Fri, May 15, 2020 at 05:08:23PM +0200, Ján Tomko wrote:
>> > On a Friday in 2020, 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(-)
>> > >
>> > > @@ -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);
>> >
>> > -soundhw pcspk is the only soundhw device present in libvirt git.
>> >
>> > Is there a way to probe for this change via QMP?
>>
>> Oops.  I'm surprised libvirt actually supports pcspk.
>>
>> There is no way to see that in qmp, and I can't think of an easy way
>> to add that.  Does libvirt check for command line switches still?
>> So it could see -soundhw going away if that happens?
>
>IIUC, instead of probing for whether -soundhw is deprecated, it should
>be suffiicent for us to probe if "isa-pcspk.audiodev" exists. Assuming
>we always use isa-pcspk.audiodev if it exists, then we'll trivially
>avoid using the -soundhw arg.
>

Yes, we can probe for that, but the phrasing in the commit message makes
it look like setting the property via -global will only be effective
after this commit.

Jano

>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, May 18, 2020 at 11:26:50AM +0100, Daniel P. Berrangé wrote:
> On Mon, May 18, 2020 at 12:16:28PM +0200, Gerd Hoffmann wrote:
> > On Fri, May 15, 2020 at 05:08:23PM +0200, Ján Tomko wrote:
> > > On a Friday in 2020, 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(-)
> > > > 
> > > > @@ -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);
> > > 
> > > -soundhw pcspk is the only soundhw device present in libvirt git.
> > > 
> > > Is there a way to probe for this change via QMP?
> > 
> > Oops.  I'm surprised libvirt actually supports pcspk.
> > 
> > There is no way to see that in qmp, and I can't think of an easy way
> > to add that.  Does libvirt check for command line switches still?
> > So it could see -soundhw going away if that happens?
> 
> IIUC, instead of probing for whether -soundhw is deprecated, it should
> be suffiicent for us to probe if "isa-pcspk.audiodev" exists. Assuming
> we always use isa-pcspk.audiodev if it exists, then we'll trivially
> avoid using the -soundhw arg.

It's not that easy unfortunately.  We have .audiodev for a few releases
already.  But just setting that isn't enough to initialize pcspk in
current qemu, "-soundhw pcspk" is still needed ...

I'm looking at how to initialize onboard audio devices currently, maybe
the best way to handle that is to do it flash-style with machine
properties (i.e have a pc.pcslk alias for pcspk.audiodev, simliar to
pc.flash0 being an alias for pflash.drive).  That'll be better that
-global and it'll also be visible in QOM.

Initialization order looks tricky though.  I'd have to create pcspk
early, simliar to flash, in pc_machine_initfn().  Problem is I don't
have a isa bus yet at that point (flash is sysbus and doesn't have this
problem).  I'm open to suggestions hiow do deal with that best.

take care,
  Gerd

  Hi,

> Initialization order looks tricky though.  I'd have to create pcspk
> early, simliar to flash, in pc_machine_initfn().  Problem is I don't
> have a isa bus yet at that point (flash is sysbus and doesn't have this
> problem).  I'm open to suggestions hiow do deal with that best.

Seems I've found a way to deal with that: "ISADevice *pcspk =
object_new(TYPE_PC_SPEAKER);" can be done before the isa bus exists
& we can fixup things later using qdev_set_parent_bus().

cheers,
  Gerd

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> Initialization order looks tricky though.  I'd have to create pcspk
>> early, simliar to flash, in pc_machine_initfn().  Problem is I don't
>> have a isa bus yet at that point (flash is sysbus and doesn't have this
>> problem).  I'm open to suggestions hiow do deal with that best.
>
> Seems I've found a way to deal with that: "ISADevice *pcspk =
> object_new(TYPE_PC_SPEAKER);" can be done before the isa bus exists
> & we can fixup things later using qdev_set_parent_bus().

You'll want to watch out for the series I hope to post shortly: it'll be
dev = qdev_new(TYPE_PC_SPEAKER); qdev_realize(dev, bus, errp) then.  No
need for qdev_set_parent_bus().

  Hi,

> >> Initialization order looks tricky though.  I'd have to create pcspk
> >> early, simliar to flash, in pc_machine_initfn().  Problem is I don't
> >> have a isa bus yet at that point (flash is sysbus and doesn't have this
> >> problem).  I'm open to suggestions hiow do deal with that best.
> >
> > Seems I've found a way to deal with that: "ISADevice *pcspk =
> > object_new(TYPE_PC_SPEAKER);" can be done before the isa bus exists
> > & we can fixup things later using qdev_set_parent_bus().
> 
> You'll want to watch out for the series I hope to post shortly: it'll be
> dev = qdev_new(TYPE_PC_SPEAKER); qdev_realize(dev, bus, errp) then.  No
> need for qdev_set_parent_bus().

Ah, cool, that shows that I'm on the right path with my idea ;)
Sneak preview:
	https://git.kraxel.org/cgit/qemu/log/?h=sirius/soundhw

Suggestions for a good name?  I've used "pc.pcspk" for now,
but maybe "pc.audiodev0" or "pc.sound0" is better?

take care,
  Gerd

On 18/05/20 15:27, Gerd Hoffmann wrote:
> Ah, cool, that shows that I'm on the right path with my idea ;)
> Sneak preview:
> 	https://git.kraxel.org/cgit/qemu/log/?h=sirius/soundhw
> 
> Suggestions for a good name?  I've used "pc.pcspk" for now,
> but maybe "pc.audiodev0" or "pc.sound0" is better?

Something like "-M pc,pcspk-audiodev=..."?

Paolo