| Message ID | 1596605064-27748-4-git-send-email-spujar@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Audio graph card updates and usage with Tegra210 audio | expand |
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
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
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
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
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
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
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.
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
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.
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
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.
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
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.
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 {
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(+)