Message ID | cover.1650874791.git.mkletzan@redhat.com |
---|---|
Headers | show |
Series | RFC: Remove deprecated audio features | expand |
On 25/04/2022 09:21, Martin Kletzander wrote: > I wanted to deal with https://bugzilla.redhat.com/2043498 and I got a > suggesstion that removing deprecated features could actually make it > easier to propagate the error. In the end (last patch) it turns out the > error is still just reported with error_fatal, so it probably is not > really needed, but I really wanted to dig into QEMU more and learn some > of the internals for quite some time now. So I used the opportunity. > The one-liner ended up being an 18 patch series which was, for someone > who has just one commit in QEMU codebase, a pretty challenging task. > Although I tried my best to do things properly, I am not sure whether I > handled everything correctly, hence the RFC. > > Any comments are very much appreciated. Thanks and have a nice day ;) > > Martin Kletzander (18): > hw/audio: Remove -soundhw support > hw/input/tsc210x: Extract common init code into new function > hw/audio: Simplify hda audio init > hw/audio/lm4549: Add errp error reporting to init function > tests/qtest: Specify audiodev= and -audiodev > ui/vnc: Require audiodev= > Introduce machine's default-audiodev property > audio: Add easy dummy audio initialiser > hw/display/xlnx_dp.c: Add audiodev property > hw/input/tsc210x.c: Support machine-default audiodev with fallback > hw/arm: Support machine-default audiodev with fallback > hw/ppc: Support machine-default audiodev with fallback > audio: Make AUD_register_card fallible and require audiodev= > audio: Require AudioState in AUD_add_capture > audio: Be more strict during audio backend initialisation > audio: Remove legacy audio environment variables and options > audio: Remove unused can_be_default > audio/spiceaudio: Fail initialisation when not using spice > > audio/alsaaudio.c | 1 - > audio/audio.c | 204 +++---- > audio/audio.h | 5 +- > audio/audio_int.h | 1 - > audio/audio_legacy.c | 555 ------------------ > audio/coreaudio.m | 1 - > audio/dbusaudio.c | 1 - > audio/dsoundaudio.c | 1 - > audio/jackaudio.c | 1 - > audio/meson.build | 1 - > audio/noaudio.c | 1 - > audio/ossaudio.c | 1 - > audio/paaudio.c | 1 - > audio/sdlaudio.c | 1 - > audio/spiceaudio.c | 3 +- > audio/wavaudio.c | 1 - > docs/about/deprecated.rst | 24 - > docs/about/removed-features.rst | 27 + > docs/qdev-device-use.txt | 21 +- > docs/replay.txt | 2 +- > hw/arm/integratorcp.c | 8 +- > hw/arm/musicpal.c | 8 +- > hw/arm/omap2.c | 11 +- > hw/arm/realview.c | 3 + > hw/arm/spitz.c | 10 +- > hw/arm/versatilepb.c | 3 + > hw/arm/vexpress.c | 3 + > hw/arm/xlnx-zcu102.c | 4 + > hw/arm/z2.c | 12 +- > hw/audio/ac97.c | 9 +- > hw/audio/adlib.c | 9 +- > hw/audio/cs4231a.c | 8 +- > hw/audio/es1370.c | 8 +- > hw/audio/gus.c | 6 +- > hw/audio/hda-codec.c | 37 +- > hw/audio/intel-hda.c | 25 +- > hw/audio/intel-hda.h | 2 +- > hw/audio/lm4549.c | 7 +- > hw/audio/lm4549.h | 3 +- > hw/audio/meson.build | 1 - > hw/audio/pcspk.c | 15 +- > hw/audio/pl041.c | 2 +- > hw/audio/sb16.c | 9 +- > hw/audio/soundhw.c | 177 ------ > hw/audio/wm8750.c | 5 +- > hw/core/machine.c | 23 + > hw/display/xlnx_dp.c | 12 +- > hw/input/tsc210x.c | 79 ++- > hw/ppc/prep.c | 4 + > hw/usb/dev-audio.c | 5 +- > include/hw/audio/soundhw.h | 15 - > include/hw/boards.h | 1 + > qemu-options.hx | 37 -- > .../codeconverter/test_regexps.py | 1 - > softmmu/qdev-monitor.c | 2 - > softmmu/vl.c | 10 - > tests/qtest/ac97-test.c | 3 +- > tests/qtest/es1370-test.c | 3 +- > tests/qtest/fuzz/generic_fuzz_configs.h | 6 +- > tests/qtest/intel-hda-test.c | 15 +- > ui/vnc.c | 15 +- > 61 files changed, 329 insertions(+), 1140 deletions(-) > delete mode 100644 audio/audio_legacy.c > delete mode 100644 hw/audio/soundhw.c > delete mode 100644 include/hw/audio/soundhw.h Thanks for the tidy-up! I'm not too familiar with the audio code, however one thing I noticed is that in patch 11 you are using qdev_prop_set_string() to set the audiodev for the machines. For CharDevs there already exists a qdev_prop_set_chr() function which is used to assign the chardev backend to the device, so I'm wondering if it makes sense to add a similar qdev_prop_set_audiodev() function in the same way? Then if the method of referencing the audiodev from the device ever changes from being a string containing the name, it won't require updating all of the callers. Finally a quick glance at the tsc210x and lm4549 devices indicates that they are using global device _init() functions which shouldn't really be used in modern implementations. If you are interested in QOMify-ing these devices as part of this work, then I can certainly help provide some pointers for getting started. ATB, Mark.
On Mon, Apr 25, 2022 at 10:21:43AM +0200, Martin Kletzander wrote: > I wanted to deal with https://bugzilla.redhat.com/2043498 and I got a > suggesstion that removing deprecated features could actually make it > easier to propagate the error. In the end (last patch) it turns out the > error is still just reported with error_fatal, so it probably is not > really needed, but I really wanted to dig into QEMU more and learn some > of the internals for quite some time now. So I used the opportunity. > The one-liner ended up being an 18 patch series which was, for someone > who has just one commit in QEMU codebase, a pretty challenging task. > Although I tried my best to do things properly, I am not sure whether I > handled everything correctly, hence the RFC. > > Any comments are very much appreciated. Thanks and have a nice day ;) Hi Martin, The -soundhw removal looks good to me. I'm not familiar with the audio subsystem so I didn't review the rest. Stefan
On Mon, Apr 25, 2022 at 06:05:56PM +0100, Mark Cave-Ayland wrote: >On 25/04/2022 09:21, Martin Kletzander wrote: > >> I wanted to deal with https://bugzilla.redhat.com/2043498 and I got a >> suggesstion that removing deprecated features could actually make it >> easier to propagate the error. In the end (last patch) it turns out the >> error is still just reported with error_fatal, so it probably is not >> really needed, but I really wanted to dig into QEMU more and learn some >> of the internals for quite some time now. So I used the opportunity. >> The one-liner ended up being an 18 patch series which was, for someone >> who has just one commit in QEMU codebase, a pretty challenging task. >> Although I tried my best to do things properly, I am not sure whether I >> handled everything correctly, hence the RFC. >> >> Any comments are very much appreciated. Thanks and have a nice day ;) >> >> Martin Kletzander (18): >> hw/audio: Remove -soundhw support >> hw/input/tsc210x: Extract common init code into new function >> hw/audio: Simplify hda audio init >> hw/audio/lm4549: Add errp error reporting to init function >> tests/qtest: Specify audiodev= and -audiodev >> ui/vnc: Require audiodev= >> Introduce machine's default-audiodev property >> audio: Add easy dummy audio initialiser >> hw/display/xlnx_dp.c: Add audiodev property >> hw/input/tsc210x.c: Support machine-default audiodev with fallback >> hw/arm: Support machine-default audiodev with fallback >> hw/ppc: Support machine-default audiodev with fallback >> audio: Make AUD_register_card fallible and require audiodev= >> audio: Require AudioState in AUD_add_capture >> audio: Be more strict during audio backend initialisation >> audio: Remove legacy audio environment variables and options >> audio: Remove unused can_be_default >> audio/spiceaudio: Fail initialisation when not using spice >> [...] > >Thanks for the tidy-up! I'm not too familiar with the audio code, however one thing I >noticed is that in patch 11 you are using qdev_prop_set_string() to set the audiodev >for the machines. > >For CharDevs there already exists a qdev_prop_set_chr() function which is used to >assign the chardev backend to the device, so I'm wondering if it makes sense to add a >similar qdev_prop_set_audiodev() function in the same way? Then if the method of >referencing the audiodev from the device ever changes from being a string containing >the name, it won't require updating all of the callers. > I guess that could be possible, but I'm not familiar with QEMU code much and to be honest have no idea how I would go about doing that right now. >Finally a quick glance at the tsc210x and lm4549 devices indicates that they are >using global device _init() functions which shouldn't really be used in modern >implementations. If you are interested in QOMify-ing these devices as part of this >work, then I can certainly help provide some pointers for getting started. > I thought about it for some devices. Not all the devices I touched here, because I feel like that would lead to a rabbit hole. And 18 patches just because I wanted to error out in one condition was already too much =) But I'll be glad for any pointers on where to start and how should I get to it. Thanks. > >ATB, > >Mark. >
On 4/25/22 10:21, Martin Kletzander wrote: > I wanted to deal with https://bugzilla.redhat.com/2043498 and I got a > suggesstion that removing deprecated features could actually make it > easier to propagate the error. In the end (last patch) it turns out the > error is still just reported with error_fatal, so it probably is not > really needed, but I really wanted to dig into QEMU more and learn some > of the internals for quite some time now. So I used the opportunity. > The one-liner ended up being an 18 patch series which was, for someone > who has just one commit in QEMU codebase, a pretty challenging task. > Although I tried my best to do things properly, I am not sure whether I > handled everything correctly, hence the RFC. Rebased and queued what's left... It took a while. :) Paolo > Martin Kletzander (18): > hw/audio: Remove -soundhw support > hw/input/tsc210x: Extract common init code into new function > hw/audio: Simplify hda audio init > hw/audio/lm4549: Add errp error reporting to init function > tests/qtest: Specify audiodev= and -audiodev > ui/vnc: Require audiodev= > Introduce machine's default-audiodev property > audio: Add easy dummy audio initialiser > hw/display/xlnx_dp.c: Add audiodev property > hw/input/tsc210x.c: Support machine-default audiodev with fallback > hw/arm: Support machine-default audiodev with fallback > hw/ppc: Support machine-default audiodev with fallback > audio: Make AUD_register_card fallible and require audiodev= > audio: Require AudioState in AUD_add_capture > audio: Be more strict during audio backend initialisation > audio: Remove legacy audio environment variables and options > audio: Remove unused can_be_default > audio/spiceaudio: Fail initialisation when not using spice > > audio/alsaaudio.c | 1 - > audio/audio.c | 204 +++---- > audio/audio.h | 5 +- > audio/audio_int.h | 1 - > audio/audio_legacy.c | 555 ------------------ > audio/coreaudio.m | 1 - > audio/dbusaudio.c | 1 - > audio/dsoundaudio.c | 1 - > audio/jackaudio.c | 1 - > audio/meson.build | 1 - > audio/noaudio.c | 1 - > audio/ossaudio.c | 1 - > audio/paaudio.c | 1 - > audio/sdlaudio.c | 1 - > audio/spiceaudio.c | 3 +- > audio/wavaudio.c | 1 - > docs/about/deprecated.rst | 24 - > docs/about/removed-features.rst | 27 + > docs/qdev-device-use.txt | 21 +- > docs/replay.txt | 2 +- > hw/arm/integratorcp.c | 8 +- > hw/arm/musicpal.c | 8 +- > hw/arm/omap2.c | 11 +- > hw/arm/realview.c | 3 + > hw/arm/spitz.c | 10 +- > hw/arm/versatilepb.c | 3 + > hw/arm/vexpress.c | 3 + > hw/arm/xlnx-zcu102.c | 4 + > hw/arm/z2.c | 12 +- > hw/audio/ac97.c | 9 +- > hw/audio/adlib.c | 9 +- > hw/audio/cs4231a.c | 8 +- > hw/audio/es1370.c | 8 +- > hw/audio/gus.c | 6 +- > hw/audio/hda-codec.c | 37 +- > hw/audio/intel-hda.c | 25 +- > hw/audio/intel-hda.h | 2 +- > hw/audio/lm4549.c | 7 +- > hw/audio/lm4549.h | 3 +- > hw/audio/meson.build | 1 - > hw/audio/pcspk.c | 15 +- > hw/audio/pl041.c | 2 +- > hw/audio/sb16.c | 9 +- > hw/audio/soundhw.c | 177 ------ > hw/audio/wm8750.c | 5 +- > hw/core/machine.c | 23 + > hw/display/xlnx_dp.c | 12 +- > hw/input/tsc210x.c | 79 ++- > hw/ppc/prep.c | 4 + > hw/usb/dev-audio.c | 5 +- > include/hw/audio/soundhw.h | 15 - > include/hw/boards.h | 1 + > qemu-options.hx | 37 -- > .../codeconverter/test_regexps.py | 1 - > softmmu/qdev-monitor.c | 2 - > softmmu/vl.c | 10 - > tests/qtest/ac97-test.c | 3 +- > tests/qtest/es1370-test.c | 3 +- > tests/qtest/fuzz/generic_fuzz_configs.h | 6 +- > tests/qtest/intel-hda-test.c | 15 +- > ui/vnc.c | 15 +- > 61 files changed, 329 insertions(+), 1140 deletions(-) > delete mode 100644 audio/audio_legacy.c > delete mode 100644 hw/audio/soundhw.c > delete mode 100644 include/hw/audio/soundhw.h >