Re: [RFC 00/37] ASoC: Intel: AVS - Audio DSP for cAVS

From: "Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>
To: Cezary Rojewski <cezary.rojewski@intel.com>,
	Mark Brown <broonie@kernel.org>
Cc: alsa-devel@alsa-project.org, upstream@semihalf.com,
	harshapriya.n@intel.com, peter.ujfalusi@linux.intel.com,
	rad@semihalf.com, pierre-louis.bossart@linux.intel.com,
	tiwai@suse.com, hdegoede@redhat.com,
	ranjani.sridharan@linux.intel.com, cujomalainey@chromium.org,
	yung-chuan.liao@linux.intel.com, lma@semihalf.com
Subject: Re: [RFC 00/37] ASoC: Intel: AVS - Audio DSP for cAVS
Date: Wed, 2 Feb 2022 14:26:01 +0100	[thread overview]
Message-ID: <7695fc34-143d-7715-85cb-7790386bbacc@linux.intel.com> (raw)
In-Reply-To: <ccb2f9f0-e9fa-3215-f623-bac58b4c9633@intel.com>

On 1/30/2022 8:15 PM, Cezary Rojewski wrote:
> 
> path-API found in path.h is limited and maps nicely to DAI operations:
> 
> avs_path_create()
> avs_path_bind(struct avs_path *path)
>      used during DAI's ->hw_params()
> 
> avs_path_free(struct avs_path *path)
> avs_path_unbind(struct avs_path *path)
>      used during DAI's ->hw_free()
> 
> avs_path_reset(struct avs_path *path)
> avs_path_pause(struct avs_path *path)
> avs_path_run(struct avs_path *path, int trigger)
>      state setters, used during DAI's ->prepare() and ->trigger()
> 
> given this picture, one could say that there are framework elements that 
> allow driver writer to implement whatever is needed for DSP-capable driver.

Although Cezary wrote that avs_path_reset/_pause/_run maps nicely to 
trigger operation it's not direct mapping. AVS FW has requirements on 
order of operations on pipelines (which are grouped in paths on kernel 
side). For example on TRIGGER_STOP we need to first pause all pipelines 
before issuing reset to any of them. This is required by FW, so that if 
there are two pipelines it doesn't pause and reset one of them, while 
the other one is still in running state, as this causes xruns on FW side.

Relevant fragment from "[RFC 27/37] ASoC: Intel: avs: non-HDA PCM BE 
operations"
+	case SNDRV_PCM_TRIGGER_STOP:
+		ret = avs_path_pause(data->path);
+		if (ret < 0)
+			dev_err(dai->dev, "pause BE path failed: %d\n", ret);
+
+		if (cmd == SNDRV_PCM_TRIGGER_STOP) {
+			ret = avs_path_reset(data->path);
+			if (ret < 0)
+				dev_err(dai->dev, "reset FE path failed: %d\n", ret);
+		}
+		break;
+

I would say that such behavior doesn't translate nicely to generic API.

I tried looking once again at how one would split the path concept to 
make it more generic, but it is hard. On one hand paths are tied to AVS 
driver topology design, on the other hand we have (mentioned above) FW 
requirements.

To describe it in more detail, in AVS we need topology as it describes 
bindings between paths. Simple topologies have route map similar to this 
one:

SectionGraph."ssp0_Tx_spt-audio-playback" {
     index "0"

     lines [
         "ssp0 Tx, , ssp0p_be"
         "ssp0p_be, , ssp0p_fe"
         "ssp0p_fe, , spt-audio-playback"
     ]
}

where ssp0p_be and ssp0p_fe are widgets describing BE and FE configuration.

Taking for example FE widget we have:

SectionWidget."ssp0p_fe" {
     index "0"
     type "scheduler"
     no_pm "true"
     ignore_suspend "false"

     data [
         "path_tmpl2_data"
     ]
}

where we can see that apart from its own configuration it has additional 
data describing path inside it:

SectionData."path_tmpl2_data" {
     tuples [
         "path_tmpl2_tuples"
         "path_tmpl2_path0_tuples"
         "path_tmpl2_path0_ppl0_tuples"
         "path_tmpl2_path0_ppl0_mod0_tuples"
         "path_tmpl2_path0_ppl0_bindid0_tuples"
     ]
}

now for the concept of paths the most interesting field is 
"path_tmpl2_path0_ppl0_bindid0_tuples" as it describes to which path we 
want to bind. It is done this way as FW modules internally have pins, 
and while in most cases one wants to just bind on pin 0, sometimes there 
is a need to describe more complicated connections. And so we circled 
back to FW requirements.

Overall I would say that path design in AVS is tied too much to FW 
requirements to be made generic. And even if some general API was 
provided we would still need most of current code on AVS path to handle 
the requirements, while we would have additional constrains coming from 
API above.

> And now back to the _full picture_ that I'm clearly not seeing yet. How 
> do you envision interfaces that should be added to the ASoC framework? 
> Are we talking about soc-path.c level of a change? It would be helpful 
> to have even a single operation (from the list above) drawn as an 
> example of what is expected.

Similarly to the above I'm open to suggestions on how such API may look 
like.

Best Regards,
Amadeusz