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

[Pierre-Louis Bossart]

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?