Paths

Table of Contentst

sound: uaudio: discard hardware sidetone feature units
AcceptedPublic
Actions

Authored by kevans on Tue, Aug 11, 2:54 AM.

Details

Reviewers

Group Reviewers

USB
audio

Summary

The Logitech H390, for instance, has the following interface layout:

7 INPUT              34 INPUT            10 INPUT
  Mic (0x201)          Mic (0x201)         USB Stream (0x101)
   |                    |                    |
   v                    v                    |
 19 FEATURE           35 FEATURE             |
   |                    |                    |
   v                    v                    |
 25 EXTENSION           +------> 36 MIXER <---+
   |                              |
   v                              v
 13 OUTPUT                     22 FEATURE
 USB Stream (0x101)               |
                                  v
                               16 OUTPUT
                               Speaker (0x301)

The 7->13 path on the left is a typical microphone-in configuration,
while the right side is a little more complicated. The 34 -> 35 -> 36
leg is describing a hardware sidetone control, while the other is a
standard audio-out configuration.

During feature unit evaluation, we need to pick up the scenario of node
35 above, which is directly wiring the microphone to the speaker. This
doesn't seem to match the traditional definitions of either IMIX or
MONITOR, so just tag it with SOUND_MIXER_NRDEVICES to avoid tying it
to pcm/vol levels.

This avoids mishandling feature unit 22 because that's evaluated in one
of the other cases: one of the inputs is the USB stream, so it's
wired up as a PCM. I think this is still technically wrong somewhere,
but I haven't decided how- on this headset, the vol control does
absolutely nothing while pcm controls the volume.

PR: 291424

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 75952
Build 72835: arc lint + arc unit

Event Timeline

kevans created this revision.Tue, Aug 11, 2:54 AM

Herald added a subscriber: imp. · View Herald TranscriptTue, Aug 11, 2:54 AM

kevans requested review of this revision.Tue, Aug 11, 2:54 AM

Harbormaster completed remote builds in B75511: Diff 183858.Tue, Aug 11, 2:54 AM

kevans edited the summary of this revision. (Show Details)Tue, Aug 11, 2:56 AM

kevans retitled this revision from sound: uaudio: recognize hardware sidetone as imix to sound: uaudio: discard hardware sidetone feature units.

Ah, vol is neutralized because of that mixer #36. It defaults to ctl == 0 from the struct being zero-init, so we don't create the synthetic vol parent from the pcm feature unit. I don't have enough hardware available to feel confident about poking that hornet's nest.

kevans added reviewers: USB, audio.Wed, Aug 12, 5:45 PM

Gentle ping

on this headset, the vol control does absolutely nothing while pcm controls the volume.

Which controls do you get in mixer(8)?

In D58779#1354749, @christos wrote:

on this headset, the vol control does absolutely nothing while pcm controls the volume.

Which controls do you get in mixer(8)?

pcm4:mixer: <Logitech USB Headset Logitech USB Headset> on uaudio0 (play/rec)
    vol       = 0.75:0.75     pbk
    bass      = 0.50:0.50     pbk
    treble    = 0.50:0.50     pbk
    pcm       = 0.75:0.75     pbk
    mic       = 0.25:0.25     pbk

I've been meaning to take a closer look at uaudio_mixer_add_mixer -- in theory vol controls 36 MIXER in my diagram in the commit message and it should Just Work(TM), but in reality it does not. I suspect that it may have a more complex cluster topology with at least one cluster that isn't a 1:1 channel mapping that we would need to expose for it to actually be a functional volume control.

In D58779#1354774, @kevans wrote:
In D58779#1354749, @christos wrote:

on this headset, the vol control does absolutely nothing while pcm controls the volume.

Which controls do you get in mixer(8)?
pcm4:mixer: <Logitech USB Headset Logitech USB Headset> on uaudio0 (play/rec)
    vol       = 0.75:0.75     pbk
    bass      = 0.50:0.50     pbk
    treble    = 0.50:0.50     pbk
    pcm       = 0.75:0.75     pbk
    mic       = 0.25:0.25     pbk

So the sidetone now is tied to which of those controls?

It isn't tied to any of those now, with this patch. It probably makes sense to add a way to plumb the name out so it can be controlled via sysctl, but IGAIN and MONITOR both sounded like they're historically used for subtly different things than this.

In D58779#1354787, @kevans wrote:

It isn't tied to any of those now, with this patch. It probably makes sense to add a way to plumb the name out so it can be controlled via sysctl, but IGAIN and MONITOR both sounded like they're historically used for subtly different things than this.

I think it'd be better to use one of the existing OSS devices and show it under mixer(8), than implement yet another sysctl.

In D58779#1354788, @christos wrote:

In D58779#1354787, @kevans wrote:

It isn't tied to any of those now, with this patch. It probably makes sense to add a way to plumb the name out so it can be controlled via sysctl, but IGAIN and MONITOR both sounded like they're historically used for subtly different things than this.

I think it'd be better to use one of the existing OSS devices and show it under mixer(8), than implement yet another sysctl.

Fair, and I kind of think MONITOR is the closest fit, but the default value for it is quite bad for this hardware:

static u_int16_t snd_mixerdefaults[SOUND_MIXER_NRDEVICES] = {
[...]
        [SOUND_MIXER_MONITOR]   = 75,                                                                         
};

Anythng above 15 or 20% results in major feedback, and it's not clear how to best handle this. You can hint it lower, of course, but I wonder if it'd make sense to add a function to the kernel mixer interface to allow uaudio to override the default.

In D58779#1354917, @kevans wrote:
static u_int16_t snd_mixerdefaults[SOUND_MIXER_NRDEVICES] = {
[...]
        [SOUND_MIXER_MONITOR]   = 75,                                                                         
};
Anythng above 15 or 20% results in major feedback, and it's not clear how to best handle this. You can hint it lower, of course, but I wonder if it'd make sense to add a function to the kernel mixer interface to allow uaudio to override the default.

You could call mix_set() from uaudio once everything is set up, and set the volume like that. Should work I think.

In D58779#1354918, @christos wrote:
In D58779#1354917, @kevans wrote:
static u_int16_t snd_mixerdefaults[SOUND_MIXER_NRDEVICES] = {
[...]
        [SOUND_MIXER_MONITOR]   = 75,                                                                         
};
Anythng above 15 or 20% results in major feedback, and it's not clear how to best handle this. You can hint it lower, of course, but I wonder if it'd make sense to add a function to the kernel mixer interface to allow uaudio to override the default.
You could call mix_set() from uaudio once everything is set up, and set the volume like that. Should work I think.

I was concerned that I'd still get a momentary blip of feedback between the two, but I will give it a shot.

In D58779#1354920, @kevans wrote:
In D58779#1354918, @christos wrote:
In D58779#1354917, @kevans wrote:
static u_int16_t snd_mixerdefaults[SOUND_MIXER_NRDEVICES] = {
[...]
        [SOUND_MIXER_MONITOR]   = 75,                                                                         
};
Anythng above 15 or 20% results in major feedback, and it's not clear how to best handle this. You can hint it lower, of course, but I wonder if it'd make sense to add a function to the kernel mixer interface to allow uaudio to override the default.
You could call mix_set() from uaudio once everything is set up, and set the volume like that. Should work I think.
I was concerned that I'd still get a momentary blip of feedback between the two, but I will give it a shot.

Possibly. Let me know :)

This seems to work reasonably well and happens quickly enough that there is infact no feedback:

diff --git a/sys/dev/sound/usb/uaudio.c b/sys/dev/sound/usb/uaudio.c
index 4cdd20315258..8fcab56230b3 100644
--- a/sys/dev/sound/usb/uaudio.c
+++ b/sys/dev/sound/usb/uaudio.c
@@ -1198,6 +1199,7 @@ uaudio_attach_sub(device_t dev, kobj_class_t mixer_class, kobj_class_t chan_clas
        }
        if (mixer_init(dev, mixer_class, sc))
                goto detach;
+       mix_set(sc->sc_child[i].mixer_dev, SOUND_MIXER_MONITOR, 10 /* XXX */, 10);
        sc->sc_child[i].mixer_init = 1;
 
        mixer_hwvol_init(dev);

In D58779#1355276, @kevans wrote:

This seems to work reasonably well and happens quickly enough that there is infact no feedback:

diff --git a/sys/dev/sound/usb/uaudio.c b/sys/dev/sound/usb/uaudio.c
index 4cdd20315258..8fcab56230b3 100644
--- a/sys/dev/sound/usb/uaudio.c
+++ b/sys/dev/sound/usb/uaudio.c
@@ -1198,6 +1199,7 @@ uaudio_attach_sub(device_t dev, kobj_class_t mixer_class, kobj_class_t chan_clas
        }
        if (mixer_init(dev, mixer_class, sc))
                goto detach;
+       mix_set(sc->sc_child[i].mixer_dev, SOUND_MIXER_MONITOR, 10 /* XXX */, 10);
        sc->sc_child[i].mixer_init = 1;
 
        mixer_hwvol_init(dev);

Looks good, can be a follow-up patch to this.

Tag them as monitors instead and re-word the comment slightly. The commit
message has also been updated locally.

Harbormaster completed remote builds in B75952: Diff 184692.Fri, Aug 21, 5:50 PM

christos accepted this revision.Sat, Aug 22, 12:44 PM

This revision is now accepted and ready to land.Sat, Aug 22, 12:44 PM

Sidenote: please name the title as "snd_uaudio: ..." instead of "sound: uaudio: ...."

Revision Contents
Changeset List

Path

Size

sys/

dev/

sound/

usb/

uaudio.c

22 lines

Diff 184692

View Options

sys/dev/sound/usb/uaudio.c

sound: uaudio: discard hardware sidetone feature unitsAcceptedPublicActions