From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 1/2] ALSA: hda - Continue probing even if i915 binding fails Date: Fri, 12 Jun 2015 07:08:36 +0200 Message-ID: References: <1433931965-12337-1-git-send-email-tiwai@suse.de> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id CE4DE266640 for ; Fri, 12 Jun 2015 07:08:39 +0200 (CEST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: "Lin, Mengdong" Cc: "Yang, Libin" , "alsa-devel@alsa-project.org" , David Henningsson List-Id: alsa-devel@alsa-project.org At Fri, 12 Jun 2015 02:08:50 +0000, Lin, Mengdong wrote: > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Wednesday, June 10, 2015 6:26 PM > > > > Currently snd-hda-intel driver aborts the probing of Intel HD-audio controller > > with i915 power well management when binding with i915 driver via > > hda_i915_init() fails. This is no big problem for Haswell and Broadwell where > > the HD-audio controllers are dedicated to HDMI/DP, thus i915 link is > > mandatory. However, Skylake, Baytrail and Braswell have only one controller > > and both HDMI/DP and analog codecs share the same bus. Thus, even if > > HDMI/DP isn't usable, we should keep the controller working for other codecs. > > > > For fixing this, this patch simply allows continuing the probing even if > > hda_i915_init() call fails. This may leave stale sound components for > > HDMI/DP devices that are unbound with graphics. We could abort the > > probing selectively, but from the code simplicity POV, it's better to continue in > > all cases. > > > > Reported-by: Libin Yang > > Signed-off-by: Takashi Iwai > > --- > > This is for 4.1. Appying to 4.2 may result in trivial conflicts. > > > > sound/pci/hda/hda_intel.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index > > fea198c58196..8a0af6770e1d 100644 > > --- a/sound/pci/hda/hda_intel.c > > +++ b/sound/pci/hda/hda_intel.c > > @@ -1855,7 +1855,7 @@ static int azx_probe_continue(struct azx *chip) > > #ifdef CONFIG_SND_HDA_I915 > > err = hda_i915_init(hda); > > if (err < 0) > > - goto out_free; > > + goto skip_i915; > > err = hda_display_power(hda, true); > > if (err < 0) { > > dev_err(chip->card->dev, > > @@ -1865,6 +1865,7 @@ static int azx_probe_continue(struct azx *chip) > > #endif > > } > > > > + skip_i915: > > err = azx_first_init(chip); > > if (err < 0) > > goto out_free; > > -- > > 2.4.3 > > Thanks for your patch. But maybe we should not skip i915 for the dedicated > display HD-A controller on Haswell/Broadwell. > > For HSW/BDW, their HD-A controller for display audio (PCI dev#3) is *partly* > in GPU power domain: > - Its PCI configuration space is out of i915 power well, but in an always-on > power well. So even if the BIOS disables the Intel GPU (PCI dev#2), the HD-A > controller is still present, and we can still access all standard registers in PCI > config space. Right, and changing this doesn't break things. > - Its audio functions are in the i915 power well. If hda_i915_init() fails and > I915 power well is unavailable, read/write to the audio register of the > Controller will fail and we'll get kernel error messages. So I feel it's better > to abort if hda_i915_init() fails for this HD-A controller. By skipping, the access to i915 power well won't happen later, since the component ops isn't set. I tested Haswell machines with nomodeset to simulate the situation, and there was no any kernel errors except for the first one indicating the failure of i915 binding. I haven't tested Broadwell, though. thanks, Takashi