[alsa-devel] [PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared
Commit af46800 ("ASoC: Implement mux control sharing") introduced function dapm_is_shared_kcontrol.
When this function returns true, the naming of DAPM controls is derived from the kcontrol_new. Otherwise, the name comes from the widget (and possibly a widget's naming prefix).
A bug in the implementation of dapm_is_shared_kcontrol made it return 1 in all cases. Hence, that commit caused a change in control naming for all controls instead of just shared controls.
Specifically, a control is always considered shared because it is always compared against itself. Solve this by never comparing against the widget containing the control being created.
I tested that with the Tegra WM8903 driver: * Shared is now mostly 0 as expected, and sometimes 1. * The expected controls are still generated after this change.
Howwever, I don't have any systems that have a widget/control naming prefix, so I can't test that aspect.
Reported-by: Liam Girdwood lrg@ti.com Root-caused-by: Jarkko Nikula jhnikula@gmail.com Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/soc-dapm.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 456617e..5397699 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -325,6 +325,7 @@ static int dapm_connect_mixer(struct snd_soc_dapm_context *dapm, }
static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm, + struct snd_soc_dapm_widget *kcontrolw, const struct snd_kcontrol_new *kcontrol_new, struct snd_kcontrol **kcontrol) { @@ -334,6 +335,8 @@ static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm, *kcontrol = NULL;
list_for_each_entry(w, &dapm->card->widgets, list) { + if (w == kcontrolw) + continue; for (i = 0; i < w->num_kcontrols; i++) { if (&w->kcontrol_news[i] == kcontrol_new) { if (w->kcontrols) @@ -468,7 +471,7 @@ static int dapm_new_mux(struct snd_soc_dapm_context *dapm, return -EINVAL; }
- shared = dapm_is_shared_kcontrol(dapm, &w->kcontrol_news[0], + shared = dapm_is_shared_kcontrol(dapm, w, &w->kcontrol_news[0], &kcontrol); if (kcontrol) { wlist = kcontrol->private_data;
On Tue, 24 May 2011 17:12:01 -0600 Stephen Warren swarren@nvidia.com wrote:
Commit af46800 ("ASoC: Implement mux control sharing") introduced function dapm_is_shared_kcontrol.
When this function returns true, the naming of DAPM controls is derived from the kcontrol_new. Otherwise, the name comes from the widget (and possibly a widget's naming prefix).
A bug in the implementation of dapm_is_shared_kcontrol made it return 1 in all cases. Hence, that commit caused a change in control naming for all controls instead of just shared controls.
Specifically, a control is always considered shared because it is always compared against itself. Solve this by never comparing against the widget containing the control being created.
I tested that with the Tegra WM8903 driver:
- Shared is now mostly 0 as expected, and sometimes 1.
- The expected controls are still generated after this change.
Howwever, I don't have any systems that have a widget/control naming prefix, so I can't test that aspect.
Reported-by: Liam Girdwood lrg@ti.com Root-caused-by: Jarkko Nikula jhnikula@gmail.com Signed-off-by: Stephen Warren swarren@nvidia.com
sound/soc/soc-dapm.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 456617e..5397699 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -325,6 +325,7 @@ static int dapm_connect_mixer(struct snd_soc_dapm_context *dapm, }
static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm,
- struct snd_soc_dapm_widget *kcontrolw, const struct snd_kcontrol_new *kcontrol_new, struct snd_kcontrol **kcontrol)
{ @@ -334,6 +335,8 @@ static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm, *kcontrol = NULL;
list_for_each_entry(w, &dapm->card->widgets, list) {
if (w == kcontrolw)continue;
Hmm.. it seems this test is not enough with tlv320aic3x.c. It still classifies a lot of kcontrols to be shared. Will keep hunting.
Hi Stephen
On Wed, 25 May 2011 10:01:55 +0300 Jarkko Nikula jhnikula@gmail.com wrote:
On Tue, 24 May 2011 17:12:01 -0600 Stephen Warren swarren@nvidia.com wrote:
Commit af46800 ("ASoC: Implement mux control sharing") introduced function dapm_is_shared_kcontrol.
When this function returns true, the naming of DAPM controls is derived from the kcontrol_new. Otherwise, the name comes from the widget (and possibly a widget's naming prefix).
A bug in the implementation of dapm_is_shared_kcontrol made it return 1 in all cases. Hence, that commit caused a change in control naming for all controls instead of just shared controls.
Specifically, a control is always considered shared because it is always compared against itself. Solve this by never comparing against the widget containing the control being created.
I tested that with the Tegra WM8903 driver:
- Shared is now mostly 0 as expected, and sometimes 1.
- The expected controls are still generated after this change.
Howwever, I don't have any systems that have a widget/control naming prefix, so I can't test that aspect.
Reported-by: Liam Girdwood lrg@ti.com Root-caused-by: Jarkko Nikula jhnikula@gmail.com
I would rather see here Reported-by/Tested-by as I don't feel that I broke here something :-)
Hmm.. it seems this test is not enough with tlv320aic3x.c. It still classifies a lot of kcontrols to be shared. Will keep hunting.
I managed to find why this didn't work with tlv320aic3x.c on rx51.c.
This triggered out that "[Left | Right] Line1[L | R]" Muxes in tlv320aic3x were pointing to same mux controls which is wrong since there are separate registers for them. I prepare a fix for this hopefully tomorrow.
Then we must skip test for widgets that are in different dapm context, i.e. we don't want to share mux controls if they are in different device instance. Does it work for you if you add the dapm test below to your patch?
@@ -334,6 +335,8 @@ static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm, *kcontrol = NULL;
list_for_each_entry(w, &dapm->card->widgets, list) { + if (w == kcontrolw || w->dapm != kcontrolw->dapm) + continue;
Jarkko Nikula wrote Wednesday, May 25, 2011 8:52 AM:
On Wed, 25 May 2011 10:01:55 +0300 Jarkko Nikula jhnikula@gmail.com wrote:
On Tue, 24 May 2011 17:12:01 -0600 Stephen Warren swarren@nvidia.com wrote:
Commit af46800 ("ASoC: Implement mux control sharing") introduced function dapm_is_shared_kcontrol.
When this function returns true, the naming of DAPM controls is derived from the kcontrol_new. Otherwise, the name comes from the widget (and possibly a widget's naming prefix).
A bug in the implementation of dapm_is_shared_kcontrol made it return 1 in all cases. Hence, that commit caused a change in control naming for all controls instead of just shared controls.
Specifically, a control is always considered shared because it is always compared against itself. Solve this by never comparing against the widget containing the control being created.
I tested that with the Tegra WM8903 driver:
- Shared is now mostly 0 as expected, and sometimes 1.
- The expected controls are still generated after this change.
Howwever, I don't have any systems that have a widget/control naming prefix, so I can't test that aspect.
Reported-by: Liam Girdwood lrg@ti.com Root-caused-by: Jarkko Nikula jhnikula@gmail.com
I would rather see here Reported-by/Tested-by as I don't feel that I broke here something :-)
OK, I can change it to Tested-by, but what I meant by that (admittedly non-standard) tag was simply that you'd pointed out what the basic problem was, i.e. done the work to determine the root-cause, not that you were the root cause!
Thanks for testing.
Hmm.. it seems this test is not enough with tlv320aic3x.c. It still classifies a lot of kcontrols to be shared. Will keep hunting.
I managed to find why this didn't work with tlv320aic3x.c on rx51.c.
This triggered out that "[Left | Right] Line1[L | R]" Muxes in tlv320aic3x were pointing to same mux controls which is wrong since there are separate registers for them. I prepare a fix for this hopefully tomorrow.
Then we must skip test for widgets that are in different dapm context, i.e. we don't want to share mux controls if they are in different device instance. Does it work for you if you add the dapm test below to your patch?
I suppose that's technically correct, and I'm fine adding this to the patch. However, I'm surprised that change is needed; do we actually have a situation where two widgets in different DAPM contexts point at the same control definition? It seems like that'd be a bug; why would there be two controls pointing at the same register field, yet in different DAPM contexts?
@@ -334,6 +335,8 @@ static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm, *kcontrol = NULL;
list_for_each_entry(w, &dapm->card->widgets, list) {
if (w == kcontrolw || w->dapm != kcontrolw->dapm)continue;
On Wed, 25 May 2011 08:24:59 -0700 Stephen Warren swarren@nvidia.com wrote:
Then we must skip test for widgets that are in different dapm context, i.e. we don't want to share mux controls if they are in different device instance. Does it work for you if you add the dapm test below to your patch?
I suppose that's technically correct, and I'm fine adding this to the patch. However, I'm surprised that change is needed; do we actually have a situation where two widgets in different DAPM contexts point at the same control definition? It seems like that'd be a bug; why would there be two controls pointing at the same register field, yet in different DAPM contexts?
Yes, I noticed this can happen when you have multiple same codecs in a system. Let's assume we have multiple wm8903 codecs and we have these in the driver:
static const struct snd_kcontrol_new adcinput_mux = SOC_DAPM_ENUM("ADC Input", adcinput_enum);
static const struct snd_soc_dapm_widget wm8903_dapm_widgets[] = { SND_SOC_DAPM_MUX("Left ADC Input", SND_SOC_NOPM, 0, 0, &adcinput_mux), };
Even the core duplicates widgets for each codec instance in this example, they still point to same adcinput_mux and thus dapm_is_shared_kcontrol() miss detects them as shared if we don't skip different dapm context.
Jarkko Nikula wrote at Thursday, May 26, 2011 12:58 AM:
On Wed, 25 May 2011 08:24:59 -0700 Stephen Warren swarren@nvidia.com wrote:
Then we must skip test for widgets that are in different dapm context, i.e. we don't want to share mux controls if they are in different device instance. Does it work for you if you add the dapm test below to your patch?
I suppose that's technically correct, and I'm fine adding this to the patch. However, I'm surprised that change is needed; do we actually have a situation where two widgets in different DAPM contexts point at the same control definition? It seems like that'd be a bug; why would there be two controls pointing at the same register field, yet in different DAPM contexts?
Yes, I noticed this can happen when you have multiple same codecs in a system. Let's assume we have multiple wm8903 codecs and we have these in the driver:
Yes, of course, that makes perfect sense. I'll rev the patch as you suggested.
static const struct snd_kcontrol_new adcinput_mux = SOC_DAPM_ENUM("ADC Input", adcinput_enum);
static const struct snd_soc_dapm_widget wm8903_dapm_widgets[] = { SND_SOC_DAPM_MUX("Left ADC Input", SND_SOC_NOPM, 0, 0, &adcinput_mux), };
Even the core duplicates widgets for each codec instance in this example, they still point to same adcinput_mux and thus dapm_is_shared_kcontrol() miss detects them as shared if we don't skip different dapm context.
participants (2)
-
Jarkko Nikula -
Stephen Warren