linux-kernel - Re: [RFC PATCH v2 4/6] ASoC: fsl_asrc_m2m: Add memory to memory function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAA+D8ANDAxS42=9zOLQY_h_ihvJCmaXzE+_iZyxbSuikqt1CBw@mail.gmail.com>
Date: Tue, 20 Aug 2024 10:53:10 +0800
From: Shengjiu Wang <shengjiu.wang@...il.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc: Shengjiu Wang <shengjiu.wang@....com>, vkoul@...nel.org, perex@...ex.cz, tiwai@...e.com, 
	alsa-devel@...a-project.org, linux-sound@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Xiubo.Lee@...il.com, festevam@...il.com, 
	nicoleotsuka@...il.com, lgirdwood@...il.com, broonie@...nel.org, 
	linuxppc-dev@...ts.ozlabs.org
Subject: Re: [RFC PATCH v2 4/6] ASoC: fsl_asrc_m2m: Add memory to memory function

On Mon, Aug 19, 2024 at 3:42 PM Pierre-Louis Bossart
<pierre-louis.bossart@...ux.intel.com> wrote:
>
>
>
> On 8/16/24 12:42, Shengjiu Wang wrote:
> > Implement the ASRC memory to memory function using
> > the compress framework, user can use this function with
> > compress ioctl interface.
> >
> > Define below private metadata key value for output
> > format, output rate and ratio modifier configuration.
> > ASRC_OUTPUT_FORMAT 0x80000001
> > ASRC_OUTPUT_RATE   0x80000002
> > ASRC_RATIO_MOD     0x80000003
>
> Can the output format/rate change at run-time?

Seldom I think.

>
> If no, then these parameters should be moved somewhere else - e.g.
> hw_params or something.

This means I will do some changes in compress_params.h, add
output format and output rate definition, follow Jaroslav's example
right?


>
> I am still not very clear on the expanding the SET_METADATA ioctl to
> deal with the ratio changes. This isn't linked to the control layer as
> suggested before, and there's no precedent of calling it multiple times
> during streaming.

Which control layer? if you means the snd_kcontrol_new?  it is bound
with sound card,  but in my case,  I need to the control bind with
the snd_compr_stream to support multi streams/instances.

>
> I also wonder how it was tested since tinycompress does not support this?

I wrote a unit test to test these ASRC M2M functions.

>
>
> > +static int fsl_asrc_m2m_fill_codec_caps(struct fsl_asrc *asrc,
> > +                                     struct snd_compr_codec_caps *codec)
> > +{
> > +     struct fsl_asrc_m2m_cap cap;
> > +     __u32 rates[MAX_NUM_BITRATES];
> > +     snd_pcm_format_t k;
> > +     int i = 0, j = 0;
> > +     int ret;
> > +
> > +     ret = asrc->m2m_get_cap(&cap);
> > +     if (ret)
> > +             return -EINVAL;
> > +
> > +     if (cap.rate_in & SNDRV_PCM_RATE_5512)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_5512);
>
> this doesn't sound compatible with the patch2 definitions?
>
> cap->rate_in = SNDRV_PCM_RATE_8000_768000;

This ASRC M2M driver is designed for two kinds of hw ASRC modules.

one cap is : cap->rate_in = SNDRV_PCM_RATE_8000_192000 | SNDRV_PCM_RATE_5512;
another is : cap->rate_in = SNDRV_PCM_RATE_8000_768000;
they are in patch2 and patch3


>
> > +     if (cap.rate_in & SNDRV_PCM_RATE_8000)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_8000);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_11025)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_11025);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_16000)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_16000);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_22050)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_22050);
>
> missing 24 kHz

There is no SNDRV_PCM_RATE_24000 in ALSA.

>
> > +     if (cap.rate_in & SNDRV_PCM_RATE_32000)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_32000);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_44100)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_44100);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_48000)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_48000);
>
> missing 64kHz

Yes, will add it.

Best regards
Shengjiu Wang

>
> > +     if (cap.rate_in & SNDRV_PCM_RATE_88200)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_88200);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_96000)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_96000);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_176400)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_176400);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_192000)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_192000);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_352800)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_352800);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_384000)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_384000);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_705600)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_705600);
> > +     if (cap.rate_in & SNDRV_PCM_RATE_768000)
> > +             rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_768000);
> > +
> > +     pcm_for_each_format(k) {
> > +             if (pcm_format_to_bits(k) & cap.fmt_in) {
> > +                     codec->descriptor[j].max_ch = cap.chan_max;
> > +                     memcpy(codec->descriptor[j].sample_rates, rates, i * sizeof(__u32));
> > +                     codec->descriptor[j].num_sample_rates = i;
> > +                     codec->descriptor[j].formats = k;
> > +                     j++;
> > +             }
> > +     }
> > +
> > +     codec->codec = SND_AUDIOCODEC_PCM;
> > +     codec->num_descriptors = j;
> > +     return 0;
>
>