Re: [alsa-devel] [PATCH] [sound] hdac-codec runtime suspended at PM:Suspend.
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.
Regards, Subhransu
Regards, Subhransu
> /* Power up afg */ > snd_hdac_codec_read(hdac, hdac->afg, 0, AC_VERB_SET_POWER_STATE, > AC_PWRST_D0); > -- > 2.7.4
-- Thanks, Anshuman
--
-- Thanks, Anshuman
--
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
On Mon, Mar 26, 2018 at 09:30:36AM +0200, Takashi Iwai wrote:
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:
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.
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.
FWIW, the i915 folks have put on their todo list to migrate the i915/hda_intel interaction to device links:
https://git.kernel.org/next/linux-next/c/7b3b61b62a58
Something similar has already been done for HDA controllers integrated into AMD / Nvidia GPUs with:
https://git.kernel.org/next/linux-next/c/07f4f97d7b4b
With device links, direct_complete will be used naturally, so time is probably best spent working towards this approach.
Thanks,
Lukas
participants (3)
-
Lukas Wunner -
Subhransu S. Prusty -
Takashi Iwai