[PATCH v2 0/2] SOF: Add compress support implementation

[PATCH v2 0/2] SOF: Add compress support implementation

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

This patch series adds compress operations support. This is tested with
SOF (codec_adapter component enabled) and i.MX8/8X/8M boards.

Changes since v1:
(https://lore.kernel.org/lkml/20220113161341.371345-1-daniel.baluta@oss.nxp.com/T/)

- Addressed review from Cezary and Pierre
- fixed layout of declaration blocks to be more consistent
- avoid using rtd and runtime simultaneously inside a function (always
  used rtd and crtd)
- check return code for create_page_table
- completely remove sof_compr_stream and use snd_compr_tstmap instead to
  keep compress stream related info.·
- add get_caps and get_codec_caps implementations (in patch 2) 

Daniel Baluta (1):
  ASoC: SOF: compr: Add compress ops implementation

Paul Olaru (1):
  ASoC: SOF: compress: Implement get_caps and get_codec_caps

 sound/soc/sof/compress.c | 347 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 345 insertions(+), 2 deletions(-)

-- 
2.27.0

[PATCH v2 1/2] ASoC: SOF: compr: Add compress ops implementation

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

Implement snd_compress_ops. There are a lot of similarities with
PCM implementation.

For now we use sof_ipc_pcm_params to transfer compress parameters to SOF
firmware.

This will be changed in the future once we either add new compress
parameters to SOF or enhance existing sof_ipc_pcm_params structure
to support all native compress params.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/sof/compress.c | 273 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 271 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/compress.c b/sound/soc/sof/compress.c
index 01ca85f0b87f..91a9c95929cd 100644
--- a/sound/soc/sof/compress.c
+++ b/sound/soc/sof/compress.c
@@ -10,6 +10,22 @@
 #include "sof-audio.h"
 #include "sof-priv.h"
 
+static void sof_set_transferred_bytes(struct snd_compr_tstamp *tstamp,
+				      u64 host_pos, u64 buffer_size)
+{
+	u64 prev_pos;
+	unsigned int copied;
+
+	div64_u64_rem(tstamp->copied_total, buffer_size, &prev_pos);
+
+	if (host_pos < prev_pos)
+		copied = (buffer_size - prev_pos) + host_pos;
+	else
+		copied = host_pos - prev_pos;
+
+	tstamp->copied_total += copied;
+}
+
 static void snd_sof_compr_fragment_elapsed_work(struct work_struct *work)
 {
 	struct snd_sof_pcm_stream *sps =
@@ -29,14 +45,16 @@ void snd_sof_compr_init_elapsed_work(struct work_struct *work)
  */
 void snd_sof_compr_fragment_elapsed(struct snd_compr_stream *cstream)
 {
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_compr_runtime *crtd = cstream->runtime;
 	struct snd_soc_component *component;
-	struct snd_soc_pcm_runtime *rtd;
+	struct snd_compr_tstamp *tstamp;
 	struct snd_sof_pcm *spcm;
 
 	if (!cstream)
 		return;
 
-	rtd = cstream->private_data;
+	tstamp = crtd->private_data;
 	component = snd_soc_rtdcom_lookup(rtd, SOF_AUDIO_PCM_DRV_NAME);
 
 	spcm = snd_sof_find_spcm_dai(component, rtd);
@@ -46,6 +64,257 @@ void snd_sof_compr_fragment_elapsed(struct snd_compr_stream *cstream)
 		return;
 	}
 
+	sof_set_transferred_bytes(tstamp, spcm->stream[cstream->direction].posn.host_posn,
+				  crtd->buffer_size);
+
 	/* use the same workqueue-based solution as for PCM, cf. snd_sof_pcm_elapsed */
 	schedule_work(&spcm->stream[cstream->direction].period_elapsed_work);
 }
+
+static int create_page_table(struct snd_soc_component *component,
+			     struct snd_compr_stream *cstream,
+			     unsigned char *dma_area, size_t size)
+{
+	struct snd_dma_buffer *dmab = cstream->runtime->dma_buffer_p;
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	int dir = cstream->direction;
+	struct snd_sof_pcm *spcm;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm)
+		return -EINVAL;
+
+	return snd_sof_create_page_table(component->dev, dmab,
+					 spcm->stream[dir].page_table.area, size);
+}
+
+int sof_compr_open(struct snd_soc_component *component,
+		   struct snd_compr_stream *cstream)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_compr_runtime *crtd = cstream->runtime;
+	struct snd_compr_tstamp *tstamp;
+	struct snd_sof_pcm *spcm;
+	int dir;
+
+	tstamp = kzalloc(sizeof(*tstamp), GFP_KERNEL);
+	if (!tstamp)
+		return -ENOMEM;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm) {
+		kfree(tstamp);
+		return -EINVAL;
+	}
+
+	dir = cstream->direction;
+
+	if (spcm->stream[dir].cstream) {
+		kfree(tstamp);
+		return -EBUSY;
+	}
+
+	spcm->stream[dir].cstream = cstream;
+	spcm->stream[dir].posn.host_posn = 0;
+	spcm->stream[dir].posn.dai_posn = 0;
+	spcm->prepared[dir] = false;
+
+	crtd->private_data = tstamp;
+
+	return 0;
+}
+
+int sof_compr_free(struct snd_soc_component *component,
+		   struct snd_compr_stream *cstream)
+{
+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
+	struct snd_compr_tstamp *tstamp = cstream->runtime->private_data;
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct sof_ipc_stream stream;
+	struct sof_ipc_reply reply;
+	struct snd_sof_pcm *spcm;
+	int ret = 0;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm)
+		return -EINVAL;
+
+	stream.hdr.size = sizeof(stream);
+	stream.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_FREE;
+	stream.comp_id = spcm->stream[cstream->direction].comp_id;
+
+	if (spcm->prepared[cstream->direction]) {
+		ret = sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd,
+					 &stream, sizeof(stream),
+					 &reply, sizeof(reply));
+		if (!ret)
+			spcm->prepared[cstream->direction] = false;
+	}
+
+	cancel_work_sync(&spcm->stream[cstream->direction].period_elapsed_work);
+	spcm->stream[cstream->direction].cstream = NULL;
+	kfree(tstamp);
+
+	return ret;
+}
+
+int sof_compr_set_params(struct snd_soc_component *component,
+			 struct snd_compr_stream *cstream, struct snd_compr_params *params)
+{
+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_compr_runtime *crtd = cstream->runtime;
+	struct sof_ipc_pcm_params_reply ipc_params_reply;
+	struct snd_compr_tstamp *tstamp;
+	struct sof_ipc_pcm_params pcm;
+	struct snd_sof_pcm *spcm;
+	int ret;
+
+	tstamp = crtd->private_data;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+
+	if (!spcm)
+		return -EINVAL;
+
+	cstream->dma_buffer.dev.type = SNDRV_DMA_TYPE_DEV_SG;
+	cstream->dma_buffer.dev.dev = sdev->dev;
+	ret = snd_compr_malloc_pages(cstream, crtd->buffer_size);
+	if (ret < 0)
+		return ret;
+
+	ret = create_page_table(component, cstream, crtd->dma_area, crtd->dma_bytes);
+	if (ret < 0)
+		return ret;
+
+	memset(&pcm, 0, sizeof(pcm));
+
+	pcm.params.buffer.pages = PFN_UP(crtd->dma_bytes);
+	pcm.hdr.size = sizeof(pcm);
+	pcm.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_PARAMS;
+
+	pcm.comp_id = spcm->stream[cstream->direction].comp_id;
+	pcm.params.hdr.size = sizeof(pcm.params);
+	pcm.params.buffer.phy_addr = spcm->stream[cstream->direction].page_table.addr;
+	pcm.params.buffer.size = crtd->dma_bytes;
+	pcm.params.direction = cstream->direction;
+	pcm.params.channels = params->codec.ch_out;
+	pcm.params.rate = params->codec.sample_rate;
+	pcm.params.buffer_fmt = SOF_IPC_BUFFER_INTERLEAVED;
+	pcm.params.frame_fmt = SOF_IPC_FRAME_S32_LE;
+	pcm.params.sample_container_bytes =
+		snd_pcm_format_physical_width(SNDRV_PCM_FORMAT_S32) >> 3;
+	pcm.params.host_period_bytes = params->buffer.fragment_size;
+
+	ret = sof_ipc_tx_message(sdev->ipc, pcm.hdr.cmd, &pcm, sizeof(pcm),
+				 &ipc_params_reply, sizeof(ipc_params_reply));
+	if (ret < 0) {
+		dev_err(component->dev, "error ipc failed\n");
+		return ret;
+	}
+
+	tstamp->byte_offset = sdev->stream_box.offset + ipc_params_reply.posn_offset;
+	tstamp->sampling_rate = params->codec.sample_rate;
+
+	spcm->prepared[cstream->direction] = true;
+
+	return 0;
+}
+
+int sof_compr_get_params(struct snd_soc_component *component,
+			 struct snd_compr_stream *cstream, struct snd_codec *params)
+{
+	/* TODO: we don't query the supported codecs for now, if the
+	 * application asks for an unsupported codec the set_params() will fail.
+	 */
+	return 0;
+}
+
+int sof_compr_trigger(struct snd_soc_component *component,
+		      struct snd_compr_stream *cstream, int cmd)
+{
+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct sof_ipc_stream stream;
+	struct sof_ipc_reply reply;
+	struct snd_sof_pcm *spcm;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm)
+		return -EINVAL;
+
+	stream.hdr.size = sizeof(stream);
+	stream.hdr.cmd = SOF_IPC_GLB_STREAM_MSG;
+	stream.comp_id = spcm->stream[cstream->direction].comp_id;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_START;
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_STOP;
+		break;
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_PAUSE;
+		break;
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_RELEASE;
+		break;
+	default:
+		dev_err(component->dev, "error: unhandled trigger cmd %d\n", cmd);
+		break;
+	}
+
+	return sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd,
+				  &stream, sizeof(stream),
+				  &reply, sizeof(reply));
+}
+
+int sof_compr_copy(struct snd_soc_component *component,
+		   struct snd_compr_stream *cstream,
+		   char __user *buf, size_t count)
+{
+	struct snd_compr_runtime *rtd = cstream->runtime;
+	unsigned int offset, n;
+	void *ptr;
+	int ret;
+
+	if (count > rtd->buffer_size)
+		count = rtd->buffer_size;
+
+	div_u64_rem(rtd->total_bytes_available, rtd->buffer_size, &offset);
+	ptr = rtd->dma_area + offset;
+	n = rtd->buffer_size - offset;
+
+	if (count < n) {
+		ret = copy_from_user(ptr, buf, count);
+	} else {
+		ret = copy_from_user(ptr, buf, n);
+		ret += copy_from_user(rtd->dma_area, buf + n, count - n);
+	}
+
+	return count - ret;
+}
+
+static int sof_compr_pointer(struct snd_soc_component *component,
+			     struct snd_compr_stream *cstream,
+			     struct snd_compr_tstamp *tstamp)
+{
+	struct snd_compr_tstamp *pstamp = cstream->runtime->private_data;
+
+	tstamp->sampling_rate = pstamp->sampling_rate;
+	tstamp->copied_total = pstamp->copied_total;
+
+	return 0;
+}
+
+struct snd_compress_ops sof_compressed_ops = {
+	.open		= sof_compr_open,
+	.free		= sof_compr_free,
+	.set_params	= sof_compr_set_params,
+	.get_params	= sof_compr_get_params,
+	.trigger	= sof_compr_trigger,
+	.pointer	= sof_compr_pointer,
+	.copy		= sof_compr_copy,
+};
+EXPORT_SYMBOL(sof_compressed_ops);
-- 
2.27.0

[PATCH v2 2/2] ASoC: SOF: compress: Implement get_caps and get_codec_caps

[Daniel Baluta]

From: Paul Olaru <paul.olaru@nxp.com>

These functions are used by the userspace to determine what the firmware
supports and tools like cplay should use in terms of sample rate, bit
rate, buffer size and channel count.

The current implementation uses i.MX8 tested scenarios!

Signed-off-by: Paul Olaru <paul.olaru@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/sof/compress.c | 74 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/sound/soc/sof/compress.c b/sound/soc/sof/compress.c
index 91a9c95929cd..e3f3f309f312 100644
--- a/sound/soc/sof/compress.c
+++ b/sound/soc/sof/compress.c
@@ -308,6 +308,78 @@ static int sof_compr_pointer(struct snd_soc_component *component,
 	return 0;
 }
 
+static int sof_compr_get_caps(struct snd_soc_component *component,
+			      struct snd_compr_stream *cstream,
+			      struct snd_compr_caps *caps)
+{
+	caps->num_codecs = 3;
+	caps->min_fragment_size = 3840;
+	caps->max_fragment_size = 3840;
+	caps->min_fragments = 2;
+	caps->max_fragments = 2;
+	caps->codecs[0] = SND_AUDIOCODEC_MP3;
+	caps->codecs[1] = SND_AUDIOCODEC_AAC;
+	caps->codecs[2] = SND_AUDIOCODEC_PCM;
+
+	return 0;
+}
+
+static struct snd_compr_codec_caps caps_pcm = {
+	.num_descriptors = 1,
+	.descriptor[0].max_ch = 2,
+	.descriptor[0].sample_rates[0] = 48000,
+	.descriptor[0].num_sample_rates = 1,
+	.descriptor[0].bit_rate = {1536, 3072},
+	.descriptor[0].num_bitrates = 2,
+	.descriptor[0].profiles = SND_AUDIOPROFILE_PCM,
+	.descriptor[0].modes = 0,
+	.descriptor[0].formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
+};
+
+static struct snd_compr_codec_caps caps_mp3 = {
+	.num_descriptors = 1,
+	.descriptor[0].max_ch = 2,
+	.descriptor[0].sample_rates[0] = 48000,
+	.descriptor[0].num_sample_rates = 1,
+	.descriptor[0].bit_rate = {32, 40, 48, 56, 64, 80, 96, 112, 224, 256, 320},
+	.descriptor[0].num_bitrates = 11,
+	.descriptor[0].profiles = 0,
+	.descriptor[0].modes = SND_AUDIOCHANMODE_MP3_STEREO,
+	.descriptor[0].formats = 0,
+};
+
+static struct snd_compr_codec_caps caps_aac = {
+	.num_descriptors = 1,
+	.descriptor[0].max_ch = 2,
+	.descriptor[0].sample_rates[0] = 48000,
+	.descriptor[0].num_sample_rates = 1,
+	.descriptor[0].bit_rate = {128, 192},
+	.descriptor[0].num_bitrates = 2,
+	.descriptor[0].profiles = 0,
+	.descriptor[0].modes = 0,
+	.descriptor[0].formats = SND_AUDIOSTREAMFORMAT_MP4ADTS | SND_AUDIOSTREAMFORMAT_MP2ADTS,
+};
+
+static int sof_compr_get_codec_caps(struct snd_soc_component *component,
+				    struct snd_compr_stream *cstream,
+				    struct snd_compr_codec_caps *codec)
+{
+	switch (codec->codec) {
+	case SND_AUDIOCODEC_MP3:
+		*codec = caps_mp3;
+		break;
+	case SND_AUDIOCODEC_AAC:
+		*codec = caps_aac;
+		break;
+	case SND_AUDIOCODEC_PCM:
+		*codec = caps_pcm;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 struct snd_compress_ops sof_compressed_ops = {
 	.open		= sof_compr_open,
 	.free		= sof_compr_free,
@@ -316,5 +388,7 @@ struct snd_compress_ops sof_compressed_ops = {
 	.trigger	= sof_compr_trigger,
 	.pointer	= sof_compr_pointer,
 	.copy		= sof_compr_copy,
+	.get_caps	= sof_compr_get_caps,
+	.get_codec_caps	= sof_compr_get_codec_caps,
 };
 EXPORT_SYMBOL(sof_compressed_ops);
-- 
2.27.0

Re: [PATCH v2 2/2] ASoC: SOF: compress: Implement get_caps and get_codec_caps

[Pierre-Louis Bossart]

On 1/18/22 3:27 PM, Daniel Baluta wrote:
> From: Paul Olaru <paul.olaru@nxp.com>
> 
> These functions are used by the userspace to determine what the firmware
> supports and tools like cplay should use in terms of sample rate, bit
> rate, buffer size and channel count.
> 
> The current implementation uses i.MX8 tested scenarios!
> 
> Signed-off-by: Paul Olaru <paul.olaru@nxp.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  sound/soc/sof/compress.c | 74 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/sound/soc/sof/compress.c b/sound/soc/sof/compress.c
> index 91a9c95929cd..e3f3f309f312 100644
> --- a/sound/soc/sof/compress.c
> +++ b/sound/soc/sof/compress.c
> @@ -308,6 +308,78 @@ static int sof_compr_pointer(struct snd_soc_component *component,
>  	return 0;
>  }
>  
> +static int sof_compr_get_caps(struct snd_soc_component *component,
> +			      struct snd_compr_stream *cstream,
> +			      struct snd_compr_caps *caps)
> +{
> +	caps->num_codecs = 3;
> +	caps->min_fragment_size = 3840;
> +	caps->max_fragment_size = 3840;
> +	caps->min_fragments = 2;
> +	caps->max_fragments = 2;
> +	caps->codecs[0] = SND_AUDIOCODEC_MP3;
> +	caps->codecs[1] = SND_AUDIOCODEC_AAC;
> +	caps->codecs[2] = SND_AUDIOCODEC_PCM;

I don't think you can add this unconditionally for all
devices/platforms, clearly this wouldn't be true for Intel for now.

If the information is not part of a firmware manifest or topology, then
it's likely we have to use an abstraction layer to add this for specific
platforms.

it's really a bit odd to hard-code all of this at the kernel level, this
was not really what I had in mind when we come up with the concept of
querying capabilities. I understand though that for testing this is
convenient, so maybe this can become a set of fall-back properties in
case the firmware doesn't advertise anything.

> +
> +	return 0;
> +}
> +
> +static struct snd_compr_codec_caps caps_pcm = {
> +	.num_descriptors = 1,
> +	.descriptor[0].max_ch = 2,
> +	.descriptor[0].sample_rates[0] = 48000,
> +	.descriptor[0].num_sample_rates = 1,
> +	.descriptor[0].bit_rate = {1536, 3072},
> +	.descriptor[0].num_bitrates = 2,
> +	.descriptor[0].profiles = SND_AUDIOPROFILE_PCM,
> +	.descriptor[0].modes = 0,
> +	.descriptor[0].formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
> +};
> +
> +static struct snd_compr_codec_caps caps_mp3 = {
> +	.num_descriptors = 1,
> +	.descriptor[0].max_ch = 2,
> +	.descriptor[0].sample_rates[0] = 48000,
> +	.descriptor[0].num_sample_rates = 1,
> +	.descriptor[0].bit_rate = {32, 40, 48, 56, 64, 80, 96, 112, 224, 256, 320},
> +	.descriptor[0].num_bitrates = 11,
> +	.descriptor[0].profiles = 0,
> +	.descriptor[0].modes = SND_AUDIOCHANMODE_MP3_STEREO,
> +	.descriptor[0].formats = 0,
> +};
> +
> +static struct snd_compr_codec_caps caps_aac = {
> +	.num_descriptors = 1,
> +	.descriptor[0].max_ch = 2,
> +	.descriptor[0].sample_rates[0] = 48000,
> +	.descriptor[0].num_sample_rates = 1,
> +	.descriptor[0].bit_rate = {128, 192},
> +	.descriptor[0].num_bitrates = 2,
> +	.descriptor[0].profiles = 0,
> +	.descriptor[0].modes = 0,
> +	.descriptor[0].formats = SND_AUDIOSTREAMFORMAT_MP4ADTS | SND_AUDIOSTREAMFORMAT_MP2ADTS,
> +};
> +
> +static int sof_compr_get_codec_caps(struct snd_soc_component *component,
> +				    struct snd_compr_stream *cstream,
> +				    struct snd_compr_codec_caps *codec)
> +{
> +	switch (codec->codec) {
> +	case SND_AUDIOCODEC_MP3:
> +		*codec = caps_mp3;
> +		break;
> +	case SND_AUDIOCODEC_AAC:
> +		*codec = caps_aac;
> +		break;
> +	case SND_AUDIOCODEC_PCM:
> +		*codec = caps_pcm;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  struct snd_compress_ops sof_compressed_ops = {
>  	.open		= sof_compr_open,
>  	.free		= sof_compr_free,
> @@ -316,5 +388,7 @@ struct snd_compress_ops sof_compressed_ops = {
>  	.trigger	= sof_compr_trigger,
>  	.pointer	= sof_compr_pointer,
>  	.copy		= sof_compr_copy,
> +	.get_caps	= sof_compr_get_caps,
> +	.get_codec_caps	= sof_compr_get_codec_caps,
>  };
>  EXPORT_SYMBOL(sof_compressed_ops);
> 

Re: [PATCH v2 2/2] ASoC: SOF: compress: Implement get_caps and get_codec_caps

[Daniel Baluta]

On 1/19/22 3:00 AM, Pierre-Louis Bossart wrote:
> 
> 
> On 1/18/22 3:27 PM, Daniel Baluta wrote:
>> From: Paul Olaru <paul.olaru@nxp.com>
>>
>> These functions are used by the userspace to determine what the firmware
>> supports and tools like cplay should use in terms of sample rate, bit
>> rate, buffer size and channel count.
>>
>> The current implementation uses i.MX8 tested scenarios!
>>
>> Signed-off-by: Paul Olaru <paul.olaru@nxp.com>
>> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>> ---
>>   sound/soc/sof/compress.c | 74 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 74 insertions(+)
>>
>> diff --git a/sound/soc/sof/compress.c b/sound/soc/sof/compress.c
>> index 91a9c95929cd..e3f3f309f312 100644
>> --- a/sound/soc/sof/compress.c
>> +++ b/sound/soc/sof/compress.c
>> @@ -308,6 +308,78 @@ static int sof_compr_pointer(struct snd_soc_component *component,
>>   	return 0;
>>   }
>>   
>> +static int sof_compr_get_caps(struct snd_soc_component *component,
>> +			      struct snd_compr_stream *cstream,
>> +			      struct snd_compr_caps *caps)
>> +{
>> +	caps->num_codecs = 3;
>> +	caps->min_fragment_size = 3840;
>> +	caps->max_fragment_size = 3840;
>> +	caps->min_fragments = 2;
>> +	caps->max_fragments = 2;
>> +	caps->codecs[0] = SND_AUDIOCODEC_MP3;
>> +	caps->codecs[1] = SND_AUDIOCODEC_AAC;
>> +	caps->codecs[2] = SND_AUDIOCODEC_PCM;
> 
> I don't think you can add this unconditionally for all
> devices/platforms, clearly this wouldn't be true for Intel for now.
> 
> If the information is not part of a firmware manifest or topology, then
> it's likely we have to use an abstraction layer to add this for specific
> platforms.
> 
> it's really a bit odd to hard-code all of this at the kernel level, this
> was not really what I had in mind when we come up with the concept of
> querying capabilities. I understand though that for testing this is
> convenient, so maybe this can become a set of fall-back properties in
> case the firmware doesn't advertise anything.

I see your point. I think for the moment I will remove this patch
until I will come with a better solution.

One important thing is: where do we advertise the supported parameters:

1) topology.
2) codec component instance (codec adapter) inside FW.
3) Linux kernel side based on some info about the current running platform.

Unfortunately, most of the existing users of this interface really do 
hardcode supported params:

e.g

intel/atom/sst/sst_drv_interface.c
qcom/qdsp6/q6asm-dai.c
uniphier/aio-compress.c

But that's because I think they only support one single platform family 
which has same capabilities.


> 
>> +
>> +	return 0;
>> +}
>> +
>> +static struct snd_compr_codec_caps caps_pcm = {
>> +	.num_descriptors = 1,
>> +	.descriptor[0].max_ch = 2,
>> +	.descriptor[0].sample_rates[0] = 48000,
>> +	.descriptor[0].num_sample_rates = 1,
>> +	.descriptor[0].bit_rate = {1536, 3072},
>> +	.descriptor[0].num_bitrates = 2,
>> +	.descriptor[0].profiles = SND_AUDIOPROFILE_PCM,
>> +	.descriptor[0].modes = 0,
>> +	.descriptor[0].formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
>> +};
>> +
>> +static struct snd_compr_codec_caps caps_mp3 = {
>> +	.num_descriptors = 1,
>> +	.descriptor[0].max_ch = 2,
>> +	.descriptor[0].sample_rates[0] = 48000,
>> +	.descriptor[0].num_sample_rates = 1,
>> +	.descriptor[0].bit_rate = {32, 40, 48, 56, 64, 80, 96, 112, 224, 256, 320},
>> +	.descriptor[0].num_bitrates = 11,
>> +	.descriptor[0].profiles = 0,
>> +	.descriptor[0].modes = SND_AUDIOCHANMODE_MP3_STEREO,
>> +	.descriptor[0].formats = 0,
>> +};
>> +
>> +static struct snd_compr_codec_caps caps_aac = {
>> +	.num_descriptors = 1,
>> +	.descriptor[0].max_ch = 2,
>> +	.descriptor[0].sample_rates[0] = 48000,
>> +	.descriptor[0].num_sample_rates = 1,
>> +	.descriptor[0].bit_rate = {128, 192},
>> +	.descriptor[0].num_bitrates = 2,
>> +	.descriptor[0].profiles = 0,
>> +	.descriptor[0].modes = 0,
>> +	.descriptor[0].formats = SND_AUDIOSTREAMFORMAT_MP4ADTS | SND_AUDIOSTREAMFORMAT_MP2ADTS,
>> +};
>> +
>> +static int sof_compr_get_codec_caps(struct snd_soc_component *component,
>> +				    struct snd_compr_stream *cstream,
>> +				    struct snd_compr_codec_caps *codec)
>> +{
>> +	switch (codec->codec) {
>> +	case SND_AUDIOCODEC_MP3:
>> +		*codec = caps_mp3;
>> +		break;
>> +	case SND_AUDIOCODEC_AAC:
>> +		*codec = caps_aac;
>> +		break;
>> +	case SND_AUDIOCODEC_PCM:
>> +		*codec = caps_pcm;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
>> +
>>   struct snd_compress_ops sof_compressed_ops = {
>>   	.open		= sof_compr_open,
>>   	.free		= sof_compr_free,
>> @@ -316,5 +388,7 @@ struct snd_compress_ops sof_compressed_ops = {
>>   	.trigger	= sof_compr_trigger,
>>   	.pointer	= sof_compr_pointer,
>>   	.copy		= sof_compr_copy,
>> +	.get_caps	= sof_compr_get_caps,
>> +	.get_codec_caps	= sof_compr_get_codec_caps,
>>   };
>>   EXPORT_SYMBOL(sof_compressed_ops);
>>

Re: [PATCH v2 2/2] ASoC: SOF: compress: Implement get_caps and get_codec_caps

[Pierre-Louis Bossart]

>>>   +static int sof_compr_get_caps(struct snd_soc_component *component,
>>> +                  struct snd_compr_stream *cstream,
>>> +                  struct snd_compr_caps *caps)
>>> +{
>>> +    caps->num_codecs = 3;
>>> +    caps->min_fragment_size = 3840;
>>> +    caps->max_fragment_size = 3840;
>>> +    caps->min_fragments = 2;
>>> +    caps->max_fragments = 2;
>>> +    caps->codecs[0] = SND_AUDIOCODEC_MP3;
>>> +    caps->codecs[1] = SND_AUDIOCODEC_AAC;
>>> +    caps->codecs[2] = SND_AUDIOCODEC_PCM;
>>
>> I don't think you can add this unconditionally for all
>> devices/platforms, clearly this wouldn't be true for Intel for now.
>>
>> If the information is not part of a firmware manifest or topology, then
>> it's likely we have to use an abstraction layer to add this for specific
>> platforms.
>>
>> it's really a bit odd to hard-code all of this at the kernel level, this
>> was not really what I had in mind when we come up with the concept of
>> querying capabilities. I understand though that for testing this is
>> convenient, so maybe this can become a set of fall-back properties in
>> case the firmware doesn't advertise anything.
> 
> I see your point. I think for the moment I will remove this patch
> until I will come with a better solution.
> 
> One important thing is: where do we advertise the supported parameters:
> 
> 1) topology.
> 2) codec component instance (codec adapter) inside FW.
> 3) Linux kernel side based on some info about the current running platform.

I like the topology approach because it doesn't require an IPC to
retrieve the information and it doesn't require additional tables in the
firmware - which would increase the size for no good reason.

The topology also allows to remove support for the compressed path if
there are any concerns with memory/mcps usages in a given platform. DSPs
have finite resources and designers will need to trade off fancy noise
suppression libraries, large SRAM buffers to reduce power and compressed
audio codecs.

I think we need to have something in the firmware manifest that lists
the presence of the codec component in the build or installed libraries,
so that we can verify that topology and firmware are aligned. Otherwise
this will be really error-prone.

We've had this problem for a while now that the topology can refer to
components that are not part of the build, and it's problematic IMHO to
see an IPC error as a result of a firmware/topology mistmatch.

> Unfortunately, most of the existing users of this interface really do
> hardcode supported params:
> 
> e.g
> 
> intel/atom/sst/sst_drv_interface.c
> qcom/qdsp6/q6asm-dai.c
> uniphier/aio-compress.c
> 
> But that's because I think they only support one single platform family
> which has same capabilities.

I think Qualcomm probably has the same problems as us, the firmware can
be modified, so hard-coding in the kernel is far from ideal.

The unifier case is a bit different, IIRC they only support IEC formats
which at the kernel level can be treated as PCM. I am not sure why the
compressed interface was required here, but it's not wrong to use the
compressed interface as a domain-specific transport mechanism. Others do
it was well for audio over SPI and on the Intel side the probes are
based on a custom transport format to multiplex all the PCM data on a
single 'compressed' stream.