[keyonjie]

This series will fix #902 (FW initiated Xrun recovery)

Currently neither FW internal nor external(ALSA triggered) Xrun recovery works for FW initiated Xrun(please use patch here to verify this issue: keyonjie@755ea15).

This series fix them(verified on UP^2), details please refer to commits.

dai: do re-config after prepare from xrun
pipeline: add pipeline_pre_trigger() for before pipeline real trigger
pipeline: stop pipeline at xrun if no internal xrun recovery

[lgirdwood]

Needs more detail in commit messages explaining what issue this fixes and how.

[keyonjie]

Needs more detail in commit messages explaining what issue this fixes and how.

Something like this make sense?

Currently neither FW internal nor external(ALSA triggered) Xrun recovery works for FW initiated Xrun(please use patch here to verify this issue: keyonjie/sof@755ea15).

[lgirdwood]

@keyonjie no, the detail should be added to each commit message of what and why each patch does alongside the issue it fixes.

[keyonjie]

@lgirdwood updated, please help review.

[lgirdwood]

This adds a lot of complexity.
Ideally we need to ensure that the pipeline state is consistent before pipeline_trigger(START) is called. The XRUN handling code should restore the pipeline to be ready for trigger(START).

[keyonjie]

This adds a lot of complexity.

@lgirdwood do you mean adding pipeline_pre_trigger() is too complex? even without doing prepare for trigger start?

Ideally we need to ensure that the pipeline state is consistent before pipeline_trigger(START) is called. The XRUN handling code should restore the pipeline to be ready for trigger(START).

@lgirdwood I got your point here, let me think if I can change to make it work like that.

[lgirdwood]

@keyonjie lets look at this again after @tlauda pipeline changes are merged.

[keyonjie]

@tlauda @lgirdwood rebase and update my PR, please help review.

[tlauda]

pipeline_prepare is called in pipeline_xrun_recover, so why do we need additional prepare here?

[keyonjie]

The pipeline_xrun_recover() may be empty once NO_XRUN_RECOVERY is defined, that means there will be trigger stop and trigger start from host side(no prepare trigger). That's why we need this.

[tlauda]

So it means that rather than empty pipeline_xrun_recover() should at least do prepare call, no matter if its disabled or not. And also you are just triggering prepare here instead of calling pipeline_prepare. Is this also intentional?

[keyonjie]

So it means that rather than empty pipeline_xrun_recover() should at least do prepare call, no matter if its disabled or not. And also you are just triggering prepare here instead of calling

yes, that's another choice. I did it in my way as some driver might send prepare IPC in future before trigger start(ALSA do has that at this senario), but ours doesn't at the moment(we haven't added prepare IPC yet actually, you can see in ipc_stream_pcm_params() we only prepare at hw_params IPC at the moment), and this prepare is actually part of the recovery, so I want to add it here to increase the robustness of our FW(to handle with and without prepare IPC before trigger start for recovery).

pipeline_prepare. Is this also intentional?

it was not intentional here, let me change this.

[keyonjie]

@tlauda updated.

[keyonjie]

@jajanusz @tlauda @slawblauciak cmoka unit test fail issue fixed.

[lgirdwood]

This is all fine, except lets move this xrun handling to a different function i..e.

pipeline_handle_xrun_trigger()
{
    // return value indicates error, caller can continue or caller should return
}

pipeline_trigger()
{
    if (p->in_xrun) {
        ret = pipeline_handle_xrun_trigger();
        if (ret < 0)
            error;
        else if (ret > 0)
           return ret; // xrun handler handled this no further action needed.
      }
    .....
}

[keyonjie]

@lrgirdwo updated as you suggested.

[tlauda]

You always return 0.

[tlauda]

Common condition for both cases, can be checked before switch.

[tlauda]

Already checked inside the function

[tlauda]

You always return 0.

ret = PPL_STATUS_PATH_STOP

[tlauda]

Should be checked against status PPL_STATUS_PATH_STOP.

[keyonjie]

@tlauda your comments addressed and PR updated.

[lgirdwood]

@keyonjie does the state transition diagram need updated in the component header ?

[keyonjie]

correct, let me update it.

[keyonjie]

@lgirdwood @tlauda updated.