make(1): document VPATH variable
I just noticed that our make(1) supports VPATH, but is not documented. OK? Index: make.1 === RCS file: /cvs/src/usr.bin/make/make.1,v diff -u -p -r1.141 make.1 --- make.1 10 Aug 2023 10:56:34 - 1.141 +++ make.1 25 Oct 2023 10:45:32 - @@ -832,6 +832,10 @@ It should not be used; see the section below. .It Va .VARIABLES List of all the names of global variables that have been set. +.It Va VPATH +A colon separated list of directories appended to the search path (see +.Va .PATH ) . +Supported for compatibility with other implementations. .El .Pp Variable expansion may be modified to select or modify each word of the
Re: Please test; midi(4): make midi{read,write}_filtops mp safe
On Sun, Sep 24, 2023 at 11:03:54PM +0300, Vitaliy Makkoveev wrote: > Please test this diff, I have no midi(4) devices. > > midi(4) already uses `audio_lock' mutex(9) for filterops, but they are > still kernel locked. Wipe out old selwakeup API and make them MP safe. > knote_locked(9) will not grab kernel lock, so call it directly from > interrupt handlers instead of scheduling software interrupts. > Works for me (tested with a usb device), pulling the usb cable properly wakes up any process waiting for input/output. ok ratchov@ with the two changes below > @@ -531,20 +518,8 @@ midiattach(struct device *parent, struct > } > #endif > > - sc->inbuf.softintr = softintr_establish(IPL_SOFTMIDI, > - midi_buf_wakeup, >inbuf); > - if (sc->inbuf.softintr == NULL) { > - printf("%s: can't establish input softintr\n", DEVNAME(sc)); > - return; > - } > - > - sc->outbuf.softintr = softintr_establish(IPL_SOFTMIDI, > - midi_buf_wakeup, >outbuf); > - if (sc->outbuf.softintr == NULL) { > - printf("%s: can't establish output softintr\n", DEVNAME(sc)); > - softintr_disestablish(sc->inbuf.softintr); > - return; > - } > + klist_init_mutex(>inbuf.klist, _lock); > + klist_init_mutex(>outbuf.klist, _lock); > > sc->hw_if = hwif; > sc->hw_hdl = hdl; > @@ -580,27 +555,19 @@ mididetach(struct device *self, int flag > KERNEL_ASSERT_LOCKED(); > if (sc->flags & FREAD) { > wakeup(>inbuf.blocking); > - mtx_enter(_lock); > - selwakeup(>inbuf.sel); > - mtx_leave(_lock); > + knote(>inbuf.klist, 0); AFAIU, the knote() calls are not necessary because klist_invalidate() already does the job. > } > if (sc->flags & FWRITE) { > wakeup(>outbuf.blocking); > - mtx_enter(_lock); > - selwakeup(>outbuf.sel); > - mtx_leave(_lock); > + knote(>outbuf.klist, 0); > } > sc->hw_if->close(sc->hw_hdl); > sc->flags = 0; > } > > - klist_invalidate(>inbuf.sel.si_note); > - klist_invalidate(>outbuf.sel.si_note); > + klist_invalidate(>inbuf.klist); > + klist_invalidate(>outbuf.klist); > Could you add two klist_free() calls here? They are simple asserts, but they'd make the attach/detach functions more midi(4) would look more like audio(4).
Re: Please test; midi(4): make midi{read,write}_filtops mp safe
On Mon, Sep 25, 2023 at 04:58:56PM +0300, Vitaliy Makkoveev wrote: > On Mon, Sep 25, 2023 at 05:39:34AM +, Visa Hankala wrote: > > On Sun, Sep 24, 2023 at 11:03:54PM +0300, Vitaliy Makkoveev wrote: > > > Please test this diff, I have no midi(4) devices. > > > > > > midi(4) already uses `audio_lock' mutex(9) for filterops, but they are > > > still kernel locked. Wipe out old selwakeup API and make them MP safe. > > > knote_locked(9) will not grab kernel lock, so call it directly from > > > interrupt handlers instead of scheduling software interrupts. > > > > https://marc.info/?l=openbsd-tech=167604232828221 has minor takeaways > > if you pay attention. > > > > The only significant difference is mididetach() where you did not replaced > selwakeup() with knote(). Did I missed something? > > @@ -577,30 +562,20 @@ mididetach(struct device *self, int flag >* in read/write/ioctl, which return EIO. >*/ > if (sc->flags) { > - KERNEL_ASSERT_LOCKED(); > - if (sc->flags & FREAD) { > + if (sc->flags & FREAD) > wakeup(>inbuf.blocking); > - mtx_enter(_lock); > - selwakeup(>inbuf.sel); > - mtx_leave(_lock); > - } IIRC, klist_invalidate() wakes up the process, so the selwakeup() section should be removed. Does this sound correct? This is how audio(4) detaches, BTW.
AMD 17h/1xh HD Audio testers wanted!
If you've an azalia(4) attaching as "AMD 17h/1xh HD Audio", please test this diff and report regressions. Especially audio lock ups that require reboot. IIRC, MSI was disabled few years ago to "fix" such lockups, and now this diff suggests we need MSI on certain boards. Context and diff below: - Forwarded message from Andreas Bartelt - Date: Sat, 4 Mar 2023 16:12:22 +0100 From: Andreas Bartelt To: Alexandre Ratchov , b...@openbsd.org Subject: Re: audio(4) output doesn't work yet on ASUS ProArt X670E-CREATOR WIFI mainboard (ALC1220 CODEC) User-Agent: Mozilla/5.0 (X11; OpenBSD amd64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 On 2/27/23 6:41 PM, Andreas Bartelt wrote: > On 2/27/23 2:40 PM, Alexandre Ratchov wrote: > > On Sat, Feb 25, 2023 at 05:20:53PM +0100, Andreas Bartelt wrote: > > > Hi, > > > > > > I've tested a recent OpenBSD snapshot of CURRENT on an ASUS ProArt > > > X670E-CREATOR WIFI mainboard. According to the information > > > provided by ASUS, > > > this mainboard features a "Realtek S1220A CODEC" which attaches as > > > Realtek > > > ALC1220 on OpenBSD -- however, audio output (tested with > > > headphones on the > > > line out connector) doesn't work there yet. Applications (e.g., mplayer, > > > mpg123) hang and I can hear no sound. > > > > > > [I don't know if this helps but I previously also had access to an > > > ASUS ROG > > > STRIX B550-E GAMING mainboard which, according to ASUS, also features an > > > S1220A CODEC which also attaches as Realtek ALC1220 on OpenBSD -- audio > > > output (tested on the line out connector) works there without problems.] > > > > > > In order to verify that the new mainboard doesn't have a physical defect > > > with regard to the line out audio connector, I've also tested a > > > FreeBSD 13.2 > > > BETA3 snapshot on the ASUS ProArt X670E-CREATOR WIFI mainboard. > > > Audio output > > > worked there out-of-the-box, so this might be a fixable problem on > > > OpenBSD. > > > > > > I've found some info with regard to audio debugging at > > > https://www.openbsd.org/faq/faq13.html#audioprob . While running > > > # cat > /dev/audio0 < /dev/zero > > > play.bytes doesn't increase at all: > > > # audioctl play.{bytes,errors} > > > play.bytes=0 > > > play.errors=0 > > > > > > > mixerctl shows that the host manages communicate with the codec, but > > above lines suggest that DMA doesn't start. Could you check if there > > are any audio-related options in the BIOS? Especially, if there's an > > option to disable the microphone (or "recording" or alike), please > > enable it. > > There's no microphone or recording specific options available. I could > only identify a single audio related configuration option. Under > Advanced\Onboard Devices Configuration: enable/disable "HD Audio > Controller" (description says Enable/Disable Azalia HD Audio). It does > exactly that, i.e., disabling this option removes the azalia1 device from > OpenBSD's dmesg. > > With this option enabled again, mp3 playback works with FreeBSD but hangs > with OpenBSD -- same BIOS config. > I've made audio work on the ASUS ProArt X670E-CREATOR WIFI mainboard, simply by enabling msi. azalia1 at pci21 dev 0 function 6 "AMD 17h/1xh HD Audio" rev 0x00: msi azalia1: codecs: Realtek ALC1220 audio0 at azalia1 The following diff fixes the problem: Index: src/sys/dev/pci/azalia.c === RCS file: /cvs/src/sys/dev/pci/azalia.c,v retrieving revision 1.283 diff -u -p -r1.283 azalia.c --- src/sys/dev/pci/azalia.c21 Feb 2023 13:42:59 - 1.283 +++ src/sys/dev/pci/azalia.c4 Mar 2023 15:02:31 - @@ -554,7 +554,6 @@ azalia_pci_attach(struct device *parent, if (PCI_VENDOR(sc->pciid) == PCI_VENDOR_AMD) { switch (PCI_PRODUCT(sc->pciid)) { case PCI_PRODUCT_AMD_17_HDA: - case PCI_PRODUCT_AMD_17_1X_HDA: case PCI_PRODUCT_AMD_HUDSON2_HDA: pa->pa_flags &= ~PCI_FLAGS_MSI_ENABLED; } OK? - End forwarded message -
Fix sndiod(8) dropping randomly connections
Sometimes sndiod closes the device sponteneously and, unless -F is used, programs need to be restarted. If -F is used, sndiod will switch to another device (ex. from usb headphones to internal speakers), which might be a privacy problem. The cause is that libsndio uses the kernel underrun counters to compensate for the silence the audio(4) driver inserts when userland provides no samples to play. This hack is to keeps underruns short in order to be less unnoticeable. Kernel counters must be accurate, otherwise the code trying to compensate for the underruns may create a deadlock making sndiod enter an infinite loop. sndiod detects such loops and closes the device. Unfortunately the kernel counters are not reliable, their logic is fragile, device-dependent and after 10+ years of attemps to use them we still get the deadlocks. The diff below makes libsndio simply restart the device if the program underruns. For restarting to be invisible to the program, libsndio saves and restores the program state. Doing so, requires to play additional silence after the underrun, which makes it *more noticeable*. Please test and report regressions like sndiod crashes, unexpected connection drops, etc. OK? Index: sio_sun.c === RCS file: /cvs/src/lib/libsndio/sio_sun.c,v retrieving revision 1.30 diff -u -p -r1.30 sio_sun.c --- sio_sun.c 27 Dec 2022 17:10:07 - 1.30 +++ sio_sun.c 27 Feb 2023 15:07:00 - @@ -42,8 +42,7 @@ struct sio_sun_hdl { int filling; unsigned int ibpf, obpf;/* bytes per frame */ unsigned int ibytes, obytes;/* bytes the hw transferred */ - unsigned int ierr, oerr;/* frames the hw dropped */ - int idelta, odelta; /* position reported to client */ + int idelta, odelta; /* position not reported yet */ }; static void sio_sun_close(struct sio_hdl *); @@ -370,8 +369,6 @@ sio_sun_start(struct sio_hdl *sh) hdl->ibpf = hdl->sio.par.rchan * hdl->sio.par.bps; hdl->ibytes = 0; hdl->obytes = 0; - hdl->ierr = 0; - hdl->oerr = 0; hdl->idelta = 0; hdl->odelta = 0; @@ -516,6 +513,62 @@ sio_sun_write(struct sio_hdl *sh, const } static int +sio_sun_xrun(struct sio_sun_hdl *hdl) +{ + int clk; + int wsil, rdrop, cmove; + int rbpf, rround; + int wbpf; + + DPRINTFN(2, "%s: 1: sil = %d, rdrop = %d, odelta = %d, idelta = %d\n", + __func__, hdl->sio.wsil, hdl->sio.rdrop, hdl->odelta, hdl->idelta); +#ifdef DEBUG + if (_sndio_debug >= 2) + _sio_printpos(>sio); +#endif + + /* +* we assume rused/wused are zero if rec/play modes are not +* selected. This allows us to keep the same formula for all +* modes, provided we set rbpf/wbpf to 1 to avoid division by +* zero. +* +* to understand the formula, draw a picture :) +*/ + rbpf = (hdl->sio.mode & SIO_REC) ? hdl->ibpf : 1; + wbpf = (hdl->sio.mode & SIO_PLAY) ? hdl->obpf : 1; + rround = hdl->sio.par.round * rbpf; + + clk = hdl->sio.cpos % hdl->sio.par.round; + rdrop = (clk * rbpf - hdl->sio.rused) % rround; + if (rdrop < 0) + rdrop += rround; + cmove = (rdrop + hdl->sio.rused) / rbpf; + wsil = cmove * wbpf + hdl->sio.wused; + + DPRINTFN(2, "%s: wsil = %d, cmove = %d, rdrop = %d\n", + __func__, wsil, cmove, rdrop); + + if (!sio_sun_flush(>sio)) + return 0; + if (!sio_sun_start(>sio)) + return 0; + if (hdl->sio.mode & SIO_PLAY) { + hdl->odelta -= cmove; + hdl->sio.wsil = wsil; + } + if (hdl->sio.mode & SIO_REC) { + hdl->idelta -= cmove; + hdl->sio.rdrop = rdrop; + } + + DPRINTFN(2, "%s: 2: wsil = %d, rdrop = %d, odelta = %d, idelta = %d\n", + __func__, hdl->sio.wsil, hdl->sio.rdrop, hdl->odelta, hdl->idelta); + + return 1; +} + +static int sio_sun_nfds(struct sio_hdl *hdl) { return 1; @@ -536,6 +589,7 @@ sio_sun_pollfd(struct sio_hdl *sh, struc hdl->sio.eof = 1; return 0; } + DPRINTFN(2, "sio_sun_pollfd: started\n"); _sio_onmove_cb(>sio, 0); } return 1; @@ -546,67 +600,56 @@ sio_sun_revents(struct sio_hdl *sh, stru { struct sio_sun_hdl *hdl = (struct sio_sun_hdl *)sh; struct audio_pos ap; - int dierr = 0, doerr = 0, offset, delta; + int delta; int revents = pfd->revents; if ((pfd->revents & POLLHUP) || (pfd->revents & (POLLIN | POLLOUT)) == 0) return pfd->revents; + if (ioctl(hdl->fd, AUDIO_GETPOS, ) == -1) { DPERROR("sio_sun_revents: GETPOS"); hdl->sio.eof = 1; return POLLHUP;
Re: [PATCH] Azalia bug in codec Realtek ALC892
On Tue, Jan 03, 2023 at 09:51:46PM -0400, Jose Maldonado wrote: > Hi, everyone! > > Right now I'm using OBSD 7.2 (my first time in OBSD) and the only issue is > my onboard audio. The audio randomly stuck (listen music or not, with high > CPU use or not) and only a hard reboot give me back audio to next stuck. > > In dmesg donĀ“t show nothing, but sndiod - give me this output: > > * snd/0 watchdog timeout * > > The error appear in debug info and my audio is gone. The issue is not new > more info in this links: > > https://www.reddit.com/r/openbsd/comments/mybklx/keep_losing_audio_with_azalia_on_current/ > > https://www.mail-archive.com/tech@openbsd.org/msg46701.html > > https://deftly.net/posts/2018-10-15-openbsd-on-lenovo-a485.html > > With this info I realized this patch (directly on OBSD-stable source, sorry) > and...work! Not audio problems in my PC using this patch (one day testing, > multiple apps using audio, not stuck/not latency problems). > > If anybody can test, verify and give OK to this simple patch, I would > appreciate it (and I'm sure others would too, specially living in -release > versions). > > PATCH: > > Index: azalia_codec.c > === > RCS file: /cvs/src/sys/dev/pci/azalia_codec.c,v > retrieving revision 1.189 > diff -u -p -u -r1.189 azalia_codec.c > --- azalia_codec.c8 Sep 2022 01:35:39 - 1.189 > +++ azalia_codec.c2 Jan 2023 19:42:28 - > @@ -312,6 +312,9 @@ azalia_codec_init_vtbl(codec_t *this) > break; > case 0x10ec0892: > this->name = "Realtek ALC892"; > + if (this->subid == 0x1462ec95) { > + this->qrks |= AZ_QRK_WID_CDIN_1C | AZ_QRK_WID_BEEP_1D; > + } > break; > case 0x10ec0897: > this->name = "Realtek ALC897"; > > Your device seems to stop because the azalia pci host stops (probably interrupts stop and a reboot is needed), while this diff is about the azalia codec (i.e. about handling the "analog" parts). So, I'm very surprised this fixes the random audio hangs (though any strange interaction between the host and the codec can't be excluded). Are there other changes that may have fixed audio? Any BIOS tweaks? Suspend-resume cycles? Any devices disabled meanwhile? Could you test with and without the diff applied in exactly the same conditions? These hangs are a longstanding problem (and hard to debug because of the randomness of the hangs), so I'm very intested in understanding what's causing it. Thanks
Re: audioctl: display variables periodically
On Sat, Dec 10, 2022 at 09:39:41AM +, Edd Barrett wrote: > I agree with ratchov that in this instance, precise timing isn't important. > So, are we OK with this or there are still objections?
Re: audioctl: display variables periodically
On Fri, Dec 09, 2022 at 12:43:31PM -0600, Scott Cheloha wrote: > On Fri, Dec 09, 2022 at 12:10:59PM +0100, Alexandre Ratchov wrote: > > This diff adds an option to display variables periodically. Basically > > it replaces this usage: > > > > while sleep 1; do audioctl play.errors; done > > > > by > > > > audioctl -w 1 play.errors > > > > The purpose of above audioctl commands is to debug underruns, so we > > don't want to fork a new process and reopen the device. This would > > trigger longer kernel code-paths and may cause additional underruns > > than the ones being investigated. > > > > OK? > > I like the idea, but I would like to tweak some things. > > - Add [-w wait] to the first synoptic form in the manpage. It's legal > to do e.g. > > # audioctl -w 1 > done > - Call the variable "wait" in audioctl.c to match the manpage. > done (used wait_sec, as there's a global wait() symbol). > - Update usagestr to mention [-w wait]. > done > - When polling variables periodically, it's better to use setitimer(2) > and sigsuspend(2) instead of sleep(3). setitimer(2) keeps the period > from drifting. > > - Let the user SIGINT (^C) out of the program without returning an > error to the shell. > > I'm unsure about this one, but it seems logical to give the user a > way to gracefully terminate the program. You say in the manpage that > the program will continue printing until it is interrupted. > I just tried these. Synchronizing the display to a clock might make sense if it was the sound card's clock, but here the result was boiler with no benefit. The intent of -w is to just show the variables from time to time, so keeping the code trivial is more important, IMHO. I've added a comment to say so. About ^C, I've changed the man page text to "audioctl will display variables forever." which implies that ^C is out of the scope. Index: audioctl.8 === RCS file: /cvs/src/usr.bin/audioctl/audioctl.8,v retrieving revision 1.4 diff -u -p -r1.4 audioctl.8 --- audioctl.8 23 Apr 2020 00:16:59 - 1.4 +++ audioctl.8 10 Dec 2022 05:50:05 - @@ -35,9 +35,11 @@ .Sh SYNOPSIS .Nm audioctl .Op Fl f Ar file +.Op Fl w Ar wait .Nm audioctl .Op Fl n .Op Fl f Ar file +.Op Fl w Ar wait .Ar name ... .Nm audioctl .Op Fl nq @@ -59,6 +61,12 @@ The default is Suppress printing of the variable name. .It Fl q Suppress all output when setting a variable. +.It Fl w Ar wait +Pause +.Ar wait +seconds between each display. +.Nm +will display variables forever. .It Ar name Ns = Ns Ar value Attempt to set the specified variable .Ar name @@ -130,10 +138,10 @@ audio control devices audio devices .El .Sh EXAMPLES -Display the number of bytes of silence inserted during play buffer -underruns since device started: +Once per-second, display the number of bytes of silence inserted due to buffer +underruns (since the device started playback): .Bd -literal -offset indent -# audioctl play.errors +# audioctl -w 1 play.errors .Ed .Pp Use signed 24-bit samples and 44100Hz sample rate: Index: audioctl.c === RCS file: /cvs/src/usr.bin/audioctl/audioctl.c,v retrieving revision 1.43 diff -u -p -r1.43 audioctl.c --- audioctl.c 12 Jul 2021 15:09:19 - 1.43 +++ audioctl.c 10 Dec 2022 05:50:05 - @@ -43,6 +43,7 @@ struct field { #define STR2 #define ENC3 int type; + int show; int set; } fields[] = { {"name",,NULL, STR}, @@ -63,11 +64,11 @@ struct field { }; const char usagestr[] = - "usage: audioctl [-f file]\n" - " audioctl [-n] [-f file] name ...\n" + "usage: audioctl [-f file] [-w wait_sec]\n" + " audioctl [-n] [-f file] [-w wait_sec] name ...\n" " audioctl [-nq] [-f file] name=value ...\n"; -int fd, show_names = 1, quiet = 0; +int fd, show_names = 1, quiet = 0, wait_sec = 0; /* * parse encoding string (examples: s8, u8, s16, s16le, s24be ...) @@ -198,20 +199,9 @@ audio_main(int argc, char **argv) char *lhs, *rhs; int set = 0; - if (ioctl(fd, AUDIO_GETSTATUS, ) == -1) - err(1, "AUDIO_GETSTATUS"); - if (ioctl(fd, AUDIO_GETDEV, ) == -1) - err(1, "AUDIO_GETDEV"); - if (ioctl(fd, AUDIO_GETPAR, ) == -1) - err(1, "AUDIO_GETPAR"); - if (ioctl(fd, AUDIO_GETPOS, ) == -1) - err(1, "AUDIO_GETPOS"); if (argc == 0) { - for (f = fields; f->name != NULL; f++) { - printf("%s=", f->nam
Re: audioctl: display variables periodically
On Fri, Dec 09, 2022 at 04:24:03PM +, Edd Barrett wrote: > On Fri, Dec 09, 2022 at 12:10:59PM +0100, Alexandre Ratchov wrote: > > -Display the number of bytes of silence inserted during play buffer > > -underruns since device started: > > +Once per second, display the number of bytes of silence inserted > > +during play buffer underruns since device started: > > Would it read better as: > > "Once per-second, display the number of bytes of silence inserted due to > buffer > underruns (since the device started playback):" > > And don't forget to update the usage message too :) > Thanks. New diff, with above suggestions plus the fix for the missing Ar tag in the synopsis. Index: audioctl.8 === RCS file: /cvs/src/usr.bin/audioctl/audioctl.8,v retrieving revision 1.4 diff -u -p -u -p -r1.4 audioctl.8 --- audioctl.8 23 Apr 2020 00:16:59 - 1.4 +++ audioctl.8 9 Dec 2022 17:40:21 - @@ -37,6 +37,7 @@ .Op Fl f Ar file .Nm audioctl .Op Fl n +.Op Fl w Ar wait .Op Fl f Ar file .Ar name ... .Nm audioctl @@ -59,6 +60,12 @@ The default is Suppress printing of the variable name. .It Fl q Suppress all output when setting a variable. +.It Fl w Ar wait +Pause +.Ar wait +seconds between each display. +.Nm +will display variables until it is interrupted. .It Ar name Ns = Ns Ar value Attempt to set the specified variable .Ar name @@ -130,10 +137,10 @@ audio control devices audio devices .El .Sh EXAMPLES -Display the number of bytes of silence inserted during play buffer -underruns since device started: +Once per-second, display the number of bytes of silence inserted due to buffer +underruns (since the device started playback): .Bd -literal -offset indent -# audioctl play.errors +# audioctl -w 1 play.errors .Ed .Pp Use signed 24-bit samples and 44100Hz sample rate: Index: audioctl.c === RCS file: /cvs/src/usr.bin/audioctl/audioctl.c,v retrieving revision 1.43 diff -u -p -u -p -r1.43 audioctl.c --- audioctl.c 12 Jul 2021 15:09:19 - 1.43 +++ audioctl.c 9 Dec 2022 17:40:21 - @@ -43,6 +43,7 @@ struct field { #define STR2 #define ENC3 int type; + int show; int set; } fields[] = { {"name",,NULL, STR}, @@ -64,10 +65,10 @@ struct field { const char usagestr[] = "usage: audioctl [-f file]\n" - " audioctl [-n] [-f file] name ...\n" + " audioctl [-n] [-w wait] [-f file] name ...\n" " audioctl [-nq] [-f file] name=value ...\n"; -int fd, show_names = 1, quiet = 0; +int fd, show_names = 1, quiet = 0, period = 0; /* * parse encoding string (examples: s8, u8, s16, s16le, s24be ...) @@ -198,20 +199,9 @@ audio_main(int argc, char **argv) char *lhs, *rhs; int set = 0; - if (ioctl(fd, AUDIO_GETSTATUS, ) == -1) - err(1, "AUDIO_GETSTATUS"); - if (ioctl(fd, AUDIO_GETDEV, ) == -1) - err(1, "AUDIO_GETDEV"); - if (ioctl(fd, AUDIO_GETPAR, ) == -1) - err(1, "AUDIO_GETPAR"); - if (ioctl(fd, AUDIO_GETPOS, ) == -1) - err(1, "AUDIO_GETPOS"); if (argc == 0) { - for (f = fields; f->name != NULL; f++) { - printf("%s=", f->name); - print_field(f, f->raddr); - printf("\n"); - } + for (f = fields; f->name != NULL; f++) + f->show = 1; } AUDIO_INITPAR(); for (; argc > 0; argc--, argv++) { @@ -231,15 +221,38 @@ audio_main(int argc, char **argv) parse_field(f, f->waddr, rhs); f->set = 1; set = 1; - } else { + } else + f->show = 1; + } + + if (set && period) + errx(1, "Can't set variables periodically"); + + while (1) { + if (ioctl(fd, AUDIO_GETSTATUS, ) == -1) + err(1, "AUDIO_GETSTATUS"); + if (ioctl(fd, AUDIO_GETDEV, ) == -1) + err(1, "AUDIO_GETDEV"); + if (ioctl(fd, AUDIO_GETPAR, ) == -1) + err(1, "AUDIO_GETPAR"); + if (ioctl(fd, AUDIO_GETPOS, ) == -1) + err(1, "AUDIO_GETPOS"); + for (f = fields; f->name != NULL; f++) { + if (!f->show) + continue; if (show_names) printf("%s=", f->name);
audioctl: display variables periodically
This diff adds an option to display variables periodically. Basically it replaces this usage: while sleep 1; do audioctl play.errors; done by audioctl -w 1 play.errors The purpose of above audioctl commands is to debug underruns, so we don't want to fork a new process and reopen the device. This would trigger longer kernel code-paths and may cause additional underruns than the ones being investigated. OK? Index: audioctl.8 === RCS file: /cvs/src/usr.bin/audioctl/audioctl.8,v retrieving revision 1.4 diff -u -p -r1.4 audioctl.8 --- audioctl.8 23 Apr 2020 00:16:59 - 1.4 +++ audioctl.8 9 Dec 2022 10:57:05 - @@ -37,6 +37,7 @@ .Op Fl f Ar file .Nm audioctl .Op Fl n +.Op Fl w wait .Op Fl f Ar file .Ar name ... .Nm audioctl @@ -59,6 +60,12 @@ The default is Suppress printing of the variable name. .It Fl q Suppress all output when setting a variable. +.It Fl w Ar wait +Pause +.Ar wait +seconds between each display. +.Nm +will display variables until it is interrupted. .It Ar name Ns = Ns Ar value Attempt to set the specified variable .Ar name @@ -130,10 +137,10 @@ audio control devices audio devices .El .Sh EXAMPLES -Display the number of bytes of silence inserted during play buffer -underruns since device started: +Once per second, display the number of bytes of silence inserted +during play buffer underruns since device started: .Bd -literal -offset indent -# audioctl play.errors +# audioctl -w 1 play.errors .Ed .Pp Use signed 24-bit samples and 44100Hz sample rate: Index: audioctl.c === RCS file: /cvs/src/usr.bin/audioctl/audioctl.c,v retrieving revision 1.43 diff -u -p -r1.43 audioctl.c --- audioctl.c 12 Jul 2021 15:09:19 - 1.43 +++ audioctl.c 9 Dec 2022 10:57:05 - @@ -43,6 +43,7 @@ struct field { #define STR2 #define ENC3 int type; + int show; int set; } fields[] = { {"name",,NULL, STR}, @@ -67,7 +68,7 @@ const char usagestr[] = " audioctl [-n] [-f file] name ...\n" " audioctl [-nq] [-f file] name=value ...\n"; -int fd, show_names = 1, quiet = 0; +int fd, show_names = 1, quiet = 0, period = 0; /* * parse encoding string (examples: s8, u8, s16, s16le, s24be ...) @@ -198,20 +199,9 @@ audio_main(int argc, char **argv) char *lhs, *rhs; int set = 0; - if (ioctl(fd, AUDIO_GETSTATUS, ) == -1) - err(1, "AUDIO_GETSTATUS"); - if (ioctl(fd, AUDIO_GETDEV, ) == -1) - err(1, "AUDIO_GETDEV"); - if (ioctl(fd, AUDIO_GETPAR, ) == -1) - err(1, "AUDIO_GETPAR"); - if (ioctl(fd, AUDIO_GETPOS, ) == -1) - err(1, "AUDIO_GETPOS"); if (argc == 0) { - for (f = fields; f->name != NULL; f++) { - printf("%s=", f->name); - print_field(f, f->raddr); - printf("\n"); - } + for (f = fields; f->name != NULL; f++) + f->show = 1; } AUDIO_INITPAR(); for (; argc > 0; argc--, argv++) { @@ -231,15 +221,38 @@ audio_main(int argc, char **argv) parse_field(f, f->waddr, rhs); f->set = 1; set = 1; - } else { + } else + f->show = 1; + } + + if (set && period) + errx(1, "Can't set variables periodically"); + + while (1) { + if (ioctl(fd, AUDIO_GETSTATUS, ) == -1) + err(1, "AUDIO_GETSTATUS"); + if (ioctl(fd, AUDIO_GETDEV, ) == -1) + err(1, "AUDIO_GETDEV"); + if (ioctl(fd, AUDIO_GETPAR, ) == -1) + err(1, "AUDIO_GETPAR"); + if (ioctl(fd, AUDIO_GETPOS, ) == -1) + err(1, "AUDIO_GETPOS"); + for (f = fields; f->name != NULL; f++) { + if (!f->show) + continue; if (show_names) printf("%s=", f->name); print_field(f, f->raddr); printf("\n"); } + if (period == 0) + break; + sleep(period); } + if (!set) return; + if (ioctl(fd, AUDIO_SETPAR, ) == -1) err(1, "AUDIO_SETPAR"); if (ioctl(fd, AUDIO_GETPAR, ) == -1) @@ -261,9 +274,10 @@ int main(int argc, char **argv) { char *path = "/dev/audioctl0"; + const char *errstr; int c; - while ((c = getopt(argc, argv, "anf:q")) != -1) { + while ((c = getopt(argc, argv, "anf:qw:")) != -1) { switch (c) { case 'a':
Re: midicat(1): use err(3)
On Wed, Nov 30, 2022 at 09:20:26AM -0600, Scott Cheloha wrote: > Couple related things: > > - Use err(3) everywhere. > > For many of these errors we are not currently printing the errno > string. Is there any reason not to do so? The errno string is > useful. > > - Set ifile/ofile to "stdin"/"stdout" if the user passes in > "-" to make the err(3) message a little more obvious. > > - Add a usage() function. > > ok? > ok except for changes below > @@ -126,20 +114,16 @@ main(int argc, char **argv) > else > mode = MIO_IN; > ih = mio_open(port0, mode, 0); > - if (ih == NULL) { > - fprintf(stderr, "%s: couldn't open port\n", port0); > - return 1; > - } > + if (ih == NULL) > + err(1, "%s: couldn't open port", port0); > > /* open second port, output only */ > if (port1 == NULL) > oh = ih; > else { > oh = mio_open(port1, MIO_OUT, 0); > - if (oh == NULL) { > - fprintf(stderr, "%s: couldn't open port\n", port1); > - exit(1); > - } > + if (oh == NULL) > + err(1, "%s: couldn't open port", port1); The mio_xxx functions don't set errno, so you should use errx() and warnx() here > @@ -152,26 +136,26 @@ main(int argc, char **argv) > if (len == 0) > break; > if (len == -1) { > - perror("stdin"); > + warn("%s", ifile); > break; > } > } else { > len = mio_read(ih, buf, sizeof(buf)); > if (len == 0) { > - fprintf(stderr, "%s: disconnected\n", port0); > + warn("%s: disconnected", port0); > break; and here > } else { > n = mio_write(oh, buf, len); > if (n != len) { > - fprintf(stderr, "%s: disconnected\n", port1); > + warn("%s: disconnected", port1); > break; and here
Re: Remove audio(9) speaker_ctl(), let open() handle speakers where needed
On Sun, Oct 30, 2022 at 11:02:39AM +, Klemens Nanni wrote: > Only five legacy half-duplex hardware drivers require this function to > change between playing and recording: > i386: ess(4), gus(4), pas(4), sb(4) > luna88k: nec86(4) > > If defined, it is always called early in audio_open(), so just move the > call from audio(4) to each hardware driver's open() handler. > > SPKR_ON/OFF remain defined to leave driver-specific code unchanged. > > Further cleanup (unchecked speaker_ctl() return values, > FWRITE -> AUMODE_PLAY -> SPKR_ON dances, etc.) can happen later. > > i386/GENERIC.MP builds fine. > Feedback? Objection? OK? ok ratchov
Re: Replace audio(9) get_props() with duplex check in open() in partial-duplex drivers
On Thu, Oct 27, 2022 at 08:35:31PM +, Klemens Nanni wrote: > On Thu, Oct 27, 2022 at 03:51:05PM +0200, Alexandre Ratchov wrote: > > On Thu, Oct 27, 2022 at 01:08:57PM +, Klemens Nanni wrote: > > > @@ -1040,6 +1041,9 @@ ad1848_open(void *addr, int flags) > > > > > > DPRINTF(("ad1848_open: sc=%p\n", sc)); > > > > > > + if ((flags & (FWRITE | FREAD)) && sc->mode != 2) > > > + return ENXIO; > > > + > > > sc->sc_pintr = sc->sc_parg = NULL; > > > sc->sc_rintr = sc->sc_rarg = NULL; > > > > > > > afaiu, the correct condition for full-duplex check is: > > > > if ((flags & (FWRITE | FREAD)) == (FWRITE | FREAD)) > > ... > > > > (both FWRITE and FREAD set) > > Here's the correct version, sorry for the noise. no problem! ok ratchov > --- > sys/dev/isa/ad1848.c| 12 > sys/dev/isa/ad1848var.h | 2 -- > sys/dev/isa/ess.c | 29 - > sys/dev/isa/gus.c | 20 > sys/dev/isa/gusvar.h| 2 -- > sys/dev/isa/pas.c | 1 - > sys/dev/isa/sb.c| 1 - > sys/dev/isa/sbdsp.c | 11 --- > sys/dev/isa/sbdspvar.h | 2 -- > 9 files changed, 24 insertions(+), 56 deletions(-) > > diff --git a/sys/dev/isa/ad1848.c b/sys/dev/isa/ad1848.c > index bb2b95277a1..26b2cc2ba88 100644 > --- a/sys/dev/isa/ad1848.c > +++ b/sys/dev/isa/ad1848.c > @@ -74,6 +74,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -1040,6 +1041,9 @@ ad1848_open(void *addr, int flags) > > DPRINTF(("ad1848_open: sc=%p\n", sc)); > > + if ((flags & (FWRITE | FREAD)) == (FWRITE | FREAD) && sc->mode != 2) > + return ENXIO; > + > sc->sc_pintr = sc->sc_parg = NULL; > sc->sc_rintr = sc->sc_rarg = NULL; > > @@ -1458,11 +1462,3 @@ ad1848_round(void *addr, int direction, size_t size) > size = MAX_ISADMA; > return size; > } > - > -int > -ad1848_get_props(void *addr) > -{ > - struct ad1848_softc *sc = addr; > - > - return (sc->mode == 2 ? AUDIO_PROP_FULLDUPLEX : 0); > -} > diff --git a/sys/dev/isa/ad1848var.h b/sys/dev/isa/ad1848var.h > index 49c199f64fc..c8af3f04b4b 100644 > --- a/sys/dev/isa/ad1848var.h > +++ b/sys/dev/isa/ad1848var.h > @@ -210,6 +210,4 @@ void *ad1848_malloc(void *, int, size_t, int, int); > void ad1848_free(void *, void *, int); > size_t ad1848_round(void *, int, size_t); > > -int ad1848_get_props(void *); > - > #endif > diff --git a/sys/dev/isa/ess.c b/sys/dev/isa/ess.c > index 8989dd94b01..eba480013cf 100644 > --- a/sys/dev/isa/ess.c > +++ b/sys/dev/isa/ess.c > @@ -74,6 +74,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -115,6 +116,7 @@ struct audio_params ess_audio_default = > > int ess_setup_sc(struct ess_softc *, int); > > +int ess_1788_open(void *, int); > int ess_open(void *, int); > void ess_1788_close(void *); > void ess_1888_close(void *); > @@ -148,8 +150,6 @@ size_tess_round_buffersize(void *, int, size_t); > > > int ess_query_devinfo(void *, mixer_devinfo_t *); > -int ess_1788_get_props(void *); > -int ess_1888_get_props(void *); > > void ess_speaker_on(struct ess_softc *); > void ess_speaker_off(struct ess_softc *); > @@ -198,7 +198,7 @@ static const char *essmodel[] = { > */ > > const struct audio_hw_if ess_1788_hw_if = { > - .open = ess_open, > + .open = ess_1788_open, > .close = ess_1788_close, > .set_params = ess_set_params, > .round_blocksize = ess_round_blocksize, > @@ -211,7 +211,6 @@ const struct audio_hw_if ess_1788_hw_if = { > .allocm = ess_malloc, > .freem = ess_free, > .round_buffersize = ess_round_buffersize, > - .get_props = ess_1788_get_props, > .trigger_output = ess_audio1_trigger_output, > .trigger_input = ess_audio1_trigger_input, > }; > @@ -230,7 +229,6 @@ const struct audio_hw_if ess_1888_hw_if = { > .allocm = ess_malloc, > .freem = ess_free, > .round_buffersize = ess_round_buffersize, > - .get_props = ess_1888_get_props, > .trigger_output = ess_audio2_trigger_output, > .trigger_input = ess_audio1_trigger_input, > }; > @@ -982,6 +980,15 @@ essattach(struct ess_softc *sc) > * Various routines to interface to higher level audio driver > */ > > +int > +ess_1788_open(void *addr, int flags) > +{ > +
Re: Replace audio(9) get_props() with duplex check in open() for record-only drivers
On Thu, Oct 27, 2022 at 08:28:53PM +, Klemens Nanni wrote: > On Thu, Oct 27, 2022 at 03:49:55PM +0200, Alexandre Ratchov wrote: > > On Thu, Oct 27, 2022 at 01:09:57PM +, Klemens Nanni wrote: > > > @@ -1859,6 +1857,9 @@ utvfu_audio_open(void *v, int flags) > > > if (usbd_is_dying(sc->sc_udev)) > > > return (EIO); > > > > > > + if ((flags & (FWRITE | FREAD))) > > > + return (ENXIO); > > > + > > > if ((flags & FWRITE)) > > > return (ENXIO); > > > > > > > We already return ENXIO if playback is requested, so no need for the > > additional full-duplex check > > Yes. My diff was purely mechanical and this single occasion does does > not need an aditional check. > > Feedback? OK? > ok ratchov@ > --- > sys/dev/usb/utvfu.c | 8 > 1 file changed, 8 deletions(-) > > diff --git a/sys/dev/usb/utvfu.c b/sys/dev/usb/utvfu.c > index 930f2d12568..37b76ffb285 100644 > --- a/sys/dev/usb/utvfu.c > +++ b/sys/dev/usb/utvfu.c > @@ -795,7 +795,6 @@ int utvfu_audio_halt_in(void *); > int utvfu_audio_mixer_set_port(void *, struct mixer_ctrl *); > int utvfu_audio_mixer_get_port(void *, struct mixer_ctrl *); > int utvfu_audio_query_devinfo(void *, struct mixer_devinfo *); > -int utvfu_audio_get_props(void *); > int utvfu_audio_trigger_output(void *, void *, void *, int, > void (*)(void *), void *, struct audio_params *); > int utvfu_audio_trigger_input(void *, void *, void *, int, > @@ -851,7 +850,6 @@ const struct audio_hw_if utvfu_au_hw_if = { > .set_port = utvfu_audio_mixer_set_port, > .get_port = utvfu_audio_mixer_get_port, > .query_devinfo = utvfu_audio_query_devinfo, > - .get_props = utvfu_audio_get_props, > .trigger_output = utvfu_audio_trigger_output, > .trigger_input = utvfu_audio_trigger_input, > }; > @@ -1995,12 +1993,6 @@ utvfu_audio_query_devinfo(void *v, struct > mixer_devinfo *mi) > return (0); > } > > -int > -utvfu_audio_get_props(void *v) > -{ > - return (0); > -} > - > int > utvfu_audio_trigger_output(void *v, void *start, void *end, int blksize, > void (*intr)(void *), void *arg, struct audio_params *param) > -- > 2.38.1 > >
Re: Replace audio(9) get_props() with duplex check in open() in partial-duplex drivers
On Thu, Oct 27, 2022 at 01:08:57PM +, Klemens Nanni wrote: > @@ -1040,6 +1041,9 @@ ad1848_open(void *addr, int flags) > > DPRINTF(("ad1848_open: sc=%p\n", sc)); > > + if ((flags & (FWRITE | FREAD)) && sc->mode != 2) > + return ENXIO; > + > sc->sc_pintr = sc->sc_parg = NULL; > sc->sc_rintr = sc->sc_rarg = NULL; > afaiu, the correct condition for full-duplex check is: if ((flags & (FWRITE | FREAD)) == (FWRITE | FREAD)) ... (both FWRITE and FREAD set) > @@ -982,6 +980,15 @@ essattach(struct ess_softc *sc) > * Various routines to interface to higher level audio driver > */ > > +int > +ess_1788_open(void *addr, int flags) > +{ > + if (flags & (FWRITE | FREAD)) > + return ENXIO; > + > + return ess_open(addr, flags); > +} > + > int > ess_open(void *addr, int flags) > { > @@ -2059,18 +2066,6 @@ ess_round_buffersize(void *addr, int direction, size_t > size) same here. > @@ -307,6 +305,9 @@ gusopen(void *addr, int flags) > > DPRINTF(("gusopen() called\n")); > > + if ((flags & (FWRITE | FREAD)) && sc->sc_recdrq == sc->sc_drq) > + return ENXIO; > + > if (sc->sc_flags & GUS_OPEN) > return EBUSY; > ditto > @@ -2132,6 +2126,8 @@ sbdsp_midi_open(void *addr, int flags, void > (*iintr)(void *, int), > > DPRINTF(("sbdsp_midi_open: sc=%p\n", sc)); > > + if ((flags & (FWRITE | FREAD)) && !sc->sc_fullduplex) > + return ENXIO; ditto
Re: Replace audio(9) get_props() with duplex check in open() for play-only drivers
On Thu, Oct 27, 2022 at 01:27:43PM +, Klemens Nanni wrote: > Make drivers which do *not* adverise AUDIO_PROP_FULLDPLEX return ENXIO > in their open() if full-duplex mode was requested. > > This way, sys/dev/audio.c:audio_open() will fail immediately rather than > later through the to-be-removed get_props() check. > > These are all drivers which simply don't support recording. > > In device-tree based drivers like simpleaudio(4)/rkiis(4) and newer > Apple ones like aplaudio(4)/aplmca(4), this adds a new open() stub to > the low-level drivers which merely does the duplex check. > > The rkiis(4) and aplmca(4) stubs don't open anything, but are now > required for simpleaudio(4) and aplaudio(4) to actually do the low-level > driver specific duplex check. > > My Pinebook Pro keeps playing audio and recording silence with this diff > just like before (rkiis(4) is currently play-only): > simpleaudio0 at mainbus0 > simpleaudio1 at mainbus0 > audio0 at simpleaudio1 > > $ aucat -i song69.wav -o rec.wav > > Builds fine on amd64 and arm64. > Feedback? Objection? OK? IMO, for play-only drivers, the check should be: if (flags & FREAD) return ENXIO; which will reject record-only and full-duplex.
Re: Replace audio(9) get_props() with duplex check in open() for record-only drivers
On Thu, Oct 27, 2022 at 01:09:57PM +, Klemens Nanni wrote: > @@ -1859,6 +1857,9 @@ utvfu_audio_open(void *v, int flags) > if (usbd_is_dying(sc->sc_udev)) > return (EIO); > > + if ((flags & (FWRITE | FREAD))) > + return (ENXIO); > + > if ((flags & FWRITE)) > return (ENXIO); > We already return ENXIO if playback is requested, so no need for the additional full-duplex check
Re: audio: make get_props() optional, remove it from duplex drivers
On Tue, Oct 25, 2022 at 10:25:30PM +, Klemens Nanni wrote: > The property bits of audio(9) are obsolete and ought to be removed > completely. > > sys/dev/audio.c:audio_open() currently uses get_props() to bail out if > read *and* write was requested on a driver non-duplex driver. > > Drivers that currently support playing but not recording need a little > adjustment for this. > > Drivers that advertise themselves as full duplex, i.e. those that > always return AUDIO_PROP_FULLDUPLEX unconditionally in their get_props() > currently always succeed this check. > > As this is the only property, we can losen audio_open()'s DIAGNOSTIC > check to make get_props() optional and then only do the duplex check if > the driver provides this function. > > This allows us to simply remove get_props() from full-duplex drivers > without adding any other code and without changing functionality. > > This includes all audio drivers under sys/dev/pci/ (the unfinished > maestro(4) being the only exception here). > > Remaining drivers needing adjustment can then be dealt with later; > after this, the actual removal from the API can be done. > > This builds on amd64, arm64, i386, macppc and sparc64. > amd64 with azalia(4) still plays, records as well as plays and records > at the same time on my X230 as tested with > > $ aucat -i play.wav [-o rec.wav] > > I can't build-test luna88k, alpha or hppa, but the mechanical removal > should be identical to what works on the above mentioned architectures. > > Feedback? Objection? OK? looks fine, ok ratchov
Re: audio(9): remove unused setfd member from struct audio_hw_if
On Wed, Oct 19, 2022 at 07:31:59PM +, Klemens Nanni wrote: > All consumers now use C99 struct init and none of them sets `.setfd'. > > My X230 still plays audio just fine: > azalia0 at pci0 dev 27 function 0 "Intel 7 Series HD Audio" rev 0x04: msi > azalia0: codecs: Realtek ALC269, Intel/0x2806, using Realtek ALC269 > audio0 at azalia0 > > Feedback? OK? > ok ratchov
Re: maestro(4), uaudio(4): constify globals
On Wed, Oct 19, 2022 at 08:33:17AM +, Klemens Nanni wrote: > Used only for lookups; builds on i386. > > OK? > sure!
Re: dev/isa: ess, pas: constify string table
On Tue, Oct 18, 2022 at 02:27:53PM +, Klemens Nanni wrote: > Each only used once for a printf() call in *_attach(). > Seen while tweaking their *_hw_if struct. > > OK? > ok
Re: sys: use C99 struct init for struct audio_hw_if
On Tue, Oct 18, 2022 at 08:02:34PM +, Klemens Nanni wrote: > > Here's the actual complete diff that was tested as per above, > the previous did not include a bunch of files. > > I can also send/commit this in smaller per driver/arch/whatever chunks > if that's preferred. Your call ;-) ok ratchov
Re: sys: use C99 struct init for struct audio_hw_if
On Tue, Oct 18, 2022 at 02:17:54PM +, Klemens Nanni wrote: > audio(9) cleanup demands removing a member from this struct, which is > cumbersome in our current tree as drivers initialise it inconsistently, > i.e. a simple removal of ".member_name = driver_func," is not always > possible. > > Most drivers call their driver_*() functions like the member name, > - some vary slightly, e.g. ".allocm = driver_alloc," > - some read vague, e.g. ".round_buffersize = driver_round," where > .round_blocksize also exists > - some have the member name as comment, on both functions and NULL lines > - some use 0 not NULL > > Use C99 style everywhere for consistency and clarity, getting rid of all > annoying differences and allow for clear member removals/additions: > > - dont change current order of members > - no explicit NULL members > - no comments or blank lines > - trailing comma in last member line > > GENERIC.MP builds fine with this diff on arm64, amd64, i386 and sparc64. > I should be able to test macppc soon. > > Feedback? Objection? OK? > AFAICS, this will make life easier and the diff looks good ok ratchov
Re: audio: remove unused AUDIO_PROP_{MMAP,INDEPENDENT}
On Mon, Oct 17, 2022 at 07:45:33PM +, Klemens Nanni wrote: > I was looking at one particular driver and needed to know what those > flags are, turns out AUDIO_PROP_FULLDUPLEX is the only audio(9) property > that is in use. Even AUDIO_PROP_FULLDUPLEX (and the associated get_props() and setfd() routines) should go. FWIW, an audio device has only these modes: AUDIO_MODE_PLAY -> play only AUDIO_MODE_RECORD -> record only AUDIO_MODE_PLAY | AUDIO_MODE_RECORD -> play and record (in sync) But this requires touching the isa(4) drivers, bba(4/alpha), arcofi(4) and alike. > > Feedback? Objection? OK? > ok ratchov, thanks! As we,re at it, last point: audio_if.h and audioio.h are private interfaces and the risk of breaking a port close to zero. distfiles contain the "sys/audioio.h" string because it's still used on SunOS and NetBSD but these are #ifdef'd out on OpenBSD.
Re: speaker(4): unhook driver and manpage from build
On Thu, Apr 28, 2022 at 06:34:00AM -0500, Scott Cheloha wrote: > speaker(4) is a whimsical thing, but I don't think we should have a > dedicated chiptune interpreter in the kernel. > > This patch unhooks the driver and the manpage from the build. The > driver is built for alpha, amd64, and i386. > > A subsequent patch will move all relevant files to the attic and clean > up manpage cross references. > > Nothing in base or xenocara includes . > > I see a couple SPKRTONE and SPKRTUNE symbols in ports, but I imagine > those ports don't use the symbols if they are missing. > > ok? I can't see any use of this driver. ok ratchov
Re: sndio: add sio_flush() function
On Wed, Apr 27, 2022 at 10:48:42PM +0200, Caspar Schutijser wrote: > Hi, > > On Thu, Mar 24, 2022 at 07:11:42AM +0100, Alexandre Ratchov wrote: > > Most audio/video players do a stop/start cycle whenever the play > > position is changed, track is changed, etc. Currently, stopping drains > > the play buffer, which by default is very large (to workaround very > > long kernel non-preemptive code-paths). This makes player controls > > sluggish. > > > > This diff adds a new sio_flush() function to workaround the jumbo > > buffer sizes: it stops playback immediately, discarding buffered > > data. Basically it's the same as sio_stop() but doesn't wait. The plan > > is to make players use it. > > > > In the network protocol, sio_flush() is implemented by adding a flag > > to the message corresponding to sio_stop(). Old sndiod servers ignore > > it and just work with new libraries. New sndiod servers see that the > > flag is not set by old libraries and properly drain the play buffer. > > > > Tested with mplayer, mpv and audacious, if we go this way other ports > > will follow. > > I tested this with Audacious and I like the new behavior a lot! When I > change the play position, stop playback or change the track it indeed > responds much faster. > > When pausing playback however, the buffer is still being drained which > suprised me a little bit. However, after some printf-debugging I > confirmed my suspicion that Audacious does not call SndioPlugin::flush() > in that case, so this patch (or sndio in general) doesn't have anything > to do with that. > > I can't really comment on the diff itself, although that makes sense > to me. > thank you for testing this. I retested multimedia/mpv and x11/mplayer as well; besides the need for an extra REVISION bump, my previous diff still applies. OK?
Re: sndio: add sio_flush() function
On Thu, Mar 24, 2022 at 07:11:42AM +0100, Alexandre Ratchov wrote: > Most audio/video players do a stop/start cycle whenever the play > position is changed, track is changed, etc. Currently, stopping drains > the play buffer, which by default is very large (to workaround very > long kernel non-preemptive code-paths). This makes player controls > sluggish. > > This diff adds a new sio_flush() function to workaround the jumbo > buffer sizes: it stops playback immediately, discarding buffered > data. Basically it's the same as sio_stop() but doesn't wait. The plan > is to make players use it. > > In the network protocol, sio_flush() is implemented by adding a flag > to the message corresponding to sio_stop(). Old sndiod servers ignore > it and just work with new libraries. New sndiod servers see that the > flag is not set by old libraries and properly drain the play buffer. > > Tested with mplayer, mpv and audacious, if we go this way other ports > will follow. > Here's the diff to make mplayer, mpv and audacious use the new function. As you see, sio_stop() and sio_flush() are perfectly interchangable. The only difference is the time the function takes to complete (and the resulting silence). Index: x11/mplayer/Makefile === RCS file: /cvs/ports/x11/mplayer/Makefile,v retrieving revision 1.319 diff -u -p -r1.319 Makefile --- x11/mplayer/Makefile11 Mar 2022 20:16:48 - 1.319 +++ x11/mplayer/Makefile23 Mar 2022 19:49:08 - @@ -3,7 +3,7 @@ COMMENT=movie player supporting many fo V= 20211106 FFMPEG_V= 4.4.1 DISTNAME= mplayer-${V} -REVISION= 0 +REVISION= 1 CATEGORIES=x11 multimedia MASTER_SITES= https://comstyle.com/source/ EXTRACT_SUFX= .tar.xz Index: x11/mplayer/patches/patch-libao2_ao_sndio_c === RCS file: x11/mplayer/patches/patch-libao2_ao_sndio_c diff -N x11/mplayer/patches/patch-libao2_ao_sndio_c --- /dev/null 1 Jan 1970 00:00:00 - +++ x11/mplayer/patches/patch-libao2_ao_sndio_c 23 Mar 2022 19:49:08 - @@ -0,0 +1,21 @@ +Index: libao2/ao_sndio.c +--- libao2/ao_sndio.c.orig libao2/ao_sndio.c +@@ -179,7 +179,7 @@ static void uninit(int immed) + */ + static void reset(void) + { +-if (!sio_stop(hdl)) ++if (!sio_flush(hdl)) + mp_msg(MSGT_AO, MSGL_ERR, "ao2: reset: couldn't stop\n"); + delay = 0; + if (!sio_start(hdl)) +@@ -235,7 +235,7 @@ static void audio_pause(void) + * sndio can't pause, so just stop + */ + prepause_delay = delay; +-if (!sio_stop(hdl)) ++if (!sio_flush(hdl)) + mp_msg(MSGT_AO, MSGL_ERR, "ao2: pause: couldn't stop\n"); + delay = 0; + } Index: multimedia/mpv/Makefile === RCS file: /cvs/ports/multimedia/mpv/Makefile,v retrieving revision 1.81 diff -u -p -r1.81 Makefile --- multimedia/mpv/Makefile 11 Mar 2022 19:39:24 - 1.81 +++ multimedia/mpv/Makefile 23 Mar 2022 19:49:08 - @@ -4,7 +4,7 @@ GH_ACCOUNT =mpv-player GH_PROJECT = mpv GH_TAGNAME = v0.34.1 -REVISION = 0 +REVISION = 1 SHARED_LIBS += mpv 0.2 # 1.109 Index: multimedia/mpv/patches/patch-audio_out_ao_sndio_c === RCS file: /cvs/ports/multimedia/mpv/patches/patch-audio_out_ao_sndio_c,v retrieving revision 1.6 diff -u -p -r1.6 patch-audio_out_ao_sndio_c --- multimedia/mpv/patches/patch-audio_out_ao_sndio_c 11 Mar 2022 19:39:24 - 1.6 +++ multimedia/mpv/patches/patch-audio_out_ao_sndio_c 23 Mar 2022 19:49:09 - @@ -245,8 +245,8 @@ Index: audio/out/ao_sndio.c +if (p->playing) { +p->playing = false; + -+if (!sio_stop(p->hdl)) { -+MP_ERR(ao, "reset: couldn't sio_stop()\n"); ++if (!sio_flush(p->hdl)) { ++MP_ERR(ao, "reset: couldn't sio_flush()\n"); +} +p->delay = 0; +if (!sio_start(p->hdl)) { Index: audio/audacious/plugins/Makefile === RCS file: /cvs/ports/audio/audacious/plugins/Makefile,v retrieving revision 1.10 diff -u -p -r1.10 Makefile --- audio/audacious/plugins/Makefile8 Mar 2022 08:53:50 - 1.10 +++ audio/audacious/plugins/Makefile23 Mar 2022 19:49:09 - @@ -1,7 +1,7 @@ COMMENT = input and output plugins for Audacious DISTNAME = audacious-plugins-${VERSION} -REVISION = 2 +REVISION = 3 # BSD / GPL PERMIT_PACKAGE = Yes Index: audio/audacious/plugins/patches/patch-src_sndio_sndio_cc === RCS file: audio/audacious/plug
sndio: add sio_flush() function
Most audio/video players do a stop/start cycle whenever the play position is changed, track is changed, etc. Currently, stopping drains the play buffer, which by default is very large (to workaround very long kernel non-preemptive code-paths). This makes player controls sluggish. This diff adds a new sio_flush() function to workaround the jumbo buffer sizes: it stops playback immediately, discarding buffered data. Basically it's the same as sio_stop() but doesn't wait. The plan is to make players use it. In the network protocol, sio_flush() is implemented by adding a flag to the message corresponding to sio_stop(). Old sndiod servers ignore it and just work with new libraries. New sndiod servers see that the flag is not set by old libraries and properly drain the play buffer. Tested with mplayer, mpv and audacious, if we go this way other ports will follow. ok? Index: include/sndio.h === RCS file: /cvs/src/include/sndio.h,v retrieving revision 1.13 diff -u -p -r1.13 sndio.h --- include/sndio.h 28 Jun 2020 05:21:38 - 1.13 +++ include/sndio.h 23 Mar 2022 20:23:29 - @@ -164,6 +164,7 @@ size_t sio_write(struct sio_hdl *, const size_t sio_read(struct sio_hdl *, void *, size_t); int sio_start(struct sio_hdl *); int sio_stop(struct sio_hdl *); +int sio_flush(struct sio_hdl *); int sio_nfds(struct sio_hdl *); int sio_pollfd(struct sio_hdl *, struct pollfd *, int); int sio_revents(struct sio_hdl *, struct pollfd *); Index: lib/libsndio/Symbols.map === RCS file: /cvs/src/lib/libsndio/Symbols.map,v retrieving revision 1.2 diff -u -p -r1.2 Symbols.map --- lib/libsndio/Symbols.map26 Feb 2020 13:53:58 - 1.2 +++ lib/libsndio/Symbols.map23 Mar 2022 20:23:29 - @@ -11,6 +11,7 @@ sio_read; sio_start; sio_stop; + sio_flush; sio_nfds; sio_pollfd; sio_revents; Index: lib/libsndio/amsg.h === RCS file: /cvs/src/lib/libsndio/amsg.h,v retrieving revision 1.14 diff -u -p -r1.14 amsg.h --- lib/libsndio/amsg.h 1 Nov 2021 14:43:24 - 1.14 +++ lib/libsndio/amsg.h 23 Mar 2022 20:23:29 - @@ -96,6 +96,9 @@ struct amsg { #define AMSG_DATAMAX 0x1000 uint32_t size; } data; + struct amsg_stop { + uint8_t drain; + } stop; struct amsg_ts { int32_t delta; } ts; Index: lib/libsndio/shlib_version === RCS file: /cvs/src/lib/libsndio/shlib_version,v retrieving revision 1.12 diff -u -p -r1.12 shlib_version --- lib/libsndio/shlib_version 26 Feb 2020 13:53:58 - 1.12 +++ lib/libsndio/shlib_version 23 Mar 2022 20:23:29 - @@ -1,2 +1,2 @@ major=7 -minor=1 +minor=2 Index: lib/libsndio/sio.c === RCS file: /cvs/src/lib/libsndio/sio.c,v retrieving revision 1.26 diff -u -p -r1.26 sio.c --- lib/libsndio/sio.c 1 Nov 2021 14:43:24 - 1.26 +++ lib/libsndio/sio.c 23 Mar 2022 20:23:29 - @@ -129,6 +129,8 @@ sio_start(struct sio_hdl *hdl) int sio_stop(struct sio_hdl *hdl) { + if (hdl->ops->stop == NULL) + return sio_flush(hdl); if (hdl->eof) { DPRINTF("sio_stop: eof\n"); return 0; @@ -139,6 +141,28 @@ sio_stop(struct sio_hdl *hdl) return 0; } if (!hdl->ops->stop(hdl)) + return 0; +#ifdef DEBUG + DPRINTFN(2, "libsndio: polls: %llu, samples = %llu\n", + hdl->pollcnt, hdl->cpos); +#endif + hdl->started = 0; + return 1; +} + +int +sio_flush(struct sio_hdl *hdl) +{ + if (hdl->eof) { + DPRINTF("sio_flush: eof\n"); + return 0; + } + if (!hdl->started) { + DPRINTF("sio_flush: not started\n"); + hdl->eof = 1; + return 0; + } + if (!hdl->ops->flush(hdl)) return 0; #ifdef DEBUG DPRINTFN(2, "libsndio: polls: %llu, samples = %llu\n", Index: lib/libsndio/sio_aucat.c === RCS file: /cvs/src/lib/libsndio/sio_aucat.c,v retrieving revision 1.20 diff -u -p -r1.20 sio_aucat.c --- lib/libsndio/sio_aucat.c9 Jan 2016 08:27:24 - 1.20 +++ lib/libsndio/sio_aucat.c23 Mar 2022 20:23:29 - @@ -49,6 +49,7 @@ struct sio_aucat_hdl { static void sio_aucat_close(struct sio_hdl *); static int sio_aucat_start(struct sio_hdl *); static int sio_aucat_stop(struct sio_hdl *); +static int sio_aucat_flush(struct sio_hdl *); static int sio_aucat_setpar(struct sio_hdl *, struct sio_par *); static int
Re: constify *_hw_if
On Mon, Mar 21, 2022 at 12:09:54PM +, Miod Vallat wrote: > The following diff makes {audio,midi,radio,video}_hw_if structs in the > kernel const, in order to move them to rodata. > ok ratchov
Re: wskbd_set_mixervolume
On Thu, Feb 10, 2022 at 04:53:59PM +0100, Anton Lindqvist wrote: > On Wed, Feb 09, 2022 at 09:13:58AM +0100, Alexandre Ratchov wrote: > > On Tue, Feb 08, 2022 at 06:59:39PM +0100, Anton Lindqvist wrote: > > > On Tue, Feb 08, 2022 at 07:32:38AM +0100, Alexandre Ratchov wrote: > > > > On Mon, Feb 07, 2022 at 06:55:21PM +0100, Anton Lindqvist wrote: > > > > > On Mon, Feb 07, 2022 at 11:21:43AM +0100, Alexandre Ratchov wrote: > > > > > > On Sun, Feb 06, 2022 at 08:40:35AM +0100, Anton Lindqvist wrote: > > > > > > > > > > > > > > Polished diff. I'm omitting a necessary refactoring commit making > > > > > > > audio_attach_mi() accept a new cookie argument. > > > > > > > > > > > > > > > > > > > I'm not sure to understand the need for the wskbd_audio structure. > > > > > > Couldn't we just store the cookie in audio and wskbd softc > > > > > > structures, > > > > > > then pass it in the wskbd_set_mixervolume_sc() calls ? > > > > > > > > > > Due to the device caching the data must be stored in either the audio > > > > > or > > > > > wskbd softc and I don't want to expose the softc structures so I ended > > > > > up introducing wskbd_audio. Dropping the caching would probably make > > > > > it > > > > > possible to only pass down the cookie to wskbd_set_mixervolume_sc() > > > > > and > > > > > always do the audio device lookup. > > > > > > > > > > Is anyone in favor of this approach? I achieves the expected behavior > > > > > in > > > > > my opinion. > > > > > > > > IMHO, handling the volume keys this way won't work in the general > > > > case. But for the short term we've no other options, have we? > > > > > > > > AFAICS, you're fixing a concrete use-case by tweaking what already > > > > exists, this won't make things more broken than they already are. I'm > > > > OK with it. > > > > > > Here's the complete diff including adding a cookie argument to > > > audio_attach_mi(). > > > > Is the caching necessary? device_lookup() seems cheap and there are at > > most two devices in most cases. Keeping the code minimal especially on > > rare and non-performace-critical code-paths would be nice. > > > > If you choose to drop the caching, this would allow to just add a a > > new "cookie" argument to wskbd_set_mixervolume(), similarly to what > > you did for audio_attach_mi() > > Sure, I ended up adding a new function after all since the > wskbd_set_mixervolume() prototype is not present in any header at this > point. > ok ratchov
Re: wskbd_set_mixervolume
On Tue, Feb 08, 2022 at 06:59:39PM +0100, Anton Lindqvist wrote: > On Tue, Feb 08, 2022 at 07:32:38AM +0100, Alexandre Ratchov wrote: > > On Mon, Feb 07, 2022 at 06:55:21PM +0100, Anton Lindqvist wrote: > > > On Mon, Feb 07, 2022 at 11:21:43AM +0100, Alexandre Ratchov wrote: > > > > On Sun, Feb 06, 2022 at 08:40:35AM +0100, Anton Lindqvist wrote: > > > > > > > > > > Polished diff. I'm omitting a necessary refactoring commit making > > > > > audio_attach_mi() accept a new cookie argument. > > > > > > > > > > > > > I'm not sure to understand the need for the wskbd_audio structure. > > > > Couldn't we just store the cookie in audio and wskbd softc structures, > > > > then pass it in the wskbd_set_mixervolume_sc() calls ? > > > > > > Due to the device caching the data must be stored in either the audio or > > > wskbd softc and I don't want to expose the softc structures so I ended > > > up introducing wskbd_audio. Dropping the caching would probably make it > > > possible to only pass down the cookie to wskbd_set_mixervolume_sc() and > > > always do the audio device lookup. > > > > > > Is anyone in favor of this approach? I achieves the expected behavior in > > > my opinion. > > > > IMHO, handling the volume keys this way won't work in the general > > case. But for the short term we've no other options, have we? > > > > AFAICS, you're fixing a concrete use-case by tweaking what already > > exists, this won't make things more broken than they already are. I'm > > OK with it. > > Here's the complete diff including adding a cookie argument to > audio_attach_mi(). Is the caching necessary? device_lookup() seems cheap and there are at most two devices in most cases. Keeping the code minimal especially on rare and non-performace-critical code-paths would be nice. If you choose to drop the caching, this would allow to just add a a new "cookie" argument to wskbd_set_mixervolume(), similarly to what you did for audio_attach_mi()
Re: wskbd_set_mixervolume
On Mon, Feb 07, 2022 at 06:55:21PM +0100, Anton Lindqvist wrote: > On Mon, Feb 07, 2022 at 11:21:43AM +0100, Alexandre Ratchov wrote: > > On Sun, Feb 06, 2022 at 08:40:35AM +0100, Anton Lindqvist wrote: > > > > > > Polished diff. I'm omitting a necessary refactoring commit making > > > audio_attach_mi() accept a new cookie argument. > > > > > > > I'm not sure to understand the need for the wskbd_audio structure. > > Couldn't we just store the cookie in audio and wskbd softc structures, > > then pass it in the wskbd_set_mixervolume_sc() calls ? > > Due to the device caching the data must be stored in either the audio or > wskbd softc and I don't want to expose the softc structures so I ended > up introducing wskbd_audio. Dropping the caching would probably make it > possible to only pass down the cookie to wskbd_set_mixervolume_sc() and > always do the audio device lookup. > > Is anyone in favor of this approach? I achieves the expected behavior in > my opinion. IMHO, handling the volume keys this way won't work in the general case. But for the short term we've no other options, have we? AFAICS, you're fixing a concrete use-case by tweaking what already exists, this won't make things more broken than they already are. I'm OK with it.
Re: wskbd_set_mixervolume
On Mon, Feb 07, 2022 at 06:58:40PM +0100, Anton Lindqvist wrote: > On Mon, Feb 07, 2022 at 11:43:43AM +0100, Alexandre Ratchov wrote: > > On Sat, Feb 05, 2022 at 06:30:43PM +0100, Claudio Jeker 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 > > > > > > > > > > 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. > > > > > > > Here's a small WIP program to do so. It also adds a key to cycle > > through audio devices. > > > > https://github.com/ratchov/sndiokeys > > > > By default it uses Ctrl+Alt+{plus,minus,0} because multimedia keys are > > already used. > > > > Anton, I've no headphones with buttons to test, does this work for > > yours? > > Interesting! I cannot get to run under X, it fails in XGetKeyboardMapping: > > (gdb) r > Starting program: /tmp/sndiokeys/sndiokeys > X Error of failed request: BadAccess (attempt to access private resource > denied) > Major opcode of failed request: 33 (X_GrabKey) > Serial number of failed request: 12 > Current serial number in output stream: 76 > > Breakpoint 1, _libc_exit (status=1) at /home/src2/lib/libc/stdlib/exit.c:54 > 54 /home/src2/lib/libc/stdlib/exit.c: No such file or directory. > (gdb) bt > #0 _libc_exit (status=1) at /home/src2/lib/libc/stdlib/exit.c:54 > #1 0x0cfba76512b1 in _XDefaultError () from /usr/X11R6/lib/libX11.so.17.1 > #2 0x0cfba76514a9 in _XError () from /usr/X11R6/lib/libX11.so.17.1 > #3 0x0cfba764d47a in handle_response () from > /usr/X11R6/lib/libX11.so.17.1 > #4 0x0cfba764dd50 in _XReply () from /usr/X11R6/lib/libX11.so.17.1 > #5 0x0cfba762d8ba in XGetKeyboardMapping () from > /usr/X11R6/lib/libX11.so.17.1 > #6 0x0cf94a4778f7 in grab_keys () at sndiokeys.c:318 > #7 0x0cf94a477279 in main (argc=0, argv=0x7f7dd760) at > sndiokeys.c:529 > > One of the keys is already used (aka grabbed) by another X program. You've to either stop the other program or start sndiokeys before it. Meanwhile, I'll try to add error handling and print a meaningful error message.
Re: wskbd_set_mixervolume
On Sat, Feb 05, 2022 at 06:30:43PM +0100, Claudio Jeker 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 > > > > > > 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. > Here's a small WIP program to do so. It also adds a key to cycle through audio devices. https://github.com/ratchov/sndiokeys By default it uses Ctrl+Alt+{plus,minus,0} because multimedia keys are already used. Anton, I've no headphones with buttons to test, does this work for yours?
Re: wskbd_set_mixervolume
On Sun, Feb 06, 2022 at 08:40:35AM +0100, Anton Lindqvist wrote: > > Polished diff. I'm omitting a necessary refactoring commit making > audio_attach_mi() accept a new cookie argument. > I'm not sure to understand the need for the wskbd_audio structure. Couldn't we just store the cookie in audio and wskbd softc structures, then pass it in the wskbd_set_mixervolume_sc() calls ? > 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 > #include > #include > +#include /* struct wskbd_audio */ > #include "audio.h" > #include "wskbd.h" > > @@ -96,6 +97,8 @@ struct wskbd_vol > #define WSKBD_MUTE_DISABLE 2 > #define WSKBD_MUTE_ENABLE3 > }; > + > +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, >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(_cd, i); > + if (sc == NULL) > + continue; > + if (sc->cookie != cookie) { > + device_unref(>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(_cd, 0); > + sc = (struct audio_softc *)device_lookup(_cd, unit); > if (sc == NULL) > return ENODEV; > vol = out ? >spkr : >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(_hw_if, sc, NULL, >dev); > + audio_attach_mi(_hw_if, sc, arg->cookie, >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_keymap, > .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
Re: wskbd_set_mixervolume
On Sat, Feb 05, 2022 at 06:30:43PM +0100, Claudio Jeker 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 > > > > > > 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. > Agreed, this will fix most volume keys-related problems.
Re: wskbd_set_mixervolume
On Sat, Feb 05, 2022 at 09:29:42AM +0100, Anton Lindqvist wrote: > 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? This is a hack to make pc keyboard volume keys mimic volume keys of old laptops, which used to simply control internal speaker/headphones level. Hence the use of the first device (in most cases the internal). > The diff below gives me the desired behavior by propagating volume > changes to all attached audio devices. > Your diff is correct. However it's based on a broken hack, so it won't work as expected: Currently volume keys change the volume but also propagate to X as XF86XK_Audio{Raise,Lower}Volume events which in turn may change the volume a second time in a different way, possibly of different device. To increase further the confusion, X keys auto-repeat, while volume key changes through wskbd_set_mixervolume() don't. For instance, in one terminal start "sndioctl -m" to monitor volume changes then play something with ports/mplayer and observe volume changes: $ sndioctl -m ... output[0].level=0.322 # changed output[1].level=0.322 # changed app/mplayer0.level=0.567# changed output[0].level=0.353 # changed output[1].level=0.353 # changed app/mplayer0.level=0.583# changed IMHO, adding a sysctl or wsconsctl knob, as kettenis@ suggested, would make your keys "work", and allow to start experimenting with proper user-space solution.
Re: sndiod: -F does not switch back to preferred device
On Sat, Dec 25, 2021 at 02:51:08PM +, Klemens Nanni wrote: > > either "devices that are" or "device that is", I'd say the latter. > commited with "device that is". BTW, the example of the -F option description seems more appropriate for the new hot plugging section. OK? diff --git a/sndiod/sndiod.8 b/sndiod/sndiod.8 index b39d021..5226d80 100644 --- a/sndiod/sndiod.8 +++ b/sndiod/sndiod.8 @@ -193,11 +193,6 @@ the one given with the previous or .Fl F option will be used. -For instance, specifying a USB device following a -PCI device allows -.Nm -to use the USB one preferably when it's connected -and to fall back to the PCI one when it's disconnected. .It Fl f Ar device Add this .Xr sndio 7 @@ -539,6 +534,15 @@ Instead, must be used to change the .Va server.device control. +.Pp +For instance, specifying a USB device with +.Fl F +following a PCI device with +.Fl f +allows +.Nm +to use the USB one preferably when it's connected +and to fall back to the PCI one when it's disconnected. .Sh EXAMPLES Start server using default parameters, creating an additional sub-device for output to channels 2:3 only (rear speakers
Re: sndiod: -F does not switch back to preferred device
On Sat, Dec 25, 2021 at 01:34:19PM +, Klemens Nanni wrote: > > This reads OK kn, but I suspect others will object to the hotplugd(8) > reference. > here's a new one, with attach/detach replaced by plug/unplug and no ref to hotplug(8) OK? diff --git a/sndiod/sndiod.8 b/sndiod/sndiod.8 index 1f95097..c31da66 100644 --- a/sndiod/sndiod.8 +++ b/sndiod/sndiod.8 @@ -185,8 +185,10 @@ Only the signedness and the precision are mandatory. Examples: .Va u8 , s16le , s24le3 , s24le4lsb . .It Fl F Ar device -Specify an alternate device to use. -If it doesn't work, the one given with the previous +Same as +.Fl f +except that if the device is disconnected, +the one given with the previous .Fl f or .Fl F @@ -196,11 +198,6 @@ PCI device allows .Nm to use the USB one preferably when it's connected and to fall back to the PCI one when it's disconnected. -Alternate devices may be switched with the -.Va server.device -control of the -.Xr sndioctl 1 -utility. .It Fl f Ar device Add this .Xr sndio 7 @@ -528,6 +525,20 @@ behave normally, while streams connected to wait for the MMC start signal and start synchronously. Regardless of which device a stream is connected to, its playback volume knob is exposed. +.Sh HOT PLUGGING +If devices specified with +.Fl F +are unavailable when needed or unplugged at runtime, +.Nm +will attempt to seamlessly fall back to the last device specified. +.Pp +.Nm +will not automatically switch to specified devices that is plugged at runtime. +Instead, +.Xr sndioctl 1 +must be used to change the +.Va server.device +control. .Sh EXAMPLES Start server using default parameters, creating an additional sub-device for output to channels 2:3 only (rear speakers
switch sndiod & aucat to 24-bit ?
This diff switches internal sample representation frem 16-bit to 24-bit fixed-point. Resampling is already 24-bit only, so there's little point in keeping the current 16-bit code. The default device precision doesn't change, i.e. we still request the same 16-bit mode (and convert everything to 24-bit when needed), so there's no risk of exposing driver bugs (yet). If you've the 24-bit capable hardware, adding "-e s24" to sndiod_flags will reduce the quantization noise, without the need for rebuilding with -DADATA_BITS=24 anymore. objections? OK? Index: sndiod/dsp.h === RCS file: /cvs/src/usr.bin/sndiod/dsp.h,v retrieving revision 1.10 diff -u -p -u -p -r1.10 dsp.h --- sndiod/dsp.h25 May 2021 08:06:12 - 1.10 +++ sndiod/dsp.h21 Dec 2021 17:53:12 - @@ -25,28 +25,14 @@ * boundary is excluded. We represent them as signed fixed point numbers * of ADATA_BITS. We also assume that 2^(ADATA_BITS - 1) fits in a int. */ -#ifndef ADATA_BITS -#define ADATA_BITS 16 -#endif +#define ADATA_BITS 24 #define ADATA_LE (BYTE_ORDER == LITTLE_ENDIAN) #define ADATA_UNIT (1 << (ADATA_BITS - 1)) -#if ADATA_BITS == 16 - -#define ADATA_MUL(x,y) (((int)(x) * (int)(y)) >> (ADATA_BITS - 1)) - -typedef short adata_t; - -#elif ADATA_BITS == 24 - #define ADATA_MUL(x,y) \ ((int)(((long long)(x) * (long long)(y)) >> (ADATA_BITS - 1))) typedef int adata_t; - -#else -#error "only 16-bit and 24-bit precisions are supported" -#endif /* * The FIR is sampled and stored in a table of fixed-point numbers Index: sndiod/sndiod.8 === RCS file: /cvs/src/usr.bin/sndiod/sndiod.8,v retrieving revision 1.12 diff -u -p -u -p -r1.12 sndiod.8 --- sndiod/sndiod.8 18 Dec 2021 21:41:49 - 1.12 +++ sndiod/sndiod.8 21 Dec 2021 17:53:13 - @@ -564,11 +564,6 @@ $ sndiod -r 48000 -b 480 -z 240 Resampling is low quality; down-sampling especially should be avoided when recording. .Pp -Processing is done using 16-bit arithmetic, -thus samples with more than 16 bits are rounded. -16 bits (i.e. 97dB dynamic) are largely enough for most applications though. -Processing precision can be increased to 24-bit at compilation time though. -.Pp If .Fl a Ar off is used, Index: sndiod/sndiod.c === RCS file: /cvs/src/usr.bin/sndiod/sndiod.c,v retrieving revision 1.47 diff -u -p -u -p -r1.47 sndiod.c --- sndiod/sndiod.c 1 Nov 2021 14:43:25 - 1.47 +++ sndiod/sndiod.c 21 Dec 2021 17:53:13 - @@ -85,6 +85,13 @@ #define DEFAULT_BUFSZ 7680 #endif +/* + * default device precision + */ +#ifndef DEFAULT_BITS +#define DEFAULT_BITS 16 +#endif + void sigint(int); void sighup(int); void opt_ch(int *, int *); @@ -486,7 +493,11 @@ main(int argc, char **argv) pmax = 1; rmin = 0; rmax = 1; - aparams_init(); + par.bits = DEFAULT_BITS; + par.bps = APARAMS_BPS(par.bits); + par.le = ADATA_LE; + par.sig = 1; + par.msb = 0; mode = MODE_PLAY | MODE_REC; dev_first = dev_next = NULL; port_first = port_next = NULL; Index: aucat/aucat.1 === RCS file: /cvs/src/usr.bin/aucat/aucat.1,v retrieving revision 1.116 diff -u -p -u -p -r1.116 aucat.1 --- aucat/aucat.1 22 Apr 2020 05:37:00 - 1.116 +++ aucat/aucat.1 21 Dec 2021 17:53:13 - @@ -88,7 +88,7 @@ Increase log verbosity. .It Fl e Ar enc Encoding of the audio file. The default is -.Va s16 . +.Va s24 . Encoding names use the following scheme: signedness .Po .Va s Index: aucat/aucat.c === RCS file: /cvs/src/usr.bin/aucat/aucat.c,v retrieving revision 1.177 diff -u -p -u -p -r1.177 aucat.c --- aucat/aucat.c 12 Jan 2021 15:46:53 - 1.177 +++ aucat/aucat.c 21 Dec 2021 17:53:13 - @@ -1393,7 +1393,11 @@ main(int argc, char **argv) rate = DEFAULT_RATE; cmin = 0; cmax = 1; - aparams_init(); + par.bits = ADATA_BITS; + par.bps = APARAMS_BPS(par.bits); + par.le = ADATA_LE; + par.sig = 1; + par.msb = 1; hdr = AFILE_HDR_AUTO; n_flag = 0; port = NULL; Index: aucat/dsp.h === RCS file: /cvs/src/usr.bin/aucat/dsp.h,v retrieving revision 1.8 diff -u -p -u -p -r1.8 dsp.h --- aucat/dsp.h 25 May 2021 08:06:12 - 1.8 +++ aucat/dsp.h 21 Dec 2021 17:53:13 - @@ -25,28 +25,14 @@ * boundary is excluded. We represent them as signed fixed point numbers * of ADATA_BITS. We also assume that 2^(ADATA_BITS - 1) fits in a int. */ -#ifndef ADATA_BITS -#define ADATA_BITS
Re: sndiod: -F does not switch back to preferred device
On Fri, Dec 17, 2021 at 07:11:41PM +, Klemens Nanni wrote: > > So we've concluded that the hotplpugging framework needs work, fine. > > I'd still like to improve sndiod(8) regarding its own behaviour. > > How about the same diff modulo hotplug referencing/examples? > This helps me understand `-f' and `-F' usage better. > > Feedback? Objections? OK? > > Index: sndiod.8 > === > RCS file: /cvs/src/usr.bin/sndiod/sndiod.8,v > retrieving revision 1.11 > diff -u -p -r1.11 sndiod.8 > --- sndiod.8 16 Jul 2021 15:05:58 - 1.11 > +++ sndiod.8 17 Dec 2021 19:02:21 - > @@ -185,7 +185,7 @@ Only the signedness and the precision ar > Examples: > .Va u8 , s16le , s24le3 , s24le4lsb . > .It Fl F Ar device > -Specify an alternate device to use. > +Specify a preferred device to use on startup. > If it doesn't work, the one given with the last > .Fl f > or Recently we dropped the "alternate device name" hack and now -F and -f are the same, except that -f may fail, while -F tries the next device. AFAICS, -F will disappear completely soon. What about just saying it's the same as -f? (BTW, the paragraph about USB may be moved to the new "HOT PLUGGING" section, if we go this way) > @@ -528,6 +528,24 @@ behave normally, while streams connected > wait for the MMC start signal and start synchronously. > Regardless of which device a stream is connected to, > its playback volume knob is exposed. > +.Sh HOT PLUGGING > +.Nm > +If preferred devices specified with > +.Fl F > +are unavailable at startup or detach at runtime, > +.Nm > +will attempt to seamlessly fall back to the last device specified. > +.Pp > +.Nm > +will not automatically switch to specified devices that attach at runtime. > +Instead, > +.Xr sndioctl 1 > +must be used to change the > +.Va server.device > +control. > +.Pp > +.Xr hotplugd 8 > +can be used to implement seamless switching at runtime. > .Sh EXAMPLES > Start server using default parameters, creating an > additional sub-device for output to channels 2:3 only (rear speakers > > "At startup" suggests this is when the daemon starts, imho "When a device is needed" is more accurate (devices are kept closed when not used, so when a device is needed again, iteration over -Ff restarts). With these tweaks, I end-up with the diff below. As this is a the "hot plugging" section, would it make sense to use plugged/unplugged instead of attach/detach words? diff --git a/sndiod/sndiod.8 b/sndiod/sndiod.8 index 910ce12..7dd72e5 100644 --- a/sndiod/sndiod.8 +++ b/sndiod/sndiod.8 @@ -185,8 +185,10 @@ Only the signedness and the precision are mandatory. Examples: .Va u8 , s16le , s24le3 , s24le4lsb . .It Fl F Ar device -Specify an alternate device to use. -If it doesn't work, the one given with the previous +Same as +.Fl f +except that if the device is disconnected, +the one given with the previous .Fl f or .Fl F @@ -196,11 +198,6 @@ PCI device allows .Nm to use the USB one preferably when it's connected and to fall back to the PCI one when it's disconnected. -Alternate devices may be switched with the -.Va server.device -control of the -.Xr sndioctl 1 -utility. .It Fl f Ar device Add this .Xr sndio 7 @@ -528,6 +525,23 @@ behave normally, while streams connected to wait for the MMC start signal and start synchronously. Regardless of which device a stream is connected to, its playback volume knob is exposed. +.Sh HOT PLUGGING +If devices specified with +.Fl F +are unavailable when needed or detach at runtime, +.Nm +will attempt to seamlessly fall back to the last device specified. +.Pp +.Nm +will not automatically switch to specified devices that attach at runtime. +Instead, +.Xr sndioctl 1 +must be used to change the +.Va server.device +control. +.Pp +.Xr hotplugd 8 +can be used to implement seamless switching at runtime. .Sh EXAMPLES Start server using default parameters, creating an additional sub-device for output to channels 2:3 only (rear speakers
Re: sndiod: Remove ambiguous use of 'last' in man page
On Sat, Dec 18, 2021 at 10:27:34AM +, Jason McIntyre wrote: > On Sat, Dec 18, 2021 at 10:51:28AM +0100, Richard Ulmer wrote: > > Hi, > > after reading about using USB audio interfaces in > > https://www.openbsd.org/faq/faq13.html I looked up what -F and -f mean > > in sndiod(8) but was briefly confused by the word 'last'. I commonly > > understand 'last' as either 'previous' or 'at the end of the list' and > > in this case the latter popped into my head first, which is the wrong > > meaning in this case (if I'm not misunderstanding things). > > > > hi. > > i agree that "previous" is probably less ambiguous, so i'm ok with that > change. > > > Maybe the attached patch could prevent this confusion. I also changed > > 'options' to singular, because it seems to me like it refers to the > > (single) previous -f/-F option. > > > > this one really depends on how the options are thought of, so either > version is ok really. but with your proposed word change of "previous", > singulAr does read better. > > so, i'm fine with this diff and happy to commit. i'll let it sit a > little in case anyone has an issue (i.e. we've both misunderstood the > text!) > thanks, both changes makes the man page better explain what the code does ok ratchov
Re: sndiod: -F does not switch back to preferred device
On Thu, Dec 09, 2021 at 09:02:07PM -0700, Theo de Raadt wrote: > > I think drivers, or maybe this can be done in higher-level subsystems, need > to be modified such that events get sent when a device is *actually ready*, > and the call from config_attach() should be deleted. > For audio and MIDI, I think we're not very far from this. Currently, as soon as audio_attach() returns, both hardware and audio(4) internal structures are properly initialized. So all the audio_{open,read,write,ioctl,close}() interfaces are safe to call. What happens in higher layers is unclear to me? for instance, what needs to be done for open(2) to reach audioopen() with no races?
Re: sndiod: -F does not switch back to preferred device
On Thu, Dec 09, 2021 at 02:04:06PM +0100, Solene Rapenne wrote: > > where does sndioctl server.device= come from? > > on my system I don't have the server.device property > There's one server.device knob for each "-s" to controls on which device are player (or recorded from). > > sndioctl server.device > server.device: no such control > > I use this line in rc.conf.local > sndiod_flags=-f rsnd/0 -m play,rec,mon -s rec -s mon -F rsnd/1 > > Do you have an AUDIODEVICE environment variable set? If so, could you verify that it's set to "snd/rec", "snd/mon" or "snd/default"? These are supposed to work: sndioctl -f snd/mon server.device sndioctl -f snd/rec server.device sndioctl -f snd/default server.device While these are not: sndioctl -f snd/0 server.device sndioctl -f snd/1 server.device because the 0 or 1 _is_ the device, so there's no device to choose. (this applies to -current, above were added after release)
Re: sndiod: -F does not switch back to preferred device
On Wed, Dec 08, 2021 at 10:30:08PM +, Klemens Nanni wrote: > Following https://www.openbsd.org/faq/faq13.html#usbaudio and reading > sndiod(8)'s > > -F device > Specify an alternate device to use. If it doesn't work, the one > given with the last -f or -F options will be used. For > instance, > specifying a USB device following a PCI device allows sndiod to > use the USB one preferably when it's connected and to fall back > to the PCI one when it's disconnected. Alternate devices may be > switched with the server.device control of the sndioctl(1) > utility. > > I configured things as follows in order to play audio via USB and > fall back to internal sound if USB is not available: > > $ rcctl get sndiod flags > -f rsnd/0 -F rsnd/1 > > Plugging in an USB headset and restarting sndiod or forcing the device > with `sndioctl server.device=1' then plays sound via USB. > > Unplugging the device makes playback fall back to internal sound. > > But plugging USB back in does not prefer USB to internal as I'd expect > now. I am currently resorting to the following hotplugd(8) script to > always select the USB sound device whenever I plug it in: > > #!/bin/ksh > set -Cefu -o pipefail > > readonly DEVCLASS=$1 DEVNAME=$2 > typeset -i devid > > case "${DEVCLASS}-${DEVNAME}" in > 0-audio*) # switch sndio(4) to USB headset when plugging it in > devid=${DEVNAME#audio} > sndioctl server.device=${devid} > ;; > esac > IFAIU, audio devices always match the audio[0-9] pattern, so testing the device class is not necessary, is it? FWIW, I use a similar script. > I'd expect sndiod to *always* use USB whenever possible. > > Is this uni-directional behaviour of sndiod intentional/by-design? sndiod doesn't use directly hotplug(4), so it can't obtain by itself a notification when a new device is plugged in order to switch to it. Using hotplugd(8) is the right way to make audio hotplug work, both daemons complete each other. When sndiod needs the audio device (i.e. when the first client connects), it will try all alternate devices (reverse -F options order, the USB headset is first in your case). This gives a false impression of hotplug support (indeed, you plug the USB headset, start an audio player and it just works!). But that's just a side effect of device priority/fail-over: if the internal device is already open when you plug the USB headset, sndiod won't switch to it until it needs to reopen the device. Certain programs (including browsers), tend to keep the device open even when they remain silent. So when actual playback starts sndiod doesn't need to open a device and doesn't "see" the new USB headset. I guess that's what happens on your system, but hotplugd(8) handles this. When devices are unplugged, we don't need hotplug because the device stops working (input/output error) and sndiod switch to the next one of the fail-over list. > If so, can we clarify the manual? Sure. While -F option description seems exact, maybe we need an extra paragraph or FAQ entry to explain how to use it with other tools like hotplugd and sndioctl. What about this wording? HOT-PLUG SUPPORT If a device is unplugged while in use, sndiod will attempt to switch to one of the alternate devices (-F), if any. This is seamless to programs connected to sndiod. Later, when the device is connected again, the server.device control of the sndioctl(1) utility could be used to switch back to it, without the need to restart all audio programs. This last step could be automated using hotplugd(8). For instance, if sndiod is started with: $ sndiod -f rsnd/0 -F rsnd/1 -F rsnd/2 -F rsnd/3 then, the following hotplugd(8) attach script could be used to automatically switch to the last connected device: #!/bin/sh DEVNAME=$2 case $DEVNAME in audio[0-3]) sndioctl server.device=${DEVNAME#audio} ;; esac Any opinions where to put such information?
better audio defaults: please test
The current sndiod latency (minimum time between when the program plays something and when sound reaches Joe's ears) is too large and makes OpenBSD unpleasant to use for telephony, games, and makes controls of video players slugish. The defaut latency (of 160ms) was set ~10 years ago to workaround various problems: KERNEL_LOCK used to block audio processing for very long, azalia(4) and uaudio(4) were unable to recover after an error, which aggravated the problem. The kernel improved a lot the last decade and such large buffers are not necessary anymore. I think something between 20ms and 40ms is a better default for the average OpenBSD system: * audio-conferencing software and games requires no sndiod_flags tweaks anymore * on modern machines (like my 7 years old i5-2500K) building a kernel doesn't make audio stutterer * sndiod_flags tweaks will still be needed for: - very slow or overloaded machines used for audio - machines running heavy/bogus SMM code - real-time synths & effects (20ms is still too small) Please try to switch you system to 40ms buffers (i.e. 1920 samples at the default 48kHz rate), for instance either apply diff below or simply do: rcctl set sndiod flags -z 480 -b 1920 rcctl restart sndiod then report any significant increase of stuttering, and what software/hardware triggers it. If you think 20ms or 30ms (i.e. 960 and 1440 sample buffers) are better, let me know as well. Note that OpenBSD is not real-time (neither are programs we run) so audio may stutter no matter how large the buffers are. The goal here is to get a ballance between disconfort caused by latency and probability of stuttering for the average OpenBSD system. If we reach a consensus, here's the diff to make above settings the default. OK? Index: sndiod.c === RCS file: /cvs/src/usr.bin/sndiod/sndiod.c,v retrieving revision 1.47 diff -u -p -r1.47 sndiod.c --- sndiod.c1 Nov 2021 14:43:25 - 1.47 +++ sndiod.c1 Nov 2021 15:28:37 - @@ -82,7 +82,7 @@ * buffer size if neither ``-z'' nor ``-b'' is used */ #ifndef DEFAULT_BUFSZ -#define DEFAULT_BUFSZ 7680 +#define DEFAULT_BUFSZ 1920 #endif void sigint(int);
midi: defer selwakeup() calls to softintr to fix locking bug
selwakeup() needs to be protected by KERNEL_LOCK, but we're not allowed to grab KERNEL_LOCK on interrupt context because midi runs at IPL_AUDIO with the audio_lock held. Furthermore, doing so is a locking order bug: syscall code-path grabs KERNEL_LOCK first while interrupt code-path does the opposite when calling selwakeup(). Fix this for midi(4) as we did years ago for audio(4): defer selwakeup() calls to a softintr. This diff is mostly copied from audio(4) to make both look similar. ok? Index: midi.c === RCS file: /cvs/src/sys/dev/midi.c,v retrieving revision 1.49 diff -u -p -u -p -r1.49 midi.c --- midi.c 29 Oct 2021 13:24:50 - 1.49 +++ midi.c 29 Oct 2021 15:26:30 - @@ -32,6 +32,8 @@ #include #include +#define IPL_SOFTMIDI IPL_SOFTNET +#define DEVNAME(sc)((sc)->dev.dv_xname) intmidiopen(dev_t, int, int, struct proc *); intmidiclose(dev_t, int, int, struct proc *); @@ -84,6 +86,25 @@ const struct filterops midiread_filtops }; void +midi_buf_wakeup(void *addr) +{ + struct midi_buffer *buf = addr; + + if (buf->blocking) { + wakeup(>blocking); + buf->blocking = 0; + } + /* +* As long as selwakeup() grabs the KERNEL_LOCK() make sure it is +* already held here to avoid lock ordering problems with `audio_lock' +*/ + KERNEL_ASSERT_LOCKED(); + mtx_enter(_lock); + selwakeup(>sel); + mtx_leave(_lock); +} + +void midi_iintr(void *addr, int data) { struct midi_softc *sc = (struct midi_softc *)addr; @@ -97,13 +118,14 @@ midi_iintr(void *addr, int data) return; /* discard data */ MIDIBUF_WRITE(mb, data); - if (mb->used == 1) { - if (sc->rchan) { - sc->rchan = 0; - wakeup(>rchan); - } - selwakeup(>rsel); - } + + /* +* As long as selwakeup() needs to be protected by the +* KERNEL_LOCK() we have to delay the wakeup to another +* context to keep the interrupt context KERNEL_LOCK() +* free. +*/ + softintr_schedule(sc->inbuf.softintr); } int @@ -131,9 +153,9 @@ midiread(dev_t dev, struct uio *uio, int error = EWOULDBLOCK; goto done_mtx; } - sc->rchan = 1; - error = msleep_nsec(>rchan, _lock, PWAIT | PCATCH, - "mid_rd", INFSLP); + sc->inbuf.blocking = 1; + error = msleep_nsec(>inbuf.blocking, _lock, + PWAIT | PCATCH, "mid_rd", INFSLP); if (!(sc->dev.dv_flags & DVF_ACTIVE)) error = EIO; if (error) @@ -206,11 +228,14 @@ void midi_out_stop(struct midi_softc *sc) { sc->isbusy = 0; - if (sc->wchan) { - sc->wchan = 0; - wakeup(>wchan); - } - selwakeup(>wsel); + + /* +* As long as selwakeup() needs to be protected by the +* KERNEL_LOCK() we have to delay the wakeup to another +* context to keep the interrupt context KERNEL_LOCK() +* free. +*/ + softintr_schedule(sc->outbuf.softintr); } void @@ -276,8 +301,8 @@ midiwrite(dev_t dev, struct uio *uio, in */ goto done_mtx; } - sc->wchan = 1; - error = msleep_nsec(>wchan, _lock, + sc->outbuf.blocking = 1; + error = msleep_nsec(>outbuf.blocking, _lock, PWAIT | PCATCH, "mid_wr", INFSLP); if (!(sc->dev.dv_flags & DVF_ACTIVE)) error = EIO; @@ -327,9 +352,9 @@ midipoll(dev_t dev, int events, struct p } if (revents == 0) { if (events & (POLLIN | POLLRDNORM)) - selrecord(p, >rsel); + selrecord(p, >inbuf.sel); if (events & (POLLOUT | POLLWRNORM)) - selrecord(p, >wsel); + selrecord(p, >outbuf.sel); } mtx_leave(_lock); device_unref(>dev); @@ -349,11 +374,11 @@ midikqfilter(dev_t dev, struct knote *kn error = 0; switch (kn->kn_filter) { case EVFILT_READ: - klist = >rsel.si_note; + klist = >inbuf.sel.si_note; kn->kn_fop = _filtops; break; case EVFILT_WRITE: - klist = >wsel.si_note; + klist = >outbuf.sel.si_note; kn->kn_fop = _filtops; break; default: @@ -376,7 +401,7 @@ filt_midirdetach(struct knote *kn) struct midi_softc *sc = (struct midi_softc *)kn->kn_hook;
Re: Please test: full poll/select(2) switch
On Fri, Oct 29, 2021 at 01:12:06PM +0100, Martin Pieuchot wrote: > On 29/10/21(Fri) 13:12, Alexandre Ratchov wrote: > > On Sat, Oct 23, 2021 at 10:40:56AM +0100, Martin Pieuchot wrote: > > > Diff below switches both poll(2) and select(2) to the kqueue-based > > > implementation. > > > > > > In addition it switches libevent(3) to use poll(2) by default for > > > testing purposes. > > > > > > I don't have any open bug left with this diff and I'm happily running > > > GNOME with it. So I'd be happy if you could try to break it and report > > > back. > > > > > > > Without the below diff (copied from audio(4) driver), kernel panics > > upon the first MIDI input byte. > > What is the panic? The mutex is taken recursively, right? > Exactly, this is the "locking against myself", panic. AFAIU, the interrupt handler grabs the audio_lock and calls midi_iintr(). It calls selwakeup(), which in turn calls filt_midiread(), which attempts to grab the audio_lock a second time. > > ok? suggestion for a better fix? > > Without seeing the panic, I'm guessing this is correct. > > That suggest kevent(2) wasn't safe to use with midi(4). > Yes, this is the very first time midi(4) is used with kevent(2).
Re: Please test: full poll/select(2) switch
On Sat, Oct 23, 2021 at 10:40:56AM +0100, Martin Pieuchot wrote: > Diff below switches both poll(2) and select(2) to the kqueue-based > implementation. > > In addition it switches libevent(3) to use poll(2) by default for > testing purposes. > > I don't have any open bug left with this diff and I'm happily running > GNOME with it. So I'd be happy if you could try to break it and report > back. > Without the below diff (copied from audio(4) driver), kernel panics upon the first MIDI input byte. ok? suggestion for a better fix? Index: midi.c === RCS file: /cvs/src/sys/dev/midi.c,v retrieving revision 1.48 diff -u -p -r1.48 midi.c --- midi.c 25 Dec 2020 12:59:52 - 1.48 +++ midi.c 29 Oct 2021 11:09:47 - @@ -386,9 +386,11 @@ filt_midiread(struct knote *kn, long hin struct midi_softc *sc = (struct midi_softc *)kn->kn_hook; int retval; - mtx_enter(_lock); + if ((hint & NOTE_SUBMIT) == 0) + mtx_enter(_lock); retval = !MIDIBUF_ISEMPTY(>inbuf); - mtx_leave(_lock); + if ((hint & NOTE_SUBMIT) == 0) + mtx_leave(_lock); return (retval); } @@ -409,9 +411,11 @@ filt_midiwrite(struct knote *kn, long hi struct midi_softc *sc = (struct midi_softc *)kn->kn_hook; intretval; - mtx_enter(_lock); + if ((hint & NOTE_SUBMIT) == 0) + mtx_enter(_lock); retval = !MIDIBUF_ISFULL(>outbuf); - mtx_leave(_lock); + if ((hint & NOTE_SUBMIT) == 0) + mtx_leave(_lock); return (retval); }
sndio: initial bits for multiple devices support, please test
Hi, Current sndiod logic supports one device only: If two -f are used then two independent instances of the whole server are created, each with its own set of clients, options, controls, and so on. This diff shifts sndiod towards a more flexible (and conceptually simpler) model: - clients connect to logical devices (created with -s option) - logical devices route data to/from physical devices registered with -f option This logic removes obstacles to working on multiple devices support, including a single client using one device for recording and another for playback. This logic makes not only sndiod internals simpler but allows to simplify sndio(7) naming scheme. Instead of: "snd/." we only need: "snd/" where becomes the logical device (or "virtual device" to use Ingo's words). This doesn't affect setup using default. For those using an advanced configuration, there is a compatibility layer making the change almost transparent. This change also makes -F and -f rougly the same: logical device can be migrated from one physical device to another, so the need for the whole "alternate device" hack^Wconcept vanishes. Documentation change is the bare minimum. Man pages could be simplified, I'll do so as soon as MIDI has received the same treatment (next step). The diff is big, but it mostly moves existing bits from one object to another, so there's little risk of breaking things, but please test and report regressions, if any. Index: usr.bin/sndiod/dev.c === RCS file: /cvs/src/usr.bin/sndiod/dev.c,v retrieving revision 1.102 diff -u -p -u -p -r1.102 dev.c --- usr.bin/sndiod/dev.c3 May 2021 04:29:50 - 1.102 +++ usr.bin/sndiod/dev.c23 Oct 2021 13:40:34 - @@ -45,16 +45,13 @@ struct dev *dev_new(char *, struct apara unsigned int, unsigned int, unsigned int, unsigned int); void dev_adjpar(struct dev *, int, int, int); int dev_allocbufs(struct dev *); -int dev_open(struct dev *); void dev_freebufs(struct dev *); -void dev_close(struct dev *); int dev_ref(struct dev *); void dev_unref(struct dev *); int dev_init(struct dev *); void dev_done(struct dev *); struct dev *dev_bynum(int); void dev_del(struct dev *); -void dev_setalt(struct dev *, unsigned int); unsigned int dev_roundof(struct dev *, unsigned int); void dev_wakeup(struct dev *); @@ -930,10 +927,8 @@ dev_new(char *path, struct aparams *par, return NULL; } d = xmalloc(sizeof(struct dev)); - d->alt_list = NULL; - dev_addname(d,path); + d->path = path; d->num = dev_sndnum++; - d->alt_num = -1; d->reqpar = *par; d->reqmode = mode; @@ -948,6 +943,7 @@ dev_new(char *path, struct aparams *par, d->slot_list = NULL; d->master = MIDI_MAXCTL; d->master_enabled = 0; + d->alt_next = d; snprintf(d->name, CTL_NAMEMAX, "%u", d->num); for (pd = _list; *pd != NULL; pd = &(*pd)->next) ; @@ -957,51 +953,6 @@ dev_new(char *path, struct aparams *par, } /* - * add a alternate name - */ -int -dev_addname(struct dev *d, char *name) -{ - struct dev_alt *a; - - if (d->alt_list != NULL && d->alt_list->idx == DEV_NMAX - 1) { - log_puts(name); - log_puts(": too many alternate names\n"); - return 0; - } - a = xmalloc(sizeof(struct dev_alt)); - a->name = name; - a->idx = (d->alt_list == NULL) ? 0 : d->alt_list->idx + 1; - a->next = d->alt_list; - d->alt_list = a; - return 1; -} - -/* - * set prefered alt device name - */ -void -dev_setalt(struct dev *d, unsigned int idx) -{ - struct dev_alt **pa, *a; - - /* find alt with given index */ - for (pa = >alt_list; (a = *pa)->idx != idx; pa = >next) - ; - - /* detach from list */ - *pa = a->next; - - /* attach at head */ - a->next = d->alt_list; - d->alt_list = a; - - /* reopen device with the new alt */ - if (idx != d->alt_num) - dev_reopen(d); -} - -/* * adjust device parameters and mode */ void @@ -1099,9 +1050,6 @@ dev_allocbufs(struct dev *d) int dev_open(struct dev *d) { - char name[CTL_NAMEMAX]; - struct dev_alt *a; - d->mode = d->reqmode; d->round = d->reqround; d->bufsz = d->reqbufsz; @@ -1123,16 +1071,6 @@ dev_open(struct dev *d) if (!dev_allocbufs(d)) return 0; - /* if there are multiple alt devs, add server.device knob */ - if (d->alt_list->next != NULL) { - for (a = d->alt_list; a != NULL; a = a->next) { - snprintf(name, sizeof(name), "%d", a->idx); - ctl_new(CTL_DEV_ALT, d, >idx, - CTL_SEL, d->name, "server", -1, "device", - name, -1, 1, a->idx == d->alt_num); -
Re: human-readable audio device descriptions
On Wed, Jul 07, 2021 at 03:13:37PM +0100, Edd Barrett wrote: > Hi, > > This diff is an attempt to make identifying audio devices easier by > giving each device a human-readable description that can be read from > userspace (and without scraping dmesg). > > This is implemented on a per-audio-device basis using a new `descr` > interface to `audio_hw_if`. For now I've only implemented this on: > > - azalia(4), which uses the codec info, e.g. `Realtek ALC1150` > > - uaudio(4), which uses the USB vendor and model info, e.g. >`Logitech HD Pro Webcam C920`. > > For drivers, which currently don't implement this interface, the > description string defaults to the audio device node name, e.g. > `uaudio2`. > > The string is exposed to userspace via the AUDIO_GETDEV ioctl(2). I've > reclaimed the space currently occupied by the (unused) `version` and > `config` fields in `audio_device_t` and used it for a new `descr` field. > Thus the size of `audio_device_t` remains the same for now. > > I've also made audioctl(8) display the description string, e.g.: > ``` > # audioctl > name=azalia1 > descr=Realtek ALC1150 > ... > # audioctl -f /dev/audioctl1 > name=uaudio0 > descr=SteelSeries SteelSeries Arctis > ... > ``` > > I'm hoping that later we can have sndiod read these strings and expose > them to its clients. For example, I'd like for sndioctl(1) to allow > choosing a device based on the device description string (instead of the > sndio device index), and for xfce4-mixer to allow choosing a device via > its description in the GUI using a drop-down box. > > Diff follows. > [...] > @@ -4019,6 +4021,37 @@ azalia_set_nblks(void *v, int mode, > nblks = HDA_BDL_MAX; > > return nblks; > +} > + > +void > +azalia_descr(void *arg, char *descr, size_t len) > +{ > + azalia_t *az = arg; > + codec_t *codec; > + const char *vendor; > + char buf[MAX_AUDIO_DESCR_LEN]; > + int i; > + > + *descr = '\0'; > + for (i = 0; i < az->ncodecs; i++) { azalia(4) fetches all codecs, but uses (and supports) only one of them. You should copy the name of the one in use, ie az->codecs[az->codecno]. > + codec = >codecs[i]; > + /* adapted from azalia_print_codec() */ > + if (codec->name == NULL) { > + vendor = pci_findvendor(codec->vid >> 16); > + if (vendor == NULL) > + snprintf(buf, MAX_AUDIO_DESCR_LEN, > + "0x%04x/0x%04x", codec->vid >> 16, > + codec->vid & 0x); > + else > + snprintf(buf, MAX_AUDIO_DESCR_LEN, "%s/0x%04x", > + vendor, codec->vid & 0x); > + } else > + snprintf(buf, MAX_AUDIO_DESCR_LEN, "%s", codec->name); > + > + if (i > 0) > + strlcat(descr, ", ", len); > + strlcat(descr, buf, len); > + } > } > > int > Index: sys/sys/audioio.h > === > RCS file: /cvs/src/sys/sys/audioio.h,v > retrieving revision 1.27 > diff -u -p -r1.27 audioio.h > --- sys/sys/audioio.h 14 Sep 2016 06:12:20 - 1.27 > +++ sys/sys/audioio.h 7 Jul 2021 10:23:09 - > @@ -76,10 +76,10 @@ struct audio_status { > * audio devices. > */ > #define MAX_AUDIO_DEV_LEN16 > +#define MAX_AUDIO_DESCR_LEN 32 > typedef struct audio_device { > char name[MAX_AUDIO_DEV_LEN]; > - char version[MAX_AUDIO_DEV_LEN]; > - char config[MAX_AUDIO_DEV_LEN]; > + char descr[MAX_AUDIO_DESCR_LEN]; /* device description string */ > } audio_device_t; > The intent of the "name" field was to be human-readable as well. You could just replace it. There's no benefit in having two fields for the same thing.
Re: human-readable audio device descriptions
On Wed, Jul 07, 2021 at 08:25:16AM -0600, Theo de Raadt wrote: > > This diff is an attempt to make identifying audio devices easier by > > giving each device a human-readable description that can be read from > > userspace (and without scraping dmesg). > > But why does anyone want that? > > Isn't everyone served best when there is a public portable interface, > and all the back-ends must try to work the same? Exactly. The long-term aim is to expose this information to programs through sndiod with a stable libsndio interface, which in turn would ease switching between audio devices. This diff is the first step; IMO, it's not necessary until we get the whole chain, but it doesn't hurt either; it improves audioctl(8) output, which is a dignostics-only tool.
Re: add PCI IDs for Thinkpad X1 Extreme Gen 3, enable WLAN
On Thu, Apr 15, 2021 at 03:24:56PM +0200, Ivo Sbalzarini wrote: > Thanks a lot! > > I also got the sound working on this machine now. As far as I > can tell, both speakers work and Dolby Atmos works, too, thanks > to the awesome quirk by Joshua Stein (jcs@). > > Iām not sure I did everything right here, though it works. > Below is the diff. > > cheers, > - ivo > > > Index: azalia.c > === > RCS file: /cvs/src/sys/dev/pci/azalia.c,v > retrieving revision 1.259 > diff -u -p -r1.259 azalia.c > --- azalia.c 25 Oct 2020 07:22:06 - 1.259 > +++ azalia.c 15 Apr 2021 13:18:07 - > @@ -490,7 +490,8 @@ azalia_configure_pci(azalia_t *az) > > const struct pci_matchid azalia_pci_devices[] = { > { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_200SERIES_U_HDA }, > - { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_300SERIES_U_HDA } > + { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_300SERIES_U_HDA }, > + { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_400SERIES_CAVS } > }; > > int > Index: azalia_codec.c > === > RCS file: /cvs/src/sys/dev/pci/azalia_codec.c,v > retrieving revision 1.183 > diff -u -p -r1.183 azalia_codec.c > --- azalia_codec.c16 Jan 2021 07:02:39 - 1.183 > +++ azalia_codec.c15 Apr 2021 13:18:07 - > @@ -162,9 +162,16 @@ azalia_codec_init_vtbl(codec_t *this) > break; > case 0x10ec0285: > this->name = "Realtek ALC285"; > - if (this->subid == 0x229217aa) /* Thinkpad X1 Carbon > 7 */ > - this->qrks |= AZ_QRK_ROUTE_SPKR2_DAC | > - AZ_QRK_WID_CLOSE_PCBEEP; > + if (PCI_VENDOR(this->subid) == PCI_VENDOR_LENOVO) { > +if (this->subid == 0x22c017aa) { /* Thinkpad X1 Extreme > 3 */ > +this->name = "Realtek ALC3286"; > +this->qrks |= AZ_QRK_DOLBY_ATMOS | > + AZ_QRK_ROUTE_SPKR2_DAC; > + } > + else if (this->subid == 0x229217aa) /* Thinkpad X1 Carbon > 7 */ > +this->qrks |= AZ_QRK_ROUTE_SPKR2_DAC | > +AZ_QRK_WID_CLOSE_PCBEEP; > + } > break; > case 0x10ec0287: > this->name = "Realtek ALC287"; > > > Thanks for looking at this. Few comments: - the "PCI_VENDOR(this->subid) == PCI_VENDOR_LENOVO" compares the 16 lower bits of the subid. It's not necessary because below we compare the full 32 bits. - the "name" field is the codec name; it is identified by the "vid" field, so it shouldn't depend on the device subid. - style: indentation is (8 char) TAB, see style(9). With above tweaks, I ended up this diff. Could you confirm it still makes audio work? Index: azalia.c === RCS file: /cvs/src/sys/dev/pci/azalia.c,v retrieving revision 1.259 diff -u -p -u -p -r1.259 azalia.c --- azalia.c25 Oct 2020 07:22:06 - 1.259 +++ azalia.c19 Apr 2021 15:37:32 - @@ -490,7 +490,8 @@ azalia_configure_pci(azalia_t *az) const struct pci_matchid azalia_pci_devices[] = { { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_200SERIES_U_HDA }, - { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_300SERIES_U_HDA } + { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_300SERIES_U_HDA }, + { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_400SERIES_CAVS } }; int Index: azalia_codec.c === RCS file: /cvs/src/sys/dev/pci/azalia_codec.c,v retrieving revision 1.183 diff -u -p -u -p -r1.183 azalia_codec.c --- azalia_codec.c 16 Jan 2021 07:02:39 - 1.183 +++ azalia_codec.c 19 Apr 2021 15:37:32 - @@ -162,9 +162,15 @@ azalia_codec_init_vtbl(codec_t *this) break; case 0x10ec0285: this->name = "Realtek ALC285"; - if (this->subid == 0x229217aa) /* Thinkpad X1 Carbon 7 */ + if (this->subid == 0x229217aa) { + /* Thinkpad X1 Carbon 7 */ this->qrks |= AZ_QRK_ROUTE_SPKR2_DAC | AZ_QRK_WID_CLOSE_PCBEEP; +} else if (this->subid == 0x22c017aa) { + /* Thinkpad X1 Extreme 3 */ + this->qrks |= AZ_QRK_DOLBY_ATMOS | + AZ_QRK_ROUTE_SPKR2_DAC; + } break; case 0x10ec0287: this->name = "Realtek ALC287";
Re: sndiod: allow mixing of duplex, record-only and play-only audio devices
On Sat, Feb 27, 2021 at 09:59:21PM +, Edd Barrett wrote: > > This is mostly a no-op for sndiod in the default configuration (except > that play-only devices now accept recording clients). If you use > alternative devices (-F), then it's possible for a record-only device to > be found first, which may be confusing if you just want to hear sound. > We can only assume that if you deviate from defaults, then you know what > you are doing. thanks for fixing this, ok ratchov
Re: sndiod: Move controls out of the device structure, please test & review
On Fri, Jan 29, 2021 at 12:42:37PM +0100, Alexandre Ratchov wrote: > Moving to a global server-wide controls list is necessary to expose > controls that are not associated to a particular device (ex. a device > selector). > > The current hack to use the device-side sioctl_desc->addr variable as > client-side key can't work anymore. So, we use a unique dynamically > allocated ctl->addr key; this is which much cleaner. A new "scope" > enum (with two "void *" arguments) is used to determine what the > control does control. This adds a lot of flexibility and allows to > easily add new control types that are not associated to devices. > > The diff touches many parts of the code and is quite big. Please test > and report regressions. > Thanks for the feedback, new diff below. It fixes a server crash when a client issues an invalid request. Index: dev.c === RCS file: /cvs/src/usr.bin/sndiod/dev.c,v retrieving revision 1.89 diff -u -p -u -p -r1.89 dev.c --- dev.c 29 Jan 2021 11:38:23 - 1.89 +++ dev.c 29 Jan 2021 15:15:38 - @@ -106,6 +106,7 @@ struct slotops zomb_slotops = { zomb_exit }; +struct ctl *ctl_list = NULL; struct dev *dev_list = NULL; unsigned int dev_sndnum = 0; @@ -370,12 +371,14 @@ dev_midi_master(struct dev *d) master = d->master; else { master = 0; - for (c = d->ctl_list; c != NULL; c = c->next) { + for (c = ctl_list; c != NULL; c = c->next) { if (c->type != CTL_NUM || - strcmp(c->group, "") != 0 || + strcmp(c->group, d->name) != 0 || strcmp(c->node0.name, "output") != 0 || strcmp(c->func, "level") != 0) continue; + if (c->u.any.arg0 != d) + continue; v = (c->curval * 127 + c->maxval / 2) / c->maxval; if (master < v) master = v; @@ -465,7 +468,7 @@ dev_midi_omsg(void *arg, unsigned char * slot_array[chan].opt->dev != d) return; slot_setvol(slot_array + chan, msg[2]); - dev_onval(d, CTLADDR_SLOT_LEVEL(chan), msg[2]); + ctl_onval(CTL_SLOT_LEVEL, slot_array + chan, NULL, msg[2]); return; } x = (struct sysex *)msg; @@ -479,7 +482,7 @@ dev_midi_omsg(void *arg, unsigned char * if (len == SYSEX_SIZE(master)) { dev_master(d, x->u.master.coarse); if (d->master_enabled) { - dev_onval(d, CTLADDR_MASTER, + ctl_onval(CTL_DEV_MASTER, d, NULL, x->u.master.coarse); } } @@ -1005,14 +1008,16 @@ dev_master(struct dev *d, unsigned int m if (d->mode & MODE_PLAY) dev_mix_adjvol(d); } else { - for (c = d->ctl_list; c != NULL; c = c->next) { + for (c = ctl_list; c != NULL; c = c->next) { + if (c->scope != CTL_HW || c->u.hw.dev != d) + continue; if (c->type != CTL_NUM || - strcmp(c->group, "") != 0 || + strcmp(c->group, d->name) != 0 || strcmp(c->node0.name, "output") != 0 || strcmp(c->func, "level") != 0) continue; v = (master * c->maxval + 64) / 127; - dev_setctl(d, c->addr, v); + ctl_setval(c, v); } } } @@ -1070,7 +1075,7 @@ dev_new(char *path, struct aparams *par, d->master = MIDI_MAXCTL; d->mtc.origin = 0; d->tstate = MMC_STOP; - d->ctl_list = NULL; + snprintf(d->name, CTL_NAMEMAX, "%u", d->num); d->next = dev_list; dev_list = d; return d; @@ -1208,10 +1213,8 @@ dev_allocbufs(struct dev *d) int dev_open(struct dev *d) { - int i; char name[CTL_NAMEMAX]; struct dev_alt *a; - struct slot *s; d->master_enabled = 0; d->mode = d->reqmode; @@ -1235,23 +1238,12 @@ dev_open(struct dev *d) if (!dev_allocbufs(d)) return 0; - for (i = 0, s = slot_array; i < DEV_NSLOT; i++, s++) { -
sndiod: Move controls out of the device structure, please test & review
Moving to a global server-wide controls list is necessary to expose controls that are not associated to a particular device (ex. a device selector). The current hack to use the device-side sioctl_desc->addr variable as client-side key can't work anymore. So, we use a unique dynamically allocated ctl->addr key; this is which much cleaner. A new "scope" enum (with two "void *" arguments) is used to determine what the control does control. This adds a lot of flexibility and allows to easily add new control types that are not associated to devices. The diff touches many parts of the code and is quite big. Please test and report regressions. Besides that no behavior change. OK? Index: dev.c === RCS file: /cvs/src/usr.bin/sndiod/dev.c,v retrieving revision 1.89 diff -u -p -u -p -r1.89 dev.c --- dev.c 29 Jan 2021 11:38:23 - 1.89 +++ dev.c 29 Jan 2021 11:39:08 - @@ -106,6 +106,7 @@ struct slotops zomb_slotops = { zomb_exit }; +struct ctl *ctl_list = NULL; struct dev *dev_list = NULL; unsigned int dev_sndnum = 0; @@ -370,12 +371,14 @@ dev_midi_master(struct dev *d) master = d->master; else { master = 0; - for (c = d->ctl_list; c != NULL; c = c->next) { + for (c = ctl_list; c != NULL; c = c->next) { if (c->type != CTL_NUM || - strcmp(c->group, "") != 0 || + strcmp(c->group, d->name) != 0 || strcmp(c->node0.name, "output") != 0 || strcmp(c->func, "level") != 0) continue; + if (c->u.any.arg0 != d) + continue; v = (c->curval * 127 + c->maxval / 2) / c->maxval; if (master < v) master = v; @@ -465,7 +468,7 @@ dev_midi_omsg(void *arg, unsigned char * slot_array[chan].opt->dev != d) return; slot_setvol(slot_array + chan, msg[2]); - dev_onval(d, CTLADDR_SLOT_LEVEL(chan), msg[2]); + ctl_onval(CTL_SLOT_LEVEL, slot_array + chan, NULL, msg[2]); return; } x = (struct sysex *)msg; @@ -479,7 +482,7 @@ dev_midi_omsg(void *arg, unsigned char * if (len == SYSEX_SIZE(master)) { dev_master(d, x->u.master.coarse); if (d->master_enabled) { - dev_onval(d, CTLADDR_MASTER, + ctl_onval(CTL_DEV_MASTER, d, NULL, x->u.master.coarse); } } @@ -1005,14 +1008,16 @@ dev_master(struct dev *d, unsigned int m if (d->mode & MODE_PLAY) dev_mix_adjvol(d); } else { - for (c = d->ctl_list; c != NULL; c = c->next) { + for (c = ctl_list; c != NULL; c = c->next) { + if (c->scope != CTL_HW || c->u.hw.dev != d) + continue; if (c->type != CTL_NUM || - strcmp(c->group, "") != 0 || + strcmp(c->group, d->name) != 0 || strcmp(c->node0.name, "output") != 0 || strcmp(c->func, "level") != 0) continue; v = (master * c->maxval + 64) / 127; - dev_setctl(d, c->addr, v); + ctl_setval(c, v); } } } @@ -1070,7 +1075,7 @@ dev_new(char *path, struct aparams *par, d->master = MIDI_MAXCTL; d->mtc.origin = 0; d->tstate = MMC_STOP; - d->ctl_list = NULL; + snprintf(d->name, CTL_NAMEMAX, "%u", d->num); d->next = dev_list; dev_list = d; return d; @@ -1208,10 +1213,8 @@ dev_allocbufs(struct dev *d) int dev_open(struct dev *d) { - int i; char name[CTL_NAMEMAX]; struct dev_alt *a; - struct slot *s; d->master_enabled = 0; d->mode = d->reqmode; @@ -1235,23 +1238,12 @@ dev_open(struct dev *d) if (!dev_allocbufs(d)) return 0; - for (i = 0, s = slot_array; i < DEV_NSLOT; i++, s++) { - if (s->opt == NULL || s->opt->dev != d || s->name[0] == 0) - continue; - slot_ctlname(s, name, CTL_NAMEMAX); - dev_addctl(d, "app", CTL_NUM, - CTLADDR_SLOT_LEVEL(i), - name, -1, "level", - NULL, -1, 127, s->vol); - } - /* if there are multiple alt devs, add server.device knob */ if (d->alt_list->next != NULL) {
Re: search usbd_interfaces in case of non-compliant device
On Tue, Jan 26, 2021 at 08:56:29PM +, Edd Barrett wrote: > Hi, > > I've recently come across a uaudio device which doesn't work in OpenBSD: > > uaudio2 at uhub0 port 1 configuration 1 interface 3 "E+ Corp. DAC Audio" rev > 1.10/0.01 addr 2 > uaudio2: class v1, full-speed, async, channels: 2 play, 0 rec, 3 ctls > audio3 at uaudio2 > > When opening the audio device, the kernel reports `uaudio0: can't get iface > handle` in the dmesg buffer. > > I posted information about the device and sthen@ and kettenis@ spotted > the problem: the device exposes non-consecutive interface numbers, and > is thus not compliant with the USB standard. > > It has interfaces 0, 1 and 3, in that order. The audio stream is on > interface 3, which the kernel expects to find at index 3 in the ifaces > array, but actually it's at index 2! > > I've pasted a load of info about the device (lsusb and debug kernel > output) here: > https://gist.github.com/vext01/958756d0fd515cc321f99a3ee1e3351a > > The following diff makes this device work. It searches the ifaces array > in the event that the correct entry is not found at the expect index. > This should be a "no functional change" for all existing compliant > devices. > > I've tested this for the past two days and all seems well, but I'd like > some USB stack hackers to verify it. > This also fixes another audio device. AFAICS this diff doesn't change the code-path for compliant devices, so I don't see how this could cause problems ok ratchov (could you split the long comment line, please)
Re: sndiod: (mostly) suppress aliasing noise
On Sun, Dec 20, 2020 at 05:58:22PM +0200, Paul Irofti wrote: > Hi, > > Interesting diff. > > I did not have time to look at it thoroughly, but here are a few > observations: > Hi, Thanks for looking at this. - why do you keep the symmetric filter coefficients? (this could halve your > while-loop computations too, right?) > The filter function is indeed symetric: h(t) = h(-t) but they are used at points that are not symetric, we need: ... h(-2 + x) h(-1 + x) h(0 + x) h(1 + x) h(2 + x) ... h(-1 + x) is not the same as h(1 + x), so using the symetry to save CPU cycles is not easy I also tried to store only half of the static table to save few bytes; this makes the code larger (and messy) so the benefit is not that important for such a small filter order. > - the diff's mainlobe look kind of strange at the end, why is that? What > kind of filter did you use? > the side-lobes seem also a bit strange as they > do not decrease monotonically nor are the first sidelobes small and then > increasing To measure the response, we're supposed to apply the filter to a peak. But on the link I sent the plot it the result of the whole 8k->48k conversion, so the peak is not really peak anymore and the result is harder to understand. Here's plot of the Fourier transform of the coefficients in the code: https://vm.caoua.org/src/coef-dft.png which should look way more familiar (x-axis is multiples of the Nyquist frequency, y-axis is the Fourier transform amplitude in dB) The filter is an ideal "sinc" low-pass with the cut-off frequency set to 0.75 of the Nyquist frequency, multiplied by a Blackman window of length 8: h(t) = 0.75 * sinc(0.75 * pi * t) * win(t / 8) where win(t) is the Blackman window function with a = 0.16, defined as: win(t) = 0.5 * ((1 - a) + cos(2 * pi * x) + a * cos(4 * pi * x)) At 48kHz, the transition band starts at 0.75 * 24kHz = 18kHz. Besides preserving audible frequencies, with this choice the ideal filter's impulse response (the pure sinc function) crosses zero at the edges of the window, which makes the result smoother (derivative is 0) and in turn improves the roll-off. > - would be nice to see some SNR comparisons if you can to better show the > effects of your anti-aliasing effort > Here's an array of measurements I did. https://vm.caoua.org/src/sweeps.html The signal and the aliasing amplitudes are represented for each frequency for various conversion ratios. Anything except the increasing white curve (the signal, 0dB) is aliasing, the color indicates its strength.
sndiod: (mostly) suppress aliasing noise
Hi, The current sndiod resampling algorithm is very basic mainly to keep CPU usage very low, which used to make sense for the zaurus. So, resampling produces aliasing noise, easily audible in 8kHz to 48kHz conversions but present in other cases. The diff below reduces the aliasing noise. It's a trade-off between audio quality, CPU consumption and code simplicity. It adds a better resampling filter (8-th order FIR low-pass), which mostly suppresses aliasing noise in the audible part of the spectrum. Few plots here: https://vm.caoua.org/src I measured the CPU usage of resampling 16-bit stereo from 44.1kHz to 48kHz. On a ~7 years old i5-2500K: - current code: 0.07% CPU - with diff: 0.45% CPU On a ~15 years old Thinkpad x40: - current:0.25% CPU - with diff: 1.90% CPU This is still acceptable even for slow machines, IMHO. Enjoy and let me know if this introduces any regression. Index: dsp.c === RCS file: /cvs/src/usr.bin/sndiod/dsp.c,v retrieving revision 1.15 diff -u -p -r1.15 dsp.c --- dsp.c 10 Dec 2020 17:30:49 - 1.15 +++ dsp.c 19 Dec 2020 23:38:24 - @@ -38,6 +38,75 @@ int aparams_ctltovol[128] = { 26008, 27029, 28090, 29193, 30339, 31530, 32768 }; +int resamp_filt[RESAMP_LENGTH / RESAMP_STEP + 1] = { + 0, 0, 3, 9, 22, 42, 73, 116, + 174, 248, 341, 454, 589, 749, 934,1148, + 1392,1666,1974,2316,2693,3107,3560,4051, + 4582,5154,5766,6420,7116,7853,8632,9451, + 10311, 11210, 12148, 13123, 14133, 15178, 16253, 17359, + 18491, 19647, 20824, 22018, 23226, 24443, 25665, 26888, + 28106, 29315, 30509, 31681, 32826, 33938, 35009, 36033, + 37001, 37908, 38744, 39502, 40174, 40750, 41223, 41582, + 41819, 41925, 41890, 41704, 41358, 40842, 40147, 39261, + 38176, 36881, 35366, 33623, 31641, 29411, 26923, 24169, + 21140, 17827, 14222, 10317,6105,1580, -3267, -8440, +-13944, -19785, -25967, -32492, -39364, -46584, -54153, -62072, +-70339, -78953, -87911, -97209, -106843, -116806, -127092, -137692, + -148596, -159795, -171276, -183025, -195029, -207271, -219735, -232401, + -245249, -258259, -271407, -284670, -298021, -311434, -324880, -338329, + -351750, -365111, -378378, -391515, -404485, -417252, -429775, -442015, + -453930, -465477, -476613, -487294, -497472, -507102, -516137, -524527, + -532225, -539181, -545344, -550664, -555090, -558571, -561055, -562490, + -562826, -562010, -559990, -556717, -552139, -546205, -538866, -530074, + -519779, -507936, -494496, -479416, -462652, -444160, -423901, -401835, + -377923, -352132, -324425, -294772, -263143, -229509, -193847, -156134, + -116348, -74474, -30494, 15601, 63822, 114174, 11, 221283, +278037, 336916, 397911, 461009, 526194, 593446, 662741, 734054, +807354, 882608, 959779, 1038826, 1119706, 1202370, 1286768, 1372846, + 1460546, 1549808, 1640566, 1732753, 1826299, 1921130, 2017169, 2114336, + 2212550, 2311723, 2411770, 2512598, 2614116, 2716228, 2818836, 2921841, + 3025142, 3128636, 3232218, 3335782, 3439219, 3542423, 3645282, 3747687, + 3849526, 3950687, 4051059, 4150530, 4248987, 4346320, 4442415, 4537163, + 4630453, 4722177, 4812225, 4900493, 4986873, 5071263, 5153561, 5233668, + 5311485, 5386917, 5459872, 5530259, 5597992, 5662986, 5725160, 5784436, + 5840739, 5893999, 5944148, 5991122, 6034862, 6075313, 6112422, 6146142, + 6176430, 6203247, 6226559, 6246335, 6262551, 6275185, 6284220, 6289647, + 6291456, 6289647, 6284220, 6275185, 6262551, 6246335, 6226559, 6203247, + 6176430, 6146142, 6112422, 6075313, 6034862, 5991122, 5944148, 5893999, + 5840739, 5784436, 5725160, 5662986, 5597992, 5530259, 5459872, 5386917, + 5311485, 5233668, 5153561, 5071263, 4986873, 4900493, 4812225, 4722177, + 4630453, 4537163, 4442415, 4346320, 4248987, 4150530, 4051059, 3950687, + 3849526, 3747687, 3645282, 3542423, 3439219, 3335782, 3232218, 3128636, + 3025142, 2921841, 2818836, 2716228, 2614116, 2512598, 2411770, 2311723, + 2212550, 2114336, 2017169, 1921130, 1826299, 1732753, 1640566, 1549808, + 1460546, 1372846, 1286768, 1202370, 1119706, 1038826, 959779, 882608, +807354, 734054, 662741, 593446, 526194, 461009, 397911, 336916, +278037, 221283, 11, 114174, 63822, 15601, -30494, -74474, + -116348, -156134, -193847, -229509, -263143, -294772, -324425, -352132, + -377923, -401835, -423901, -444160, -462652, -479416, -494496, -507936, + -519779,
Re: delays in sensors thread
On Fri, Dec 11, 2020 at 09:07:45AM +0100, Marcus Glocker wrote: > > After doing some deeper analyzes in to asmc_wait() I agree to that. > Something seems to go fundamental wrong there. In every asmc_update() > execution, I can see asmc_wait() timeout 9 times, always on the > ASMC_ACCEPT check. That means out of ~48ms to execute asmc_update(), > we spend 45ms waiting for timeouts! > > When this can be fixed, I guess we should be fine with the remaining > ~3ms to execute asmc_update(). > Even 3ms seems the wrong order of magnitude; that's ~300 CPU cycles on a 1GHz processor. This is 3 usb frames, so this will still hurt audio in low-latency corner cases.
Re: delays in sensors thread
On Thu, Dec 10, 2020 at 05:27:16PM +0100, Marcus Glocker wrote: > Hi All, > > I recently started to play around with uvideo(4) and uaudio(4) on my > amd64 iMacs. There I quickly noticed regular freezes when streaming > USB video or audio. On some of those machines it was very frequent, > like every few seconds the video or audio stream did freeze for ~1s, > then resume, while the rest of the system did continue to operate fine. > > First I found that when running the machine with an SP kernel, the issue > disappears. On SP kernels, interrupts are still working while the CPU is spinning in kernel mode (as long as current IPL permits it). That's why audio works better. > Secondly some debugging hours, and quite some e-mail > exchanges with mpi@ later, I found that the freeze is getting triggered > by the asmc(4) driver, specifically by the sensor_task_register() > update function. My first intention was to change > sensor_task_register() to call taskq_create() with the TASKQ_MPSAFE > flag for a test, to remove the kernel lock, which also resolved the > freezing with an MP kernel. [1] > > In the end I found that the asmc(4) sensor update code is calling a > busy loop in asmc_wait(), where the delay call is spending ~50ms in > average. Doing that during the KERNEL_LOCK() is resulting in > noticeable USB ISOC transfer delays. Obviously replacing the delay(9) > with tsleep_nsec(9) in asmc(4) did fix the issue as well. [2] > > I'm not sure if just applying diff [2] to the driver is the right > approach finally or if we need to take a more generic path to address > this problem. Any feedback, help, comments appreciated. > Would asmc(4) work if we sleep for very long (tsleep_nsec() has no upper bound)? Spinning during 50ms in kernel mode doesn't look right, so using tsleep() looks as a step forward as long as sleeping doesn't break asmc(4). AFAIU, calling delay(10) + yield() would have a similar effect.
sio_open.3: clarify what sio_start() does
this wording is shorter and more precise and complete. ok? Index: sio_open.3 === RCS file: /cvs/src/lib/libsndio/sio_open.3,v retrieving revision 1.51 diff -u -p -r1.51 sio_open.3 --- sio_open.3 20 Nov 2020 12:09:45 - 1.51 +++ sio_open.3 27 Nov 2020 18:02:16 - @@ -387,17 +387,17 @@ bitmasks should always be used. .Ss Starting and stopping the device The .Fn sio_start -function puts the device in a waiting state: -the device will wait for playback data to be provided -(using the -.Fn sio_write -function). -Once enough data is queued to ensure that play buffers -will not underrun, actual playback is started automatically. -If record mode only is selected, then recording starts -immediately. +function prepares the devices to start. +Once the play buffer is full, i.e. +.Fa sio_par.bufsz +samples are queued with +.Fn sio_write , +playback starts automatically. +If record mode only is selected, then +.Fn sio_start +starts recording immediately. In full-duplex mode, playback and recording will start -synchronously as soon as enough data to play is available. +synchronously as soon as the play buffer is full. .Pp The .Fn sio_stop
Re: sndioctl.1: group is optional
On Fri, Nov 20, 2020 at 05:49:00PM +0100, Klemens Nanni wrote: > Not every control has a group as the manual wording says, i.e. > > $ sndioctl > input.level=0.486 > input.mute=0 > output.level=1.000 > output.mute=0 > app/aucat0.level=1.000 > > Feedack? OK? OK ratchov@ If the control has a group, the group must be specified (this is different from channel numbers: not specifying channel number means select "all channels"). > Index: sndioctl.1 > === > RCS file: /cvs/src/usr.bin/sndioctl/sndioctl.1,v > retrieving revision 1.14 > diff -u -p -r1.14 sndioctl.1 > --- sndioctl.127 May 2020 17:34:45 - 1.14 > +++ sndioctl.120 Nov 2020 16:46:45 - > @@ -72,8 +72,8 @@ The set of available controls depends on > Commands use the following two formats to display and change > controls respectively: > .Pp > -.Dl group/stream[channel].function > -.Dl group/stream[channel].function=value > +.Dl [group/]stream[channel].function > +.Dl [group/]stream[channel].function=value > .Pp > On the left-hand side are specified the control group (if any), > the affected stream name, and the optional channel number. > >
Re: AUDIORECDEVICE environment variable in sndio lib
On Tue, Nov 17, 2020 at 05:09:28PM +, Stuart Henderson wrote: > On 2020/11/17 17:13, Peter J. Philipp wrote: > > Hi, > > > > I have a mic on snd/1 and speakers on snd/0. I had tried a lot of different > > settings with audacity port but couldn't get this to work, so I chose the > > method of last resort. Below is a patch to allow an AUDIORECDEVICE > > environment > > variable specifying the wanted microphone. > > This seems a worthwhile addition. > > > Index: sio.c > > === > > RCS file: /cvs/src/lib/libsndio/sio.c,v > > retrieving revision 1.24 > > diff -u -p -u -r1.24 sio.c > > --- sio.c 29 Jun 2019 06:05:26 - 1.24 > > +++ sio.c 17 Nov 2020 16:13:04 - > > @@ -43,6 +43,7 @@ sio_open(const char *str, unsigned int m > > { > > static char devany[] = SIO_DEVANY; > > struct sio_hdl *hdl; > > + char *rec = NULL; > > > > #ifdef DEBUG > > _sndio_debug_init(); > > @@ -55,6 +56,12 @@ sio_open(const char *str, unsigned int m > > str = getenv("AUDIODEVICE"); > > if (str == NULL) > > str = devany; > > + rec = getenv("AUDIORECDEVICE"); > > + if (rec == NULL) > > + rec = devany; > > + > > + if (rec && mode == SIO_REC) > > + str = rec; > > If AUDIORECDEVICE is unset, it would be better to fallback to > AUDIODEVICE rather than directly to devany. > Thanks; semarie suggested a similar diff, so below is an attempt to take into account all the suggestions: - add AUDIOPLAYDEVICE, to handle play-only devices as well. We've the very same problem for them. - use AUDIODEVICE if play-only (rec-only) mode is used but the corresponding AUDIOPLAYDEVICE (AUDIORECDEVICE) var is not defined - minimal update for AUDIO{PLAY,REC}DEVICE in the man page OK? Index: sio.c === RCS file: /cvs/src/lib/libsndio/sio.c,v retrieving revision 1.24 diff -u -p -r1.24 sio.c --- sio.c 29 Jun 2019 06:05:26 - 1.24 +++ sio.c 18 Nov 2020 09:49:24 - @@ -52,7 +52,12 @@ sio_open(const char *str, unsigned int m if (str == NULL) /* backward compat */ str = devany; if (strcmp(str, devany) == 0 && !issetugid()) { - str = getenv("AUDIODEVICE"); + if ((mode & SIO_PLAY) == 0) + str = getenv("AUDIORECDEVICE"); + if ((mode & SIO_REC) == 0) + str = getenv("AUDIOPLAYDEVICE"); + if (mode == (SIO_PLAY | SIO_REC) || str == NULL) + str = getenv("AUDIODEVICE"); if (str == NULL) str = devany; } Index: sndio.7 === RCS file: /cvs/src/lib/libsndio/sndio.7,v retrieving revision 1.24 diff -u -p -r1.24 sndio.7 --- sndio.7 18 Jul 2020 05:01:14 - 1.24 +++ sndio.7 18 Nov 2020 09:49:24 - @@ -151,9 +151,11 @@ If .Cm default is used as the audio device, the program will use the one specified in the -.Ev AUDIODEVICE -environment variable. -If it is not set, the program first tries to connect to +.Ev AUDIODEVICE , AUDIOPLAYDEVICE +and/or +.Ev AUDIORECDEVICE +environment variables. +If they are not set, the program first tries to connect to .Li snd/0 . If that fails, it then tries to use .Li rsnd/0 . @@ -190,10 +192,20 @@ and contains 128 bits of raw random data If a session needs to be shared between multiple users, they can connect to the server using the same cookie. .Sh ENVIRONMENT -.Bl -tag -width "AUDIODEVICEXXX" -compact +.Bl -tag -width "AUDIOPLAYDEVICE" -compact .It Ev AUDIODEVICE Audio device descriptor to use when no descriptor is explicitly specified to a program. +.It Ev AUDIOPLAYDEVICE +Audio device descriptor to use for play-only mode +when no descriptor is explicitly specified to a program. +Overrides +.Ev AUDIODEVICE . +.It Ev AUDIORECDEVICE +Audio device descriptor to use for record-only mode +when no descriptor is explicitly specified to a program. +Overrides +.Ev AUDIODEVICE . .It Ev MIDIDEVICE MIDI port descriptor to use when no descriptor is explicitly specified to a program.
Re: mixerctl names
On Sat, Oct 17, 2020 at 05:52:58PM +0200, Jan Stary wrote: > Currently, mixerctl.conf(5) says > > Most devices have a number of digital to analogue converters > (DACs), used for sound playback, and each DAC has a corresponding > output mixer. The mixers are labelled āmixā or āselā. > > That doesn't seem to be the case, at least not universaly > as the wording seems to imply. For example, this is > mixerctl output on a Thinkpad T400: > > inputs.dac-0:1=222,222 > inputs.dac-2:3=222,222 > inputs.beep=0 > record.adc-2:3_source=mic2 > record.adc-2:3=219,219 > record.adc-0:1_source=mic > record.adc-0:1=219,219 > outputs.hp_source=dac-0:1 > outputs.hp_boost=on > inputs.mic=189,189 > outputs.mic_dir=input-vr80 > outputs.spkr_source=dac-2:3 > outputs.spkr_eapd=on > inputs.mic2=189,189 > outputs.hp_sense=unplugged > outputs.mic_sense=unplugged > outputs.master=240,240 > outputs.master.mute=off > outputs.master.slaves= > record.volume=240,240 > record.volume.mute=off > record.volume.slaves= > record.enable=sysctl > > Apparently, it has two DACS (for the speakers and the headphones). > The current wording might confuse the user into thinking he has > no output mixer, but the > > inputs.dac-0:1=222,222 > inputs.dac-2:3=222,222 > > do control the respective volumes, > while no "mix" or "sel" exists. > > Similarly for recording via the two ADCs. > It depends on the hardware. Basically, azalia exposes the schematic of the mixer and, in turn, the driver exposes all "widgets" to the upper layers, with a generated name. This is very similar to uaudio. Certain devices have no "mix_xxx" widgets others have. On one of my machines: $ doas mixerctl | egrep '(inputs|outputs).mix' | wc -l 20 desktop computers have many jacks and tend to have many mixers. The "sel_xxx" widgets are rare but still exist.
Re: pledge(2) sndioctl(1)
On Fri, May 22, 2020 at 08:10:54AM +0100, Ricardo Mestre wrote: > Hello, > > I tried to open the raw device but now it seems I was to sleepy to > figure out that I couldn't access it due to sndiod(8) having the device > opened earlier and therefore coundn't reach that code path. > > Here's the audio promise added, but maybe it raises the question again > if these utilities should have direct access to the devices, and also > includes sndiod(8) not running when this is done? > > ratchov@ will this change eventually? Otherwise the below keeps that > code path happy. As devices nodes are disabled by default for regular users, the "audio" promise doesn't add additional possibilities for regular users. On the other hand, running sndioctl as root on the raw device is useful for testing and experimenting. ok ratchov
Re: Audio over hdmi
On Fri, May 01, 2020 at 05:16:10PM +0200, Damien Couderc wrote: > > So if I'm not wrong it could be possible to set the -f option with the > analog device and the -F option with the digital-only one. > > That said, it would work only if you have two audio device (e.g. HDMI from > video card as audio0 and analog from soundcard as audio1). > > This is not true on Thinkpad laptops for example because they have two > output codecs on the same device for both analog and digital (and only one > is kept actually). > > Maybe we could make first analog and first digital codec available on each > audio device. Then sndiod would take the first analog and the first digital > from the devices in the given order. Does it sound sane? -F is just a tool to *ease* switching between devices, but doesn't solve the "default device problem" completely. It's intended to be used as follows: -f rsnd/0 -F rsnd/1 -F rsnd/2 ... which might become the default. This way if there's only one sound-card, it's used. If the user connects a new device, the new one is used until it's disconnected. That works by pure luck because attach order maches device priority. To apply this model to hdmi, there must be a mean to detect if it's connected to speakers and make it fail if it is not, so it can be skipped. If we can't detect whether it's connected, we must be sure it won't be used instead of the others. But even if hdmi has properly detected speakers, the user may want to use the analog one because it is more capable, for instance for telephony. AFAICS, this general problem can't be solved by just adding the hdmi device with the proper minor number, changes will be needed in many places of the audio sub-system. In the short term if you make sure the digital codec and the hdmi-only device properly work, never attache as audio0 and speakers are detected, the current defaults will work and nobody will complain. This would be a big step forward. my 2 cents
Re: Audio over hdmi
On Fri, May 01, 2020 at 03:01:04PM +0200, Mark Kettenis wrote: > > Date: Fri, 1 May 2020 14:17:56 +0200 > > From: Alexandre Ratchov > > > > On Fri, May 01, 2020 at 01:11:16PM +0200, Damien Couderc wrote: > > > > > > Speaking of the hdmi-only devices that were disabled in 2009: does the > > > project still stand on this position in 2020? I made a quick search and it > > > seems that more than half of the screens are audio capable now. I > > > understand > > > the defaults back in 2009, but now is it still true? > > > > There's nothing wrong with hdmi-only devices. As long as audio works > > by default with no tweaks, nobody will object to re-enabling > > them. AFAIK, this was the only reason to disable them. > > Right. The main issue was that by default we only send output to > audio0. On many machines the audio device associated with the HDMI > port appears before the audio device that is associated with the > speakers and/or headphone jack on our laptops. Therefore by default > audio would go to an unconnected HDMI. Just enabling digital-only > devices would not work. > > There are a couple things we could do here. > > 1. Make sndiod(8) responsible for picking a default output device. > > 2. Use locators (like I did for drm(4) and wsdisplay(4)) such that >audio0 is always the non-HDMI audio device. > > Option #2 has the downside that if your HDMI audio device is the only > supported audio device in the system, it will still be audio1 and > therefore not the default device. On the other hand that has the > benefit that if you later plug something like uaudio(4) into your > system it will become the default device. Since recently, sndiod can switch between devices. So with minimal tweaks we could make it use audio1 if audio0 is not present. Detecting if the hdmi codec is commected could ease this further > Option #1 would give us more flexibility, but I'm not sure if the > current audio(4) ioctls allow us to implement the proper heuristics. > > > > About the multi-codec devices, how do you see it ? Keeping all the codecs > > > and adding a knob to switch between analog and digital to select the > > > codec > > > This seems to make sense. > > It's never entirely clear to me what exactly a "codec" is. Basically there's are two chips: a PCI chip (the host) that does only DMA and talks through a simple serial link to the other chip (the codec) which has a bunch of DACs, ADCs, optical transmitters, the HDMI interfaces and so on. The interface between these is standardized. HDA host chips can handle multiple codecs, not sure if they are trully independent, specs say they are.
Re: Default device in audioctl and mixerctl
On Fri, May 01, 2020 at 01:34:54PM +0200, Damien Couderc wrote: > Hi, > > I noticed that audioctl and mixerctl both use /dev/audioctl0 as a default > since the reimplementation of the new api. > > Shouldn't it use the /dev/audioctl link instead to permit choosing which > device we want as the default? > There's no audioctl symlink anymore, it was removed few years ago. The default device is the one set with sndiod_flags, this way the same device is used to play, record and volume control. {audio,mixerctl}ctl are lower layer tools which are no longer for everyday use and don't participate to the default device logic
Re: Audio over hdmi
On Fri, May 01, 2020 at 01:11:16PM +0200, Damien Couderc wrote: > > Speaking of the hdmi-only devices that were disabled in 2009: does the > project still stand on this position in 2020? I made a quick search and it > seems that more than half of the screens are audio capable now. I understand > the defaults back in 2009, but now is it still true? There's nothing wrong with hdmi-only devices. As long as audio works by default with no tweaks, nobody will object to re-enabling them. AFAIK, this was the only reason to disable them. > About the multi-codec devices, how do you see it ? Keeping all the codecs > and adding a knob to switch between analog and digital to select the codec ? This seems to make sense.
Re: Install: Invalid group _sndiop in #163 amd64
On Wed, Apr 29, 2020 at 01:26:51PM +0200, Otto Moerbeek wrote: > On Wed, Apr 29, 2020 at 01:02:29PM +0200, Otto Moerbeek wrote: > > > On Wed, Apr 29, 2020 at 10:28:37AM +, Oliver Marugg wrote: > > > > > Possible typo group _sniop shoud be _sndiod: > > > > Nope. _sniop is the correct name. The real issue is that that group isn't in > > the /etc/group file used during install. > > > > How that can happen I do not know yet. > > Just did an install using install67.fg and did not see that error. > There was a snap ~1-2 weeks ago with a broken bsd.rd (with /etc/groups missing _sndiop). Now this is fixed.
Re: Audio over hdmi
On Sat, Apr 25, 2020 at 05:16:03PM +0200, Damien Couderc wrote: > > > > I can see in the full dmesg that there are two different FTYPE results > > provided during azalia_codec_init and only the first one seems to be > > displayed in the mixerctl output. > > > > I think that maybe mixerctl does not recurse all the audio functions or > > that some bits aren't recorded. I'll take a look. > > Ok, I did a bit of reading and from what I understood there are 2 detected > codecs (0 and 2) for the same instance of azalia but the latest is deleted > in azalia_init_codecs() because there is only one that can be enabled. > > Alexandre could maybe tell us what is possible to do here. I think it's > maybe possible to keep all detected codecs but to change which one is > enabled. Hi, Sorry couldn't test your diff, my machines have one codec only. The azalia driver can handle only one codec at a time. Multiple codecs can't be presented as different devices because they can't be used at the same time. Quickly adding support for multiple codecs would need to expose "mixer" controls of all the codecs and the ability to switch between them. This may look difficult but in most (all?) real-world cases there is at most 1 analog and 1 digital codec and the latter has very few controls, if any. Independenty, support for the hdmi-only devices, present on certain graphics cards was removed as they were not useful, but used to attach as audio0, i.e. they took the place of the default audio device, which made audio not work by default. But this is not related to your patch. HTH
mixerctl: use /dev/audioctl0 instead of /dev/mixer by default
The /dev/audioctlN files, when used in O_WRONLY mode offer the same functionality as /dev/mixerN. Ports don't use /dev/mixerN anymore, by switching mixerctl(4) to /dev/audioctlN too, we remove the last /dev/mixerN user. OK? Index: mixerctl.1 === RCS file: /cvs/src/usr.bin/mixerctl/mixerctl.1,v retrieving revision 1.35 diff -u -p -r1.35 mixerctl.1 --- mixerctl.1 30 Jul 2018 17:24:25 - 1.35 +++ mixerctl.1 2 Apr 2020 20:22:34 - @@ -68,7 +68,7 @@ This is the default, if no parameters ar .It Fl f Ar file Specify an alternative audio mixing device. The default is -.Pa /dev/mixer0 . +.Pa /dev/audioctl0 . .It Fl n Suppress printing of the variable name. .It Fl q @@ -153,7 +153,7 @@ The audio mixer device to use. .El .Sh FILES .Bl -tag -width "/etc/mixerctl.confXXX" -compact -.It Pa /dev/mixer0 +.It Pa /dev/audioctl0 Default mixer audio device. .It Pa /etc/mixerctl.conf .Nm Index: mixerctl.c === RCS file: /cvs/src/usr.bin/mixerctl/mixerctl.c,v retrieving revision 1.32 diff -u -p -r1.32 mixerctl.c --- mixerctl.c 28 Jun 2019 13:35:02 - 1.32 +++ mixerctl.c 2 Apr 2020 20:22:34 - @@ -249,7 +249,7 @@ main(int argc, char **argv) int ndev; if ((file = getenv("MIXERDEVICE")) == 0 || *file == '\0') - file = "/dev/mixer"; + file = "/dev/audioctl0"; while ((ch = getopt(argc, argv, "af:nqtvw")) != -1) { switch (ch) { @@ -284,15 +284,12 @@ main(int argc, char **argv) if (argc == 0 && tflag == 0) aflag = 1; - if (unveil(file, "rw") == -1) + if (unveil(file, "w") == -1) err(1, "unveil"); - if ((fd = open(file, O_RDWR)) == -1) { + if ((fd = open(file, O_WRONLY)) == -1) { if (unveil(file, "r") == -1) err(1, "unveil"); - - if ((fd = open(file, O_RDONLY)) == -1) - err(1, "%s", file); } if (unveil(NULL, NULL) == -1)
Re: libossaudio: start using sndio
On Thu, Apr 02, 2020 at 05:21:45PM +0100, Stuart Henderson wrote: > On 2020/04/02 17:13, Landry Breuil wrote: > > On Wed, Apr 01, 2020 at 07:27:12PM +0200, Alexandre Ratchov wrote: > > > ping! > > > > > > FWIW, the diff below affects the following ports: > > > - emulators/gambatte > > > - gstreamer-0.10 mixer plugin > > > - sysutils/conky > > > - sysutils/gkrellm > > > - sysutil/tpb > > > - telephony/iaxclient > > > - anything depending on gstreamer, ex xfce4-mixer > > > > > > If you use one of above ports and want to test, just apply this diff > > > to src/lib/libossaudio in base, rebuild it and reinstall it. The ABI > > > is not changing, so no need to rebuild or update ports. > > btw, most of these ports are either not really used (tpb, iaxclient), > or where the mixer functionality isn't hugely important (conky, gkrellm), > so Landry's tests here should cover things fairly well. I've tested conky and gkrellm, it works. They show the new controls, but this is an improvement, imho
Re: libossaudio: start using sndio
On Thu, Apr 02, 2020 at 05:13:49PM +0200, Landry Breuil wrote: > On Wed, Apr 01, 2020 at 07:27:12PM +0200, Alexandre Ratchov wrote: > > ping! > > > > FWIW, the diff below affects the following ports: > > - emulators/gambatte > > - gstreamer-0.10 mixer plugin > > - sysutils/conky > > - sysutils/gkrellm > > - sysutil/tpb > > - telephony/iaxclient > > - anything depending on gstreamer, ex xfce4-mixer > > > > If you use one of above ports and want to test, just apply this diff > > to src/lib/libossaudio in base, rebuild it and reinstall it. The ABI > > is not changing, so no need to rebuild or update ports. > > I've briefly tested it with xfce4-mixer which uses gst 0.10 - on a t430u > with: > > azalia0 at pci0 dev 27 function 0 "Intel 7 Series HD Audio" rev 0x04: msi > azalia0: codecs: Realtek ALC269, Intel/0x2806, using Realtek ALC269 > > previously, it saw two controls, one for the output acting/wired to > mixerctl values inputs.dac-0:1 & inputs.dac-2:3 (ie both changes upon > control changes), and one for the input wired to inputs.mic > > with the diff, xfce4-mixer now sees the 3 expected controls: > * 'volume', wired to aucatctl 'master' / sndioctl 'output.level' value > * 'input gain', wired to mixerctl record.volume, record.adc-0:1 & > * record.adc-2:3 (and to sndioctl hw/input.level) > * 'output gain', wired to mixerctl outputs.master, inputs.dac-0:1 & > * inputs.dac-2:3 (and to sndioctl hw/output.level) > > trying to toggle the 'mute' button in xfce4-mixer (for the 'input gain' > only) leads to 'Error setting mixer recording devices (0x0): Invalid > argument' msgs on stderr but im not sure it matters. > This is "ok", it tries to select the input source, but as there's only one choice exposed it complains on stderr. > toggling the mute button for volume and input gain set the level to 0 > instead of toggling the hw/output.mute button but im not sure it's a big > deal either. This is expected as well. The mute controls, instead xfce4-mixer saves the current level and sets it to zero. When the control is unmuted, it sets the level to the saved value. > all in all that seems an improvement to me, at least does what's > expected for gst 0.10/xfce4-mixer. > Many thanks for looking at this.
libossaudio: start using sndio
ping! FWIW, the diff below affects the following ports: - emulators/gambatte - gstreamer-0.10 mixer plugin - sysutils/conky - sysutils/gkrellm - sysutil/tpb - telephony/iaxclient - anything depending on gstreamer, ex xfce4-mixer If you use one of above ports and want to test, just apply this diff to src/lib/libossaudio in base, rebuild it and reinstall it. The ABI is not changing, so no need to rebuild or update ports. - Forwarded message from Alexandre Ratchov - Date: Tue, 17 Mar 2020 12:27:40 +0100 From: Alexandre Ratchov To: po...@openbsd.org Subject: libossaudio: important diff to test User-Agent: Mutt/1.11.4 (2019-03-13) Hi, This diff makes libossaudio use sndio instead of the kernel mixer(4) interface. I post it to ports@ as it's only used by few ports. Switching libossaudio to sndio implies that programs using it will always use the right device (instead of the first one) and will always see at least the sndiod master level control. With this diff, libossaudio will expose the following controls: - output.level, sndiod master level, always present - hw/output.level, hardware output level, present on certain devices only - hw/input.level, hardware input level, present on certain devices only OK? -- Alexandre Index: lib/libossaudio/Makefile === RCS file: /cvs/src/lib/libossaudio/Makefile,v retrieving revision 1.5 diff -u -p -u -p -r1.5 Makefile --- lib/libossaudio/Makefile16 Jul 2014 20:02:17 - 1.5 +++ lib/libossaudio/Makefile22 Feb 2020 16:07:05 - @@ -4,9 +4,11 @@ LIB= ossaudio MAN= ossaudio.3 -SRCS= ossaudio.c +SRCS= ossaudio.c aucat.c debug.c sioctl.c sioctl_aucat.c sioctl_sun.c CPPFLAGS+= -I${.CURDIR} + +.PATH: ${.CURDIR}/../libsndio includes: @cd ${.CURDIR}; cmp -s soundcard.h ${DESTDIR}/usr/include/soundcard.h || \ Index: lib/libossaudio/ossaudio.c === RCS file: /cvs/src/lib/libossaudio/ossaudio.c,v retrieving revision 1.20 diff -u -p -u -p -r1.20 ossaudio.c --- lib/libossaudio/ossaudio.c 28 Jun 2019 13:32:42 - 1.20 +++ lib/libossaudio/ossaudio.c 22 Feb 2020 16:07:05 - @@ -36,26 +36,38 @@ #include #include #include -#include -#include #include - +#include +#include +#include +#include #include "soundcard.h" -#undef ioctl -#define GET_DEV(com) ((com) & 0xff) +#ifdef DEBUG +#define DPRINTF(...) do { fprintf(stderr, __VA_ARGS__); } while (0) +#else +#define DPRINTF(...) do {} while (0) +#endif -#define TO_OSSVOL(x) (((x) * 100 + 127) / 255) -#define FROM_OSSVOL(x) x) > 100 ? 100 : (x)) * 255 + 50) / 100) +#define GET_DEV(com) ((com) & 0xff) +#define INTARG (*(int*)argp) -static struct audiodevinfo *getdevinfo(int); +struct control { + struct control *next; + int type; /* one of SOUND_MIXER_xxx */ + int chan; /* 0 -> left, 1 -> right, -1 -> mono */ + int addr; /* sioctl control id */ + int value; /* current value */ + int max; +}; static int mixer_ioctl(int, unsigned long, void *); -static int opaque_to_enum(struct audiodevinfo *di, audio_mixer_name_t *label, int opq); -static int enum_to_ord(struct audiodevinfo *di, int enm); -static int enum_to_mask(struct audiodevinfo *di, int enm); -#define INTARG (*(int*)argp) +static int initialized; +static struct control *controls; +static struct sioctl_hdl *hdl; +static char *dev_name = SIO_DEVANY; +static struct pollfd *pfds; int _oss_ioctl(int fd, unsigned long com, ...) @@ -71,201 +83,163 @@ _oss_ioctl(int fd, unsigned long com, .. else if (IOCGROUP(com) == 'M') return mixer_ioctl(fd, com, argp); else - return ioctl(fd, com, argp); + return (ioctl)(fd, com, argp); } -/* If the mixer device should have more than MAX_MIXER_DEVS devices - * some will not be available to Linux */ -#define MAX_MIXER_DEVS 64 -struct audiodevinfo { - int done; - dev_t dev; - ino_t ino; - int16_t devmap[SOUND_MIXER_NRDEVICES], - rdevmap[MAX_MIXER_DEVS]; - char names[MAX_MIXER_DEVS][MAX_AUDIO_DEV_LEN]; - int enum2opaque[MAX_MIXER_DEVS]; -u_long devmask, recmask, stereomask; - u_long caps, recsource; -}; - -static int -opaque_to_enum(struct audiodevinfo *di, audio_mixer_name_t *label, int opq) +/* + * new control + */ +static void +mixer_ondesc(void *unused, struct sioctl_desc *d, int val) { - int i, o; + struct control *i, **pi; + int type; - for (i = 0; i < MAX_MIXER_DEVS; i++) { - o = di->enum2opaque[i]; - if (o == opq) - break; - if (o == -1 && label != NULL && - !strncmp(di->names[i], label-&g
Re: umidi: missing NULL checks
On Mon, Mar 23, 2020 at 10:51:06PM +0100, Tobias Heider wrote: > In alloc_all_jacks() the variables 'sc_in_jacks' and 'sc_out_checks' > are set to NULL if 'sc_in_num_jacks' and 'sc_out_num_jacks' are 0. > Further down both are dereferenced unconditionally. I added explicit NULL > checks where I think they belong. > I think 'sc_in_ep' and 'sc_out_ep' can also be NULL, but I am not > entirely sure about those. > > Objections or ok? > sc_{out,in}_jacks is used only if sc_out_num_jacks > 0, i.e. if there's at least one iteration. But if sc_out_num_jacks > 0, then sc_{out,in}_jacks is properly initialized by this chunk: sc->sc_out_jacks = sc->sc_out_num_jacks ? sc->sc_jacks : NULL; sc->sc_in_jacks = sc->sc_in_num_jacks ? sc->sc_jacks + sc->sc_out_num_jacks : NULL; AFAICS, the code is correct; but the style looks misleading to me. > Index: dev/usb/umidi.c > === > RCS file: /mount/openbsd/cvs/src/sys/dev/usb/umidi.c,v > retrieving revision 1.53 > diff -u -p -r1.53 umidi.c > --- dev/usb/umidi.c 16 Mar 2020 16:12:43 - 1.53 > +++ dev/usb/umidi.c 23 Mar 2020 21:50:49 - > @@ -724,50 +724,58 @@ alloc_all_jacks(struct umidi_softc *sc) > sc->sc_in_jacks = > sc->sc_in_num_jacks ? sc->sc_jacks+sc->sc_out_num_jacks : NULL; > > - jack = >sc_out_jacks[0]; > - for (i=0; isc_out_num_jacks; i++) { > - jack->opened = 0; > - jack->binded = 0; > - jack->arg = NULL; > - jack->u.out.intr = NULL; > - jack->intr = 0; > - jack->cable_number = i; > - jack++; > + if (sc->sc_out_jacks) { > + jack = >sc_out_jacks[0]; > + for (i=0; isc_out_num_jacks; i++) { > + jack->opened = 0; > + jack->binded = 0; > + jack->arg = NULL; > + jack->u.out.intr = NULL; > + jack->intr = 0; > + jack->cable_number = i; > + jack++; > + } > } > - jack = >sc_in_jacks[0]; > - for (i=0; isc_in_num_jacks; i++) { > - jack->opened = 0; > - jack->binded = 0; > - jack->arg = NULL; > - jack->u.in.intr = NULL; > - jack->cable_number = i; > - jack++; > + if (sc->sc_in_jacks) { > + jack = >sc_in_jacks[0]; > + for (i=0; isc_in_num_jacks; i++) { > + jack->opened = 0; > + jack->binded = 0; > + jack->arg = NULL; > + jack->u.in.intr = NULL; > + jack->cable_number = i; > + jack++; > + } > } > > /* assign each jacks to each endpoints */ > - jack = >sc_out_jacks[0]; > - ep = >sc_out_ep[0]; > - for (i=0; isc_out_num_endpoints; i++) { > - rjack = >jacks[0]; > - for (j=0; jnum_jacks; j++) { > - *rjack = jack; > - jack->endpoint = ep; > - jack++; > - rjack++; > + if (sc->sc_out_jacks && sc->sc_out_ep) { > + jack = >sc_out_jacks[0]; > + ep = >sc_out_ep[0]; > + for (i=0; isc_out_num_endpoints; i++) { > + rjack = >jacks[0]; > + for (j=0; jnum_jacks; j++) { > + *rjack = jack; > + jack->endpoint = ep; > + jack++; > + rjack++; > + } > + ep++; > } > - ep++; > } > - jack = >sc_in_jacks[0]; > - ep = >sc_in_ep[0]; > - for (i=0; isc_in_num_endpoints; i++) { > - rjack = >jacks[0]; > - for (j=0; jnum_jacks; j++) { > - *rjack = jack; > - jack->endpoint = ep; > - jack++; > - rjack++; > + if (sc->sc_in_jacks && sc->sc_in_ep) { > + jack = >sc_in_jacks[0]; > + ep = >sc_in_ep[0]; > + for (i=0; isc_in_num_endpoints; i++) { > + rjack = >jacks[0]; > + for (j=0; jnum_jacks; j++) { > + *rjack = jack; > + jack->endpoint = ep; > + jack++; > + rjack++; > + } > + ep++; > } > - ep++; > } > > return USBD_NORMAL_COMPLETION; >
Re: Fix brightness control on ASUS 1005PXD
On Mon, Mar 16, 2020 at 10:16:34AM +0100, Patrick Wildt wrote: > Otherwise, if we want to do that in acpivout_get_brightness(), > I guess we can update acpivout_select_brightness() and its caller > to remove the check for -1, since there will be no -1 anymore? > Sure, I missed those. Here's a new diff with the checks removed. Tested and works as expected on my machne. Index: acpivout.c === RCS file: /cvs/src/sys/dev/acpi/acpivout.c,v retrieving revision 1.19 diff -u -p -r1.19 acpivout.c --- acpivout.c 8 Feb 2020 19:08:17 - 1.19 +++ acpivout.c 16 Mar 2020 14:42:17 - @@ -227,8 +227,10 @@ acpivout_get_brightness(struct acpivout_ aml_freevalue(); DPRINTF(("%s: BQC = %d\n", DEVNAME(sc), level)); - if (level < sc->sc_bcl[0] || level > sc->sc_bcl[sc->sc_bcl_len -1]) - level = -1; + if (level < sc->sc_bcl[0]) + level = sc->sc_bcl[0]; + else if (level > sc->sc_bcl[sc->sc_bcl_len - 1]) + level = sc->sc_bcl[sc->sc_bcl_len - 1]; return (level); } @@ -239,9 +241,6 @@ acpivout_select_brightness(struct acpivo int nindex, level; level = sc->sc_brightness; - if (level == -1) - return level; - nindex = acpivout_find_brightness(sc, nlevel); if (sc->sc_bcl[nindex] == level) { if (nlevel > level && (nindex + 1 < sc->sc_bcl_len)) @@ -375,13 +374,11 @@ acpivout_set_param(struct wsdisplay_para } if (sc != NULL && sc->sc_bcl_len != 0) { nindex = acpivout_select_brightness(sc, dp->curval); - if (nindex != -1) { - sc->sc_brightness = sc->sc_bcl[nindex]; - acpi_addtask(sc->sc_acpi, - acpivout_set_brightness, sc, 0); - acpi_wakeup(sc->sc_acpi); - return 0; - } + sc->sc_brightness = sc->sc_bcl[nindex]; + acpi_addtask(sc->sc_acpi, + acpivout_set_brightness, sc, 0); + acpi_wakeup(sc->sc_acpi); + return 0; } return -1; default:
Re: Fix brightness control on ASUS 1005PXD
On Mon, Mar 16, 2020 at 10:16:34AM +0100, Patrick Wildt wrote: > On Sat, Mar 14, 2020 at 04:28:26AM +0100, Alexandre Ratchov wrote: > > On ASUS 1001PXD, _BQC returns an out of range value which makes > > acpivout_get_brightness() return -1, in turn breaking the > > display.brightness control (wsconsctl displays a mangled value). > > > > This diff ignores the out of range value and makes the brighness > > control just work again. > > > > OK? > > With the current code _BQC is only called on attach, where we > probably want a value higher than the lowest one. I wonder if > maybe we should move this check into the attach functions, and > in case of an error like this, set a reasonable brightness? Just to be sure that I understand, do you suggest to change the brightness at attach time?
Fix brightness control on ASUS 1005PXD
On ASUS 1001PXD, _BQC returns an out of range value which makes acpivout_get_brightness() return -1, in turn breaking the display.brightness control (wsconsctl displays a mangled value). This diff ignores the out of range value and makes the brighness control just work again. OK? Index: acpivout.c === RCS file: /cvs/src/sys/dev/acpi/acpivout.c,v retrieving revision 1.19 diff -u -p -r1.19 acpivout.c --- acpivout.c 8 Feb 2020 19:08:17 - 1.19 +++ acpivout.c 14 Mar 2020 03:19:02 - @@ -227,8 +227,10 @@ acpivout_get_brightness(struct acpivout_ aml_freevalue(); DPRINTF(("%s: BQC = %d\n", DEVNAME(sc), level)); - if (level < sc->sc_bcl[0] || level > sc->sc_bcl[sc->sc_bcl_len -1]) - level = -1; + if (level < sc->sc_bcl[0]) + level = sc->sc_bcl[0]; + else if (level > sc->sc_bcl[sc->sc_bcl_len - 1]) + level = sc->sc_bcl[sc->sc_bcl_len - 1]; return (level); }
Re: Audio control API
On Tue, Feb 25, 2020 at 01:02:06PM +0100, Alexandre Ratchov wrote: > On Thu, Feb 13, 2020 at 04:52:12AM +, Raf Czlonka wrote: > > > > Hi Alexandre, > > > > I have to say that I also find the two ranges mildly confusing, > > i.e. 0-255 in one place, and 0-127 in another. In terms of units, > > personally, I'm used to, and quite like, the granularity of 0-255. > > > > Again, not my place so others will certainly be more help here. > > > > One more point regarding the interface, though. > > > > This is the way mixerctl(1) currently behaves: > > > > $ mixerctl outputs.master > > outputs.master=255,255 > > $ mixerctl outputs.master=100 > > outputs.master: 255,255 -> 100,100 > > $ mixerctl outputs.master=300 > > outputs.master: 100,100 -> 255,255 > > > > Should sndioctl(1) behave the same way? > > Many thanks for the feedback. > > After some thinking and experimenting, floats in the [0:1] seem the > simplest option. It avoids discussions about preferences and allows > arbitrary precision (if needed, later). > > Furthermore, as all controls are in the [0:1] range, it makes sense to > request the user to provide numbers between 0 and 1. Providing numbers > outside this range indicates he is misunderstanding how the program > works. > > Below are all 3 base diffs combined (libsndio, sndiod, sndioctl, > libossaudio), to ease testing. And here's the ports part of the diff including audio/gqmpeg, sysutils/tray-app and x11/i3status ports. Index: sysutils/tray-app/Makefile === RCS file: /cvs/ports/sysutils/tray-app/Makefile,v retrieving revision 1.8 diff -u -p -u -p -r1.8 Makefile --- sysutils/tray-app/Makefile 12 Jul 2019 20:49:53 - 1.8 +++ sysutils/tray-app/Makefile 22 Feb 2020 16:06:19 - @@ -5,7 +5,7 @@ ONLY_FOR_ARCHS= ${APM_ARCHS} COMMENT= small utilities for X11 system tray: eject, battery, mixer DISTNAME= tray-app-0.3.1 -REVISION= 0 +REVISION= 1 CATEGORIES=sysutils x11 Index: sysutils/tray-app/patches/patch-sound_Makefile === RCS file: /cvs/ports/sysutils/tray-app/patches/patch-sound_Makefile,v retrieving revision 1.1.1.1 diff -u -p -u -p -r1.1.1.1 patch-sound_Makefile --- sysutils/tray-app/patches/patch-sound_Makefile 17 Sep 2013 11:21:50 - 1.1.1.1 +++ sysutils/tray-app/patches/patch-sound_Makefile 22 Feb 2020 16:06:19 - @@ -1,6 +1,7 @@ $OpenBSD: patch-sound_Makefile,v 1.1.1.1 2013/09/17 11:21:50 sthen Exp $ sound/Makefile.origMon Mar 12 08:46:04 2012 -+++ sound/Makefile Tue Sep 17 11:39:18 2013 +Index: sound/Makefile +--- sound/Makefile.orig sound/Makefile @@ -8,13 +8,13 @@ MAN= gtk_CFLAGS!= pkg-config --cflags gtk+-2.0 @@ -8,7 +9,8 @@ $OpenBSD: patch-sound_Makefile,v 1.1.1.1 -CFLAGS= -W -Wall -g -O0 -I../lib $(gtk_CFLAGS) +CFLAGS+= -W -Wall -I../lib $(gtk_CFLAGS) LDFLAGS= -L../lib $(gtk_LDFLAGS) - LDADD= -ltrayapp +-LDADD= -ltrayapp ++LDADD= -ltrayapp -lsndio -BINDIR=/usr/local/libexec/tray-app +BINDIR=${TRUEPREFIX}/libexec/tray-app Index: sysutils/tray-app/patches/patch-sound_sound_c === RCS file: sysutils/tray-app/patches/patch-sound_sound_c diff -N sysutils/tray-app/patches/patch-sound_sound_c --- /dev/null 1 Jan 1970 00:00:00 - +++ sysutils/tray-app/patches/patch-sound_sound_c 22 Feb 2020 16:06:19 - @@ -0,0 +1,516 @@ +$OpenBSD$ + +Index: sound/sound.c +--- sound/sound.c.orig sound/sound.c +@@ -4,12 +4,13 @@ + */ + #include + #include +-#include ++#include + + #include + + #include + #include ++#include + #include + #include + +@@ -19,16 +20,20 @@ + #include "sound-1.xpm" + #include "sound-2.xpm" + ++struct control { ++ struct control *next; ++ unsigned int addr; ++ unsigned int value; ++ unsigned int max; ++ int ismute; ++}; ++ + static void usage(const char *prog); +-static intget_mixer_index(int fd, mixer_devinfo_t *devinfo); +-static intget_volume(int fd, mixer_devinfo_t *devinfo, u_char *volume); +-static intset_volume(int fd, mixer_devinfo_t *devinfo, u_char volume); +-static intget_mute(int fd, mixer_devinfo_t *devinfo, int *mute); +-static intset_mute(int fd, mixer_devinfo_t *devinfo, int mute); ++static void set_state(int ismute, int mute); ++static void get_state(int *rvolume, int *rmute); + + static void prepare_tooltip(int mute, u_char volume, char *text, size_t sz); + +-static gboolean cb_timer(GtkWidget *widget); + static void cb_button_toggled(GtkWidget *widget, gpointer data); + static void cb_scale_value_changed(GtkScale *scale, GtkAdjustment *adj
Re: Audio control API
open.3 mio_open.3 sndio.7 +MAN= sio_open.3 mio_open.3 sioctl_open.3 sndio.7 SRCS= debug.c aucat.c sio_aucat.c sio_sun.c sio.c \ - mio_rmidi.c mio_aucat.c mio.c + mio_rmidi.c mio_aucat.c mio.c \ + sioctl_aucat.c sioctl_sun.c sioctl.c CFLAGS+=-DDEBUG COPTS+=-Wall -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wundef Index: lib/libsndio/Symbols.map === RCS file: /cvs/src/lib/libsndio/Symbols.map,v retrieving revision 1.1 diff -u -p -u -p -r1.1 Symbols.map --- lib/libsndio/Symbols.map26 Dec 2017 19:12:22 - 1.1 +++ lib/libsndio/Symbols.map24 Feb 2020 06:03:28 - @@ -27,10 +27,22 @@ mio_revents; mio_eof; + sioctl_open; + sioctl_close; + sioctl_ondesc; + sioctl_onval; + sioctl_setval; + sioctl_nfds; + sioctl_pollfd; + sioctl_revents; + sioctl_eof; + mio_rmidi_getfd; mio_rmidi_fdopen; sio_sun_getfd; sio_sun_fdopen; + sioctl_sun_getfd; + sioctl_sun_fdopen; local: *; }; Index: lib/libsndio/amsg.h === RCS file: /cvs/src/lib/libsndio/amsg.h,v retrieving revision 1.12 diff -u -p -u -p -r1.12 amsg.h --- lib/libsndio/amsg.h 12 Jul 2019 06:30:55 - 1.12 +++ lib/libsndio/amsg.h 24 Feb 2020 06:03:28 - @@ -43,6 +43,11 @@ #define AUCAT_PORT 11025 /* + * limits + */ +#define AMSG_CTL_NAMEMAX 16 /* max name length */ + +/* * WARNING: since the protocol may be simultaneously used by static * binaries or by different versions of a shared library, we are not * allowed to change the packet binary representation in a backward @@ -64,6 +69,9 @@ struct amsg { #define AMSG_HELLO 10 /* say hello, check versions and so ... */ #define AMSG_BYE 11 /* ask server to drop connection */ #define AMSG_AUTH 12 /* send authentication cookie */ +#define AMSG_CTLSUB13 /* ondesc/onctl subscription */ +#define AMSG_CTLSET14 /* set control value */ +#define AMSG_CTLSYNC 15 /* end of controls descriptions */ uint32_t cmd; uint32_t __pad; union { @@ -108,7 +116,38 @@ struct amsg { #define AMSG_COOKIELEN 16 uint8_t cookie[AMSG_COOKIELEN]; } auth; + struct amsg_ctlsub { + uint8_t desc, val; + } ctlsub; + struct amsg_ctlset { + uint16_t addr, val; + } ctlset; } u; +}; + +/* + * network representation of sioctl_node structure + */ +struct amsg_ctl_node { + char name[AMSG_CTL_NAMEMAX]; + int16_t unit; + uint8_t __pad[2]; +}; + +/* + * network representation of sioctl_desc structure + */ +struct amsg_ctl_desc { + struct amsg_ctl_node node0; /* affected channels */ + struct amsg_ctl_node node1; /* dito for AMSG_CTL_{SEL,VEC,LIST} */ + char func[AMSG_CTL_NAMEMAX];/* parameter function name */ + char group[AMSG_CTL_NAMEMAX]; /* group of the control */ + uint8_t type; /* see sioctl_desc structure */ + uint8_t __pad1[1]; + uint16_t addr; /* control address */ + uint16_t maxval; + uint16_t curval; + uint32_t __pad2[3]; }; /* Index: lib/libsndio/shlib_version === RCS file: /cvs/src/lib/libsndio/shlib_version,v retrieving revision 1.11 diff -u -p -u -p -r1.11 shlib_version --- lib/libsndio/shlib_version 26 Dec 2017 15:23:33 - 1.11 +++ lib/libsndio/shlib_version 24 Feb 2020 06:03:28 - @@ -1,2 +1,2 @@ major=7 -minor=0 +minor=1 Index: lib/libsndio/sioctl.c === RCS file: lib/libsndio/sioctl.c diff -N lib/libsndio/sioctl.c --- /dev/null 1 Jan 1970 00:00:00 - +++ lib/libsndio/sioctl.c 24 Feb 2020 06:03:30 - @@ -0,0 +1,177 @@ +/* $OpenBSD$ */ +/* + * Copyright (c) 2014-2020 Alexandre Ratchov + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, A
Re: Audio control API, part 1: libsndio, sndiod bits
On Thu, Feb 13, 2020 at 05:15:34AM +, Raf Czlonka wrote: > On Sun, Feb 09, 2020 at 12:13:02PM GMT, Alexandre Ratchov wrote: > > +++ lib/libsndio/sioctl_aucat.c 8 Feb 2020 14:49:37 - > > [...] > > + * Copyright (c) 2010-2011 Alexandre Ratchov > > > > [...] > > > > +++ lib/libsndio/sioctl_open.3 8 Feb 2020 14:49:37 - > > [...] > > +.\" Copyright (c) 2011 Alexandre Ratchov > > > > [...] > > > > +++ lib/libsndio/sioctl_priv.h 8 Feb 2020 14:49:38 - > > [...] > > + * Copyright (c) 2008 Alexandre Ratchov > > > > [...] > > > > +++ lib/libsndio/sioctl_sun.c 8 Feb 2020 14:49:38 - > > [...] > > + * Copyright (c) 2010-2011 Alexandre Ratchov > > > > [...] > > > > +++ lib/libsndio/sioctl.c 8 Feb 2020 14:49:37 - > > [...] > > + * Copyright (c) 2008 Alexandre Ratchov > > > > [...] > > > > +++ usr.bin/sndioctl/sndioctl.1 9 Feb 2020 11:05:02 - > > [...] > > +.\" Copyright (c) 2007 Alexandre Ratchov > > > > [...] > > > > +++ usr.bin/sndioctl/sndioctl.c 9 Feb 2020 11:05:02 - > > [...] > > + * Copyright (c) 2007-2011 Alexandre Ratchov > > > > [...] > > > > +++ usr.bin/sndiod/dev_sioctl.c 8 Feb 2020 14:49:38 - > > [...] > > + * Copyright (c) 2014 Alexandre Ratchov > > > > [...] > > > > +++ usr.bin/sndiod/dev_sioctl.h 8 Feb 2020 14:49:38 - > > [...] > > + * Copyright (c) 2014 Alexandre Ratchov > > > > [...] > > > > Hi Alexandre, > > Shouldn't all of these dates be adjusted? > Sure; added 2020 as copyright year. Thanks.
Re: Audio control API, part 1: libsndio, sndiod bits
On Wed, Feb 12, 2020 at 09:28:38PM +0100, Jan Stary wrote: > Hi, > > On Feb 09 13:13:02, a...@caoua.org wrote: > > cd /usr/src > > patch -p0 <1.diff > > patch -p0 <2.diff > > patch -p0 <3.diff > > cd /usr/src/include && doas make includes > > cd /usr/src/lib/libsndio && make obj && make && doas make install > > cd /usr/src/lib/libossaudio && make obj && make && doas make install > > cd /usr/src/usr.bin/sndiod && make obj && make && doas make install > > cd /usr/src/usr.bin/sndioctl && make obj && make && doas make install > > doas rcctl restart sndiod > > cd /usr/ports > > patch -p0 <4.diff > > cd /usr/ports/audio/gqmpeg && make && make install > > cd /usr/ports/sysutils/tray-app && make && make install > > > > Feedback about the new features > > is of course welcome as well. > > sndioctl -m works fine with e.g. mplayer, > i.e. sndioctl notices the "added" app, > and reports the volume changes. great :) > With ffplay and firefox:youtube, volume changes are not reported. > Is that expected? Do they tweak their "internal" volume knob > but not the volume they register with sndio? As opposed to mplayer? Yes exactly. I haven't checked ffplay, but firefox uses an internal volume control because its internal API has no "getter", this is explained here: https://marc.info/?l=openbsd-ports=152641946326955
Re: Audio control API, part 1: libsndio, sndiod bits
On Wed, Feb 12, 2020 at 09:22:20PM +0100, Jan Stary wrote: > Hi, > > On Feb 09 13:13:02, a...@caoua.org wrote: > > cd /usr/src > > patch -p0 <1.diff > > patch -p0 <2.diff > > patch -p0 <3.diff > > cd /usr/src/include && doas make includes > > cd /usr/src/lib/libsndio && make obj && make && doas make install > > cd /usr/src/lib/libossaudio && make obj && make && doas make install > > cd /usr/src/usr.bin/sndiod && make obj && make && doas make install > > cd /usr/src/usr.bin/sndioctl && make obj && make && doas make install > > doas rcctl restart sndiod > > > > I'm very interested in any regression. > > After restarting sndiod (with empty flags), > starting sndioctl -m makes sndiod crash with > > Feb 12 21:02:31 box /bsd: sndiod[95433]: pledge "tty", syscall 54 > > That's ioctl(2). I don't see that in any of the three pledge(2) calls > revealed by a grep in (the patched) .../sndiod/, but apparently > the new code does call ioctl(). sndiod does unauthorized ioctls on this kernel, see below > > Strangely, this happens on > > OpenBSD 6.6-current (GENERIC.MP) #0: Tue Feb 4 17:33:19 CET 2020 > h...@box.stare.cz:/usr/src/sys/arch/amd64/compile/GENERIC.MP > > but not on > > OpenBSD 6.6-current (GENERIC.MP) #0: Sun Feb 9 17:30:47 CET 2020 > h...@dell.stare.cz:/usr/src/sys/arch/amd64/compile/GENERIC.MP > > (Did I miss somethong pledge() related in that window?) > Yes, there was a kernel commit to allow programs with the "audio" promise (like sndiod) to use the mixer ioctl()s.
Re: Audio control API, part 4: switch ports to sndio API
On Sun, Feb 09, 2020 at 01:27:27PM +0100, Alexandre Ratchov wrote: > There are two ports only using the kernel-based API, this diff updates > them to use new libsndio-based API: > - sysutils/tray-app > - audio/gqmpeg > > As a side effect, this fixes gqmpeg crashes, makes these programs use > the right device, and make them work on many uaudio(4) and envy(4) > devices with no hardware master level knob. Also when one program > changes the volume, slider of other programs are properly updated. > I forgot x11/i3status; below is a diff to make it use sndio. Index: Makefile === RCS file: /cvs/ports/x11/i3status/Makefile,v retrieving revision 1.58 diff -u -p -r1.58 Makefile --- Makefile27 Sep 2019 20:33:22 - 1.58 +++ Makefile12 Feb 2020 13:55:43 - @@ -5,7 +5,7 @@ ONLY_FOR_ARCHS= ${APM_ARCHS} COMMENT= generate a statusbar for use with i3/xmobar/dzen2 DISTNAME= i3status-2.13 -REVISION= 1 +REVISION= 2 CATEGORIES=x11 sysutils HOMEPAGE= https://i3wm.org/i3status/ @@ -28,7 +28,9 @@ BUILD_DEPENDS=textproc/asciidoc>=8.6.8 LIB_DEPENDS= devel/libconfuse \ devel/libyajl -CONFIGURE_STYLE = gnu +AUTOCONF_VERSION = 2.69 +AUTOMAKE_VERSION = 1.16 +CONFIGURE_STYLE = autoreconf SEPARATE_BUILD = Yes FAKE_FLAGS += sysconfdir=${PREFIX}/share/examples/i3status/ Index: patches/patch-Makefile_am === RCS file: patches/patch-Makefile_am diff -N patches/patch-Makefile_am --- /dev/null 1 Jan 1970 00:00:00 - +++ patches/patch-Makefile_am 12 Feb 2020 13:55:43 - @@ -0,0 +1,37 @@ +$OpenBSD$ + +Index: Makefile.am +--- Makefile.am.orig Makefile.am +@@ -1,5 +1,3 @@ +-@CODE_COVERAGE_RULES@ +- + echo-version: + @echo "@I3STATUS_VERSION@" + +@@ -30,6 +28,7 @@ i3status_CFLAGS = \ + $(PULSE_CFLAGS) \ + $(NLGENL_CFLAGS) \ + $(ALSA_CFLAGS) \ ++ $(SNDIO_CFLAGS) \ + $(PTHREAD_CFLAGS) + + i3status_CPPFLAGS = \ +@@ -42,6 +41,7 @@ i3status_LDADD = \ + $(PULSE_LIBS) \ + $(NLGENL_LIBS) \ + $(ALSA_LIBS) \ ++ $(SNDIO_LIBS) \ + $(PTHREAD_LIBS) + + i3status_SOURCES = \ +@@ -69,7 +69,8 @@ i3status_SOURCES = \ + src/print_wireless_info.c \ + src/print_file_contents.c \ + src/process_runs.c \ +- src/pulse.c ++ src/pulse.c \ ++ src/sndio.c + + dist_sysconf_DATA = \ + i3status.conf Index: patches/patch-Makefile_in === RCS file: patches/patch-Makefile_in diff -N patches/patch-Makefile_in --- patches/patch-Makefile_in 6 Jul 2019 20:20:27 - 1.1 +++ /dev/null 1 Jan 1970 00:00:00 - @@ -1,15 +0,0 @@ -$OpenBSD: patch-Makefile_in,v 1.1 2019/07/06 20:20:27 jasper Exp $ - -The CODE_COVERAGE_RULES fragment contains an unmatched "if" clause. - -Index: Makefile.in Makefile.in.orig -+++ Makefile.in -@@ -1851,7 +1851,6 @@ uninstall-man: uninstall-man1 - - .PRECIOUS: Makefile - --@CODE_COVERAGE_RULES@ - - echo-version: - @echo "@I3STATUS_VERSION@" Index: patches/patch-configure_ac === RCS file: patches/patch-configure_ac diff -N patches/patch-configure_ac --- /dev/null 1 Jan 1970 00:00:00 - +++ patches/patch-configure_ac 12 Feb 2020 13:55:43 - @@ -0,0 +1,19 @@ +$OpenBSD$ + +Index: configure.ac +--- configure.ac.orig configure.ac +@@ -91,6 +91,13 @@ case $host_os in + ;; + esac + ++# if sndio is available, define USE_SNDIO ++AC_CHECK_HEADER(sndio.h, ++ [AC_CHECK_LIB([sndio], [sio_open], [ ++ AC_SUBST(SNDIO_LIBS, "-lsndio") ++ AC_DEFINE([USE_SNDIO], [], [Use sndio]) ++ ], [])], []) ++ + dnl TODO: check for libbsd for GNU/kFreeBSD + + # Checks for programs. Index: patches/patch-include_i3status_h === RCS file: patches/patch-include_i3status_h diff -N patches/patch-include_i3status_h --- /dev/null 1 Jan 1970 00:00:00 - +++ patches/patch-include_i3status_h12 Feb 2020 13:55:43 - @@ -0,0 +1,13 @@ +$OpenBSD$ + +Index: include/i3status.h +--- include/i3status.h.orig include/i3status.h +@@ -232,6 +232,7 @@ int volume_pulseaudio(uint32_t sink_idx, const char *s + bool description_pulseaudio(uint32_t sink_idx, const char *sink_name, char buffer[MAX_SINK_DESCRIPTION_LEN]); + bool pulse_initialize(void); + void print_file_contents(yajl_gen json_gen, char *buffer, const char *title, const char *path, const char *format, const char *format_bad, const int max_chars); ++int volume_sndio(void); + + /* socket file descriptor for general purpose
Re: Audio control API, part 1: libsndio, sndiod bits
On Tue, Feb 11, 2020 at 07:01:28PM +0100, Florian Obser wrote: > I've been running the base diffs since you posted them. Firefox, > chrome and mpv still make noise :) > > I'm puzzled by this: > > $ cat /etc/mixerctl.conf > outputs.master=255,255 > record.enable=off > > $ mixerctl outputs.master > outputs.master=255,255 > > $ sndioctl > output.level=127 > > I don't understand how they relate and why one goes to 255 and the > other to 127. > The error reporting is confusing, too: > > $ sndioctl output.level=128 > integer overflow > > But no regressions to report :) > Thanks, the code is base on MIDI bits, which uses the 0..127 range; sndiod, aucat and many codecs also use the 0..127 range. Anyway, replaced the error message by: $ sndioctl output.level=128 128: expected integer in the 0..127 range [...] I'm wondering if persents or floating points in the [0:1] range would be less confusing and solve most "units" problems.
Re: Audio control API, part 2: add new sndioctl(1) utility
On Mon, Feb 10, 2020 at 09:59:09AM +, Raf Czlonka wrote: > On Sun, Feb 09, 2020 at 12:14:47PM GMT, Alexandre Ratchov wrote: > > Here's a new sndioctl utility similar to mixerctl(1) but using the new > > sndio API. Example: > > > > $ sndioctl > > output.level=127 > > app/aucat0.level=127 > > app/firefox0.level=127 > > app/firefox1.level=12 > > app/midisyn0.level=127 > > app/mpv0.level=127 > > app/prog5.level=127 > > app/prog6.level=127 > > app/prog7.level=127 > > hw/input.level=62 > > hw/input.mute=0 > > hw/output.level=63 > > hw/output.mute=0 > > > > Hi Alexandre, > > Just a quick question. > > Is there a good reason to have the above using "slash" ('/') as the > first separator instead of the, more familiar, "dot" ('.') known > from sysctl(8)'s MIB (Management Information Base) style names or > even the "pseudo" MIB know from mixerctl(1)? Hi, I don't know if the following qualifies as a "good reason". The first part (the group) is a prefix of the control identifier. The identifier itself has a strict "[channel]." format. The prefix is not always present, examples: output.level<- sndiod volume knob hw/output.level <- underlying hardware volume knob I tried to avoid the group part, but as mixers may be nested it seems necessary to avoid name clashes. In the sndioctl syntax, we could replace '/' by '.' but this looks confusing as the syntax doesn't map directly to the underlying model. But maybe we should hide such developer-centric details and just use only dots to make this look as a MIB. Another option I've considered is to drop the group concept in the API and simply prefix the stream name to make it unique; in turn we obtain a flat control list. It's uglier and seems to complicate GUIs task. For instance the group part could be used to represent controls of different groups in different sections or to filter-out certain groups).
Re: Audio control API, part 2: add new sndioctl(1) utility
On Sun, Feb 09, 2020 at 07:20:15PM +0100, Landry Breuil wrote: > On Sun, Feb 09, 2020 at 01:14:47PM +0100, Alexandre Ratchov wrote: > > Here's a new sndioctl utility similar to mixerctl(1) but using the new > > sndio API. Example: > > > > $ sndioctl > > output.level=127 > > app/aucat0.level=127 > > app/firefox0.level=127 > > app/firefox1.level=12 > > app/midisyn0.level=127 > > app/mpv0.level=127 > > app/prog5.level=127 > > app/prog6.level=127 > > app/prog7.level=127 > > hw/input.level=62 > > hw/input.mute=0 > > hw/output.level=63 > > hw/output.mute=0 > > i suppose that replaces audio/aucatctl port ? Yes. But aucatctl will continue to work, it's based on the MIDI interface that will stay for now. The advantage of sndioctl is that it exposes selected hardware controls, while aucatctl exposes sndiod internal knobs only. > audio/cmixer relies on the > latter but i'll have no issue migrating to it if sndioctl gets added. > The -m option might be useful to cmixer to get updates of the knobs state without the need to restart sndioctl periodically.
Audio control API, part 4: switch ports to sndio API
There are two ports only using the kernel-based API, this diff updates them to use new libsndio-based API: - sysutils/tray-app - audio/gqmpeg As a side effect, this fixes gqmpeg crashes, makes these programs use the right device, and make them work on many uaudio(4) and envy(4) devices with no hardware master level knob. Also when one program changes the volume, slider of other programs are properly updated. enjoy, Index: audio/gqmpeg/Makefile === RCS file: /cvs/ports/audio/gqmpeg/Makefile,v retrieving revision 1.64 diff -u -p -u -p -r1.64 Makefile --- audio/gqmpeg/Makefile 12 Jul 2019 20:43:33 - 1.64 +++ audio/gqmpeg/Makefile 4 Feb 2020 22:28:34 - @@ -3,7 +3,7 @@ COMMENT= front-end to various audio players DISTNAME= gqmpeg-0.91.1 -REVISION= 14 +REVISION= 15 CATEGORIES=audio HOMEPAGE= http://gqmpeg.sourceforge.net/ Index: audio/gqmpeg/patches/patch-src_Makefile_in === RCS file: audio/gqmpeg/patches/patch-src_Makefile_in diff -N audio/gqmpeg/patches/patch-src_Makefile_in --- /dev/null 1 Jan 1970 00:00:00 - +++ audio/gqmpeg/patches/patch-src_Makefile_in 4 Feb 2020 22:28:34 - @@ -0,0 +1,14 @@ +$OpenBSD$ + +Index: src/Makefile.in +--- src/Makefile.in.orig src/Makefile.in +@@ -342,7 +342,7 @@ gqmpeg_SOURCES = \ + $(module_mpg123) $(module_xmp) $(module_ogg123) $(module_radio) + + +-gqmpeg_LDADD = $(GTK_LIBS) $(LIBPNG) ++gqmpeg_LDADD = $(GTK_LIBS) $(LIBPNG) -lsndio + + EXTRA_DIST = \ + $(extra_SLIK) \ Index: audio/gqmpeg/patches/patch-src_mixer_c === RCS file: /cvs/ports/audio/gqmpeg/patches/patch-src_mixer_c,v retrieving revision 1.2 diff -u -p -u -p -r1.2 patch-src_mixer_c --- audio/gqmpeg/patches/patch-src_mixer_c 14 Oct 2007 14:12:42 - 1.2 +++ audio/gqmpeg/patches/patch-src_mixer_c 4 Feb 2020 22:28:34 - @@ -1,39 +1,287 @@ $OpenBSD: patch-src_mixer_c,v 1.2 2007/10/14 14:12:42 jasper Exp $ src/mixer.c.orig Tue Sep 10 16:16:26 2002 -+++ src/mixer.cSun Oct 14 15:47:27 2007 -@@ -285,7 +285,11 @@ void mixer_init(gint init_device_id) - - mixer_device = getenv("MIXERDEVICE"); - if (mixer_device == NULL) -+#ifdef __OpenBSD__ -+mixer_device = "/dev/mixer"; -+#else - mixer_device = "/dev/mixer0"; -+#endif +Index: src/mixer.c +--- src/mixer.c.orig src/mixer.c +@@ -39,10 +39,16 @@ + #include + #endif - if ((fd = open(mixer_device, O_RDWR)) == -1) { - perror(mixer_device); -@@ -362,7 +366,11 @@ static void mixer_set_vol(DeviceData *device, gint vol - - mixer_device = getenv("MIXERDEVICE"); - if (mixer_device == NULL) -+#ifdef __OpenBSD__ -+mixer_device = "/dev/mixer"; -+#else - mixer_device = "/dev/mixer0"; -+#endif +-#if defined(__NetBSD__) || defined(__OpenBSD__) ++#if defined(__NetBSD__) + #include + #endif - if ((fd = open(mixer_device, O_RDWR)) == -1) { - perror(mixer_device); -@@ -406,7 +414,11 @@ static gint mixer_get_vol(DeviceData *device) - - mixer_device = getenv("MIXERDEVICE"); - if (mixer_device == NULL) -+#ifdef __OpenBSD__ -+mixer_device = "/dev/mixer"; -+#else - mixer_device = "/dev/mixer0"; ++#if defined(__OpenBSD__) ++#include ++#include ++#include "display.h" +#endif ++ + #if defined(sun) && defined(__svr4__) + #include + #endif +@@ -267,11 +273,11 @@ static gint mixer_get_vol(DeviceData *device) + + /* + * +- * NetBSD and OpenBSD ++ * NetBSD + * + */ + +-#elif defined(__NetBSD__) || defined(__OpenBSD__) ++#elif defined(__NetBSD__) + + mixer_devinfo_t *infos; + mixer_ctrl_t *values; +@@ -442,6 +448,241 @@ static gint mixer_get_vol(DeviceData *device) + + /* + * ++ * OpenBSD ++ * ++ */ ++ ++#elif defined(__OpenBSD__) ++ ++struct control { ++ struct control *next; ++ unsigned int addr; ++ unsigned int value; ++}; ++ ++static struct control *controls; ++static struct sioctl_hdl *hdl; ++static struct pollfd *pfds; ++static int initialized; ++ ++/* ++ * new control registered ++ */ ++static void ondesc(void *unused, struct sioctl_desc *d, int val) ++{ ++ struct control *i, **pi; ++ ++ if (d == NULL) ++ return; ++ ++ /* ++ * delete existing control with the same address ++ */ ++ for (pi = (i = *pi) != NULL; pi = >next) { ++ if (d->addr == i->addr) { ++ *pi = i->next; ++ free(i); ++ break; ++ } ++ } ++ ++ /* ++ * SIOCTL_NONE
Audio control API, part 3: switch -lossaudio to sndio
Certain ports use -lossaudio to get access to the volume knobs. This diff updates -lossaudio to use the new sndio API. By default, it exposes both sndiod output level and hardware master input/output knobs (if present). The source selectors are not exposed any longer as sndiod doesn't expose them. As a side effect, this solves two problems: - makes mixer programs (like the xfce4-mixer port) use the right device. Example if you use a usb head-set, the program will display knobs of the head-set instead of knobs of the build-in device. - makes mixer programs work on systems that have no master level knobs, like many uaudio(4) or envy(4) devices The library version stays the same as thigs works exactly as before, so no need to rebuild ports relying no this hack^Wlibrary. Index: lib/libossaudio/Makefile === RCS file: /cvs/src/lib/libossaudio/Makefile,v retrieving revision 1.5 diff -u -p -u -p -r1.5 Makefile --- lib/libossaudio/Makefile16 Jul 2014 20:02:17 - 1.5 +++ lib/libossaudio/Makefile8 Feb 2020 14:49:55 - @@ -4,9 +4,11 @@ LIB= ossaudio MAN= ossaudio.3 -SRCS= ossaudio.c +SRCS= ossaudio.c aucat.c debug.c sioctl.c sioctl_aucat.c sioctl_sun.c CPPFLAGS+= -I${.CURDIR} + +.PATH: ${.CURDIR}/../libsndio includes: @cd ${.CURDIR}; cmp -s soundcard.h ${DESTDIR}/usr/include/soundcard.h || \ Index: lib/libossaudio/ossaudio.c === RCS file: /cvs/src/lib/libossaudio/ossaudio.c,v retrieving revision 1.20 diff -u -p -u -p -r1.20 ossaudio.c --- lib/libossaudio/ossaudio.c 28 Jun 2019 13:32:42 - 1.20 +++ lib/libossaudio/ossaudio.c 8 Feb 2020 14:49:55 - @@ -36,26 +36,41 @@ #include #include #include -#include -#include #include - +#include +#include +#include +#include #include "soundcard.h" -#undef ioctl + +#ifdef DEBUG +#define DPRINTF(...) do { fprintf(stderr, __VA_ARGS__); } while (0) +#else +#define DPRINTF(...) do {} while (0) +#endif #define GET_DEV(com) ((com) & 0xff) -#define TO_OSSVOL(x) (((x) * 100 + 127) / 255) -#define FROM_OSSVOL(x) x) > 100 ? 100 : (x)) * 255 + 50) / 100) +#define TO_OSSVOL(x) (((x) * 100 + SIOCTL_VALMAX / 2) / SIOCTL_VALMAX) +#define FROM_OSSVOL(x) x) > 100 ? 100 : (x)) * SIOCTL_VALMAX + 50) / 100) -static struct audiodevinfo *getdevinfo(int); +#define INTARG (*(int*)argp) + +struct control { + struct control *next; + int type; /* one of SOUND_MIXER_xxx */ + int chan; /* 0 -> left, 1 -> right, -1 -> mono */ + int addr; /* sioctl control id */ + int value; /* current value */ +}; static int mixer_ioctl(int, unsigned long, void *); -static int opaque_to_enum(struct audiodevinfo *di, audio_mixer_name_t *label, int opq); -static int enum_to_ord(struct audiodevinfo *di, int enm); -static int enum_to_mask(struct audiodevinfo *di, int enm); -#define INTARG (*(int*)argp) +static int initialized; +static struct control *controls; +static struct sioctl_hdl *hdl; +static char *dev_name = SIOCTL_DEVANY; +static struct pollfd *pfds; int _oss_ioctl(int fd, unsigned long com, ...) @@ -71,201 +86,162 @@ _oss_ioctl(int fd, unsigned long com, .. else if (IOCGROUP(com) == 'M') return mixer_ioctl(fd, com, argp); else - return ioctl(fd, com, argp); + return (ioctl)(fd, com, argp); } -/* If the mixer device should have more than MAX_MIXER_DEVS devices - * some will not be available to Linux */ -#define MAX_MIXER_DEVS 64 -struct audiodevinfo { - int done; - dev_t dev; - ino_t ino; - int16_t devmap[SOUND_MIXER_NRDEVICES], - rdevmap[MAX_MIXER_DEVS]; - char names[MAX_MIXER_DEVS][MAX_AUDIO_DEV_LEN]; - int enum2opaque[MAX_MIXER_DEVS]; -u_long devmask, recmask, stereomask; - u_long caps, recsource; -}; - -static int -opaque_to_enum(struct audiodevinfo *di, audio_mixer_name_t *label, int opq) +/* + * new control + */ +static void +mixer_ondesc(void *unused, struct sioctl_desc *d, int val) { - int i, o; + struct control *i, **pi; + int type; - for (i = 0; i < MAX_MIXER_DEVS; i++) { - o = di->enum2opaque[i]; - if (o == opq) - break; - if (o == -1 && label != NULL && - !strncmp(di->names[i], label->name, sizeof di->names[i])) { - di->enum2opaque[i] = opq; + if (d == NULL) + return; + + /* +* delete existing control with the same address +*/ + for (pi = (i = *pi) != NULL; pi = >next) { + if (d->addr == i->addr) { + *pi = i->next; + free(i); break; } } - if (i >= MAX_MIXER_DEVS) - i
Audio control API, part 2: add new sndioctl(1) utility
Here's a new sndioctl utility similar to mixerctl(1) but using the new sndio API. Example: $ sndioctl output.level=127 app/aucat0.level=127 app/firefox0.level=127 app/firefox1.level=12 app/midisyn0.level=127 app/mpv0.level=127 app/prog5.level=127 app/prog6.level=127 app/prog7.level=127 hw/input.level=62 hw/input.mute=0 hw/output.level=63 hw/output.mute=0 Configuration parameters that are not exposed by sndiod will be handled by audioctl(1), including the /etc/mixerctl.conf file at system startup. Originally the program was designed to handle modern many-channel devices by presenting many-channel knobs on a single line; this feature isn't used yet as the corresponding kernel bits are missing. Index: usr.bin/Makefile === RCS file: /cvs/src/usr.bin/Makefile,v retrieving revision 1.161 diff -u -p -u -p -r1.161 Makefile --- usr.bin/Makefile9 Aug 2019 06:18:25 - 1.161 +++ usr.bin/Makefile9 Feb 2020 11:05:02 - @@ -22,7 +22,7 @@ SUBDIR= apply arch at aucat audioctl awk pr printenv printf quota radioctl rcs rdist rdistd \ readlink renice rev rpcgen rpcinfo rs rsync rup rusers rwall \ sdiff script sed sendbug shar showmount signify skey \ - skeyaudit skeyinfo skeyinit sndiod snmp \ + skeyaudit skeyinfo skeyinit sndioctl sndiod snmp \ sort spell split ssh stat su systat \ tail talk tcpbench tee telnet tftp tic time \ tmux top touch tput tr true tset tsort tty usbhidaction usbhidctl \ Index: usr.bin/sndioctl/Makefile === RCS file: usr.bin/sndioctl/Makefile diff -N usr.bin/sndioctl/Makefile --- /dev/null 1 Jan 1970 00:00:00 - +++ usr.bin/sndioctl/Makefile 9 Feb 2020 11:05:02 - @@ -0,0 +1,5 @@ +# $OpenBSD$ + +PROG= sndioctl +LDADD+=-lsndio +.include Index: usr.bin/sndioctl/sndioctl.1 === RCS file: usr.bin/sndioctl/sndioctl.1 diff -N usr.bin/sndioctl/sndioctl.1 --- /dev/null 1 Jan 1970 00:00:00 - +++ usr.bin/sndioctl/sndioctl.1 9 Feb 2020 11:05:02 - @@ -0,0 +1,148 @@ +.\" $OpenBSD$ +.\" +.\" Copyright (c) 2007 Alexandre Ratchov +.\" +.\" Permission to use, copy, modify, and distribute this software for any +.\" purpose with or without fee is hereby granted, provided that the above +.\" copyright notice and this permission notice appear in all copies. +.\" +.\" THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES +.\" WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF +.\" MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR +.\" ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES +.\" WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN +.\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF +.\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +.\" +.Dd $Mdocdate: April 8 2011 $ +.Dt SNDIOCTL 1 +.Os +.Sh NAME +.Nm sndioctl +.Nd control audio parameters +.Sh SYNOPSIS +.Nm +.Bk -words +.Op Fl iv +.Op Fl f Ar device +.Op Ar command ... +.Ek +.Nm +.Bk -words +.Fl d +.Ek +.Sh DESCRIPTION +The +.Nm +utility can display or change parameters of +.Xr sndio 7 +audio devices. +The options are as follows: +.Bl -tag -width Ds +.It Fl d +Dump the raw list of available parameters and exit. +Useful as a debug tool. +.It Fl f Ar device +Use this +.Xr sndio 7 +audio device. +.It Fl m +Monitor and display audio parameters changes. +.It Fl i +Display characteristics of requested parameters +instead of their values. +.It Fl v +Enable verbose mode, a.k.a. multi-channel mode. +By default parameters affecting different channels +of the same stream are disguised as a single mono +parameter to hide details that are not essential. +.El +.Pp +If no commands are specified all valid parameters are displayed on +.Em stdout . +Unless +.Fl d , +.Fl m , +or +.Fl i +are used, displayed parameters are valid commands. +The set of available controls depends on the control device. +.Pp +Commands use the following two formats to display and set +parameters respectively: +.Pp +.Dl group/stream[channel].function +.Dl group/stream[channel].function=value +.Pp +On the left-hand side are specified the optional parameter group, +the affected stream name, and the optional channel number. +Examples of left-hand side terms: +.Pp +.Dl output.level +.Dl hw/spkr[6].mute +.Pp +There are 4 parameter types: switches, numbers, selectors, and vectors. +.Pp +Numbers are specified in decimal and follow the same semantics +as MIDI controllers. +Values are in the 0..127 range and 64 is the neutral state (if applicable). +Two-state controls (switches) take either 0 or 1 as value, +typically corresponding to
Audio control API, part 1: libsndio, sndiod bits
_arg, desc, val); +} + +void +_sioctl_onval_cb(struct sioctl_hdl *hdl, unsigned int addr, unsigned int val) +{ + DPRINTF("_sioctl_onval_cb: %u -> %u\n", addr, val); + if (hdl->ctl_cb) + hdl->ctl_cb(hdl->ctl_arg, addr, val); +} + +int +sioctl_setval(struct sioctl_hdl *hdl, unsigned int addr, unsigned int val) +{ + if (!(hdl->mode & SIOCTL_WRITE)) + return 0; + return hdl->ops->setctl(hdl, addr, val); +} Index: lib/libsndio/sioctl_aucat.c === RCS file: lib/libsndio/sioctl_aucat.c diff -N lib/libsndio/sioctl_aucat.c --- /dev/null 1 Jan 1970 00:00:00 - +++ lib/libsndio/sioctl_aucat.c 8 Feb 2020 14:49:37 - @@ -0,0 +1,272 @@ +/* + * Copyright (c) 2010-2011 Alexandre Ratchov + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include "debug.h" +#include "aucat.h" +#include "sioctl_priv.h" + +struct sioctl_aucat_hdl { + struct sioctl_hdl sioctl; + struct aucat aucat; + struct sioctl_desc desc; + struct amsg_ctl_desc buf[16]; + size_t buf_wpos; + int dump_wait; +}; + +static void sioctl_aucat_close(struct sioctl_hdl *); +static int sioctl_aucat_nfds(struct sioctl_hdl *); +static int sioctl_aucat_pollfd(struct sioctl_hdl *, struct pollfd *, int); +static int sioctl_aucat_revents(struct sioctl_hdl *, struct pollfd *); +static int sioctl_aucat_setctl(struct sioctl_hdl *, unsigned int, unsigned int); +static int sioctl_aucat_onval(struct sioctl_hdl *); +static int sioctl_aucat_ondesc(struct sioctl_hdl *); + +/* + * operations every device should support + */ +struct sioctl_ops sioctl_aucat_ops = { + sioctl_aucat_close, + sioctl_aucat_nfds, + sioctl_aucat_pollfd, + sioctl_aucat_revents, + sioctl_aucat_setctl, + sioctl_aucat_onval, + sioctl_aucat_ondesc +}; + +static int +sioctl_aucat_rdata(struct sioctl_aucat_hdl *hdl) +{ + struct sioctl_desc desc; + struct amsg_ctl_desc *c; + size_t rpos; + int n; + + while (hdl->aucat.rstate == RSTATE_DATA) { + + /* read entries */ + while (hdl->buf_wpos < sizeof(hdl->buf) && + hdl->aucat.rstate == RSTATE_DATA) { + n = _aucat_rdata(>aucat, + (unsigned char *)hdl->buf + hdl->buf_wpos, + sizeof(hdl->buf) - hdl->buf_wpos, + >sioctl.eof); + if (n == 0 || hdl->sioctl.eof) + return 0; + hdl->buf_wpos += n; + } + + /* parse entries */ + c = hdl->buf; + rpos = 0; + while (rpos < hdl->buf_wpos) { + strlcpy(desc.group, c->group, SIOCTL_NAMEMAX); + strlcpy(desc.node0.name, c->node0.name, SIOCTL_NAMEMAX); + desc.node0.unit = (int16_t)ntohs(c->node0.unit); + strlcpy(desc.node1.name, c->node1.name, SIOCTL_NAMEMAX); + desc.node1.unit = (int16_t)ntohs(c->node1.unit); + strlcpy(desc.func, c->func, SIOCTL_NAMEMAX); + desc.type = c->type; + desc.addr = ntohs(c->addr); + _sioctl_ondesc_cb(>sioctl, + , ntohs(c->curval)); + rpos += sizeof(struct amsg_ctl_desc); + c++; + } + hdl->buf_wpos = 0; + } + return 1; +} + +/* + * execute the next message, return 0 if blocked + */ +static int +sioctl_aucat_runmsg(struct sioctl_aucat_hdl *hdl) +{ + if (!_aucat_rmsg(>aucat, >sioctl.eof)) + return 0; + switch (ntohl(hdl->aucat.rmsg.cmd)) { + case AMSG_DATA: + hdl->buf_wpos = 0; + if (!sioctl_aucat_rdata(hdl)) + return 0; + break; + case AMSG_CTLSET: + DPRINTF("sioctl_aucat_runmsg:
Re: get rid of almost empty /etc/examples/mixerctl.conf
On Sat, Feb 08, 2020 at 11:04:10PM +0100, Ingo Schwarze wrote: > Hi Theo, > > you have a point, that was a lot of cheap talk and no patch. > > I don't aim at changing yacc(1) grammars. I think most parts of > OpenBSD configuration systems already have sane defaults and most > configuration syntaxes are already good with respect to simplicity > and usability. At least "most", maybe even all. > > > The example files were moved from /etc, so that /etc contained fewer > > files for unconfigured software. > > That was a very good step, and it was also good to not encumber it > by trying to do more at the same time. > > > Little to no effort was put into curating them. That's the gap that > > occured. > > So, here i'm putting a few pennies where my mouth is, starting from the > shortest file in /etc/examples/. The example is so short that cut and > paste from the manual page will certainly cause no trouble, and the > file also isn't a special case with respect to permissions, > it is -rw-r--r-- root:wheel. > > OK? > Ingo > > > P.S. > When preparing this, i noticed i already had the change to > mixerctl.conf.5 in my tree. It looks like i started it > in the past, then forgot about it. > > P.P.S. > Note that i will not propose to get rid of *all* of /etc/examples/, > but just of trivial examples that provide no benefit by being outside > the manual page. There are certainly some that make sense as they > are. I agree that the mixerctl.conf is pointless. Furthermore not all devices have an outputs.master knob (ex. envy(4) and uaudio(4) don't), so the example is wrong in certain cases. thanks for looking at this ok ratchov
Include AUDIO_MIXER_xxx ioctls in the "audio" pledge() promise
The plan is to make sndiod need to control volume knobs. This diff adds the AUDIO_MIXER_xxx ioctls to the "audio" pledge. Another option would be to introduce a new "audioctl" promise, but IMHO we don't seed such fine-grained promises. OK? Index: kern_pledge.c === RCS file: /cvs/src/sys/kern/kern_pledge.c,v retrieving revision 1.256 diff -u -p -u -p -r1.256 kern_pledge.c --- kern_pledge.c 8 Dec 2019 23:08:59 - 1.256 +++ kern_pledge.c 2 Feb 2020 09:02:27 - @@ -1123,6 +1123,9 @@ pledge_ioctl(struct proc *p, long com, s case AUDIO_SETPAR: case AUDIO_START: case AUDIO_STOP: + case AUDIO_MIXER_DEVINFO: + case AUDIO_MIXER_READ: + case AUDIO_MIXER_WRITE: if (fp->f_type == DTYPE_VNODE && vp->v_type == VCHR && cdevsw[major(vp->v_rdev)].d_open == audioopen)
Add support for notifications about audio(4) "mixer" controls changes
The diff bellow allows programs to get notifications about changes of audio controls. Programs open the /dev/audioctlN device node and use the read(4) syscall to get the index of each changed control. There may be only one reader at a time. Supporting multiple readers would be much more complicated and the purpose of this diff is to allow sndiod(8) to expose the controls to programs. So, there will be one reader only. To test the diff, run: hexdump -ve '1/4 "%d\n"' 0 struct wskbd_vol spkr, mic; struct task wskbd_task; @@ -1192,6 +1205,8 @@ audio_attach(struct device *parent, stru sc->mix_nent = mi->index; sc->mix_ents = mallocarray(sc->mix_nent, sizeof(struct mixer_ctrl), M_DEVBUF, M_WAITOK); + sc->mix_evbuf = mallocarray(sc->mix_nent, + sizeof(struct mixer_ev), M_DEVBUF, M_WAITOK | M_ZERO); ent = sc->mix_ents; mi->index = 0; @@ -1329,6 +1344,7 @@ audio_detach(struct device *self, int fl } /* free resources */ + free(sc->mix_evbuf, M_DEVBUF, sc->mix_nent * sizeof(struct mixer_ev)); free(sc->mix_ents, M_DEVBUF, sc->mix_nent * sizeof(struct mixer_ctrl)); audio_buf_done(sc, >play); audio_buf_done(sc, >rec); @@ -1718,6 +1734,28 @@ audio_ioctl(struct audio_softc *sc, unsi return error; } +void +audio_event(struct audio_softc *sc, int addr) +{ + struct mixer_ev *e; + + mtx_enter(_lock); + if (sc->mix_isopen) { + e = sc->mix_evbuf + addr; + if (!e->pending) { + e->pending = 1; + e->next = sc->mix_pending; + sc->mix_pending = e; + } + if (sc->mix_blocking) { + wakeup(>mix_blocking); + sc->mix_blocking = 0; + } + selwakeup(>mix_sel); + } + mtx_leave(_lock); +} + int audio_mixer_devinfo(struct audio_softc *sc, struct mixer_devinfo *devinfo) { @@ -1784,6 +1822,7 @@ audio_mixer_set(struct audio_softc *sc, return error; if (sc->ops->commit_settings) return sc->ops->commit_settings(sc->arg); + audio_event(sc, c->dev); return 0; } @@ -1834,6 +1873,107 @@ audio_ioctl_mixer(struct audio_softc *sc } int +audio_mixer_read(struct audio_softc *sc, struct uio *uio, int ioflag) +{ + struct mixer_ev *e; + int data; + int error; + + DPRINTF("%s: mixer read: resid = %zd\n", DEVNAME(sc), uio->uio_resid); + + /* block if quiesced */ + while (sc->quiesce) + tsleep_nsec(>quiesce, 0, "mix_qrd", INFSLP); + + mtx_enter(_lock); + + /* if there are no events then sleep */ + while (!sc->mix_pending) { + if (ioflag & IO_NDELAY) { + mtx_leave(_lock); + return EWOULDBLOCK; + } + DPRINTF("%s: mixer read sleep\n", DEVNAME(sc)); + sc->mix_blocking = 1; + error = msleep_nsec(>mix_blocking, + _lock, PWAIT | PCATCH, "mix_rd", INFSLP); + if (!(sc->dev.dv_flags & DVF_ACTIVE)) + error = EIO; + if (error) { + DPRINTF("%s: mixer read woke up error = %d\n", + DEVNAME(sc), error); + mtx_leave(_lock); + return error; + } + } + + /* at this stage, there is an event to transfer */ + while (uio->uio_resid >= sizeof(int) && sc->mix_pending) { + e = sc->mix_pending; + sc->mix_pending = e->next; + e->pending = 0; + data = e - sc->mix_evbuf; + mtx_leave(_lock); + DPRINTF("%s: mixer read: %u\n", DEVNAME(sc), data); + error = uiomove(, sizeof(int), uio); + if (error) + return error; + mtx_enter(_lock); + } + + mtx_leave(_lock); + return 0; +} + +int +audio_mixer_poll(struct audio_softc *sc, int events, struct proc *p) +{ + int revents = 0; + + mtx_enter(_lock); + if (sc->mix_isopen && sc->mix_pending) + revents |= events & (POLLIN | POLLRDNORM); + if (revents == 0) { + if (events & (POLLIN | POLLRDNORM)) + selrecord(p, >mix_sel); + } + mtx_leave(_lock); + return revents; +} + +int +audio_mixer_open(struct audio_softc *sc, int flags) +{ + DPRINTF("%s: flags = 0x%x\n", __func__, flags); + + if (flags & FREAD) { + if (sc->mix_isopen) + return EBUSY; + sc->mix_isopen = 1; + } + return 0; +} + +int +audio_mixer_close(struct audio_softc *sc, int flags) +{ + int i; + + DPRINTF("%s: flags
Re: eso(4): msleep(9) -> msleep_nsec(9)
On Thu, Jan 16, 2020 at 10:32:21PM -0600, Scott Cheloha wrote: > Ticks to milliseconds. > > Pretty sure all we need to do to convert the drain timeouts is > substitute 1000 for hz in each expression. > > While we're here, I noticed that at halt time for input/output we > msleep to allow a drain and then immediately relinquish the mutex > after coming out of the sleep. We could just opt not to reenter the > mutex with PNORELOCK, though that might make the code harder to read. > Maybe you have a preference? I don't have the hardware to test, but this looks correct. > Otherwise, ok? > ok ratchov > Index: pci/esovar.h > === > RCS file: /cvs/src/sys/dev/pci/esovar.h,v > retrieving revision 1.6 > diff -u -p -r1.6 esovar.h > --- pci/esovar.h 21 Sep 2010 20:11:44 - 1.6 > +++ pci/esovar.h 17 Jan 2020 04:29:28 - > @@ -129,7 +129,7 @@ struct eso_softc { > void(*sc_rintr)(void *); > void * sc_rarg; > > - /* Auto-initialize DMA transfer block drain timeouts, in ticks */ > + /* Auto-initialize DMA transfer block drain timeouts, in milliseconds */ > int sc_pdrain; > int sc_rdrain; > > Index: pci/eso.c > === > RCS file: /cvs/src/sys/dev/pci/eso.c,v > retrieving revision 1.45 > diff -u -p -r1.45 eso.c > --- pci/eso.c 14 Dec 2019 12:49:50 - 1.45 > +++ pci/eso.c 17 Jan 2020 04:29:28 - > @@ -760,8 +760,8 @@ eso_halt_output(void *hdl) > ESO_IO_A2DMAM_DMAENB); > > sc->sc_pintr = NULL; > - error = msleep(>sc_pintr, _lock, PWAIT, "esoho", > sc->sc_pdrain); > - mtx_leave(_lock); > + error = msleep_nsec(>sc_pintr, _lock, PWAIT | PNORELOCK, > + "esoho", MSEC_TO_NSEC(sc->sc_pdrain)); > > /* Shut down DMA completely. */ > eso_write_mixreg(sc, ESO_MIXREG_A2C1, 0); > @@ -787,8 +787,8 @@ eso_halt_input(void *hdl) > DMA37MD_WRITE | DMA37MD_DEMAND); > > sc->sc_rintr = NULL; > - error = msleep(>sc_rintr, _lock, PWAIT, "esohi", > sc->sc_rdrain); > - mtx_leave(_lock); > + error = msleep_nsec(>sc_rintr, _lock, PWAIT | PNORELOCK, > + "esohi", MSEC_TO_NSEC(sc->sc_rdrain)); > > /* Shut down DMA completely. */ > eso_write_ctlreg(sc, ESO_CTLREG_A1C2, > @@ -1605,8 +1605,8 @@ eso_trigger_output(void *hdl, void *star > sc->sc_pintr = intr; > sc->sc_parg = arg; > > - /* Compute drain timeout. */ > - sc->sc_pdrain = hz * (blksize * 3 / 2) / > + /* Compute drain timeout in milliseconds. */ > + sc->sc_pdrain = 1000 * (blksize * 3 / 2) / > (param->sample_rate * param->channels * param->bps); > > /* DMA transfer count (in `words'!) reload using 2's complement. */ > @@ -1688,8 +1688,8 @@ eso_trigger_input(void *hdl, void *start > sc->sc_rintr = intr; > sc->sc_rarg = arg; > > - /* Compute drain timeout. */ > - sc->sc_rdrain = hz * (blksize * 3 / 2) / > + /* Compute drain timeout in milliseconds. */ > + sc->sc_rdrain = 1000 * (blksize * 3 / 2) / > (param->sample_rate * param->channels * param->bps); > > /* Set up ADC DMA converter parameters. */
Re: pci(4): autri(4), eap(4): tsleep(9) -> tsleep_nsec(9)
On Sat, Jan 11, 2020 at 02:33:07AM -0600, Scott Cheloha wrote: > Both of these cards have a 100ms sleep when closed. > > ok? > sure, ok ratchov > Index: pci/eap.c > === > RCS file: /cvs/src/sys/dev/pci/eap.c,v > retrieving revision 1.57 > diff -u -p -r1.57 eap.c > --- pci/eap.c 14 Dec 2019 12:48:32 - 1.57 > +++ pci/eap.c 11 Jan 2020 08:29:27 - > @@ -1536,7 +1536,9 @@ eap_midi_close(void *addr) > { > struct eap_softc *sc = addr; > > - tsleep(sc, PWAIT, "eapclm", hz/10); /* give uart a chance to drain */ > + /* give uart a chance to drain */ > + tsleep_nsec(sc, PWAIT, "eapclm", MSEC_TO_NSEC(100)); > + > EWRITE1(sc, EAP_UART_CONTROL, 0); > EWRITE4(sc, EAP_ICSC, EREAD4(sc, EAP_ICSC) & ~EAP_UART_EN); > > Index: pci/autri.c > === > RCS file: /cvs/src/sys/dev/pci/autri.c,v > retrieving revision 1.43 > diff -u -p -r1.43 autri.c > --- pci/autri.c 4 Dec 2019 20:21:35 - 1.43 > +++ pci/autri.c 11 Jan 2020 08:29:27 - > @@ -1352,7 +1352,8 @@ autri_midi_close(void *addr) > > DPRINTF(("autri_midi_close()\n")); > > - tsleep(sc, PWAIT, "autri", hz/10); /* give uart a chance to drain */ > + /* give uart a chance to drain */ > + tsleep_nsec(sc, PWAIT, "autri", MSEC_TO_NSEC(100)); > > sc->sc_iintr = NULL; > sc->sc_ointr = NULL; >
Re: bktr(4) (again): tsleep(9) -> tsleep_nsec(9)
On Fri, Jan 10, 2020 at 08:23:14PM -0600, Scott Cheloha wrote: > Whoops, missed one last time. > > ok? ok > > Index: pci/bktr/bktr_tuner.c > === > RCS file: /cvs/src/sys/dev/pci/bktr/bktr_tuner.c,v > retrieving revision 1.8 > diff -u -p -r1.8 bktr_tuner.c > --- pci/bktr/bktr_tuner.c 14 Mar 2015 03:38:49 - 1.8 > +++ pci/bktr/bktr_tuner.c 11 Jan 2020 02:20:51 - > @@ -900,7 +900,7 @@ do_afc( bktr_ptr_t bktr, int addr, int f > origFrequency = frequency; > > /* wait for first setting to take effect */ > - tsleep( BKTR_SLEEP, PZERO, "tuning", hz/8 ); > + tsleep_nsec( BKTR_SLEEP, PZERO, "tuning", MSEC_TO_NSEC(1000 / 8) ); > > if ( (status = i2cRead( bktr, addr + 1 )) < 0 ) > return( -1 );
Re: midi(4): *sleep(9) -> *sleep_nsec(9)
On Wed, Dec 18, 2019 at 10:41:56AM -0600, Scott Cheloha wrote: > On Wed, Dec 18, 2019 at 09:54:43AM +0100, Alexandre Ratchov wrote: > > On Tue, Dec 17, 2019 at 07:09:02PM -0600, Scott Cheloha wrote: > > > The only conversion I'm having trouble with is the tsleep(). > > > The comment says "20ms", but then we use some arithmetic > > > to derive a count of ticks. > > > > > > Given > > > > > > hz * MIDI_MAXWRITE / MIDI_RATE > > > > > > You have hz ticks/second, and 32 bytes, and 3125 bytes/second, so you > > > have > > > > > > hz ticks 32 bytes 3125 bytes > > > * / -- > > > second1 second > > > > > > = 32 * hz ticks > > > - > > > 3125 > > > > > > = 1 ticks > > > > > > if hz = 100, with integer division. > > > > > > I'm not sure how to use the constants to produce a count of > > > milliseconds. Maybe I'm just having a slow day. > > > > The problem is that close() may reset the transmitter before the few > > bytes of its internal buffer is sent on the wire; there's no "wait for > > completion" feature in such simple hardware, so we just wait few > > milliseconds. > > > > The transmitter buffer size is around 16 bytes, the byte rate is 3125 > > bytes/second. So if we wait at least 16B / 3125B/s = 5.12ms, we're > > safe. Waiting 10ms-20ms is enough and is unnoticeable. > > Just to be sure we're all on the same page, the comment says 20ms is > 64 bytes' worth, and MIDI_MAXWRITE is 32 bytes, but you're saying the > buffer is around 16 bytes. Is any of this inconsistent? Yeah, the comment doesn't match the code. 16B is what "typical" UARTs buffer. MIDI_MAXWRITE and 64 are both upper bounds of it. Waiting 20ms is safer. MIDI_MAXWRITE could be removed completely once your diff is in.