Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check-alsabat.sh: add a new option '-N' to specify the channel number… #1143

Merged

Conversation

keqiaozhang
Copy link
Contributor

… for playback

Before the channel number of playback was hard coded to 2. since sof playback PCMs only support 2 channels. But we now we have Bluetooth offload with nocodec topology which support both 1 channel and 2 channels. So replace this hard coding to optional.

@keqiaozhang keqiaozhang requested a review from a team as a code owner December 28, 2023 06:44
… for playback

Before the channel number of playback was hard coded to 2. since sof playback
PCMs only support 2 channels. But we now we have Bluetooth offload with nocodec
topology which support both 1 channel and 2 channels. So replace this hard coding
to optional.

Signed-off-by: Keqiao Zhang <[email protected]>
@keqiaozhang keqiaozhang force-pushed the alsabat_channel_optional branch from f886a03 to cb81613 Compare January 4, 2024 06:49
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would anyone use a different number of channels for playback and capture?

@plbossart
Copy link
Member

Why would anyone use a different number of channels for playback and capture?

speaker has 2 channels and DMIC 4. If you want to run an alsa-bat test using speaker+dmic the number of channels MUST be different.

that said I am not sure which nocodec topology this test is using, to the best of my knowledge the SSP MUST use the same number of channels in both directions. Some slots may not be used and contain zero data, but that's a separate issue. The SSP requires that both RX and TX use the same bclock, fsync, wordsize and number of channels/slots. If we are testing with two SSPs then one of the two needs to be slave. this could be fun at multiple levels.

What are we trying to test really?

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 4, 2024

speaker has 2 channels and DMIC 4. If you want to run an alsa-bat test using speaker+dmic the number of channels MUST be different.

I don't think we ever use this test "over the air", that would be too unreliable in very noisy lab.

Even if we did or someone else does in a very quiet environment: why wouldn't using just 2 channels be enough? Or even just one in that case.

What are we trying to test really?

Good question; the test description does not say what it's meant for, what it supports versus not (it's also using alsabat in undocumented way, minor doc fix submitted in #1148)

So far we've been using stereo USB adapters with headset jack loopback.

This PR adds the following comment: # BT offload PCMs also support mono playback.

@keqiaozhang
Copy link
Contributor Author

Before playback channel number was hard coded to 2, but now we have a new request for Bluetooth offload tplg, it has 3 cases:

  1. Test Bluetooth SCO HFP 8kHz (narrow-band) case
  2. Test Bluetooth SCO HFP 16kHz (wide-band) case
  3. Test Bluetooth A2DP 48kHz stereo case

Case 1 & 2 only support mono playback/capture, that's why I replaced the hard coded channel number with parameter forms.
Please refer to:
https://wiki.ith.intel.com/display/LINUXAUDIO/How+to+test+Bluetooth+Offload+with+nocodec+topology

@marc-hb marc-hb requested a review from kv2019i January 5, 2024 20:11
@marc-hb
Copy link
Collaborator

marc-hb commented Jan 5, 2024

@keqiaozhang my question is why patch below and a single option is not enough, at least for now. It would be 100% compatible with the wiki page you shared.

--- a/test-case/check-alsabat.sh
+++ b/test-case/check-alsabat.sh
@@ -33,7 +33,7 @@ source "$(dirname "${BASH_SOURCE[0]}")"/../case-lib/lib.sh
 OPT_NAME['p']='pcm_p'     	OPT_DESC['p']='pcm for playback. Example: hw:0,0'
 OPT_HAS_ARG['p']=1          	OPT_VAL['p']=''
 
-OPT_NAME['C']='channel_c'       OPT_DESC['C']='channel number for capture.'
+OPT_NAME['C']='channel_c'       OPT_DESC['C']='number of channels.'
 OPT_HAS_ARG['C']=1             OPT_VAL['C']='1'
 
 OPT_NAME['r']='rate'            OPT_DESC['r']='sample rate'
@@ -110,9 +110,8 @@ aplay   -Dplug$pcm_p -d 1 /dev/zero -q || die "Failed to play on PCM: $pcm_p"
 arecord -Dplug$pcm_c -d 1 /dev/null -q || die "Failed to capture on PCM: $pcm_c"
 
 # alsabat test
-# hardcode the channel number of playback to 2, as sof doesnot support mono wav.
-dlogc "alsabat -P$pcm_p --standalone -n $frames -r $rate -c 2 -f $format -F $frequency -k $sigmak"
-alsabat -P$pcm_p --standalone -n $frames -c 2 -r $rate -f $format -F $frequency -k $sigmak & playPID=$!
+dlogc "alsabat -P$pcm_p --standalone -n $frames -r $rate -c $channel -f $format -F $frequency -k $sigmak"
+alsabat -P$pcm_p --standalone -n $frames -c $channel -r $rate -f $format -F $frequency -k $sigmak & playPID=$!
 
 # playback may have low latency, add one second delay to aviod recording zero at beginning.
 sleep 1

Even if we need a way to configure a different number of channels in the future, there should be a simple way to configure both channels the same.

Keep it simple for now.

@keqiaozhang
Copy link
Contributor Author

my question is why patch below and a single option is not enough

Besides of nocodec platforms, not all our devices have stereo USB sound card connected. For most of the CAVS platforms, the USB sound card only support mono capture, that why we cannot use one option to configure both channels the same.

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 8, 2024

SOFCI TEST

alsabat -P$pcm_p --standalone -n $frames -c 2 -r $rate -f $format -F $frequency -k $sigmak & playPID=$!
# BT offload PCMs also support mono playback.
dlogc "alsabat -P$pcm_p --standalone -n $frames -r $rate -c $channel_p -f $format -F $frequency -k $sigmak"
alsabat -P$pcm_p --standalone -n $frames -c $channel_p -r $rate -f $format -F $frequency -k $sigmak & playPID=$!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the change sounds fine but I can't figure out how this test works with separate commands for playback and capture. I don't have enough understanding to review this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have some configuration files in framework side to specify PCM IDs and channel numbers, e.g:https://github.com/intel-innersource/drivers.audio.ci.sof-framework/blob/main/parameters/alsabat-playback.csv.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure out how this test works with separate commands for playback and capture

Maybe because ALSA man pages are utterly out of date? I struggled with that too, see yesterday's 84dcefd

"man speaker-test" is 11 years old... speaker-test -h looks fine.

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 9, 2024

For most of the CAVS platforms, the USB sound card only support mono capture, that why we cannot use one option to configure both channels the same.

Why not use -c 1 everywhere in that case? What's the point of playing 2 channels and capturing only 1?

@keqiaozhang
Copy link
Contributor Author

Why not use -c 1 everywhere in that case?

Hope I understand your question correctly, our SOF doesn't support 1 channel playback except for Bluetooth offload. So we cannot set 1 channel for both playback and USB capture.

What's the point of playing 2 channels and capturing only 1?

Because at that time we only have one model of USB sound card to use, and it only support mono capture. In order to enable audio quality testing, we have no other choice.

I will merge this PR to move the blocker for Bluetooth test.

@keqiaozhang keqiaozhang merged commit 7ff3b0e into thesofproject:main Jan 10, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants