Enable probes on MTL #7989

serhiy-katsyuba-intel · 2023-07-28T12:48:46Z

First commit is borrowed from Adrian Bonislawski abonislawski@e5b45c6 -- this is to port probe component to use Zephyr DMA stuff.

Rest are fixes required for probes to work properly.

Additionally, this zephyr_ll scheduler fix is required for injection probes not to cause FW panic: #7988

btian1

QB still have failed case, please check.

btian1 · 2023-07-31T02:15:14Z

src/probe/probe.c

just a suggestion, use one #if from line 264 - 277, this can remove one more "#if #else #endif", overhead is duplicated err message.

just a suggestion, use one #if from line 264 - 277, this can remove one more "#if #else #endif", overhead is duplicated err message.

@btian1 The difference is purely for the reader - the compilation result is the same. And in its present form the difference between the two cases is easier to see, so, I'd keep it as is. In general, however, yes, the fewer preprocessor conditionals we use, especially in .c, the better

src/probe/probe.c

src/audio/module_adapter/module_adapter.c

This patch ports probe component to use zephyr native drivers. Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com> Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>

src/audio/buffer.c

tmleman · 2023-08-08T10:19:41Z

The only thing I would change are commit titles. This [MTL] is especially eye-catching.

kv2019i

Thanks @serhiy-katsyuba-intel , looks good. Can you update the commit prefixes to align with existing style?

app/boards/intel_adsp_ace15_mtpm.conf

src/audio/module_adapter/module_adapter.c

btian1 · 2023-08-09T01:43:30Z

src/audio/buffer.c


 	/* return if no bytes */
 	if (!bytes) {
+#if CONFIG_SOF_LOG_DBG_BUFFER


it build nothing as default with buf_dbg, if change build log level, then it will print lot of log messages, I just mean default there is no cycle cost, and enable it indeed cost too much, not sure need a config to just cover is expensive or not.

lgirdwood · 2023-08-09T14:30:01Z

Thanks @serhiy-katsyuba-intel , looks good. Can you update the commit prefixes to align with existing style?

oh yeah - did not see that. ready to merge as soon as this is fixed.

lyakh · 2023-08-14T09:45:45Z

src/probe/probe.c

+		break;
+	}
+
+	err = dma_config(dma->dc.dmac->z_dev, dma->dc.chan->index, &dma_cfg);


return dma_config(...)

lyakh · 2023-08-14T09:48:56Z

src/probe/probe.c

just a suggestion, use one #if from line 264 - 277, this can remove one more "#if #else #endif", overhead is duplicated err message.

@btian1 The difference is purely for the reader - the compilation result is the same. And in its present form the difference between the two cases is easier to see, so, I'd keep it as is. In general, however, yes, the fewer preprocessor conditionals we use, especially in .c, the better

lyakh · 2023-08-14T09:55:24Z

src/probe/probe.c

+				      _probe->inject_dma[j].dc.chan->index) < 0) {
+#else
 			if (dma_start_legacy(_probe->inject_dma[j].dc.chan) < 0) {
+#endif


this confuses at least some editors. They don't understand that only one of the ifs is compiled, so they interpret it as

if (x) { if (y) {

and keep trying to "fix" alignment for a nested if and also gets confused about too few closing braces. Not something critical and not even an issue for everybody, but at least we did modify one such code fragment for this reason to read

#if X if (a) #else if (b) #endif {

This contradicts style but at least the editor is happy about balanced braces

comp_update_buffer_consume() and comp_update_buffer_produce() generate too much log output. Usually such amount of logs results in many log messages been dropped. It is also results in significant CPU load if selected log backend formats log messages at runtime. This patch adds a separate switch to enable these logs only when necessary. Default value is 'n'. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>

Probe module subscribes to buffer produce notifications in order to perform data extraction or injection. Hence this fix is required to enable probe feature. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>

Both extraction and injection probes require to subscribe to buffer produce notification. Hence notification subscription logic should be enabled for both types of probes. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>

Enables extraction and injection probes for MTL platform. NOTE: this commit does NOT enable probe log backend. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>

serhiy-katsyuba-intel · 2023-08-16T15:23:19Z

Thanks @serhiy-katsyuba-intel , looks good. Can you update the commit prefixes to align with existing style?

oh yeah - did not see that. ready to merge as soon as this is fixed.

Done.

jsarha

I have just a minor nitpick, that can be ignored, but there were some other valid comments, however with those fixed, approved by me.

jsarha · 2023-08-16T11:57:51Z

src/probe/probe.c

 	return 0;
 }

+#if !CONFIG_ZEPHYR_NATIVE_DRIVERS


Just a nitpick, I find it slightly annoying that this in #if the condition is negated, but in all the following ones the native drivers version comes first.

serhiy-katsyuba-intel force-pushed the en_probes branch 3 times, most recently from 0043546 to 4b12205 Compare July 28, 2023 16:26

serhiy-katsyuba-intel marked this pull request as ready for review July 28, 2023 16:41

serhiy-katsyuba-intel requested review from dbaluta, jxstelter, lbetlej, lgirdwood, mmaka1, plbossart and ranj063 as code owners July 28, 2023 16:41

serhiy-katsyuba-intel requested a review from abonislawski July 28, 2023 16:41

btian1 reviewed Jul 31, 2023

View reviewed changes

serhiy-katsyuba-intel force-pushed the en_probes branch from 4b12205 to 43f2b64 Compare July 31, 2023 13:06

lgirdwood reviewed Jul 31, 2023

View reviewed changes

src/probe/probe.c Outdated Show resolved Hide resolved

src/audio/module_adapter/module_adapter.c Outdated Show resolved Hide resolved

serhiy-katsyuba-intel force-pushed the en_probes branch 2 times, most recently from e891dbf to d1098ae Compare August 2, 2023 13:20

probe: add support for zephyr native drivers

da10ef9

This patch ports probe component to use zephyr native drivers. Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com> Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>

serhiy-katsyuba-intel force-pushed the en_probes branch from d1098ae to 7d65955 Compare August 2, 2023 13:26

lgirdwood reviewed Aug 2, 2023

View reviewed changes

src/audio/buffer.c Outdated Show resolved Hide resolved

serhiy-katsyuba-intel force-pushed the en_probes branch 3 times, most recently from 2334def to 3fb4aed Compare August 4, 2023 07:46

lgirdwood approved these changes Aug 7, 2023

View reviewed changes

jxstelter approved these changes Aug 8, 2023

View reviewed changes

abonislawski approved these changes Aug 8, 2023

View reviewed changes

tmleman approved these changes Aug 8, 2023

View reviewed changes

kv2019i reviewed Aug 8, 2023

View reviewed changes

app/boards/intel_adsp_ace15_mtpm.conf Show resolved Hide resolved

src/audio/module_adapter/module_adapter.c Show resolved Hide resolved

btian1 reviewed Aug 9, 2023

View reviewed changes

dnikodem approved these changes Aug 10, 2023

View reviewed changes

lyakh reviewed Aug 14, 2023

View reviewed changes

serhiy-katsyuba-intel added 4 commits August 16, 2023 16:57

module_adapter: Enable buffer produce notifications required by probe

735f8ee

Probe module subscribes to buffer produce notifications in order to perform data extraction or injection. Hence this fix is required to enable probe feature. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>

probe: Fix injection probe creation

57c0dff

Both extraction and injection probes require to subscribe to buffer produce notification. Hence notification subscription logic should be enabled for both types of probes. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>

app: Enable probe for MTL

daa1231

Enables extraction and injection probes for MTL platform. NOTE: this commit does NOT enable probe log backend. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>

serhiy-katsyuba-intel force-pushed the en_probes branch from 3fb4aed to daa1231 Compare August 16, 2023 14:58

jsarha approved these changes Aug 16, 2023

View reviewed changes

kv2019i merged commit 81bccef into thesofproject:main Aug 16, 2023

This was referenced Aug 17, 2023

[BUG] unable to enable probes logging for MTL #7218

Closed

[FEATURE][meta] Use Zephyr native interfaces #5794

Open

Enable probes on MTL #7989

Enable probes on MTL #7989

Uh oh!

Conversation

serhiy-katsyuba-intel commented Jul 28, 2023

Uh oh!

btian1 left a comment

Choose a reason for hiding this comment

Uh oh!

btian1 Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

lyakh Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tmleman commented Aug 8, 2023

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

btian1 Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Aug 9, 2023

Uh oh!

lyakh Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

lyakh Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

lyakh Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-katsyuba-intel commented Aug 16, 2023

Uh oh!

jsarha left a comment

Choose a reason for hiding this comment

Uh oh!

jsarha Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants