Skip to content

dma: disable hda when pipeline is reset #7439

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

Merged
mwasko merged 1 commit into from
May 12, 2023
Merged

dma: disable hda when pipeline is reset #7439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 5 additions & 18 deletions src/audio/dai-zephyr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,7 +1033,6 @@ static int dai_reset(struct comp_dev *dev)
/* used to pass standard and bespoke command (with data) to component */
static int dai_comp_trigger_internal(struct dai_data *dd, struct comp_dev *dev, int cmd)
{
int prev_state = dev->state;
int ret;

comp_dbg(dev, "dai_comp_trigger_internal(), command = %u", cmd);
Expand Down Expand Up @@ -1079,11 +1078,9 @@ static int dai_comp_trigger_internal(struct dai_data *dd, struct comp_dev *dev,
/* only start the DAI if we are not XRUN handling */
if (dd->xrun == 0) {
/* recover valid start position */
if (dev->state == COMP_STATE_ACTIVE) {
ret = dma_stop(dd->chan->dma->z_dev, dd->chan->index);
if (ret < 0)
return ret;
}
ret = dma_stop(dd->chan->dma->z_dev, dd->chan->index);
if (ret < 0)
return ret;

/* dma_config needed after stop */
ret = dma_config(dd->chan->dma->z_dev, dd->chan->index, dd->z_config);
Expand Down Expand Up @@ -1132,21 +1129,11 @@ static int dai_comp_trigger_internal(struct dai_data *dd, struct comp_dev *dev,
case COMP_TRIGGER_PAUSE:
comp_dbg(dev, "dai_comp_trigger_internal(), PAUSE");
#if CONFIG_COMP_DAI_TRIGGER_ORDER_REVERSE
if (prev_state == COMP_STATE_ACTIVE) {
ret = dma_suspend(dd->chan->dma->z_dev, dd->chan->index);
} else {
comp_warn(dev, "dma was stopped earlier");
ret = 0;
}
ret = dma_suspend(dd->chan->dma->z_dev, dd->chan->index);
dai_trigger_op(dd->dai, cmd, dev->direction);
#else
dai_trigger_op(dd->dai, cmd, dev->direction);
if (prev_state == COMP_STATE_ACTIVE) {
ret = dma_suspend(dd->chan->dma->z_dev, dd->chan->index);
} else {
comp_warn(dev, "dma was stopped earlier");
ret = 0;
}
ret = dma_suspend(dd->chan->dma->z_dev, dd->chan->index);
#endif
break;
case COMP_TRIGGER_PRE_START:
Expand Down
14 changes: 5 additions & 9 deletions src/audio/host-zephyr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,13 +557,10 @@ int host_zephyr_trigger(struct host_data *hd, struct comp_dev *dev, int cmd)
break;
case COMP_TRIGGER_STOP:
case COMP_TRIGGER_XRUN:
if (dev->state == COMP_STATE_ACTIVE) {
ret = dma_stop(hd->chan->dma->z_dev, hd->chan->index);
if (ret < 0)
comp_err(dev, "host_trigger(): dma stop failed: %d",
ret);
}

ret = dma_stop(hd->chan->dma->z_dev, hd->chan->index);
if (ret < 0)
comp_err(dev, "host_trigger(): dma stop failed: %d",
ret);
break;
default:
break;
Expand Down Expand Up @@ -1015,8 +1012,7 @@ static int host_position(struct comp_dev *dev,
void host_zephyr_reset(struct host_data *hd, uint16_t state)
Copy link
Collaborator

@kv2019i kv2019i Apr 13, 2023

Choose a reason for hiding this comment

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

The description is a bit misleading as it assume this code is always used with HD-DMA, while this code is in fact generic and could be used with a wide variety of DMA hardware and drivers.

But this does explain why this is important step to do (and explains what e.g. happens on HD-DMA hardware when not done), so not a blocker.

{
if (hd->chan) {
if (state == COMP_STATE_ACTIVE)
dma_stop(hd->chan->dma->z_dev, hd->chan->index);
dma_stop(hd->chan->dma->z_dev, hd->chan->index);
Copy link
Collaborator

@ranj063 ranj063 Apr 13, 2023

Choose a reason for hiding this comment

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

@fkwasowi what issue does this change fix? AFAICT we also do dma_stop() in host_zephyr_trigger() in the STOP case with the same check for state. While the state may not be active during reset, i'd assume that it must have been active during stop and the dma should be stopped there

Copy link
Contributor Author

@fkwasowi fkwasowi Apr 14, 2023

Choose a reason for hiding this comment

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

There is a dma_start in host_zephyr_trigger that sets DGCS_GEN to 1, and a dma_stop that sets it to 0. Pipeline reset usually occurs after PAUSE (there is also a COMP_STATE_PREPARE state in between, and it is on this state that we enter the host_zephyr_reset function). I have not seen such a scenario that the RESET pipeline state is executed immediately after the RUN pipeline state (COMP_STATE_ACTIVE is only when we are in the RUN state). Such a sequence is incorrect. This fixes the scenario: https://github.com/intel-innersource/drivers.audio.firmware.converged/issues/505

Copy link
Collaborator

@lyakh lyakh Apr 17, 2023

Choose a reason for hiding this comment

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

does this mean, that we're effectively reverting #6849 ? Was it wrong?

Copy link
Member

@plbossart plbossart Apr 17, 2023

Choose a reason for hiding this comment

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

I am also not following the change nor the failing scenario.

We do clear DGCS_GEN in hda_hda_stop_common, what causes it not to be cleared?

I'll add a "request change" to mark my concern, this DGCS_GEN bit is very very touchy and we have to be super careful with any changes to these sequences.

Copy link
Contributor Author

@fkwasowi fkwasowi Apr 18, 2023

Choose a reason for hiding this comment

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

IF was added because in sof the same channel was stopped several times in some scenarios. Because of this, the ref count in the dev pm was going off. But the problem was fixed later on the hda driver side, so now the IF can be removed.

Copy link
Contributor Author

@fkwasowi fkwasowi Apr 18, 2023

Choose a reason for hiding this comment

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

HDA DMA programming sequence in pipeline flows, coming from closed-source FW (obligatory to work properly with closed-source driver):

Reset (XRUN):

IPC: Set Pipe State(PAUSE): FW: if coming from RUN state, then no DMA programming
SW: Set DMA RUN = 0 and Poll untill it reflects 0
IPC: Set Pipe State(RESET): FW:
a. GEN=0
b. FIFORDY=0
SW: HDA Stream RESET
SW: HDA Stream Configure(BDL, CTL etc.)
SW: start Host DMA
IPC: Set Pipe State(PAUSE): FW: if coming from RESET state then FIFORDY=1
IPC: Set Pipe State (RUN): FW: Set GEN=1

Copy link
Member

@plbossart plbossart Apr 18, 2023

Choose a reason for hiding this comment

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

IF was added because in sof the same channel was stopped several times in some scenarios. Because of this, the ref count in the dev pm was going off. But the problem was fixed later on the hda driver side, so now the IF can be removed.

That's a valid explanation @fkwasowi, however why not revert fa0456e completely then?

We still have cases where the dma_stop is handled conditionally, e.g.

case COMP_TRIGGER_STOP:
	case COMP_TRIGGER_XRUN:
		if (dev->state == COMP_STATE_ACTIVE) {
			ret = dma_stop(hd->chan->dma->z_dev, hd->chan->index);
			if (ret < 0)
				comp_err(dev, "host_trigger(): dma stop failed: %d",
					 ret);
		}

Copy link
Contributor Author

@fkwasowi fkwasowi Apr 19, 2023

Choose a reason for hiding this comment

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

@tmleman Is it necessary, to undo all your changes fa0456e ?

Copy link
Contributor

@tmleman tmleman Apr 24, 2023

Choose a reason for hiding this comment

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

Is it necessary, to undo all your changes fa0456e ?

I don't know if it's necessary but it should be possible to do. This has already been partially done in this commit
a942f10

dma_release_channel(hd->dma->z_dev, hd->chan->index);
hd->chan = NULL;
}
Expand Down
3 changes: 1 addition & 2 deletions src/ipc/ipc4/dai.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,7 @@ void dai_dma_release(struct dai_data *dd, struct comp_dev *dev)
*/
#if CONFIG_ZEPHYR_NATIVE_DRIVERS
/* if reset is after pause dma has already been stopped */
if (dev->state != COMP_STATE_PAUSED)
dma_stop(dd->chan->dma->z_dev, dd->chan->index);
dma_stop(dd->chan->dma->z_dev, dd->chan->index);

dma_release_channel(dd->chan->dma->z_dev, dd->chan->index);
#else
Expand Down