optimize copy times and remove dai device from copier by btian1 · Pull Request #7229 · thesofproject/sof

btian1 · 2023-03-08T05:44:42Z

After SOF upgraded to IPC4, copier actually created 2 devices for
data input(capture) and output(playback), it is
dai and copier,
hence there are two times copy from DMA buffer
to endpoint buffer then to component buffer.

This PR intend to remove dai device and one more time copy
through use dai data to handle host interface calling.

Basically, there are several steps to go there:

Add dai data to copier data structure.
Share dai data structure to copier by add a dai copier common header file.
In copier, all dai related ops calling should be replaced with new dai exposed interface.
Remove one more time copy and dai endpoint buffer.
Remove dai device.
Performance data:

mcps, saved 1.6mcps, 5.4 --> 3.8, around 30% saved for copier.
Before: 0x40001 perf comp_copy samples 48 period 1000 cpu avg 533 peak 553
After: 0x40001 perf comp_copy samples 48 period 1000 cpu avg 378 peak 397

memory, since host endpoint buffer and host device got removed, for each copier,
16bit/32bit 384/768 bytes saved for buffer, 112 saved for dai device.
As a summary, 16/32 bit playback/capture totally saved 496/884 bytes for each dai copier usage.
For the cases that have more than 10 copier usage, memory saving is quite remarkable.

Note：multiple sink dai copier is not tested and will be fixed in follow up PR.

Signed-off-by: Baofeng Tian baofeng.tian@intel.com

gkbldcig · 2023-03-28T00:19:44Z

Can one of the admins verify this patch?

btian1 · 2023-03-29T01:15:47Z

Can one of the admins verify this patch?

this is still under development, please stay tuned.

lgirdwood

@juimonen @abonislawski @singalsu pls review

lgirdwood · 2023-04-07T13:28:40Z

src/ipc/ipc4/dai.c

I think we can probably make some of these light function static inline in the header.

agree, just because this is an formal interface for driver, if move ipc4 to inline, then legacy part also need inline, it is not a performance function, so overhead is not a problem.

lgirdwood · 2023-04-07T13:29:10Z

src/audio/copier/copier.c

Needs some inline commenst to explain what this is doing.

lgirdwood · 2023-04-07T13:29:59Z

src/audio/dai-zephyr.c

this could be an inline in the header.

it is part of drv interface, and copier actually does not use dai_copy, it is using dai_zephyr_copy.

lgirdwood · 2023-04-07T13:32:39Z

mcps, saved 1.6mcps, 5.4 --> 3.8, around 30% saved for copier.
Before: 0x40001 perf comp_copy samples 48 period 1000 cpu avg 533 peak 553
After: 0x40001 perf comp_copy samples 48 period 1000 cpu avg 378 peak 397

@btian1 this is a good saving for link copier. Good work.

btian1 · 2023-04-10T02:28:48Z

pier. Good work.

this is with log print data, actual saving for MCPS should be larger, due to zephyr log print always have impact on performance measure.

RanderWang

why do we need to create endpoint buffer for dai ? when copier is bound to another module, the buffer is created. Dai can reuse copier buffer. please check host part.

RanderWang · 2023-04-10T07:12:55Z

src/audio/dai-zephyr.c

return -ENODEV; error label does nothing now

src/audio/dai-zephyr.c

lyakh · 2023-04-12T10:34:34Z

src/audio/copier/copier.c

else if. This time cd->ipc_gtw is unimportant? Or you could use switch (dev->ipc_config.type)

config->type = SOF_COMP_HOST;

gtw assigned type as HOST, so for DAI case, there is no possible with ipc_gtw == true.

I think this would look better:

switch (dev->ipc_config.type) { case SOF_COMP_HOST: if (!cd->ipc_gtw) { host_zephyr_free(cd->hd); rfree(cd->hd); } break; case SOF_COMP_DAI: ai_zephyr_free(cd->dd, cd->endpoint[0]); rfree(cd->endpoint[0]); buffer_free(cd->endpoint_buffer[0]); }

similar in other cases

yes, I will change to this style, previously focused on resolve multisink and other CI related errors.

src/audio/dai-zephyr.c

lyakh · 2023-04-12T10:39:42Z

src/audio/copier/copier.c

switch will cover 3 cases, host/dai/gtw, anything not covered?

lyakh · 2023-04-12T10:44:11Z

src/audio/copier/copier.c

switch ()

lyakh · 2023-04-12T10:45:41Z

src/audio/copier/copier.c

switch ()

lyakh · 2023-04-12T10:49:50Z

src/audio/copier/copier.c

is this related?

already merged PR: #7303

lyakh · 2023-04-12T10:51:52Z

src/audio/copier/copier.c

space missing

lyakh · 2023-04-12T10:52:46Z

src/audio/copier/copier.c

the indentation was correct

ok, changed aligned back.

lyakh · 2023-04-12T10:53:10Z

src/audio/copier/copier.c

actually both these lines aren't needed

lyakh · 2023-04-12T10:53:46Z

src/audio/copier/copier.c

switch ()

serhiy-katsyuba-intel · 2023-04-24T10:28:21Z

src/audio/dai-zephyr.c

It looks like this "else" processing here is unnecessary. In case of capture, dev->bsink_list should not be empty but should contain one or more regular sink buffers. That case is already handled below in a loop.

keep it for now, I tried with remove this part, I saw some input/output error with capture:
arecord: pcm_read:2221: read error: Input/output error

ranj063

I'm going to stop at this commit @btian1. I think you need to fix the multiple endpoint case right from the begininng and also add a common header

ranj063 · 2023-04-26T16:45:01Z

src/audio/copier/copier.c

is this even possible for a copier to not be of type host or DAI? If not, please remove the error

yes, this part should not be error, I observed NONE with endpoint_num = 0 case, left a warning is more suitable.

ranj063 · 2023-04-26T16:45:46Z

src/audio/dai-legacy.c

can we please leave dai-legacy alone and not touch it at all?

I also don't want to touch, but I remember dai is different, if not touch, I can't make CI pass.

will do it with a separate patch after this PR merged.

if DAI merge with copier is any similar to what previously was done to host, then we do need to touch legacy: 69deb6f

ranj063 · 2023-04-26T16:48:11Z

src/audio/dai-zephyr.c

we need a dai_put() here isnt it? I know it doesnt exist in the current code but can we please fix it?

ok, like new/free right? if dma_get failed, need dma_put.

yes, like new / free, that's why - no, not dma_put(), but dai_put(), because dai_get() was successful above

ranj063 · 2023-04-26T16:49:45Z

src/audio/dai-zephyr.c

this comment is confusing? Are you trying to say this needs to be changed? If so, can you adda TODO here?

should change or delete, it is a self-remind comments when I worked on it.

ranj063 · 2023-04-26T16:53:34Z

src/include/sof/audio/dai_copier.h

can we please drop the "through data_data" in all the comments. I think its pretty self-explanatory given that it is one of the args

do you mean delete those comments? please confirm, if so, I will drop.

ranj063 · 2023-04-26T17:51:47Z

src/audio/copier/copier.c

why are you not passing dev here instead of cd->endpoint[0]?

this is just change to zephyr reset, for replace, I made it in patch 11 together.

ranj063 · 2023-04-26T17:52:40Z

src/audio/copier/copier.c

and isn't this going to break multitple endpoint DAIs? why remove the loop?

endpoint_num always 1, right? is there a case for endpoint_num >= 2?

Yes, there is a case when endpoint_num is >= 2. It's called "multi-channel ALH" or "ALH multi-gateway" feature. Basically, there could be multiple ALH endpoints audio data from/to which should be multiplexed/demultiplexed into/from single stream. So, for example, stereo audio could come from two ALH gateways, one for left channel, second for right, and then that audio is multiplexed into single stereo stream. See mux_into_multi_endpoint_buffer() and demux_from_multi_endpoint_buffer().

ranj063 · 2023-04-26T17:57:59Z

src/include/sof/lib/dai-legacy.h

common header please her eand all other common function

ranj063 · 2023-04-26T18:37:40Z

src/audio/copier/copier.c

ahh i forgot a module copier is not of type DAI or HOST. Please remove all errors as there's just nothing to do for that case.

ok, remove error is fine, for moduler copier, what's the assigned type, or it does not need a type?

ranj063 · 2023-04-26T18:38:21Z

src/audio/copier/copier.c

why not dev instead of cd->endpoint[0]? and why remove the loop?

change dev with patch 11.

serhiy-katsyuba-intel · 2023-04-27T09:49:15Z

@btian1 , before merge could you ask @wszypelt to run a full scope of Windows MTL tests on firmware with this PR. Unfortunately, CI is only done on some subset of tests. After similar Host copy optimizations we had some glitches/distorted audio regressions. Full scope tests take several hours, I guess, that is why they are not all added to CI.

I'll put "request changes" just to block merge until @wszypelt is back with the tests results.

serhiy-katsyuba-intel

Blocking merge until @wszypelt gets back with full scope tests results.

wszypelt · 2023-04-27T10:12:11Z

@serhiy-katsyuba-intel @btian1 Sure thing, I can set up full scope, within 24 hours we should have test results for quick comparison
For now PR also blocks CI(Internal Intel CI System/merge/build— Tests failed ), unfortunately the sdw tests don't pass properly

Create and expose two functions for dai allocation and free along with struct dai_data,these functions will be shared by both dai and copier components. This is in preparation to remove the creation of dai component and endpoint buffer in init_dai(). Below is the preparation work: Add dai data structure to copier data structure Currently, copier call dai interface need through ops driver. However, with dedicated work, all dai interface can be called through dai_data, this commit add dai data into copier data. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Create and expose dai common functions for further copier usage. Those functions does not depend on dai device, and it can work with dai data directly, this PR is for DAI prepare zephyr usage. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Create and expose a couple of functions for host DMA reset along with struct dai_data,these functions will be shared by both dai and copier component. This is in preparation to remove the creation of dai component and endpoint buffer in init_dai(). Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Create and expose a couple of functions for dai comp trigger along with struct dai_data,these functions will be shared by both dai and copier component. This is in preparation to remove the creation of dai component and endpoint buffer in init_dai(). Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Implement dai data processed data and dai position in copier to avoid use dai ops driver call it. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

btian1 · 2023-04-28T13:02:03Z

unfortuna

please stay tuned, will continue on ww17, need more time to work on this PR.

Create and expose a couple of functions for dai parameters along with struct dai_data,these functions will be shared by both dai and copier component. This is in preparation to remove the creation of dai component and endpoint buffer in init_dai(). Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Create and expose a couple of functions for dai DMA copy along with struct dai_data,these functions will be shared by both dai and copier component. This is in preparation to remove the creation of a dai component and endpoint buffer in creat_dai(). Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Dai get hw parameters only used for ipc3 case, there is no need keep it for dai zephyr. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Current code works with ipc3 only, for ipc4 with zephyr, current implementation will cause finding device is copier device instead of dai device, in order to fix this, all dai related zephyr api moved to asrc module, then dai device usage will be moved out for zephyr case. Also, with this change, asrc will have no dependency on dai zephyr. dai zephyr interface are fully aligned with module adapter. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

With this change, dai config was removed from dai zephyr driver. and instead of use dai device, dai config is using copier device. This is preparation for remove dai device. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Replace all dai device usage with copier device, also remove the converter function in dai_params, assign it in copier with initial converter function. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Current implementation only support one source and one sink buffer dai data copy, however, after remove endpoint buffer, there is a need to copy multiple times data from source buffer to multiple endpoint buffer, loop all the sink device and copy it, after all done, consume source buffer accordingly. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

btian1 force-pushed the daiopt branch 6 times, most recently from 3dedd75 to 5f93a1f Compare March 8, 2023 07:10

btian1 changed the title ~~[DONT REVIEW] DRAFT PR FOR DAI OPTIMIZATION~~ [DON'T REVIEW] DRAFT PR FOR DAI OPTIMIZATION Mar 8, 2023

btian1 force-pushed the daiopt branch 23 times, most recently from 0806524 to 0d5f6d0 Compare March 13, 2023 14:02

btian1 mentioned this pull request Mar 28, 2023

asrc: fix bug for ipc4 with zephyr case #7339

Closed

btian1 force-pushed the daiopt branch from 8b928b6 to 0b98092 Compare March 28, 2023 13:54

btian1 force-pushed the daiopt branch from d961c83 to 337c2c3 Compare March 29, 2023 13:27

lgirdwood reviewed Apr 7, 2023

View reviewed changes

RanderWang reviewed Apr 10, 2023

View reviewed changes

lyakh reviewed Apr 12, 2023

View reviewed changes

btian1 mentioned this pull request Apr 20, 2023

dai-zephyr: refactor dai params #7345

Closed

serhiy-katsyuba-intel reviewed Apr 24, 2023

View reviewed changes

ranj063 requested changes Apr 26, 2023

View reviewed changes

serhiy-katsyuba-intel requested changes Apr 27, 2023

View reviewed changes

btian1 added 5 commits April 28, 2023 20:55

copier: Implement dai processed data and position in copier

cefb3fb

Implement dai data processed data and dai position in copier to avoid use dai ops driver call it. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

btian1 added 7 commits April 28, 2023 21:10

dai-zephyr: remove dai get hw parameters

545e240

Dai get hw parameters only used for ipc3 case, there is no need keep it for dai zephyr. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

copier: remove dai device usage in copier

f5e0d11

Replace all dai device usage with copier device, also remove the converter function in dai_params, assign it in copier with initial converter function. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

ranj063 mentioned this pull request May 2, 2023

DAI copier optimization #7530

Merged

Conversation

btian1 commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkbldcig commented Mar 28, 2023

Uh oh!

btian1 commented Mar 29, 2023

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btian1 Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lgirdwood commented Apr 7, 2023

Uh oh!

btian1 commented Apr 10, 2023

Uh oh!

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

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!

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!

btian1 commented Mar 8, 2023 •

edited

Loading

btian1 Apr 25, 2023 •

edited

Loading