Bandan Das <b...@redhat.com> writes:
> Markus Armbruster <arm...@redhat.com> writes:
>
>> Bandan Das <b...@redhat.com> 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.
[...]
