Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

split lib/dma.h and start namespacing with DMA_ #9719

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kv2019i
Copy link
Collaborator

@kv2019i kv2019i commented Dec 10, 2024

Opening PR for DMA cleanup. It does seem this is going to be a bit more invasive (= need to touch a lot of code) than originally thought, so I'd like to get some feedback with this smaller PR before I go about mass renaming the whole SOF side DMA interface.

The definitions moved out to dma-legacy.h don't need to be touched (about 300 lines worth of definitions).

The definitions that remain in zephyr/lib/dma.h however need to be prefixed, and same needs to be done in xtos/lib/dma.h as well.

The benefit of all this is that in generic code, it's immediately clear which DMA function calls and definitions are Zephyr API calls, and which are related to the SOF DMA layer that still sits on top.

Link: #9561

Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks a bit confusing that the two last patches are doing the same renaming in different headers and C-code, but I suppose it's correct - we really do have 2 non-overlapping sets of .c and .h using the same macros which then can be renamed separately?

@kv2019i
Copy link
Collaborator Author

kv2019i commented Dec 11, 2024

@lyakh wrote:

it looks a bit confusing that the two last patches are doing the same renaming in different headers and C-code, but I suppose it's correct - we really do have 2 non-overlapping sets of .c and .h using the same macros which then can be

Agreed, this is does look weird and I expect I'll need more tree wide big single patch for other parts of dma.h. OTOH, there is some logic to it as majority (but not all) of DMA usage is in the host/dai code and this code is also branched in calling code (e.g. there is dai-zephyr.c and dai-legacy.c). So there is cleanup work needed at multiple layers I'm afraid.

@kv2019i kv2019i changed the title split lib/dma.h and start namespacing with DMA_CAP_ [DNM] split lib/dma.h and start namespacing with DMA_CAP_ Dec 11, 2024
@kv2019i
Copy link
Collaborator Author

kv2019i commented Dec 11, 2024

Actually, while replying to Guennadi in previous comment I realize the split in call sites is quite extensive. We have ifefs in DAI code, chain-dma is native driver only, probe has ifdefs for native drivers, and so forth.

So let me try again with the option to add a namespace only for zephyr/lib/dma.h. This is a bit strange, but if works a) much less code churn needed, and b) original goal to clarify the code (what calls are SOF and what are Zephyr) would still be met. To be continued...

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @kv2019i - one step at a time.

Copy link

@bhiregoudar bhiregoudar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Simplify the stub DMA driver in posix builds. As the posix platform is
built on the Zephyr posix support, there's no need to support builds
with XTOS DMA driver interface. Drop the XTOS DMA driver interface
support and default to native Zephyr DMA interface. A minimal dma_info
description needs to be kept as this is (still) used in SOF dma_get() to
choose the right DMA instance to use.

Signed-off-by: Kai Vehmanen <[email protected]>
@kv2019i kv2019i force-pushed the 202412-dma-namespacing-part1 branch from a29c239 to a74317f Compare December 12, 2024 13:26
@kv2019i kv2019i changed the title [DNM] split lib/dma.h and start namespacing with DMA_CAP_ split lib/dma.h and start namespacing with DMA_ Dec 12, 2024
@kv2019i
Copy link
Collaborator Author

kv2019i commented Dec 12, 2024

New version pushed:

  • fixed the stub/fuzzer build fails (and cleaned up the posix DMA driver implementation)
  • pivot on the approach -> not touching the XTOS drivers in this version anymore, just modify the common code and add minimal compatibility definitions to xtos/lib/dma.h (really only <10 uses in generic code so)
  • UPDATE: fuzzer/posix works but testbench now fails. marking with DNM, but please review the approach

The main idea is to push ugly compatibility code to layers that will be removed later (with Zephyr transition) and keep the "longterm code" clean.

E.g. now it's easy to see in src/audio/host-zephyr.c that SOF_DMA_DEV_HOST is a SOF definition while DMA_ATTR_BUFFER_ADDRESS_ALIGNMENT is coming from Zephyr dma.h.

@kv2019i kv2019i added the DNM Do Not Merge tag label Dec 12, 2024
@kv2019i kv2019i force-pushed the 202412-dma-namespacing-part1 branch from a74317f to b2eb734 Compare December 12, 2024 19:48
@kv2019i
Copy link
Collaborator Author

kv2019i commented Dec 12, 2024

New version:

  • issue with testbench solved (compatibility also added to posix lib/dma.h

@kv2019i kv2019i removed the DNM Do Not Merge tag label Dec 12, 2024

int (*probe)(struct dma *dma);
int (*remove)(struct dma *dma);
struct dma_chan_data {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a reason for a new revision, but if you do end up doing another update - would it be possible to keep this structure at the same place as now to reduce the diff?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh Ack, fixed in new revision.

#define SOF_DMA_DIR_LMEM_TO_HMEM DMA_DIR_LMEM_TO_HMEM
#define SOF_DMA_DIR_MEM_TO_DEV DMA_DIR_MEM_TO_DEV
#define SOF_DMA_DIR_DEV_TO_MEM DMA_DIR_DEV_TO_MEM
#define SOF_DMA_DIR_DEV_TO_DEV DMA_DIR_DEV_TO_DEV
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, maybe the compiler doesn't care, but wouldn't it be easier for the humans to have these under right-hand definitions just below? More cases below

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh Hmm, ok, let me make a rare concession for the humans among us and change the order. Addressed in the updated version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry, I made the changes to xtos/lib/dma.h. Let me make the same to posix/lib/dma.h as well. Will make a new update in a minute.

Move parts of the dma.h interface only needed by XTOS drivers to a
separate dma-legacy.h file. Use of non-native drivers is a transitory
tool and used by a subset of targets. To make the code easier to read
and maintain, the main dma.h should only require definitions needed when
building SOF Zephyr with native Zephyr drivers
(CONFIG_ZEPHYR_NATIVE_DRIVERS). This is especially important for DMA as
there is lot of similarly named definitions and interfaces related to
DMA in Zephyr and SOF.

Signed-off-by: Kai Vehmanen <[email protected]>
Add a namespace prefix to DMA_ macro definitions in the zephyr/lib/dma.h.
This change makes it easier to identify what code is using native Zephyr
definitions (DMA_*) and which are SOF side definition (SOF_DMA_*).

To support non-Zephyr targets, add a minimimal compatibility layer
to xtos/lib/dma.h and posix/lib/dma.h. This only covers a few definitions
needed by e.g. ipc/ipc[34]/dai.c.

To support Zephyr targets that still use XTOS drivers (like imx8m),
add compatibility definitions to zephyr/lib/dma-legacy.h. It will
be later easy to drop dma-legacy.h when all targets have moved
over.

Signed-off-by: Kai Vehmanen <[email protected]>
@kv2019i kv2019i force-pushed the 202412-dma-namespacing-part1 branch from b2eb734 to cdc1442 Compare December 13, 2024 15:02
@kv2019i
Copy link
Collaborator Author

kv2019i commented Dec 13, 2024

New version:

  • fixed build error in "instaler / checktree" CI run
  • address Guennadi's comments (no fucntional change, reordering defintions)

@kv2019i kv2019i force-pushed the 202412-dma-namespacing-part1 branch from cdc1442 to e51e2d2 Compare December 13, 2024 15:09
@kv2019i
Copy link
Collaborator Author

kv2019i commented Dec 13, 2024

New version:

  • fixed Guennadi's comment also in posix/lib/dma.h

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants