From patchwork Thu Nov 26 16:16:26 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 7707851 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 16A569F401 for ; Thu, 26 Nov 2015 16:16:34 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D8536206C3 for ; Thu, 26 Nov 2015 16:16:32 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id B11032063D for ; Thu, 26 Nov 2015 16:16:31 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 431EB6E0E5; Thu, 26 Nov 2015 08:16:31 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4A2906E0E5 for ; Thu, 26 Nov 2015 08:16:30 -0800 (PST) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id C8DC4AAB2; Thu, 26 Nov 2015 16:14:47 +0000 (UTC) Date: Thu, 26 Nov 2015 17:16:26 +0100 Message-ID: From: Takashi Iwai To: Ville =?UTF-8?B?U3lyasOkbMOk?= In-Reply-To: <20151126155809.GE4437@intel.com> References: <8082FF9BCB2B054996454E47167FF4EC0B0F2951@SHSMSX104.ccr.corp.intel.com> <8082FF9BCB2B054996454E47167FF4EC0B0F2A4D@SHSMSX104.ccr.corp.intel.com> <56572056.6090805@canonical.com> <56572548.1020603@canonical.com> <20151126154323.GD4437@intel.com> <20151126155809.GE4437@intel.com> 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: "Vetter, Daniel" , "alsa-devel@alsa-project.org" , "intel-gfx@lists.freedesktop.org" , David Henningsson Subject: Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, 26 Nov 2015 16:58:09 +0100, Ville Syrjälä wrote: > > On Thu, Nov 26, 2015 at 04:51:04PM +0100, Takashi Iwai wrote: > > On Thu, 26 Nov 2015 16:43:23 +0100, > > Ville Syrjälä wrote: > > > > > > On Thu, Nov 26, 2015 at 04:29:12PM +0100, David Henningsson wrote: > > > > > > > > > > > > On 2015-11-26 16:23, Takashi Iwai wrote: > > > > > On Thu, 26 Nov 2015 16:08:06 +0100, > > > > > David Henningsson wrote: > > > > >> > > > > >> > > > > >> > > > > >> On 2015-11-26 10:24, Takashi Iwai wrote: > > > > >>> On Thu, 26 Nov 2015 10:16:17 +0100, > > > > >>> Zhang, Xiong Y wrote: > > > > >>>> > > > > >>>> > > > > >>>>> On Thu, 26 Nov 2015 08:57:30 +0100, > > > > >>>>> Zhang, Xiong Y wrote: > > > > >>>>>> > > > > >>>>>>>>> BTW, I have a patchset to avoid the audio h/w wakeup by a new > > > > >>>>>>>>> component ops to get ELD and connection states. It was posted to > > > > >>>>>>>>> alsa-devel shortly ago just as a reference, but this should work well > > > > >>>>>>>>> in such a case, too. The test patches are found in test/hdmi-jack > > > > >>>>>>>>> branch of my sound git tree. > > > > >>>>>>> > > > > >>>>>>> Did you try this branch (merge onto intel-test-nightly)? > > > > >>>>>>> > > > > >>>>>> [Zhang, Xiong Y] Yes, this branch could fix my issue. > > > > >>>>> > > > > >>>>> OK, good to hear. But this isn't for 4.4 or backport, as it's more > > > > >>>>> radical changes. > > > > >>>>> > > > > >>>>> Could you check the patch below instead? This suppresses the notifier > > > > >>>>> handling during suspend. It's bad, but the hotplug should be still > > > > >>>>> handled via normal unsol event, so it should keep working, good enough > > > > >>>>> as a stop gap. > > > > >>>>> > > > > >>>>> > > > > >>>>> Takashi > > > > >>>>> > > > > >>>>> --- > > > > >>>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > > > > >>>>> index bdb6f226d006..f7c70e2ae65c 100644 > > > > >>>>> --- a/sound/pci/hda/patch_hdmi.c > > > > >>>>> +++ b/sound/pci/hda/patch_hdmi.c > > > > >>>>> @@ -33,6 +33,7 @@ > > > > >>>>> #include > > > > >>>>> #include > > > > >>>>> #include > > > > >>>>> +#include > > > > >>>>> #include > > > > >>>>> #include > > > > >>>>> #include > > > > >>>>> @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int > > > > >>>>> port) > > > > >>>>> struct hda_codec *codec = audio_ptr; > > > > >>>>> int pin_nid = port + 0x04; > > > > >>>>> > > > > >>>>> - check_presence_and_report(codec, pin_nid); > > > > >>>>> + if (!atomic_read(&codec->core.in_pm) && > > > > >>>>> + !pm_runtime_suspended(hda_codec_dev(codec))) > > > > >>>>> + check_presence_and_report(codec, pin_nid); > > > > >>>>> } > > > > >>>>> > > > > >>>>> static int patch_generic_hdmi(struct hda_codec *codec) > > > > >>>> [Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend(). > > > > >>> > > > > >>> OK, then the problem isn't about the HD-audio driver but rather i915 > > > > >>> driver. As already mentioned, i915 driver shouldn't issue eld_notify > > > > >>> callback in the suspend code path. > > > > >> > > > > >> We don't have a complete drm log so I'm only speculating here; but the > > > > >> HDA log seems to indicate the power well is off when we try to talk to > > > > >> the HDA controller. That means either the i915 shut it down while we > > > > >> were holding a lock on it, or HDA tried to lock it and that didn't make > > > > >> it through; in which case the HDA side should handle an error from > > > > >> get_power more gracefully...? > > > > > > > > > > My understanding is that it's triggered *during* i915 suspend. Then > > > > > the path calls the HDA callback, and HDA controller tries to get power > > > > > and proceeds as it has no idea that it's being shut off. > > > > > > > > Well; that can also be stopped by letting the get_power call return a > > > > failure code in case i915 is trying to suspend itself. That seems more > > > > robust to me, as it could potentially avoid other S3 races too...? > > > > > > > > > > > > > > Unfortunately, neither get_power callback or the corresponding HDA > > > > > code has a capability to check that state. So, changing / fixing this > > > > > there would be nice but very hard. > > > > > > > > Surely the i915 driver has some "am_i_suspending" variable it can check > > > > in the get_power callback? > > > > > > I don't understand why you need to do anything special. When the eld > > > notify happens during suspend, the hardware is still fully powered up, > > > so it should just work(tm) as long as the eld_notify is a synchronous > > > call and it drops the power reference at the end. > > > > Hm, that's the question. It's never clear so far as we haven't got > > any detailed logs. > > > > The symptom implies that the graphics side is off while HDA tries to > > execute some verbs. So the power well is the first suspect. We reall > > need to track down the code path triggering the issue. > > > > > I guess for any get_power after i915 has suspended we'd need to just > > > reject the get_power call. Or does something force hda to suspend before > > > and resume after i915 always? > > > > The HDA code itself calls get_power at the beginning of the resume. > > But not sure whether this suffices for the execution ordering. > > Just chatted with Imre a bit, and he clarified the suspend order thing: > > So what happens is that the normal suspend hooks for hda and i915 can > in theory happen in any order. But i915 reamains powered on until the > late suspend hook. > > So we can get > 1. i915 suspend > 2. hda suspend > 3. i915 late suspend > > or > 1. hda suspend > 2. i915 suspend > 3. i915 late suspend > > So in this latter case i915 can call the eld notify hook after hda has > already suspended. So that at least you would need to handle in your > side. OK, for the latter case, we may test a band-aid patch like below. This will skip the notifier certainly while being suspended. Xiong, could you check it? Takashi diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..0cd7bb30b045 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,6 +2352,10 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) struct hda_codec *codec = audio_ptr; int pin_nid = port + 0x04; + /* skip notification during suspend */ + if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0) + return; + check_presence_and_report(codec, pin_nid); }