topology2: force all SoundWire link-side copiers to use S24_LE #9282

plbossart · 2024-07-04T11:04:29Z

Using S32_LE wastes bandwdith for no good reason, we should use 24 bits on the link to maximize bus efficiency with the 9.6 MHz bus clock.

~~FIXME: it's likely we need a kernel-side change as well to configure the hw_params to S24_LE, currently the stream/port is configured with~~

~~sconfig.bps = snd_pcm_format_width(params_format(params));~~

Link: #8960

plbossart · 2024-07-04T12:00:10Z

looks like this topology change is enough, according to kernel logs this changes the format

[  167.550540] snd_sof:sof_ipc4_pcm_dai_link_fixup: sof-audio-pci-intel-mtl 0000:00:1f.3: Set SDW0-Playback to 24 bit format
[  167.748407] snd_sof:sof_ipc4_pcm_dai_link_fixup: sof-audio-pci-intel-mtl 0000:00:1f.3: Set SDW1-Playback to 24 bit format
[  167.770560] snd_sof:sof_ipc4_pcm_dai_link_fixup: sof-audio-pci-intel-mtl 0000:00:1f.3: Set SDW0-Capture to 24 bit format
````

bardliao · 2024-07-04T12:15:57Z

looks like this topology change is enough, according to kernel logs this changes the format

[  167.550540] snd_sof:sof_ipc4_pcm_dai_link_fixup: sof-audio-pci-intel-mtl 0000:00:1f.3: Set SDW0-Playback to 24 bit format
[  167.748407] snd_sof:sof_ipc4_pcm_dai_link_fixup: sof-audio-pci-intel-mtl 0000:00:1f.3: Set SDW1-Playback to 24 bit format
[  167.770560] snd_sof:sof_ipc4_pcm_dai_link_fixup: sof-audio-pci-intel-mtl 0000:00:1f.3: Set SDW0-Capture to 24 bit format

Yes, we did it at https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/ipc4-pcm.c#L713.

plbossart · 2024-07-04T12:38:45Z

@bardliao I vaguely recall that the RT713/1713 configuration would give us one playback stream for the heaphone and two capture streams for headset and mic. Did we have any other cases where we ran out of bandwidth with a 4.8MHz clock?
I guess for the Rt722 we will have to use 9.6 MHz or two lanes, there's potentially 4 streams and that will not fly at 4.8/single lane.

plbossart · 2024-07-04T12:41:50Z

Not sure what I did but now I have zero valid bits haha Pin index #0: 0Hz, 0bit, 0ch

https://sof-ci.01.org/sofpr/PR9282/build6224/devicetest/index.html?model=LNLM_SDW_AIOC&testcase=check-capture-10sec

[  245.072209] kernel: snd_sof:sof_ipc4_init_input_audio_fmt: sof-audio-pci-intel-lnl 0000:00:1f.3: Init input audio formats for alh-copier.SDW0-Capture.0
[  245.072213] kernel: snd_sof:sof_ipc4_dbg_audio_format: sof-audio-pci-intel-lnl 0000:00:1f.3: Pin index #0: 48000Hz, 32bit, 2ch (ch_map 0xffffff10 ch_cfg 1 interleaving_style 0 fmt_cfg 0x1802) buffer size 384
[  245.072219] kernel: snd_sof:sof_ipc4_prepare_copier_module: sof-audio-pci-intel-lnl 0000:00:1f.3: copier alh-copier.SDW0-Capture.0: reference output rate 48000, channels 2 valid_bits 0
[  245.072223] kernel: snd_sof:sof_ipc4_prepare_copier_module: sof-audio-pci-intel-lnl 0000:00:1f.3: Output audio format for alh-copier.SDW0-Capture.0
[  245.072225] kernel: snd_sof:sof_ipc4_dbg_audio_format: sof-audio-pci-intel-lnl 0000:00:1f.3: Pin index #0: 0Hz, 0bit, 0ch (ch_map 0x0 ch_cfg 0 interleaving_style 0 fmt_cfg 0x0) buffer size 0
[  245.072235] kernel: sof-audio-pci-intel-lnl 0000:00:1f.3: invalid PCM valid_bits 0
[  245.072331] kernel: sof-audio-pci-intel-lnl 0000:00:1f.3: failed to prepare widget alh-copier.SDW0-Capture.0

plbossart · 2024-07-04T12:55:48Z

v2: fixed the jack capture. the output format was missing.

bardliao · 2024-07-05T01:22:02Z

@bardliao I vaguely recall that the RT713/1713 configuration would give us one playback stream for the heaphone and two capture streams for headset and mic. Did we have any other cases where we ran out of bandwidth with a 4.8MHz clock? I guess for the Rt722 we will have to use 9.6 MHz or two lanes, there's potentially 4 streams and that will not fly at 4.8/single lane.

rt713/1713 uses a separated SDW interfaces for mic which is 1713. And rt713 is headset only. rt712 is rt713 + speaker playback. So, rt712 has up to 2 playback streams (headphone and speaker) and 1 capture stream (headset). IOW, the 4.8MHz bandwidth is enough for rt712/713 series. However, rt722 uses the same SDW interface for mic, and has up to 2 playback streams (headphone and speaker) and 2 capture streams (headset and mic). So, rt722 needs 9.6MHz or multilane. Same as rt712-VB. @shumingfan Please correct me if I am wrong.

plbossart · 2024-07-05T06:38:41Z

Just to be clear @bardliao "the 4.8MHz bandwidth is enough for rt712/713 series" is only true with this topology modification. If we use 32-bits, then we don't have enough bandwidth for 3 streams at 4.8 MHz with a 50x4 frame shape.

bardliao · 2024-07-05T07:13:36Z

Just to be clear @bardliao "the 4.8MHz bandwidth is enough for rt712/713 series" is only true with this topology modification. If we use 32-bits, then we don't have enough bandwidth for 3 streams at 4.8 MHz with a 50x4 frame shape.

Oh, yes, that's right. I assume 24 bit format is used.

plbossart · 2024-07-05T07:33:06Z

I am having second-thoughts on the PR, maybe we should avoid hard-coding the valid bits to 24, e.g. if we use 12.288 MHz 32 bits would be just fine since we'd have 64 rows. Probably a macro would be better.

bardliao · 2024-07-05T07:37:16Z

I am having second-thoughts on the PR, maybe we should avoid hard-coding the valid bits to 24, e.g. if we use 12.288 MHz 32 bits would be just fine since we'd have 64 rows. Probably a macro would be better.

Then we just need to set SDW_AMP_FMT_24 and SDW_JACK_FMT_24 = true by default.

plbossart · 2024-07-05T07:43:44Z

Then we just need to set SDW_AMP_FMT_24 and SDW_JACK_FMT_24 = true by default.

Humm... the configuration does not really depend on the type of function but rather on the bus/link configuration. it's really SDW_LINK_BITS or something, and we want this to be a value not a boolean to make the code simpler.

bardliao · 2024-07-05T07:47:07Z

Then we just need to set SDW_AMP_FMT_24 and SDW_JACK_FMT_24 = true by default.

Humm... the configuration does not really depend on the type of function but rather on the bus/link configuration. it's really SDW_LINK_BITS or something, and we want this to be a value not a boolean to make the code simpler.

Be a value is definitely better. I just don't know how to do it. :)

Using S32_LE wastes bandwdith for no good reason, we should use 24 bits on the link to maximize bus efficiency with the 9.6 MHz bus clock. There is no need for a kernel-side change, the dailink fixup already changes the dailink format based on the topology information. Link: thesofproject#8960 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2024-07-05T08:10:01Z

tools/topology/topology2/platform/intel/sdw-amp-generic.conf

 								in_sample_type		$SAMPLE_TYPE_MSB_INTEGER
 								in_fmt_cfg		"$[($in_channels | ($in_valid_bit_depth * 256))]"
 							}
 						]
 						Object.Base.output_audio_format [
 							{
 								out_bit_depth            32
-								out_valid_bit_depth      32
-								out_sample_type		$SAMPLE_TYPE_MSB_INTEGER
+								out_valid_bit_depth      24


this is wrong....

plbossart · 2024-07-05T08:23:45Z

v3: added SDW_LINK_VALID_BITS macro and fixed bad edit for output format.

bardliao · 2024-07-05T08:48:46Z

Do we need to consider about in_bit_depth and out_bit_depth, too? For example, in_bit_depth and out_bit_depth can be 16 if the valid bit is 16.

plbossart · 2024-07-05T09:12:41Z

The in_bit_depth is always 32 for the capture dai copier and the output_bit_depth is always 32 for the playback dai copier, isn't it? The options for 16 bits are only on the host-copier side

bardliao · 2024-07-05T11:23:22Z

The in_bit_depth is always 32 for the capture dai copier and the output_bit_depth is always 32 for the playback dai copier, isn't it? The options for 16 bits are only on the host-copier side

I am not sure about it. We have for example out_bit_depth 16 in current sdw-amp-generic.conf.

lgirdwood

LGTM, and will get plenty of PR testing prior to v2.11

plbossart · 2024-07-05T16:39:20Z

The in_bit_depth is always 32 for the capture dai copier and the output_bit_depth is always 32 for the playback dai copier, isn't it? The options for 16 bits are only on the host-copier side

I am not sure about it. We have for example out_bit_depth 16 in current sdw-amp-generic.conf.

I removed all this to have a single bit depth on the link side. This configuration of 16 bits in 16 bit words makes no sense for SoundWire, the FIFOs only work for 32 bits. The only variable is the word length i.e. the number of bits pushed on the bus.

plbossart · 2024-07-05T16:40:24Z

@kv2019i @lgirdwood this PR is not urgent, it should only be merged after re-testing all the SoundWire devices we have in the daily tests.

lgirdwood · 2024-07-08T09:14:47Z

@kv2019i @lgirdwood this PR is not urgent, it should only be merged after re-testing all the SoundWire devices we have in the daily tests.

@ssavati @fredoh9 can someone help here with soundwire testing using this PR ?

ssavati · 2024-07-08T14:37:51Z

@ssavati @fredoh9 can someone help here with soundwire testing using this PR ?

Sure @lgirdwood I will run more test on SDW

ssavati · 2024-07-11T04:56:45Z

@ssavati @fredoh9 can someone help here with soundwire testing using this PR ?

@lgirdwood I have completed soundwire functional and stress tests for MTL and LNL . no new regression observed.
For more details look into test report ID 43727 and 43732

plbossart · 2024-07-15T10:13:02Z

Thanks @ssavati, all good to merge then.

plbossart requested review from ranj063, bardliao and ujfalusi July 4, 2024 11:04

plbossart force-pushed the sdw/use-24-bits-on-link-side branch from e32b01c to c1487a0 Compare July 4, 2024 12:55

plbossart commented Jul 5, 2024

View reviewed changes

plbossart force-pushed the sdw/use-24-bits-on-link-side branch from c1487a0 to 08efff3 Compare July 5, 2024 08:22

plbossart marked this pull request as ready for review July 5, 2024 08:23

plbossart requested a review from jsarha as a code owner July 5, 2024 08:23

lgirdwood added this to the v2.11 milestone Jul 5, 2024

lgirdwood approved these changes Jul 5, 2024

View reviewed changes

bardliao approved these changes Jul 8, 2024

View reviewed changes

kv2019i merged commit 6382963 into thesofproject:main Jul 15, 2024
45 of 47 checks passed

bardliao mentioned this pull request Aug 2, 2024

[BUG] Acer Swift Go 14 - No sound card detected - "No SoundWire machine driver found" thesofproject/linux#4923

Open

bardliao mentioned this pull request Aug 16, 2024

topology2: cavs-sdw: Add cs42l43 and cs35l56 support on ADL/RPL, MTL/ARL, LNL #9355

Merged

bardliao mentioned this pull request Sep 5, 2024

[BUG] SoundWire topology: need to use 24 bits #8960

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

topology2: force all SoundWire link-side copiers to use S24_LE #9282

topology2: force all SoundWire link-side copiers to use S24_LE #9282

plbossart commented Jul 4, 2024 •

edited

Loading

plbossart commented Jul 4, 2024

bardliao commented Jul 4, 2024

plbossart commented Jul 4, 2024

plbossart commented Jul 4, 2024 •

edited

Loading

plbossart commented Jul 4, 2024

bardliao commented Jul 5, 2024

plbossart commented Jul 5, 2024

bardliao commented Jul 5, 2024

plbossart commented Jul 5, 2024

bardliao commented Jul 5, 2024

plbossart commented Jul 5, 2024 •

edited

Loading

bardliao commented Jul 5, 2024

plbossart Jul 5, 2024

plbossart Jul 5, 2024

plbossart commented Jul 5, 2024

bardliao commented Jul 5, 2024

plbossart commented Jul 5, 2024

bardliao commented Jul 5, 2024

lgirdwood left a comment

plbossart commented Jul 5, 2024

plbossart commented Jul 5, 2024

lgirdwood commented Jul 8, 2024

ssavati commented Jul 8, 2024 •

edited

Loading

ssavati commented Jul 11, 2024

plbossart commented Jul 15, 2024

topology2: force all SoundWire link-side copiers to use S24_LE #9282

topology2: force all SoundWire link-side copiers to use S24_LE #9282

Conversation

plbossart commented Jul 4, 2024 • edited Loading

plbossart commented Jul 4, 2024

bardliao commented Jul 4, 2024

plbossart commented Jul 4, 2024

plbossart commented Jul 4, 2024 • edited Loading

plbossart commented Jul 4, 2024

bardliao commented Jul 5, 2024

plbossart commented Jul 5, 2024

bardliao commented Jul 5, 2024

plbossart commented Jul 5, 2024

bardliao commented Jul 5, 2024

plbossart commented Jul 5, 2024 • edited Loading

bardliao commented Jul 5, 2024

plbossart Jul 5, 2024

Choose a reason for hiding this comment

plbossart Jul 5, 2024

Choose a reason for hiding this comment

plbossart commented Jul 5, 2024

bardliao commented Jul 5, 2024

plbossart commented Jul 5, 2024

bardliao commented Jul 5, 2024

lgirdwood left a comment

Choose a reason for hiding this comment

plbossart commented Jul 5, 2024

plbossart commented Jul 5, 2024

lgirdwood commented Jul 8, 2024

ssavati commented Jul 8, 2024 • edited Loading

ssavati commented Jul 11, 2024

plbossart commented Jul 15, 2024

plbossart commented Jul 4, 2024 •

edited

Loading

plbossart commented Jul 4, 2024 •

edited

Loading

plbossart commented Jul 5, 2024 •

edited

Loading

ssavati commented Jul 8, 2024 •

edited

Loading