On Sat, Feb 05, 2022 at 02:46:53PM +0100, Anton Lindqvist wrote:
> 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 <[email protected]>
> > >
> > > 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.
> >
> > Cheers,
> >
> > Mark
>
> Thanks for the input. Another approach would be to correlate the audio
> and wskbd device. Here's a proof of concept diff that propagates a
> "cookie" from the common point of attachment, being uhub for my
> particular device. The same cookie is later used to find the matching
> audio device while handling volume keys. The found audio device could
> probably be cached inside `struct wskbd_softc' if it grabs a device
> reference. I'm taking quite a few shortcuts here to avoid changing the
> prototypes of commonly used functions.
Polished diff. I'm omitting a necessary refactoring commit making
audio_attach_mi() accept a new cookie argument.
diff --git sys/dev/audio.c sys/dev/audio.c
index 64696025a50..f2b40637771 100644
--- sys/dev/audio.c
+++ sys/dev/audio.c
@@ -28,6 +28,7 @@
#include <sys/audioio.h>
#include <dev/audio_if.h>
#include <dev/mulaw.h>
+#include <dev/wscons/wskbdvar.h> /* struct wskbd_audio */
#include "audio.h"
#include "wskbd.h"
@@ -96,6 +97,8 @@ struct wskbd_vol
#define WSKBD_MUTE_DISABLE 2
#define WSKBD_MUTE_ENABLE 3
};
+
+int wskbd_set_mixervolume_unit(int, long, long);
#endif
/*
@@ -2455,10 +2458,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,13 +2568,55 @@ wskbd_set_mixermute(long mute, long out)
return 0;
}
+int
+wskbd_set_mixervolume_sc(struct wskbd_audio *audio, long dir, long out)
+{
+ if (audio->dev != NULL) {
+ if ((audio->dev->dv_flags & DVF_ACTIVE) == 0) {
+ /* Audio device gone, fallback to audio0. */
+ device_unref(audio->dev);
+ audio->dev = NULL;
+ audio->unit = 0;
+ }
+ } else if (audio->cookie != NULL) {
+ void *cookie;
+ int i;
+
+ cookie = audio->cookie;
+ audio->cookie = NULL;
+ for (i = 0; i < audio_cd.cd_ndevs; i++) {
+ struct audio_softc *sc;
+
+ sc = (struct audio_softc *)device_lookup(&audio_cd, i);
+ if (sc == NULL)
+ continue;
+ if (sc->cookie != cookie) {
+ device_unref(&sc->dev);
+ continue;
+ }
+
+ audio->dev = (struct device *)sc;
+ audio->unit = i;
+ break;
+ }
+ }
+
+ return wskbd_set_mixervolume_unit(audio->unit, dir, out);
+}
+
int
wskbd_set_mixervolume(long dir, long out)
+{
+ return wskbd_set_mixervolume_unit(0, dir, out);
+}
+
+int
+wskbd_set_mixervolume_unit(int unit, long dir, long out)
{
struct audio_softc *sc;
struct wskbd_vol *vol;
- sc = (struct audio_softc *)device_lookup(&audio_cd, 0);
+ sc = (struct audio_softc *)device_lookup(&audio_cd, unit);
if (sc == NULL)
return ENODEV;
vol = out ? &sc->spkr : &sc->mic;
diff --git sys/dev/usb/uaudio.c sys/dev/usb/uaudio.c
index 0a19d512a5c..d86019311e0 100644
--- sys/dev/usb/uaudio.c
+++ sys/dev/usb/uaudio.c
@@ -3841,7 +3841,7 @@ uaudio_attach(struct device *parent, struct device *self,
void *aux)
/* print a nice uaudio attach line */
uaudio_print(sc);
- audio_attach_mi(&uaudio_hw_if, sc, NULL, &sc->dev);
+ audio_attach_mi(&uaudio_hw_if, sc, arg->cookie, &sc->dev);
}
int
diff --git sys/dev/usb/ucc.c sys/dev/usb/ucc.c
index f23f32990bb..705a31b327b 100644
--- sys/dev/usb/ucc.c
+++ sys/dev/usb/ucc.c
@@ -104,7 +104,7 @@ void ucc_attach(struct device *, struct device *,
void *);
int ucc_detach(struct device *, int);
void ucc_intr(struct uhidev *, void *, u_int);
-void ucc_attach_wskbd(struct ucc_softc *);
+void ucc_attach_wskbd(struct ucc_softc *, void *);
int ucc_enable(void *, int);
void ucc_set_leds(void *, int);
int ucc_ioctl(void *, u_long, caddr_t, int, struct proc *);
@@ -680,7 +680,7 @@ ucc_attach(struct device *parent, struct device *self, void
*aux)
/* Cannot load an empty map. */
if (sc->sc_maplen > 0)
- ucc_attach_wskbd(sc);
+ ucc_attach_wskbd(sc, uha->uaa->cookie);
}
int
@@ -772,7 +772,7 @@ unknown:
}
void
-ucc_attach_wskbd(struct ucc_softc *sc)
+ucc_attach_wskbd(struct ucc_softc *sc, void *cookie)
{
static const struct wskbd_accessops accessops = {
.enable = ucc_enable,
@@ -784,6 +784,7 @@ ucc_attach_wskbd(struct ucc_softc *sc)
.keymap = &sc->sc_keymap,
.accessops = &accessops,
.accesscookie = sc,
+ .audiocookie = cookie,
};
sc->sc_keydesc[0].name = KB_US;
diff --git sys/dev/usb/usb_subr.c sys/dev/usb/usb_subr.c
index 7d8480f0f01..fc5808bfeb1 100644
--- sys/dev/usb/usb_subr.c
+++ sys/dev/usb/usb_subr.c
@@ -839,6 +839,7 @@ usbd_status
usbd_probe_and_attach(struct device *parent, struct usbd_device *dev, int port,
int addr)
{
+ static char *cookie = 0;
struct usb_attach_arg uaa;
usb_device_descriptor_t *dd = &dev->ddesc;
int i, confi, nifaces;
@@ -860,6 +861,7 @@ usbd_probe_and_attach(struct device *parent, struct
usbd_device *dev, int port,
uaa.vendor = UGETW(dd->idVendor);
uaa.product = UGETW(dd->idProduct);
uaa.release = UGETW(dd->bcdDevice);
+ uaa.cookie = ++cookie;
/* First try with device specific drivers. */
DPRINTF(("usbd_probe_and_attach trying device specific drivers\n"));
diff --git sys/dev/usb/usbdi.h sys/dev/usb/usbdi.h
index 05209bcb809..15404ffe9bd 100644
--- sys/dev/usb/usbdi.h
+++ sys/dev/usb/usbdi.h
@@ -227,6 +227,7 @@ struct usb_attach_arg {
int usegeneric;
struct usbd_interface **ifaces;/* all interfaces */
int nifaces; /* number of interfaces */
+ void *cookie;
};
/* Match codes. */
diff --git sys/dev/wscons/wskbd.c sys/dev/wscons/wskbd.c
index 5693a5b614e..e0a73830a2d 100644
--- sys/dev/wscons/wskbd.c
+++ sys/dev/wscons/wskbd.c
@@ -169,6 +169,10 @@ struct wskbd_softc {
int sc_refcnt;
u_char sc_dying; /* device is being detached */
+
+#if NAUDIO > 0
+ struct wskbd_audio sc_audio;
+#endif
};
#define MOD_SHIFT_L (1 << 0)
@@ -307,7 +311,7 @@ static struct wskbd_internal wskbd_console_data;
void wskbd_update_layout(struct wskbd_internal *, kbd_t);
#if NAUDIO > 0
-extern int wskbd_set_mixervolume(long, long);
+extern int wskbd_set_mixervolume_sc(struct wskbd_audio *, long, long);
#endif
void
@@ -395,6 +399,10 @@ wskbd_attach(struct device *parent, struct device *self,
void *aux)
timeout_set(&sc->sc_repeat_ch, wskbd_repeat, sc);
#endif
+#if NAUDIO > 0
+ sc->sc_audio.cookie = ap->audiocookie;
+#endif
+
sc->id->t_sc = sc;
sc->sc_accessops = ap->accessops;
@@ -608,6 +616,11 @@ wskbd_detach(struct device *self, int flags)
}
#endif
+#if NAUDIO > 0
+ if (sc->sc_audio.dev != NULL)
+ device_unref(sc->sc_audio.dev);
+#endif
+
if (sc->sc_isconsole) {
KASSERT(wskbd_console_device == sc);
wskbd_cndetach();
@@ -1766,13 +1779,13 @@ wskbd_translate(struct wskbd_internal *id, u_int type,
int value)
switch (ksym) {
#if NAUDIO > 0
case KS_AudioMute:
- wskbd_set_mixervolume(0, 1);
+ wskbd_set_mixervolume_sc(&sc->sc_audio, 0, 1);
return (0);
case KS_AudioLower:
- wskbd_set_mixervolume(-1, 1);
+ wskbd_set_mixervolume_sc(&sc->sc_audio, -1, 1);
return (0);
case KS_AudioRaise:
- wskbd_set_mixervolume(1, 1);
+ wskbd_set_mixervolume_sc(&sc->sc_audio, 1, 1);
return (0);
#endif
default:
diff --git sys/dev/wscons/wskbdvar.h sys/dev/wscons/wskbdvar.h
index 91bca39e30f..ce0d88d6350 100644
--- sys/dev/wscons/wskbdvar.h
+++ sys/dev/wscons/wskbdvar.h
@@ -61,6 +61,15 @@ struct wskbd_consops {
void (*debugger)(void *);
};
+/*
+ * Audio device associated with keyboard.
+ */
+struct wskbd_audio {
+ void *cookie;
+ struct device *dev;
+ int unit;
+};
+
/*
* Attachment information provided by wskbddev devices when attaching
* wskbd units.
@@ -71,6 +80,8 @@ struct wskbddev_attach_args {
const struct wskbd_accessops *accessops; /* access ops */
void *accesscookie; /* access cookie */
+
+ void *audiocookie;
};
#define WSKBDDEVCF_CONSOLE 0
