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.

Sign up for GitHub

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

Fix reverse dai trigger config and disable CONFIG_DMA_DW_SUSPEND_DRAIN for mtl as well #8835

Merged
merged 2 commits into from
Feb 9, 2024
Merged

Fix reverse dai trigger config and disable CONFIG_DMA_DW_SUSPEND_DRAIN for mtl as well #8835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3506304
audio: dai-zephyr: Fix config option string for reversed trigger
ujfalusi Feb 5, 2024
83fb77c
board: intel_adsp_ace15_mtpm: Drop CONFIG_DMA_DW_SUSPEND_DRAIN
ujfalusi Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion app/boards/intel_adsp_ace15_mtpm.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ CONFIG_ZEPHYR_DP_SCHEDULER=y
CONFIG_DMA=y
CONFIG_DMA_INTEL_ADSP_GPDMA=y
CONFIG_DMA_DW_LLI_POOL_SIZE=50
CONFIG_DMA_DW_SUSPEND_DRAIN=y
CONFIG_INTEL_MODULES=y
CONFIG_LIBRARY_MANAGER=y
CONFIG_LIBRARY_AUTH_SUPPORT=y
Expand Down
4 changes: 2 additions & 2 deletions src/audio/dai-zephyr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1108,7 +1108,7 @@ static int dai_comp_trigger_internal(struct dai_data *dd, struct comp_dev *dev,
* drain the FIFO in order to stop the channel
* as soon as possible.
*/
#if CONFIG_COMP_DAI_TRIGGER_ORDER_REVERSE
#if CONFIG_COMP_DAI_STOP_TRIGGER_ORDER_REVERSE
ret = dma_stop(dd->chan->dma->z_dev, dd->chan->index);
dai_trigger_op(dd->dai, cmd, dev->direction);
#else
Expand All @@ -1122,7 +1122,7 @@ static int dai_comp_trigger_internal(struct dai_data *dd, struct comp_dev *dev,
break;
case COMP_TRIGGER_PAUSE:
comp_dbg(dev, "dai_comp_trigger_internal(), PAUSE");
#if CONFIG_COMP_DAI_TRIGGER_ORDER_REVERSE
#if CONFIG_COMP_DAI_STOP_TRIGGER_ORDER_REVERSE
Copy link
Member

Choose a reason for hiding this comment

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

@ujfalusi is this saying that a6a80ec ("dai-zephyr: Fix the ordering of DAI and DMA triggers") only fixed the start trigger but kept the behavior for stop the same as before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That commit added the Kconfig:
config COMP_DAI_**STOP_**TRIGGER_ORDER_REVERSE

and in code it used:
#if CONFIG_COMP_DAI_TRIGGER_ORDER_REVERSE

This patch corrects that config option use in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ranj063, @plbossart, the drain issue on UPX is not the same, hw:0,1 does not work at all, it always fail, but it allowed me to have a fix for the handling of such.

I still think that this PR will help the MTL issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ujfalusi can you please remind me what this kconfig CONFIG_DMA_DW_SUSPEND_DRAIN is meant for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ranj063, in case you want to stop the DMA without loosing data in it's FIFO (we don't afaik) then the recommended stop sequence is:
SUSPEND+DRAIN and wait for drain to complete
Clear the EN bit to disable the channel

In case of pause you would need only toggle the SUSPEND bit.

There is no case documented when you just want to disable the channel and you don't care about the FIFO, but it is afaik is the clearing of EN (the FIFO will be lost).

Imho the correct fix would be to not enable CONFIG_DMA_DW_SUSPEND_DRAIN for any Intel platform.

ret = dma_suspend(dd->chan->dma->z_dev, dd->chan->index);
dai_trigger_op(dd->dai, cmd, dev->direction);
#else
Expand Down
Loading