[Sound-open-firmware] [PATCH] sof: Turn IRQs off globally when send command to pipeline component
When pipeline processing the command, we have to turn off the IRQs. If not, the DMA IRQ will interrupt the pipeline state set process, when pipeline is scheduled, the states inconsistence will cause xrun.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com
--- Tested with apl-gpmrb board
kernel: https://github.com/plbossart/sound.git branch: topic/sof-v4.14 95d77adbaadc44b28c8975a3714f70824d1a8529
firmware: git://git.alsa-project.org/sound-open-firmware.git branch: 1.1-stable 210989dffeea811de2370fccb7cf5d53106b1e6e
tools: branch: 1.1-stable cc91c73aa3e91eea35abdeb76d578b97f718feff --- src/audio/pipeline.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/audio/pipeline.c b/src/audio/pipeline.c index 1b7f248..23be503 100644 --- a/src/audio/pipeline.c +++ b/src/audio/pipeline.c @@ -627,6 +627,7 @@ int pipeline_cmd(struct pipeline *p, struct comp_dev *host, int cmd, { struct op_data op_data; int ret; + uint32_t flags;
trace_pipe("cmd");
@@ -635,7 +636,7 @@ int pipeline_cmd(struct pipeline *p, struct comp_dev *host, int cmd, op_data.cmd = cmd; op_data.cmd_data = data;
- spin_lock(&p->lock); + spin_lock_irq(&p->lock, flags);
if (host->params.direction == SOF_IPC_STREAM_PLAYBACK) { /* send cmd downstream from host to DAI */ @@ -651,7 +652,7 @@ int pipeline_cmd(struct pipeline *p, struct comp_dev *host, int cmd, trace_error_value(cmd); }
- spin_unlock(&p->lock); + spin_unlock_irq(&p->lock, flags); return ret; }
On Tue, 2018-03-27 at 17:17 +0800, Wu Zhigang wrote:
When pipeline processing the command, we have to turn off the IRQs. If not, the DMA IRQ will interrupt the pipeline state set process, when pipeline is scheduled, the states inconsistence will cause xrun.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com
Applied.
Thanks
Liam
On 3/27/18 4:17 AM, Wu Zhigang wrote:
When pipeline processing the command, we have to turn off the IRQs. If not, the DMA IRQ will interrupt the pipeline state set process, when pipeline is scheduled, the states inconsistence will cause xrun.
we disable the irqs but then will walk the graph from host to DAI with component_op_downstream to pass a COMPS_OPS_CMD with in turn will call all .cmd callbacks from all components
This could take a significant amount of time and lead to interrupts unrelated to that specific pipeline not being serviced, leading to xruns.
what am I missing?
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com
Tested with apl-gpmrb board
kernel: https://github.com/plbossart/sound.git branch: topic/sof-v4.14 95d77adbaadc44b28c8975a3714f70824d1a8529
firmware: git://git.alsa-project.org/sound-open-firmware.git branch: 1.1-stable 210989dffeea811de2370fccb7cf5d53106b1e6e
tools: branch: 1.1-stable cc91c73aa3e91eea35abdeb76d578b97f718feff
src/audio/pipeline.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/audio/pipeline.c b/src/audio/pipeline.c index 1b7f248..23be503 100644 --- a/src/audio/pipeline.c +++ b/src/audio/pipeline.c @@ -627,6 +627,7 @@ int pipeline_cmd(struct pipeline *p, struct comp_dev *host, int cmd, { struct op_data op_data; int ret;
uint32_t flags;
trace_pipe("cmd");
@@ -635,7 +636,7 @@ int pipeline_cmd(struct pipeline *p, struct comp_dev *host, int cmd, op_data.cmd = cmd; op_data.cmd_data = data;
- spin_lock(&p->lock);
spin_lock_irq(&p->lock, flags);
if (host->params.direction == SOF_IPC_STREAM_PLAYBACK) { /* send cmd downstream from host to DAI */
@@ -651,7 +652,7 @@ int pipeline_cmd(struct pipeline *p, struct comp_dev *host, int cmd, trace_error_value(cmd); }
- spin_unlock(&p->lock);
- spin_unlock_irq(&p->lock, flags); return ret; }
On Tue, 2018-03-27 at 10:37 -0500, Pierre-Louis Bossart wrote:
On 3/27/18 4:17 AM, Wu Zhigang wrote:
When pipeline processing the command, we have to turn off the IRQs. If not, the DMA IRQ will interrupt the pipeline state set process, when pipeline is scheduled, the states inconsistence will cause xrun.
we disable the irqs but then will walk the graph from host to DAI with component_op_downstream to pass a COMPS_OPS_CMD with in turn will call all .cmd callbacks from all components
This could take a significant amount of time and lead to interrupts unrelated to that specific pipeline not being serviced, leading to xruns.
what am I missing?
The pipeline cmd just walks the pipeline and updates the component state alongside calling comp_cmd() on each component (START, STOP, PAUSE etc). This is very fast in practice, about 20us on BYT. I agree that we do need to be clear this is in atomic context (like ALSA trigger) and that any component de- init/cleanup must be done in comp_reset().
I will be writing some docs to go alongside 1.2 so I can include this in the programming documentation.
Liam
On 03/27/2018 01:34 PM, Liam Girdwood wrote:
On Tue, 2018-03-27 at 10:37 -0500, Pierre-Louis Bossart wrote:
On 3/27/18 4:17 AM, Wu Zhigang wrote:
When pipeline processing the command, we have to turn off the IRQs. If not, the DMA IRQ will interrupt the pipeline state set process, when pipeline is scheduled, the states inconsistence will cause xrun.
we disable the irqs but then will walk the graph from host to DAI with component_op_downstream to pass a COMPS_OPS_CMD with in turn will call all .cmd callbacks from all components
This could take a significant amount of time and lead to interrupts unrelated to that specific pipeline not being serviced, leading to xruns.
what am I missing?
The pipeline cmd just walks the pipeline and updates the component state alongside calling comp_cmd() on each component (START, STOP, PAUSE etc). This is very fast in practice, about 20us on BYT. I agree that we do need to be clear this is in atomic context (like ALSA trigger) and that any component de- init/cleanup must be done in comp_reset().
I will be writing some docs to go alongside 1.2 so I can include this in the programming documentation.
ok, but then we need to go back to the existing components. Just looking at e.g. eq_fir and src the assumption of fast operation is not right, there are things like the code below which go beyond just a state change.
switch (cmd) { case COMP_CMD_SET_VALUE: ret = fir_cmd_set_value(dev, cdata); break; case COMP_CMD_SET_DATA: ret = fir_cmd_set_data(dev, cdata); break; }
if (cmd == COMP_CMD_SET_VALUE) ret = src_ctrl_cmd(dev, cdata);
Or it could be that we need a 'fast' state change and an additional hook for actual commands?
Liam
On Tue, 2018-03-27 at 13:50 -0500, Pierre-Louis Bossart wrote:
On 03/27/2018 01:34 PM, Liam Girdwood wrote:
On Tue, 2018-03-27 at 10:37 -0500, Pierre-Louis Bossart wrote:
On 3/27/18 4:17 AM, Wu Zhigang wrote:
When pipeline processing the command, we have to turn off the IRQs. If not, the DMA IRQ will interrupt the pipeline state set process, when pipeline is scheduled, the states inconsistence will cause xrun.
we disable the irqs but then will walk the graph from host to DAI with component_op_downstream to pass a COMPS_OPS_CMD with in turn will call all .cmd callbacks from all components
This could take a significant amount of time and lead to interrupts unrelated to that specific pipeline not being serviced, leading to xruns.
what am I missing?
The pipeline cmd just walks the pipeline and updates the component state alongside calling comp_cmd() on each component (START, STOP, PAUSE etc). This is very fast in practice, about 20us on BYT. I agree that we do need to be clear this is in atomic context (like ALSA trigger) and that any component de- init/cleanup must be done in comp_reset().
I will be writing some docs to go alongside 1.2 so I can include this in the programming documentation.
ok, but then we need to go back to the existing components. Just looking at e.g. eq_fir and src the assumption of fast operation is not right, there are things like the code below which go beyond just a state change.
switch (cmd) { case COMP_CMD_SET_VALUE: ret = fir_cmd_set_value(dev, cdata); break; case COMP_CMD_SET_DATA: ret = fir_cmd_set_data(dev, cdata); break; } if (cmd == COMP_CMD_SET_VALUE) ret = src_ctrl_cmd(dev, cdata);Or it could be that we need a 'fast' state change and an additional hook for actual commands?
We probably want to keep the trigger() like commands atomic and add another operation for non atomic get/set parameter type calls. It's not a big change.
I can tackle it tomorrow.
Liam
Liam
On 2018年03月27日 23:37, Pierre-Louis Bossart wrote:
On 3/27/18 4:17 AM, Wu Zhigang wrote:
When pipeline processing the command, we have to turn off the IRQs. If not, the DMA IRQ will interrupt the pipeline state set process, when pipeline is scheduled, the states inconsistence will cause xrun.
we disable the irqs but then will walk the graph from host to DAI with component_op_downstream to pass a COMPS_OPS_CMD with in turn will call all .cmd callbacks from all components
This could take a significant amount of time and lead to interrupts unrelated to that specific pipeline not being serviced, leading to xruns.
what am I missing?
This point I also mentioned in the previous email list. I also worried about the IRQs masked duration might be too long. My suggestion is change the state set sequence, set the DAI component first, then VOL, the last HOST. Liam's comments is the mask duration is short. thanks zhigang.wu
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com
Tested with apl-gpmrb board
kernel: https://github.com/plbossart/sound.git%C2%A0 branch: topic/sof-v4.14 95d77adbaadc44b28c8975a3714f70824d1a8529
firmware: git://git.alsa-project.org/sound-open-firmware.git branch: 1.1-stable 210989dffeea811de2370fccb7cf5d53106b1e6e
tools: branch: 1.1-stable cc91c73aa3e91eea35abdeb76d578b97f718feff
src/audio/pipeline.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/audio/pipeline.c b/src/audio/pipeline.c index 1b7f248..23be503 100644 --- a/src/audio/pipeline.c +++ b/src/audio/pipeline.c @@ -627,6 +627,7 @@ int pipeline_cmd(struct pipeline *p, struct comp_dev *host, int cmd, { struct op_data op_data; int ret; + uint32_t flags; trace_pipe("cmd"); @@ -635,7 +636,7 @@ int pipeline_cmd(struct pipeline *p, struct comp_dev *host, int cmd, op_data.cmd = cmd; op_data.cmd_data = data; - spin_lock(&p->lock); + spin_lock_irq(&p->lock, flags); if (host->params.direction == SOF_IPC_STREAM_PLAYBACK) { /* send cmd downstream from host to DAI */ @@ -651,7 +652,7 @@ int pipeline_cmd(struct pipeline *p, struct comp_dev *host, int cmd, trace_error_value(cmd); } - spin_unlock(&p->lock); + spin_unlock_irq(&p->lock, flags); return ret; }
participants (4)
-
Liam Girdwood -
Pierre-Louis Bossart -
Wu Zhigang -
zhigang.wu