module adapter and copier performance optimization by btian1 · Pull Request #7909 · thesofproject/sof

btian1 · 2023-07-10T06:08:50Z

this PR did following things:
optimize module adapter and copier, reduce host and dai performance increase caused by #7890.
below is the rough result:

lyakh

how about single-line wrappers in src/audio/source_api_helper.c and src/audio/sink_api_helper.c - they're in .c files so they're never inlined and I think some of them might be called on hot paths.

lyakh · 2023-07-11T10:22:37Z

what about setting md->state?

seems md->state is for ipc3 special, and copier_process does not depend on this, so there is no error reported in CI and QB.

This is a change in behaviour. In other cases functions, that you inlined, really did nothing else apart from calling a method. And if it's really only needed for IPC3, removing those lines is more difficult to verify, because CI doesn't test IPC3 any more. It seems to me that md->state is something like a debugging flag, used to verify that we aren't running various operations in a wrong state, nothing else. Hitting such a wrong state would be an error, and I don't think we're hitting those errors ever, so maybe that .state could be removed completely.

I spent time to check, I also think this can be removed completely, let me submit a new patch to cover this.
Thanks, @lyakh

lyakh · 2023-07-11T10:28:39Z

this loses these potential errors, but yes, I can see, that possibly they aren't needed

yeah, I did not remove it totally, we can try to remove those checks later.
SOF worry too much about each possible error and this cause much more overhead in component.

@lyakh , I intend to remove this check, as other converter function does not have this check, what do you think？

lyakh · 2023-07-11T10:54:48Z

not related to this your PR and probably not relevant for performance, but could we make the code above more readable and less error prone by removing those mid-function jumps and rewriting it as

if (j >= IPC4_COPIER_MODULE_OUTPUT_PINS_COUNT) { comp_err(dev, "Sink queue ID: %d >= max output pin count: %d\n", j, IPC4_COPIER_MODULE_OUTPUT_PINS_COUNT); ret = -EINVAL; } else if (!converter[j]) { comp_err(dev, "No PCM converter for sink queue %d\n", j); ret = -EINVAL; } else if (sink_dev && sink_dev->state == COMP_STATE_ACTIVE) { ret = dma_buffer_copy_from_no_consume(dma_buf, sink_c, converter[j], bytes); }

Those code are added may be can resolve some CI or QB failures, @ranj063 may comment on this.

Actually not only this, there is a lot of code, like below in dai_common_copy:

/* Check possibility of glitch occurrence */ if (dev->direction == SOF_IPC_STREAM_PLAYBACK && copy_bytes + avail_bytes < dd->period_bytes) comp_warn(dev, "dai_common_copy(): Copy_bytes %d + avail bytes %d < period bytes %d, possible glitch", copy_bytes, avail_bytes, dd->period_bytes); else if (dev->direction == SOF_IPC_STREAM_CAPTURE && copy_bytes + free_bytes < dd->period_bytes) comp_warn(dev, "dai_common_copy(): Copy_bytes %d + free bytes %d < period bytes %d, possible glitch", copy_bytes, free_bytes, dd->period_bytes); /* return if nothing to copy */ if (!copy_bytes) { comp_warn(dev, "dai_zephyr_copy(): nothing to copy"); dma_reload(dd->chan->dma->z_dev, dd->chan->index, 0, 0, 0); return 0; }

I will try to remove or optimize those in next round of copier optimization.

@btian1 this is code carried over from the previous implementation when we did the optimization or conversion to module interface. If there's scope for clean up/further optimization lets do it.

we can try to do it next round.

btian1

inline is only used for time critical function, otherwise, it will bring extra cost in code size.

ranj063 · 2023-07-13T16:01:57Z

@btian1 can you please rebase

btian1

@kv2019i @lyakh @ranj063 @RanderWang , please continue review this PR.

btian1 · 2023-08-09T03:27:47Z

@lyakh , I intend to remove this check, as other converter function does not have this check, what do you think？

lgirdwood · 2023-08-09T13:29:39Z

+#if CONFIG_IPC_MAJOR_3
 		return module_process_legacy(mod, NULL, 0, NULL, 0);
+#else
+		return mod->priv.ops->process_audio_stream(mod, NULL, 0, NULL, 0);


This should be 0 cost with the staic inline in the header - I know we do check the ops != NULL, so that will add cost, but we shouldcheck all mandarory ops != NULL at init and check all optional ops != NULL at every usage.

inline module_process_legacy is not an option, due to it was called 3 times in module_adapter.c, should not inline it.
Also, not only function parameter take cycles, but also the "if/else if/else" and later "(ret && ret != -ENOSPC && ret != -ENODATA)", these are also expensive.

regarding add a check, this cost adding is quite transparent, it is acceptable, however, I do think:
https://github.com/thesofproject/sof/blob/main/src/audio/module_adapter/module_adapter.c#L1177
also did the check, right?

This can be optimised, the md->ops will be known at init time, we dont need to check this at process time to know which ops function to call.

The return value check can also be improved by only checking (ret < 0), we should not be filtering error numbers. Likewise modules should only return -ENODATA or -ENOSPC iff these are real errors that stop the module from processing any further data (without a reset).

int module_process_legacy(struct processing_module *mod, struct input_stream_buffer *input_buffers, int num_input_buffers, struct output_stream_buffer *output_buffers, int num_output_buffers) { struct comp_dev *dev = mod->dev; int ret; struct module_data *md = &mod->priv; comp_dbg(dev, "module_process_legacy() start"); #if CONFIG_IPC_MAJOR_3 if (md->state != MODULE_IDLE) { comp_err(dev, "module_process(): wrong state of comp_id %x, state %d", dev_comp_id(dev), md->state); return -EPERM; } /* set state to processing */ md->state = MODULE_PROCESSING; #endif if (md->ops->process_audio_stream) ret = md->ops->process_audio_stream(mod, input_buffers, num_input_buffers, output_buffers, num_output_buffers); else if (md->ops->process_raw_data) ret = md->ops->process_raw_data(mod, input_buffers, num_input_buffers, output_buffers, num_output_buffers); else ret = -EOPNOTSUPP; if (ret && ret != -ENOSPC && ret != -ENODATA) { comp_err(dev, "module_process() error %d: for comp %d", ret, dev_comp_id(dev)); return ret; } comp_dbg(dev, "module_process_legacy() done"); #if CONFIG_IPC_MAJOR_3 /* reset state to idle */ md->state = MODULE_IDLE; #endif return 0; }

and for

int module_adapter_copy(struct comp_dev *dev) { comp_dbg(dev, "module_adapter_copy(): start"); struct processing_module *mod = comp_get_drvdata(dev); if (IS_PROCESSING_MODE_AUDIO_STREAM(mod)) return module_adapter_audio_stream_type_copy(dev); if (IS_PROCESSING_MODE_RAW_DATA(mod)) return module_adapter_raw_data_type_copy(dev); if (IS_PROCESSING_MODE_SINK_SOURCE(mod)) return module_adapter_sink_source_copy(dev); comp_err(dev, "module_adapter_copy(): unknown processing_data_type"); return -EINVAL; }

This can be improved by moving the mode check to init time rather than process time.

module_init already checked:

/*check interface, there must be one and only one of processing procedure */ if (!interface->init || !interface->prepare || !interface->reset || !interface->free || (!!interface->process + !!interface->process_audio_stream + !!interface->process_raw_data != 1)) { comp_err(dev, "module_init(): comp %d is missing mandatory interfaces", dev_comp_id(dev)); return -EIO; }

However, we still need classify which process is valid during each processing:
process
process_raw_data
process_audio_stream

to avoid IS_PROCESSING_MODE_AUDIO_STREAM(mod), I need add a flag in mod to indicate which process is valid.
then use switch: module_adapter_copy, like below:

switch (mod->process_module) {
case 1:
process_audio_stream;
case 2:
process_raw_data;
case 3: process;
}

something like this, any more good suggestions?

do you think it is not allowed or don't want to add more #if IPC3?
Later, I want to submit a PR to separate module_adapter.c to ipc3/ipc4, then inside #if will be totally disappear.

Since we check for ops != NULL in init then I would expect

// header static inline int module_process(mod, blah, blah) { return mod->priv.ops->process_audio_stream(mod, NULL, 0, NULL, 0); } // C file #if CONFIG_IPC_MAJOR_3 return module_process_legacy(mod, NULL, 0, NULL, 0); #else return module_process(mod, NULL, 0, NULL, 0); #endif

lgirdwood · 2023-08-09T13:33:06Z


-int audio_stream_copy(const struct audio_stream __sparse_cache *source, uint32_t ioffset,
-		      struct audio_stream __sparse_cache *sink, uint32_t ooffset, uint32_t samples)
+void audio_stream_copy(const struct audio_stream __sparse_cache *source, uint32_t ioffset,


what about failures here ? i.e. some error occurs in a module during copy. modules should also be allowed to return 0 bytes.

current sof implementation does not have this consideration, if search audio_stream_copy, then you will find audio_stream_copy used as a void, like below:
https://github.com/thesofproject/sof/blob/main/src/audio/eq_iir/eq_iir.c#L309

Also, copy just did as source/sink/sample indicated, if 0, then means the input copy_size is zero.

RanderWang · 2023-08-11T02:41:18Z

 	buffer_release(source);

-	if (ret < 0)
-		return;


if ret < 0 we will not update stream position for kernel side, but now it is not correct

there is no chance to get ret < 0 case, so I removed the return.

lyakh · 2023-08-16T10:29:34Z

-	if (audio_stream_get_avail_samples(source) < samples + ioffset)
-		return -EINVAL;
-	if (audio_stream_get_free_samples(sink) < samples + ooffset)
-		return -EINVAL;


an example of where these PCM processing functions are called in

sof/src/lib/dma.c

Line 339 in 8f60180

ret = process(istream, 0, &sink->stream, 0, samples);

and it seems like there the check isn't complete - it gets available data from the source, but doesn't check whether the sink has sufficient free space. Not sure if this can ever become a problem (e.g. with multiple sources writing data to a single sink), but if that does have to be checked, maybe it has to be checked uniformly for all such calls.

@btian1 Can you comment?

I like "uniform", sof did a lot of check in many functions, hard to say those check are all must, especially those on critical timing path.

based on current sof architecture(2X period buffer size), source is checked by samples( as long as it is not zero).

for sink, I am not sure whether there is a case with multi src --> single sink, if there have, single sink buffer size still need 2X of total src, I mean, if sink have no space, there is pipeline overrun.

we can add sink check if you still feel not safe enough.

seems I reverted #3251 in this patch, @ktrzcinx , may I know any reason to add those checkers in patch #3251

@lgirdwood @lyakh @mmaka1 , if you think keep error handling is must for converter, I can remove this patch, and find other way to do optimization.

@btian1 before #3251 there already was this check as an assert(). The change was related with some bug which ends up with SOF crash so I've changed assert() to if() return to gain human readable log, in proper context about root cause of the issue in a similar cases in the future.

@ktrzcinx ， this part can be added back, I mean the whole #3251, in this patch, I almost reverted all.

kv2019i

I'll lift my request-changes. I have a couple of minor nitpicks left, but nothing to block merge. The changes to drop "int" returns still seem quite scary and it's not so obvious that we can live withotu the capabillity to return from errors, but your point this is not universally followed, does make sense, so I'm ok to proceed.

kv2019i · 2023-08-16T13:52:15Z

-	if (audio_stream_get_avail_samples(source) < samples + ioffset)
-		return -EINVAL;
-	if (audio_stream_get_free_samples(sink) < samples + ooffset)
-		return -EINVAL;


@btian1 Can you comment?

No blocking issues in PR anymore

lgirdwood

@btian1 lets merge inline Kconfig 1st then refactor this on top with ops accessors APIs and resubmit.

lgirdwood

@btian1 some of these patches can be merged now, you could split the PR so the patches with no comments can be merged.
I need to see the inline usage for merging the 1st patch as per my comment.

lgirdwood · 2023-08-21T15:26:15Z

+#if CONFIG_IPC_MAJOR_3
 		return module_process_legacy(mod, NULL, 0, NULL, 0);
+#else
+		return mod->priv.ops->process_audio_stream(mod, NULL, 0, NULL, 0);


Since we check for ops != NULL in init then I would expect

// header static inline int module_process(mod, blah, blah) { return mod->priv.ops->process_audio_stream(mod, NULL, 0, NULL, 0); } // C file #if CONFIG_IPC_MAJOR_3 return module_process_legacy(mod, NULL, 0, NULL, 0); #else return module_process(mod, NULL, 0, NULL, 0); #endif

create new module process stream function for host and dai audio stream handling. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Save one more wrapper time for host copier. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Use small tricks to make dai have better performance, with this patch, dai single playback performance back to origin state(before convert to module interface). Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

keqiaozhang · 2023-08-24T08:23:55Z

SOFCI TEST

lgirdwood · 2023-08-24T10:57:43Z

+ * \brief Module-proces type
+ */
+enum module_process_type {
+	MODULE_PROCESS_STREAM,


Can we have some inline comments describing each type with examples and how to categorize. Can be in another PR .

yes, I removed this patch, only keep 3 patches for this PR, and will submit new PR later.

btian1 changed the title ~~Copier further opt~~ module adapter: module adapter and copier optimization to reduce the cost of copier module interface Jul 10, 2023

btian1 changed the title ~~module adapter: module adapter and copier optimization to reduce the cost of copier module interface~~ module adapter and copier optimization to eliminate the cost of copier module interface Jul 10, 2023

btian1 force-pushed the copier_further_opt branch 5 times, most recently from 7395487 to 2866abd Compare July 11, 2023 05:20

btian1 marked this pull request as ready for review July 11, 2023 08:57

btian1 requested review from RanderWang, abonislawski, aborisovich, dbaluta, jxstelter, lbetlej, lgirdwood, libinyang, marcinszkudlinski, mmaka1, pblaszko, plbossart and ranj063 as code owners July 11, 2023 08:57

lyakh reviewed Jul 11, 2023

View reviewed changes

btian1 marked this pull request as draft July 11, 2023 12:30

btian1 force-pushed the copier_further_opt branch 2 times, most recently from 3c77110 to 6fb56f9 Compare July 12, 2023 10:13

btian1 commented Jul 13, 2023

View reviewed changes

Comment thread src/audio/dai-zephyr.c Outdated

btian1 force-pushed the copier_further_opt branch 2 times, most recently from 8e12f35 to ae78595 Compare July 14, 2023 03:08

btian1 marked this pull request as ready for review July 14, 2023 05:45

btian1 mentioned this pull request Aug 3, 2023

module_adapter: inline some time critical functions #7993

Closed

btian1 commented Aug 9, 2023

View reviewed changes

btian1 force-pushed the copier_further_opt branch 2 times, most recently from f5254cd to 0ccf11c Compare August 9, 2023 04:42

lgirdwood requested changes Aug 9, 2023

View reviewed changes

btian1 force-pushed the copier_further_opt branch from 0ccf11c to d49e67a Compare August 11, 2023 02:43

RanderWang reviewed Aug 11, 2023

View reviewed changes

btian1 requested review from RanderWang, kv2019i, lgirdwood and lyakh August 15, 2023 13:06

btian1 force-pushed the copier_further_opt branch from d49e67a to 7e967bc Compare August 16, 2023 02:48

lyakh reviewed Aug 16, 2023

View reviewed changes

kv2019i reviewed Aug 16, 2023

View reviewed changes

lgirdwood reviewed Aug 16, 2023

View reviewed changes

btian1 force-pushed the copier_further_opt branch from 7e967bc to 7628be0 Compare August 18, 2023 07:56

lgirdwood reviewed Aug 21, 2023

View reviewed changes

btian1 force-pushed the copier_further_opt branch 2 times, most recently from 2313e31 to 195032d Compare August 24, 2023 02:31

btian1 added 3 commits August 24, 2023 10:42

audio: module adapter: create new module process for host and dai

eecfd67

create new module process stream function for host and dai audio stream handling. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

copier: inline host common copy to save one more function call.

e2a34fa

Save one more wrapper time for host copier. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

copier-dai: optimize dai copy part

195032d

Use small tricks to make dai have better performance, with this patch, dai single playback performance back to origin state(before convert to module interface). Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

btian1 force-pushed the copier_further_opt branch from b7a2827 to 9921f85 Compare August 24, 2023 07:36

lgirdwood approved these changes Aug 24, 2023

View reviewed changes

btian1 force-pushed the copier_further_opt branch from 9921f85 to 195032d Compare August 24, 2023 12:35

lyakh approved these changes Aug 25, 2023

View reviewed changes

kv2019i approved these changes Aug 25, 2023

View reviewed changes

kv2019i merged commit 7a77c50 into thesofproject:main Aug 25, 2023

Conversation

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

Uh oh!

Uh oh!

lyakh left a comment

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ranj063 commented Jul 13, 2023

Uh oh!

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

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!

RanderWang Aug 11, 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!

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!

ktrzcinx Aug 18, 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!

btian1 commented Jul 10, 2023 •

edited

Loading

RanderWang Aug 11, 2023 •

edited

Loading

ktrzcinx Aug 18, 2023 •

edited

Loading