Re: [PATCH] [sound] hdac-codec runtime suspended at PM:Suspend.

[Takashi Iwai <tiwai@suse.de>]

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