module_adapter : audio : fix DTS support on MTL by joechengxperi · Pull Request #8379 · thesofproject/sof

joechengxperi · 2023-10-24T06:38:57Z

Add the commits to support DTS processing on MTL.

dbaluta · 2023-10-24T07:16:03Z

src/audio/CMakeLists.txt

@@ -169,7 +169,8 @@ check_optimization(fma -mfma -ftree-vectorize -DOPS_FMA)
 check_optimization(hifi2ep -mhifi2ep "" -DOPS_HIFI2EP)


Please split this into multiple patches. One patch per logical change.

I've tried to split the patches. Please let me know if it's okay or not. Thanks.

kv2019i

Thanks @joechengxperi ! Code looks good, but like @dbaluta noted, it would be good to split the series. This is bit of a special case as DTS is only enabled if the overlay is selected, but in principle fixes should come first and the code enable for IPC4 as the last commit.

btian1 · 2023-10-25T02:16:24Z

app/overlays/mtl/dts_overlay.conf

@@ -0,0 +1,3 @@
+CONFIG_COMP_IIR=y


I noticed this may be followed from:
./src/arch/xtensa/configs/override/mt8195_chrome_dts.config:4:CONFIG_DTS_CODEC=y
./src/arch/xtensa/configs/override/renoir_chrome_dts.config:3:CONFIG_DTS_CODEC=y
./src/arch/xtensa/configs/override/mt8186_chrome_dts.config:3:CONFIG_DTS_CODEC=y

my question is: is IIR must follow with DTS codec? they should have no relation from module perspecitve.

We use EQ IIR + DTS to support customer project. For different platforms, EQ IIR might be default enabled or disabled. So we add it to make sure IIR is enabled. On MTL, IIR is default enabled. I could remove it if you think it's better.

So the Kconfig can set a dependecy, we could use depends in Kconfig to state this relationship between DTS and IIR.
@singalsu fyi.

marc-hb · 2023-10-25T04:15:42Z

This is a good page

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

It would build fail without LOG_MODULE_REGISTER. Co-developed-by: Mac Chiang <mac.chiang@intel.com> Signed-off-by: Mac Chiang <mac.chiang@intel.com> Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

joechengxperi · 2023-10-25T07:58:26Z

Thanks @joechengxperi ! Code looks good, but like @dbaluta noted, it would be good to split the series. This is bit of a special case as DTS is only enabled if the overlay is selected, but in principle fixes should come first and the code enable for IPC4 as the last commit.

@kv2019i I've tried to split the patches and adjusted the order of commits. Please let me know if it's okay. Thanks.

joechengxperi · 2023-10-25T08:01:46Z

This is a good page

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

This is super helpful. Thanks @marc-hb!

lgirdwood

Looks good, I think only some minor opens from me.

src/include/sof/audio/dts/DtsSofInterfaceVersion.h

lgirdwood · 2023-10-25T15:36:55Z

src/include/sof/audio/component.h

 void sys_comp_module_tdfb_interface_init(void);
 void sys_comp_module_volume_interface_init(void);

+void sys_comp_module_dts_interface_init(void);


nitpick: this should be in line above, next to the others.

not in alphabetical order

lgirdwood · 2023-10-25T15:38:58Z

app/overlays/mtl/dts_overlay.conf

@@ -0,0 +1,3 @@
+CONFIG_COMP_IIR=y


So the Kconfig can set a dependecy, we could use depends in Kconfig to state this relationship between DTS and IIR.
@singalsu fyi.

btian1

one suggestion： I suggest create a new folder under src/audio with codec/dts, and move out dts related into this folder.
Reason: now, most of modules are module interface, modules does not need put it under:
src/audio/module_adapter/module directory, it should have a standalone folder to cover.

ideally, all modules will be under src/audio with integrated all its source code in future.

kv2019i

Excellent, thanks @joechengxperi , looks very good. Only issue with the last patch and I know you are addressing to feedback, so @lgirdwood @btian1 I'd propose we drop the last patch... ok?

kv2019i · 2023-10-26T12:16:07Z

src/audio/Kconfig

 	select COMP_BLOB
 	default y
-	depends on COMP_MODULE_ADAPTER
+	depends on COMP_MODULE_ADAPTER || DTS_CODEC


I think this is the wrong way. I'd be happy with your original solution and just have this in the overlay as this is not a strict dependency at code level, but a requirement at the topologoy level, so I think the overlay is a better place.

I also think this is not a good place, IIR can't depend on DTS, it is a standalone feature.

@lgirdwood May I know your feedback?

@joechengxperi I agree, I am not sure why the config is defined this way, you should select the IIR component from the DTS component.

Thanks @cujomalainey. It looks better using select as you mentioned in this case. But after thinking about this a few days, I prefer to move it back to overlay rather than config. As @kv2019i said, it's not a strict dependency. Also, this dependency may probably change in the future, depends on customer requirement or platform. It seems using overlay and topology is more flexible.

They should not depend on each other.
Besides I don't think this meets your intention "make sure IIR is enabled on different platforms when DTS_CODEC is enabled", which is "if DTS=y; set IIR=y". If so, you should state "config DTS select IIR"

Quoting another thread:

We use EQ IIR + DTS to support customer project.

Overlays is where you define that, not in Kconfig files (and overlays don't have to be in this git repo, they can come from anywhere).

The purpose of Kconfig is to make sure the code compiles and does not crash and nothing else (and managing these basic dependencies is difficult and error-prone enough already, random Kconfig problem of the day: #8425). Kconfig is only for "hard science" and universal dependencies that affect everyone using the code base, never for "preferences" and even less for "personal" preferences.

While Kconfig came from the Linux kernel, Zephyr has IMHO better Kconfig documentation:

https://docs.zephyrproject.org/latest/build/kconfig/index.html
https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html

They should be bug-for-bug compatible so both documentations should apply.

I see the current version does not seem to change Kconfig
https://github.com/thesofproject/sof/commits/1ef2f59db0a

@marc-hb I removed that commit which added the wrong dependency and restored the original solution in overlay yesterday.

btian1

@kv2019i @joechengxperi , please check my another comments, I am suggesting move out dts to src/audio/codec/dts, please let me know what do you think about this?

btian1 · 2023-10-27T02:24:35Z

src/audio/Kconfig

 	select COMP_BLOB
 	default y
-	depends on COMP_MODULE_ADAPTER
+	depends on COMP_MODULE_ADAPTER || DTS_CODEC


I also think this is not a good place, IIR can't depend on DTS, it is a standalone feature.

joechengxperi · 2023-10-30T09:38:16Z

one suggestion： I suggest create a new folder under src/audio with codec/dts, and move out dts related into this folder. Reason: now, most of modules are module interface, modules does not need put it under: src/audio/module_adapter/module directory, it should have a standalone folder to cover.

ideally, all modules will be under src/audio with integrated all its source code in future.

It sounds a reasonable move to me. If it will be the target to do, I'll work on it in another PR.

lyakh · 2023-10-30T09:55:34Z

src/audio/module_adapter/module/dts/dts.c

 				       fragment_size, response, response_size);
 	if (ret < 0)
 		return ret;
+	}


is this removed in a later commit again?

src/audio/module_adapter/module/dts/dts.c

lyakh · 2023-10-30T09:57:21Z

src/audio/module_adapter/module/dts/dts.c

 	ret = module_set_configuration(mod, config_id, pos, data_offset_size, fragment,
 				       fragment_size, response, response_size);
-	if (ret < 0)
+	if (ret < 0) {


yes, both braces must be in the same commit to avoid breaking bisection

Done. Put them in the same commit now.

md->state is set in IPC3, not in IPC4. set_configuration() would fail on MTL because md->state is not set. Add macro CONFIG_IPC_MAJOR_3 to avoid this problem. Co-developed-by: Mac Chiang <mac.chiang@intel.com> Signed-off-by: Mac Chiang <mac.chiang@intel.com> Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

Add more error logs to identify potential problem in set_configuration() Co-developed-by: Mac Chiang <mac.chiang@intel.com> Signed-off-by: Mac Chiang <mac.chiang@intel.com> Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

MTL support is added in DTS V1.1.3. Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

cujomalainey · 2023-10-30T20:08:31Z

src/include/sof/audio/component.h

 void sys_comp_module_tdfb_interface_init(void);
 void sys_comp_module_volume_interface_init(void);

+void sys_comp_module_dts_interface_init(void);


not in alphabetical order

zephyr/CMakeLists.txt

cujomalainey · 2023-10-30T20:17:17Z

src/audio/Kconfig

 	select COMP_BLOB
 	default y
-	depends on COMP_MODULE_ADAPTER
+	depends on COMP_MODULE_ADAPTER || DTS_CODEC


@joechengxperi I agree, I am not sure why the config is defined this way, you should select the IIR component from the DTS component.

johnylin76 · 2023-10-31T16:20:03Z

src/audio/Kconfig

 	select COMP_BLOB
 	default y
-	depends on COMP_MODULE_ADAPTER
+	depends on COMP_MODULE_ADAPTER || DTS_CODEC


They should not depend on each other.
Besides I don't think this meets your intention "make sure IIR is enabled on different platforms when DTS_CODEC is enabled", which is "if DTS=y; set IIR=y". If so, you should state "config DTS select IIR"

johnylin76 · 2023-10-31T16:25:08Z

src/audio/CMakeLists.txt


-set(sof_audio_modules mixer volume src asrc eq-fir eq-iir dcblock crossover tdfb drc multiband_drc mfcc mux)
+set(sof_audio_modules mixer volume src asrc eq-fir eq-iir dcblock crossover
+    tdfb drc multiband_drc mfcc mux dts)


May I know why is this commit required?
If yes, may it be moved to src/audio/module_adapter/CMakeLists.txt?

I think it could be removed now as it's already handled in src/audio/module_adapter/CMakeLists.txt.

zephyr/CMakeLists.txt

Add DTS init function Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

btian1 · 2023-11-03T01:44:18Z

Excellent, thanks @joechengxperi , looks very good. Only issue with the last patch and I know you are addressing to feedback, so @lgirdwood @btian1 I'd propose we drop the last patch... ok?

This PR purpose is enable DTS on MTL, if drop last one, then DTS will not be enabled on MTL, my opinion is we can go for now, this feature does not have too much relation with sof other modules, we can go with this and fix new issue if have in future.

kv2019i

This is fine to me. The location of the DTS source files should be moved to a better place, but I'm ok to do that in a follow-up PR (as we are anyways moving files around during SOF2.8 cycles that is now ongoing).

joechengxperi · 2023-11-10T12:32:49Z

@lgirdwood For this PR, may I know if there is still anything I have to do for merge? Thanks.

kv2019i · 2023-11-10T13:09:21Z

@joechengxperi wrote:

@lgirdwood For this PR, may I know if there is still anything I have to do for merge? Thanks.

@cujomalainey has a -1 on this, that's blocking the merge now. Otherwise looks good to go.

lgirdwood · 2023-11-13T13:48:58Z

@joechengxperi wrote:

@lgirdwood For this PR, may I know if there is still anything I have to do for merge? Thanks.

@cujomalainey has a -1 on this, that's blocking the merge now. Otherwise looks good to go.

@joechengxperi I assume all of @cujomalainey comments are aligned/closed now ? if so, he should be able to re-review.

cujomalainey

sorry spotted one more thing

cujomalainey · 2023-11-13T16:41:34Z

zephyr/CMakeLists.txt

 zephyr_library_sources_ifdef(CONFIG_DTS_CODEC
 	${SOF_AUDIO_PATH}/module_adapter/module/dts/dts.c
 )
+if (CONFIG_DTS_CODEC)


This broke the stub build. Please fix

This broke the stub build. Please fix

@joechengxperi it is the responsibility of the submitter to look at failures, not the reviewers. Ask for help if you don't understand some failures (or worse: if you don't have access to some)

https://github.com/thesofproject/sof/actions/runs/6718130608/job/18257259780?pr=8379

The last error messages can be misleading. Always scroll up and look at the first errors first. Parallel builds may require scrolling quite a way up.

Fixed. Apologize that I did not notice that build error.

Importing DTS library here to ensure DTS library is built into SOF firmware as a static library. Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

This patch adds DTS_CODEC for IPC4 Intel platform builds. Since EQ IIR + DTS is the combination we use to support customer, adding EQ IIR here to ensure it's enabled. Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

joechengxperi · 2023-11-14T08:53:45Z

@joechengxperi wrote:

@lgirdwood For this PR, may I know if there is still anything I have to do for merge? Thanks.

@cujomalainey has a -1 on this, that's blocking the merge now. Otherwise looks good to go.

@joechengxperi I assume all of @cujomalainey comments are aligned/closed now ? if so, he should be able to re-review.

Thank you @lgirdwood @kv2019i. Yes, I think those are all aligned now.

cujomalainey · 2023-11-14T16:20:44Z

On device test failure is unrelated to this change, merging

joechengxperi requested review from cujomalainey, johnylin76, macchian and markbartonxperi October 24, 2023 06:38

joechengxperi requested review from dbaluta, kv2019i, lbetlej, lgirdwood, marc-hb, mmaka1 and plbossart as code owners October 24, 2023 06:38

dbaluta reviewed Oct 24, 2023

View reviewed changes

kv2019i reviewed Oct 24, 2023

View reviewed changes

btian1 reviewed Oct 25, 2023

View reviewed changes

dts : add LOG_MODULE_REGISTER()

9074a05

It would build fail without LOG_MODULE_REGISTER. Co-developed-by: Mac Chiang <mac.chiang@intel.com> Signed-off-by: Mac Chiang <mac.chiang@intel.com> Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

joechengxperi force-pushed the main-dts-mtl-merge-2 branch from 554e988 to de8db87 Compare October 25, 2023 07:55

lgirdwood reviewed Oct 25, 2023

View reviewed changes

btian1 reviewed Oct 26, 2023

View reviewed changes

joechengxperi force-pushed the main-dts-mtl-merge-2 branch from de8db87 to 694aeb6 Compare October 26, 2023 09:47

kv2019i reviewed Oct 26, 2023

View reviewed changes

btian1 reviewed Oct 27, 2023

View reviewed changes

lyakh reviewed Oct 30, 2023

View reviewed changes

joechengxperi added 3 commits October 30, 2023 20:28

dts : add more error logs in set_configuration()

f07d601

Add more error logs to identify potential problem in set_configuration() Co-developed-by: Mac Chiang <mac.chiang@intel.com> Signed-off-by: Mac Chiang <mac.chiang@intel.com> Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

dts : update DTS version to V1.1.3

8bb4a05

MTL support is added in DTS V1.1.3. Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

joechengxperi force-pushed the main-dts-mtl-merge-2 branch from 694aeb6 to 47822aa Compare October 30, 2023 12:46

cujomalainey suggested changes Oct 30, 2023

View reviewed changes

joechengxperi force-pushed the main-dts-mtl-merge-2 branch from 47822aa to f41e4fb Compare October 31, 2023 07:38

johnylin76 reviewed Oct 31, 2023

View reviewed changes

audio : dts : add DTS support for MTL

04991be

Add DTS init function Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

joechengxperi force-pushed the main-dts-mtl-merge-2 branch from f41e4fb to 1ef2f59 Compare November 1, 2023 09:45

marc-hb requested a review from andyross November 1, 2023 17:07

btian1 approved these changes Nov 3, 2023

View reviewed changes

kv2019i approved these changes Nov 3, 2023

View reviewed changes

joechengxperi requested review from cujomalainey and lgirdwood November 8, 2023 02:48

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

cujomalainey approved these changes Nov 13, 2023

View reviewed changes

cujomalainey suggested changes Nov 13, 2023

View reviewed changes

joechengxperi added 2 commits November 14, 2023 14:55

zephyr : import dts library

7d5cc30

Importing DTS library here to ensure DTS library is built into SOF firmware as a static library. Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

app: overlays: Add DTS support to MTL

ac2f487

This patch adds DTS_CODEC for IPC4 Intel platform builds. Since EQ IIR + DTS is the combination we use to support customer, adding EQ IIR here to ensure it's enabled. Signed-off-by: Joe Cheng <joe.cheng@xperi.com>

joechengxperi force-pushed the main-dts-mtl-merge-2 branch from 1ef2f59 to ac2f487 Compare November 14, 2023 06:59

cujomalainey approved these changes Nov 14, 2023

View reviewed changes

cujomalainey merged commit a8db157 into thesofproject:main Nov 14, 2023

		@@ -169,7 +169,8 @@ check_optimization(fma -mfma -ftree-vectorize -DOPS_FMA)
		check_optimization(hifi2ep -mhifi2ep "" -DOPS_HIFI2EP)

Conversation

joechengxperi commented Oct 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

marc-hb commented Oct 25, 2023

Uh oh!

joechengxperi commented Oct 25, 2023

Uh oh!

joechengxperi commented Oct 25, 2023

Uh oh!

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

btian1 left a comment

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

joechengxperi commented Oct 30, 2023

Uh oh!

Choose a reason for hiding this comment

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!

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!