Re: [Qemu-devel] [PATCH 0/2] Make mixer emulation configurable at runtime

[Bandan Das]

Markus Armbruster <address@hidden> writes:

> Bandan Das <address@hidden> writes:
>
>> Currently, hda-codec mixer emulation can only be enabled by using the
>> "--enable-mixemu" option to configure at compile time. These patches add 
>> the option to make it configurable at runtime as well. 
>>
>> An example usage could be a qemu cmdline  : 
>> "-device hda-duplex,mixer=(on/off)" although this 
>> is not part of this change.
>
> Really?  I see a new property "mixer" in PATCH 2/2.  It's UINT32 rather
> than BOOL, though.

Well, what else can I say, Bad example... 

> Let's review where we are and where your patch takes us.
>
> CONFIG_MIXEMU is set by configure --enable-mixemu (default off).  It
> does two things:
>
> * It makes mixeng_volume() actually do something.  Users:
>
>   audio_pcm_sw_read():
>
>     if (!(hw->ctl_caps & VOICE_VOLUME_CAP)) {
>         mixeng_volume (sw->buf, ret, &sw->vol);
>     }
>
>   audio_pcm_sw_write():
>
>         if (!(sw->hw->ctl_caps & VOICE_VOLUME_CAP)) {
>             mixeng_volume (sw->buf, swlim, &sw->vol);
>         }
>
>   Only pulse and the spice audio backends set VOICE_VOLUME_CAP.
>
>   Impact on users isn't obvious to me.
>
> * Devices hda-{output,duplex,micro} have a mixer if and only if
>   CONFIG_MIXEMU is enabled.
>
>   Reason: their mixer cannot work without CONFIG_MIXEMU.  Advertizing a
>   broken hardware mixer to guests breaks volume control.  Not nice.  So,
>   advertize it only when it works.  When CONFIG_MIXEMU is disabled, the
>   hardware provides no mixer, guests fall back to a software mixer, and
>   volume control works.  Commit d61a4ce.
>
>   So, -device hda-duplex gives you *different* devices depending on
>   whether you enabled mixer emulation.  Enabling/disabling QEMU's mixer
>   emulation changes guest ABI.  This was a bad idea.  We should have
>   created either two sets of devices, one with and one without a mixer,
>   or a device property to control mixer presence.
>
>   Your patch implements the latter.  And boy is it ugly :)
>

I prefer having a device property rather than two different devices
which is probably overkill IMO. This is the most straight-forward way
I could do it.

> Questions (not just for you, Bandan):
>
> 1. Why is CONFIG_MIXEMU off by default?
>
>    We generally default optional code to "on", to avoid bit-rot.  We
>    make exceptions only when the optional code is truly unwanted by a
>    clear majority of our users.  Why would anyone want hda audio devices
>    without a mixer?
>
> 2. Why do we bother providing these devices when CONFIG_MIXEMU off?
>
>    Why would anyone want hda audio devices without a mixer?  Why
>    wouldn't anyone who wants hda audio devices also want CONFIG_MIXEMU
>    enabled?
>
>    The only remotely convincing reason I can think of is "we foolishly
>    provided them, and now we have to keep them for backward
>    compatibility".
>
> 3. Why does CONFIG_MIXEMU exist?
>
I can post a patch for it (again) on top of these changes and 
see where it goes. If the default value of the "mixer" property stays
off, malc's concerns mentioned elsewhere in this thread will be taken 
care of too (I think).

Bandan
>    Closest thing to a discussion I could find in the list archives:
>
>    http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg02313.html
>    http://lists.gnu.org/archive/html/qemu-devel/2012-04/msg02285.html
>
>    I don't exactly find the argument for CONFIG_MIXEMU convincing.