| Message ID | 20181105111912.14464-1-tzungbi@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: dapm: Use less aggressive caching to ensure correctness | expand |
On Mon, Nov 05, 2018 at 07:19:12PM +0800, Tzung-Bi Shih wrote: > +++ b/sound/soc/soc-dapm.c > @@ -2722,7 +2722,7 @@ static int snd_soc_dapm_add_path(struct snd_soc_dapm_context *dapm, > dapm_mark_dirty(widgets[dir], "Route added"); > } > > - if (dapm->card->instantiated && path->connect) > + if (path->connect) > dapm_path_invalidate(path); > > return 0; The whole point with the instantiated check here is that when we instantiate the card we're supposed to go through and redo all the DAPM configuration for the card in one fell swoop rather than having to constantly redo things while we're building up the graph. If that invalidation isn't happening then we should fix that.
The original assumption at the bottom of snd_soc_instantiate_card() would be: all widget->endpoints are -1, and all widgets are in the list card->dapm_dirty. So that before returning from snd_soc_instantiate_card(), the snd_soc_dapm_sync() can calculate widget->endpoints and check power states for the whole graph of the card. However, if some widgets' endpoints have been polluted and get removed from the dirty list in the middle of snd_soc_instantiate_card(), they would have no chance to be recalculated at the bottom of snd_soc_instantiate_card(). Two ideas so far: The first one is protecting the endpoints cache and the dirty list during card instantiating by checking card->instantiated in is_connected_output_ep(), is_connected_input_ep(), and dapm_power_widgets(). Note that is_connected_output_ep() and is_connected_input_ep() will be invoked if someone read from debugfs. We could return -1 to indicate the endpoints cache is disallowed to access in the case. The second one is forcing to recalculate the graph at the bottom of snd_soc_instantiate_card() by additionally calling dapm_mark_endpoints_dirty(). It would waste some computing power by comparing with the first idea but it would be also simpler than the first one. Which one makes the most sense for you? Or any other ideas? On Wed, Nov 7, 2018 at 1:18 AM Mark Brown <broonie@kernel.org> wrote: > > On Mon, Nov 05, 2018 at 07:19:12PM +0800, Tzung-Bi Shih wrote: > > > +++ b/sound/soc/soc-dapm.c > > @@ -2722,7 +2722,7 @@ static int snd_soc_dapm_add_path(struct snd_soc_dapm_context *dapm, > > dapm_mark_dirty(widgets[dir], "Route added"); > > } > > > > - if (dapm->card->instantiated && path->connect) > > + if (path->connect) > > dapm_path_invalidate(path); > > > > return 0; > > The whole point with the instantiated check here is that when we > instantiate the card we're supposed to go through and redo all the DAPM > configuration for the card in one fell swoop rather than having to > constantly redo things while we're building up the graph. If that > invalidation isn't happening then we should fix that.
On Wed, Nov 07, 2018 at 12:03:06PM +0800, Tzung-Bi Shih wrote: Please don't top post, reply in line with needed context. This allows readers to readily follow the flow of conversation and understand what you are talking about and also helps ensure that everything in the discussion is being addressed. > The second one is forcing to recalculate the graph at the bottom of > snd_soc_instantiate_card() by additionally calling > dapm_mark_endpoints_dirty(). It would waste some computing power by > comparing with the first idea but it would be also simpler than the > first one. As we just discussed in person this is probably the best option - it's what we used to have (or were supposed to have, but I'm fairly sure I remember writing the code), I'm guessing it's gone AWOL during all the refactorings for componentization.
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index a5178845065b..2914f3adb333 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -2722,7 +2722,7 @@ static int snd_soc_dapm_add_path(struct snd_soc_dapm_context *dapm, dapm_mark_dirty(widgets[dir], "Route added"); } - if (dapm->card->instantiated && path->connect) + if (path->connect) dapm_path_invalidate(path); return 0;
Commit 92a99ea439c4 ("ASoC: dapm: Use more aggressive caching") only invalidates necessary caches to improve the cache hit rate. However, in snd_soc_dapm_add_path(), the cache is too aggressive which results in wrong state of the audio map. E.g. imaging the following examples during snd_soc_instantiate_card(): time 1: at the beginning in:-1 in:-1 in:-1 out:-1 out:-1 out:-1 A -----> B Spk time 2: after someone called snd_soc_dapm_new_widgets() (e.g. create_fill_widget_route_map() in sound/soc/codecs/hdac_hdmi.c) in:0 in:0 in:0 out:0 out:0 out:1 A ----> B Spk time 3: route added to Spk in:0 in:0 in:0 out:0 out:0 out:1 A ----> B ----> Spk time N: route added from SIGGEN in:1 in:0 in:0 in:0 out:0 out:0 out:0 out:1 SIGGEN ----> A ----> B ----> Spk In the end, the path will not power on but it should be. At time 2, the caches have been polluted. At time 3, it will not get the correct state if snd_soc_dapm_add_path() does not invalidate "out" of B and A. time 3 (expected result): in:0 in:0 in:0 out:1 out:1 out:1 A ----> B ----> Spk Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> --- sound/soc/soc-dapm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)