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

basefw: Add handling of IPC4_DMA_CONTROL messages #9156

Merged

Conversation

tmleman
Copy link
Contributor

@tmleman tmleman commented May 22, 2024

This pull request introduces a handling of IPC4_DMA_CONTROL messages for the SSP DAI driver.

Below is a summary of the commits included in this pull request:

  • 64ce0b9 Exposes the function to retrieve Zephyr DAI device structures, allowing other parts of the SOF codebase to access Zephyr native DAI drivers.
  • 3801123: Implements handling for the IPC4_DMA_CONTROL message within the base firmware.

TODO before merge:

src/lib/dai.c Outdated Show resolved Hide resolved
src/audio/base_fw.c Outdated Show resolved Hide resolved
src/audio/base_fw_intel.c Outdated Show resolved Hide resolved
src/audio/base_fw_intel.c Outdated Show resolved Hide resolved
@tmleman tmleman force-pushed the topic/upstream/pr/basefw/dma_constrol branch 2 times, most recently from 8b66f23 to d03015e Compare May 24, 2024 10:30
@tmleman tmleman changed the title basefw: Add handling of IPC4_DMA_CONTROL messages [DNM] basefw: Add handling of IPC4_DMA_CONTROL messages May 24, 2024
@tmleman tmleman requested a review from lyakh May 24, 2024 11:15
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.

Thanks for addressing the comments! Formally the PR looks good to me now, although I still don't know why exactly this is needed - "improving the flexibility" doesn't tell me much, sorry :-) Would be nice to know what specific use-cases this is addressing.

@tmleman
Copy link
Contributor Author

tmleman commented Jun 5, 2024

I still don't know why exactly this is needed - "improving the flexibility" doesn't tell me much

The implementation of the IPC4_DMA_CONTROL message handling is a requirement for SOF to align with the IPC4 interface specifications, which are part of the convergence effort with Windows audio infrastructure. This specific IPC message allows for dynamic configuration of DMA gateways, which is essential for use-cases where audio streams need to be routed between various components in a flexible manner, adapting to changes in audio topologies at runtime.

For instance, this capability is crucial for scenarios where audio endpoints can be added or removed dynamically, or where stream parameters may change on-the-fly without restarting the system or audio pipeline. It enables SOF to handle complex audio routing scenarios that are common in modern Windows-based systems, ensuring compatibility and feature parity with the Windows audio stack.

@lyakh Please also take a look at the changes on the Zephyr side that are required for this PR.

@tmleman tmleman requested a review from lyakh June 5, 2024 11:28
@lgirdwood
Copy link
Member

@tmleman any update on ETA ? Do you have the Zephyr update now merged ?

@lgirdwood lgirdwood added this to the v2.11 milestone Jun 25, 2024
src/audio/base_fw_intel.c Outdated Show resolved Hide resolved
@lgirdwood
Copy link
Member

@tmleman I guess we are blocked until after Zephyr merge Window ?

@tmleman
Copy link
Contributor Author

tmleman commented Jul 8, 2024

I guess we are blocked until after Zephyr merge Window ?

@lgirdwood Yes, this change is blocked until the release of Zephyr LTS.

@tmleman tmleman force-pushed the topic/upstream/pr/basefw/dma_constrol branch from 5c45e51 to 913972e Compare July 8, 2024 11:34
@lgirdwood
Copy link
Member

I guess we are blocked until after Zephyr merge Window ?

@lgirdwood Yes, this change is blocked until the release of Zephyr LTS.

ok, tagged for v2.11 to remind everyone.

Copy link
Contributor

@iganakov iganakov left a comment

Choose a reason for hiding this comment

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

Please use IPC4 error codes

src/audio/base_fw.c Outdated Show resolved Hide resolved
src/audio/base_fw_intel.c Outdated Show resolved Hide resolved
@tmleman tmleman force-pushed the topic/upstream/pr/basefw/dma_constrol branch from 913972e to 3801123 Compare July 29, 2024 13:40
@tmleman tmleman changed the title [DNM] basefw: Add handling of IPC4_DMA_CONTROL messages basefw: Add handling of IPC4_DMA_CONTROL messages Jul 29, 2024
@tmleman tmleman requested a review from iganakov July 29, 2024 13:45
}

dma_control = (struct ipc4_dma_control *)data;
if (dma_control->config_length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

From my perspective this check is wrong and should be removed or modified to
if (!dma_control->config_length).
config_length cannot be zero if DMA Control IPC has config data and contains the size of such data in number of dw. At least it is true for DMIC and UAOL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tmleman can you comment, is the config_length used or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And from my perspective, it was correct (however, I focused only on SSP during the implementation). I looked at how it looks in the reference fw and changed the logic in the IF. Now the error is returned when config_length is greater than the actual data size. The condition you proposed will not work for SSP because in all cases I was able to check, this parameter is equal to zero, which is consistent with the description in the documentation.

src/audio/base_fw.c Show resolved Hide resolved
@tmleman tmleman force-pushed the topic/upstream/pr/basefw/dma_constrol branch from 3801123 to 1d8d32f Compare August 6, 2024 07:01
@tmleman
Copy link
Contributor Author

tmleman commented Aug 6, 2024

Rebase

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, but lets get alignment with @iganakov

Copy link
Collaborator

@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.

Looks good to me. If you can conclude the one discussion with @iganakov w.r.t. dma_control->config_length, this is good to go.

}

dma_control = (struct ipc4_dma_control *)data;
if (dma_control->config_length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@tmleman can you comment, is the config_length used or not?

src/audio/base_fw_intel.c Outdated Show resolved Hide resolved
src/audio/base_fw_intel.c Outdated Show resolved Hide resolved
src/audio/base_fw.c Outdated Show resolved Hide resolved
@tmleman tmleman force-pushed the topic/upstream/pr/basefw/dma_constrol branch 4 times, most recently from 0673918 to 8359eb5 Compare August 7, 2024 14:23
src/audio/base_fw.c Outdated Show resolved Hide resolved
@tmleman tmleman force-pushed the topic/upstream/pr/basefw/dma_constrol branch from 8359eb5 to 3fa5277 Compare August 9, 2024 15:57
@tmleman tmleman requested a review from tlissows August 9, 2024 15:59
This patch exposes the function to retrieve a pointer to the Zephyr
device structure for a DAI of a given type and index. Previously, the
function `dai_get_zephyr_device` was static and only usable within
`dai.c`. By introducing `dai_get_device`, other parts of the SOF
codebase can now access the Zephyr DAI device pointers, facilitating
integration with Zephyr native DAI drivers.

Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
This patch introduces handling for the IPC4_DMA_CONTROL message type in
the base firmware. The implementation includes a new function
`basefw_vendor_dma_control` to process the DMA Control configuration for
any DAI type.

The `basefw_dma_control` function has been added to handle the
IPC4_DMA_CONTROL message. It ensures the message is atomic and contains
all necessary information before casting the data buffer to the
`ipc4_dma_control` structure and processing it. The function also calls
`basefw_vendor_dma_control` to apply the DMA Control configuration to
the hardware.

The `basefw_set_large_config` function in `src/audio/base_fw.c` has been
updated to call `basefw_dma_control` when an IPC4_DMA_CONTROL message is
received. If the `dai_config_update` operation is not implemented by the
DAI driver, the function will return `-ENOSYS`.

This change allows the base firmware to initialize or modify DMA gateway
configurations dynamically, improving the flexibility of DMA management
in response to IPC messages.

Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
Copy link
Contributor

@iganakov iganakov left a comment

Choose a reason for hiding this comment

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

Looks good to me

@kv2019i
Copy link
Collaborator

kv2019i commented Sep 5, 2024

Ready to go, but we need a clean pass of mandatory tests.

@kv2019i
Copy link
Collaborator

kv2019i commented Sep 5, 2024

SOFCI TEST

@kv2019i kv2019i merged commit f9575cd into thesofproject:main Sep 6, 2024
45 of 47 checks passed
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.

7 participants