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

[Markus Armbruster]

Bandan Das <address@hidden> writes:

> 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... 

Commit messages are easy enough to fix :)

>> 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.

Two devices would probably be similarly ugly.

I'm fine with either way.  The property way now has the distinct
advantage of working code.

>> 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).

I think we should default to working, fully-featured devices, because
that's what most users want.  The few users prepared to sacrifice the
hardware mixer in the hope of saving a few cycles can try mixer=off.

[...]