From patchwork Fri Jun 12 06:06:45 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 6594701 X-Patchwork-Delegate: tiwai@suse.de Return-Path: X-Original-To: patchwork-alsa-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 6C8FDC0020 for ; Fri, 12 Jun 2015 06:07:12 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8DEBD20670 for ; Fri, 12 Jun 2015 06:07:10 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.kernel.org (Postfix) with ESMTP id 24E0F20671 for ; Fri, 12 Jun 2015 06:07:09 +0000 (UTC) Received: by alsa0.perex.cz (Postfix, from userid 1000) id 13DFB2666DB; Fri, 12 Jun 2015 08:07:03 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 Received: from alsa0.perex.cz (localhost [IPv6:::1]) by alsa0.perex.cz (Postfix) with ESMTP id B92BA2666A1; Fri, 12 Jun 2015 08:06:54 +0200 (CEST) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id C85EE2666A5; Fri, 12 Jun 2015 08:06:53 +0200 (CEST) Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 947B4266690 for ; Fri, 12 Jun 2015 08:06:46 +0200 (CEST) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 6A36EAAC1; Fri, 12 Jun 2015 06:06:45 +0000 (UTC) Date: Fri, 12 Jun 2015 08:06:45 +0200 Message-ID: From: Takashi Iwai To: "Lin, Mengdong" In-Reply-To: References: <1433931965-12337-1-git-send-email-tiwai@suse.de> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Cc: "Yang, Libin" , "alsa-devel@alsa-project.org" , David Henningsson Subject: Re: [alsa-devel] [PATCH 1/2] ALSA: hda - Continue probing even if i915 binding fails X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" 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 X-Virus-Scanned: ClamAV using ClamSMTP At Fri, 12 Jun 2015 05:50:43 +0000, Lin, Mengdong wrote: > > > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Friday, June 12, 2015 1:09 PM > > To: Lin, Mengdong > > Cc: alsa-devel@alsa-project.org; David Henningsson; Yang, Libin > > Subject: Re: [PATCH 1/2] ALSA: hda - Continue probing even if i915 binding fails > > > > 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. > > Hi Takashi, > > I think there is no error because the i915 power well is ON by default on > Haswell and BIOS does not touch this. However, it's possible that the BIOS may > disable this power well and depends on i915 driver to turn it on when need, or > just disable the power well when disabling the Intel GPU. > > I remember previously we met audio register I/O error when HSW introduced > this power well and i915 disabled it without sync with audio driver. > > Since we cannot expect the exact BIOS behavior, I feel it's safer not to use this > HD-A controller without i915 on HSW and BDW. Fair enough. Then I'm going to queue the patch below in addition to for-linus branch. thanks, Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] ALSA: hda - Abort the probe without i915 binding for HSW/BDW The previous patch tried to continue the probe if i915 binding fails. For for simplicity reason, we haven't implemented abort even for controller chips that are dedicated for HDMI/DP on HSW and BDW. However, Mengdong suggested that this can be dangerous; BIOS may disable gfx power well although the PCI entry for HD-audio is left, and this may result in the unexpected behavior, kernel errors, etc. For avoiding this situation, abort the probe at i915 binding failure only for HSW/BDW chips selectively. For other chips, it still continues. Fixes: bf06848bdbe5 ('ALSA: hda - Continue probing even if i915 binding fails') Reported-by: Mengdong Lin Signed-off-by: Takashi Iwai --- sound/pci/hda/hda_intel.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 8a0af6770e1d..a244ba706317 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -340,6 +340,11 @@ enum { #define use_vga_switcheroo(chip) 0 #endif +#define CONTROLLER_IN_GPU(pci) (((pci)->device == 0x0a0c) || \ + ((pci)->device == 0x0c0c) || \ + ((pci)->device == 0x0d0c) || \ + ((pci)->device == 0x160c)) + static char *driver_short_names[] = { [AZX_DRIVER_ICH] = "HDA Intel", [AZX_DRIVER_PCH] = "HDA Intel PCH", @@ -1854,8 +1859,17 @@ static int azx_probe_continue(struct azx *chip) if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) { #ifdef CONFIG_SND_HDA_I915 err = hda_i915_init(hda); - if (err < 0) - goto skip_i915; + if (err < 0) { + /* if the controller is bound only with HDMI/DP + * (for HSW and BDW), we need to abort the probe; + * for other chips, still continue probing as other + * codecs can be on the same link. + */ + if (CONTROLLER_IN_GPU(pci)) + goto out_free; + else + goto skip_i915; + } err = hda_display_power(hda, true); if (err < 0) { dev_err(chip->card->dev,