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

[Cezary Rojewski]

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.