soundwire: stream: skip m_rt which is not in preparing stage #5246

bardliao · 2024-11-19T03:50:52Z

sdw_compute_group_params() will count payload bandwidth and it count all m_rt in the bus. The m_rt will be added to the bus by sdw_stream_add_master(). If 2 streams are opened simultaneously, the m_rt of the second stream will be counted when
sdw_compute_group_params() is counting the payload bandwidth of the first stream. Which is incorrect.
Set PREPARED state earlier so that sdw_compute_group_params() can skip the m_rt that is not in the preparing stage yet.

ujfalusi · 2024-11-19T07:30:44Z

drivers/soundwire/stream.c

@@ -1456,15 +1464,15 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
 		if (ret < 0) {
 			dev_err(bus->dev, "Prepare port(s) failed ret = %d\n",
 				ret);
+			stream->state = state;
 			return ret;


I wonder why this is not jumping to restore_params?

We didn't change bus->params at this point yet.

ujfalusi · 2024-11-19T07:35:42Z

drivers/soundwire/stream.c

+	 * the stream that is CONFIGURED and PREPARED and skip the stream that is CONFIGURED
+	 * but not in the preparing stage.
+	 */
+	stream->state = SDW_STREAM_PREPARED;


I think in other patches we also moved the set to PREPARED state prior that we actually prepare things.

I got the reason and at the same time it sounds wrong to set something as PREPARED when it is not...

I agree that the stream->state may not represent the actual state perfectly. But, I can't find a better way to skip the m_rt that should not be counted yet. And stream->state is used by the stream helpers only and the helpers will be called one by one. So, I think it is safe to change the stream->state earlier in the same stream helper.

@bardliao this really seems like a hack or a workaround. Maybe the best thing to do would be to revisit the state machine so that we can gracefully skip the runtime thats not ready to be included yet?

ranj063 · 2024-11-20T18:00:48Z

drivers/soundwire/generic_bandwidth_allocation.c

@@ -189,6 +189,9 @@ static int sdw_compute_group_params(struct sdw_bus *bus,
 	}

 	list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
+		if (m_rt->stream->state < SDW_STREAM_PREPARED)


I think the issue manifests because sdw_prepare_stream() looks at the m_rt associated with the stream but sdw_compute_group_params() looks at the bus->m_rt_list. Can you maybe filter this list in this function by passing the stream as an argument when called from sdw_prepare_stream()?

I thought about that. sdw_compute_group_params() lools at the bus->m_rt_list is because it needs to calculate all the active stream's bandwidth in the bus. We assume that all m_rt in the bus are active, but that is not true. And we did similar change in d7722a643421. That's why I choose to set the PREPARE state earlier.
But I am fine with the passing stream solution, too. PR updated.

include/linux/soundwire/sdw.h

drivers/soundwire/generic_bandwidth_allocation.c

ranj063 · 2024-11-21T17:05:13Z

drivers/soundwire/generic_bandwidth_allocation.c

+			if (stream->state != SDW_STREAM_CONFIGURED)
+				continue;
+		} else {
+			if (m_rt->stream->state != SDW_STREAM_ENABLED &&


and this is not intuitive at all. What are you trying to say here? Ignore a runtime thats not associated with the stream if it has already been activated once?

No, to the opposite. Only calculate the bandwidth of the active streams, and add the bandwidth of the current stream.

ok then the check for != ENABLED makes sense. but why also check for != DISABLED? Is this the case with open but paused streams? Anyway, I think this needs a comment here with the explanation. And the commit message needs to be modified too based on whatever explanation you add here.

Yes, DISABLED is about paused. Please see https://github.com/thesofproject/linux/blob/topic/sof-dev/Documentation/driver-api/soundwire/stream.rst#soundwire-stream-states and https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sdw_utils/soc_sdw_utils.c#L809

ranj063 · 2024-11-23T02:16:17Z

drivers/soundwire/generic_bandwidth_allocation.c

+		if (m_rt->stream == stream) {
+			if (stream->state != SDW_STREAM_CONFIGURED)
+				continue;
+		} else {


no need for an else after a continue btw

ranj063 · 2024-11-25T16:19:33Z

drivers/soundwire/generic_bandwidth_allocation.c

@@ -190,6 +190,21 @@ static int sdw_compute_group_params(struct sdw_bus *bus,
 	}

 	list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
+		/*
+		 * sdw_compute_group_params could be called in the deprepared case, skip the
+		 * m_rt in that case.


a better comment would be to include the runtime only during prepare

ranj063 · 2024-11-25T16:23:43Z

drivers/soundwire/generic_bandwidth_allocation.c

+				continue;
+		/*
+		 * Stream stage ENABLED and DISABLED are between PREPARED and DEPREPARED,
+		 * We should count the bandwidth only for ENABLED and DISABLED stages.


@bardliao I think looking at the enum definition, its easy to see this. A better explanation would be something like "Include runtimes with running (ENABLED state) and paused (DISABLED state) streams"

Both updated.

ranj063 · 2024-11-26T18:57:33Z

@bardliao there's a build error in qcom.c. You'll need to fix it too because you changed the signature

The stream parameter will be used in the follow up commit. No function change. Signed-off-by: Bard Liao <[email protected]>

…e streams only sdw_compute_group_params() should only count payload bandwidth of the active streams which is in the ENABLED and DISABLED state in the bus. And add the payload bandwidth of the stream that calls sdw_compute_group_params() in sdw_prepare_stream(). Signed-off-by: Bard Liao <[email protected]>

bardliao · 2024-11-27T05:40:53Z

@bardliao there's a build error in qcom.c. You'll need to fix it too because you changed the signature

Thanks for pointing this out. Fix it now.

bardliao requested review from plbossart, ranj063 and ujfalusi November 19, 2024 03:50

ujfalusi reviewed Nov 19, 2024

View reviewed changes

ranj063 reviewed Nov 20, 2024

View reviewed changes

bardliao force-pushed the fix-multilane branch 2 times, most recently from 819dda7 to 6628b7a Compare November 21, 2024 14:24

ranj063 reviewed Nov 21, 2024

View reviewed changes

include/linux/soundwire/sdw.h Show resolved Hide resolved

ranj063 reviewed Nov 21, 2024

View reviewed changes

bardliao force-pushed the fix-multilane branch from 6628b7a to 82126db Compare November 22, 2024 09:03

ranj063 reviewed Nov 23, 2024

View reviewed changes

bardliao force-pushed the fix-multilane branch from 82126db to 5075d87 Compare November 25, 2024 13:29

ranj063 reviewed Nov 25, 2024

View reviewed changes

bardliao force-pushed the fix-multilane branch from 5075d87 to 904a483 Compare November 26, 2024 03:44

ranj063 previously approved these changes Nov 26, 2024

View reviewed changes

ujfalusi previously approved these changes Nov 26, 2024

View reviewed changes

bardliao added 2 commits November 27, 2024 13:34

SoundWire: pass stream to compute_params()

b1cbc68

The stream parameter will be used in the follow up commit. No function change. Signed-off-by: Bard Liao <[email protected]>

bardliao dismissed stale reviews from ujfalusi and ranj063 via 924e9f9 November 27, 2024 05:38

bardliao force-pushed the fix-multilane branch from 904a483 to 924e9f9 Compare November 27, 2024 05:38

ranj063 approved these changes Nov 27, 2024

View reviewed changes

ranj063 requested a review from ujfalusi November 27, 2024 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

soundwire: stream: skip m_rt which is not in preparing stage #5246

soundwire: stream: skip m_rt which is not in preparing stage #5246

bardliao commented Nov 19, 2024

ujfalusi Nov 19, 2024

bardliao Nov 19, 2024

ujfalusi Nov 19, 2024

bardliao Nov 19, 2024

ranj063 Nov 20, 2024

ranj063 Nov 20, 2024 •

edited

Loading

bardliao Nov 21, 2024

ranj063 Nov 21, 2024

bardliao Nov 22, 2024

ranj063 Nov 22, 2024

bardliao Nov 25, 2024

ranj063 Nov 23, 2024

ranj063 Nov 25, 2024

ranj063 Nov 25, 2024

bardliao Nov 26, 2024

ranj063 commented Nov 26, 2024

bardliao commented Nov 27, 2024

soundwire: stream: skip m_rt which is not in preparing stage #5246

Are you sure you want to change the base?

soundwire: stream: skip m_rt which is not in preparing stage #5246

Conversation

bardliao commented Nov 19, 2024

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 Nov 20, 2024 • edited Loading

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 commented Nov 26, 2024

bardliao commented Nov 27, 2024

ranj063 Nov 20, 2024 •

edited

Loading