Page MenuHomeFreeBSD

sound: Include more information in kevent returned from the kernel
ClosedPublic

Authored by meka_tilda.center on Sat, May 30, 5:44 PM.
Tags
None
Referenced Files
F160392856: D57362.id179811.diff
Wed, Jun 24, 1:40 AM
F160392163: D57362.id178994.diff
Wed, Jun 24, 1:33 AM
F160379647: D57362.id179776.diff
Tue, Jun 23, 10:11 PM
Unknown Object (File)
Tue, Jun 23, 1:54 AM
Unknown Object (File)
Tue, Jun 23, 1:43 AM
Unknown Object (File)
Tue, Jun 23, 12:41 AM
Unknown Object (File)
Sun, Jun 21, 5:16 PM
Unknown Object (File)
Sun, Jun 21, 5:23 AM
Subscribers

Details

Summary

Simplified, I wanted this:

if (kn->kn_filter == EVFILT_READ) {
  kn->kn_data = sndbuf_getready(ch->bufsoft);
  kn->kn_kevent.ext[0] = sndbuf_getfreeptr(ch->bufsoft);
} else {
  kn->kn_data = sndbuf_getfree(ch->bufsoft);
  kn->kn_kevent.ext[0] = sndbuf_getreadyptr(ch->bufsoft);
}
kn->kn_kevent.ext[1] = ch->xruns;

But that introduces deadlock hence the rest of the kernel side patch. The example is changed so it reflects the new change. Rationale for this is that in real-time audio applications, code will always use mmap'ed device access and do this in a loop:

  • calculate wait time (or use kqueue for waiting)
  • use ioctl for SNDCTL_DSP_GETIPTR (or GETOPTR for playback) to get the pointer into the memory-mapped buffer where new data starts
  • use ioctl for SNDCTL_DSP_GETERROR to get xruns and handle them

With this patch, all the info is returned to user space in one syscall, reducing CPU churn and processing time, which is critical for real-time applications. For non-mmaped access, SNDCTL_DSP_CURRENT_IPTR is returned instead of SNDCTL_DSP_GETIPTR in ext[0].

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I like this idea. As an initial step, could you split the example, kqueue, and dsp_oss_syncstart() patches into 3 with appropriate commit messages?

It'd also be good to document kqueue in the man page, especially the values sound(4) populated the kevent structure with, so that users don't have to dig into sound(4) code to figure these things out. This can be part of the kernel patch.

Also, does the dsp_oss_syncstart() patch fix the LOR we've been talking about for a while?

My English is terrible. Although man page edit describes what was done, the language is just bad. I even tried to ask AI to improve it and I still don't like it, so I obviously need help with wording. And yes, dsp_oss_syncstart patch fixes LOR we were seeing for so long. It's now part of D57399.

The man page needs a date bump.

In dsp_kqevent() you add some comments above ext[0] assignments, because it's not immediately obvious what these mean.

share/man/man4/pcm.4
537–538

No need to mention "sound devices", it is implied since we are in the sound man page.

546

Would be better to word this section like:

ext[0]: For devices that use mmap, the field contains the ...., whereas for devices that do not use mmap, it contains ....
ext[1]: ...

Of course, all this with the appropriate man page formatting.

585

Albeit a bit obvious, we could explain why this is beneficial, i.e., we avoid additional ioctl calls.

share/man/man4/pcm.4
537

Isn't kevent implied if we're talking about kqueue? I think we could omit that.

538

This could be reworded roughly like "The kevent structure's fields are defined as follows:"

543
559

This could be made into a .Bl

590

What do you mean by "all needed information"? In other words, why is this useful? You could mention the mmap scenario where this is useful.

LGTM, minus some details which I will take care of when I'll commit it. Thanks :)

This revision is now accepted and ready to land.Mon, Jun 15, 8:14 PM