capture pause/release failure when play/capture work at same time #35

ghost · 2018-07-02T05:04:15Z

Please add a description.

keyonjie

This looks good to me, good catch and fix, Liam, we need this, please help review and merge it.

lgirdwood

good catch, but lets also remove the old capture data so we dont send it to the host.

lgirdwood · 2018-07-02T09:04:10Z

src/audio/pipeline.c

@@ -223,6 +222,22 @@ static void pipeline_trigger_sched_comp(struct pipeline *p,
 			}
 		}
 		break;
+	case COMP_TRIGGER_RELEASE:


Ok, this is in the right direction, but instead of scheduling the pipeline to fix this issue with old data we should actually reset the pipeline buffers (i.e. clear the buffers to 0s and reset their R/W ptr to initial start positions). This way we recover without XRUN and we do not send stale/old capture data to the host (as pause can be longer than a few seconds).

This is for the pause/release process. If we do it like this. What is different between the stop/start and pause/release. Should we merge them?

yes, should be merged. Capture stop/start and pause/release have same flow, i.e we discard old data and reset R/W pts.

ok, I will send the next version.
I think the playback also has the same case, right?

no, playback should not clear the buffers on pause, only on stop

I think it is not big issue that we don't clear buffers on capture pause? in that way, those captured data(in buffer) will be written to application somewhat late only, those data still make sense.

no, that's wrong. if you are recording a conversation and then pause for 5 seconds you do not expect the first few periods after resume to contain audio from 5 secs ago as they will be out of context and may even cause audible artifacts.

but think from another side, if we reset buffers there may be milliseconds data lost, it is also another kind of artifacts?
and, the case you listed above may happen for playback also, considering live playback, if we don't reset buffer for pause on playback, we may also hear out of context audio which may also cause audible artifacts.

What about another solution:
Still scheduling the pipeline to fix this bug, but when pause the pipeline, we clear the buffer to 0s. do not reset the R/W ptr.

I dont mind if we bzero at pause or release for capture, maybe easier at release since we are not waiting for any DMA completions.

lgirdwood

Some more details please.

lgirdwood · 2018-07-03T08:53:02Z

src/audio/dai.c

+			dma_status(dd->dma, dd->chan,
+				   &status, SOF_IPC_STREAM_CAPTURE);
+
+			if (status.w_pos != (uint32_t)dma_buffer->w_ptr)


Why do you release the DMA in the callback if stream is not active ?

When the pipeline is paused, this is last time we reach here before release the pipeline.
over here, we reset the buffer pointer, call the dma_release() to adjust the dma pointer.
then ready for the release command coming.

Consider a DAI capture buffer B with 2 periods p0, p1. At release the downstream pipeline will consume 0s from p0 whilst the DMA is filling p1. Once p1 is full DMA will IRQ and then the DMA will fill p0 and pipeline will consume valid data from p1 (and so on...)

Actually we did not schedule the capture pipeline when we do the start/release. the capture's pipeline is scheduled when the DMA finish filling one period buffer. At the beginning stage, the downstream pipeline is not working, only the dai DMA is doing the data filling.
So we have to reset the buffer pointer and DMA pointer over here.

lgirdwood · 2018-07-03T08:54:23Z

src/audio/dai.c

+			 */
+			if (dev->params.direction == SOF_IPC_STREAM_PLAYBACK) {
+				/* recover the dma status */
+				ret = dma_release(dd->dma, dd->chan);


Where do we release() capture stream ?

When the release command comes, we did not need to do the dma pointer again(by calling dma_release() function). because we adjust the dma pointer in dai_dma_cb() function already.

lgirdwood · 2018-07-03T08:58:07Z

src/audio/host.c

+			dma_status(hd->dma, hd->chan,
+				   &status, SOF_IPC_STREAM_CAPTURE);
+			if (status.r_pos !=
+			    ((uint32_t)hd->dma_buffer->r_ptr & 0x00FFFFFF)) {


Why do we & 0xffffff here ?

because the HOST DMA's DGBRP only has the bit0~bit23 valid.
I will replace it with macro.

This is DMA specific, it should be done by dma_status API on r_pos.

I will confirm it by trace log.

lgirdwood · 2018-07-03T08:58:34Z

src/audio/host.c

+				 * just adjust the buffer pointer to sync
+				 * with host dma's register pointer.
+				 */
+				hd->dma_buffer->r_ptr += hd->dma_buffer->size;


The comments say what you are doing but dont say why ?

OK. I will update it in the commit description.
Let me describe it over here before:
the host buffer pointer should be sync with host dma pointer. because we did not stop the host dma, we have to read the DGBRP pointer first, then adjust the host buffer pointer according to the DGBRP pointer value.

ghost · 2018-07-03T09:01:08Z

Host dma is not stopped during pause. I talked with Keyon about this, this might has some problems if we stop it. Currently I have to read the host dma's BRP register and reset the host dma buffer pointer according to this BRP register.
This RFC still has problem: in pass-through mode, after the reset of host and dai. Because we did not reset the host buffer pointer to buffer's start address sometimes. the XRUN will happen.
We do the capture with these patches on my GP platform, there is no panic!
My GP platform has the alsa XRUN 100% percent. it will cause panic 100%. But after adding the patch, there is no panic at all.
When the ALSA XRUN happen, the stop/start command sent to firmware, the firmware did not reset the dma pointer and buffer pointer, which cause the XRUN again and again.

lgirdwood · 2018-07-03T09:28:37Z

Host DMA must be stopped at pause. ALSA/userspace are not expecting the HW ptr to move when stream is paused.
Passthrough is a special case where we have two DMAs reading and writing to same buffer. The reset function should check this and align ptrs so that that they do not collide.
This code is too specific for your use case and may break other use cases and topologies.

ghost · 2018-07-03T14:30:11Z

I will try the solution: stop the Host DMA when pause for capture.
I have to think about the pass-through and refine this buffer pointer re-initialization.
Yes, you are correct. It is for the special case. I will refine it.

ghost · 2018-07-05T09:06:45Z

If we did not reset the R/W pointer, we only clear the buffer to 0s before release.
the audible artifact will disappear. then we did not need to stop/start the HOST dma
during pause/release process.
Sure there is a period 0s taken in, but it does not have the negative effect.
What do you think of this?

lgirdwood · 2018-07-05T19:48:16Z

Capturing a period full of 0s at start or release is fine.

ghost · 2018-07-06T01:41:29Z

In these three RFCs, we do several things:

setup the buffer_zero() function, which only zero specific buffer content to 0s.
in the dai_dma_cb(), we will call the buffer_zero function to zero buffer at capture paused already.
when the release command comes, we will schedule the capture pipeline actively to avoid the xrun.

lgirdwood · 2018-07-06T08:26:48Z

Ok, so it sounds like you have a plan with 3 changes so best to send 3 patches in the PR update. i.e. 1 patch for each change above.

ghost · 2018-07-06T08:42:58Z

this is the patch set for this issue fix, this must be merged in one time.
I can do the change to remove the RFC prefix.

ghost · 2018-07-09T02:10:43Z

I update the patches already. this is the cluster patch. we should not divide them into three PRs.

lgirdwood · 2018-07-09T22:23:14Z

src/audio/pipeline.c

@@ -223,6 +222,22 @@ static void pipeline_trigger_sched_comp(struct pipeline *p,
 			}
 		}
 		break;
+	case COMP_TRIGGER_RELEASE:


I dont mind if we bzero at pause or release for capture, maybe easier at release since we are not waiting for any DMA completions.

lgirdwood · 2018-07-09T22:24:05Z

src/audio/buffer.c

@@ -100,6 +100,13 @@ void buffer_free(struct comp_buffer *buffer)
 	rfree(buffer);
 }

+void buffer_zero(struct comp_buffer *buffer)


static inline

ghost · 2018-07-10T02:36:25Z

updated according to the comments already.

plbossart

Please fix all your commit messages to use your Intel affiliation. The following tag is not acceptable:

Signed-off-by: Wu Zhigang wzgpeter@163.com

the issue case: playback and capture work at same time. when pause and resume the capture, the DSP will enter panic. the issue analysis: when pause the capture pipeline, the capture's last dma interrupt happens, and the capture will be paused. at this time, each buffer in capture pipeline will have the data unprocessed, sometimes it might be full. if we did not trigger the capture pipeline in resume process, the coming dma interrupt will have the xrun in the buffer pointer update. it is the high possibility. Signed-off-by: Wu Zhigang <zhigang.wu@linux.intel.com>

add the buffer_zero() function, we can use it when we want to clear the buffer data to 0s without resetting the buffer R/W pointer. Signed-off-by: Wu Zhigang <zhigang.wu@linux.intel.com>

before the capture pipeline is released, we should zero the dai buffer data. then the history data will not be sent out after release. Signed-off-by: Wu Zhigang <zhigang.wu@linux.intel.com>

ghost · 2018-07-11T01:16:28Z

update it already.

ghost changed the title ~~buf fix: capture release failure~~ capture pause/release failure when play/capture work at same time Jul 2, 2018

keyonjie reviewed Jul 2, 2018

View reviewed changes

lgirdwood requested changes Jul 2, 2018

View reviewed changes

lgirdwood requested changes Jul 3, 2018

View reviewed changes

lgirdwood requested changes Jul 9, 2018

View reviewed changes

ghost mentioned this pull request Jul 10, 2018

"DSP panic" when pause/release capture and playback is runing #22

Closed

plbossart requested changes Jul 10, 2018

View reviewed changes

Wu Zhigang added 3 commits July 11, 2018 09:11

buffer: add the buffer_zero() function

38c4d14

add the buffer_zero() function, we can use it when we want to clear the buffer data to 0s without resetting the buffer R/W pointer. Signed-off-by: Wu Zhigang <zhigang.wu@linux.intel.com>

dai: zero the dai dma buffer when capture pipeline paused

3f1cab0

before the capture pipeline is released, we should zero the dai buffer data. then the history data will not be sent out after release. Signed-off-by: Wu Zhigang <zhigang.wu@linux.intel.com>

lgirdwood approved these changes Jul 11, 2018

View reviewed changes

lgirdwood merged commit 6b94226 into thesofproject:master Jul 11, 2018

ghost deleted the topic/capture-resume-failure branch September 11, 2018 09:45

capture pause/release failure when play/capture work at same time #35

capture pause/release failure when play/capture work at same time #35

Conversation

ghost commented Jul 2, 2018 • edited by lgirdwood Loading

keyonjie left a comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 3, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 3, 2018

lgirdwood commented Jul 3, 2018

ghost commented Jul 3, 2018

ghost commented Jul 5, 2018

lgirdwood commented Jul 5, 2018

ghost commented Jul 6, 2018 • edited by ghost Loading

lgirdwood commented Jul 6, 2018

ghost commented Jul 6, 2018 • edited by ghost Loading

ghost commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 10, 2018

plbossart left a comment

Choose a reason for hiding this comment

ghost commented Jul 11, 2018

ghost commented Jul 2, 2018 •

edited by lgirdwood

Loading

ghost Jul 2, 2018 •

edited by ghost

Loading

ghost Jul 2, 2018 •

edited by ghost

Loading

ghost Jul 3, 2018 •

edited by ghost

Loading

ghost commented Jul 6, 2018 •

edited by ghost

Loading

ghost commented Jul 6, 2018 •

edited by ghost

Loading