[alsa-devel] [PATCH 1/4] ASoC: Add verbose debugging showing why widgets get marked dirty
Help diagnose why we're checking widgets by providing some logging when we first dirty them. This should possibly be a trace point if it's useful but can be absurdly verbose if enabled, we can always change it later if desired.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/soc-dapm.c | 26 +++++++++++++++----------- 1 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 214a709..e6a0882 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -124,10 +124,13 @@ static bool dapm_dirty_widget(struct snd_soc_dapm_widget *w) return !list_empty(&w->dirty); }
-static void dapm_mark_dirty(struct snd_soc_dapm_widget *w) +static void dapm_mark_dirty(struct snd_soc_dapm_widget *w, const char *reason) { - if (!dapm_dirty_widget(w)) + if (!dapm_dirty_widget(w)) { + dev_vdbg(w->dapm->dev, "Marking %s dirty due to %s\n", + w->name, reason); list_add_tail(&w->dirty, &w->dapm->card->dapm_dirty); + } }
/* create a new dapm widget */ @@ -1227,7 +1230,7 @@ static void dapm_widget_set_peer_power(struct snd_soc_dapm_widget *peer, /* If the peer is already in the state we're moving to then we * won't have an impact on it. */ if (power != peer->power) - dapm_mark_dirty(peer); + dapm_mark_dirty(peer, "peer state change"); }
static void dapm_widget_set_power(struct snd_soc_dapm_widget *w, bool power, @@ -1624,16 +1627,17 @@ static int dapm_mux_update_power(struct snd_soc_dapm_widget *widget, /* we now need to match the string in the enum to the path */ if (!(strcmp(path->name, e->texts[mux]))) { path->connect = 1; /* new connection */ - dapm_mark_dirty(path->source); + dapm_mark_dirty(path->source, "mux connection"); } else { if (path->connect) - dapm_mark_dirty(path->source); + dapm_mark_dirty(path->source, + "mux disconnection"); path->connect = 0; /* old connection must be powered down */ } }
if (found) { - dapm_mark_dirty(widget); + dapm_mark_dirty(widget, "mux change"); dapm_power_widgets(widget->dapm, SND_SOC_DAPM_STREAM_NOP); }
@@ -1660,11 +1664,11 @@ static int dapm_mixer_update_power(struct snd_soc_dapm_widget *widget, /* found, now check type */ found = 1; path->connect = connect; - dapm_mark_dirty(path->source); + dapm_mark_dirty(path->source, "mixer connection"); }
if (found) { - dapm_mark_dirty(widget); + dapm_mark_dirty(widget, "mixer update"); dapm_power_widgets(widget->dapm, SND_SOC_DAPM_STREAM_NOP); }
@@ -1810,7 +1814,7 @@ static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm, w->connected = status; if (status == 0) w->force = 0; - dapm_mark_dirty(w); + dapm_mark_dirty(w, "pin configuration");
return 0; } @@ -2699,7 +2703,7 @@ static void soc_dapm_stream_event(struct snd_soc_dapm_context *dapm, dev_vdbg(w->dapm->dev, "widget %s\n %s stream %s event %d\n", w->name, w->sname, stream, event); if (strstr(w->sname, stream)) { - dapm_mark_dirty(w); + dapm_mark_dirty(w, "stream event"); switch(event) { case SND_SOC_DAPM_STREAM_START: w->active = 1; @@ -2789,7 +2793,7 @@ int snd_soc_dapm_force_enable_pin(struct snd_soc_dapm_context *dapm, dev_dbg(w->dapm->dev, "dapm: force enable pin %s\n", pin); w->connected = 1; w->force = 1; - dapm_mark_dirty(w); + dapm_mark_dirty(w, "force enable");
return 0; }
Some widgets will get power_check() run on them more than once during a DAPM run, most commonly due to supply widgets checking to see if their consumers are powered up. It's wasteful to do this so cache the result of power_check() during a run. For one system I tested this on I got an improvement of:
Power Path Neighbour Before: 106 970 1186 After: 69 727 905
from this.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- include/sound/soc-dapm.h | 2 ++ sound/soc/soc-dapm.c | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index c080635..e2853da 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -473,6 +473,8 @@ struct snd_soc_dapm_widget { unsigned char ext:1; /* has external widgets */ unsigned char force:1; /* force state */ unsigned char ignore_suspend:1; /* kept enabled over suspend */ + unsigned char new_power:1; /* power from this run */ + unsigned char power_checked:1; /* power checked this run */ int subseq; /* sort within widget type */
int (*power_check)(struct snd_soc_dapm_widget *w); diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index e6a0882..c39146d 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -787,10 +787,17 @@ EXPORT_SYMBOL_GPL(dapm_reg_event);
static int dapm_widget_power_check(struct snd_soc_dapm_widget *w) { + if (w->power_checked) + return w->new_power; + if (w->force) - return 1; + w->new_power = 1; else - return w->power_check(w); + w->new_power = w->power_check(w); + + w->power_checked = true; + + return w->new_power; }
/* Generic check to see if a widget should be powered. @@ -1322,6 +1329,10 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event)
memset(&card->dapm_stats, 0, sizeof(card->dapm_stats));
+ list_for_each_entry(w, &card->widgets, list) { + w->power_checked = false; + } + /* Check which widgets we need to power and store them in * lists indicating if they should be powered up or down. We * only check widgets that have been flagged as dirty but note
The whole point of supply widgets is that they aren't inputs to their sinks so a state change in a supply should never affect the state of the widget being supplied and we don't need to mark them as dirty.
Power Path Neighbour Before: 69 727 905 After: 63 607 731
This is particularly useful where supplies affect large portions of the chip (eg, a bandgap supplying the analogue sections).
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/soc-dapm.c | 15 +++++++++++---- 1 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index c39146d..cbca1dd 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -1260,11 +1260,18 @@ static void dapm_widget_set_power(struct snd_soc_dapm_widget *w, bool power, path->connect); } } - list_for_each_entry(path, &w->sinks, list_source) { - if (path->sink) { - dapm_widget_set_peer_power(path->sink, power, - path->connect); + switch (w->id) { + case snd_soc_dapm_supply: + /* Supplies can't affect their outputs, only their inputs */ + break; + default: + list_for_each_entry(path, &w->sinks, list_source) { + if (path->sink) { + dapm_widget_set_peer_power(path->sink, power, + path->connect); + } } + break; }
if (power)
We don't really care how many widgets a supply is supplying, we just care if the number is non-zero. This didn't actually produce any improvement in the test cases I've been using but seems obviously sensible enough that I'm pushing it out anyway.
We could do a similar thing for other widgets but this may be unhelpful for further refactorings Liam was working on aiming to allow us to identify connected audio paths.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/soc-dapm.c | 9 +++------ 1 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index cbca1dd..82d93bf 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -851,7 +851,6 @@ static int dapm_dac_check_power(struct snd_soc_dapm_widget *w) static int dapm_supply_check_power(struct snd_soc_dapm_widget *w) { struct snd_soc_dapm_path *path; - int power = 0;
DAPM_UPDATE_STAT(w, power_checks);
@@ -869,15 +868,13 @@ static int dapm_supply_check_power(struct snd_soc_dapm_widget *w) if (!path->sink) continue;
- if (dapm_widget_power_check(path->sink)) { - power = 1; - break; - } + if (dapm_widget_power_check(path->sink)) + return 1; }
dapm_clear_walk(w->dapm);
- return power; + return 0; }
static int dapm_always_on_check_power(struct snd_soc_dapm_widget *w)
participants (1)
-
Mark Brown