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

[Cezary Rojewski <cezary.rojewski@intel.com>]

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.