Re: [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events

From: Imre Deak <imre.deak@intel.com>
To: David Henningsson <david.henningsson@canonical.com>,
	Takashi Iwai <tiwai@suse.de>
Cc: "Vetter, Daniel" <daniel.vetter@intel.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
Date: Thu, 26 Nov 2015 17:37:11 +0200	[thread overview]
Message-ID: <1448552231.15224.28.camel@intel.com> (raw)
In-Reply-To: <56572548.1020603@canonical.com>

On to, 2015-11-26 at 16:29 +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 <linux/delay.h>
> > > > > >    #include <linux/slab.h>
> > > > > >    #include <linux/module.h>
> > > > > > +#include <linux/pm_runtime.h>
> > > > > >    #include <sound/core.h>
> > > > > >    #include <sound/jack.h>
> > > > > >    #include <sound/asoundef.h>
> > > > > > @@ -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?

As I understand this happens during the i915 system suspend/resume
hooks are running. There is no reason why the getting a power domain
reference would fail there. In fact we are keeping all power wells for
the whole duration of these callbacks, see the call to
intel_display_set_init_power(true) in i915_drm_suspend()
and i915_drm_resume_early()->intel_power_domains_init_hw(). So not sure
how the audio power well could be off at that time.

> 
> > 
> > So I believe it's easier to avoid calling the eld_notify callback
> > from
> > i915 side during its suspend code.
> > 
> > 
> > Takashi
> > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx