[alsa-devel] [PATCH] ASoC: SOF: intel: Fix probe error without i915 graphics

[alsa-devel] [PATCH] ASoC: SOF: intel: Fix probe error without i915 graphics

[Takashi Iwai]

When the driver is configured with CONFIG_SND_SOC_SOF_HDA, the SOF
driver tries to bind with i915 audio component.  But there is also a
system without Intel graphics.  On such a system, snd_hdac_i915_init()
returns -ENODEV error and it leads to the whole probe error of SOF
driver.

For avoiding this spurious probe error, this patch changes
snd_hdac_i915_init() to return -ENOENT for non-existing i915 graphics
case.  Then the caller can check the error code and handle as no fatal
error.

In SOF side, a few changes have been added to hda_codec_i915_init()
and hda_codec_i915_exit() for skipping the i915 init and exit calls
on such a system.

BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1163677
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206085
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---

This is an attempt to fix the regression for HP OMEN 17, which showed
the probe error as:
  [    6.278811] sof-audio-pci 0000:00:1f.3: error: init i915 and HDMI codec failed
  [    6.278907] sof-audio-pci 0000:00:1f.3: error: failed to probe DSP -19

Although the i915 binding error is gone by the patch, it still fails
to probe:

  [    6.298259] sof-audio-pci 0000:00:1f.3: error: no matching ASoC machine driver found - aborting probe
  [    6.298299] sof-audio-pci 0000:00:1f.3: error: failed to get machine info -19
  [    6.298668] sof-audio-pci 0000:00:1f.3: error: sof_probe_work failed err: -19

I'm submitting this fix as is for now since the i915 error itself
gets fixed, and the rest seems to be another unsolved problem.


Takashi

 sound/hda/hdac_i915.c           | 4 +++-
 sound/soc/sof/intel/hda-codec.c | 5 +++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 3c2db3816029..a451df3efc8a 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -116,6 +116,8 @@ static const struct drm_audio_component_audio_ops i915_init_ops = {
  * with i915 graphics driver.
  *
  * Returns zero for success or a negative error code.
+ * -ENOENT indicates that i915 graphics doesn't exist.
+ * -ENODEV means the binding with i915 graphics failed.
  */
 int snd_hdac_i915_init(struct hdac_bus *bus)
 {
@@ -123,7 +125,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
 	int err;
 
 	if (!i915_gfx_present())
-		return -ENODEV;
+		return -ENOENT;
 
 	init_completion(&bind_complete);
 
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index 9106ab8dac6f..7202355a701e 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -186,6 +186,8 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev)
 
 	/* i915 exposes a HDA codec for HDMI audio */
 	ret = snd_hdac_i915_init(bus);
+	if (ret == -ENOENT)
+		return 0; /* no i915 graphics present on the system */
 	if (ret < 0)
 		return ret;
 
@@ -200,6 +202,9 @@ int hda_codec_i915_exit(struct snd_sof_dev *sdev)
 	struct hdac_bus *bus = sof_to_bus(sdev);
 	int ret;
 
+	if (!bus->audio_component) /* not bound with i915 */
+		return 0;
+
 	hda_codec_i915_display_power(sdev, false);
 
 	ret = snd_hdac_i915_exit(bus);
-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: SOF: intel: Fix probe error without i915 graphics

[Kai Vehmanen]

Hi Takashi,

On Sun, 16 Feb 2020, Takashi Iwai wrote:

> When the driver is configured with CONFIG_SND_SOC_SOF_HDA, the SOF
> driver tries to bind with i915 audio component.  But there is also a
> system without Intel graphics.  On such a system, snd_hdac_i915_init()
> returns -ENODEV error and it leads to the whole probe error of SOF
> driver.

d'oh -- I was a bit too late with this. I've been working on the bug for a 
while and a work in progress patch has been available in the bug for a 
couple of weeks: 
https://github.com/thesofproject/linux/issues/1658 
https://github.com/thesofproject/linux/pull/1731

.. there have been multiple issues, so that the patchset has taken 
multiple rounds, but it's very close now. Probably should have added 
something in the other bugtrackers as well to avoid duplicated efforts.

With my patchset, if i915 is not present or the init fails, only the idisp 
codec is dropped and the probe continues for other codecs.

Your patch will partly conflict with mine as I rely on i915_init to 
report failure in this case. Ok if we wait until my patch gets in? 

Br, Kai
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: SOF: intel: Fix probe error without i915 graphics

[Takashi Iwai]

On Mon, 17 Feb 2020 10:11:59 +0100,
Kai Vehmanen wrote:
> 
> Hi Takashi,
> 
> On Sun, 16 Feb 2020, Takashi Iwai wrote:
> 
> > When the driver is configured with CONFIG_SND_SOC_SOF_HDA, the SOF
> > driver tries to bind with i915 audio component.  But there is also a
> > system without Intel graphics.  On such a system, snd_hdac_i915_init()
> > returns -ENODEV error and it leads to the whole probe error of SOF
> > driver.
> 
> d'oh -- I was a bit too late with this. I've been working on the bug for a 
> while and a work in progress patch has been available in the bug for a 
> couple of weeks: 
> https://github.com/thesofproject/linux/issues/1658 
> https://github.com/thesofproject/linux/pull/1731
> 
> .. there have been multiple issues, so that the patchset has taken 
> multiple rounds, but it's very close now. Probably should have added 
> something in the other bugtrackers as well to avoid duplicated efforts.
> 
> With my patchset, if i915 is not present or the init fails, only the idisp 
> codec is dropped and the probe continues for other codecs.
> 
> Your patch will partly conflict with mine as I rely on i915_init to 
> report failure in this case. Ok if we wait until my patch gets in? 

Sure, mine is just a quick fix.  If there is already a work on it,
let's take it instead.

But, this HP machine still seems broken even after ignoring i915, so
we'd need more work in anyway?


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: SOF: intel: Fix probe error without i915 graphics

[Kai Vehmanen]

Hey,

On Mon, 17 Feb 2020, Takashi Iwai wrote:

> On Mon, 17 Feb 2020 10:11:59 +0100, Kai Vehmanen wrote:
> > With my patchset, if i915 is not present or the init fails, only the idisp 
> > codec is dropped and the probe continues for other codecs.
> > 
> > Your patch will partly conflict with mine as I rely on i915_init to 
> > report failure in this case. Ok if we wait until my patch gets in? 
> 
> Sure, mine is just a quick fix.  If there is already a work on it,
> let's take it instead.

ok, I'll try to fix the remaining issue with module reload today.

> But, this HP machine still seems broken even after ignoring i915, so
> we'd need more work in anyway?

I needed to do some changes in the machine driver as well, but it should 
work on devices like this HP now. SOF will create PCM nodes for HDMI/DP, 
but they will never be connected -- these ports will be handled by the DGPU 
driver.

The one thing I haven't checked is how UCM behaves when the "Jack" nodes 
for HDMI/DP are not created. That may require some further changes to UCM 
(fortunately with UCMv2 we can now additionl conditional statements to 
cover this case as well). Another option is to create dummy mixer nodes, 
but that's not yet in my patch series.

Br, Kai
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: SOF: intel: Fix probe error without i915 graphics

[Kai Vehmanen]

Hey,

On Sun, 16 Feb 2020, Takashi Iwai wrote:

> @@ -200,6 +202,9 @@ int hda_codec_i915_exit(struct snd_sof_dev *sdev)
>  	struct hdac_bus *bus = sof_to_bus(sdev);
>  	int ret;
>  
> +	if (!bus->audio_component) /* not bound with i915 */
> +		return 0;
> +
>  	hda_codec_i915_display_power(sdev, false);
>  
>  	ret = snd_hdac_i915_exit(bus);

hmm, actually found the issue in my patch. It turned out to be a bit 
suprising issue with acomp&devres.

 snd_hdac_i915_exit() calls
   -> snd_hdac_acomp_exit(), which again calls
   -> component.c:component_master_del(dev, &hdac_component_master_ops);
   -> component.c:take_down_master()
   -> devres.c:devres_release_group(master->dev, NULL);

...  now that is somewhat suprising. Basicly that goes on to release all 
devres resources for the hdac_bus device (not just those related to 
acomp!).

I can fix my immediate problem by not calling snd_hdac_i915_exit() if 
hdac_bus contineus to be used, but I'm a bit worried we could hit this in 
other flows.

Anyways, FYI at least for the list archives.

Br, Kai
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: SOF: intel: Fix probe error without i915 graphics

[Takashi Iwai]

On Mon, 17 Feb 2020 14:17:54 +0100,
Kai Vehmanen wrote:
> 
> Hey,
> 
> On Sun, 16 Feb 2020, Takashi Iwai wrote:
> 
> > @@ -200,6 +202,9 @@ int hda_codec_i915_exit(struct snd_sof_dev *sdev)
> >  	struct hdac_bus *bus = sof_to_bus(sdev);
> >  	int ret;
> >  
> > +	if (!bus->audio_component) /* not bound with i915 */
> > +		return 0;
> > +
> >  	hda_codec_i915_display_power(sdev, false);
> >  
> >  	ret = snd_hdac_i915_exit(bus);
> 
> hmm, actually found the issue in my patch. It turned out to be a bit 
> suprising issue with acomp&devres.
> 
>  snd_hdac_i915_exit() calls
>    -> snd_hdac_acomp_exit(), which again calls
>    -> component.c:component_master_del(dev, &hdac_component_master_ops);
>    -> component.c:take_down_master()
>    -> devres.c:devres_release_group(master->dev, NULL);
> 
> ...  now that is somewhat suprising. Basicly that goes on to release all 
> devres resources for the hdac_bus device (not just those related to 
> acomp!).

Does it really?  I thought it applies only to the group, and id=NULL
indicating that it's a local group that was created with id=NULL (by
address match).


Takashi

> 
> I can fix my immediate problem by not calling snd_hdac_i915_exit() if 
> hdac_bus contineus to be used, but I'm a bit worried we could hit this in 
> other flows.
> 
> Anyways, FYI at least for the list archives.
> 
> Br, Kai
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: SOF: intel: Fix probe error without i915 graphics

[Kai Vehmanen]

Hey,

On Mon, 17 Feb 2020, Takashi Iwai wrote:

> On Mon, 17 Feb 2020 14:17:54 +0100, Kai Vehmanen wrote:
> >  snd_hdac_i915_exit() calls
> >    -> snd_hdac_acomp_exit(), which again calls
> >    -> component.c:component_master_del(dev, &hdac_component_master_ops);
> >    -> component.c:take_down_master()
> >    -> devres.c:devres_release_group(master->dev, NULL);
> > 
> > ...  now that is somewhat suprising. Basicly that goes on to release all 
> > devres resources for the hdac_bus device (not just those related to 
> > acomp!).
> 
> Does it really?  I thought it applies only to the group, and id=NULL
> indicating that it's a local group that was created with id=NULL (by
> address match).

at least in my tests, acomp_exit() ended up releasing a bunch of devres 
allocations made with devm_kzalloc() that had no relation to acomp. I 
noted that in devres.c:find_group(), docs say:

/* Find devres group with ID @id.  If @id is NULL, look for the latest. */
static struct devres_group * find_group(struct device *dev, void *id)

... I don't see how it would be limiting a local group.

Br, Kai
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ASoC: SOF: intel: Fix probe error without i915 graphics

[Takashi Iwai]

On Mon, 17 Feb 2020 15:21:37 +0100,
Kai Vehmanen wrote:
> 
> Hey,
> 
> On Mon, 17 Feb 2020, Takashi Iwai wrote:
> 
> > On Mon, 17 Feb 2020 14:17:54 +0100, Kai Vehmanen wrote:
> > >  snd_hdac_i915_exit() calls
> > >    -> snd_hdac_acomp_exit(), which again calls
> > >    -> component.c:component_master_del(dev, &hdac_component_master_ops);
> > >    -> component.c:take_down_master()
> > >    -> devres.c:devres_release_group(master->dev, NULL);
> > > 
> > > ...  now that is somewhat suprising. Basicly that goes on to release all 
> > > devres resources for the hdac_bus device (not just those related to 
> > > acomp!).
> > 
> > Does it really?  I thought it applies only to the group, and id=NULL
> > indicating that it's a local group that was created with id=NULL (by
> > address match).
> 
> at least in my tests, acomp_exit() ended up releasing a bunch of devres 
> allocations made with devm_kzalloc() that had no relation to acomp. I 
> noted that in devres.c:find_group(), docs say:
> 
> /* Find devres group with ID @id.  If @id is NULL, look for the latest. */
> static struct devres_group * find_group(struct device *dev, void *id)
> 
> ... I don't see how it would be limiting a local group.

Isn't it the devres object allocated in the component base code
itself?


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: SOF: intel: Fix probe error without i915 graphics

[Kai Vehmanen]

Hey,

On Mon, 17 Feb 2020, Takashi Iwai wrote:

> On Mon, 17 Feb 2020 15:21:37 +0100,
> Kai Vehmanen wrote: 
>> at least in my tests, acomp_exit() ended up releasing a bunch of devres 
>> allocations made with devm_kzalloc() that had no relation to acomp. I 
>> noted that in devres.c:find_group(), docs say:
> 
> Isn't it the devres object allocated in the component base code
> itself?

true, but problem are other devres allocations done to the PCI device.

I did some more testing and if I force a failure in snd_hdac_i915_init()
and do cleanup with snd_hdac_acomp_exit() in the same function, it works
as expected:

[ 1476.111313] sof-audio-pci 0000:00:1f.3: bound 0000:00:02.0 (ops vgt_balloon_space [i915])
[ 1476.111315] sof-audio-pci 0000:00:1f.3: couldn't bind with audio component
[ 1476.111338] i915 0000:00:02.0: DEVRES REL 00000000b76305ff grp< (0 bytes)
[ 1476.111346] sof-audio-pci 0000:00:1f.3: DEVRES REL 000000006f2e9c46 grp< (0 bytes)
[ 1476.111360] sof-audio-pci 0000:00:1f.3: DEVRES REM 000000000810c760 hdac_acomp_release (64 bytes)
[ 1476.111366] sof-audio-pci 0000:00:1f.3: init of i915 and HDMI codec failed

So this is good. It released the group created for acomp. But, but,
if the init is successful, but later in probe I end up disabling i915 
support (e.g. no codec driver in the kernel) and try to do cleanup of 
acomp resources by calling snd_hdac_i915_exit(), I get this suprising 
devres log:

[ 3007.592029] i915 0000:00:02.0: DEVRES REL 0000000071c16ac0 grp< (0 bytes)
[ 3007.592038] sof-audio-pci 0000:00:1f.3: DEVRES REL 000000008b5ea24e grp< (0 bytes)
[ 3007.592044] sof-audio-pci 0000:00:1f.3: DEVRES REL 00000000f9d4d35c devm_kzalloc_release (1816 bytes)
[ 3007.592051] sof-audio-pci 0000:00:1f.3: DEVRES REL 00000000fa1e4d1a devm_kzalloc_release (1816 bytes)
[ 3007.592057] sof-audio-pci 0000:00:1f.3: DEVRES REL 0000000057d8edec devm_kzalloc_release (320 bytes)
[ 3007.592063] sof-audio-pci 0000:00:1f.3: DEVRES REL 00000000aea01a3d devm_kzalloc_release (320 bytes)
[ 3007.592069] sof-audio-pci 0000:00:1f.3: DEVRES REL 0000000039d17832 devm_kzalloc_release (320 bytes)
[ 3007.592075] sof-audio-pci 0000:00:1f.3: DEVRES REL 000000009958e500 devm_kzalloc_release (320 bytes)
[ 3007.592081] sof-audio-pci 0000:00:1f.3: DEVRES REL 00000000eea2922f devm_kzalloc_release (320 bytes)
[ 3007.592088] sof-audio-pci 0000:00:1f.3: DEVRES REL 00000000dd6f6f2a devm_kzalloc_release (320 bytes)
[ 3007.592093] sof-audio-pci 0000:00:1f.3: DEVRES REL 000000005ca30f2d devm_kzalloc_release (320 bytes)
[ 3007.592099] sof-audio-pci 0000:00:1f.3: DEVRES REL 00000000bf594d2d devm_kzalloc_release (320 bytes)
[ 3007.592104] sof-audio-pci 0000:00:1f.3: DEVRES REL 000000009a0669c2 devm_kzalloc_release (320 bytes)
[ 3007.592110] sof-audio-pci 0000:00:1f.3: DEVRES REL 00000000757c42ee devm_kzalloc_release (320 bytes)
[ 3007.592115] sof-audio-pci 0000:00:1f.3: DEVRES REL 00000000a928376b devm_kzalloc_release (320 bytes)
[ 3007.592121] sof-audio-pci 0000:00:1f.3: DEVRES REL 000000007bf93e19 devm_kzalloc_release (320 bytes)
[ 3007.592127] sof-audio-pci 0000:00:1f.3: DEVRES REL 00000000485f5b74 devm_kzalloc_release (320 bytes)
[ 3007.592133] sof-audio-pci 0000:00:1f.3: DEVRES REL 000000007c95c553 devm_kzalloc_release (320 bytes)
[ 3007.592139] sof-audio-pci 0000:00:1f.3: DEVRES REL 00000000d7b60566 devm_kzalloc_release (320 bytes)
[ 3007.592145] sof-audio-pci 0000:00:1f.3: DEVRES REL 000000002805a826 devm_kzalloc_release (320 bytes)
[ 3007.592160] sof-audio-pci 0000:00:1f.3: DEVRES REM 0000000085720a3d hdac_acomp_release (64 bytes)


These allocations are not done for acomp, but are rather allocs
done for the SOF DSP device during the init. E.g. the 1816 byte alloc is 
devm_kzalloc of "struct hdac_hda_priv":

[ 3007.558102] sof-audio-pci 0000:00:1f.3: HDA codec #0 probed OK: response: 10ec0700
[ 3007.558109] sof-audio-pci 0000:00:1f.3: DEVRES ADD 00000000fa1e4d1a devm_kzalloc_release (1816 bytes)

... now I guess this is just how the component framework works. We do 
register the hdac-bus device as aggregate master device in 
snd_hdac_acomp_init() by calling component_master_add_with_match(). So 
calling snd_hdac_acomp_exit() will bring down the aggregate master device.

I now worked around this my putting the acomp_exit() at end of the SOF
DSP device removal.

Br, Kai