lnl: sndw: use HDA DMA to transfer stream data by iganakov · Pull Request #8361 · thesofproject/sof

iganakov · 2023-10-20T08:58:59Z

Since LNL SoundWire interface uses HDA DMA to transfer stream data.

This approach uses HDA dai and HDA DMA. No changes needed on Zephyr side.
Following solution involves FW based SoundWire link aggregation mechanism which
is also used on MTL for backward compatibility with Windows driver.

This is a working solution. It was tested in E2E scenarios with Windows driver.

plbossart · 2023-10-20T15:10:31Z

Erm, sorry, whaaaaat?

LNL aggregation already works without any firmware support. The aggregation is handled with the host driver programming the PCMsCM registers in the multi-link configuration. The firmware should only deal with ONE stream ID and does not need to know whether any aggregation is handled since the DMA hardware deals with aggregation.

Why is a firmware-based solution introduced now?

@iganakov @lgirdwood @mwasko @mmaka1 ?

plbossart

on top of my earlier comment that aggregation does not need any firmware support, the change is inconsistent for the DMIC/SSP support that already works fine as well.

src/audio/copier/copier_dai.c

iganakov · 2023-10-23T07:36:24Z

@plbossart I've been anticipating such kind of questions that's why I submitted a draft :)
I didn't add any firmware-based aggregation I just used what was done for MTL. Firmware-based aggregation is needed for compatibility with Windows driver.

My intention was to use current FW infrastructure to enable SNDW on LNL using HDA DMA and since HDA dai doesn't program any hardware I decided to use it for SNDW configuration on SOF side.

lyakh · 2023-10-23T09:54:59Z

src/ipc/ipc4/helper.c

+			if (dma_cfg->channel_map.map[i].device_address == device_id) {
+				dai->host_dma_config[dma_cfg_count] = dma_cfg;
+				hit = true;
+				break;


please just do return IPC4_SUCCESS here and remove hit

lyakh · 2023-10-23T09:56:36Z

src/ipc/ipc4/helper.c

+	struct ipc_dma_config *dma_cfg;
+	int dma_cfg_count = 0;
+
+	while ((uint32_t)tlvs < end_addr) {


could do

for (tlvs = (struct sof_tlv *)data_buffer; (uint32_t)tlvs < end_addr; tlvs = tlv_next(tlvs))

lyakh · 2023-10-23T10:00:05Z

src/audio/copier/copier.c

+			goto error_cd;
+		}
+
+		cd->gtw_cfg = gtw_cfg;


I would assign only in success case - below memcpy_s() - to avoid a dangling pointer

lyakh · 2023-10-23T10:00:22Z

src/audio/copier/copier.c

+	 * config is used to assing dma_channel_id value.
+	 */
+	if (copier->gtw_cfg.config_length) {
+		gtw_cfg_size = (copier->gtw_cfg.config_length << 2);


superfluous parentheses

lyakh · 2023-10-23T10:01:08Z

src/audio/copier/copier.c

+
+		if (memcpy_s(cd->gtw_cfg, gtw_cfg_size,
+			     &copier->gtw_cfg.config_data, gtw_cfg_size) < 0) {
+			ret = -EINVAL;


propagate error

lyakh · 2023-10-23T10:05:30Z

src/audio/copier/copier.c

 	struct module_data *md = &mod->priv;
 	struct ipc4_copier_module_cfg *copier = (struct ipc4_copier_module_cfg *)md->cfg.init_data;
 	struct comp_ipc_config *config = &dev->ipc_config;
+	uint32_t *gtw_cfg;


lyakh · 2023-10-23T10:05:40Z

src/include/ipc4/copier.h

 	 * from the IPC data.
 	 */
 	struct ipc4_copier_module_cfg config;
+	uint32_t *gtw_cfg;


lyakh · 2023-10-23T10:06:19Z

src/audio/copier/copier_dai.c

 }

+static int copier_alh_assign_dai_index(struct comp_dev *dev,
+				       uint32_t *gtw_cfg_data,


void * and remove the cast in line 77

lyakh · 2023-10-23T10:21:57Z

src/ipc/ipc4/dai.c

+			}
+
+			const struct sof_alh_configuration_blob *alh_blob =
+				(const struct sof_alh_configuration_blob *)cd->gtw_cfg;


this type-cast would go too

lyakh · 2023-11-02T09:18:37Z

src/ipc/ipc4/helper.c

+
+	for (tlvs = (struct sof_tlv *)data_buffer; (uint32_t)tlvs < end_addr;
+	     tlvs = tlv_next(tlvs)) {
+		dma_cfg = (struct ipc_dma_config *)tlv_value_ptr_get(


no need to type-cast a void * pointer to another pointer type

lyakh · 2023-11-02T09:23:01Z

src/ipc/ipc4/helper.c

+
+		for (uint32_t i = 0; i < dma_cfg->channel_map.device_count; i++) {
+			if (dma_cfg->channel_map.map[i].device_address == device_id) {
+				dai->host_dma_config[dma_cfg_count] = dma_cfg;


this is a somewhat strange counter: dma_cfg_count how many valid (with a non-NULL dma_cfg pointer) didn't match the requested device and then assigns to an array with that index... The connection isn't obvious. Could you clarify?

device_id corresponds to SNDW master id (alh_id or channel_map.map[i].device_address) in gtw_cfg and it is not necessary match gtw_cfg number in Copier module blob. E.g. driver could send a request to configure aggregation on SNDW master 1 (alh_id=1) and 3 (alh_id=3) then two gtw_cfg will be appended to the init instance IPC and will receive 0 and 1 indexes in this loop. To put them one by one in host_dma_config[] I added an additional counter.

that is absolutely not what we discussed with @mmaka1. device_address was supposed to be the PCMsCMy FIFO address.

Yes, device_address should be PCMsCMy, but Windows drv always set this value and I need to deal with it.

I am still lost by all this.

what exactly is this 'device_id'

a) the link_id as I can read in the November 3 comment
b) the address of the PCMsCMy register
c) the PDI index written in PCMsCMy?

Also we all agree that the firmware does not program the PCMsCMy register, so what exactly does the firmware do with the device_id.

How many engineers does it take to define what a 'device' is?

Two additional questions: what happens if we do

One additional question: the 'device_id' aka PDI ID is specific to a link. The DMA configuration is not link specific, we can use whatever stream_tag we want.

So how do we know which PDI

lyakh · 2023-11-02T09:37:38Z

src/audio/copier/copier_dai.c

+		}
+		dai_index[0] = dai->host_dma_config[0]->stream_id;
+
+		return ret;


here it's always return 0

lyakh · 2023-11-02T09:45:29Z

src/ipc/ipc4/dai.c

+
+			if (!cd->gtw_cfg) {
+				comp_err(dev, "No gateway config found!");
+				return channel;


I think it would be more obvious to have return DMA_CHAN_INVALID here and move line 93 to around line 100 below

I agree, fixed

RanderWang

Looks good to me. BTW we support llp update with GP-DMA, how about HD-DMA ?

lgirdwood · 2023-11-10T17:13:53Z

@plbossart @iganakov do you guys have alignment now or still opens around the latest patch updates ?

plbossart · 2023-11-10T17:54:38Z

@plbossart @iganakov do you guys have alignment now or still opens around the latest patch updates ?

No alignment, there's no clarity on what exactly the gateway configuration should be. Both @bardliao and I looked into this - no reply on our comments so far.

lyakh · 2023-11-13T08:40:26Z

src/audio/copier/copier.c

+	if (copier->gtw_cfg.config_length) {
+		gtw_cfg_size = copier->gtw_cfg.config_length << 2;
+		gtw_cfg = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM,
+				  gtw_cfg_size);


no need for rzalloc() - it's completely overwritten 6 lines below. Just use rmalloc()

Fixed in PR #8574

lyakh · 2023-11-13T08:47:29Z

src/audio/copier/copier_dai.c

-		 *     }
-		 *   }
-		 * }
-		 */


is this... description not valid any more? I'm not entirely sure what it meant, but some documentation is often better than no documentation

This comment belongs to struct ipc4_copier_gateway_cfg for ALH node_id. This structure is well commented in alh.h an copier.h headers. So I think it's not necessary to have it here.

iganakov · 2023-11-14T09:32:16Z

@lgirdwood @plbossart sorry guys I was on a sick leave. I'm in touch with @bardliao we're making some additional experiments

kv2019i

Hmm. A generic concern is that this is exploding amount of seemingly Intel specific code in generic sof/src/audio code. This is hinting the split of what is done in Zephyr versus SOF is not right with the DAI interfaces, if we need to do this much platform specific ifdefs in generic SOF code. Many bits have existed before this PR, but this PR does double/triple the size.

kv2019i · 2023-11-14T16:56:58Z

src/ipc/ipc4/helper.c

+int ipc4_find_dma_config_multiple(struct ipc_config_dai *dai, uint8_t *data_buffer,
+				  uint32_t size, uint32_t device_id)
+{
+#if defined(CONFIG_ACE_VERSION_2_0)


This is very Intel specific code in a vendor-neutral file. At least a todo marker is needed that this need to be a generic option that can be enabled for multiple platforms (one such being Intel ACE2.0 and newer).

@kv2019i I found a way to make code more generic, please check a new version.

kv2019i · 2023-11-14T17:12:23Z

src/audio/copier/copier_dai.c

+		return -EINVAL;
+	}
+
+#if defined(CONFIG_ACE_VERSION_2_0)


Here again, we have very Intel specific code in copier_dai.c which would need to be generic. We've had DAI type specific code segments before in src/audio , but this is expanding the amount of code quite significantly.

I transformed this code to be generic one

lgirdwood · 2023-11-17T12:58:24Z

@iganakov thinking aloud, do we need a new boot ID flag to indicate the presence of this feature i.e. supported modes ?

iganakov · 2023-11-29T08:14:26Z

@lgirdwood We have internal discussions about common blob format for Linux and Windows. I'll update this PR when proper solution will appear.

lgirdwood · 2023-12-20T14:32:49Z

Adding @bardliao as IIUC he has to comment on using dynamic array for blob data impact for MTL unless no impact for SDW blob data usage for this PR ? @plbossart @iganakov

My test PR is thesofproject/linux#4752. I tested it on my Dell MTL laptop, and no regression found. PR test result also looks good.

Great - @plbossart what are the next steps now ?

plbossart · 2023-12-20T18:00:52Z

Just to be clear, what tests did you run @bardliao?

we have 3 possible configurations:

a) existing firmware with thesofproject/linux#4752
b) firmware with this PR with existing kernel
c) firmware with this PR and with thesofproject/linux#4752

I am mostly concerned about a) since that's a clear example of "we don't break user setups".

plbossart

I don't really have any comments on the code proper, but there's no description of what it does and what the TLV needs to be.

plbossart · 2023-12-20T18:09:59Z

src/ipc/ipc4/helper.c

+
+		for (uint32_t i = 0; i < dma_cfg->channel_map.device_count; i++) {
+			if (dma_cfg->channel_map.map[i].device_address == device_id) {
+				dai->host_dma_config[dma_cfg_count] = dma_cfg;


I am still lost by all this.

what exactly is this 'device_id'

a) the link_id as I can read in the November 3 comment
b) the address of the PCMsCMy register
c) the PDI index written in PCMsCMy?

Also we all agree that the firmware does not program the PCMsCMy register, so what exactly does the firmware do with the device_id.

How many engineers does it take to define what a 'device' is?

Two additional questions: what happens if we do

One additional question: the 'device_id' aka PDI ID is specific to a link. The DMA configuration is not link specific, we can use whatever stream_tag we want.

So how do we know which PDI

bardliao · 2023-12-21T12:00:08Z

Just to be clear, what tests did you run @bardliao?

we have 3 possible configurations:

a) existing firmware with thesofproject/linux#4752 b) firmware with this PR with existing kernel c) firmware with this PR and with thesofproject/linux#4752

I am mostly concerned about a) since that's a clear example of "we don't break user setups".

I tested a) The existing firmware (v2.7) with thesofproject/linux#4752

bardliao · 2024-01-03T14:07:21Z

src/ipc/ipc4/helper.c

+			}
+		}
+
+		dma_cfg_count++;


@iganakov We should only do dma_cfg_count++ when we assign dai->host_dma_config[dma_cfg_count] = dma_cfg;.
IMHO, dma_cfg_count should be an argument of ipc4_find_dma_config_multiple not a local variable. ipc4_find_dma_config_multiple will return immediately when it find the dma config with the device_id. We need to specify dai->host_dma_config[] index from the caller.
I do meet an issue if the dma config is not found in the first tlv. In that case, we don't assign dai->host_dma_config[dma_cfg_count] = dma_cfg; in the first for (uint32_t i = 0; i < dma_cfg->channel_map.device_count; i++) loop, and dma_cfg_count++. As a result, dai->host_dma_config[0] = NULL and dma_cfg is assigned to dai->host_dma_config[1] that is not what we want.

Good catch. I'll change ipc4_find_dma_config_multiple() in next revision of this patch. Thanks @bardliao

lgirdwood · 2024-01-17T10:40:59Z

@iganakov @bardliao any update ? Do we have any working updates now ?

iganakov · 2024-01-17T16:33:35Z

@lgirdwood I prepared some minor improvements and will update this PR soon. As I know @bardliao modified Linux blob and is able to use FW aggregation for playback but still has some issues with capture.

lgirdwood · 2024-01-24T14:55:00Z

@bardliao any update for capture ?
@plbossart blob format good for you ?

plbossart

I still have objections on the clarity of the code

What is a device?
Can we clarify the design and why a DMA 'stream_id' becomes a 'dai_index'

when it looks confusing it's awful to maintain...

src/ipc/ipc4/helper.c

src/ipc/ipc4/dai.c

src/ipc/ipc4/helper.c

src/audio/copier/copier_dai.c

bardliao · 2024-01-25T07:02:53Z

@bardliao any update for capture ?

I am still struggling with aggregation capture. I always get pcm IO error even with exact the same blob that was verified with slim driver.

src/ipc/ipc4/helper.c

Add function which retrieves pointer to the TLV structure value of the specified type Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>

Add host_dma_config array to be able to keep multiple DMA tlv pointers. This change is needed to enable SNDW FW aggregation using HD-A DMA on LNL Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>

Add helper function to parse muiltiple DMA config tlv structures added to copier Init Instance IPC in case of SNDW FW aggregation. To be able to find correct config we need to iterate over the sequence of DMA tlv with the same tlv type. Thus, we need to check if device_address value (which contains PDI) is equal to device_id parameter (passed with alh_id value). Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>

plbossart

@bardliao must approve this one.
I would still add a comment, see below.

src/audio/copier/copier_dai.c

Since LNL soundwire uses HD-A DMA to transfer data. Add LNL specific configuration to select HD-A DMA in case of SoundWire audio interface. Refactor copier dai code for sndw/alh node id type. Add support for sndw link aggregation mode for LNL platform based on DMA config being sent during Copier Init Instance IPC. Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>

plbossart

Thanks @iganakov

marc-hb · 2024-01-31T18:33:37Z

Causing regression:

[BUG] DSP panic on LNL-SDW #8816

plbossart requested changes Oct 20, 2023

View reviewed changes

src/audio/copier/copier_dai.c Show resolved Hide resolved

lyakh reviewed Oct 23, 2023

View reviewed changes

iganakov force-pushed the iganakov/sndw_use_hda_dma branch 2 times, most recently from b31b017 to 3bb6ca5 Compare November 1, 2023 10:45

lyakh reviewed Nov 2, 2023

View reviewed changes

iganakov force-pushed the iganakov/sndw_use_hda_dma branch from 3bb6ca5 to b615983 Compare November 3, 2023 09:19

iganakov marked this pull request as ready for review November 7, 2023 12:14

iganakov requested review from RanderWang, abonislawski, dbaluta, kv2019i, lbetlej, lgirdwood, marcinszkudlinski, mmaka1 and pblaszko as code owners November 7, 2023 12:14

RanderWang reviewed Nov 8, 2023

View reviewed changes

abonislawski requested a review from jxstelter November 8, 2023 08:01

lyakh reviewed Nov 13, 2023

View reviewed changes

lgirdwood added this to the v2.9 milestone Nov 13, 2023

kv2019i reviewed Nov 14, 2023

View reviewed changes

softwarecki mentioned this pull request Nov 16, 2023

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

Closed

plbossart mentioned this pull request Nov 30, 2023

copier: store gateway config in copier_data #8558

Closed

plbossart reviewed Dec 20, 2023

View reviewed changes

bardliao reviewed Jan 3, 2024

View reviewed changes

bardliao mentioned this pull request Jan 5, 2024

[BUG][LNL] Speaker playback start to distort in the middle of playing thesofproject/linux#4754

Closed

iganakov force-pushed the iganakov/sndw_use_hda_dma branch from a8b32b1 to 5c9fe39 Compare January 19, 2024 15:38

plbossart requested changes Jan 24, 2024

View reviewed changes

src/ipc/ipc4/helper.c Show resolved Hide resolved

src/ipc/ipc4/dai.c Show resolved Hide resolved

src/ipc/ipc4/helper.c Show resolved Hide resolved

src/audio/copier/copier_dai.c Show resolved Hide resolved

tmleman requested changes Jan 25, 2024

View reviewed changes

src/ipc/ipc4/helper.c Show resolved Hide resolved

iganakov added 3 commits January 26, 2024 13:27

tlv: add tlv_value_ptr_get() function

6d952e5

Add function which retrieves pointer to the TLV structure value of the specified type Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>

ipc4: add host_dma_config array

791c163

Add host_dma_config array to be able to keep multiple DMA tlv pointers. This change is needed to enable SNDW FW aggregation using HD-A DMA on LNL Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>

iganakov force-pushed the iganakov/sndw_use_hda_dma branch from 5c9fe39 to 37e76e8 Compare January 26, 2024 14:09

iganakov requested review from plbossart and tmleman January 26, 2024 14:09

plbossart approved these changes Jan 26, 2024

View reviewed changes

src/audio/copier/copier_dai.c Show resolved Hide resolved

tmleman approved these changes Jan 26, 2024

View reviewed changes

lgirdwood approved these changes Jan 26, 2024

View reviewed changes

iganakov force-pushed the iganakov/sndw_use_hda_dma branch from 37e76e8 to 3787071 Compare January 26, 2024 15:43

bardliao approved these changes Jan 29, 2024

View reviewed changes

dnikodem approved these changes Jan 29, 2024

View reviewed changes

plbossart approved these changes Jan 29, 2024

View reviewed changes

kv2019i merged commit a1deb74 into thesofproject:main Jan 29, 2024

bardliao mentioned this pull request Jan 30, 2024

ASoC: SOF: use FW based aggregation on ACE2.x thesofproject/linux#4790

Merged

Conversation

iganakov commented Oct 20, 2023

Uh oh!

plbossart commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iganakov commented Oct 23, 2023

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Nov 10, 2023

Uh oh!

plbossart commented Nov 10, 2023

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!

iganakov commented Nov 14, 2023

Uh oh!

kv2019i 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Oct 20, 2023 •

edited

Loading