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

[DNM] dai: add support for Intel UAOL DAI #9227

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

Conversation

tlissows
Copy link
Contributor

@tlissows tlissows commented Jun 14, 2024

This adds support for Intel USB Audio Offload Link (UAOL) DAI.

This PR needs another (Zephyr) PR drivers: dai: add DAI driver for Intel UAOL to be merged first.

src/audio/base_fw_intel.c 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
src/audio/copier/copier_dai.c Outdated Show resolved Hide resolved
src/lib/dai.c Show resolved Hide resolved
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

Not following the directions, this doesn't seem aligned with what we previously did for ALH / SoundWire.

#else
channel = copier_cfg->gtw_cfg.node_id.f.v_index;
#endif
break;
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand this.

for LNL+ all the DMA stuff is handled in the same way with an HDaudio-based solution.

We just moved some of the ALH stuff into the HDaudio prodessing - see line 94 the test for ALH.

so shouldn't the UAOL stuff also be moved under the HDAudio category? Why special case what looks identical?

Copy link
Member

Choose a reason for hiding this comment

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

my comment still stands.

Copy link
Member

Choose a reason for hiding this comment

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

@tlissows any update here ?

@tlissows tlissows changed the title dai: add support for Intel UAOL DAI [DNM] dai: add support for Intel UAOL DAI Jul 19, 2024
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

we really need to make UAOL support dependent on a version of silicon (pick one), e.g. ACE2.0, and not add code that will never be used on previous generations.

There's really no point in supporting anything older than ACE2.0?

src/ipc/ipc4/dai.c Outdated Show resolved Hide resolved
#else
channel = copier_cfg->gtw_cfg.node_id.f.v_index;
#endif
break;
Copy link
Member

Choose a reason for hiding this comment

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

my comment still stands.

src/audio/copier/copier_dai.c Outdated Show resolved Hide resolved
src/lib/dai.c Outdated Show resolved Hide resolved
@lgirdwood
Copy link
Member

@tlissows ping ?

Currently, the dai_set_config() function is called with copier gateway
config data passed but without the config length, possibly making the
config not parsable for some DAI drivers. This patch adds passing of the
full ipc4_copier_gateway_cfg structure, giving DAI drivers the ability
to access config length if needed.

Signed-off-by: Tomasz Lissowski <tomasz.lissowski@intel.com>
Add strict use of dai_type enum items as a call parameter for
dai_get_device() function. Currently, both the dai_type and
sof_ipc_dai_type items are used inconsistently.

Signed-off-by: Tomasz Lissowski <tomasz.lissowski@intel.com>
This adds support for Intel USB Audio Offload Link (UAOL) DAI.

Signed-off-by: Tomasz Lissowski <tomasz.lissowski@intel.com>
@tlissows
Copy link
Contributor Author

tlissows commented Oct 2, 2024

Removed support for ACE1.x platform from this PR. Also aligned to the changes introduced by PR basefw: Add handling of IPC4_DMA_CONTROL messages.

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.

@tlissows I assume still undergoing testing hence the DNM ? Pls also respond to open from @plbossart

@@ -153,25 +154,25 @@ int dai_set_config(struct dai *dai, struct ipc_config_dai *common_config,
switch (common_config->type) {
case SOF_DAI_INTEL_SSP:
cfg.type = is_blob ? DAI_INTEL_SSP_NHLT : DAI_INTEL_SSP;
cfg_params = is_blob ? spec_config : &sof_cfg->ssp;
cfg_params = is_blob ? (void *)&gtw_cfg->config_data : &sof_cfg->ssp;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the void* cast here or is the compiler throwing up a warning in this case ? cfg_params is also const too.


static void tlv_value_set_uaol_caps(struct sof_tlv *tuple, uint32_t type)
{
unsigned int dev_count = ARRAY_SIZE(uaol_devs);
Copy link
Member

Choose a reason for hiding this comment

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

const ?

#else
channel = copier_cfg->gtw_cfg.node_id.f.v_index;
#endif
break;
Copy link
Member

Choose a reason for hiding this comment

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

@tlissows any update here ?

Copy link
Contributor

@tmleman tmleman left a comment

Choose a reason for hiding this comment

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

I have no remarks regarding the code, but I would like to see that the whole thing passes through CI. There is a temporary commit missing here that would indicate the Zephyr version from your second pull request:

--- a/west.yml
+++ b/west.yml
@@ -43,7 +43,7 @@ manifest:

     - name: zephyr
       repo-path: zephyr
-      revision: 99e6280d7e22552de9a94992b626acdcbde00fee
+      revision: pull/69906/head
       remote: zephyrproject

       # Import some projects listed in zephyr/west.yml@revision

@lgirdwood
Copy link
Member

@tlissows @tmleman it looks like this is good to go once it passes CI - I think @plbossart comments have now been addressed. Are we blocking on any Zephyr PRs before we can merge ?

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.

6 participants