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@xxxxxxxxx> 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@xxxxxxxxx> 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
