-
Notifications
You must be signed in to change notification settings - Fork 311
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
host: remove reset in stop to keep status #170
Conversation
seems to fix #128 on APL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiulipan can you tell us why we need to keep the status ?
src/audio/host.c
Outdated
|
||
/* reset elements, to let next start from original one */ | ||
host_elements_reset(dev); | ||
//host_elements_reset(dev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pointers reset by this function aren't used for Host DMA anyway, so I doubt it changes anything on APL.
src/audio/host.c
Outdated
@@ -746,10 +746,10 @@ static int host_pointer_reset(struct comp_dev *dev) | |||
static int host_stop(struct comp_dev *dev) | |||
{ | |||
/* reset host side buffer pointers */ | |||
host_pointer_reset(dev); | |||
//host_pointer_reset(dev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you verified this change? I think ALSA expects position to be reset after stop.
If we reset after each stop, we will get some mismatch from the buffer pointer position in comp buffer and dma copy pointer. Not sure if but from the code
The local_pos is also be reset. But as now we need to keep what we have in buffer, this pointer should be be cleaned. |
@lgirdwood @tlauda the below is the call stack to configure the host dma's hardware pointer on kernel side, the BDL's parameter configuration call-stack show below: Sure, if the ALSA did not do this re-configuration for BDL, we could use another way to fix this. |
@xiulipan can you update us on test results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please cleanup.
@lgirdwood @tlauda |
Need to keep status of host comp at when we stop. Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>
@lgirdwood |
@lgirdwood
Now in the PR. |
@xiulipan yes it uses docker, I spent a few hours and added it today. However, the travis build will exceed the time limit when it tries to build initial container. We probably want to use a pre-built container and pass this to travis. Please see https://docs.travis-ci.com/user/docker/#examples. Our Dockerfile also needs updated to use new gcc 8.1 too. Can you create a feature issue for both gcc8.1 and pre-built travis container and assign yourself. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've manually stopped travis due to container build time out.
@lgirdwood |
@xiulipan great, I've disabled Travis until this is ready than we can re-enable |
host: remove reset in stop to keep status
Need to keep status of host comp at when we stop.
Check ALSA handler for these XRUN patterns to make sure no more bugs happen
depend on #169 to fix #161