audio: dai-zephyr/host-zephyr: stop dma only once #6849
Conversation
tmleman
requested review from
lgirdwood,
plbossart,
mmaka1,
lbetlej and
dbaluta
as code owners
tmleman
requested review from
abonislawski,
lyakh,
kv2019i and
marcinszkudlinski
There was a problem hiding this comment.
Thanks @tmleman ! I bit reluctant approval on this one. I think we have to make this change and I couldn't figure out a better way. Still, this seems to add brittleness that might bite us later. If anyone has better suggestions, please chime in.
| @@ -1054,7 +1054,12 @@ static int dai_comp_trigger_internal(struct comp_dev *dev, int cmd) | |||
| dai_trigger_op(dd->dai, cmd, dev->direction); | |||
| #else | |||
| dai_trigger_op(dd->dai, cmd, dev->direction); | |||
| ret = dma_stop(dd->chan->dma->z_dev, dd->chan->index); | |||
| if (dev->state == COMP_STATE_ACTIVE) { | |||
| ret = dma_stop(dd->chan->dma->z_dev, dd->chan->index); | |||
There was a problem hiding this comment.
I guess we have to do this now. I'm a bit worried about coupling device state and DMA state this way. This opens a problem if some state is set to not ACTIVE in some other piece of code. Then dma_stop() is never called. Probably this is ok now, but it's actually pretty hard to verify this is the case by reading current dai-zephyr.c code.
OTOH, it seems there are no calls to get dma channel status with the public API. Right @teburd @juimonen ? dma_get_status() does not seem fit to gate calls to dma_stop().
There was a problem hiding this comment.
This will work but the best way would be to balance this in zephyr driver, in gpdma we already have channel internal state so calling dma_stop several times will not result in unbalanced pm state.
/* Validate the channel state */
if (chan_data->state != DW_DMA_ACTIVE &&
chan_data->state != DW_DMA_SUSPENDED) {
ret = -EINVAL;
goto out;
}
but there is no such internal state check in HDA zephyr driver so we need to take care of this in SOF (this PR) or track device state in Zephyr (HDA driver)
There was a problem hiding this comment.
@abonislawski But as SOF code should work with any Zephyr driver, just fixing HDA is not enough, we'd need to have this as a guarantee at Zephyr dma.h level.
There was a problem hiding this comment.
at dma.h level we can only expose current state
tmleman
force-pushed
the
topic/upstream/pr/zephyr/pm/dma/start_stop_balance
branch
2 times, most recently
from
7d71a9f to
214fdec
Compare
tmleman
changed the title
tmleman
force-pushed
the
topic/upstream/pr/zephyr/pm/dma/start_stop_balance
branch
from
214fdec to
7029d46
Compare
|
@abonislawski Ok to merge this? We can't proceed with #6828 without this.... |
Until now, one dma channel could be stopped multiple times without any problems. From commit af6d827b64 on zephyr it is required that the start and stop calls be perfectly balanced. Zephyr device power manager stores an internal counter. Counter is incremented if device is used and decremented on device release. In this case when we call dma_start() and dma_stop(). If dma_stop() is called more times than dma_start() power manager will reaport a error -EALREADY. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
tmleman
force-pushed
the
topic/upstream/pr/zephyr/pm/dma/start_stop_balance
branch
from
7029d46 to
e6489e9
Compare
|
@kv2019i @abonislawski @ranj063 I had to make additional changes, I encourage you to take a look again. Here you can check how it behave with updated zephyr kernel #6853 |
Im ok but please wait for CI |
Until now, one dma channel could be stopped multiple times without any problems. From commit af6d827b64 on zephyr it is required that the start and stop calls be perfectly balanced.
Zephyr device power manager stores an internal counter. Counter is incremented if device is used and decremented on device release. In this case when we call dma_start() and dma_stop().
If dma_stop() is called more times than dma_start() power manager will reaport a error -EALREADY.
Signed-off-by: Tomasz Leman tomasz.m.leman@intel.com