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

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

[Anshuman Gupta]

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;
 	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;
 	/* Power up afg */
 	snd_hdac_codec_read(hdac, hdac->afg, 0,	AC_VERB_SET_POWER_STATE,
 							AC_PWRST_D0);
-- 
2.7.4

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

[Rafael J. Wysocki]

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.

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

>         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
>

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

[Rafael J. Wysocki]

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:
>> > 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?

It might help, but is this a leaf device or does it have children?

>> 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);
>> >

So it sounds like you need the "get" part, but you don't need the
"sync" one, because you've just checked that the device is not
suspended.

I guess the idea is to return 1 for the direct-complete mechanism to
kick in if the device is already suspended, but otherwise bump up its
reference counter to prevent it from suspending going forward, right?

>> >         /*
>> > @@ -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.
>>

And here you want to avoid the below if the device is still suspended.

Why is the below code located in the ->complete callback anyway?
Shouldn't it be there in the ->resume one?

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

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

[Subhransu S. Prusty]

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.

I think this patch defeats the purpose.

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

-- 

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

[Subhransu S. Prusty]

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

-- 

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

[Takashi Iwai]

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

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

[Lukas Wunner]

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

[PATCH v2 0/1] cover-letter hdac-codec runtime suspended at PM:Suspend.

[Anshuman Gupta]

Current implementation of hdac hdmi codec driver uses its
suspend/resume operation callback in its prepare/complete callback
which has issues with hdac direct-complete, it has been reviewed earlier
that hdac hdmi codec driver requires a rework
(https://patchwork.kernel.org/patch/10276021/),
but as there is no rework with driver, it require to fix hdac direct complete
issue to leverage hdac direct complete in order to optimize its resume latency
while resuming from system wide suspend.

Anshuman Gupta (1):
  hdac-codec runtime suspended at PM:Suspend.

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

-- 
2.7.4

[PATCH v2 1/1] hdac-codec runtime suspended at PM:Suspend.

[Anshuman Gupta]

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

Re: [PATCH v2 1/1] hdac-codec runtime suspended at PM:Suspend.

[Anshuman Gupta]

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.
 
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.
 

Re: [PATCH v2 1/1] hdac-codec runtime suspended at PM:Suspend.

[Takashi Iwai]

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.
>  
>