Skip to content

basefw: Add handling of IPC4_DMA_CONTROL messages - #9156

Merged
kv2019i merged 2 commits into
thesofproject:mainfrom
tmleman:topic/upstream/pr/basefw/dma_constrol
Sep 6, 2024
Merged

basefw: Add handling of IPC4_DMA_CONTROL messages#9156
kv2019i merged 2 commits into
thesofproject:mainfrom
tmleman:topic/upstream/pr/basefw/dma_constrol

Conversation

@tmleman

@tmlemantmleman commented May 22, 2024

Copy link
Copy Markdown
Contributor

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:

Comment threadsrc/lib/dai.c Outdated
Comment threadsrc/audio/base_fw.c Outdated
Comment threadsrc/audio/base_fw_intel.c Outdated
Comment threadsrc/audio/base_fw_intel.c Outdated
@tmleman
tmlemanforce-pushed the topic/upstream/pr/basefw/dma_constrol branch 2 times, most recently from 8b66f23 to d03015eCompareMay 24, 2024 10:30
@tmlemantmleman changed the title basefw: Add handling of IPC4_DMA_CONTROL messages[DNM] basefw: Add handling of IPC4_DMA_CONTROL messagesMay 24, 2024
@tmleman
tmleman requested a review from lyakhMay 24, 2024 11:15

@lyakhlyakh left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
ContributorAuthor

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 lyakhJune 5, 2024 11:28
@lgirdwood

Copy link
Copy Markdown
Member

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

@lgirdwoodlgirdwood added this to the v2.11 milestone Jun 25, 2024
Comment threadsrc/audio/base_fw_intel.c Outdated
@lgirdwood

Copy link
Copy Markdown
Member

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

@tmleman

Copy link
Copy Markdown
ContributorAuthor

I guess we are blocked until after Zephyr merge Window ?

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

@tmleman
tmlemanforce-pushed the topic/upstream/pr/basefw/dma_constrol branch from 5c45e51 to 913972eCompareJuly 8, 2024 11:34
@lgirdwood

Copy link
Copy Markdown
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.

@iganakoviganakov left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadsrc/audio/base_fw.c Outdated
Comment threadsrc/audio/base_fw_intel.c Outdated
@iganakoviganakov mentioned this pull request Jul 23, 2024
@tmleman
tmlemanforce-pushed the topic/upstream/pr/basefw/dma_constrol branch from 913972e to 3801123CompareJuly 29, 2024 13:40
@tmlemantmleman changed the title [DNM] basefw: Add handling of IPC4_DMA_CONTROL messagesbasefw: Add handling of IPC4_DMA_CONTROL messagesJul 29, 2024
@tmleman
tmleman requested a review from iganakovJuly 29, 2024 13:45
Comment threadsrc/audio/base_fw.c Outdated
}

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

Comment threadsrc/audio/base_fw.c
@tmleman
tmlemanforce-pushed the topic/upstream/pr/basefw/dma_constrol branch from 3801123 to 1d8d32fCompareAugust 6, 2024 07:01
@tmleman

Copy link
Copy Markdown
ContributorAuthor

Rebase

@lgirdwoodlgirdwood left a comment

Copy link
Copy Markdown
Member

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

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment threadsrc/audio/base_fw.c Outdated
}

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

Copy link
Copy Markdown
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?

Comment threadsrc/audio/base_fw_intel.c Outdated
Comment threadsrc/audio/base_fw_intel.c Outdated
Comment threadsrc/audio/base_fw.c Outdated
@tmleman
tmlemanforce-pushed the topic/upstream/pr/basefw/dma_constrol branch 4 times, most recently from 0673918 to 8359eb5CompareAugust 7, 2024 14:23
Comment threadsrc/audio/base_fw.c Outdated
@tmleman
tmlemanforce-pushed the topic/upstream/pr/basefw/dma_constrol branch from 8359eb5 to 3fa5277CompareAugust 9, 2024 15:57
@tmleman
tmleman requested a review from tlissowsAugust 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>

@iganakoviganakov left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Collaborator

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

@kv2019i

Copy link
Copy Markdown
Collaborator

SOFCI TEST

@kv2019i
kv2019i merged commit f9575cd into thesofproject:mainSep 6, 2024
Sign up for freeto 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

@tmleman@lgirdwood@kv2019i@lyakh@tlissows@iganakov@softwarecki