ASoC: SOF: use FW based aggregation on ACE2.x by bardliao · Pull Request #4790 · thesofproject/linux

bardliao · 2024-01-19T05:20:52Z

To support FW based aggregation on ACE2.x.

…re DAIs" This reverts commit 699e146. Don't reset device_count as we will use the multi-gateway firmware configuration. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

plbossart

Looks mostly good @bardliao but the description of a 'device' is missing, and combining the lengths of two or more TLVs is unusual.

sound/soc/sof/ipc4-topology.c

sound/soc/sof/intel/hda-dai.c

plbossart · 2024-01-19T09:21:19Z

sound/soc/sof/ipc4-topology.c

 				else
 					blob->alh_cfg.mapping[i].channel_mask = mask << (step * i);

+				/* Copy device data to dma_cfg */


again what is a 'device'?

sound/soc/sof/intel/hda-dai-ops.c

sound/soc/sof/ipc4-topology.c

Each stream needs a dma_config_tlv. We will handle multi dma_config_tlv in the follow up commits. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

sound/soc/sof/intel/hda-dai.c

plbossart · 2024-01-26T12:03:27Z

sound/soc/sof/ipc4-topology.c

+				 */
+				dma_config = &ipc4_copier->dma_config_tlv[i].dma_config;
+				dma_config->dma_stream_channel_map.mapping[0].device =
+					blob->alh_cfg.mapping[i].device;


I keep repeating the same question "what is a device" in this context?

It's too cryptic and can be explained better.

@plbossart Currently, the device = node_id which is node_type | stream_tag - 1. But, it doesn't have to be stream_tag - 1. The only rule is that the device is unique for each stream and should be identical between dma_config and ALH blob. @iganakov Can you advice what device should be?

The comments in the firmware code seem to suggest it was link_id | pdi_id?

I'd really like to have a clear documented description of what this argument needs to be, otherwise this could be fun with breakages at any time.

The comments in the firmware code seem to suggest it was link_id | pdi_id?

Sorry, I didn't see that comment. And I notice that the dai_index is stream_tag. thesofproject/sof#8361 (comment)
Given that there is no ALH hardware on ACE2.x, I think it is more reasonable to use stream_tag?

BTW, I can add a commit to use link_id | pdi_id if we are all agree.

I'd really like to have a clear documented description of what this argument needs to be, otherwise this could be fun with breakages at any time.

Totally agree.

Can you check with @iganakov and document all this?

there's another comment which mentions alh_id

https://github.com/thesofproject/sof/pull/8361/files#diff-11acf7917806ca7b7727d4b030f8b7ff71c153d4e5b0b757d32e59ed2ea0ef37R1065

/* To be able to retrieve proper DMA config we need to check if * device_id value (which is alh_id) is equal to device_address. * They both contain SNDW master id and PDI. If they match then * proper config is found.

but it's unclear what the encoding might be...

Now that the fw part has been merged, we have a regression filed, so we need to progress on this PR and bring back the functionality.

there's another comment which mentions alh_id

https://github.com/thesofproject/sof/pull/8361/files#diff-11acf7917806ca7b7727d4b030f8b7ff71c153d4e5b0b757d32e59ed2ea0ef37R1065

/* To be able to retrieve proper DMA config we need to check if * device_id value (which is alh_id) is equal to device_address. * They both contain SNDW master id and PDI. If they match then * proper config is found.

but it's unclear what the encoding might be...

Now that the fw part has been merged, we have a regression filed, so we need to progress on this PR and bring back the functionality.

@plbossart I think alh_id here is referring to alh_blob->alh_cfg.mapping[i].alh_id. That is what I mentioned the device should be identical between dma_config and ALH blob. So, it is dma_cfg->channel_map.map[i].device_address = alh_blob->alh_cfg.mapping[i].alh_id in the firmware. And the device_address (alh_id) can be any number. @iganakov Please correct me if I am wrong.

@bardliao Yes, you're right, dma_cfg->channel_map.map[i].device_address = alh_blob->alh_cfg.mapping[i].alh_id. And from the parsing algorithm perspective device_address (alh_id) can be any number

@iganakov @bardliao can we try to align with the other OS that shall not be named so that we use the same convention for the device address. This is one of the cases where we don't want to be creative, using the same convention is useful when comparing IPC payloads.

sound/soc/sof/ipc4-topology.c

sound/soc/sof/intel/hda-dai-ops.c

sound/soc/sof/intel/hda-dai.c

plbossart

Looks good to me @bardliao, we just need to agree on the device_id and we can merge this. Woohoo.

We always use the lowest N channels of stream. So, set ch_mask to GENMASK(params_channels(params) - 1, 0). Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

sof_ipc4_dma_config_tlv{} is required for ACE2.x. The patch follow the convention to set the dma_stream_channel_map.mapping device as "link_id << 8 | pdi_id". And the mapping in sof_ipc4_alh_configuration_blob{} should be the same as dma_stream_channel_map.mapping in sof_ipc4_dma_config{}. The purposes of device id is to map DMA tlv. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

…for first CPU DAI" This reverts commit f8ba62a. The SoundWire aggregated solution was to use one DMA on multiple links. But, the solution changed to use one DMA for each link. It means that we should assign HDaudio stream_tag for each cpu_dai. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Set ipc4_copier->data.gtw_cfg.config_length dynamically based on blob->alh_cfg.device_count to align with the other OS. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

This reverts commit 7f9512a. commit 7f9512a ("ASoC: SOF: Intel: hda-dai-ops: fix HDaudio link format") added up all DAIs channels for SoundWire aggregation mode. That was correct because we used a DMA for all DAIs. But, it is not valid anymore after we use a DMA for each DAI. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

plbossart

@iganakov still one open for you

plbossart · 2024-02-01T10:15:53Z

sound/soc/sof/ipc4-topology.c

+				if (ipc4_copier->dma_config_tlv[i].length) {
+					dma_config = &ipc4_copier->dma_config_tlv[i].dma_config;
+					blob->alh_cfg.mapping[i].device =
+						dma_config->dma_stream_channel_map.mapping[0].device;


I don't understand this part. Do we actually care about the alh_cfg mapping? It seems completely redundant.

@iganakov do you actually use this for the aggregation or do you rely on the DMA TLV only?

plbossart · 2024-02-01T10:17:47Z

sound/soc/sof/ipc4-topology.c

+
+				/*
+				 * The mapping[i] device in ALH blob should be the same as the
+				 * dma_config_tlv[i] mapping device if a dma_config_tlv is present.


Is this really a requirement? this isn't something we discussed? I don't mind setting it to the same value as the DMA config TLV if the goal was to align with the other OS, but I would be surprised if the firmware used this value.

plbossart · 2024-02-02T18:32:02Z

@lgirdwood @ujfalusi @bardliao I am going to merge this to unlock LNL tests and deal with the rest of the opens in a follow-up PR.

bardliao requested a review from plbossart January 19, 2024 05:20

bardliao force-pushed the lnl-aggregation branch 2 times, most recently from b57c1da to 2097aaa Compare January 19, 2024 07:19

Revert "ASoC: SOF: Intel: hda-dai-ops: reset device count for SoundWi…

7dc06d5

…re DAIs" This reverts commit 699e146. Don't reset device_count as we will use the multi-gateway firmware configuration. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

plbossart requested changes Jan 19, 2024

View reviewed changes

bardliao force-pushed the lnl-aggregation branch 4 times, most recently from e59fce2 to e6b9c0e Compare January 22, 2024 12:17

ASoC: SOF: make dma_config_tlv be an array

c6235d9

Each stream needs a dma_config_tlv. We will handle multi dma_config_tlv in the follow up commits. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao mentioned this pull request Jan 25, 2024

[BUG] LNL has FW reported error: 7 with multiple pipeline playback thesofproject/sof#8275

Closed

bardliao force-pushed the lnl-aggregation branch 2 times, most recently from db3f2c4 to caf006e Compare January 26, 2024 03:00

plbossart reviewed Jan 26, 2024

View reviewed changes

bardliao force-pushed the lnl-aggregation branch from 5b7f7fa to 1ce3baa Compare January 29, 2024 07:08

bardliao marked this pull request as ready for review January 29, 2024 11:51

bardliao requested review from dbaluta, kv2019i, lgirdwood, ranj063 and ujfalusi as code owners January 29, 2024 11:51

bardliao force-pushed the lnl-aggregation branch from 1ce3baa to ad66cb7 Compare January 29, 2024 12:20

plbossart mentioned this pull request Jan 30, 2024

[BUG] DSP panic on LNL-SDW thesofproject/sof#8816

Closed

bardliao force-pushed the lnl-aggregation branch 3 times, most recently from bc35eae to 07b8e7e Compare January 30, 2024 13:44

plbossart reviewed Jan 30, 2024

View reviewed changes

plbossart mentioned this pull request Jan 31, 2024

SoundWire: aggregation fixes #4800

Closed

ASoC: SOF: Intel: hda-dai: set lowest N bits in ch_mask

24170d1

We always use the lowest N channels of stream. So, set ch_mask to GENMASK(params_channels(params) - 1, 0). Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the lnl-aggregation branch from 07b8e7e to 6d516f8 Compare February 1, 2024 01:29

bardliao force-pushed the lnl-aggregation branch 2 times, most recently from e76ea13 to 9949349 Compare February 1, 2024 02:02

bardliao added 4 commits February 1, 2024 10:12

ASoC: SOF: ipc4-topology: set config_length based on device_count

21bb3b9

Set ipc4_copier->data.gtw_cfg.config_length dynamically based on blob->alh_cfg.device_count to align with the other OS. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

plbossart reviewed Feb 1, 2024

View reviewed changes

plbossart approved these changes Feb 2, 2024

View reviewed changes

plbossart merged commit 02866bb into thesofproject:topic/sof-dev Feb 2, 2024

Conversation

bardliao commented Jan 19, 2024

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Feb 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants