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.
diff --git sys/dev/audio.c sys/dev/audio.c
index ec52ee6ef01..f412e497edd 100644
--- sys/dev/audio.c
+++ sys/dev/audio.c
@@ -112,6 +112,7 @@ struct mixer_ev {
struct audio_softc {
struct device dev;
struct audio_hw_if *ops; /* driver funcs */
+ void *cookie; /* XXX */
void *arg; /* first arg to driver funcs */
int mode; /* bitmask of AUMODE_* */
int quiesce; /* device suspended */
@@ -1236,6 +1237,7 @@ audio_attach(struct device *parent, struct device *self,
void *aux)
}
#endif
sc->ops = ops;
+ sc->cookie = sa->cookie;
sc->arg = arg;
#if NWSKBD > 0
@@ -1474,6 +1476,24 @@ audio_attach_mi(struct audio_hw_if *ops, void *arg,
struct device *dev)
aa.type = AUDIODEV_TYPE_AUDIO;
aa.hwif = ops;
aa.hdl = arg;
+ aa.cookie = NULL;
+
+ /*
+ * attach this driver to the caller (hardware driver), this
+ * checks the kernel config and possibly calls audio_attach()
+ */
+ return config_found_sm(dev, &aa, audioprint, audio_submatch);
+}
+
+struct device *
+audio_attach_mi1(struct audio_hw_if *ops, void *arg, struct device *dev, void
*cookie)
+{
+ struct audio_attach_args aa;
+
+ aa.type = AUDIODEV_TYPE_AUDIO;
+ aa.hwif = ops;
+ aa.hdl = arg;
+ aa.cookie = cookie;
/*
* attach this driver to the caller (hardware driver), this
@@ -2452,10 +2472,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))
@@ -2566,6 +2582,37 @@ wskbd_set_mixermute(long mute, long out)
return 0;
}
+int
+wskbd_set_mixervolume1(void *cookie, long dir, long out)
+{
+ int minor;
+
+ 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;
+ if (sc->cookie != cookie) {
+ device_unref(&sc->dev);
+ 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);
+ return 0;
+ }
+
+ /* XXX probably want to fallback to device minor 0 */
+ return ENODEV;
+}
+
int
wskbd_set_mixervolume(long dir, long out)
{
diff --git sys/dev/audio_if.h sys/dev/audio_if.h
index 8b96211bced..70883b8650c 100644
--- sys/dev/audio_if.h
+++ sys/dev/audio_if.h
@@ -143,6 +143,7 @@ struct audio_attach_args {
int type;
void *hwif; /* either audio_hw_if * or midi_hw_if * */
void *hdl;
+ void *cookie;
};
#define AUDIODEV_TYPE_AUDIO 0
#define AUDIODEV_TYPE_MIDI 1
@@ -152,6 +153,7 @@ struct audio_attach_args {
/* Attach the MI driver(s) to the MD driver. */
struct device *audio_attach_mi(struct audio_hw_if *, void *, struct device *);
+struct device *audio_attach_mi1(struct audio_hw_if *, void *, struct device *,
void *);
int audioprint(void *, const char *);
int audio_blksz_bytes(int,
struct audio_params *, struct audio_params *, int);
diff --git sys/dev/usb/uaudio.c sys/dev/usb/uaudio.c
index 025892ddf04..ad5078d4000 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, &sc->dev);
+ audio_attach_mi1(&uaudio_hw_if, sc, &sc->dev, arg->cookie);
}
int
diff --git sys/dev/usb/ucc.c sys/dev/usb/ucc.c
index baccdb7f90a..618ded00f16 100644
--- sys/dev/usb/ucc.c
+++ sys/dev/usb/ucc.c
@@ -50,6 +50,7 @@ int ucc_debug = 1;
struct ucc_softc {
struct uhidev sc_hdev;
struct device *sc_wskbddev;
+ void *sc_cookie;
struct mutex sc_mtx;
@@ -676,6 +677,7 @@ ucc_attach(struct device *parent, struct device *self, void
*aux)
int error, repid, size;
mtx_init(&sc->sc_mtx, IPL_USB);
+ sc->sc_cookie = uha->uaa->cookie;
sc->sc_mode = WSKBD_TRANSLATED;
sc->sc_last_translate = -1;
task_set(&sc->sc_proc.p_task, ucc_task, sc);
@@ -813,6 +815,7 @@ ucc_attach_wskbd(struct ucc_softc *sc)
.keymap = &sc->sc_keymap,
.accessops = &accessops,
.accesscookie = sc,
+ .audiocookie = sc->sc_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..5553456c560 100644
--- sys/dev/wscons/wskbd.c
+++ sys/dev/wscons/wskbd.c
@@ -149,6 +149,8 @@ struct wskbd_softc {
const struct wskbd_accessops *sc_accessops;
void *sc_accesscookie;
+ void *sc_audiocookie;
+
int sc_ledstate;
int sc_isconsole;
@@ -307,7 +309,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_mixervolume1(void *, long, long);
#endif
void
@@ -399,6 +401,7 @@ wskbd_attach(struct device *parent, struct device *self,
void *aux)
sc->sc_accessops = ap->accessops;
sc->sc_accesscookie = ap->accesscookie;
+ sc->sc_audiocookie = ap->audiocookie;
sc->sc_repeating = 0;
sc->sc_translating = 1;
sc->sc_ledstate = -1; /* force update */
@@ -1766,13 +1769,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_mixervolume1(sc->sc_audiocookie, 0, 1);
return (0);
case KS_AudioLower:
- wskbd_set_mixervolume(-1, 1);
+ wskbd_set_mixervolume1(sc->sc_audiocookie, -1, 1);
return (0);
case KS_AudioRaise:
- wskbd_set_mixervolume(1, 1);
+ wskbd_set_mixervolume1(sc->sc_audiocookie, 1, 1);
return (0);
#endif
default:
diff --git sys/dev/wscons/wskbdvar.h sys/dev/wscons/wskbdvar.h
index 91bca39e30f..0cb7067eeba 100644
--- sys/dev/wscons/wskbdvar.h
+++ sys/dev/wscons/wskbdvar.h
@@ -71,6 +71,8 @@ struct wskbddev_attach_args {
const struct wskbd_accessops *accessops; /* access ops */
void *accesscookie; /* access cookie */
+
+ void *audiocookie; /* XXX */
};
#define WSKBDDEVCF_CONSOLE 0
