From bdd814e3be7f544f3a83b31971845532ff24cae2 Mon Sep 17 00:00:00 2001 From: Keyon Jie Date: Thu, 11 Apr 2019 15:37:50 +0800 Subject: [PATCH 1/3] component: refine state change for xrun handling When trigger xrun arrives, we will reset the component state to be READY, which means prepare will needed before we can recover. Update state machine diagram and cmoka unit test cases also. Signed-off-by: Keyon Jie --- src/audio/component.c | 7 ++- src/include/sof/audio/component.h | 52 ++++++++++--------- .../src/audio/component/comp_set_state.c | 22 ++++---- 3 files changed, 42 insertions(+), 39 deletions(-) diff --git a/src/audio/component.c b/src/audio/component.c index 0fb34a89e9f3..236f8c9f4c42 100644 --- a/src/audio/component.c +++ b/src/audio/component.c @@ -159,17 +159,20 @@ 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 || dev->state == COMP_STATE_PAUSED) { dev->state = COMP_STATE_PREPARE; } else { trace_comp_error("comp_set_state() error: " "wrong state = %u, " - "COMP_TRIGGER_STOP/XRUN", dev->state); + "COMP_TRIGGER_STOP", dev->state); ret = -EINVAL; } break; + case COMP_TRIGGER_XRUN: + /* reset component status to ready at xrun */ + dev->state = COMP_STATE_READY; + break; case COMP_TRIGGER_PAUSE: /* only support pausing for running */ if (dev->state == COMP_STATE_ACTIVE) { diff --git a/src/include/sof/audio/component.h b/src/include/sof/audio/component.h index 64ced4802c74..9bf56eb5fbe6 100644 --- a/src/include/sof/audio/component.h +++ b/src/include/sof/audio/component.h @@ -68,29 +68,33 @@ /** * States may transform as below:- * \verbatim - * - * ------------- - * pause | | stop/xrun - * +-------------| ACTIVITY |---------------+ - * | | | | prepare - * | ------------- | +-----------+ - * | ^ ^ | | | - * | | | | | | - * v | | v | | - * ------------- | | ------------- | - * | | release | | start | | | - * | PAUSED |-----------+ +-------------| PREPARE |<-------+ - * | | | | - * ------------- ------------- - * | ^ ^ - * | stop/xrun | | - * +--------------------------------------+ | - * | prepare - * ------------- | - * | | | - * ----------->| READY |------------------+ - * reset | | - * ------------- + * +---------------------------------------+ + * | | + * ------------- | + * pause | | stop | + * +-------------| ACTIVITY |---------------+ | + * | | | | prepare | + * | ------------- | +---------+ | + * | ^ ^ | | | | + * | | | | | | | + * v | | v | | | + * ------------- | | ------------- | | + * | | release | | start | | | | + * | PAUSED |-----------+ +-------------| PREPARE |<-----+ | + * | | | | | + * ------------- ------------- | + * | ^ ^ | + * | stop | | | + * +--------------------------------------+ | | + * | prepare | + * ------------- | | + * | | | | + * ----------->| READY |------------------+ | + * reset | | | + * ------------- | + * ^ | + * | xrun | + * +---------------------------------------+ * * \endverbatim */ @@ -662,12 +666,12 @@ static inline int comp_get_requested_state(int cmd) break; case COMP_TRIGGER_PREPARE: case COMP_TRIGGER_STOP: - case COMP_TRIGGER_XRUN: state = COMP_STATE_PREPARE; break; case COMP_TRIGGER_PAUSE: state = COMP_STATE_PAUSED; break; + case COMP_TRIGGER_XRUN: case COMP_TRIGGER_RESET: state = COMP_STATE_READY; break; diff --git a/test/cmocka/src/audio/component/comp_set_state.c b/test/cmocka/src/audio/component/comp_set_state.c index 42084413bc5d..b936bb32197d 100644 --- a/test/cmocka/src/audio/component/comp_set_state.c +++ b/test/cmocka/src/audio/component/comp_set_state.c @@ -78,10 +78,6 @@ struct test_case test_cases[] = { COMP_TRIGGER_STOP, NULL_STATE), TEST_CASE(SUCCEED, COMP_STATE_PAUSED, COMP_TRIGGER_STOP, NULL_STATE), - TEST_CASE(SUCCEED, COMP_STATE_ACTIVE, - COMP_TRIGGER_XRUN, NULL_STATE), - TEST_CASE(SUCCEED, COMP_STATE_PAUSED, - COMP_TRIGGER_XRUN, NULL_STATE), TEST_CASE(SUCCEED, COMP_STATE_ACTIVE, COMP_TRIGGER_PAUSE, NULL_STATE), TEST_CASE(SUCCEED, COMP_STATE_INIT, @@ -120,12 +116,6 @@ struct test_case test_cases[] = { NULL_STATE), TEST_CASE(FAIL, COMP_STATE_SUSPEND, COMP_TRIGGER_STOP, NULL_STATE), - TEST_CASE(FAIL, COMP_STATE_INIT, COMP_TRIGGER_XRUN, - NULL_STATE), - TEST_CASE(FAIL, COMP_STATE_READY, COMP_TRIGGER_XRUN, - NULL_STATE), - TEST_CASE(FAIL, COMP_STATE_SUSPEND, COMP_TRIGGER_XRUN, - NULL_STATE), TEST_CASE(FAIL, COMP_STATE_INIT, COMP_TRIGGER_PAUSE, NULL_STATE), TEST_CASE(FAIL, COMP_STATE_READY, COMP_TRIGGER_PAUSE, @@ -152,10 +142,16 @@ struct test_case test_cases[] = { COMP_TRIGGER_STOP, COMP_STATE_PREPARE), TEST_CASE(CORRECT_OUTPUT_STATE, COMP_STATE_PAUSED, COMP_TRIGGER_STOP, COMP_STATE_PREPARE), - TEST_CASE(CORRECT_OUTPUT_STATE, COMP_STATE_ACTIVE, - COMP_TRIGGER_XRUN, COMP_STATE_PREPARE), + TEST_CASE(CORRECT_OUTPUT_STATE, COMP_STATE_INIT, + COMP_TRIGGER_XRUN, COMP_STATE_READY), + TEST_CASE(CORRECT_OUTPUT_STATE, COMP_STATE_SUSPEND, + COMP_TRIGGER_XRUN, COMP_STATE_READY), + TEST_CASE(CORRECT_OUTPUT_STATE, COMP_STATE_PREPARE, + COMP_TRIGGER_XRUN, COMP_STATE_READY), TEST_CASE(CORRECT_OUTPUT_STATE, COMP_STATE_PAUSED, - COMP_TRIGGER_XRUN, COMP_STATE_PREPARE), + COMP_TRIGGER_XRUN, COMP_STATE_READY), + TEST_CASE(CORRECT_OUTPUT_STATE, COMP_STATE_ACTIVE, + COMP_TRIGGER_XRUN, COMP_STATE_READY), TEST_CASE(CORRECT_OUTPUT_STATE, COMP_STATE_ACTIVE, COMP_TRIGGER_PAUSE, COMP_STATE_PAUSED), TEST_CASE(CORRECT_OUTPUT_STATE, COMP_STATE_INIT, From ecd41a202ceefed9c9424b7eb113b1ff6edfbda0 Mon Sep 17 00:00:00 2001 From: Keyon Jie Date: Wed, 10 Apr 2019 18:37:03 +0800 Subject: [PATCH 2/3] pipeline: stop periodic scheduling at xrun At xrun, we will try internal recovery(if we have), if recovering fails(or no internal recovery existed), we need to stop the periodic scheduling. Signed-off-by: Keyon Jie --- src/audio/pipeline.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/audio/pipeline.c b/src/audio/pipeline.c index 05848a831986..e57742831e82 100644 --- a/src/audio/pipeline.c +++ b/src/audio/pipeline.c @@ -433,6 +433,7 @@ static void pipeline_comp_trigger_sched_comp(struct pipeline *p, switch (cmd) { case COMP_TRIGGER_PAUSE: case COMP_TRIGGER_STOP: + case COMP_TRIGGER_XRUN: pipeline_schedule_cancel(p); p->status = COMP_STATE_PAUSED; break; @@ -453,7 +454,6 @@ static void pipeline_comp_trigger_sched_comp(struct pipeline *p, break; case COMP_TRIGGER_SUSPEND: case COMP_TRIGGER_RESUME: - case COMP_TRIGGER_XRUN: default: break; } @@ -854,7 +854,7 @@ static uint64_t pipeline_task(void *arg) /* try to recover */ err = pipeline_xrun_recover(p); if (err < 0) - goto sched;/* skip copy if still in xrun */ + return 0;/* skip copy if still in xrun */ } err = pipeline_copy(p); @@ -863,11 +863,10 @@ static uint64_t pipeline_task(void *arg) err = pipeline_xrun_recover(p); if (err < 0) { trace_pipe_error_with_ids(p, "pipeline_task(): xrun recover failed! pipeline will be stopped!"); - goto sched; /* failed - host will stop this pipeline */ + return 0; /* failed - host will stop this pipeline */ } } -sched: tracev_pipe("pipeline_task() sched"); return p->ipc_pipe.period; } From 09ea37aeb32eece4cb88afb0c593efb45dc181f0 Mon Sep 17 00:00:00 2001 From: Keyon Jie Date: Wed, 10 Apr 2019 18:50:15 +0800 Subject: [PATCH 3/3] pipeline: improve trigger to including xrun handling Once FW initiated xrun happens, the pipeline will be stopped(via pipeline_xrun()) first internally, here add logic to ignore any extra trigger_stop from host, and do prepare for xrun recovery before we can really trigger it start. Signed-off-by: Keyon Jie --- src/audio/pipeline.c | 55 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/audio/pipeline.c b/src/audio/pipeline.c index e57742831e82..0fa8df35bce1 100644 --- a/src/audio/pipeline.c +++ b/src/audio/pipeline.c @@ -530,6 +530,49 @@ static int pipeline_trigger_on_core(struct pipeline *p, struct comp_dev *host, return ret; } +/* + * trigger handler for pipelines in xrun, used for recovery from host only. + * return values: + * 0 -- success, further trigger in caller needed. + * PPL_STATUS_PATH_STOP -- done, no more further trigger needed. + * minus -- failed, caller should return failure. + */ +static int pipeline_xrun_handle_trigger(struct pipeline *p, int cmd) +{ + int ret = 0; + + /* it is expected in paused status for xrun pipeline */ + if (!p->xrun_bytes || p->status != COMP_STATE_PAUSED) + return 0; + + /* in xrun, handle start/stop trigger */ + switch (cmd) { + case COMP_TRIGGER_START: + /* in xrun, prepare before trigger start needed */ + trace_pipe_with_ids(p, "in xrun, prepare it first"); + /* prepare the pipeline */ + ret = pipeline_prepare(p, p->source_comp); + if (ret < 0) { + trace_pipe_error_with_ids(p, "prepare error: ret = %d", + ret); + return ret; + } + /* now ready for start, clear xrun_bytes */ + p->xrun_bytes = 0; + break; + case COMP_TRIGGER_STOP: + /* in xrun, suppose pipeline is already stopped, ignore it */ + trace_pipe_with_ids(p, "already stopped in xrun"); + /* no more further trigger stop needed */ + ret = PPL_STATUS_PATH_STOP; + break; + default: + break; + } + + return ret; +} + /* trigger pipeline */ int pipeline_trigger(struct pipeline *p, struct comp_dev *host, int cmd) { @@ -539,6 +582,18 @@ int pipeline_trigger(struct pipeline *p, struct comp_dev *host, int cmd) trace_pipe_with_ids(p, "pipeline_trigger()"); + /* handle pipeline global checks before going into each components */ + if (p->xrun_bytes) { + ret = pipeline_xrun_handle_trigger(p, cmd); + if (ret < 0) { + trace_pipe_error_with_ids(p, "xrun handle error: ret = %d", + ret); + return ret; + } else if (ret == PPL_STATUS_PATH_STOP) + /* no further action needed*/ + return 0; + } + /* if current core is different than requested */ if (p->ipc_pipe.core != cpu_get_id()) return pipeline_trigger_on_core(p, host, cmd);