On Sat, Feb 05, 2022 at 12:28:08PM +0100, Mark Kettenis wrote:
> > Date: Sat, 5 Feb 2022 09:29:42 +0100
> > From: Anton Lindqvist <an...@basename.se>
> >
> > Hi,
> > I recently got a USB headset with physical volume buttons, handled by
> > ucc(4). However, after enabling the device in sndiod the volume buttons
> > does not cause the volume to change. Turns out wskbd_set_mixervolume()
> > is only propagating volume changes to first attached audio device. Is
> > their any good not to consider all attached audio devices?
>
> I think this is tricky. The mixer values of different audio devices
> may start out differently and may have different granularity and
> probably operate on a different scale. This may lead to situations
> where as you turn the volume up and down, the relative output volume
> between devices changes considerably. I also think that your
> implementation will unmute all audio devices as soon as you touch the
> volume control buttons, which is probably not desirable.
>
> Thinking about other ways to do this, we could:
>
> - Add a knob that allows the user to control which audio device is
> controlled by the volume control buttons. The choice could include
> "none" and "all" as well as the individual devices.
>
> - Add infrastructure to bind specific keyboards to specific audio
> devices, a bit like how we support binding specific wskbd devices to
> specific wsdisplay devices.
>
> The first suggestion is probably relatively easy to achieve. The
> implementation of the latter would defenitely need more thought and
> discussion.
>
> The "none" choice above would (partly) solve another issue where
> userland applications see the key presses and act upon them even
> though the kernel already did the volume adjustment.
There is a 3rd option of passing the information to sndiod and let it do
the volume scaling.
> Cheers,
>
> Mark
>
> > The diff below gives me the desired behavior by propagating volume
> > changes to all attached audio devices.
> >
> > diff --git sys/dev/audio.c sys/dev/audio.c
> > index ec52ee6ef01..ca19557d39e 100644
> > --- sys/dev/audio.c
> > +++ sys/dev/audio.c
> > @@ -2452,10 +2452,6 @@ wskbd_mixer_init(struct audio_softc *sc)
> > };
> > int i;
> >
> > - if (sc->dev.dv_unit != 0) {
> > - DPRINTF("%s: not configuring wskbd keys\n", DEVNAME(sc));
> > - return;
> > - }
> > for (i = 0; i < sizeof(spkr_names) / sizeof(spkr_names[0]); i++) {
> > if (wskbd_initvol(sc, &sc->spkr,
> > spkr_names[i].cn, spkr_names[i].dn))
> > @@ -2569,19 +2565,26 @@ wskbd_set_mixermute(long mute, long out)
> > int
> > wskbd_set_mixervolume(long dir, long out)
> > {
> > - struct audio_softc *sc;
> > - struct wskbd_vol *vol;
> > + int error = ENODEV;
> > + int minor;
> >
> > - sc = (struct audio_softc *)device_lookup(&audio_cd, 0);
> > - if (sc == NULL)
> > - return ENODEV;
> > - vol = out ? &sc->spkr : &sc->mic;
> > - if (dir == 0)
> > - vol->mute_pending ^= WSKBD_MUTE_TOGGLE;
> > - else
> > - vol->val_pending += dir;
> > - if (!task_add(systq, &sc->wskbd_task))
> > - device_unref(&sc->dev);
> > - return 0;
> > + for (minor = 0; minor < audio_cd.cd_ndevs; minor++) {
> > + struct audio_softc *sc;
> > + struct wskbd_vol *vol;
> > +
> > + sc = (struct audio_softc *)device_lookup(&audio_cd, minor);
> > + if (sc == NULL)
> > + continue;
> > + vol = out ? &sc->spkr : &sc->mic;
> > + if (dir == 0)
> > + vol->mute_pending ^= WSKBD_MUTE_TOGGLE;
> > + else
> > + vol->val_pending += dir;
> > + if (!task_add(systq, &sc->wskbd_task))
> > + device_unref(&sc->dev);
> > + error = 0;
> > + }
> > +
> > + return error;
> > }
> > #endif /* NWSKBD > 0 */
> >
> >
>
--
:wq Claudio
