Re: [PATCH v2 2/2] ASoC: SOF: compress: Implement get_caps and get_codec_caps - Pierre-Louis Bossart

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Daniel Baluta <daniel.baluta@oss.nxp.com>,
	lgirdwood@gmail.com, broonie@kernel.org
Cc: alsa-devel@alsa-project.org, kai.vehmanen@linux.intel.com,
	cezary.rojewski@intel.com, linux-kernel@vger.kernel.org,
	ranjani.sridharan@linux.intel.com, paul.olaru@nxp.com,
	daniel.baluta@nxp.com, sound-open-firmware@alsa-project.org
Subject: Re: [PATCH v2 2/2] ASoC: SOF: compress: Implement get_caps and get_codec_caps
Date: Tue, 18 Jan 2022 19:00:35 -0600	[thread overview]
Message-ID: <41ae6093-8e27-01d4-e532-8a28fb1d9cf1@linux.intel.com> (raw)
In-Reply-To: <20220118212732.281657-3-daniel.baluta@oss.nxp.com>

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);
> 