Re: [PATCH v3 7/7] ASoC: Intel: avs: SoundWire PCM BE operations

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

I am afraid this patch needs to be completely re-visited. You mixed dai
and dailink layers, and that explains most of the misunderstandings in
the review of the PDI stuff where I mentioned that what matters is how
the dailink is created.

> +static int avs_sdw_dai_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct sdw_stream_runtime *sdw_stream;
> +	struct snd_soc_acpi_mach *mach;
> +	struct snd_soc_dai *codec_dai;
> +	struct avs_dma_data *data;
> +	struct sdw_cdns_pdi *pdi;
> +	struct sdw_slave *codec;
> +	int dir, ret;
> +
> +	mach = dev_get_platdata(dai->component->card->dev);
> +	codec = mach->pdata;
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> +		dir = SDW_DATA_DIR_RX;
> +	else
> +		dir = SDW_DATA_DIR_TX;
> +
> +	ret = avs_dai_startup(substream, dai);
> +	if (ret)
> +		return ret;
> +
> +	sdw_stream = sdw_alloc_stream(substream->name, SDW_STREAM_PCM);
> +	if (!sdw_stream) {
> +		ret = -ENOMEM;
> +		goto err_alloc_stream;
> +	}

This isn't quite the intented use of the SoundWire stream API, which
provides a sdw_startup_stream() that's supposed to be invoked during the
dailink creation in the machine driver that binds cpu and codec dais

"
 sdw_alloc_stream should be called only once per stream. Typically
 invoked from ALSA/ASoC machine/platform driver.
"


The dailink maps exactly 1:1 to the SoundWire stream concept, and that's
what allows us to deal with aggregation - when multiple cpu_dais on
multiple links are connected to multiple amps. That's the #1 reason why
SoundWire exists and the #1 difference to other audio interfaces.

BPT/BRA support is the only case where sdw_alloc_stream() is used in the
Intel platform driver, since there's clearly no concept of cpu_ or codec
DAI.

> +
> +	pdi = sdw_cdns_assign_pdi(codec->bus, dir);
> +	if (!pdi) {
> +		ret = -EBUSY;
> +		goto err_assign_pdi;
> +	}
> +
> +	/* Fulfill the codec needs by setting the sdw_stream. */
> +	codec_dai = snd_soc_rtd_to_codec(rtd, 0);
> +	snd_soc_dai_set_stream(codec_dai, sdw_stream, substream->stream);

That absolutely does not scale to the case where you have multiple
cpu_dais and multiple codec_dais.

Please take a look at the sof_sdw machine driver (same solution used by
AMD), and please use the sdw_utils library to deal with all the stream
management.

> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +	data->sdw_stream = sdw_stream;
> +	data->pdi = pdi;
> +	return 0;
> +
> +err_assign_pdi:
> +	sdw_release_stream(sdw_stream);
> +err_alloc_stream:
> +	avs_dai_shutdown(substream, dai);
> +	return ret;
> +}