On 2022-02-25 8:42 PM, Pierre-Louis Bossart wrote:
On 2/7/22 07:25, Cezary Rojewski wrote:
Add functions to ease with state changing of all objects found in the
path. Each represents either a BIND/UNBIND or SET_PIPEPILE_STATE IPC.
SET_PIPELINE?
A typo! Thanks for spotting this out!
DSP pipelines follow simple state machine scheme:
CREATE -> RESET -> PAUSE -> RUNNING -> PAUSE -> RESET -> DELETE>
There is no STOP, PAUSE serves that purpose instead.
that's not fully correct, the STOP can be handled in two steps due to
DMA programming flows.
From DSP perspective, there's no stop. Literally one cannot send
SET_PIPELINE_STATE with STOP as a requested state.
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
Signed-off-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
---
sound/soc/intel/avs/path.c | 130 +++++++++++++++++++++++++++++++++++++
sound/soc/intel/avs/path.h | 5 ++
2 files changed, 135 insertions(+)
diff --git a/sound/soc/intel/avs/path.c b/sound/soc/intel/avs/path.c
index 272d868bedc9..c6db3f091e66 100644
--- a/sound/soc/intel/avs/path.c
+++ b/sound/soc/intel/avs/path.c
@@ -285,3 +285,133 @@ struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id,
return path;
}
+
+int avs_path_bind(struct avs_path *path)
+{
+ struct avs_path_pipeline *ppl;
+ struct avs_dev *adev = path->owner;
+ int ret;
+
+ list_for_each_entry(ppl, &path->ppl_list, node) {
+ struct avs_path_binding *binding;
+
+ list_for_each_entry(binding, &ppl->binding_list, node) {
+ struct avs_path_module *source, *sink;
+
+ source = binding->source;
+ sink = binding->sink;
+
+ ret = avs_ipc_bind(adev, source->module_id,
+ source->instance_id, sink->module_id,
+ sink->instance_id, binding->sink_pin,
+ binding->source_pin);
+ if (ret) {
+ dev_err(adev->dev, "bind path failed: %d\n", ret);
+ return AVS_IPC_RET(ret);
so what happens for all the previously bound path?
Is there an error-unwinding loop somewhere?
This is a good question. It's an unknown ground. Usually if anything
wrong happens, all the pipelines that are part of the path would be
forcefully deleted. What I can do, is add unwinding and check with
validation using some unusual scenarios to see if no regression occurs.
Not promising anything though - see below.
+ }
+ }
+ }
+
+ return 0;
+}
+
+int avs_path_unbind(struct avs_path *path)
+{
+ struct avs_path_pipeline *ppl;
+ struct avs_dev *adev = path->owner;
+ int ret;
+
+ list_for_each_entry(ppl, &path->ppl_list, node) {
+ struct avs_path_binding *binding;
+
+ list_for_each_entry(binding, &ppl->binding_list, node) {
+ struct avs_path_module *source, *sink;
+
+ source = binding->source;
+ sink = binding->sink;
+
+ ret = avs_ipc_unbind(adev, source->module_id,
+ source->instance_id, sink->module_id,
+ sink->instance_id, binding->sink_pin,
+ binding->source_pin);
+ if (ret) {
+ dev_err(adev->dev, "unbind path failed: %d\n", ret);
+ return AVS_IPC_RET(ret);
what happens then? reboot?
Yeah, unfortunately when an IPC fails, it's usually for a very "bad"
reason and only DSP recovery can help here.
+ }
+ }
+ }
+
+ return 0;
+}
+
+int avs_path_reset(struct avs_path *path)
+{
+ struct avs_path_pipeline *ppl;
+ struct avs_dev *adev = path->owner;
+ int ret;
+
+ if (path->state == AVS_PPL_STATE_RESET)
+ return 0;
+
+ list_for_each_entry(ppl, &path->ppl_list, node) {
+ ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
+ AVS_PPL_STATE_RESET);
+ if (ret) {
+ dev_err(adev->dev, "reset path failed: %d\n", ret);
+ path->state = AVS_PPL_STATE_INVALID;
+ return AVS_IPC_RET(ret);
+ }
+ }
+
+ path->state = AVS_PPL_STATE_RESET;
+ return 0;
+}
+
+int avs_path_pause(struct avs_path *path)
+{
+ struct avs_path_pipeline *ppl;
+ struct avs_dev *adev = path->owner;
+ int ret;
+
+ if (path->state == AVS_PPL_STATE_PAUSED)
+ return 0;
+
+ list_for_each_entry_reverse(ppl, &path->ppl_list, node) {
does the order actually matter?
Yes, it does. We do here what's recommended.
+ ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
+ AVS_PPL_STATE_PAUSED);
+ if (ret) {
+ dev_err(adev->dev, "pause path failed: %d\n", ret);
+ path->state = AVS_PPL_STATE_INVALID;
+ return AVS_IPC_RET(ret);
+ }
+ }
+
+ path->state = AVS_PPL_STATE_PAUSED;
+ return 0;
+}
it looks like you could use a helper since the flows are identical.
I did try doing that in the past but the readability got hurt : (
avs_path_run() has an additional check when compared to the other two.
avs_path_pause() and avs_path_reset() to the things in the opposite
order. So, I still believe it's not good to provide a helper for these.
If you are seeing something I'm not, please feel free to correct me.
+
+int avs_path_run(struct avs_path *path, int trigger)
+{
+ struct avs_path_pipeline *ppl;
+ struct avs_dev *adev = path->owner;
+ int ret;
+
+ if (path->state == AVS_PPL_STATE_RUNNING && trigger == AVS_TPLG_TRIGGER_AUTO)
+ return 0;
+
+ list_for_each_entry(ppl, &path->ppl_list, node) {
+ if (ppl->template->cfg->trigger != trigger)
+ continue;
+
+ ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
+ AVS_PPL_STATE_RUNNING);
+ if (ret) {
+ dev_err(adev->dev, "run path failed: %d\n", ret);
+ path->state = AVS_PPL_STATE_INVALID;
+ return AVS_IPC_RET(ret);
+ }
+ }
+
+ path->state = AVS_PPL_STATE_RUNNING;
+ return 0;
+}
diff --git a/sound/soc/intel/avs/path.h b/sound/soc/intel/avs/path.h
index 3843e5a062a1..04a06473f04b 100644
--- a/sound/soc/intel/avs/path.h
+++ b/sound/soc/intel/avs/path.h
@@ -62,5 +62,10 @@ struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id,
struct avs_tplg_path_template *template,
struct snd_pcm_hw_params *fe_params,
struct snd_pcm_hw_params *be_params);
+int avs_path_bind(struct avs_path *path);
+int avs_path_unbind(struct avs_path *path);
+int avs_path_reset(struct avs_path *path);
+int avs_path_pause(struct avs_path *path);
+int avs_path_run(struct avs_path *path, int trigger);
#endif
