diff mbox series

[RFC,v2,4/6] ASoC: soc-pcm: tweak DPCM BE hw_param() call order

Message ID 1648448050-15237-5-git-send-email-spujar@nvidia.com
State New
Headers show
Series Flexible codec clock configuration | expand

Commit Message

Sameer Pujar March 28, 2022, 6:14 a.m. UTC
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.

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.

Signed-off-by: Sameer Pujar <spujar@nvidia.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(-)

Comments

Ranjani Sridharan March 28, 2022, 3:29 p.m. UTC | #1
On Mon, 2022-03-28 at 11:44 +0530, 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.
> 
> 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.
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.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;
Is this check necessary?
> +
> +	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
> +
>  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) {
What determines MAX_CLK_PROVIDER_BE? why 10? Can you use rtd->num_cpus
instead? 
Thanks,
Ranjani
Sameer Pujar March 29, 2022, 8:28 a.m. UTC | #2
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@nvidia.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.
Sameer Pujar March 29, 2022, 8:31 a.m. UTC | #3
On 28-03-2022 20:59, Ranjani Sridharan wrote:
> On Mon, 2022-03-28 at 11:44 +0530, 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.
>>
>> 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.
>>
>> Signed-off-by: Sameer Pujar<spujar@nvidia.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;
> Is this check necessary?

By default the link has "SND_SOC_DAIFMT_CBC_CFC". When no format 
(I2S/RIGHT_J etc.,) is specified, the links are mostly internal and the 
normal order can be followed.

>> +
>> +     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
>> +
>>   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) {
> What determines MAX_CLK_PROVIDER_BE? why 10? Can you use rtd->num_cpus
> instead?

There is no specific reason as why it cannot be more than 10. I mostly 
thought it would be a fair assumption to have these many clock providers 
for audio paths. I will check if such limitation can be avoided. I 
cannot rely on "rtd->num_cpus", since in my case there are two different 
rtds one acting as provider and other as consumer.
diff mbox series

Patch

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
+
 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.
+	 */
+	for (i = 0; i < count; i++) {
+
+		be = prov[i];
+
 		/* newly connected FE and BE */
 		err = dpcm_be_connect(fe, be, stream);
 		if (err < 0) {