soundwire: add multi-lane support #4733

bardliao · 2023-12-06T10:07:25Z

The first 2 commits are from #4704.

This PR adds SoundWire multi-lane support.

bardliao · 2023-12-07T08:11:39Z

Hmm, the CI test Failure on TGLU_RVP_SDW-ipc4 is because that 2 1308 amps are on the same sdw link, and 1 controller data port maps to 2 peripheral data ports. But I assume that controller data port and peripheral data port is 1:1 mapping. I need to think about how to handle this case.

plbossart

Good start @bardliao and @aiChaoSONG. I added a number of comments to make this PR more precise and clarify a number of technical opens.

drivers/soundwire/mipi_disco.c

plbossart · 2024-03-06T20:10:45Z

drivers/soundwire/mipi_disco.c

+		len = strlen(prop_val);
+		/* The last character is enough to identify the connection */
+		prop->lane_maps[i] = prop_val[len - 1];
+		dev_err(dev, "[Chao] %c\n", prop->lane_maps[i]);


remove this dev_err or demote to dev_dbg.

plbossart · 2024-03-06T20:11:55Z

include/linux/soundwire/sdw.h

@@ -363,6 +365,7 @@ struct sdw_dpn_prop {
 * @p15_behave: Slave behavior when the Master attempts a read to the Port15
 * alias
 * @lane_control_support: Slave supports lane control
+ * @lane_maps: Lane mapping for the slave


presumably only valid if lane_control_support is true?

presumably only valid if lane_control_support is true?

Yes, and changed the description to "Lane mapping for the slave, only valid if lane_control_support is set"

drivers/soundwire/mipi_disco.c

plbossart · 2024-03-06T20:19:28Z

drivers/soundwire/generic_bandwidth_allocation.c

@@ -314,7 +314,7 @@ static int sdw_select_row_col(struct sdw_bus *bus, int clk_freq)
 			frame_freq = clk_freq / frame_int;

 			if ((clk_freq - (frame_freq * SDW_FRAME_CTRL_BITS)) <
-			    bus->params.bandwidth)
+			    bandwidth)


I don't follow this patch at all.

First, we need to avoid that the frame size selected depends on the bandwidth required on each lane. It would be a disaster we had different results for each lane.

Second, this definition

include/linux/soundwire/sdw.h:#define SDW_FRAME_CTRL_BITS 48

is ONLY valid for lane0. There are no control bits on non-zero lanes, we can use the entire set of columns for audio data (or BPT/BRA).

Let me think more about it. My intent is to allocate lane according to available bandwidth on each lane. That's why I extend bandwidth to an array.

Right, but you could pick the lane which has the lowest bandwidth used, or conversely try to use the lane that's the most used. You have two dimensions to play with.

For corner cases, we will have to use max speed and max lanes. It's the intermediate cases that aren't so clear.

Hmm, I guess I can use a new lane if current bandwidth is not enough in sdw_compute_bus_params(). I will try it.

plbossart · 2024-03-06T20:20:08Z

drivers/soundwire/bus.h

@@ -97,7 +97,7 @@ int sdw_find_col_index(int col);
 */
 struct sdw_port_runtime {
 	int num;
-	int ch_mask;
+	u32 ch_mask;


commit title prefix: s/soundwore/soundwire/

plbossart · 2024-03-06T20:25:28Z

include/linux/soundwire/sdw.h

 */
 struct sdw_port_config {
 	unsigned int num;
 	unsigned int ch_mask;
+	unsigned int lane_mask;


lane 0 can only be used by port 1

I don't think it's realistic to assume that some ports can only be used with non-zero lanes. By default I think the behavior is that all ports support Lane0. It'd be surprised if this wasn't the case, since that's the common denominator for all systems. It's possible that some non-zero lanes are not supported, e.g. a device could perfectly well support lane0..3 only.

The lane_maps[i] will be 0 by default and we can't tell if a lane is connected to lane 0 or not connected.

I couldn't follow the "lane is connected to lane0"....

lane 0 can only be used by port 1

That is just an example to describe codec driver may want to specify a port to use a specific lane.
For example, headphone playback uses port 0 and lane 0. In that case, codec driver can set port_config.num=0 and port_config.lane_mask=0x1;

I don't think it's realistic to assume that some ports can only be used with non-zero lanes.

No, I don't assume that some ports can only be used with non-zero lanes.
The issue is that if peripheral lane x is not connected, the mipi-sdw-lane-x-mapping property will not be set and prop->lane_maps[x] will be 0 which is the same as if peripheral lane x is connected to manager lane 0.

The lane_maps[i] will be 0 by default and we can't tell if a lane is connected to lane 0 or not connected.

I couldn't follow the "lane is connected to lane0"....

Sorry, it should be "a peripheral lane is connected to manager lane 0".

plbossart · 2024-03-06T20:27:59Z

drivers/soundwire/stream.c

+	for (i = 0; i < SDW_MAX_LANES; i++)
+		available_bandwidth[i] = bus->params.max_dr_freq - bus->params.bandwidth[i];
+
+	/* find available lane */


find first available lane?

I think the algorithm described tries to use a minimal number of lanes, instead of spreading the load on multiple lanes? It could be interesting if we try to change the frequency as well :-)

I think the algorithm described tries to use a minimal number of lanes, instead of spreading the load on multiple lanes? It could be interesting if we try to change the frequency as well

Good point. I was thinking to use as less lanes as possible. Let me try to spread the load on multiple lanes.

I don't know what's best in terms of power consumption.

There are two options:

optimize the clock and reduce the number of lanes, i.e. only use additional lanes when the bandwidth is too low at max speed

always use additional lanes to reduce the speed.

If the speed is fixed, it's not clear either. On one hand there's a need to enable a new line, but with the NRZI encoding if there's nothing to transmit then there are no transitions on the data line.

plbossart · 2024-03-06T20:30:46Z

drivers/soundwire/stream.c

 	int ret;
+	int i;


"A controller should use the lane that connects to the peripheral"

That wording is controversial.

First we are talking about a Manager. Controllers don't have a concept of Lanes, they are made of one or more Managers. Each Manager can have one or more Lanes. Different level.

Also, you could perfectly well have multiple lanes connecting a Manager and a Peripheral. For example to transmit 8ch 192kHz 24 bits we would need 4 lanes, one for each stereo pair.

The commit is to map the lane that connects to a specific peripheral lane. For example, peripheral lane 1 connects to manager lane 2. We already know which peripheral lane is used and need to get the manager lane by slave_prop->lane_maps.

plbossart

Sorry for the late review @bardliao, see comments below.

plbossart · 2024-03-28T19:58:12Z

drivers/soundwire/mipi_disco.c

+			continue;
+		len = strlen(prop_val);
+		/* The last character is enough to identify the connection */
+		prop->lane_maps[i] = prop_val[len - 1] - '0';


comment from @andy-shev still applies
https://github.com/thesofproject/linux/pull/4857/files/856d130a14d412c016565ba7a429df38eb07b7a4..618319957b33c5a7638becc6ef7d3aefdeb0b822#r1529112951
"
IIRC, the string can be empty, this code then has a potential out-of-boundary access.
"

drivers/soundwire/generic_bandwidth_allocation.c

plbossart · 2024-03-28T20:04:40Z

drivers/soundwire/generic_bandwidth_allocation.c

+
+			/* Find a non-zero manager lane */
+			for (i = 1; i < SDW_MAX_LANES; i++) {
+				if (!slave_prop->lane_maps[i])


I think you should first check that all peripherals connected to that manager and using that stream do support the same lanes. Otherwise you may pick a lane that works for one of the peripherals but not for another.

This is a bit paranoid but it's not that crazy: we could have a big all-in-one device along with a companion amplifier on the same link. The big device will likely support multi-lane, the companion amplifier probably not.

drivers/soundwire/generic_bandwidth_allocation.c

plbossart · 2024-04-25T21:05:20Z

drivers/soundwire/mipi_disco.c

+		ret = kstrtou8(&prop_val[len - 1], 10, &lane);
+		if (ret)
+			return ret;
+		if (in_range(lane, 1, 7))


1, SDW_MAX_LANES - 1 ?

drivers/soundwire/mipi_disco.c

plbossart · 2024-04-25T21:06:46Z

drivers/soundwire/mipi_disco.c

@@ -382,6 +415,8 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
 	sdw_slave_read_dpn(slave, prop->sink_dpn_prop, nval,
 			   prop->sink_ports, "sink");

+	sdw_slave_read_lane_mapping(slave);


do we want to discard errors? The error codes are not handled.

Changed to return sdw_slave_read_lane_mapping(slave);

plbossart · 2024-04-25T21:29:07Z

drivers/soundwire/generic_bandwidth_allocation.c

+		/*
+		 * Find an available manager lane for the first Peripheral
+		 * Testing the first Peripheral is enough because we can't use multi-lane
+		 * if we can't find any available lane for the first Peripheral


I don't follow the logic.

If we can't find any available lanes for the first Peripheral, then we can't use multi-lane.
Buf if we want one available lane for the first Peripheral, we still need to check if other peripherals can use this lane.

Edit: and later you actually test all lanes so the comment is misleading. You start by testing the first peripheral and later verify that that this configuration works for all peripherals.

The logic here is to find an available lane from the first Peripheral, then check if the lane is available to all Peripherals.
The comment is to explain why I use s_rt = list_first_entry(&m_rt->slave_rt_list, struct sdw_slave_runtime, m_rt_node);

drivers/soundwire/generic_bandwidth_allocation.c

plbossart · 2024-04-25T21:34:45Z

drivers/soundwire/generic_bandwidth_allocation.c

+			}
+		}
+		/* Set Manager lanes */
+		/* Assume there is only one Manager in multi-lane configurations. */


I don't follow this comment, in the most agressive 192kHz 8ch bandwidth, we will have two links with two lanes each.

I don't follow this comment, in the most agressive 192kHz 8ch bandwidth, we will have two links with two lanes each.

Hmm, I can't test the 2 links case for now. But for that case, sdw_compute_bus_params() will be called twice for each Manager, right?

Edited the comments. Hope it will be more readable.

drivers/soundwire/stream.c

drivers/soundwire/generic_bandwidth_allocation.c

drivers/soundwire/mipi_disco.c

ranj063

looks good except for the first patch missing your sign-off @bardliao

The DisCo for SoundWire 2.0 added support for the 'mipi-sdw-lane-<n>-mapping' property. Co-developed-by: Chao Song <chao.song@linux.intel.com> Signed-off-by: Chao Song <chao.song@linux.intel.com> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

To support multi-lane, we need to know how much bandwidth is used on each lane. And to use the lane that has enough bandwidth. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao · 2024-08-28T01:50:16Z

looks good except for the first patch missing your sign-off @bardliao

Updated to

Co-developed-by: Chao Song <chao.song@linux.intel.com>
Signed-off-by: Chao Song <chao.song@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao · 2024-08-29T06:10:23Z

@plbossart @ranj063 @ujfalusi I just found an issue when I tested with dynamic sdw bus clock. We need to clear p_rt->ch_mask when deprepare. Otherwise, sdw_compute_group_params() will still count the port bandwidth which is already stop when it is called in deprepare. And it will return error when we stop a stream and reduce the sdw clock. The same issue happens in the existing code, too. But we won't meet the issue because currently the sdw clock will never change.

bardliao · 2024-08-29T08:29:00Z

@plbossart @ranj063 @ujfalusi I just found an issue when I tested with dynamic sdw bus clock. We need to clear p_rt->ch_mask when deprepare. Otherwise, sdw_compute_group_params() will still count the port bandwidth which is already stop when it is called in deprepare. And it will return error when we stop a stream and reduce the sdw clock. The same issue happens in the existing code, too. But we won't meet the issue because currently the sdw clock will never change.

It will impact the multi-lane support. I need to think more about it.

ranj063 · 2024-08-29T19:04:10Z

@plbossart @ranj063 @ujfalusi I just found an issue when I tested with dynamic sdw bus clock. We need to clear p_rt->ch_mask when deprepare. Otherwise, sdw_compute_group_params() will still count the port bandwidth which is already stop when it is called in deprepare. And it will return error when we stop a stream and reduce the sdw clock. The same issue happens in the existing code, too. But we won't meet the issue because currently the sdw clock will never change.

It will impact the multi-lane support. I need to think more about it.

@bardliao are you saying this PR is not ready for merging then? Also, what is the plan for testing multilane? I understand you have tested this locally but do we need to add tests to our CI also?

bardliao · 2024-08-30T05:16:08Z

@plbossart @ranj063 @ujfalusi I just found an issue when I tested with dynamic sdw bus clock. We need to clear p_rt->ch_mask when deprepare. Otherwise, sdw_compute_group_params() will still count the port bandwidth which is already stop when it is called in deprepare. And it will return error when we stop a stream and reduce the sdw clock. The same issue happens in the existing code, too. But we won't meet the issue because currently the sdw clock will never change.

It will impact the multi-lane support. I need to think more about it.

@bardliao are you saying this PR is not ready for merging then? Also, what is the plan for testing multilane? I understand you have tested this locally but do we need to add tests to our CI also?

After 1 and half day's investigation, I decide to roll back to the reviewed/approved and full tested version. I.e. remove the set p_rt->ch_mask = 0 in deprepare change. I will continue debug the issue when enabling dynamic sdw clock change. But in the meantime, we can merge this if you agree. @ranj063 @plbossart @ujfalusi.

And yes, we need to setup a device to test multilane in CI. We will need a codec that can support multilane like rt722, rt712-vb

bardliao · 2024-09-02T11:39:47Z

change: remove duplicated trace.

drivers/soundwire/generic_bandwidth_allocation.c

ujfalusi · 2024-09-04T08:48:52Z

drivers/soundwire/generic_bandwidth_allocation.c

-		column_needed += params[i].hwidth;
+			params[i].hwidth = (sel_col *
+				params[i].payload_bw + params[i].full_bw - 1) /
+				params[i].full_bw;


nitpick, but I think it would be easier to grasp this way:

params[i].hwidth = (sel_col * params[i].payload_bw + params[i].full_bw - 1) / params[i].full_bw;

drivers/soundwire/generic_bandwidth_allocation.c

If a peripheral supports multi-lane, we can use data lane x to extend the bandwidth. The patch suggests to select data lane x where x > 0 when bandwidth is not enough on data lane 0. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

All active streams with the same parameters are grouped together and the params are store in the sdw_group struct. We compute the required bandwidth for each group. However, each lane has individual bandwidth. Therefore, we should separate different lanes in different params groups. Add lane variable to separate params groups. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Rt722 supports multi-lane and the driver doesn't call sdw_slave_read_prop() to get all properties. Add sdw_slave_read_lane_mapping() to get the required lane mapping property. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao requested review from charleskeepax, plbossart, shumingfan and aiChaoSONG December 6, 2023 10:07

bardliao force-pushed the for-multi-lane branch 3 times, most recently from 92e8966 to 96b2055 Compare December 7, 2023 07:29

bardliao force-pushed the for-multi-lane branch from 96b2055 to d08d891 Compare December 7, 2023 11:39

bardliao force-pushed the for-multi-lane branch 2 times, most recently from 6a87659 to 8b8ec3b Compare March 5, 2024 11:29

plbossart requested changes Mar 6, 2024

View reviewed changes

bardliao force-pushed the for-multi-lane branch 2 times, most recently from aac2feb to c8fea39 Compare March 22, 2024 11:29

plbossart reviewed Mar 28, 2024

View reviewed changes

bardliao force-pushed the for-multi-lane branch 4 times, most recently from 181bbf0 to e4313db Compare April 3, 2024 11:05

bardliao force-pushed the for-multi-lane branch 2 times, most recently from 2f65517 to 2b2853f Compare April 16, 2024 13:30

bardliao changed the title ~~[RFC]: soundwire: add multi-lane support~~ soundwire: add multi-lane support Apr 16, 2024

bardliao marked this pull request as ready for review April 16, 2024 13:32

bardliao force-pushed the for-multi-lane branch from 2b2853f to c542de2 Compare April 17, 2024 08:48

bardliao requested a review from plbossart April 23, 2024 09:38

plbossart reviewed Apr 25, 2024

View reviewed changes

bardliao force-pushed the for-multi-lane branch 2 times, most recently from f388e84 to 137b0e8 Compare April 26, 2024 09:25

plbossart mentioned this pull request Apr 30, 2024

SoundWire: mipi-disco: add partial 2.1 support #4857

Merged

plbossart reviewed May 13, 2024

View reviewed changes

drivers/soundwire/generic_bandwidth_allocation.c Show resolved Hide resolved

ujfalusi previously approved these changes Aug 27, 2024

View reviewed changes

plbossart previously approved these changes Aug 27, 2024

View reviewed changes

plbossart requested a review from ranj063 August 27, 2024 12:01

ranj063 reviewed Aug 27, 2024

View reviewed changes

drivers/soundwire/mipi_disco.c Show resolved Hide resolved

ranj063 previously approved these changes Aug 27, 2024

View reviewed changes

bardliao added 2 commits August 28, 2024 09:44

soundwire: add lane_used_bandwidth in struct sdw_bus

0989a01

To support multi-lane, we need to know how much bandwidth is used on each lane. And to use the lane that has enough bandwidth. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the for-multi-lane branch from b425ded to a49a843 Compare August 28, 2024 01:45

bardliao dismissed stale reviews from ujfalusi, ranj063, and plbossart via fba53ae August 29, 2024 05:46

bardliao force-pushed the for-multi-lane branch from a49a843 to fba53ae Compare August 29, 2024 05:46

bardliao force-pushed the for-multi-lane branch from fba53ae to 9a34250 Compare August 29, 2024 09:43

bardliao force-pushed the for-multi-lane branch from 9a34250 to 999288c Compare August 30, 2024 04:59

bardliao force-pushed the for-multi-lane branch from 999288c to 620edbe Compare September 2, 2024 11:39

ujfalusi reviewed Sep 4, 2024

View reviewed changes

bardliao force-pushed the for-multi-lane branch from 620edbe to 72fc8f1 Compare September 4, 2024 13:35

bardliao added 3 commits September 5, 2024 02:54

ASoC: rt722: get lane mapping property

1b15719

Rt722 supports multi-lane and the driver doesn't call sdw_slave_read_prop() to get all properties. Add sdw_slave_read_lane_mapping() to get the required lane mapping property. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the for-multi-lane branch from 72fc8f1 to 1b15719 Compare September 5, 2024 02:55

ujfalusi approved these changes Sep 5, 2024

View reviewed changes

plbossart approved these changes Sep 9, 2024

View reviewed changes

bardliao merged commit 99076b1 into thesofproject:topic/sof-dev Sep 9, 2024
10 of 14 checks passed

soundwire: add multi-lane support #4733

soundwire: add multi-lane support #4733

Conversation

bardliao commented Dec 6, 2023

bardliao commented Dec 7, 2023

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 left a comment

Choose a reason for hiding this comment

bardliao commented Aug 28, 2024

bardliao commented Aug 29, 2024

bardliao commented Aug 29, 2024

ranj063 commented Aug 29, 2024

bardliao commented Aug 30, 2024

bardliao commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment