Re: [alsa-devel] No sound since 5.4 on skl_n88l25_s4567
Hi ojab
Please take a look, I want my sound back ._.
Forgot to mention, it's reproducible on linux master git.
Hmm... This patch 1) removes unneeded if check 2) adds missing error check.
1) shouldn't have effect to your PC. Thus, maybe your PC failed by 2). It had been worked by accident before ? I guess your kernel is telling some message like this ?
ASoC: no dapm match for %s --> %s --> %s
Thank you for your help !! Best regards --- Kuninori Morimoto
Please take a look, I want my sound back ._.
Forgot to mention, it's reproducible on linux master git.
Hmm... This patch
removes unneeded if check
adds missing error check.
shouldn't have effect to your PC.
Thus, maybe your PC failed by 2). It had been worked by accident before ? I guess your kernel is telling some message like this ?
ASoC: no dapm match for %s --> %s --> %s
It's likely that the topology file does not contain all the widgets/dais required by the hard-coded routes the machine driver, and by becoming stricter warnings became errors which in turn prevent the card from being created. It may be that we have to define a backwards compatible behavior that's enabled only for Intel legacy cases.
Please post the results of alsa-info.sh somewhere so that we can take a look.
On Wed, Jan 15, 2020 at 4:41 AM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
Please post the results of alsa-info.sh somewhere so that we can take a look.
5.3.0: http://alsa-project.org/db/?f=d82b3ef237a5050dfb73231ba114e45a6a4420ef 5.4.0: http://alsa-project.org/db/?f=d922b77306a1287eae3624d241f3d46d347c8098
//wbr ojab
On 1/15/20 5:57 AM, ojab // wrote:
On Wed, Jan 15, 2020 at 4:41 AM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
Please post the results of alsa-info.sh somewhere so that we can take a look.
Thanks, I think that confirms my theory.
5.3.0: http://alsa-project.org/db/?f=d82b3ef237a5050dfb73231ba114e45a6a4420ef
In this case, there's a warning on the routes:
[ 22.364318] skl_n88l25_s4567 skl_n88l25_s4567: ASoC: Failed to add route iDisp3_out -> direct -> iDisp3 Tx
5.4.0: http://alsa-project.org/db/?f=d922b77306a1287eae3624d241f3d46d347c8098
And we know that in 5.4 errors on routes caused the card registration to stop. I *think* the patch was daa480bde6b3a9 ("ASoC: soc-core: tidyup for snd_soc_dapm_add_routes()")
This patch won't revert cleanly, can you try the following hack on v5.4 to see if this improves the card registration? Thanks!
On Wed, Jan 15, 2020 at 6:07 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
This patch won't revert cleanly, can you try the following hack on v5.4 to see if this improves the card registration? Thanks!
Yep, it works with the patch.
//wbr ojab
This patch won't revert cleanly, can you try the following hack on v5.4 to see if this improves the card registration? Thanks!
Yep, it works with the patch.
ok, thanks for testing! So that leaves us with two options:
a) remove the error handling after soc_dapm_add_routes() to be backwards compatible with Intel problematic machine drivers. Not really nice for everyone else.
b) remove this error conditionally so that legacy Intel solutions still work but new ones deal with errors upfront.
I am not sure if there's a 'clean' way to implement b), maybe with a Kconfig selected by some machine drivers? Morimito-san, any suggestions now that we've root caused the problem (copied again below for reference)?
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 88978a3036c4..796d14feab4d 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1076,8 +1076,8 @@ static int soc_probe_component(struct snd_soc_card *card, ret = snd_soc_dapm_add_routes(dapm, component->driver->dapm_routes, component->driver->num_dapm_routes); - if (ret < 0) - goto err_probe; + //if (ret < 0) + // goto err_probe;
/* see for_each_card_components */ list_add(&component->card_list, &card->component_dev_list); @@ -2065,13 +2065,13 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
ret = snd_soc_dapm_add_routes(&card->dapm, card->dapm_routes, card->num_dapm_routes); - if (ret < 0) - goto probe_end; + //if (ret < 0) + // goto probe_end;
ret = snd_soc_dapm_add_routes(&card->dapm, card->of_dapm_routes, - card->num_of_dapm_routes); - if (ret < 0) - goto probe_end; + card->num_of_dapm_routes); + //if (ret < 0) + // goto probe_end;
/* try to set some sane longname if DMI is available */ snd_soc_set_dmi_name(card, NULL);
Hi Pierre-Louis Cc Mark
a) remove the error handling after soc_dapm_add_routes() to be backwards compatible with Intel problematic machine drivers. Not really nice for everyone else.
b) remove this error conditionally so that legacy Intel solutions still work but new ones deal with errors upfront.
I am not sure if there's a 'clean' way to implement b), maybe with a Kconfig selected by some machine drivers? Morimito-san, any suggestions now that we've root caused the problem (copied again below for reference)?
Is it possible to *real* fixup in the future, some day ? We need correct code, but backward compatibility is also needed. Not only Intel but some machine might have similar issue, so, how about to indicate WARNING, and comment ?
Like this
ret = snd_soc_dapm_add_routes(dapm, component->driver->dapm_routes, component->driver->num_dapm_routes); if (ret < 0) - goto err_probe; + /* + * This is error, but legacy Intel want + * compatibility until xxxx. + * call "goto err_probe" after that + */ + dev_warn(dev, "DAPM route add failed.")
Thank you for your help !! Best regards --- Kuninori Morimoto
a) remove the error handling after soc_dapm_add_routes() to be backwards compatible with Intel problematic machine drivers. Not really nice for everyone else.
b) remove this error conditionally so that legacy Intel solutions still work but new ones deal with errors upfront.
I am not sure if there's a 'clean' way to implement b), maybe with a Kconfig selected by some machine drivers? Morimito-san, any suggestions now that we've root caused the problem (copied again below for reference)?
Is it possible to *real* fixup in the future, some day ?
Quite unlikely I am afraid. This is a mismatch between topology and machine driver, and there are no planned updates of those topologies (which were never released for upstream use).
In this case I guess the topology file was taken from a ChromeOS distribution (based on v4.4 IIRC), so there's really no guarantee that a fix would ever reach the user without a convoluted set of manual updates.
We need correct code, but backward compatibility is also needed. Not only Intel but some machine might have similar issue, so, how about to indicate WARNING, and comment ?
Like this
ret = snd_soc_dapm_add_routes(dapm, component->driver->dapm_routes, component->driver->num_dapm_routes); if (ret < 0)
goto err_probe;
/** This is error, but legacy Intel want* compatibility until xxxx.* call "goto err_probe" after that*/dev_warn(dev, "DAPM route add failed.")
we already had a warning before:
[ 22.364318] skl_n88l25_s4567 skl_n88l25_s4567: ASoC: Failed to add route iDisp3_out -> direct -> iDisp3 Tx
and it was ignored since there was no functional impact....
Maybe we could have some sort of boolean flag in the component->driver definition and explicitly request a backwards-compatible behavior (e.g. for all SKL/KBL machine drivers) when that driver is known to be flaky. There's already things like 'fully_routed', maybe we can add something such as 'disable_route_check'?
Hi Pierre-Louis, Mark
Is it possible to *real* fixup in the future, some day ?
Quite unlikely I am afraid. This is a mismatch between topology and machine driver, and there are no planned updates of those topologies (which were never released for upstream use).
In this case I guess the topology file was taken from a ChromeOS distribution (based on v4.4 IIRC), so there's really no guarantee that a fix would ever reach the user without a convoluted set of manual updates.
(snip)
Maybe we could have some sort of boolean flag in the component->driver definition and explicitly request a backwards-compatible behavior (e.g. for all SKL/KBL machine drivers) when that driver is known to be flaky. There's already things like 'fully_routed', maybe we can add something such as 'disable_route_check'?
Hmm... I have no idea, but have no objection about this.
Thank you for your help !! Best regards --- Kuninori Morimoto
On Wed, Jan 15, 2020 at 07:04:48PM -0600, Pierre-Louis Bossart wrote:
Maybe we could have some sort of boolean flag in the component->driver definition and explicitly request a backwards-compatible behavior (e.g. for all SKL/KBL machine drivers) when that driver is known to be flaky. There's already things like 'fully_routed', maybe we can add something such as 'disable_route_check'?
A quirk for old stuff that can't be fixed sounds like a sensible solution to this.
On Thu, Jan 16, 2020 at 5:57 PM Mark Brown broonie@kernel.org wrote:
On Wed, Jan 15, 2020 at 07:04:48PM -0600, Pierre-Louis Bossart wrote:
Maybe we could have some sort of boolean flag in the component->driver definition and explicitly request a backwards-compatible behavior (e.g. for all SKL/KBL machine drivers) when that driver is known to be flaky. There's already things like 'fully_routed', maybe we can add something such as 'disable_route_check'?
A quirk for old stuff that can't be fixed sounds like a sensible solution to this.
Any update on this?
//wbr ojab
On 2/17/20 9:10 AM, ojab // wrote:
On Thu, Jan 16, 2020 at 5:57 PM Mark Brown broonie@kernel.org wrote:
On Wed, Jan 15, 2020 at 07:04:48PM -0600, Pierre-Louis Bossart wrote:
Maybe we could have some sort of boolean flag in the component->driver definition and explicitly request a backwards-compatible behavior (e.g. for all SKL/KBL machine drivers) when that driver is known to be flaky. There's already things like 'fully_routed', maybe we can add something such as 'disable_route_check'?
A quirk for old stuff that can't be fixed sounds like a sensible solution to this.
Any update on this?
Sorry, this is what I had in mind (not even compile-tested). I don't know if the checks need to be disabled twice.
diff --git a/include/sound/soc.h b/include/sound/soc.h index f0e4f36f83bf..7a4643d87e39 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1058,6 +1058,7 @@ struct snd_soc_card { const struct snd_soc_dapm_route *of_dapm_routes; int num_of_dapm_routes; bool fully_routed; + bool disable_route_checks;
/* lists of probed devices belonging to this card */ struct list_head component_dev_list; diff --git a/sound/soc/intel/boards/skl_nau88l25_max98357a.c b/sound/soc/intel/boards/skl_nau88l25_max98357a.c index e6de3b28d840..b57f55731390 100644 --- a/sound/soc/intel/boards/skl_nau88l25_max98357a.c +++ b/sound/soc/intel/boards/skl_nau88l25_max98357a.c @@ -644,6 +644,7 @@ static struct snd_soc_card skylake_audio_card = { .num_dapm_routes = ARRAY_SIZE(skylake_map), .fully_routed = true, .late_probe = skylake_card_late_probe, + .disable_route_checks = true, };
static int skylake_audio_probe(struct platform_device *pdev) diff --git a/sound/soc/intel/boards/skl_nau88l25_ssm4567.c b/sound/soc/intel/boards/skl_nau88l25_ssm4567.c index c99c8b23e509..8f3b724fec27 100644 --- a/sound/soc/intel/boards/skl_nau88l25_ssm4567.c +++ b/sound/soc/intel/boards/skl_nau88l25_ssm4567.c @@ -687,6 +687,7 @@ static struct snd_soc_card skylake_audio_card = { .num_configs = ARRAY_SIZE(ssm4567_codec_conf), .fully_routed = true, .late_probe = skylake_card_late_probe, + .disable_route_checks = true, };
static int skylake_audio_probe(struct platform_device *pdev) diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 068d809c349a..d56cb655d89c 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1257,7 +1257,10 @@ static int soc_probe_component(struct snd_soc_card *card, component->driver->dapm_routes, component->driver->num_dapm_routes); if (ret < 0) - goto err_probe; + if (card->disable_route_checks) + dev_info(component->dev, "Ignoring errors on snd_soc_dapm_add_routes\n"); + else + goto err_probe;
/* see for_each_card_components */ list_add(&component->card_list, &card->component_dev_list); @@ -1939,7 +1942,10 @@ static int snd_soc_bind_card(struct snd_soc_card *card) ret = snd_soc_dapm_add_routes(&card->dapm, card->dapm_routes, card->num_dapm_routes); if (ret < 0) - goto probe_end; + if (card->disable_route_checks) + dev_info(component->dev, "Ignoring errors on snd_soc_dapm_add_routes\n"); + else + goto probe_end;
ret = snd_soc_dapm_add_routes(&card->dapm, card->of_dapm_routes, card->num_of_dapm_routes);
On Tue, Feb 18, 2020 at 8:24 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
On 2/17/20 9:10 AM, ojab // wrote:
On Thu, Jan 16, 2020 at 5:57 PM Mark Brown broonie@kernel.org wrote:
On Wed, Jan 15, 2020 at 07:04:48PM -0600, Pierre-Louis Bossart wrote:
Maybe we could have some sort of boolean flag in the component->driver definition and explicitly request a backwards-compatible behavior (e.g. for all SKL/KBL machine drivers) when that driver is known to be flaky. There's already things like 'fully_routed', maybe we can add something such as 'disable_route_check'?
A quirk for old stuff that can't be fixed sounds like a sensible solution to this.
Any update on this?
Sorry, this is what I had in mind (not even compile-tested). I don't know if the checks need to be disabled twice.
Unfortunately no sound with this patch on top of 5.6-rc2 http://alsa-project.org/db/?f=0f5049d8f7dc906bd13f354056282a48d2e626cc
//wbr ojab
On Wed, Jan 15, 2020 at 3:23 AM Kuninori Morimoto kuninori.morimoto.gx@renesas.com wrote:
I guess your kernel is telling some message like this ?
ASoC: no dapm match for %s --> %s --> %s
No such entries on both 5.3 & 5.4 kernels, but I hope needed dmesg excerpts can be found in alsa-info.sh output.
//wbr ojab
participants (4)
-
Kuninori Morimoto -
Mark Brown -
ojab // -
Pierre-Louis Bossart