| 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
| ||
|
Date: Tue, 29 Mar 2022 13:58:23 +0530
From: Sameer Pujar <spujar@...dia.com>
To: Amadeusz Sławiński
<amadeuszx.slawinski@...ux.intel.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 28-03-2022 20:41, Amadeusz Sławiński wrote:
> 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?
>
This patch makes provider calls to happen first.
>>
>> 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.
The dpcm_be_connect() call adds the new connection to a list using
list_add() which would be a stack. When dpcm_be_connect() is deferred
for provider BEs, these occupy top of the stack. When operating on this
list during hw_params() stage, the provider call happen first. Is this
part clear now? I can rephrase the comments/commit message for more clarity.
>
>
> Overall I don't exactly understand correct order of events after reading
> commit message and patch...
>
Consider there are two BEs (BE1 and BE2) and "BE1<==>BE2" can be an I2S
interface for example. I am trying to get following sequence.
1. When BE1 is provider and BE2 is consumer, the call sequence expected
is : hw_params(BE1) -> hw_params(BE2).
2. When BE2 is provider and BE1 is consumer, the call sequence expected
is : hw_params(BE2) -> hw_params(BE1).
Idea is to make use of standard clock functions for rate info. Provider
can use clk_set_rate() and consumer can clk_get_rate().
>> 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;
> }
>
Thanks for the suggestion. I will check if loop copy can be avoided.
>> +
>> 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.
Sorry about this. I will fix it.
Powered by blists - more mailing lists