diff mbox series

[06/17] ASoC: Intel: avs: Add pipeline management requests

Message ID 20220207122108.3780926-7-cezary.rojewski@intel.com (mailing list archive)
State Superseded
Headers show
Series ASoC: Intel: AVS - Audio DSP for cAVS | expand

Commit Message

Cezary Rojewski Feb. 7, 2022, 12:20 p.m. UTC 
A 'Pipeline' represents both a container of module instances, and a
scheduling entity. Multiple pipelines can be bound together to create an
audio graph. The Pipeline state machine is entirely controlled by IPCs
(creation, deletion and state changes).

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/messages.c | 76 ++++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/messages.h | 56 +++++++++++++++++++++++++
 2 files changed, 132 insertions(+)

Comments

Pierre-Louis Bossart Feb. 25, 2022, 1:11 a.m. UTC | #1 
On 2/7/22 06:20, Cezary Rojewski wrote:
> A 'Pipeline' represents both a container of module instances, and a
> scheduling entity. Multiple pipelines can be bound together to create an
> audio graph. The Pipeline state machine is entirely controlled by IPCs
> (creation, deletion and state changes).

How are the module instances connected within a pipeline? You've said
too much or too little here.


> +int avs_ipc_create_pipeline(struct avs_dev *adev, u16 req_size, u8 priority,
> +			    u8 instance_id, bool lp, u16 attributes)
> +{
> +	union avs_global_msg msg = AVS_GLOBAL_REQUEST(CREATE_PIPELINE);
> +	struct avs_ipc_msg request = {0};
> +	int ret;
> +
> +	msg.create_ppl.ppl_mem_size = req_size;
> +	msg.create_ppl.ppl_priority = priority;
> +	msg.create_ppl.instance_id = instance_id;
> +	msg.ext.create_ppl.lp = lp;

you may want to describe what the concepts of 'priority', 'lp' and
'attributes' are and which entity defines the values (topology?)

> +	msg.ext.create_ppl.attributes = attributes;
> +	request.header = msg.val;
> +
> +	ret = avs_dsp_send_msg(adev, &request, NULL);
> +	if (ret)
> +		avs_ipc_err(adev, &request, "create pipeline", ret);
> +
> +	return ret;
> +}

>  			u32 val;
> +			/* pipeline management */
> +			struct {
> +				u32 lp:1;
> +				u32 rsvd:3;
> +				u32 attributes:16;
> +			} create_ppl;
> +			struct {
> +				u32 multi_ppl:1;
> +				u32 sync_stop_start:1;

these two are not described at all?

> +			} set_ppl_state;
>  		} ext;
>  	};
>  } __packed;

> +/* Pipeline management messages */
> +enum avs_pipeline_state {
> +	AVS_PPL_STATE_INVALID,
> +	AVS_PPL_STATE_UNINITIALIZED,
> +	AVS_PPL_STATE_RESET,
> +	AVS_PPL_STATE_PAUSED,
> +	AVS_PPL_STATE_RUNNING,
> +	AVS_PPL_STATE_EOS,
> +	AVS_PPL_STATE_ERROR_STOP,
> +	AVS_PPL_STATE_SAVED,
> +	AVS_PPL_STATE_RESTORED,

can you describe that the last two enums might entail and what the
purpose might be?

I can see how the firmware state could be saved in IMR for faster
suspend/resume, but save/restore at the pipeline level doesn't seem to
have an obvious match for an ASoC driver?
Cezary Rojewski Feb. 25, 2022, 6:31 p.m. UTC | #2 
On 2022-02-25 2:11 AM, Pierre-Louis Bossart wrote:
> On 2/7/22 06:20, Cezary Rojewski wrote:
>> A 'Pipeline' represents both a container of module instances, and a
>> scheduling entity. Multiple pipelines can be bound together to create an
>> audio graph. The Pipeline state machine is entirely controlled by IPCs
>> (creation, deletion and state changes).
> 
> How are the module instances connected within a pipeline? You've said
> too much or too little here.


Hmm.. I doubt commit messages is the place to bring up entire FW 
specification. A high level description is provided to give a 
maintainer/reviewer idea of what the pipeline is. Perhaps s/module 
instances/modules/ would suffice.

>> +int avs_ipc_create_pipeline(struct avs_dev *adev, u16 req_size, u8 priority,
>> +			    u8 instance_id, bool lp, u16 attributes)
>> +{
>> +	union avs_global_msg msg = AVS_GLOBAL_REQUEST(CREATE_PIPELINE);
>> +	struct avs_ipc_msg request = {0};
>> +	int ret;
>> +
>> +	msg.create_ppl.ppl_mem_size = req_size;
>> +	msg.create_ppl.ppl_priority = priority;
>> +	msg.create_ppl.instance_id = instance_id;
>> +	msg.ext.create_ppl.lp = lp;
> 
> you may want to describe what the concepts of 'priority', 'lp' and
> 'attributes' are and which entity defines the values (topology?)


These fields match firmware equivalents 1:1 and are part of pipeline 
descriptor excepted by firmware when initializing a pipeline. Handlers 
found in messages.c are responsible for one and only one task only: 
sending a concrete message. Part of the driver that implements PCM 
operations (not part of this series) cares about the topology (where 
these values actually come from) and invokes the necessary IPCs.

>> +	msg.ext.create_ppl.attributes = attributes;
>> +	request.header = msg.val;
>> +
>> +	ret = avs_dsp_send_msg(adev, &request, NULL);
>> +	if (ret)
>> +		avs_ipc_err(adev, &request, "create pipeline", ret);
>> +
>> +	return ret;
>> +}
> 
>>   			u32 val;
>> +			/* pipeline management */
>> +			struct {
>> +				u32 lp:1;
>> +				u32 rsvd:3;
>> +				u32 attributes:16;
>> +			} create_ppl;
>> +			struct {
>> +				u32 multi_ppl:1;
>> +				u32 sync_stop_start:1;
> 
> these two are not described at all?


Ack.

>> +			} set_ppl_state;
>>   		} ext;
>>   	};
>>   } __packed;
> 
>> +/* Pipeline management messages */
>> +enum avs_pipeline_state {
>> +	AVS_PPL_STATE_INVALID,
>> +	AVS_PPL_STATE_UNINITIALIZED,
>> +	AVS_PPL_STATE_RESET,
>> +	AVS_PPL_STATE_PAUSED,
>> +	AVS_PPL_STATE_RUNNING,
>> +	AVS_PPL_STATE_EOS,
>> +	AVS_PPL_STATE_ERROR_STOP,
>> +	AVS_PPL_STATE_SAVED,
>> +	AVS_PPL_STATE_RESTORED,
> 
> can you describe that the last two enums might entail and what the
> purpose might be?
> 
> I can see how the firmware state could be saved in IMR for faster
> suspend/resume, but save/restore at the pipeline level doesn't seem to
> have an obvious match for an ASoC driver?


The enum lists all available pipeline states. We're planning to move 
these to uapi later on to allow apps to monitor running pipelines states 
real-time.
Pierre-Louis Bossart Feb. 25, 2022, 8:42 p.m. UTC | #3 
On 2/25/22 12:31, Cezary Rojewski wrote:
> On 2022-02-25 2:11 AM, Pierre-Louis Bossart wrote:
>> On 2/7/22 06:20, Cezary Rojewski wrote:
>>> A 'Pipeline' represents both a container of module instances, and a
>>> scheduling entity. Multiple pipelines can be bound together to create an
>>> audio graph. The Pipeline state machine is entirely controlled by IPCs
>>> (creation, deletion and state changes).
>>
>> How are the module instances connected within a pipeline? You've said
>> too much or too little here.
> 
> 
> Hmm.. I doubt commit messages is the place to bring up entire FW
> specification. A high level description is provided to give a
> maintainer/reviewer idea of what the pipeline is. Perhaps s/module
> instances/modules/ would suffice.

No one is asking you to to provide the entire specification, just enough
for reviewers with no access to that spec to understand what your intent is.

> 
>>> +int avs_ipc_create_pipeline(struct avs_dev *adev, u16 req_size, u8
>>> priority,
>>> +                u8 instance_id, bool lp, u16 attributes)
>>> +{
>>> +    union avs_global_msg msg = AVS_GLOBAL_REQUEST(CREATE_PIPELINE);
>>> +    struct avs_ipc_msg request = {0};
>>> +    int ret;
>>> +
>>> +    msg.create_ppl.ppl_mem_size = req_size;
>>> +    msg.create_ppl.ppl_priority = priority;
>>> +    msg.create_ppl.instance_id = instance_id;
>>> +    msg.ext.create_ppl.lp = lp;
>>
>> you may want to describe what the concepts of 'priority', 'lp' and
>> 'attributes' are and which entity defines the values (topology?)
> 
> 
> These fields match firmware equivalents 1:1 and are part of pipeline
> descriptor excepted by firmware when initializing a pipeline. Handlers
> found in messages.c are responsible for one and only one task only:
> sending a concrete message. Part of the driver that implements PCM
> operations (not part of this series) cares about the topology (where
> these values actually come from) and invokes the necessary IPCs.

add a comment then that the driver is just relaying information found in
the topology to the firmware.


>>> +/* Pipeline management messages */
>>> +enum avs_pipeline_state {
>>> +    AVS_PPL_STATE_INVALID,
>>> +    AVS_PPL_STATE_UNINITIALIZED,
>>> +    AVS_PPL_STATE_RESET,
>>> +    AVS_PPL_STATE_PAUSED,
>>> +    AVS_PPL_STATE_RUNNING,
>>> +    AVS_PPL_STATE_EOS,
>>> +    AVS_PPL_STATE_ERROR_STOP,
>>> +    AVS_PPL_STATE_SAVED,
>>> +    AVS_PPL_STATE_RESTORED,
>>
>> can you describe that the last two enums might entail and what the
>> purpose might be?
>>
>> I can see how the firmware state could be saved in IMR for faster
>> suspend/resume, but save/restore at the pipeline level doesn't seem to
>> have an obvious match for an ASoC driver?
> 
> 
> The enum lists all available pipeline states. We're planning to move
> these to uapi later on to allow apps to monitor running pipelines states
> real-time.

That doesn't answer to my question. You are not using the last two in
the rest of this patchset, so why create doubt in the confused brain on
the reviewer?
Cezary Rojewski Feb. 28, 2022, 3:25 p.m. UTC | #4 
On 2022-02-25 9:42 PM, Pierre-Louis Bossart wrote:
> On 2/25/22 12:31, Cezary Rojewski wrote:
>> On 2022-02-25 2:11 AM, Pierre-Louis Bossart wrote:
>>> On 2/7/22 06:20, Cezary Rojewski wrote:
>>>> A 'Pipeline' represents both a container of module instances, and a
>>>> scheduling entity. Multiple pipelines can be bound together to create an
>>>> audio graph. The Pipeline state machine is entirely controlled by IPCs
>>>> (creation, deletion and state changes).
>>>
>>> How are the module instances connected within a pipeline? You've said
>>> too much or too little here.
>>
>>
>> Hmm.. I doubt commit messages is the place to bring up entire FW
>> specification. A high level description is provided to give a
>> maintainer/reviewer idea of what the pipeline is. Perhaps s/module
>> instances/modules/ would suffice.
> 
> No one is asking you to to provide the entire specification, just enough
> for reviewers with no access to that spec to understand what your intent is.


Removed the confusing part of the message in v2.

>>
>>>> +int avs_ipc_create_pipeline(struct avs_dev *adev, u16 req_size, u8
>>>> priority,
>>>> +                u8 instance_id, bool lp, u16 attributes)
>>>> +{
>>>> +    union avs_global_msg msg = AVS_GLOBAL_REQUEST(CREATE_PIPELINE);
>>>> +    struct avs_ipc_msg request = {0};
>>>> +    int ret;
>>>> +
>>>> +    msg.create_ppl.ppl_mem_size = req_size;
>>>> +    msg.create_ppl.ppl_priority = priority;
>>>> +    msg.create_ppl.instance_id = instance_id;
>>>> +    msg.ext.create_ppl.lp = lp;
>>>
>>> you may want to describe what the concepts of 'priority', 'lp' and
>>> 'attributes' are and which entity defines the values (topology?)
>>
>>
>> These fields match firmware equivalents 1:1 and are part of pipeline
>> descriptor excepted by firmware when initializing a pipeline. Handlers
>> found in messages.c are responsible for one and only one task only:
>> sending a concrete message. Part of the driver that implements PCM
>> operations (not part of this series) cares about the topology (where
>> these values actually come from) and invokes the necessary IPCs.
> 
> add a comment then that the driver is just relaying information found in
> the topology to the firmware.


Ack.

>>>> +/* Pipeline management messages */
>>>> +enum avs_pipeline_state {
>>>> +    AVS_PPL_STATE_INVALID,
>>>> +    AVS_PPL_STATE_UNINITIALIZED,
>>>> +    AVS_PPL_STATE_RESET,
>>>> +    AVS_PPL_STATE_PAUSED,
>>>> +    AVS_PPL_STATE_RUNNING,
>>>> +    AVS_PPL_STATE_EOS,
>>>> +    AVS_PPL_STATE_ERROR_STOP,
>>>> +    AVS_PPL_STATE_SAVED,
>>>> +    AVS_PPL_STATE_RESTORED,
>>>
>>> can you describe that the last two enums might entail and what the
>>> purpose might be?
>>>
>>> I can see how the firmware state could be saved in IMR for faster
>>> suspend/resume, but save/restore at the pipeline level doesn't seem to
>>> have an obvious match for an ASoC driver?
>>
>>
>> The enum lists all available pipeline states. We're planning to move
>> these to uapi later on to allow apps to monitor running pipelines states
>> real-time.
> 
> That doesn't answer to my question. You are not using the last two in
> the rest of this patchset, so why create doubt in the confused brain on
> the reviewer?


Removed both in v2, thanks.
diff mbox series

Patch

diff --git a/sound/soc/intel/avs/messages.c b/sound/soc/intel/avs/messages.c
index 8dac946dd8dd..ab13fc7809fe 100644
--- a/sound/soc/intel/avs/messages.c
+++ b/sound/soc/intel/avs/messages.c
@@ -63,3 +63,79 @@  int avs_ipc_load_library(struct avs_dev *adev, u32 dma_id, u32 lib_id)
 
 	return ret;
 }
+
+int avs_ipc_create_pipeline(struct avs_dev *adev, u16 req_size, u8 priority,
+			    u8 instance_id, bool lp, u16 attributes)
+{
+	union avs_global_msg msg = AVS_GLOBAL_REQUEST(CREATE_PIPELINE);
+	struct avs_ipc_msg request = {0};
+	int ret;
+
+	msg.create_ppl.ppl_mem_size = req_size;
+	msg.create_ppl.ppl_priority = priority;
+	msg.create_ppl.instance_id = instance_id;
+	msg.ext.create_ppl.lp = lp;
+	msg.ext.create_ppl.attributes = attributes;
+	request.header = msg.val;
+
+	ret = avs_dsp_send_msg(adev, &request, NULL);
+	if (ret)
+		avs_ipc_err(adev, &request, "create pipeline", ret);
+
+	return ret;
+}
+
+int avs_ipc_delete_pipeline(struct avs_dev *adev, u8 instance_id)
+{
+	union avs_global_msg msg = AVS_GLOBAL_REQUEST(DELETE_PIPELINE);
+	struct avs_ipc_msg request = {0};
+	int ret;
+
+	msg.ppl.instance_id = instance_id;
+	request.header = msg.val;
+
+	ret = avs_dsp_send_msg(adev, &request, NULL);
+	if (ret)
+		avs_ipc_err(adev, &request, "delete pipeline", ret);
+
+	return ret;
+}
+
+int avs_ipc_set_pipeline_state(struct avs_dev *adev, u8 instance_id,
+			       enum avs_pipeline_state state)
+{
+	union avs_global_msg msg = AVS_GLOBAL_REQUEST(SET_PIPELINE_STATE);
+	struct avs_ipc_msg request = {0};
+	int ret;
+
+	msg.set_ppl_state.ppl_id = instance_id;
+	msg.set_ppl_state.state = state;
+	request.header = msg.val;
+
+	ret = avs_dsp_send_msg(adev, &request, NULL);
+	if (ret)
+		avs_ipc_err(adev, &request, "set pipeline state", ret);
+
+	return ret;
+}
+
+int avs_ipc_get_pipeline_state(struct avs_dev *adev, u8 instance_id,
+			       enum avs_pipeline_state *state)
+{
+	union avs_global_msg msg = AVS_GLOBAL_REQUEST(GET_PIPELINE_STATE);
+	struct avs_ipc_msg request = {0};
+	struct avs_ipc_msg reply = {0};
+	int ret;
+
+	msg.get_ppl_state.ppl_id = instance_id;
+	request.header = msg.val;
+
+	ret = avs_dsp_send_msg(adev, &request, &reply);
+	if (ret) {
+		avs_ipc_err(adev, &request, "get pipeline state", ret);
+		return ret;
+	}
+
+	*state = reply.rsp.ext.get_ppl_state.state;
+	return ret;
+}
diff --git a/sound/soc/intel/avs/messages.h b/sound/soc/intel/avs/messages.h
index b9ec1c64179b..67f7e1826e45 100644
--- a/sound/soc/intel/avs/messages.h
+++ b/sound/soc/intel/avs/messages.h
@@ -26,6 +26,10 @@  enum avs_msg_direction {
 enum avs_global_msg_type {
 	AVS_GLB_LOAD_MULTIPLE_MODULES = 15,
 	AVS_GLB_UNLOAD_MULTIPLE_MODULES = 16,
+	AVS_GLB_CREATE_PIPELINE = 17,
+	AVS_GLB_DELETE_PIPELINE = 18,
+	AVS_GLB_SET_PIPELINE_STATE = 19,
+	AVS_GLB_GET_PIPELINE_STATE = 20,
 	AVS_GLB_LOAD_LIBRARY = 24,
 	AVS_GLB_NOTIFICATION = 27,
 };
@@ -45,6 +49,23 @@  union avs_global_msg {
 			struct {
 				u32 mod_cnt:8;
 			} load_multi_mods;
+			/* pipeline management */
+			struct {
+				u32 ppl_mem_size:11;
+				u32 ppl_priority:5;
+				u32 instance_id:8;
+			} create_ppl;
+			struct {
+				u32 rsvd:16;
+				u32 instance_id:8;
+			} ppl; /* generic ppl request */
+			struct {
+				u32 state:16;
+				u32 ppl_id:8;
+			} set_ppl_state;
+			struct {
+				u32 ppl_id:8;
+			} get_ppl_state;
 			/* library loading */
 			struct {
 				u32 dma_id:5;
@@ -54,6 +75,16 @@  union avs_global_msg {
 		};
 		union {
 			u32 val;
+			/* pipeline management */
+			struct {
+				u32 lp:1;
+				u32 rsvd:3;
+				u32 attributes:16;
+			} create_ppl;
+			struct {
+				u32 multi_ppl:1;
+				u32 sync_stop_start:1;
+			} set_ppl_state;
 		} ext;
 	};
 } __packed;
@@ -101,6 +132,10 @@  union avs_reply_msg {
 			struct {
 				u32 err_mod_id:16;
 			} load_multi_mods;
+			/* pipeline management */
+			struct {
+				u32 state:5;
+			} get_ppl_state;
 		} ext;
 	};
 } __packed;
@@ -189,4 +224,25 @@  int avs_ipc_load_modules(struct avs_dev *adev, u16 *mod_ids, u32 num_mod_ids);
 int avs_ipc_unload_modules(struct avs_dev *adev, u16 *mod_ids, u32 num_mod_ids);
 int avs_ipc_load_library(struct avs_dev *adev, u32 dma_id, u32 lib_id);
 
+/* Pipeline management messages */
+enum avs_pipeline_state {
+	AVS_PPL_STATE_INVALID,
+	AVS_PPL_STATE_UNINITIALIZED,
+	AVS_PPL_STATE_RESET,
+	AVS_PPL_STATE_PAUSED,
+	AVS_PPL_STATE_RUNNING,
+	AVS_PPL_STATE_EOS,
+	AVS_PPL_STATE_ERROR_STOP,
+	AVS_PPL_STATE_SAVED,
+	AVS_PPL_STATE_RESTORED,
+};
+
+int avs_ipc_create_pipeline(struct avs_dev *adev, u16 req_size, u8 priority,
+			    u8 instance_id, bool lp, u16 attributes);
+int avs_ipc_delete_pipeline(struct avs_dev *adev, u8 instance_id);
+int avs_ipc_set_pipeline_state(struct avs_dev *adev, u8 instance_id,
+			       enum avs_pipeline_state state);
+int avs_ipc_get_pipeline_state(struct avs_dev *adev, u8 instance_id,
+			       enum avs_pipeline_state *state);
+
 #endif /* __SOUND_SOC_INTEL_AVS_MSGS_H */