In D59084#1360654, @markj wrote:

In D59084#1359548, @christos wrote:

In D59084#1358453, @markj wrote:

The patch looks like it does what the review description says it does, but apparently it doesn't actually fix the cited PR?

Yes. I cited the PR because it does fix an issue raised indirectly in the PR. Should I omit the "PR" line?

I'm not sure. It depends on the root cause of the bug. Maybe this patch helps but isn't sufficient, in which case the PR line should be included, or maybe the problem is unrelated, in which case it shouldn't be included.

No problem! Make sure you MFC this to stable/15 at some point.

Can you please take care of MFCing this?

In D59084#1358453, @markj wrote:

The patch looks like it does what the review description says it does, but apparently it doesn't actually fix the cited PR?

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

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

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.

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.

Doesn't snd_hdsp(4) need the same?

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.

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?

Please also add an MFC date for this.

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

Which controls do you get in mixer(8)?

@markj @devesas.campos_gmail.com D59055

In D58889#1353814, @devesas.campos_gmail.com wrote:

In D58889#1352129, @devesas.campos_gmail.com wrote:

Will try to do more extensive testing tomorrow.

Tested with the vchiq_test utility from rpi-userland in mode bulk, functional, ping and timer and they all succeeded.

In D56845#1349251, @fernape wrote:

I would argue that the user might not know it. "Invalid argument" is barely descriptive IMHO. If mixer knows the problem is that the supplied device number is negative, why not tell the user the exact problem instead of trusting the user "should know" something?

bcmchan_trigger() calls chn_intr() which causes a lock recursion because chn_trigger() calls bcmchan_trigger() with the lock held already. However, chn_intr() is not meant to be called there in the CHANNEL_TRIGGER() method, so the code here is wrong in the first place. Does triggering properly if the chn_intr() call is removed from bcmchan_trigger()? I see that it also calls bcm2835_worker_play_start(), which should be enough?

To be fair, I don't think these changes are meaningful. mixer(8) will fail for all the cases you are trying to catch anyway, and I think the messages are clear enough already.

Should we commit this?

Continued here: https://ron-dev.freebsd.org/FreeBSD/src/pulls/63

I don't have a device like that right now. But everything works as expected? Playback, recording, latencies, etc?

Tested with my HW, no problems. Is there some particular way you are testing this?

This looks good. The only thing I am still skeptical about are the various casts you've introduced, not sure if they are needed. I will be testing the patch as well.