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

[RFC] treewide: zephyr: add sof_ prefix to dma_get/put() and struct dma #9728

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kv2019i
Copy link
Collaborator

@kv2019i kv2019i commented Dec 16, 2024

Add "sof_" namespace to dma_get()/put() in the Zephyr native drivers builds. The main "struct dma" is also renamed, so this touches quite many places in code.

Link: #9561

Add "sof_" namespace to dma_get()/put() in the Zephyr native
drivers builds. The main "struct dma" is also renamed, so this touches
quite many places in code.

Signed-off-by: Kai Vehmanen <[email protected]>
Copy link
Collaborator Author

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Marking this as RFC. Not sure yet whether this is a net improvement given the level of code acrobatics needed to keep all targets compiling. But let me know what you think.

High-level logic remains the same. Target is to make code in src/audio/ is easier to read, even if at expense of uglier code in the RTOS adaption.

#if CONFIG_ZEPHYR_NATIVE_DRIVERS
iipc->dh_buffer.dmac = sof_dma_get(SOF_DMA_DIR_HMEM_TO_LMEM, 0, SOF_DMA_DEV_HOST,
SOF_DMA_ACCESS_SHARED);
#else
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This can be removed with #9608 .

#else
typedef struct dma sof_dma;
#endif

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK I admit this is starting to get (too?) ugly. This is really only needed for the few cases where SOF Zephyr is built with using XTOS drivers. One alternative is to convert the naming also in all XTOS code, but it is quite massive amount of change as this affects most drivers and most platform code.

Copy link
Member

Choose a reason for hiding this comment

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

Its going to be short term, so will be removed at some point.

if (!cd->dma_link) {
dma_put(cd->dma_host);
sof_dma_put(cd->dma_host);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good example of code where the namespacing helps readability. If you expand this function you can see calls to e.g. dma_get_attribute() (which is a Zephyr DMA call).

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.

I'm assuming this fixes some naming collisions we are seeing now as we more tightly bind with Zephyr ?

#else
typedef struct dma sof_dma;
#endif

Copy link
Member

Choose a reason for hiding this comment

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

Its going to be short term, so will be removed at some point.

@kv2019i
Copy link
Collaborator Author

kv2019i commented Dec 17, 2024

@lgirdwood wrote:

I'm assuming this fixes some naming collisions we are seeing now as we more tightly bind with Zephyr ?

I think it's a fair argument that the practical gains are not big. Neither Zephyr or SOF DMA interfaces see a lot of change (at this point at least), so actual risk of conflicts (that would show up as build errors), is small. So this is in the end about readability of code like:

	dma_release_channel(cd->chan_host->dma->z_dev, cd->chan_host->index);
-	dma_put(cd->dma_host);
+	sof_dma_put(cd->dma_host);
	dma_release_channel(cd->chan_link->dma->z_dev, cd->chan_link->index);
-	dma_put(cd->dma_link);
+	sof_dma_put(cd->dma_link);

Having the prefix makes this immediately easier to follow and see which calls are calls to Zephyr and which are SOF side calls (sof_dma_get() is a simple helper on top of device-tree interfaces to find the correct DMA to use).

But whether the readibility improvement is worth the cruft this PR brings in (like struct typedefs), that's another question. Other option is to wait until we can drop the XTOS support and clean at that time (which will be a lot easier as no need to keep various XTOS and Zephyr/XTOS hybrid build targets working anymore).

@kv2019i
Copy link
Collaborator Author

kv2019i commented Dec 20, 2024

Parts of this PR submitted as non-RFC as #9753

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.

2 participants