[v2,3/9] ASoC: audio-graph: Identify 'no_pcm' DAI links for DPCM
diff mbox series

Message ID 1596605064-27748-4-git-send-email-spujar@nvidia.com
State New
Headers show
Series
  • Audio graph card updates and usage with Tegra210 audio
Related show

Commit Message

Sameer Pujar Aug. 5, 2020, 5:24 a.m. UTC
PCM devices are created for FE dai links with 'no-pcm' flag as '0'.
Such DAI links have CPU component which implement either pcm_construct()
or pcm_new() at component or dai level respectively. Based on this,
current patch exposes a helper function to identify such components
and populate 'no_pcm' flag for DPCM DAI link.

This helps to have BE<->BE component links where PCM devices need
not be created for CPU component involved in such links.

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 sound/soc/generic/audio-graph-card.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Kuninori Morimoto Aug. 18, 2020, 2:41 a.m. UTC | #1
Hi Sameer

> PCM devices are created for FE dai links with 'no-pcm' flag as '0'.
> Such DAI links have CPU component which implement either pcm_construct()
> or pcm_new() at component or dai level respectively. Based on this,
> current patch exposes a helper function to identify such components
> and populate 'no_pcm' flag for DPCM DAI link.
> 
> This helps to have BE<->BE component links where PCM devices need
> not be created for CPU component involved in such links.
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> ---
(snip)
> @@ -259,6 +270,16 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv,
>  		if (ret < 0)
>  			goto out_put_node;
>  
> +		/*
> +		 * In BE<->BE connections it is not required to create
> +		 * PCM devices at CPU end of the dai link and thus 'no_pcm'
> +		 * flag needs to be set. It is useful when there are many
> +		 * BE components and some of these have to be connected to
> +		 * form a valid audio path.
> +		 */
> +		if (!soc_component_is_pcm(cpus))
> +			dai_link->no_pcm = 1;
> +

For safety, I want to judge with data->component_chaining.

	if (data->component_chaining &&
	    !soc_component_is_pcm(cpus))
			dai_link->no_pcm = 1;

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Kuninori Morimoto Aug. 18, 2020, 5:23 a.m. UTC | #2
Hi again

> PCM devices are created for FE dai links with 'no-pcm' flag as '0'.
> Such DAI links have CPU component which implement either pcm_construct()
> or pcm_new() at component or dai level respectively. Based on this,
> current patch exposes a helper function to identify such components
> and populate 'no_pcm' flag for DPCM DAI link.
> 
> This helps to have BE<->BE component links where PCM devices need
> not be created for CPU component involved in such links.
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> ---
(snip)
> +static bool soc_component_is_pcm(struct snd_soc_dai_link_component *dlc)
> +{
> +	struct snd_soc_dai *dai = snd_soc_find_dai(dlc);
> +
> +	if (dai && (dai->component->driver->pcm_construct ||
> +		    dai->driver->pcm_new))
> +		return true;
> +
> +	return false;
> +}

This snd_soc_find_dai() will indicate WARNING
if .config has CONFIG_LOCKDEP for me.

Maybe implement it at soc-core.c with client_mutex lock
is needed.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Sameer Pujar Aug. 18, 2020, 8:04 a.m. UTC | #3
Hi Kuninori,

On 8/18/2020 10:53 AM, Kuninori Morimoto wrote:
> External email: Use caution opening links or attachments
>
>
> Hi again
>
>> PCM devices are created for FE dai links with 'no-pcm' flag as '0'.
>> Such DAI links have CPU component which implement either pcm_construct()
>> or pcm_new() at component or dai level respectively. Based on this,
>> current patch exposes a helper function to identify such components
>> and populate 'no_pcm' flag for DPCM DAI link.
>>
>> This helps to have BE<->BE component links where PCM devices need
>> not be created for CPU component involved in such links.
>>
>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>> ---
> (snip)
>> +static bool soc_component_is_pcm(struct snd_soc_dai_link_component *dlc)
>> +{
>> +     struct snd_soc_dai *dai = snd_soc_find_dai(dlc);
>> +
>> +     if (dai && (dai->component->driver->pcm_construct ||
>> +                 dai->driver->pcm_new))
>> +             return true;
>> +
>> +     return false;
>> +}
> This snd_soc_find_dai() will indicate WARNING
> if .config has CONFIG_LOCKDEP for me.
>
> Maybe implement it at soc-core.c with client_mutex lock
> is needed.

Thank you for testing this. I will update this in next revision.

>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto
Sameer Pujar Aug. 18, 2020, 8:06 a.m. UTC | #4
Hi Kuninori,

On 8/18/2020 8:11 AM, Kuninori Morimoto wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Sameer
>
>> PCM devices are created for FE dai links with 'no-pcm' flag as '0'.
>> Such DAI links have CPU component which implement either pcm_construct()
>> or pcm_new() at component or dai level respectively. Based on this,
>> current patch exposes a helper function to identify such components
>> and populate 'no_pcm' flag for DPCM DAI link.
>>
>> This helps to have BE<->BE component links where PCM devices need
>> not be created for CPU component involved in such links.
>>
>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>> ---
> (snip)
>> @@ -259,6 +270,16 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv,
>>                if (ret < 0)
>>                        goto out_put_node;
>>
>> +             /*
>> +              * In BE<->BE connections it is not required to create
>> +              * PCM devices at CPU end of the dai link and thus 'no_pcm'
>> +              * flag needs to be set. It is useful when there are many
>> +              * BE components and some of these have to be connected to
>> +              * form a valid audio path.
>> +              */
>> +             if (!soc_component_is_pcm(cpus))
>> +                     dai_link->no_pcm = 1;
>> +
> For safety, I want to judge with data->component_chaining.
>
>          if (data->component_chaining &&
>              !soc_component_is_pcm(cpus))
>                          dai_link->no_pcm = 1;

OK. I will keep the additional check. Thanks.

>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto
Sameer Pujar Aug. 25, 2020, 3:25 a.m. UTC | #5
Hi Morimoto-san,

>> (snip)
>>> +static bool soc_component_is_pcm(struct snd_soc_dai_link_component 
>>> *dlc)
>>> +{
>>> +     struct snd_soc_dai *dai = snd_soc_find_dai(dlc);
>>> +
>>> +     if (dai && (dai->component->driver->pcm_construct ||
>>> +                 dai->driver->pcm_new))
>>> +             return true;
>>> +
>>> +     return false;
>>> +}
>> This snd_soc_find_dai() will indicate WARNING
>> if .config has CONFIG_LOCKDEP for me.
>>
>> Maybe implement it at soc-core.c with client_mutex lock
>> is needed.

I tried testing this with LOCKDEP config enabled at my end. It seems I 
don't see warning originated from above function. Are you suggesting 
that, in general, snd_soc_find_dai() should be called with client_mutex 
held?

However I do see below warning and stack which is not related to above 
function call.
   dump_backtrace+0x0/0x1c0
   show_stack+0x18/0x28
   dump_stack+0xc8/0x128
   __warn+0xa0/0x15c
   report_bug+0xc8/0x180
   bug_handler+0x20/0x80
   brk_handler+0x6c/0xc0
   do_debug_exception+0xd8/0x1f0
   el1_sync_handler+0x98/0x128
   el1_sync+0x7c/0x100
   snd_soc_find_dai+0x10c/0x120 [snd_soc_core]
   snd_soc_dai_link_set_capabilities+0xc0/0x168 [snd_soc_core]
   graph_dai_link_of_dpcm+0x3a4/0x410 [snd_soc_audio_graph_card]
   graph_for_each_link+0x174/0x220 [snd_soc_audio_graph_card]
   graph_probe+0x174/0x270 [snd_soc_audio_graph_card]

May be *snd_soc_dai_link_set_capabilities**()* requires similar fix?


Thanks,
Sameer
Kuninori Morimoto Aug. 25, 2020, 4:54 a.m. UTC | #6
Hi Sameer

>             +static bool soc_component_is_pcm(struct snd_soc_dai_link_component *dlc)
>             +{
>             +     struct snd_soc_dai *dai = snd_soc_find_dai(dlc);
>             +
>             +     if (dai && (dai->component->driver->pcm_construct ||
>             +                 dai->driver->pcm_new))
>             +             return true;
>             +
>             +     return false;
>             +}
(snip)
> I tried testing this with LOCKDEP config enabled at my end.
> It seems I don't see warning originated from above function.
> Are you suggesting that, in general, snd_soc_find_dai()
> should be called with client_mutex held?

Hmm ? strange...

snd_soc_find_dai() is using lockdep_assert_held()

	struct snd_soc_dai *snd_soc_find_dai(...)
	{
		...
=>		lockdep_assert_held(&client_mutex);
		...
	}

and lockdep_assert_held() will indicate WARN_ON()

	-- lockdep.h --
	...
	#ifdef CONFIG_LOCKDEP
	...
	#define lockdep_assert_held(l)	do {				\
=>			WARN_ON(debug_locks && !lockdep_is_held(l));	\
		} while (0)

> May be snd_soc_dai_link_set_capabilities() requires similar fix?

Yes, I'm posting fixup patch.

	https://patchwork.kernel.org/patch/11719919/

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Sameer Pujar Aug. 25, 2020, 5:15 a.m. UTC | #7
Hi Morimoto-san,


> (snip)
>> I tried testing this with LOCKDEP config enabled at my end.
>> It seems I don't see warning originated from above function.
>> Are you suggesting that, in general, snd_soc_find_dai()
>> should be called with client_mutex held?
> Hmm ? strange...

Yes indeed. For saftely I will follow the same as other callers are doing.

...

> Yes, I'm posting fixup patch.
>
>          https://patchwork.kernel.org/patch/11719919/

Just curious that why snd_soc_find_dai() itself cannot be protected, 
instead of leaving this to callers.


Thanks,
Sameer.
Kuninori Morimoto Aug. 25, 2020, 5:40 a.m. UTC | #8
Hi Sameer

> > Yes, I'm posting fixup patch.
> > 
> >          https://patchwork.kernel.org/patch/11719919/
> 
> Just curious that why snd_soc_find_dai() itself cannot be protected,
> instead of leaving this to callers.

Because, snd_soc_find_dai() is called both with/without client_mutex.
(same/sof are calling it with mutex, simple-card/audio-graph are calling without mutex)

Other solution is create both snd_soc_find_dai_with_mutex()/without_mutex().
I'm not sure which style is best.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Sameer Pujar Aug. 25, 2020, 5:53 a.m. UTC | #9
Hi Morimoto-san,

>>> Yes, I'm posting fixup patch.
>>>
>>>           https://patchwork.kernel.org/patch/11719919/
>> Just curious that why snd_soc_find_dai() itself cannot be protected,
>> instead of leaving this to callers.
> Because, snd_soc_find_dai() is called both with/without client_mutex.
> (same/sof are calling it with mutex, simple-card/audio-graph are calling without mutex)
>
> Other solution is create both snd_soc_find_dai_with_mutex()/without_mutex().
> I'm not sure which style is best.

I don't know how complex it is to have a unified solution. But if we can 
protect snd_soc_find_dai() itself, things would be simpler may be in 
long term. Right now there are separate source files for soc-core, 
soc-dai and soc-component, but because of two approaches looks like the 
function need to be moved around and need to be placed in soc-core. Also 
the issue might go unnoticed if LOCKDEP is not enabled.

May be start with a wrapper for now and eventually unify?

Thanks,
Sameer.
Kuninori Morimoto Aug. 25, 2020, 6:46 a.m. UTC | #10
Hi Sameer

> > Other solution is create both snd_soc_find_dai_with_mutex()/without_mutex().
> > I'm not sure which style is best.
> 
> I don't know how complex it is to have a unified solution. But if we
> can protect snd_soc_find_dai() itself, things would be simpler may be
> in long term. Right now there are separate source files for soc-core,
> soc-dai and soc-component, but because of two approaches looks like
> the function need to be moved around and need to be placed in
> soc-core. Also the issue might go unnoticed if LOCKDEP is not enabled.
> 
> May be start with a wrapper for now and eventually unify?

Yeah, it seems has _with_mutex() can be better idea.
I'm posting patch, but I noticed that Mark's branch vs Linus branch
have some mismatch (?), and now I'm asking it to him.
I can post _with_mutex() version as v2 if I could get answer.
After that I'm happy your next patch can re-use it.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Sameer Pujar Aug. 25, 2020, 7:18 a.m. UTC | #11
Hi Morimoto-san,

>>> Other solution is create both snd_soc_find_dai_with_mutex()/without_mutex().
>>> I'm not sure which style is best.
>> I don't know how complex it is to have a unified solution. But if we
>> can protect snd_soc_find_dai() itself, things would be simpler may be
>> in long term. Right now there are separate source files for soc-core,
>> soc-dai and soc-component, but because of two approaches looks like
>> the function need to be moved around and need to be placed in
>> soc-core. Also the issue might go unnoticed if LOCKDEP is not enabled.
>>
>> May be start with a wrapper for now and eventually unify?
> Yeah, it seems has _with_mutex() can be better idea.
> I'm posting patch, but I noticed that Mark's branch vs Linus branch
> have some mismatch (?), and now I'm asking it to him.
> I can post _with_mutex() version as v2 if I could get answer.
> After that I'm happy your next patch can re-use it.
>

Sure. BTW, there are more such candidates which require 'lock' version 
of these helpers.
For example: soc_find_component(), snd_soc_add/remove_pcm_runtime() and 
snd_soc_register_dai().

Thank you for the feedback.
Kuninori Morimoto Aug. 26, 2020, 11:12 p.m. UTC | #12
Hi Sameer

> Sure. BTW, there are more such candidates which require 'lock' version
> of these helpers.
> For example: soc_find_component(), snd_soc_add/remove_pcm_runtime()
> and snd_soc_register_dai().

soc_find_component() is static function, no need to care about mutex.
other functions are indeed exported function, but is used from
topology.c which is calling it under mutex.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Sameer Pujar Aug. 29, 2020, 10:52 a.m. UTC | #13
Hi Morimoto-san,

Sorry for the delayed reply as I was on sick leave.
>> Sure. BTW, there are more such candidates which require 'lock' version
>> of these helpers.
>> For example: soc_find_component(), snd_soc_add/remove_pcm_runtime()
>> and snd_soc_register_dai().
> soc_find_component() is static function, no need to care about mutex.
> other functions are indeed exported function, but is used from
> topology.c which is calling it under mutex.
>
>
I was just thinking if we need to future proof these functions. As you 
mentioned, currently these have limited usage (in topology.c) and should 
just be fine to address snd_soc_find_dai() for now.

Thanks,
Sameer.

Patch
diff mbox series

diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c
index 1e20562..93bddf6 100644
--- a/sound/soc/generic/audio-graph-card.c
+++ b/sound/soc/generic/audio-graph-card.c
@@ -111,6 +111,17 @@  static int graph_get_dai_id(struct device_node *ep)
 	return id;
 }
 
+static bool soc_component_is_pcm(struct snd_soc_dai_link_component *dlc)
+{
+	struct snd_soc_dai *dai = snd_soc_find_dai(dlc);
+
+	if (dai && (dai->component->driver->pcm_construct ||
+		    dai->driver->pcm_new))
+		return true;
+
+	return false;
+}
+
 static int asoc_simple_parse_dai(struct device_node *ep,
 				 struct snd_soc_dai_link_component *dlc,
 				 int *is_single_link)
@@ -259,6 +270,16 @@  static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv,
 		if (ret < 0)
 			goto out_put_node;
 
+		/*
+		 * In BE<->BE connections it is not required to create
+		 * PCM devices at CPU end of the dai link and thus 'no_pcm'
+		 * flag needs to be set. It is useful when there are many
+		 * BE components and some of these have to be connected to
+		 * form a valid audio path.
+		 */
+		if (!soc_component_is_pcm(cpus))
+			dai_link->no_pcm = 1;
+
 		/* card->num_links includes Codec */
 		asoc_simple_canonicalize_cpu(dai_link, is_single_links);
 	} else {