[RFC,10/13] ASoC: Intel: avs: Path state management

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.

DSP pipelines follow simple state machine scheme:
CREATE -> RESET -> PAUSE -> RUNNING -> PAUSE -> RESET -> DELETE

There is no STOP, PAUSE serves that purpose instead.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/path.c | 130 +++++++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/path.h |   5 ++
 2 files changed, 135 insertions(+)

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?

> 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.

> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  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?

> +			}
> +		}
> +	}
> +
> +	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?

> +			}
> +		}
> +	}
> +
> +	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?

> +		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.

> +
> +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

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@linux.intel.com>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   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