Message-ID: <2e9d903f-dd76-5b22-77ea-5fc42be306fd@microchip.com>
Date:   Wed, 24 Mar 2021 09:51:14 +0000
From:   <Codrin.Ciubotariu@...rochip.com>
To:     <pierre-louis.bossart@...ux.intel.com>,
        <alsa-devel@...a-project.org>, <linux-kernel@...r.kernel.org>
CC:     <gustavoars@...nel.org>, <tiwai@...e.com>, <lgirdwood@...il.com>,
        <mirq-linux@...e.qmqm.pl>, <broonie@...nel.org>
Subject: Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones

On 23.03.2021 21:25, Pierre-Louis Bossart wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> On 3/23/21 6:43 AM, Codrin Ciubotariu wrote:
>> HW constraints are needed to set limitations for HW parameters used to
>> configure the DAIs. All DAIs on the same link must agree upon the HW
>> parameters, so the parameters are affected by the DAIs' features and
>> their limitations. In case of DPCM, the FE DAIs can be used to perform
>> different kind of conversions, such as format or rate changing, bringing
>> the audio stream to a configuration supported by all the DAIs of the BE's
>> link. For this reason, the limitations of the BE DAIs are not always
>> important for the HW parameters between user-space and FE, only for the
>> paratemers between FE and BE DAI links. This brings us to this patch-set,
>> which aims to separate the FE HW constraints from the BE HW constraints.
>> This way, the FE can be used to perform more efficient HW conversions, on
>> the initial audio stream parameters, to parameters supported by the BE
>> DAIs.
>> To achieve this, the first thing needed is to detect whether a HW
>> constraint rule is enforced by a FE or a BE DAI. This means that
>> snd_pcm_hw_rule_add() needs to be able to differentiate between the two
>> type of DAIs. For this, the runtime pointer to struct snd_pcm_runtime is
>> replaced with a pointer to struct snd_pcm_substream, to be able to reach
>> substream->pcm->internal to differentiate between FE and BE DAIs.
>> This change affects many sound drivers (and one gpu drm driver).
>> All these changes are included in the first patch, to have a better
>> overview of the implications created by this change.
>> The second patch adds a new struct snd_pcm_hw_constraints under struct
>> snd_soc_dpcm_runtime, which is used to store the HW constraint rules
>> added by the BE DAIs. This structure is initialized with a subset of the
>> runtime constraint rules which does not include the rules that affect
>> the buffer or period size. snd_pcm_hw_rule_add() will add the BE rules
>> to the new struct snd_pcm_hw_constraints.
>> The third and last patch will apply the BE rule constraints, after the
>> fixup callback. If the fixup HW parameters do not respect the BE
>> constraint rules, the rules will exit with an error. The FE mask and
>> interval constraints are copied to the BE ones, to satisfy the
>> dai_link->dpcm_merged_* flags. The dai_link->dpcm_merged_* flags are
>> used to know if the FE does format or sampling rate conversion.
>>
>> I tested with ad1934 and wm8731 codecs as BEs, with a not-yet-mainlined
>> ASRC as FE, that can also do format conversion. I realize that the
>> change to snd_pcm_hw_rule_add() has a big impact, even though all the
>> drivers use snd_pcm_hw_rule_add() with substream->runtime, so passing
>> substream instead of runtime is not that risky.
> 
> can you use the BE hw_params_fixup instead?
> 
> That's what we use for SOF.
> 
> The FE hw_params are propagated to the BE, and then the BE can update
> the hw_params based on its own limitations and pass the result
> downstream, e.g. to a codec.
> 
> I'll copy below my understanding of the flow, which we discussed
> recently in the SOF team:
> 
> my understanding is that we start with the front-end hw_params as the
> basis for the back-end hw_params.
> 
> static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
>                   struct snd_pcm_hw_params *params)
> {
>      struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
>      int ret, stream = substream->stream;
> 
>      mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
>      dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
> 
>      memcpy(&fe->dpcm[stream].hw_params, params,
>              sizeof(struct snd_pcm_hw_params));
>      ret = dpcm_be_dai_hw_params(fe, stream);
> <<< the BE is handled first.
>      if (ret < 0) {
>          dev_err(fe->dev,"ASoC: hw_params BE failed %d\n", ret);
>          goto out;
>      }
> 
>      dev_dbg(fe->dev, "ASoC: hw_params FE %s rate %d chan %x fmt %d\n",
>              fe->dai_link->name, params_rate(params),
>              params_channels(params), params_format(params));
> 
>      /* call hw_params on the frontend */
>      ret = soc_pcm_hw_params(substream, params);
> 
> then each BE will be configured
> 
> int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
> {
>      struct snd_soc_dpcm *dpcm;
>      int ret;
> 
>      for_each_dpcm_be(fe, stream, dpcm) {
> 
>          struct snd_soc_pcm_runtime *be = dpcm->be;
>          struct snd_pcm_substream *be_substream =
>              snd_soc_dpcm_get_substream(be, stream);
> 
>          /* is this op for this BE ? */
>          if (!snd_soc_dpcm_be_can_update(fe, be, stream))
>              continue;
> 
>          /* copy params for each dpcm */
>          memcpy(&dpcm->hw_params, &fe->dpcm[stream].hw_params,
>                  sizeof(struct snd_pcm_hw_params));
> 
>          /* perform any hw_params fixups */
>          ret = snd_soc_link_be_hw_params_fixup(be, &dpcm->hw_params);
> 
> The fixup is the key, in SOF this is where we are going to look for
> information from the topology.
> 
> /* fixup the BE DAI link to match any values from topology */
> int sof_pcm_dai_link_fixup(struct snd_soc_pcm_runtime *rtd, struct
> snd_pcm_hw_params *params)
> {
>      struct snd_interval *rate = hw_param_interval(params,
>              SNDRV_PCM_HW_PARAM_RATE);
>      struct snd_interval *channels = hw_param_interval(params,
>                          SNDRV_PCM_HW_PARAM_CHANNELS);
>      struct snd_mask *fmt = hw_param_mask(params,
> SNDRV_PCM_HW_PARAM_FORMAT);
>      struct snd_soc_component *component =
>          snd_soc_rtdcom_lookup(rtd, SOF_AUDIO_PCM_DRV_NAME);
>      struct snd_sof_dai *dai =
>          snd_sof_find_dai(component, (char *)rtd->dai_link->name);
>      struct snd_soc_dpcm *dpcm;
> 
>      /* no topology exists for this BE, try a common configuration */
>      if (!dai) {
>          dev_warn(component->dev,
>               "warning: no topology found for BE DAI %s config\n",
>               rtd->dai_link->name);
> 
>          /*  set 48k, stereo, 16bits by default */
>          rate->min = 48000;
>          rate->max = 48000;
> 
>          channels->min = 2;
>          channels->max = 2;
> 
>          snd_mask_none(fmt);
>          snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
> 
>          return 0;
>      }
> 
>      /* read format from topology */
>      snd_mask_none(fmt);
> 
>      switch (dai->comp_dai.config.frame_fmt) {
>      case SOF_IPC_FRAME_S16_LE:
>          snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
>          break;
>      case SOF_IPC_FRAME_S24_4LE:
>          snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);
>          break;
>      case SOF_IPC_FRAME_S32_LE:
>          snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S32_LE);
>          break;
>      default:
>          dev_err(component->dev, "error: No available DAI format!\n");
>          return -EINVAL;
>      }
> 
>      /* read rate and channels from topology */
>      switch (dai->dai_config->type) {
>      case SOF_DAI_INTEL_SSP:
>          rate->min = dai->dai_config->ssp.fsync_rate;
>          rate->max = dai->dai_config->ssp.fsync_rate;
>          channels->min = dai->dai_config->ssp.tdm_slots;
>          channels->max = dai->dai_config->ssp.tdm_slots;

I am using hw_params_fixup, but it's not enough. The first thing I do is 
to not add the BE HW constraint rules in runtime->hw_constraints, 
because this will potentially affect the FE HW params. If the FE does 
sampling rate conversion, for example, applying the sampling rate 
constrain rules from a BE codec on FE is useless. The second thing I do 
is to apply these BE HW constraint rules to the BE HW params. It's true 
that the BE HW params can be fine tuned via hw_params_fixup (topology, 
device-tree or even static parameters) and set in such a way that avoid 
the BE HW constraints, so we could ignore the constraint rules added by 
their drivers. It's not every elegant and running the BE HW constraint 
rules for the fixup BE HW params can be a sanity check. Also, I am 
thinking that if the FE does no conversion (be_hw_params_fixup missing) 
and more than one BEs are available, applying the HW constraint rules on 
the same set of BE HW params could rule out the incompatible BE DAI 
links and start the compatible ones only. I am not sure this is a real 
usecase.

Thank you for your insights!

Best regards,
Codrin

Powered by blists - more mailing lists