[RFC] Dai zephyr stub #5731

juimonen · 2022-04-26T05:59:47Z

No description provided.

src/drivers/intel/ssp/ssp-zephyr.c

src/lib/dai.c

src/platform/intel/cavs/lib/dai.c

src/lib/dai.c

lyakh

I don't understand what the first of the two commits - the merge commit - in this PR is doing, hopefully it's really something that's needed here. The approval is to the actual commit with code

marcinszkudlinski · 2022-04-27T12:04:57Z

src/drivers/intel/ssp/ssp-zephyr.c

Suggestion: ssp-zephyr-shim.c

ok, Liam has a better solution: #5727 (review)

hmm ok, let me check if I can do something like that

marcinszkudlinski · 2022-04-27T14:35:15Z

src/lib/dai.c

What if dai_get_zephyr returns NULL? Currently the flow will go through non-zephyr dai creation. I think we rather should return NULL here.

the problem is that in this transient period we have drivers both in zephyr and xtos side. So even if ZEPHYR is defined, we might have only like ssp and alh in zephyr. So the dai_get_zephyr will check the type and not do anything if driver is noton zephyr side.

ujfalusi · 2022-05-04T06:02:51Z

src/audio/dai.c

why and what the op postfix means? Would it sound better if they are postfixed instead with sof?
dai_trigger -> sof_dai_trigger

nitpick

...and you already use dai_*_xtos(), so why not use "xtos" everywhere? And I agree with @ujfalusi - a prefix seems both more common and more logical - you usually go from the least to the most specific part. I.e. "xtos" is the least specific part, "dai" is the next level, and the actual operation is the most specific part

This may be a naming challenges we can't win.

I wonder if it'd be cleaner to rename the public "dai.h" to have "sof_" prefix, and then just have different implementations for this in src/lib/dai.c.

yeah totally fine with that, will try and see

ujfalusi · 2022-05-04T06:13:23Z

src/lib/dai.c

For minimized #ifdef __ZEPHYR__ I would have the dai_get(uint32_t type, uint32_t index, uint32_t flags) ifdefed.
Or use the same name for the sub dai_get / dai_put OS dependent implementation, but the naming can get funny...
os_dai_get / dai_get_from_os / ...

lgirdwood

@abonislawski @marcinszkudlinski any comments ?

src/audio/dai-zephyr.c

lgirdwood · 2022-06-14T13:40:46Z

src/audio/dai-zephyr.c

this could be made static inline (if Zephyr) and go away ?

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

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

src/lib/dai.c

lgirdwood · 2022-06-14T14:18:14Z

@juimonen one other thing, it would be good to add inline TODO comments in these files where we have future Zephyr integration to help align everyone.

XiaoyunWu6666 · 2022-06-14T16:58:07Z

SOFCI TEST

kv2019i

Started the review, but didn't have time to complete. Will resume.

src/audio/dai-zephyr.c

juimonen · 2022-06-14T17:32:27Z

@juimonen one other thing, it would be good to add inline TODO comments in these files where we have future Zephyr integration to help align everyone.

@lgirdwood... I'm not seeing currently obvious TODO in files changed in this PR... for example lib/dma.c should be changed to similar mechanism than here with dai. That might effect some of the struct members in this PR, but can't really say for sure in some TODO.

kv2019i

Looks good. I think further improvements to dai-zephyr.h are better done in follow-up PRs.

kv2019i · 2022-06-15T07:21:06Z

src/lib/dai.c

minor: I think this could be a good actual usage of an assert to check that strlen of drv_name is sufficient.

oh, something is missing as I changed this to Liams suggerstion.... hold on for awhile

@kv2019i I screwed up yesterday with git reset... now includes the "dai array" changes suggested by Liam for this part of the code... apologies

This patch adds a mechanism to load and use dai driver from zephyr instead of xtos driver. Patch includes following changes: - dai-zephyr.c calls directly zephyr driver methods - include/lib/dai.h is divided into dai-legacy.h and dai-zephyr.h - use zephyr dais by configuring CONFIG_ZEPHYR_NATIVE_DRIVERS=y Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>

lgirdwood

Some logging could be added later. @abonislawski @marcinszkudlinski @ArsenEloglian any comments ?

lgirdwood · 2022-06-15T18:43:29Z

src/lib/dai.c

 		group->group_id = 0;
 }

+#if CONFIG_ZEPHYR_NATIVE_DRIVERS


@juimonen @ArsenEloglian it seems like future steps are to split lib/dai.c into legacy and Zephyr versions. For another PR though...

lgirdwood · 2022-06-15T18:44:20Z

src/lib/dai.c

+	{"SSP_", {
+			.type = SOF_DAI_INTEL_SSP,
+			.dma_dev = DMA_DEV_SSP,
+			.dma_caps = DMA_CAP_GP_LP | DMA_CAP_GP_HP,


I supose these caps are not needed for Zephyr (as the Zephyr driver will probably know via DT which memory) ?

This is interesting thing. Currently we still need this as our dma_get() API is asking for a DMA instance that matches a given filter and the filter includes "caps". Zephyr DT described HW capabilities, but the lookup code is still on SOF side in src/lib/dma.c and database is filled in src/platform/intel/cavs/lib/dma.c . The latter could be filled directly from Zephyr, but some lookup logic is still needed.

But indeed, @juimonen and @ArsenEloglian , I guess we could leave "dma_caps==0" here when doing dma_get() for native Zephyr drivers. The caps checks will be skipped and DMA is chosen based on dma_dev alone. But this seems good enough. Zephyr drivers don't need this info, this is only to select the DMA instance.

lgirdwood · 2022-06-15T18:45:42Z

src/lib/dai.c

+
+	drv = device_get_binding(dai_name);
+	if (!drv)
+		return NULL;


shall we tell the user in the logs ?

lgirdwood · 2022-06-15T18:45:56Z

src/lib/dai.c

+	}
+
+	if (!found)
+		return NULL;


Log the error ?

kv2019i

Still good to go, but do check the dai_info_table comment. Maybe we can drop the "dma_caps" from the dai query.

kv2019i · 2022-06-16T10:14:37Z

src/lib/dai.c

+	{"SSP_", {
+			.type = SOF_DAI_INTEL_SSP,
+			.dma_dev = DMA_DEV_SSP,
+			.dma_caps = DMA_CAP_GP_LP | DMA_CAP_GP_HP,


This is interesting thing. Currently we still need this as our dma_get() API is asking for a DMA instance that matches a given filter and the filter includes "caps". Zephyr DT described HW capabilities, but the lookup code is still on SOF side in src/lib/dma.c and database is filled in src/platform/intel/cavs/lib/dma.c . The latter could be filled directly from Zephyr, but some lookup logic is still needed.

But indeed, @juimonen and @ArsenEloglian , I guess we could leave "dma_caps==0" here when doing dma_get() for native Zephyr drivers. The caps checks will be skipped and DMA is chosen based on dma_dev alone. But this seems good enough. Zephyr drivers don't need this info, this is only to select the DMA instance.

marc-hb · 2022-07-06T00:31:22Z

As discovered by @deb-intel, the duplication between dai-legacy.h and dai-zephyr.h broke sof-docs:

Just a test IGNORE - repro WARNING: Duplicate explicit target name: sof-docs#419

@juimonen can you please remove all the doxygen duplication? (The duplication of code is fine, the docs don't care about that)

marc-hb · 2022-07-06T00:40:59Z

Why no dai-common.h?

lgirdwood · 2022-07-06T12:13:32Z

Why no dai-common.h?

This is in plan for @juimonen @ArsenEloglian @abonislawski

From time to time sof-docs regressions are introduced in sof.git. This introduces a random and sometimes long delay between when the regression is introduced and when it is found. A recent example is thesofproject#5731 (comment) where the doxygen comments were duplicated. Doxygen alone did not mind, then the sof-docs build failed much later which took multiple people a lot of time to understand and bisect. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

From time to time sof-docs regressions are introduced in sof.git. This introduces a random and sometimes long delay between when the regression is introduced and when it is found. A recent example is #5731 (comment) where the doxygen comments were duplicated. Doxygen alone did not mind, then the sof-docs build failed much later which took multiple people a lot of time to understand and bisect. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

This fixes the sof-docs build and stops producing a dai-drivers-api.html garbage that randomly mixed the two .h files. ``` Warning: Duplicate target detected: group__sof__dai__drivers_1ga8c720c310f408b2f97ce014562d6a910 sof-docs/api/dai-drivers-api.rst:4: WARNING: Duplicate ID: "group__sof__dai__drivers_1ga8c720c310f408b2f97ce014562d6a910". sof-docs/api/dai-drivers-api.rst:4: WARNING: Duplicate explicit target name: "group__sof__dai__drivers_1ga8c720c310f408b2f97ce014562d6a910". sof/sof-docs/api/dai-drivers-api.rst:6: WARNING: Duplicate declaration, const struct device* dai::drv ``` (Temporarily?) fixes commit 015ec63 ("zephyr: add glue code to use zephyr dai drivers") that introduced a lot of duplication with dai-zephyr.h. See more details at thesofproject#5731 (comment) Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-hb · 2022-07-06T22:03:47Z

Just learned @juimonen is on vacation. 2-lines fix/workaround submitted at:

[SKIP CI] Exclude dai-legacy.h from doxygen to avoid clashes with dai-zephyr.h #5995

This simply excludes dai-legacy.h from doxygen instead of randomly mixing both; good enough for now.

This fixes the sof-docs build and stops producing a dai-drivers-api.html garbage that randomly mixed the two .h files. ``` Warning: Duplicate target detected: group__sof__dai__drivers_1ga8c720c310f408b2f97ce014562d6a910 sof-docs/api/dai-drivers-api.rst:4: WARNING: Duplicate ID: "group__sof__dai__drivers_1ga8c720c310f408b2f97ce014562d6a910". sof-docs/api/dai-drivers-api.rst:4: WARNING: Duplicate explicit target name: "group__sof__dai__drivers_1ga8c720c310f408b2f97ce014562d6a910". sof/sof-docs/api/dai-drivers-api.rst:6: WARNING: Duplicate declaration, const struct device* dai::drv ``` (Temporarily?) fixes commit 015ec63 ("zephyr: add glue code to use zephyr dai drivers") that introduced a lot of duplication with dai-zephyr.h. See more details at #5731 (comment) Signed-off-by: Marc Herbert <marc.herbert@intel.com>

juimonen requested review from aborisovich, dbaluta, lbetlej, lgirdwood, libinyang, marc-hb, mmaka1 and plbossart as code owners April 26, 2022 05:59

juimonen requested review from lyakh and removed request for aborisovich, dbaluta, lbetlej, lgirdwood, libinyang, marc-hb, mmaka1 and plbossart April 26, 2022 06:00

juimonen marked this pull request as draft April 26, 2022 06:00

lyakh reviewed Apr 26, 2022

View reviewed changes

juimonen force-pushed the ssp_zephyr_stub branch 2 times, most recently from 8c94447 to 6d8c379 Compare April 26, 2022 12:13

lyakh approved these changes Apr 27, 2022

View reviewed changes

marcinszkudlinski reviewed Apr 27, 2022

View reviewed changes

juimonen force-pushed the ssp_zephyr_stub branch 4 times, most recently from dec7307 to 1e4e93e Compare May 4, 2022 05:59

ujfalusi reviewed May 4, 2022

View reviewed changes

juimonen force-pushed the ssp_zephyr_stub branch 2 times, most recently from 758d12d to a8d81d7 Compare June 13, 2022 18:29

lgirdwood reviewed Jun 14, 2022

View reviewed changes

juimonen force-pushed the ssp_zephyr_stub branch from a8d81d7 to 181c083 Compare June 14, 2022 14:49

kv2019i reviewed Jun 14, 2022

View reviewed changes

src/audio/dai-zephyr.c Outdated Show resolved Hide resolved

juimonen force-pushed the ssp_zephyr_stub branch 2 times, most recently from fd19609 to 3b4319f Compare June 14, 2022 17:29

kv2019i approved these changes Jun 15, 2022

View reviewed changes

juimonen force-pushed the ssp_zephyr_stub branch 2 times, most recently from 30b2ea1 to d16ad08 Compare June 15, 2022 12:43

juimonen force-pushed the ssp_zephyr_stub branch 2 times, most recently from 9aeab38 to cd90420 Compare June 15, 2022 15:16

lgirdwood approved these changes Jun 15, 2022

View reviewed changes

kv2019i approved these changes Jun 16, 2022

View reviewed changes

lgirdwood merged commit 015ec63 into thesofproject:main Jun 16, 2022

This was referenced Jul 6, 2022

[SKIP CI] .github: add sof-docs build to Github Actions #5990

Merged

Algorithms: Add equalizers design tutorial thesofproject/sof-docs#417

Merged

marc-hb mentioned this pull request Jul 6, 2022

[SKIP CI] Exclude dai-legacy.h from doxygen to avoid clashes with dai-zephyr.h #5995

Merged

[RFC] Dai zephyr stub #5731

[RFC] Dai zephyr stub #5731

Uh oh!

Conversation

juimonen commented Apr 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgirdwood commented Jun 14, 2022

Uh oh!

XiaoyunWu6666 commented Jun 14, 2022

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

juimonen commented Jun 14, 2022

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!

lgirdwood 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