bug fix: "DSP panic" when ALSA XRUN recovery #88
Conversation
when stop command comes, the pipeline has to be prepared. This case has to be taken into account: when ALSA has XRUN, it will send stop/start commands to FW and try to recover the pipepine. If we did not prepare the pipeline before start, the recover process will fail. Signed-off-by: Wu Zhigang <[email protected]>
the host state must be PREPARE when stopped, if not, it will block our prepare process. Signed-off-by: Wu Zhigang <[email protected]>
when xrun comes, component may be in prepare state. Signed-off-by: Wu Zhigang <[email protected]>
ghost
changed the title
ghost
requested a review
from 
| * the pipeline before start, the recover process will fail. | ||
| */ | ||
| if (cmd == COMP_TRIGGER_STOP) { | ||
| ret = pipeline_prepare(pcm_dev->cd->pipeline, pcm_dev->cd); |
There was a problem hiding this comment.
that looks quite questionable. it's rather when you start that you should make sure things are prepared, no?
There was a problem hiding this comment.
It takes effect similar like the pipeline XRUN recover process.
you can refer to the pipeline_xrun_recover() to understand the logic.
without prepare, the firmware will hit XRUN again when start command comes.
There was a problem hiding this comment.
I make a stupid typo error, the Line459 should be:
ret = pipeline_prepare(pcm_dev->pipeline, pcm_dev->pipeline->source_comp);
I will update it after back office.
| @@ -740,7 +740,7 @@ static int host_stop(struct comp_dev *dev) | |||
| /* reset elements, to let next start from original one */ | |||
| host_elements_reset(dev); | |||
|
|
|||
| dev->state = COMP_STATE_PAUSED; | |||
| dev->state = COMP_STATE_PREPARE; | |||
There was a problem hiding this comment.
this is really odd. You will end-up with different states in different components, see e.g. the SSP dai code where the component are set to PAUSED.
Does this even work?
There was a problem hiding this comment.
I think when the component is truly paused, it should be paused state.
but if the command is stop. we have to set it prepare state to ready for the next start comes.
that makes sense.
| @@ -142,7 +142,8 @@ int comp_set_state(struct comp_dev *dev, int cmd) | |||
| break; | |||
| case COMP_TRIGGER_STOP: | |||
| case COMP_TRIGGER_XRUN: | |||
| if (dev->state == COMP_STATE_ACTIVE) { | |||
| if (dev->state == COMP_STATE_ACTIVE || | |||
| dev->state == COMP_STATE_PREPARE) { | |||
There was a problem hiding this comment.
WTH?
If you are in PREPARE state you set the component to PREPARE? I call this a no-op...
There was a problem hiding this comment.
conform with the previous RFC changed in src/audio/host.c
|
This RFCs idea comes from the pipeline_xrun_recover() process. |
|
@zhigang-wu: The flow in open source is that you have to convince people that your solution is correct, not expect maintainers to tell you what is wrong in the logic. The code you provided does not convince me, so the ball is in your court. We've had so many issues with those state machines, you'd need a much higher level explanations on the intent and effect of changing the state from PAUSED to PREPARED in some components. My main issue with your code is that the DAIs aren't touched and therefore it looks like a point solution, not a generic fix. |
ghost
mentioned this pull request
|
I will refine it. and analysis more code to make this patch more clearly and generic. |
|
seems @tlauda refine this solution. I will close it now. |
ghost
closed this
I did not test it with latest version, because of the some known issues or regression issues on the latest version.