sound: Separate implementations for SNDCTL_AUDIOINFO[_EX] and SNDCTL_ENGINEINFO

[christos]

Summary

FreeBSD's implementation of SNDCTL_AUDIOINFO[_EX] and SNDCTL_ENGINEINFO does
not exactly work as intended. The problem is essentially that both
IOCTLs return the same information, while in fact the information
returned currently by dsp_oss_audioinfo() is what _only_
SNDCTL_ENGINEINFO is meant to return.

This behavior is also noted in the OSS manual [1] (see bold paragraph in
"Audio engines and device files" section), but since e8c0d15a64fa
("sound: Get rid of snd_clone and use DEVFS_CDEVPRIV(9)") we can
actually fix this, because we now expose only a single device for each
soundcard, and create the engines (channels) internally.
SNDCTL_ENGINEINFO will now report info about all channels in a given
device, and SNDCTL_AUDIOINFO[_EX] will only report information about
/dev/dspX.

To make this work, we also have to modify the SNDCTL_SYSINFO IOCTL to
report the number of audio devices and audio engines correctly.

While here, modernize the minimum and maximum channel counting in both
SNDCTL_AUDIOINFO[_EX] and SNDCTL_ENGINEINFO. Currently these IOCTLs will
report only up to 2 channels, which is no longer the case.

[1] http://manuals.opensound.com/developer/SNDCTL_AUDIOINFO.html

PR: 246231
Sponsored by: The FreeBSD Foundation
MFC after: 1 day

Test Plan

ossinfo(1) now reports the number of audio engines and and audio devices correctly:

Number of audio devices:        1
Number of audio engines:        6
Number of mixer devices:        1

Audio devices:

root@freebsd:~ # ./a.out -a

Audio devices
Focusrite Scarlett Solo USB       /dev/dsp0  (device index 0)

Audio engines:

root@freebsd:~ # ./a.out -e

Audio engines
00: pcm0:play:dsp0.p0 (device file /dev/dsp0)
01: pcm0:virtual_play:dsp0.vp0 (device file /dev/dsp0)
02: pcm0:virtual_play:dsp0.vp1 (device file /dev/dsp0)
03: pcm0:record:dsp0.r0 (device file /dev/dsp0)
04: pcm0:virtual_record:dsp0.vr0 (device file /dev/dsp0)
05: pcm0:virtual_record:dsp0.vr1 (device file /dev/dsp0)

[christos]

sys/dev/sound/pcm/dsp.c

2088

As mentioned in D45150, I will fix this case in a separate patch.

2137

@dev_submerge.ch Do you think there is any other special handling we have to do for SNDCTL_AUDIOINFO_EX apart from this?

[dev_submerge.ch]

sys/dev/sound/pcm/dsp.c

2137

Frankly I'm not sure whether we get the physical capabilities directly from the driver here, I'll have to test. In case we do, you'll also have to skip the non-virtual channels for SNDCTL_AUDIOINFO. For particular fields see below.

2151–2163

I know you copied this code, but I think it's way stuck in the past with only stereo channel operation :-)

For virtual channels (SNDCTL_AUDIOINFO) we need the minimum and maximum channel number of the matrix mixer. They may be propagated correctly here in the format list, we have to check.

For physical devices (SNDCTL_AUDIOINFO_EX) we need the minimum and maximum channel number of the format list provided by the driver (see comment above).

Could you update the code here to extract the actual minimum and maximum number of channels from the format list? Then we can test and see whether we need to handle these cases differently.

2165–2169

We cannot aggregate formats like this. The sample formats are flags, but there's also a channel number in the format which is a base 2 integer.

[christos]

sys/dev/sound/pcm/dsp.c

2137

Seems like we go get the from the driver, or am I missing something?

struct pcmchan_caps *
chn_getcaps(struct pcm_channel *c)
{
	CHN_LOCKASSERT(c);
	return CHANNEL_GETCAPS(c->methods, c->devinfo);
}

2165–2169

Maybe we should leave this field blank then.

[christos]

sys/dev/sound/pcm/dsp.c

2151–2163

So this is how sndstat gets the min and max channel numbers to populate the nvlist:

		for (i = 0; caps->fmtlist[i]; i++) {
			encoding |= AFMT_ENCODING(caps->fmtlist[i]);
			*minchn = min(AFMT_CHANNEL(encoding), *minchn);
			*maxchn = max(AFMT_CHANNEL(encoding), *maxchn);
		}

I guess we can do the same?

[christos]

Add locking inside loop in AUDIOINFO. Haven't addressed Florian's comments yet.

[christos]

sys/dev/sound/pcm/dsp.c

2088

@dev_submerge.ch D45256

[dev_submerge.ch]

The patch is fine as is, but it does not fully address the issue described in the commit message, only D45256 does actually resolve it. You could either merge the two commits (and commit messages) when D45256 is ready, or adapt the commit messages to describe what part of the issue is addressed in each.

[dev_submerge.ch]

Sorry, this was meant for D45150, "Login to comment" took me to the wrong page.

[dev_submerge.ch]

Uhm, sorry again, how do I take back the review accepted and change requested flags?

[dev_submerge.ch]

sys/dev/sound/pcm/dsp.c

2137

I thought c->devinfo can be the parent channel, not necessarily a physical device. There's also vchan_getcaps(), which is why I'm not so sure about this coming from the physical device.

2151–2163

Does that sndstat code work correctly? It looks to me like you'd have to get AFMT_CHANNEL(caps->fmtlist[i]) directly, and not from AFMT_CHANNEL(encoding) where the channel number bits are masked out by AFMT_ENCODING.

Apart from that, this would be a solution, yes.

2165–2169

Or we could aggregate only the sample format encoding without the channel numbers, as seen in the sndstat code by using AFMT_ENCODING. I think this as also how it is meant in the OSSv4 docs, it doesn't mention channel numbers in the paragraph about iformats and oformats.

[christos]

sys/dev/sound/pcm/dsp.c

2151–2163

I do not have a card with more than 2 channels to make sure if it works correctly. Can you test this when you have time?

2165–2169

There is chn_getformats() already so we can use this I guess.

[dev_submerge.ch]

sys/dev/sound/pcm/dsp.c

2151–2163

That's called by SNDSTIOC_GET_DEVS ioctl, right? I don't find any applications using that, do you have a test / example program?

Also, from reading the sndstat code I'd expect minchn and maxchn to get a value of 0 when vchans are disabled, means you should be able to test that ioctl with a 2 channel card.

2165–2169

chn_getformats() looks about right.

[dev_submerge.ch]

All in all the output shows up correctly now in ossinfo, except for the issues already mentioned here.

sys/dev/sound/pcm/dsp.c

2137

From testing I would say chn_getcaps() gets us the physical format in case of the non-virtual channels, and the current vchan settings (see sysctl dev.pcm) for virtual channels. Means SNDCTL_AUDIOINFO_EX is ok, but SNDCTL_AUDIOINFO adds the physical formats to it which is undesirable.

You can add a similar check here to skip non-virtual channels for SNDCTL_AUDIOINFO.

2151–2163

For a test I replaced the loop here with the snippet from sndstat you proposed. As expected the format aggregation is fine, but minchn and maxchn are always 0. Please use the correction I outlined above and also fix the code in sndstat when you get to it.

2165–2169

Beware that chn_getformats() also adds all vchan conversion formats, which we do not want here. At least as I understand OSSv4 docs, iformats and oformats are used to prevent unnecessary conversion, not to present all formats that can be accepted through conversion. But the format aggregation in the sndstat snippet above should work fine, we can use that.

[christos]

Address all Florian's comments. Apply the following changes:

diff --git a/sys/dev/sound/pcm/dsp.c b/sys/dev/sound/pcm/dsp.c
index 0acb4c8c3a29..3ccea58d0564 100644
--- a/sys/dev/sound/pcm/dsp.c
+++ b/sys/dev/sound/pcm/dsp.c
@@ -2137,9 +2137,11 @@ dsp_oss_audioinfo(struct cdev *i_dev, oss_audioinfo *ai, bool ex)
 		CHN_LOCK(ch);
 
 		/*
-		 * Skip VCHANs if we are servicing SNDCTL_AUDIOINFO_EX.
+		 * Skip physical channels if we are servicing SNDCTL_AUDIOINFO,
+		 * or VCHANs if we are servicing SNDCTL_AUDIOINFO_EX.
 		 */
-		if (ex && (ch->flags & CHN_F_VIRTUAL) != 0) {
+		if ((ex && (ch->flags & CHN_F_VIRTUAL) != 0) ||
+		    (!ex && (ch->flags & CHN_F_VIRTUAL) == 0)) {
 			CHN_UNLOCK(ch);
 			continue;
 		}
@@ -2160,14 +2162,9 @@ dsp_oss_audioinfo(struct cdev *i_dev, oss_audioinfo *ai, bool ex)
 		maxch = 0;
 		fmts = 0;
 		for (i = 0; caps->fmtlist[i]; i++) {
-			fmts |= caps->fmtlist[i];
-			if (AFMT_CHANNEL(caps->fmtlist[i]) > 1) {
-				minch = (minch == 0) ? 2 : minch;
-				maxch = 2;
-			} else {
-				minch = 1;
-				maxch = (maxch == 0) ? 1 : maxch;
-			}
+			fmts |= AFMT_ENCODING(caps->fmtlist[i]);
+			minch = min(AFMT_CHANNEL(caps->fmtlist[i]), minch);
+			maxch = max(AFMT_CHANNEL(caps->fmtlist[i]), maxch);
 		}
 
 		if (ch->direction == PCMDIR_PLAY)
@@ -2342,14 +2339,9 @@ dsp_oss_engineinfo(struct cdev *i_dev, oss_audioinfo *ai)
 			maxch = 0;
 			fmts = 0;
 			for (i = 0; caps->fmtlist[i]; i++) {
-				fmts |= caps->fmtlist[i];
-				if (AFMT_CHANNEL(caps->fmtlist[i]) > 1) {
-					minch = (minch == 0) ? 2 : minch;
-					maxch = 2;
-				} else {
-					minch = 1;
-					maxch = (maxch == 0) ? 1 : maxch;
-				}
+				fmts |= AFMT_ENCODING(caps->fmtlist[i]);
+				minch = min(AFMT_CHANNEL(caps->fmtlist[i]), minch);
+				maxch = max(AFMT_CHANNEL(caps->fmtlist[i]), maxch);
 			}
 
 			if (ch->direction == PCMDIR_PLAY)

[christos]

Tests with ossinfo(1) run fine.

[christos]

Simplify min/max stuff in dsp_oss_audioinfo().

[dev_submerge.ch]

In D45164#1033838, @christos wrote:

Tests with ossinfo(1) run fine.

Not yet, check ossinfo -v9 -a, see inline comment.

sys/dev/sound/pcm/dsp.c

2160

minch has to be initialized to something, e.g.

minch = AFMT_CHANNEL(caps->fmtlist[0]);

Otherwise minch will always be 0. Same for dsp_oss_engineinfo().

[christos]

Fix minch initialization.

[christos]

sys/dev/sound/pcm/dsp.c

2160

I did it for ai->min_channels but not for minch. I somehow missed this one... Thanks. :-)

[christos]

I think this patch should fix https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=252761 as well.