Skip to content

hda: hw support for hda chain - #6656

Merged
mwasko merged 5 commits into
thesofproject:mainfrom
makarukp:hw_hda_chain_support
Jan 19, 2023
Merged

hda: hw support for hda chain#6656
mwasko merged 5 commits into
thesofproject:mainfrom
makarukp:hw_hda_chain_support

Conversation

@makarukp

@makarukpmakarukp commented Nov 23, 2022

Copy link
Copy Markdown
Contributor

This PR enables hardware support for HDA chain transfers (without deploying internal pipelines):
Content:

  • hda chain management
  • hda chain tasks and DMA logic implementation

Signed-off-by: Piotr Makaruk piotr.makaruk@intel.com

Comment threadsrc/audio/pipeline/chain_dma.c Outdated
Comment threadsrc/audio/pipeline/chain_dma.c Outdated
Comment threadsrc/audio/pipeline/chain_dma.c Outdated
Comment threadsrc/include/sof/audio/chain_dma.h Outdated
Comment threadsrc/include/sof/audio/chain_dma.h Outdated
Comment threadsrc/ipc/ipc4/helper.c Outdated
Comment threadsrc/ipc/ipc4/helper.c Outdated
Comment threadsrc/ipc/ipc4/helper.c Outdated
Comment threadsrc/ipc/ipc4/helper.c Outdated
Comment threadsrc/audio/pipeline/chain_dma.c Outdated
Comment threadsrc/include/sof/audio/chain_dma.h Outdated
Comment threadzephyr/CMakeLists.txt Outdated
Comment threadsrc/audio/pipeline/chain_dma.c Outdated
Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/audio/chain_dma.c Outdated
@lgirdwood

Copy link
Copy Markdown
Member

@makarukp is this PR still pending validation or implementation completion or external dependencies ?

Comment threadsrc/audio/chain_dma.c Outdated
@makarukp

Copy link
Copy Markdown
ContributorAuthor

@lgirdwood This PR is still under development, it requires at lease one more commit with code completion before it is ready for validation.

@makarukp
makarukp marked this pull request as ready for review December 14, 2022 14:24
@makarukpmakarukp changed the title [DO NOT MERGE]hda: hw support for hda chainhda: hw support for hda chainDec 15, 2022
@makarukp

Copy link
Copy Markdown
ContributorAuthor

Code is completed. PR is ready for validation.

@makarukp
makarukpforce-pushed the hw_hda_chain_support branch 2 times, most recently from 896317d to 6eb4ac3CompareJanuary 12, 2023 13:57
Comment threadsrc/include/sof/audio/chain_dma.h Outdated
Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/audio/chain_dma.c Outdated

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.

yeah, this looks a bit suspicious. And in fact dw_dma_get_status() can return -EINVAL if the channel number is invalid, and that is a hard error and in that case stat isn't filled. So, this at least deserves a comment, much more so than the two comments above which explain something, that's pretty clear from the code itself.

Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/include/sof/audio/chain_dma.h Outdated
Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/ipc/ipc4/helper.c Outdated
Comment threadsrc/ipc/ipc4/handler.c Outdated
@makarukp

makarukp commented Jan 13, 2023

Copy link
Copy Markdown
ContributorAuthor

@lyakh What do you think about such approach of critical error handling in response to 'dma_get_status()' ?

/* Status can return -ENODATA and current status if xrun occurs, then it is not critical
* and flow shall continue. In worst case, when channel index exceeds max value, status can
* return -EINVAL, then it is critical error.
*/
ret = dma_get_status(cd->chan_link->dma->z_dev, cd->chan_link->index, &stat);
switch (ret) {
case 0:
break;
case -ENODATA:
tr_warn(&chain_dma_tr, "chain_task_run(): dma_get_status() xrun occured, ret = %u",
ret);
break;
default:
tr_err(&chain_dma_tr, "chain_task_run(): dma_get_status() error, ret = %u", ret);
return SOF_TASK_STATE_COMPLETED;
}

@lyakh

Copy link
Copy Markdown
Collaborator

Status can return -ENODATA and current status if xrun occurs

According to https://www.alsa-project.org/alsa-doc/alsa-lib/pcm.html-EPIPE is used by ALSA to indicate an xrun?

@makarukp

Copy link
Copy Markdown
ContributorAuthor

@lyakh Accually -EINVAL can be returned in current implementation only by dw_dma_get_status() which is used by GPDMA in Zephyr. DMA uses intel_adsp_hda_dma_status() and there is only assertion __ASSERT(channel < cfg->dma_channels, "Channel does not exist"); .

@makarukp
makarukpforce-pushed the hw_hda_chain_support branch from 6eb4ac3 to 36e1c70CompareJanuary 16, 2023 11:48

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

This starts to look good. A few minor issues, but no showstoppers.

Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/ipc/ipc4/handler.c Outdated
Comment threadapp/boards/intel_adsp_cavs25_tgph.conf Outdated
@makarukp
makarukpforce-pushed the hw_hda_chain_support branch from 36e1c70 to ca41ffaCompareJanuary 16, 2023 12:38
Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/audio/chain_dma.c Outdated
Comment threadsrc/ipc/ipc4/handler.c Outdated

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.

ipc4_chain_manager_create() seems to be a better name for this, but it's already there

Hardware HDA chain require manager for controlling multiple streams tasks.
Enable support with LL task and DMA mandatory logic.
Signed-off-by: Piotr Makaruk <piotr.makaruk@intel.com>
HW chain dma support require new logic in handler
Signed-off-by: Piotr Makaruk <piotr.makaruk@intel.com>
Enable building chain dma component on TGL platform
Signed-off-by: Piotr Makaruk <piotr.makaruk@intel.com>
Enable building chain dma component on MTL platform
Signed-off-by: Piotr Makaruk <piotr.makaruk@intel.com>
Enable building chain dma component on TGL-H platform
Signed-off-by: Piotr Makaruk <piotr.makaruk@intel.com>
@makarukp
makarukpforce-pushed the hw_hda_chain_support branch from ca41ffa to 414a12cCompareJanuary 17, 2023 08:13
Comment threadsrc/audio/chain_dma.c Outdated

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.

@lyakh this looks resolved now, error code is checked ?

Comment threadsrc/audio/chain_dma.c Outdated

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.

@ujfalusi resolved ?

@marc-hb

Copy link
Copy Markdown
Collaborator

This apparently caused regressions #6975 and #6976.

It was also merged while failing a large number of tests: https://sof-ci.01.org/sofpr/PR6656/build3409/devicetest/index.html

@lgirdwood

Copy link
Copy Markdown
Member

There is a lot of test failures on ADL capture, I've created a revert PR that can be merged today prior to daily test runs if we don't have fixes ready in time.

@lgirdwood

Copy link
Copy Markdown
Member

@makarukp@mwasko@kv2019i@lyakh fyi - does anyone have fix PRs in progress otherwise we will have to revert ?

@mwasko

Copy link
Copy Markdown
Contributor

@makarukp@mwasko@kv2019i@lyakh fyi - does anyone have fix PRs in progress otherwise we will have to revert ?

@lgirdwood, @kv2019i the regression is affecting only cAVS targets which are not POR in Zephyr configuration. The tests were passing before on cAVS because they were using w/a in form of copier-copier solution which is not correct implementation. We should treat it as we have enabled feature for MTL and for cAVS it is a to do action (on best effort basis).

@lgirdwood

Copy link
Copy Markdown
Member

@makarukp@mwasko@kv2019i@lyakh fyi - does anyone have fix PRs in progress otherwise we will have to revert ?

@lgirdwood, @kv2019i the regression is affecting only cAVS targets which are not POR in Zephyr configuration. The tests were passing before on cAVS because they were using w/a in form of copier-copier solution which is not correct implementation. We should treat it as we have enabled feature for MTL and for cAVS it is a to do action (on best effort basis).

I'm fine to make this a MTL only feature, but it should not break legacy features on other platforms. Can this be fixed with a Kconfig ?

@makarukp

makarukp commented Jan 20, 2023

Copy link
Copy Markdown
ContributorAuthor

@makarukp@mwasko@kv2019i@lyakh fyi - does anyone have fix PRs in progress otherwise we will have to revert ?

@lgirdwood, @kv2019i the regression is affecting only cAVS targets which are not POR in Zephyr configuration. The tests were passing before on cAVS because they were using w/a in form of copier-copier solution which is not correct implementation. We should treat it as we have enabled feature for MTL and for cAVS it is a to do action (on best effort basis).

I'm fine to make this a MTL only feature, but it should not break legacy features on other platforms. Can this be fixed with a Kconfig ?

@lgirdwood Unfortunately it is not possible to fix it with Kconfig because legacy approach was erased by hw chain dma PR.

As for #6975 (comment) issue it does not look like a big deal. I will prepare debug FW which could probably solve that problem or provide at least more information.

As for #6976 (comment) issue I don't know what happened and still thinking about further steps for debug ...

@makarukp

Copy link
Copy Markdown
ContributorAuthor

I prepared fix for #6975:
#6983

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.

12 participants

@makarukp@lgirdwood@RanderWang@marc-hb@lyakh@mwasko@ujfalusi@dabekjakub@kv2019i@marcinszkudlinski@tlissows@softwarecki