Skip to content

hda: dma preload timeout #783

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
lgirdwood merged 1 commit into from
Jan 8, 2019
Merged

hda: dma preload timeout #783

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
11 changes: 9 additions & 2 deletions src/drivers/intel/cavs/hda-dma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,12 +328,19 @@ static int hda_dma_host_preload(struct dma *dma, struct hda_chan_data *chan)
int i;
int period_cnt;

uint64_t deadline = platform_timer_get(platform_timer) +
clock_ms_to_ticks(PLATFORM_DEFAULT_CLOCK, 1) *
PLATFORM_HOST_DMA_TIMEOUT / 1000;

/* waiting for buffer full after start
* first try is unblocking, then blocking
*/
while (!(host_dma_reg_read(dma, chan->index, DGCS) & DGCS_BF) &&
(chan->state & HDA_STATE_BF_WAIT))
; /* TODO: this should timeout and not wait forever */
(chan->state & HDA_STATE_BF_WAIT)) {
if (deadline < platform_timer_get(platform_timer)) {
return -ETIME;
}

@keyonjie keyonjie Dec 29, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could you please only emit a warning or error, and still return success to let the trigger_start continue?
For user, 1/2 ms noise is tolerable, comparing to the start failed annoyance.

@jajanusz jajanusz Dec 31, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that it's a good idea. Maybe it solves your problem but looks like architectural/design flaw. Timeout is timeout - usually error caused by something external to the FW (HW or driver error), if you want to preload something it should happen, otherwise you get error. I guess we need something like start without preload.

What is root cause of your problem, why it cannot preload in time (or at all) ?

@keyonjie keyonjie Jan 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure that it's a good idea. Maybe it solves your problem but looks like architectural/design flaw. Timeout is timeout - usually error caused by something external to the FW (HW or driver error), if you want to preload something it should happen, otherwise you get error. I guess we need something like start without preload.

What is root cause of your problem, why it cannot preload in time (or at all) ?

I don't know what is the root cause of the problem why buffer never full at some cases, we have followed the sequence Marcin suggested in driver side, haven't you met this issue with slim driver stress testing?

@mmaka1 mmaka1 Jan 3, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@keyonjie BF at the startup is the only way to control the flow and align the state of HW ptrs and sw ptrs. Could you re-test with the DMI L0 forced all the time? Do you hit the t/o again?

@keyonjie keyonjie Jan 4, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mmaka1 how to do the DMI L0 forced?

@lgirdwood lgirdwood Jan 7, 2019

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.

@keyonjie I think @mmaka1 means DMA forced to L0 state. Look at pm_runtime.c and disable the PM for DMA.

@mmaka1 mmaka1 Jan 7, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@keyonjie The current cavs/pm_runtime.c cavs_pm_runtime_force_host_dma_l1_exit() forces L0 for short time (PLATFORM_FORCE_L1_EXIT_TIME). Please do a quick test with the bit toggled early once and do not toggle it back.

@keyonjie keyonjie Jan 8, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mmaka1 thanks for explain. I didn't get why it is named "L0 forced", I thought "L1 exit" means invalidate the L1 cached data and write it back to physical memory so couldn't imagine what L0 related there.

Thanks for the hints, I think it's better to merge this PR now and report any BF errors based on that for future debugging and fixes.

@mmaka1 @lgirdwood @jajanusz are you OK with it?

@jajanusz jajanusz Jan 8, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IMO it's good to merge. Timeout is better than freezing, so it's improvement anyway.

@keyonjie
I think it would be good to close old issue #723 when this is merged and create new issue that explains problem better (that it's going to be timeout) + what steps/env are needed to reproduce potential bug with BF.

}

if (host_dma_reg_read(dma, chan->index, DGCS) & DGCS_BF) {
chan->state &= ~(HDA_STATE_HOST_PRELOAD | HDA_STATE_BF_WAIT);
Expand Down