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:

...

Keep hdac-codec to be in runtime suspended while entering to suspend. If hdac-codec is already in runtime suspend state skip its power down sequence in prepare, power up sequence in complete phase.

Avoid resuming hdac controller PCI device 00:1f.3 from runtime suspend state in case hdac-codec already in runtime-suspend state, this is unnecessary and block the direct complete even for hdac controller PCI device 00:1f.3.

This enabled direct complete path for hdac-codec and PCI device 00:1f.3.

Signed-off-by: Anshuman Gupta anshuman.gupta@intel.com

sound/soc/codecs/hdac_hdmi.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index f3b4f4d..810a8a6 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -1852,6 +1852,8 @@ static int hdmi_codec_prepare(struct device *dev) struct hdac_ext_device *edev = to_hda_ext_device(dev); struct hdac_device *hdac = &edev->hdac;

•   if (pm_runtime_status_suspended(dev))
  

•           return 1;
  

This is racy in principle, because the runtime PM status can still change after you've checked here.

Will using pm_runtime_disable/pm_runtime_enable for safe check of runtime status avoids this race?

But even if it isn't racy in practice, the following pm_runtime_get_sync() becomes redundant after it, doesn't it?

I have no idea but if there can be a case that PCI 00:1f.3 (hdac controller) is runtime suspended and hdac hdmi codec is active, in that case it may be required to use pm_runtime_get_sync() for hdac controller as it is parent of hdac hdmi codec.

...

    pm_runtime_get_sync(&edev->hdac.dev);

    /*

@@ -1873,6 +1875,8 @@ static void hdmi_codec_complete(struct device *dev) struct hdac_hdmi_priv *hdmi = edev->private_data; struct hdac_device *hdac = &edev->hdac;

•   if (pm_runtime_status_suspended(dev))
  

•           return;
  

That, again, is somewhat fragile from the concurrency perspective.

...

    /* Power up afg */
    snd_hdac_codec_read(hdac, hdac->afg, 0, AC_VERB_SET_POWER_STATE,
                                                    AC_PWRST_D0);

-- 2.7.4

Keep hdac hdmi codec to be in runtime suspended while entering to system wide suspend. Currently hdac hdmi codec driver using its suspend and resume operation in prepare and complete PM callbacks, and it resumes the hd audio controller (parent of self) from runtime suspend and blocks the direct complete for hd audio controller.

If hdac-codec is already in runtime suspend state skip its power down sequence in prepare, power up sequence in complete phase. It enables direct complete path for hdac-codec and hd audio controller PCI device.

Signed-off-by: Anshuman Gupta anshuman.gupta@intel.com --- Changes in v2 - Changed the commit message. - Using pm_runtime_suspended instead of pm_runtime_status_suspended in order to handle any race condition.

sound/soc/codecs/hdac_hdmi.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 84f7a7a..e965338 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -1859,6 +1859,9 @@ static int hdmi_codec_prepare(struct device *dev) struct hdac_ext_device *edev = to_hda_ext_device(dev); struct hdac_device *hdev = &edev->hdev;

+ if (pm_runtime_suspended(dev)) + return 1; + pm_runtime_get_sync(&edev->hdev.dev);

/* @@ -1880,6 +1883,9 @@ static void hdmi_codec_complete(struct device *dev) struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(&edev->hdev); struct hdac_device *hdev = &edev->hdev;

+ if (pm_runtime_suspended(dev)) + return; + /* Power up afg */ snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE, AC_PWRST_D0);

On Tue, 28 Aug 2018 18:34:15 +0200, Anshuman Gupta wrote:

Is this patch not in consideration, there are no review comment for it. this patch fixes an issue with hdac hdmi codec driver.

On one of our platform HD audio controller takes arounf 300ms. Below are the snippet of dmesg log.

[ 3778.461888] calling 0000:00:0e.0+ @ 3667, parent: pci0000:00, cb: pci_pm_resume [ 3778.775273] call 0000:00:0e.0+ returned 0 after 306016 usecs

When HD audio controller is in runtime suspend state, with direct complete, we can improve overall system wide resume latency.

Actually, *this* should have been mentioned in the changelog, and the subject would be better phrased to reflect it.

After all, you're trying to reduce / optimize the runtime PM latency.

thanks,

Takashi

On Sat, Aug 18, 2018 at 11:42:03PM +0530, Anshuman Gupta wrote:

...

Keep hdac hdmi codec to be in runtime suspended while entering to system wide suspend. Currently hdac hdmi codec driver using its suspend and resume operation in prepare and complete PM callbacks, and it resumes the hd audio controller (parent of self) from runtime suspend and blocks the direct complete for hd audio controller.

If hdac-codec is already in runtime suspend state skip its power down sequence in prepare, power up sequence in complete phase. It enables direct complete path for hdac-codec and hd audio controller PCI device.

Signed-off-by: Anshuman Gupta anshuman.gupta@intel.com

Changes in v2

• Changed the commit message.
• Using pm_runtime_suspended instead of pm_runtime_status_suspended in order to handle any race condition.

sound/soc/codecs/hdac_hdmi.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 84f7a7a..e965338 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -1859,6 +1859,9 @@ static int hdmi_codec_prepare(struct device *dev) struct hdac_ext_device *edev = to_hda_ext_device(dev); struct hdac_device *hdev = &edev->hdev;

• if (pm_runtime_suspended(dev))
  

•         return 1;
  

• pm_runtime_get_sync(&edev->hdev.dev);
  
  /*
  

@@ -1880,6 +1883,9 @@ static void hdmi_codec_complete(struct device *dev) struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(&edev->hdev); struct hdac_device *hdev = &edev->hdev;

• if (pm_runtime_suspended(dev))
  

•         return;
  

• /* Power up afg */
  snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE,
                                                  AC_PWRST_D0);
  

-- 2.7.4

-- Thanks, Anshuman.