SDW Amp feedback not working on 6.14-rc3 or sof/linux - bus clashes

[rfvirgil]

On 6.14-rc3 I am seeing that using the SDW amp feeback causes lots of bus clashes. I get the same on current HEAD of sof/linux.
I have determined that the problem is this patch:

"soundwire: generic_bandwidth_allocation: count the bandwidth of active streams only"

If I revert that patch the amp feedback works.
I have attached kernel logs that include extra debug around the peripheral port setup code. These show the bad case using 6.14-rc3 and the working case with that patch reverted.

614-rc3-port-prep-bad.txt
614-rc3-patch-reverted-good.txt

This is the patch for the extra logging:
0001-Debugging-for-SDW-port-enable.patch.txt

The test case is to do loopback testing (simultaneous play to amps and capture from amps) with various different settings for which channel each amplifier is looping back (including not looping back anything), Here is the file listing the settings for each loopback test case, to give an idea of what the test cases are:
loopback2_cs35l56_sofsoundwire.yaml.txt
(The actual test is part of a larger test harness so not something that can easily be attached to a ticket and installed on another system)

It's difficult to match up what is happening in the good and bad cases, because the order of some of the actions seems to get shuffled around. But it looks like with that suspect patch:

• It is missing some writes to clear the enabled channels on an amp by writing 0 to the channel enable register
• There are extra writes to enable channels on an amp, which are not enabled in the good (reverted) case

Test hardware:

• UpXtreme MTL
• Cirrus Logic CDB35L56-EIGHT-C
• All 8 amps in use.
• DP1 is playback, DP3 is feedback.
• There are four feedback channels per bus, but only one DP3 channel is enabled on each amp, a different channel on each amp.

[rfvirgil]

@bardliao

[bardliao]

@rfvirgil Thanks for reporting this issue. I can't reproduce the issue if I simply do 2 channel playback and 6 channel capture simultaneously. Can you print the m_rt->stream->state in sdw_compute_group_params() and share the dmesg?

diff --git a/drivers/soundwire/generic_bandwidth_allocation.c b/drivers/soundwire/generic_bandwidth_allocation.c
index 840454e1d5fe..89dcdb4eb12e 100644
--- a/drivers/soundwire/generic_bandwidth_allocation.c
+++ b/drivers/soundwire/generic_bandwidth_allocation.c
@@ -232,7 +232,9 @@ static int sdw_compute_group_params(struct sdw_bus *bus,
                params[i].full_bw = bus->params.curr_dr_freq / params[i].rate;
        }

        list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
+               dev_dbg(bus->dev, "m_rt->stream->state %d\n", m_rt->stream->state);
                if (m_rt->stream == stream) {
                        /* Only runtime during prepare should be added */
                        if (stream->state != SDW_STREAM_CONFIGURED)
diff --git a/sound/soc/intel/common/soc-acpi-intel-lnl-match.c b/sound/soc/intel/common/soc-acpi-intel-lnl-match.c

I would like to check is there any m_rt->stream in an unexpected state.

[rfvirgil]

I added the debug and did 3 runs

debug1.txt
debug2.txt
debug3.txt

I noticed that the deprepare states at the end are different between logs 1/2 and log 3. Race condition?
Log 1:
[ 88.782654] soundwire sdw-master-0-0: m_rt->stream->state 3
[ 88.782660] soundwire sdw-master-0-0: m_rt->stream->state 5
[ 88.789191] soundwire sdw-master-0-1: m_rt->stream->state 3
[ 88.789197] soundwire sdw-master-0-1: m_rt->stream->state 5

Log 2:
[ 124.849147] soundwire sdw-master-0-0: m_rt->stream->state 3
[ 124.849153] soundwire sdw-master-0-0: m_rt->stream->state 5
[ 124.855338] soundwire sdw-master-0-1: m_rt->stream->state 3
[ 124.855340] soundwire sdw-master-0-1: m_rt->stream->state 5

Log 3:
[ 62.924182] soundwire sdw-master-0-0: m_rt->stream->state 5
[ 62.924190] soundwire sdw-master-0-0: m_rt->stream->state 3
[ 62.930021] soundwire sdw-master-0-1: m_rt->stream->state 5
[ 62.930023] soundwire sdw-master-0-1: m_rt->stream->state 3

[rfvirgil]

Here is a shell script that reproduces the problem on my UpX MTL with CDB35L56-EIGHT-C:

test.sh.txt

[bardliao]

@rfvirgil I still can't reproduce the issue on LNL. But I can only capture 6 channels as I am using AIOC v3 with 6 amps only. Not sure if it is the reason that I can't reproduce the issue.
I checked your logs and the stream states looks correct. The purpose of the "soundwire: generic_bandwidth_allocation: count the bandwidth of active streams only" patch is to not calculate that is not enabled. In other words, it is expected that the bandwidth will not be counted when the stream state is 5 (SDW_STREAM_DEPREPARED).
The difference after the patch reverted is that the deprepared stream's bandwidth will be calculated. As a result, the HCtrl, OffsetCtrl1 will be different.
Can you check does frame shape or bus clock change in your test?

diff --git a/drivers/soundwire/generic_bandwidth_allocation.c b/drivers/soundwire/generic_bandwidth_allocation.c
index 840454e1d5fe..eadfb6474abd 100644
--- a/drivers/soundwire/generic_bandwidth_allocation.c
+++ b/drivers/soundwire/generic_bandwidth_allocation.c
@@ -432,6 +432,7 @@ static int sdw_select_row_col(struct sdw_bus *bus, int clk_freq)

                        bus->params.row = sdw_rows[r];
                        bus->params.col = sdw_cols[c];
+                       dev_dbg(bus->dev, "clk_freq %d Row %d Col %d\n", clk_freq, bus->params.row, bus->params.col);
                        return 0;
                }
        }

Thanks.

[rfvirgil]

I tested a 6-amp configuration and that is ok.
I've added some more debug

0001-Debugging-for-SDW-port-enable.patch.txt

and captured logs of the working 6-amp and broken 8-amp:

6amp-noclash.txt
8amp-clash.txt

[bardliao]

@rfvirgil I think I got the issue now. It happened when the stream is enabled not disabled. I was focusing on the wrong direction. I.e. Disabling the stream.

[   62.627612] soundwire sdw-master-0-0: stream 'subdevice #0-Capture' state 1
[   62.627615] soundwire sdw-master-0-0: ... stream rate:48000 bps:24 ch=4
[   62.627618] soundwire sdw-master-0-0: ... payload_bw=96
[   62.627620] soundwire sdw-master-0-0: stream 'subdevice #0-Playback' state 2
[   62.627623] soundwire sdw-master-0-0: ... stream state 2 skipped
[   62.627625] soundwire sdw-master-0-0: ... group 0 payload_bw=96 full_bw=200 hwidth=2

It looks like a timing issue. We only count the bandwidth when the stream state is enabled or disabled.
But, if you open 2 streams on the same bus at the same time, the second sdw_compute_group_params() may be called when the first stream is still in the prepared state. Then the bandwidth of the first stream will not be counted. And the bus driver will not allocate enough bandwidth.
Can you try if below change help?

diff --git a/drivers/soundwire/generic_bandwidth_allocation.c b/drivers/soundwire/generic_bandwidth_allocation.c
index 840454e1d5fe..1444a3866e33 100644
--- a/drivers/soundwire/generic_bandwidth_allocation.c
+++ b/drivers/soundwire/generic_bandwidth_allocation.c
@@ -243,6 +243,7 @@ static int sdw_compute_group_params(struct sdw_bus *bus,
                         * streams
                         */
                        if (m_rt->stream->state != SDW_STREAM_ENABLED &&
+                           m_rt->stream->state != SDW_STREAM_PREPARED &&
                            m_rt->stream->state != SDW_STREAM_DISABLED)
                                continue;
                }

[rfvirgil]

Yes. Adding the check for SDW_STREAM_PREPARED stopped the bus clashes.

[bardliao]

Yes. Adding the check for SDW_STREAM_PREPARED stopped the bus clashes.

Thanks @rfvirgil I will double check if it is the right solution.