Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 26 Mar 2018 09:30:36 +0200
Message-ID: <s5hr2o770pv.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     " Subhransu S. Prusty " <subhransu.s.prusty@intel.com>
Cc:     "Anshuman Gupta" <anshuman.gupta@intel.com>,
        "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." 
        <alsa-devel@alsa-project.org>,
        "Liam Girdwood" <lgirdwood@gmail.com>,
        "Mark Brown" <broonie@kernel.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        "Jaroslav Kysela" <perex@perex.cz>,
        "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
        "Linux PM" <linux-pm@vger.kernel.org>
Subject: Re: [PATCH] [sound] hdac-codec runtime suspended at PM:Suspend.
In-Reply-To: <20180326070349.GA14895@subhransu-desktop>
References: <1520853467-31653-1-git-send-email-anshuman.gupta@intel.com>
        <CAJZ5v0hyk=jto8Wt0er==YmKEsgvfvZhO2wDbGOhxZaWWCNgBg@mail.gmail.com>
        <5aa8fbe9.4251620a.c3daa.3711SMTPIN_ADDED_BROKEN@mx.google.com>
        <CAJZ5v0jrKPNFhgpoL97Ac-k9jAmMa-vzgc-4PD+uuynSMqE2Fw@mail.gmail.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/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 26 Mar 2018 09:03:55 +0200,
 Subhransu S. Prusty  wrote:
> 
> On Thu, Mar 15, 2018 at 09:02:30PM +0530, Anshuman Gupta wrote:
> > On Thu, Mar 15, 2018 at 04:12:44PM +0530, Subhransu S. Prusty wrote:
> > > On Wed, Mar 14, 2018 at 09:07:14PM +0530, Anshuman Gupta wrote:
> > > > On Wed, Mar 14, 2018 at 11:53:58AM +0100, Rafael J. Wysocki wrote:
> > > > > On Wed, Mar 14, 2018 at 11:38 AM, Anshuman Gupta
> > > > > <anshuman.gupta@intel.com> wrote:
> > > > > > On Mon, Mar 12, 2018 at 12:26:53PM +0100, Rafael J. Wysocki wrote:
> > > > > >> On Mon, Mar 12, 2018 at 12:17 PM, Anshuman Gupta
> > > > > >> <anshuman.gupta@intel.com> wrote:
> > > > > >> >
> > > > > >> > +       if (pm_runtime_status_suspended(dev))
> > > > > >> > +               return;
> > > > > >>
> > > > > >> That, again, is somewhat fragile from the concurrency perspective.
> > > > > >>
> > > > > 
> > > > > And here you want to avoid the below if the device is still suspended.
> > > >   Yes, if we do not avoid the code below, complete callback takes about 
> > > >   3 seconds due to snd_hdac_codec_read timed out because hdac controller 
> > > >   would be in runtime suspend state.	
> > > > > 
> > > > > Why is the below code located in the ->complete callback anyway?
> > > > > Shouldn't it be there in the ->resume one?
> > > > > 
> > > >   Yes even i am also having same doubt, why these power down and power up
> > > >   sequences are part of prepare and complete callback.
> > > >   Adding driver author "Subhransu S. Prusty" to provide more inputs on this.
> > > 
> > > This driver needs a late resume as it receives a jack notification from the
> > > i915 driver and the skl controller driver resume may not have happened and
> > > in turn hda controller may not ready. This ensures a synchronization for
> > > jack event during resume from S3.
> >   Let me give you insight of the issue, this driver blocks the direct complete
> >   of hda controller PCI 00:1f.3, sometimes it takes 280ms to resume hda controller
> >   from S3 and s2idle. So it does not make sense to suspend/resume hda controller 
> >   when it is already in runtime suspend.
> 
> I get it now. I think this patch needs rework to address both the issues. 
> 
> > > 
> > > I think this patch defeats the purpose.
> >   Here in this case PCI driver may kick the direct complete for hda controller
> >   https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-driver.c#L691
> >   But hdac hdmi codec driver is blocking it.
> >   So i think it will be ok to keep this codec and hda controller in runtime 
> >   suspend while entering S3 or s2idle, both can resume by runtime PM as well,
> >   will it brake any audio functionality?
> 
> There was some PM and jack detection issues (I don't recollect now) because
> of which this was added. This was due to the gfx display power and hda
> controller synchronization. The rework may be required in both hdac_hdmi
> and skylake drivers.

If it's about the power well issue, it had been fixed in multiple
ways.  In i915 side, every relevant component callback is wrapped with
power domain get/put.  And, at least HD-audio legacy side, such a
spurious notification is filtered out in the eld notifier:

static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe)
{
	....
	/* skip notification during system suspend (but not in runtime PM);
	 * the state will be updated at resume
	 */
	if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
		return;
	/* ditto during suspend/resume process itself */
	if (atomic_read(&(codec)->core.in_pm))
		return;

	snd_hdac_i915_set_bclk(&codec->bus->core);
	check_presence_and_report(codec, pin_nid, dev_id);
}

Last but not least, the jack state is explicitly updated via reading
the ELD at resume callback.

In that way, we can live with the standard suspend/resume procedure.


Takashi