| 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
| ||
|
Message-ID: <6ae534e4-25f8-5825-a2ec-cb1c512dac57@linux.intel.com>
Date: Mon, 28 Mar 2022 17:11:48 +0200
From: Amadeusz Sławiński
<amadeuszx.slawinski@...ux.intel.com>
To: Sameer Pujar <spujar@...dia.com>, broonie@...nel.org,
lgirdwood@...il.com, robh+dt@...nel.org, krzk+dt@...nel.org,
perex@...ex.cz, tiwai@...e.com, peter.ujfalusi@...ux.intel.com,
pierre-louis.bossart@...ux.intel.com
Cc: oder_chiou@...ltek.com, devicetree@...r.kernel.org,
alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
jonathanh@...dia.com, thierry.reding@...il.com,
linux-tegra@...r.kernel.org
Subject: Re: [RFC PATCH v2 4/6] ASoC: soc-pcm: tweak DPCM BE hw_param() call
order
On 3/28/2022 8:14 AM, Sameer Pujar wrote:
> For DPCM links, the order of hw_param() call depends on the sequence of
> BE connection to FE. It is possible that one BE link can provide clock
> to another BE link. In such cases consumer BE DAI, to get the rate set
> by provider BE DAI, can use the standard clock functions only if provider
> has already set the appropriate rate during its hw_param() stage.
Above sentence seems to suggest that consumer can set clock only after
provider has started, but code in this patch seems to do it the other
way around?
>
> Presently the order is fixed and does not depend on the provider and
> consumer relationships. So the clock rates need to be known ahead of
> hw_param() stage.
>
> This patch tweaks the hw_param() order by connecting the provider BEs
> late to a FE. With this hw_param() calls for provider BEs happen first
> and then followed by consumer BEs. The consumers can use the standard
> clk_get_rate() function to get the rate of the clock they depend on.
>
I'm bit confused by " With this hw_param() calls for provider BEs happen
first and then followed by consumer BEs. "
Aren't consumers started first and provider second? Code and previous
sentence "connecting the provider BEs late to a FE" confuse me.
Overall I don't exactly understand correct order of events after reading
commit message and patch...
> Signed-off-by: Sameer Pujar <spujar@...dia.com>
> ---
> TODO:
> * The FE link is not considered in this. For Tegra it is fine to
> call hw_params() for FE at the end. But systems, which want to apply
> this tweak for FE as well, have to extend this tweak to FE.
> * Also only DPCM is considered here. If normal links require such
> tweak, it needs to be extended.
>
> sound/soc/soc-pcm.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 9a95468..5829514 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -1442,6 +1442,29 @@ static int dpcm_prune_paths(struct snd_soc_pcm_runtime *fe, int stream,
> return prune;
> }
>
> +static bool defer_dpcm_be_connect(struct snd_soc_pcm_runtime *rtd)
> +{
> + struct snd_soc_dai *dai;
> + int i;
> +
> + if (!(rtd->dai_link->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK))
> + return false;
> +
> + if ((rtd->dai_link->dai_fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) ==
> + SND_SOC_DAIFMT_CBC_CFC) {
> +
> + for_each_rtd_cpu_dais(rtd, i, dai) {
> +
> + if (!snd_soc_dai_is_dummy(dai))
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> +#define MAX_CLK_PROVIDER_BE 10
Not sure about this define, it adds unnecessary limitation on max clock
number, can't you just run same loop twice while checking
defer_dpcm_be_connect() first time and !defer_dpcm_be_connect() second
time? defer_dpcm_be_connect() function name may need a bit of adjustment
(rtd_is_clock_consumer() maybe?), but it gets rid of the limit.
or do something like following instead of copy pasting loop twice:
rename original dpcm_add_paths() to _dpcm_add_paths() and add additional
argument and check somewhere inline:
static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream,
struct snd_soc_dapm_widget_list **list_, bool clock_consumer)
{
...
// with renamed defer_dpcm_be_connect
if (clock_consumer ^ !rtd_is_clock_consumer())
continue;
...
}
static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream,
struct snd_soc_dapm_widget_list **list_)
{
int ret;
/* start clock consumer BEs */
ret = _dpcm_add_paths(*fe, stream, **list_, true);
if (ret)
return ret;
/* start clock provider BEs */
ret = _dpcm_add_paths(*fe, stream, **list_, false);
return ret;
}
> +
> static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream,
> struct snd_soc_dapm_widget_list **list_)
> {
> @@ -1449,7 +1472,8 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream,
> struct snd_soc_dapm_widget_list *list = *list_;
> struct snd_soc_pcm_runtime *be;
> struct snd_soc_dapm_widget *widget;
> - int i, new = 0, err;
> + struct snd_soc_pcm_runtime *prov[MAX_CLK_PROVIDER_BE];
> + int i, new = 0, err, count = 0;
>
> /* Create any new FE <--> BE connections */
> for_each_dapm_widgets(list, i, widget) {
> @@ -1489,6 +1513,40 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream,
> (be->dpcm[stream].state != SND_SOC_DPCM_STATE_CLOSE))
> continue;
>
> + /* Connect clock provider BEs at the end */
> + if (defer_dpcm_be_connect(be)) {
> + if (count >= MAX_CLK_PROVIDER_BE) {
> + dev_err(fe->dev, "ASoC: too many clock provider BEs\n");
> + return -EINVAL;
> + }
> +
> + prov[count++] = be;
> + continue;
> + }
> +
> + /* newly connected FE and BE */
> + err = dpcm_be_connect(fe, be, stream);
> + if (err < 0) {
> + dev_err(fe->dev, "ASoC: can't connect %s\n",
> + widget->name);
> + break;
> + } else if (err == 0) /* already connected */
> + continue;
> +
> + /* new */
> + dpcm_set_be_update_state(be, stream, SND_SOC_DPCM_UPDATE_BE);
> + new++;
> + }
> +
> + /*
> + * Now connect clock provider BEs. A late connection means,
> + * these BE links appear first in the list maintained by FE
> + * and hw_param() call for these happen first.
Let's stick to ALSA terminology, hw_params() please, same in commit message.
> + */
> + for (i = 0; i < count; i++) {
> +
> + be = prov[i];
> +
> /* newly connected FE and BE */
> err = dpcm_be_connect(fe, be, stream);
> if (err < 0) {
Powered by blists - more mailing lists