Message-ID: <CAA+D8ANqb89UavAXTiHvcHyv9uMt8_MYR9hfBaxEJDPNy5C=-g@mail.gmail.com>
Date: Tue, 20 Aug 2024 15:37:26 +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 Tue, Aug 20, 2024 at 2:59 PM Pierre-Louis Bossart
<pierre-louis.bossart@...ux.intel.com> wrote:
>
>
>
> On 8/20/24 04:53, Shengjiu Wang wrote:
> > 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?
>
> yes, having parameters for the PCM case would make sense.
>
> >> 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 can only quote Jaroslav's previous answer:
>
> "
> This argument is not valid. The controls are bound to the card, but the
> element identifiers have already iface (interface), device and subdevice
> numbers. We are using controls for PCM devices for example. The binding
> is straight.
>
> Just add SNDRV_CTL_ELEM_IFACE_COMPRESS define and specify the compress
> device number in the 'struct snd_ctl_elem_id'.
> "

I don't think it is doable,  or at least I don't know how to do it.

My case is just one card/one device/one subdevice.  can't use it to
distinguish multi streams.

>
> >> I also wonder how it was tested since tinycompress does not support this?
> >
> > I wrote a unit test to test these ASRC M2M functions.
>
> This should be shared IMHO, usually when we add/extend a new interface
> it's best to have a userspace test program that can be used by others.

After Jaroslav updates the tinycompress,  I can update this example to it.

>
> >>> +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.
>
> Right, but that doesn't mean 24kHz cannot be supported. We use
> constraints in those cases. see quote from Takashi found with a 2s
> Google search
>
> https://mailman.alsa-project.org/pipermail/alsa-devel/2013-November/069356.html
>
> "
> CONTINUOUS means that any rate between the specified min and max is
> fine, if no min or max is specified any rate is fine. KNOT means there
> are rates supported other than the standard rates defines by ALSA, but
> the other rates are enumerable. You'd typically specify them by
> explicitly listing them all and use a list constraint or you'd use one
> of the ratio constraints.
> "
>
> >>> +     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.
>

Powered by blists - more mailing lists