[PATCH 1/2] ASoC: Intel: Skylake: fix error message of NHLT blob selection

[PATCH 1/2] ASoC: Intel: Skylake: fix error message of NHLT blob selection

[Icenowy Zheng]

The query to select NHLT blob had been changed to use parameters of the
pipeline instead of PCM, however the error message returned when it fails
still uses PCM properties.

Fix this inconsistency.

Fixes: 87b265260046 ("ASoC: Intel: Skylake: Select proper format for NHLT blob")
Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 sound/soc/intel/skylake/skl-topology.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index 9bdf020a2b64..19994ec8bba1 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -1864,7 +1864,8 @@ static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
 	} else {
 		dev_err(dai->dev, "Blob NULL for id:%d type:%d dirn:%d ch:%d, freq:%d, fmt:%d\n",
 			mconfig->vbus_id, link_type, params->stream,
-			params->ch, params->s_freq, params->s_fmt);
+			pipe_fmt->channels, pipe_fmt->freq,
+			pipe_fmt->bps);
 		return -EINVAL;
 	}
 
-- 
2.37.1

[PATCH 2/2] ASoC: Intel: Skylake: try to get NHLT blob with PCM params as fallback

[Icenowy Zheng]

Switching to use pipeline parameters to get NHLT blob breaks audio on
HP Chromebook 13 G1 (at least with MrChromeBox firmware).

Fix this by retrying to get NHLT blob with PCM parameters (which is the
old behavior) if pipeline parameters fail.

Fixes: 87b265260046 ("ASoC: Intel: Skylake: Select proper format for NHLT blob")
Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 sound/soc/intel/skylake/skl-topology.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index 19994ec8bba1..3d5a3ee1c82c 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -1858,6 +1858,15 @@ static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
 					pipe_fmt->bps, params->s_cont,
 					pipe_fmt->channels, pipe_fmt->freq,
 					pipe->direction, dev_type);
+	if (!cfg) {
+		/* Retry with PCM parameters, as the old behavior */
+		cfg = intel_nhlt_get_endpoint_blob(dai->dev, skl->nhlt,
+						mconfig->vbus_id, link_type,
+						params->s_fmt, params->s_cont,
+						params->ch, params->s_freq,
+						params->stream, dev_type);
+	}
+
 	if (cfg) {
 		mconfig->formats_config[SKL_PARAM_INIT].caps_size = cfg->size;
 		mconfig->formats_config[SKL_PARAM_INIT].caps = (u32 *)&cfg->caps;
@@ -1866,6 +1875,8 @@ static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
 			mconfig->vbus_id, link_type, params->stream,
 			pipe_fmt->channels, pipe_fmt->freq,
 			pipe_fmt->bps);
+		dev_err(dai->dev, "PCM: ch %d, freq %d, fmt %d\n",
+			params->ch, params->s_freq, params->s_fmt);
 		return -EINVAL;
 	}
 
-- 
2.37.1

Re: [PATCH 2/2] ASoC: Intel: Skylake: try to get NHLT blob with PCM params as fallback

[Cezary Rojewski]

On 2022-07-25 1:10 PM, Icenowy Zheng wrote:
> Switching to use pipeline parameters to get NHLT blob breaks audio on
> HP Chromebook 13 G1 (at least with MrChromeBox firmware).
> 
> Fix this by retrying to get NHLT blob with PCM parameters (which is the
> old behavior) if pipeline parameters fail.
> 
> Fixes: 87b265260046 ("ASoC: Intel: Skylake: Select proper format for NHLT blob")
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>

Hello,


Could you share the NHLT file from your platform plus the format used by 
the cras/userspace tool? Did you try playing over simple aplay tool instead?


>   sound/soc/intel/skylake/skl-topology.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
> index 19994ec8bba1..3d5a3ee1c82c 100644
> --- a/sound/soc/intel/skylake/skl-topology.c
> +++ b/sound/soc/intel/skylake/skl-topology.c
> @@ -1858,6 +1858,15 @@ static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
>   					pipe_fmt->bps, params->s_cont,
>   					pipe_fmt->channels, pipe_fmt->freq,
>   					pipe->direction, dev_type);
> +	if (!cfg) {
> +		/* Retry with PCM parameters, as the old behavior */

Drop the "old behavior" - most of the readers are not aware of what that 
actually means.

> +		cfg = intel_nhlt_get_endpoint_blob(dai->dev, skl->nhlt,
> +						mconfig->vbus_id, link_type,
> +						params->s_fmt, params->s_cont,
> +						params->ch, params->s_freq,
> +						params->stream, dev_type);
> +	}
> +
>   	if (cfg) {
>   		mconfig->formats_config[SKL_PARAM_INIT].caps_size = cfg->size;
>   		mconfig->formats_config[SKL_PARAM_INIT].caps = (u32 *)&cfg->caps;
> @@ -1866,6 +1875,8 @@ static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
>   			mconfig->vbus_id, link_type, params->stream,
>   			pipe_fmt->channels, pipe_fmt->freq,
>   			pipe_fmt->bps);
> +		dev_err(dai->dev, "PCM: ch %d, freq %d, fmt %d\n",
> +			params->ch, params->s_freq, params->s_fmt);
>   		return -EINVAL;
>   	}
>   

Re: [PATCH 2/2] ASoC: Intel: Skylake: try to get NHLT blob with PCM params as fallback

[Icenowy Zheng]

[-- Attachment #1: Type: text/plain, Size: 3339 bytes --]

在 2022-08-02星期二的 12:30 +0200，Cezary Rojewski写道：
> On 2022-07-25 1:10 PM, Icenowy Zheng wrote:
> > Switching to use pipeline parameters to get NHLT blob breaks audio
> > on
> > HP Chromebook 13 G1 (at least with MrChromeBox firmware).
> > 
> > Fix this by retrying to get NHLT blob with PCM parameters (which is
> > the
> > old behavior) if pipeline parameters fail.
> > 
> > Fixes: 87b265260046 ("ASoC: Intel: Skylake: Select proper format
> > for NHLT blob")
> > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> 
> Hello,
> 
> 
> Could you share the NHLT file from your platform plus the format used
> by 
> the cras/userspace tool? Did you try playing over simple aplay tool
> instead?

I tried 48000Hz 2ch 32bit with speaker-test.

Attached is /sys/firmware/acpi/tables/NHLT.

> 
> 
> >   sound/soc/intel/skylake/skl-topology.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> > 
> > diff --git a/sound/soc/intel/skylake/skl-topology.c
> > b/sound/soc/intel/skylake/skl-topology.c
> > index 19994ec8bba1..3d5a3ee1c82c 100644
> > --- a/sound/soc/intel/skylake/skl-topology.c
> > +++ b/sound/soc/intel/skylake/skl-topology.c
> > @@ -1858,6 +1858,15 @@ static int
> > skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
> >                                         pipe_fmt->bps, params-
> > >s_cont,
> >                                         pipe_fmt->channels,
> > pipe_fmt->freq,
> >                                         pipe->direction, dev_type);
> > +       if (!cfg) {
> > +               /* Retry with PCM parameters, as the old behavior
> > */
> 
> Drop the "old behavior" - most of the readers are not aware of what
> that 
> actually means.
> 
> > +               cfg = intel_nhlt_get_endpoint_blob(dai->dev, skl-
> > >nhlt,
> > +                                               mconfig->vbus_id,
> > link_type,
> > +                                               params->s_fmt,
> > params->s_cont,
> > +                                               params->ch, params-
> > >s_freq,
> > +                                               params->stream,
> > dev_type);
> > +       }
> > +
> >         if (cfg) {
> >                 mconfig->formats_config[SKL_PARAM_INIT].caps_size =
> > cfg->size;
> >                 mconfig->formats_config[SKL_PARAM_INIT].caps = (u32
> > *)&cfg->caps;
> > @@ -1866,6 +1875,8 @@ static int
> > skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
> >                         mconfig->vbus_id, link_type, params-
> > >stream,
> >                         pipe_fmt->channels, pipe_fmt->freq,
> >                         pipe_fmt->bps);
> > +               dev_err(dai->dev, "PCM: ch %d, freq %d, fmt %d\n",
> > +                       params->ch, params->s_freq, params->s_fmt);
> >                 return -EINVAL;
> >         }
> >   


[-- Attachment #2: NHLT --]
[-- Type: application/octet-stream, Size: 6736 bytes --]

Re: [PATCH 2/2] ASoC: Intel: Skylake: try to get NHLT blob with PCM params as fallback

[Cezary Rojewski]

On 2022-08-07 7:26 PM, Icenowy Zheng wrote:
> 在 2022-08-02星期二的 12:30 +0200，Cezary Rojewski写道：
>> On 2022-07-25 1:10 PM, Icenowy Zheng wrote:
>>> Switching to use pipeline parameters to get NHLT blob breaks audio
>>> on
>>> HP Chromebook 13 G1 (at least with MrChromeBox firmware).

...

>> Could you share the NHLT file from your platform plus the format used
>> by
>> the cras/userspace tool? Did you try playing over simple aplay tool
>> instead?
> 
> I tried 48000Hz 2ch 32bit with speaker-test.
> 
> Attached is /sys/firmware/acpi/tables/NHLT.


Thanks for the NHLT dump. Total of five endpoints are part of the 
description:

id 0, DMIC capture, formats:
	[0] 2/16/16/48000
	[1] 2/32/32/48000
id 1, I2S SSP0 playback, formats:
	[0] 2/24/32/48000
id 2, I2S SSP0 capture, formats:
	[0] 4/32/32/48000
id 3, I2S SSP1 playback, formats:
	[0] 2/24/32/48000
id 4, I2S SSP1 capture, formats:
	[0] 2/24/32/48000

I know not what "speaker-test" means. Could you specify which endpoint 
you are speaking of? Providing either alsa info or at least output of 
'lsmod | grep snd' would help. I'd like to be aware of which machine 
board are we talking about.

What could be guessed with the current info (and some google query), is 
that we are dealing with SKL-Y m5-6Y57, codenamed 'Chell' and the 
speaker-test is playing through an I2S codec connected to SSP0 port. 
It's probably max98357a. The skylake-driver version you have gets 
confused with 24/32 format (userspace is probably attempting 32/32). 
That or topology file is incorrect..

..and at this point I probably know more than enough. We have tested 
basically all of the KBL and AML configurations when fixing regressions 
during recent skylake-driver up-revs. But Chell (and Lars for that 
matter) families were not among them as these are based on SKL. I'll 
follow up on this with our partners and come back here. I'm almost 
certain topology files for the two families mentioned were not updated 
along the way.


Regards,
Czarek

Re: [PATCH 2/2] ASoC: Intel: Skylake: try to get NHLT blob with PCM params as fallback

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

On Tue, Aug 16, 2022 at 09:08:31PM +0200, Cezary Rojewski wrote:

> I know not what "speaker-test" means. Could you specify which endpoint you
> are speaking of? Providing either alsa info or at least output of 'lsmod |
> grep snd' would help. I'd like to be aware of which machine board are we
> talking about.

speaker-test is one of the standard alsa-utils apps, it plays generated
audio (sine waves, various varieties of noise, whatever) with command
line specified control of the audio parameters.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] ASoC: Intel: Skylake: try to get NHLT blob with PCM params as fallback

[Icenowy Zheng]

在 2022-08-16星期二的 21:08 +0200，Cezary Rojewski写道：
> On 2022-08-07 7:26 PM, Icenowy Zheng wrote:
> > 在 2022-08-02星期二的 12:30 +0200，Cezary Rojewski写道：
> > > On 2022-07-25 1:10 PM, Icenowy Zheng wrote:
> > > > Switching to use pipeline parameters to get NHLT blob breaks
> > > > audio
> > > > on
> > > > HP Chromebook 13 G1 (at least with MrChromeBox firmware).
> 
> ...
> 
> > > Could you share the NHLT file from your platform plus the format
> > > used
> > > by
> > > the cras/userspace tool? Did you try playing over simple aplay
> > > tool
> > > instead?
> > 
> > I tried 48000Hz 2ch 32bit with speaker-test.
> > 
> > Attached is /sys/firmware/acpi/tables/NHLT.
> 
> 
> Thanks for the NHLT dump. Total of five endpoints are part of the 
> description:
> 
> id 0, DMIC capture, formats:
>         [0] 2/16/16/48000
>         [1] 2/32/32/48000
> id 1, I2S SSP0 playback, formats:
>         [0] 2/24/32/48000
> id 2, I2S SSP0 capture, formats:
>         [0] 4/32/32/48000
> id 3, I2S SSP1 playback, formats:
>         [0] 2/24/32/48000
> id 4, I2S SSP1 capture, formats:
>         [0] 2/24/32/48000
> 
> I know not what "speaker-test" means. Could you specify which
> endpoint 

Well to be honest I know nearly nothing about Intel ASoC. (In fact I
have only a little experience dealing with ASoC on DT-based platforms
with asoc-simple-card)

I assume it uses the first playback stream, because I didn't explicitly
choose any streams; and speaker-test is only a playback test program.

> you are speaking of? Providing either alsa info or at least output of
> 'lsmod | grep snd' would help. I'd like to be aware of which machine 
> board are we talking about.
> 
> What could be guessed with the current info (and some google query),
> is 
> that we are dealing with SKL-Y m5-6Y57, codenamed 'Chell' and the 
> speaker-test is playing through an I2S codec connected to SSP0 port. 
> It's probably max98357a. The skylake-driver version you have gets 
> confused with 24/32 format (userspace is probably attempting 32/32). 
> That or topology file is incorrect..

Yes, it's Chell.

BTW do you need other ACPI tables?

> 
> ..and at this point I probably know more than enough. We have tested 
> basically all of the KBL and AML configurations when fixing
> regressions 
> during recent skylake-driver up-revs. But Chell (and Lars for that 
> matter) families were not among them as these are based on SKL. I'll 
> follow up on this with our partners and come back here. I'm almost 
> certain topology files for the two families mentioned were not
> updated 
> along the way.

Could this be an issue of Coreboot, which generates the NHLT table?

BTW I think Google pinned the official OS of this hardware to a much
lower kernel version (but I don't want to use the official OS because
of limited storage of Chell and lack of VM ability of the OS on Chell).

> 
> 
> Regards,
> Czarek

Re: [PATCH 2/2] ASoC: Intel: Skylake: try to get NHLT blob with PCM params as fallback

[Cezary Rojewski]

On 2022-08-17 2:48 PM, Icenowy Zheng wrote:
> 在 2022-08-16星期二的 21:08 +0200，Cezary Rojewski写道：

...

> 
> Yes, it's Chell.
> 
> BTW do you need other ACPI tables?

At this point, no. Thanks for confirming the platform's name.

>>
>> ..and at this point I probably know more than enough. We have tested
>> basically all of the KBL and AML configurations when fixing
>> regressions
>> during recent skylake-driver up-revs. But Chell (and Lars for that
>> matter) families were not among them as these are based on SKL. I'll
>> follow up on this with our partners and come back here. I'm almost
>> certain topology files for the two families mentioned were not
>> updated
>> along the way.
> 
> Could this be an issue of Coreboot, which generates the NHLT table?

NHLT was left alone across all the updates. Updating it is one way of 
fixing problems but I don't believe it's necessary in your case. 
Topology update is more desirable approach.

> BTW I think Google pinned the official OS of this hardware to a much
> lower kernel version (but I don't want to use the official OS because
> of limited storage of Chell and lack of VM ability of the OS on Chell).

That clarifies things out. Guess the kernel version used there is v4.4 
(plus a ton of un-upstreamed patches). Again, will propagate the 
information up the chain. Perhaps one of the solutions for end-users 
would be providing working UCM files to alsa-topology-conf repo so users 
are not powerless in situations such as this one.


Regards,
Czarek

Re: [PATCH 2/2] ASoC: Intel: Skylake: try to get NHLT blob with PCM params as fallback

[Icenowy Zheng]

在 2022-08-17星期三的 15:19 +0200，Cezary Rojewski写道：
> On 2022-08-17 2:48 PM, Icenowy Zheng wrote:
> > 在 2022-08-16星期二的 21:08 +0200，Cezary Rojewski写道：
> 
> ...
> 
> > 
> > Yes, it's Chell.
> > 
> > BTW do you need other ACPI tables?
> 
> At this point, no. Thanks for confirming the platform's name.
> 
> > > 
> > > ..and at this point I probably know more than enough. We have
> > > tested
> > > basically all of the KBL and AML configurations when fixing
> > > regressions
> > > during recent skylake-driver up-revs. But Chell (and Lars for
> > > that
> > > matter) families were not among them as these are based on SKL.
> > > I'll
> > > follow up on this with our partners and come back here. I'm
> > > almost
> > > certain topology files for the two families mentioned were not
> > > updated
> > > along the way.
> > 
> > Could this be an issue of Coreboot, which generates the NHLT table?
> 
> NHLT was left alone across all the updates. Updating it is one way of
> fixing problems but I don't believe it's necessary in your case. 
> Topology update is more desirable approach.

BTW how could I fix the topology?

I now use topology files from GalliumOS (which, I assume, is extracted
from ChromeOS).

> 
> > BTW I think Google pinned the official OS of this hardware to a
> > much
> > lower kernel version (but I don't want to use the official OS
> > because
> > of limited storage of Chell and lack of VM ability of the OS on
> > Chell).
> 
> That clarifies things out. Guess the kernel version used there is
> v4.4 
> (plus a ton of un-upstreamed patches). Again, will propagate the 
> information up the chain. Perhaps one of the solutions for end-users 
> would be providing working UCM files to alsa-topology-conf repo so
> users 
> are not powerless in situations such as this one.
> 
> 
> Regards,
> Czarek

Re: [PATCH 2/2] ASoC: Intel: Skylake: try to get NHLT blob with PCM params as fallback

[Cezary Rojewski]

On 2022-08-18 11:25 AM, Icenowy Zheng wrote:
> 在 2022-08-17星期三的 15:19 +0200，Cezary Rojewski写道：

...

>> NHLT was left alone across all the updates. Updating it is one way of
>> fixing problems but I don't believe it's necessary in your case.
>> Topology update is more desirable approach.
> 
> BTW how could I fix the topology?
> 
> I now use topology files from GalliumOS (which, I assume, is extracted
> from ChromeOS).

(save)

>> That clarifies things out. Guess the kernel version used there is
>> v4.4
>> (plus a ton of un-upstreamed patches). Again, will propagate the
>> information up the chain. Perhaps one of the solutions for end-users
>> would be providing working UCM files to alsa-topology-conf repo so
>> users
>> are not powerless in situations such as this one.

The message of mine above is the answer to this. Without some ADSP 
firmware knowledge it's almost impossible to write a topology file from 
scratch. During v4.4 -> v5.4 transition several problems with endpoints 
were detected as not all of them were behaving as expected. Long story 
short, I do not believe pure v4.4 on some of the designs (mainly I2S 
designs) works as intended. Some external patches are needed for that to 
happen.

The situation on v5.4 is much cleaner - the problem there are the 
topology files, as these are not updated automatically when you flash 
new kernel. In fact, these were never shared in alsa-topology-ucm repo. 
Only HDAudio topology has been fixed and shared. As long as you stick to 
the original ChromeOS the issues on your machine should cease to exist. 
I need some approvals before I2S designs can be shared. It's not a 
process that takes a day or two, unfortunately.


Regards,
Czarek