| Message ID | 1546827721-25586-1-git-send-email-libin.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2,1/2] ASoC: refine ASoC hdmi audio suspend/resume | expand |
On 1/6/19 8:22 PM, libin.yang@intel.com wrote: > From: Libin Yang <libin.yang@intel.com> > > hdmi_codec_prepare() will trigger hdmi runtime resume, which will set > the bitmask of hdev->addr. And skl_suspend() will clear the bitmask of > HDA_CODEC_IDX_CONTROLLER. HDMI codec idx is not the same as > HDA_CODEC_IDX_CONTROLLER, which means i915 power will not be released > when suspend. > > On the other hand, hdmi_codec_prepare() don't need to call > pm_runtime_get_sync() to wake up the audio subsystem (HDMI auido) > for setting the codec registers. Turning display power on with > snd_hdac_display_power() is enough. > > Let's use S3 without playback as an example: > hdmi_codec_prepare() invokes the runtime resume of codec => > snd_hdac_display_power(bus, hdev->addr, true) > skl runtime resume > skl_suspend() => > snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false); > > THis means hdev->addr will never release the display power when > suspend. > > The new sequence will be: > hdmi_codec_prepare() => > snd_hdac_display_power(bus, hdev->addr, true) > snd_hdac_display_power(bus, hdev->addr, false) > skl runtime resume > skl suspned I for one find this RFC difficult to follow. The documentation of "Power management sequences" seems obsolete after Takashi's changes, you may want to start with an update of the documentation which might help point out what is broken in the sequences. I also don't see the point in trying to bypass the runtime_pm code in codec_prepare and codec_complete. if we have the display on/off sequence only in probe/remove and suspend/resume it makes it easy to track states, and I wonder if it makes sense anyways to be in suspend with the display on or vice-versa in D0 with the display off. Simple state machines always win. > > Signed-off-by: Libin Yang <libin.yang@intel.com> > --- > sound/soc/codecs/hdac_hdmi.c | 6 ++++-- > sound/soc/intel/skylake/skl.c | 7 ------- > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c > index 3ab2949..782b323 100644 > --- a/sound/soc/codecs/hdac_hdmi.c > +++ b/sound/soc/codecs/hdac_hdmi.c > @@ -1895,7 +1895,7 @@ static int hdmi_codec_prepare(struct device *dev) > { > struct hdac_device *hdev = dev_to_hdac_dev(dev); > > - pm_runtime_get_sync(&hdev->dev); > + snd_hdac_display_power(hdev->bus, hdev->addr, true); > > /* > * Power down afg. > @@ -1906,6 +1906,7 @@ static int hdmi_codec_prepare(struct device *dev) > */ > snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE, > AC_PWRST_D3); > + snd_hdac_display_power(hdev->bus, hdev->addr, false); > > return 0; > } > @@ -1915,6 +1916,7 @@ static void hdmi_codec_complete(struct device *dev) > struct hdac_device *hdev = dev_to_hdac_dev(dev); > struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); > > + snd_hdac_display_power(hdev->bus, hdev->addr, true); > /* Power up afg */ > snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE, > AC_PWRST_D0); > @@ -1930,7 +1932,7 @@ static void hdmi_codec_complete(struct device *dev) > */ > hdac_hdmi_present_sense_all_pins(hdev, hdmi, false); > > - pm_runtime_put_sync(&hdev->dev); > + snd_hdac_display_power(hdev->bus, hdev->addr, false); > } > #else > #define hdmi_codec_prepare NULL > diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c > index 60c9483..89f4d66 100644 > --- a/sound/soc/intel/skylake/skl.c > +++ b/sound/soc/intel/skylake/skl.c > @@ -336,9 +336,6 @@ static int skl_suspend(struct device *dev) > skl->skl_sst->fw_loaded = false; > } > > - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) > - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false); > - > return 0; > } > > @@ -350,10 +347,6 @@ static int skl_resume(struct device *dev) > struct hdac_ext_link *hlink = NULL; > int ret; > > - /* Turned OFF in HDMI codec driver after codec reconfiguration */ > - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) > - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true); > - > /* > * resume only when we are not in suspend active, otherwise need to > * restore the device
>-----Original Message----- >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] >Sent: Tuesday, January 8, 2019 12:58 AM >To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org; >tiwai@suse.de; broonie@kernel.org >Cc: liam.r.girdwood@linux.intel.com; Lin, Mengdong ><mengdong.lin@intel.com> >Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio >suspend/resume > > >On 1/6/19 8:22 PM, libin.yang@intel.com wrote: >> From: Libin Yang <libin.yang@intel.com> >> >> hdmi_codec_prepare() will trigger hdmi runtime resume, which will set >> the bitmask of hdev->addr. And skl_suspend() will clear the bitmask of >> HDA_CODEC_IDX_CONTROLLER. HDMI codec idx is not the same as >> HDA_CODEC_IDX_CONTROLLER, which means i915 power will not be >released >> when suspend. >> >> On the other hand, hdmi_codec_prepare() don't need to call >> pm_runtime_get_sync() to wake up the audio subsystem (HDMI auido) for >> setting the codec registers. Turning display power on with >> snd_hdac_display_power() is enough. >> >> Let's use S3 without playback as an example: >> hdmi_codec_prepare() invokes the runtime resume of codec => >> snd_hdac_display_power(bus, hdev->addr, true) skl runtime resume >> skl_suspend() => >> snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false); >> >> THis means hdev->addr will never release the display power when >> suspend. >> >> The new sequence will be: >> hdmi_codec_prepare() => >> snd_hdac_display_power(bus, hdev->addr, true) >> snd_hdac_display_power(bus, hdev->addr, false) skl runtime resume >> skl suspned > >I for one find this RFC difficult to follow. The documentation of "Power >management sequences" seems obsolete after Takashi's changes, you may >want to start with an update of the documentation which might help point out >what is broken in the sequences. I also don't see the point in trying to bypass >the runtime_pm code in codec_prepare and codec_complete. if we have the >display on/off sequence only in probe/remove and suspend/resume it makes >it easy to track states, and I wonder if it makes sense anyways to be in >suspend with the display on or vice-versa in D0 with the display off. Simple >state machines always win. Thanks for comments. I will modify the documentation of "Power Management sequences" when I submit the formal patch. The rough flow is described in my patch description. Do you think I need add more details? Actually, the HDMI driver is a little stranger. pm runtime resume of HDMI is not called even prepare() return 0. What I understand is that the runtime resume should be called. I asked a friend of mine who is familiar with kernel power management. He also can't tell why. Maybe our ALSA experts know why :). My patch is based on hdmi codec pm runtime resume not being called. The normal flow of suspend should be: Pm runtime resume => power up Pm suspend => power down. Unfortunately, our ASoC HDMI codec driver doesn't have pm suspend Callback (maybe this is why HDMI runtime resume is not called?) In original code, the hdmi prepare() will call pm_runtime_get_sync() which will trigger hdmi pm runtime resume, which will turn on display power: snd_hdac_display_power(hdev->bus, hdev->addr, true). And there is no chance to turn off the 'hdev->addr' display power before suspend, which is wrong. Regards, Libin > >> >> Signed-off-by: Libin Yang <libin.yang@intel.com> >> --- >> sound/soc/codecs/hdac_hdmi.c | 6 ++++-- >> sound/soc/intel/skylake/skl.c | 7 ------- >> 2 files changed, 4 insertions(+), 9 deletions(-) >> >> diff --git a/sound/soc/codecs/hdac_hdmi.c >> b/sound/soc/codecs/hdac_hdmi.c index 3ab2949..782b323 100644 >> --- a/sound/soc/codecs/hdac_hdmi.c >> +++ b/sound/soc/codecs/hdac_hdmi.c >> @@ -1895,7 +1895,7 @@ static int hdmi_codec_prepare(struct device *dev) >> { >> struct hdac_device *hdev = dev_to_hdac_dev(dev); >> >> - pm_runtime_get_sync(&hdev->dev); >> + snd_hdac_display_power(hdev->bus, hdev->addr, true); >> >> /* >> * Power down afg. >> @@ -1906,6 +1906,7 @@ static int hdmi_codec_prepare(struct device *dev) >> */ >> snd_hdac_codec_read(hdev, hdev->afg, 0, > AC_VERB_SET_POWER_STATE, >> AC_PWRST_D3); >> + snd_hdac_display_power(hdev->bus, hdev->addr, false); >> >> return 0; >> } >> @@ -1915,6 +1916,7 @@ static void hdmi_codec_complete(struct device >*dev) >> struct hdac_device *hdev = dev_to_hdac_dev(dev); >> struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); >> >> + snd_hdac_display_power(hdev->bus, hdev->addr, true); >> /* Power up afg */ >> snd_hdac_codec_read(hdev, hdev->afg, 0, > AC_VERB_SET_POWER_STATE, >> AC_PWRST_D0); >> @@ -1930,7 +1932,7 @@ static void hdmi_codec_complete(struct device >*dev) >> */ >> hdac_hdmi_present_sense_all_pins(hdev, hdmi, false); >> >> - pm_runtime_put_sync(&hdev->dev); >> + snd_hdac_display_power(hdev->bus, hdev->addr, false); >> } >> #else >> #define hdmi_codec_prepare NULL >> diff --git a/sound/soc/intel/skylake/skl.c >> b/sound/soc/intel/skylake/skl.c index 60c9483..89f4d66 100644 >> --- a/sound/soc/intel/skylake/skl.c >> +++ b/sound/soc/intel/skylake/skl.c >> @@ -336,9 +336,6 @@ static int skl_suspend(struct device *dev) >> skl->skl_sst->fw_loaded = false; >> } >> >> - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) >> - snd_hdac_display_power(bus, >HDA_CODEC_IDX_CONTROLLER, false); >> - >> return 0; >> } >> >> @@ -350,10 +347,6 @@ static int skl_resume(struct device *dev) >> struct hdac_ext_link *hlink = NULL; >> int ret; >> >> - /* Turned OFF in HDMI codec driver after codec reconfiguration */ >> - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) >> - snd_hdac_display_power(bus, >HDA_CODEC_IDX_CONTROLLER, true); >> - >> /* >> * resume only when we are not in suspend active, otherwise need to >> * restore the device
On Tue, 08 Jan 2019 08:53:49 +0100, Yang, Libin wrote: > > > >-----Original Message----- > >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] > >Sent: Tuesday, January 8, 2019 12:58 AM > >To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org; > >tiwai@suse.de; broonie@kernel.org > >Cc: liam.r.girdwood@linux.intel.com; Lin, Mengdong > ><mengdong.lin@intel.com> > >Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio > >suspend/resume > > > > > >On 1/6/19 8:22 PM, libin.yang@intel.com wrote: > >> From: Libin Yang <libin.yang@intel.com> > >> > >> hdmi_codec_prepare() will trigger hdmi runtime resume, which will set > >> the bitmask of hdev->addr. And skl_suspend() will clear the bitmask of > >> HDA_CODEC_IDX_CONTROLLER. HDMI codec idx is not the same as > >> HDA_CODEC_IDX_CONTROLLER, which means i915 power will not be > >released > >> when suspend. > >> > >> On the other hand, hdmi_codec_prepare() don't need to call > >> pm_runtime_get_sync() to wake up the audio subsystem (HDMI auido) for > >> setting the codec registers. Turning display power on with > >> snd_hdac_display_power() is enough. > >> > >> Let's use S3 without playback as an example: > >> hdmi_codec_prepare() invokes the runtime resume of codec => > >> snd_hdac_display_power(bus, hdev->addr, true) skl runtime resume > >> skl_suspend() => > >> snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false); > >> > >> THis means hdev->addr will never release the display power when > >> suspend. > >> > >> The new sequence will be: > >> hdmi_codec_prepare() => > >> snd_hdac_display_power(bus, hdev->addr, true) > >> snd_hdac_display_power(bus, hdev->addr, false) skl runtime resume > >> skl suspned > > > >I for one find this RFC difficult to follow. The documentation of "Power > >management sequences" seems obsolete after Takashi's changes, you may > >want to start with an update of the documentation which might help point out > >what is broken in the sequences. I also don't see the point in trying to bypass > >the runtime_pm code in codec_prepare and codec_complete. if we have the > >display on/off sequence only in probe/remove and suspend/resume it makes > >it easy to track states, and I wonder if it makes sense anyways to be in > >suspend with the display on or vice-versa in D0 with the display off. Simple > >state machines always win. > > Thanks for comments. I will modify the documentation of "Power > Management sequences" when I submit the formal patch. The rough > flow is described in my patch description. Do you think I need add more > details? > > Actually, the HDMI driver is a little stranger. pm runtime resume of HDMI > is not called even prepare() return 0. What I understand is that the runtime > resume should be called. I asked a friend of mine who is familiar with kernel > power management. He also can't tell why. Maybe our ALSA experts know > why :). > > My patch is based on hdmi codec pm runtime resume not being called. > > The normal flow of suspend should be: > Pm runtime resume => power up > Pm suspend => power down. > Unfortunately, our ASoC HDMI codec driver doesn't have pm suspend > Callback (maybe this is why HDMI runtime resume is not called?) > > In original code, the hdmi prepare() will call pm_runtime_get_sync() which > will trigger hdmi pm runtime resume, which will turn on display power: > snd_hdac_display_power(hdev->bus, hdev->addr, true). And > there is no chance to turn off the 'hdev->addr' display power before > suspend, which is wrong. Well, the problem is that HD-audio controller takes the display power unnecessarily at suspend and resume. Since the refactoring, this is superfluous and confuses the system. Also, I see no reason to stick with prepare and complete PM calls in hdac_hdmi driver; the display power is managed in each domain now, so we shouldn't rely on the refcount done by the controller driver any longer. Below is an untested patch I cooked up for simplification and fix for this issue. Could you check whether this works at all? thanks, Takashi -- 8< -- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH] ASoC: intel: skl: Fix display power regression Since the refactoring of HD-audio display power management, the display power status is managed per domain. Meanwhile the ASoC hdac_hdmi driver still keeps and relies (incorrectly) on the refcounting together with ASoC skl driver, and this leads to the display state always on. This patch is an attempt to address the regression by simplifying the PM code of ASoC skl and hdac_hdmi drivers. Basically, since the refactoring, we don't have to manage the display power at HD-audio controller suspend / resume but only at HD-audio HDMI codec suspend / resume. So the patch drops the superfluous snd_hdac_display_power() calls in skl driver. Meanwhile, in hdac_hdmi side, we rewrite the PM call just to re-use the runtime PM callbacks like other drivers do. Now the logic is simple: turn off at suspend and turn on at resume. The patch also fixes the possibly missing display-power off at skl driver removal as well as some error paths at probe. Fixes: 029d92c289bd ("ALSA: hda: Refactor display power management") Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/soc/codecs/hdac_hdmi.c | 112 +++------------------------------- sound/soc/intel/skylake/skl.c | 13 ++-- 2 files changed, 13 insertions(+), 112 deletions(-) diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 3ab2949c1dfa..cb4668b82882 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -1890,38 +1890,16 @@ static void hdmi_codec_remove(struct snd_soc_component *component) pm_runtime_disable(&hdev->dev); } -#ifdef CONFIG_PM -static int hdmi_codec_prepare(struct device *dev) -{ - struct hdac_device *hdev = dev_to_hdac_dev(dev); - - pm_runtime_get_sync(&hdev->dev); - - /* - * Power down afg. - * codec_read is preferred over codec_write to set the power state. - * This way verb is send to set the power state and response - * is received. So setting power state is ensured without using loop - * to read the state. - */ - snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE, - AC_PWRST_D3); - - return 0; -} - -static void hdmi_codec_complete(struct device *dev) +#ifdef CONFIG_PM_SLEEP +static int hdmi_codec_resume(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); + int ret; - /* Power up afg */ - snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE, - AC_PWRST_D0); - - hdac_hdmi_skl_enable_all_pins(hdev); - hdac_hdmi_skl_enable_dp12(hdev); - + ret = pm_runtime_force_resume(dev); + if (ret < 0) + return ret; /* * As the ELD notify callback request is not entertained while the * device is in suspend state. Need to manually check detection of @@ -1929,12 +1907,10 @@ static void hdmi_codec_complete(struct device *dev) * already set pin caps. */ hdac_hdmi_present_sense_all_pins(hdev, hdmi, false); - - pm_runtime_put_sync(&hdev->dev); + return 0; } #else -#define hdmi_codec_prepare NULL -#define hdmi_codec_complete NULL +#define hdmi_codec_resume NULL #endif static const struct snd_soc_component_driver hdmi_hda_codec = { @@ -2135,75 +2111,6 @@ static int hdac_hdmi_dev_remove(struct hdac_device *hdev) } #ifdef CONFIG_PM -/* - * Power management sequences - * ========================== - * - * The following explains the PM handling of HDAC HDMI with its parent - * device SKL and display power usage - * - * Probe - * ----- - * In SKL probe, - * 1. skl_probe_work() powers up the display (refcount++ -> 1) - * 2. enumerates the codecs on the link - * 3. powers down the display (refcount-- -> 0) - * - * In HDAC HDMI probe, - * 1. hdac_hdmi_dev_probe() powers up the display (refcount++ -> 1) - * 2. probe the codec - * 3. put the HDAC HDMI device to runtime suspend - * 4. hdac_hdmi_runtime_suspend() powers down the display (refcount-- -> 0) - * - * Once children are runtime suspended, SKL device also goes to runtime - * suspend - * - * HDMI Playback - * ------------- - * Open HDMI device, - * 1. skl_runtime_resume() invoked - * 2. hdac_hdmi_runtime_resume() powers up the display (refcount++ -> 1) - * - * Close HDMI device, - * 1. hdac_hdmi_runtime_suspend() powers down the display (refcount-- -> 0) - * 2. skl_runtime_suspend() invoked - * - * S0/S3 Cycle with playback in progress - * ------------------------------------- - * When the device is opened for playback, the device is runtime active - * already and the display refcount is 1 as explained above. - * - * Entering to S3, - * 1. hdmi_codec_prepare() invoke the runtime resume of codec which just - * increments the PM runtime usage count of the codec since the device - * is in use already - * 2. skl_suspend() powers down the display (refcount-- -> 0) - * - * Wakeup from S3, - * 1. skl_resume() powers up the display (refcount++ -> 1) - * 2. hdmi_codec_complete() invokes the runtime suspend of codec which just - * decrements the PM runtime usage count of the codec since the device - * is in use already - * - * Once playback is stopped, the display refcount is set to 0 as explained - * above in the HDMI playback sequence. The PM handlings are designed in - * such way that to balance the refcount of display power when the codec - * device put to S3 while playback is going on. - * - * S0/S3 Cycle without playback in progress - * ---------------------------------------- - * Entering to S3, - * 1. hdmi_codec_prepare() invoke the runtime resume of codec - * 2. skl_runtime_resume() invoked - * 3. hdac_hdmi_runtime_resume() powers up the display (refcount++ -> 1) - * 4. skl_suspend() powers down the display (refcount-- -> 0) - * - * Wakeup from S3, - * 1. skl_resume() powers up the display (refcount++ -> 1) - * 2. hdmi_codec_complete() invokes the runtime suspend of codec - * 3. hdac_hdmi_runtime_suspend() powers down the display (refcount-- -> 0) - * 4. skl_runtime_suspend() invoked - */ static int hdac_hdmi_runtime_suspend(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); @@ -2277,8 +2184,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev) static const struct dev_pm_ops hdac_hdmi_pm = { SET_RUNTIME_PM_OPS(hdac_hdmi_runtime_suspend, hdac_hdmi_runtime_resume, NULL) - .prepare = hdmi_codec_prepare, - .complete = hdmi_codec_complete, + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, hdmi_codec_resume) }; static const struct hda_device_id hdmi_list[] = { diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 60c94836bf5b..4ed5b7e17d44 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -336,9 +336,6 @@ static int skl_suspend(struct device *dev) skl->skl_sst->fw_loaded = false; } - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false); - return 0; } @@ -350,10 +347,6 @@ static int skl_resume(struct device *dev) struct hdac_ext_link *hlink = NULL; int ret; - /* Turned OFF in HDMI codec driver after codec reconfiguration */ - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true); - /* * resume only when we are not in suspend active, otherwise need to * restore the device @@ -446,8 +439,10 @@ static int skl_free(struct hdac_bus *bus) snd_hdac_ext_bus_exit(bus); cancel_work_sync(&skl->probe_work); - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) + if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) { + snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false); snd_hdac_i915_exit(bus); + } return 0; } @@ -814,7 +809,7 @@ static void skl_probe_work(struct work_struct *work) err = skl_platform_register(bus->dev); if (err < 0) { dev_err(bus->dev, "platform register failed: %d\n", err); - return; + goto out_err; } err = skl_machine_device_register(skl);
Hi Takashi, >-----Original Message----- >From: Takashi Iwai [mailto:tiwai@suse.de] >Sent: Tuesday, January 8, 2019 7:15 PM >To: Yang, Libin <libin.yang@intel.com> >Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; alsa- >devel@alsa-project.org; broonie@kernel.org; >liam.r.girdwood@linux.intel.com; Lin, Mengdong <mengdong.lin@intel.com> >Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio >suspend/resume > >On Tue, 08 Jan 2019 08:53:49 +0100, >Yang, Libin wrote: >> >> >> >-----Original Message----- >> >From: Pierre-Louis Bossart >> >[mailto:pierre-louis.bossart@linux.intel.com] >> >Sent: Tuesday, January 8, 2019 12:58 AM >> >To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org; >> >tiwai@suse.de; broonie@kernel.org >> >Cc: liam.r.girdwood@linux.intel.com; Lin, Mengdong >> ><mengdong.lin@intel.com> >> >Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi >> >audio suspend/resume >> > >> > >> >On 1/6/19 8:22 PM, libin.yang@intel.com wrote: >> >> From: Libin Yang <libin.yang@intel.com> >> >> >> >> hdmi_codec_prepare() will trigger hdmi runtime resume, which will >> >> set the bitmask of hdev->addr. And skl_suspend() will clear the >> >> bitmask of HDA_CODEC_IDX_CONTROLLER. HDMI codec idx is not the >same >> >> as HDA_CODEC_IDX_CONTROLLER, which means i915 power will not be >> >released >> >> when suspend. >> >> >> >> On the other hand, hdmi_codec_prepare() don't need to call >> >> pm_runtime_get_sync() to wake up the audio subsystem (HDMI auido) >> >> for setting the codec registers. Turning display power on with >> >> snd_hdac_display_power() is enough. >> >> >> >> Let's use S3 without playback as an example: >> >> hdmi_codec_prepare() invokes the runtime resume of codec => >> >> snd_hdac_display_power(bus, hdev->addr, true) skl runtime resume >> >> skl_suspend() => >> >> snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false); >> >> >> >> THis means hdev->addr will never release the display power when >> >> suspend. >> >> >> >> The new sequence will be: >> >> hdmi_codec_prepare() => >> >> snd_hdac_display_power(bus, hdev->addr, true) >> >> snd_hdac_display_power(bus, hdev->addr, false) skl runtime >> >> resume skl suspned >> > >> >I for one find this RFC difficult to follow. The documentation of >> >"Power management sequences" seems obsolete after Takashi's changes, >> >you may want to start with an update of the documentation which might >> >help point out what is broken in the sequences. I also don't see the >> >point in trying to bypass the runtime_pm code in codec_prepare and >> >codec_complete. if we have the display on/off sequence only in >> >probe/remove and suspend/resume it makes it easy to track states, and >> >I wonder if it makes sense anyways to be in suspend with the display >> >on or vice-versa in D0 with the display off. Simple state machines always >win. >> >> Thanks for comments. I will modify the documentation of "Power >> Management sequences" when I submit the formal patch. The rough flow >> is described in my patch description. Do you think I need add more >> details? >> >> Actually, the HDMI driver is a little stranger. pm runtime resume of >> HDMI is not called even prepare() return 0. What I understand is that >> the runtime resume should be called. I asked a friend of mine who is >> familiar with kernel power management. He also can't tell why. Maybe >> our ALSA experts know why :). >> >> My patch is based on hdmi codec pm runtime resume not being called. >> >> The normal flow of suspend should be: >> Pm runtime resume => power up >> Pm suspend => power down. >> Unfortunately, our ASoC HDMI codec driver doesn't have pm suspend >> Callback (maybe this is why HDMI runtime resume is not called?) >> >> In original code, the hdmi prepare() will call pm_runtime_get_sync() >> which will trigger hdmi pm runtime resume, which will turn on display >power: >> snd_hdac_display_power(hdev->bus, hdev->addr, true). And there is no >> chance to turn off the 'hdev->addr' display power before suspend, >> which is wrong. > >Well, the problem is that HD-audio controller takes the display power >unnecessarily at suspend and resume. Since the refactoring, this is >superfluous and confuses the system. > >Also, I see no reason to stick with prepare and complete PM calls in >hdac_hdmi driver; the display power is managed in each domain now, so we >shouldn't rely on the refcount done by the controller driver any longer. > >Below is an untested patch I cooked up for simplification and fix for this issue. >Could you check whether this works at all? This patch makes the flow more clear now. Thanks for help. I have done the test on sof audio driver. Because SOF has issue of suspend when playback, I didn't do the test of suspend with hdmi playing. I only test the audio suspend/resume without playing anything. The test result is: Suspend/resume works well. And after suspend/resume, playback works well. However, I don't have platform to test SST driver. I tried to setup the environment for SST driver test, but failed. I will continue to setup the SST environment. On the meantime, I will ask Intel internal team to help on SST driver test if they are willing to help. And please see my comments below. [...] >- >-static void hdmi_codec_complete(struct device *dev) >+#ifdef CONFIG_PM_SLEEP >+static int hdmi_codec_resume(struct device *dev) > { > struct hdac_device *hdev = dev_to_hdac_dev(dev); > struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); >+ int ret; > >- /* Power up afg */ >- snd_hdac_codec_read(hdev, hdev->afg, 0, > AC_VERB_SET_POWER_STATE, >- AC_PWRST_D0); >- >- hdac_hdmi_skl_enable_all_pins(hdev); >- hdac_hdmi_skl_enable_dp12(hdev); >- >+ ret = pm_runtime_force_resume(dev); The code hopes to call hdmi pm_runtime() whenever this resume() is called? If so, I'm afraid it will not work. pm_runtime_force_resume() calls pm_runtime with conditions. Sometimes pm_runtime will not be called. In my suspend/resume test, it shows that pm_runtime() is not called. This may cause the following function hdac_hdmi_present_sense_all_pins() not work well as there is no power on. Regards, Libin >+ if (ret < 0) >+ return ret; > /* > * As the ELD notify callback request is not entertained while the > * device is in suspend state. Need to manually check detection of @@
On Wed, 09 Jan 2019 09:16:34 +0100, Yang, Libin wrote: > > >- > >-static void hdmi_codec_complete(struct device *dev) > >+#ifdef CONFIG_PM_SLEEP > >+static int hdmi_codec_resume(struct device *dev) > > { > > struct hdac_device *hdev = dev_to_hdac_dev(dev); > > struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); > >+ int ret; > > > >- /* Power up afg */ > >- snd_hdac_codec_read(hdev, hdev->afg, 0, > > AC_VERB_SET_POWER_STATE, > >- AC_PWRST_D0); > >- > >- hdac_hdmi_skl_enable_all_pins(hdev); > >- hdac_hdmi_skl_enable_dp12(hdev); > >- > >+ ret = pm_runtime_force_resume(dev); > > The code hopes to call hdmi pm_runtime() whenever this resume() > is called? If so, I'm afraid it will not work. pm_runtime_force_resume() > calls pm_runtime with conditions. Sometimes pm_runtime will not > be called. In my suspend/resume test, it shows that pm_runtime() is > not called. This may cause the following function > hdac_hdmi_present_sense_all_pins() not work well as there is no > power on. Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins() into runtime resume. It may be called unconditionally, but safer to call it conditionally only after S3 or so... thanks, Takashi
On Wed, 09 Jan 2019 09:29:36 +0100, Takashi Iwai wrote: > > On Wed, 09 Jan 2019 09:16:34 +0100, > Yang, Libin wrote: > > > > >- > > >-static void hdmi_codec_complete(struct device *dev) > > >+#ifdef CONFIG_PM_SLEEP > > >+static int hdmi_codec_resume(struct device *dev) > > > { > > > struct hdac_device *hdev = dev_to_hdac_dev(dev); > > > struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); > > >+ int ret; > > > > > >- /* Power up afg */ > > >- snd_hdac_codec_read(hdev, hdev->afg, 0, > > > AC_VERB_SET_POWER_STATE, > > >- AC_PWRST_D0); > > >- > > >- hdac_hdmi_skl_enable_all_pins(hdev); > > >- hdac_hdmi_skl_enable_dp12(hdev); > > >- > > >+ ret = pm_runtime_force_resume(dev); > > > > The code hopes to call hdmi pm_runtime() whenever this resume() > > is called? If so, I'm afraid it will not work. pm_runtime_force_resume() > > calls pm_runtime with conditions. Sometimes pm_runtime will not > > be called. In my suspend/resume test, it shows that pm_runtime() is > > not called. This may cause the following function > > hdac_hdmi_present_sense_all_pins() not work well as there is no > > power on. > > Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins() > into runtime resume. It may be called unconditionally, but safer to > call it conditionally only after S3 or so... Now reading back the code again, no, it's correct to call hdac_hdmi_present_sense_all_pins() there. The function doesn't access any HD-audio codec verbs but only with the i915 audio component. Hence it's safe to call there no matter whether the device is opened (thus resumed) or not. And, it is even mandatory to call there; if a HDMI connection changed during suspend, we need to notify it right after the system resume. thanks, Takashi
>-----Original Message----- >From: Takashi Iwai [mailto:tiwai@suse.de] >Sent: Wednesday, January 9, 2019 5:14 PM >To: Yang, Libin <libin.yang@intel.com> >Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; alsa- >devel@alsa-project.org; broonie@kernel.org; >liam.r.girdwood@linux.intel.com; Lin, Mengdong <mengdong.lin@intel.com> >Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio >suspend/resume > >On Wed, 09 Jan 2019 09:29:36 +0100, >Takashi Iwai wrote: >> >> On Wed, 09 Jan 2019 09:16:34 +0100, >> Yang, Libin wrote: >> > >> > >- >> > >-static void hdmi_codec_complete(struct device *dev) >> > >+#ifdef CONFIG_PM_SLEEP >> > >+static int hdmi_codec_resume(struct device *dev) >> > > { >> > > struct hdac_device *hdev = dev_to_hdac_dev(dev); >> > > struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); >> > >+ int ret; >> > > >> > >- /* Power up afg */ >> > >- snd_hdac_codec_read(hdev, hdev->afg, 0, >> > > AC_VERB_SET_POWER_STATE, >> > >- AC_PWRST_D0); >> > >- >> > >- hdac_hdmi_skl_enable_all_pins(hdev); >> > >- hdac_hdmi_skl_enable_dp12(hdev); >> > >- >> > >+ ret = pm_runtime_force_resume(dev); >> > >> > The code hopes to call hdmi pm_runtime() whenever this resume() is >> > called? If so, I'm afraid it will not work. >> > pm_runtime_force_resume() calls pm_runtime with conditions. >> > Sometimes pm_runtime will not be called. In my suspend/resume test, >> > it shows that pm_runtime() is not called. This may cause the >> > following function >> > hdac_hdmi_present_sense_all_pins() not work well as there is no >> > power on. >> >> Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins() >> into runtime resume. It may be called unconditionally, but safer to >> call it conditionally only after S3 or so... > >Now reading back the code again, no, it's correct to call >hdac_hdmi_present_sense_all_pins() there. The function doesn't access any >HD-audio codec verbs but only with the i915 audio component. >Hence it's safe to call there no matter whether the device is opened (thus >resumed) or not. And, it is even mandatory to call there; if a HDMI connection >changed during suspend, we need to notify it right after the system resume. Yes. It seems this is safe. Thanks for clarification. SOF audio driver test passes. No reply from our internal SST driver team. From SOF test, this patch can fix the display power issue. BTW: I have another question. Maybe it is not related to this topic, but related the general audio power sequence. I'm not sure my understand is correct or not. Currently, codec device's parent is the card device, right? If so, the suspend sequence is: codec power off (child) card power off (parent) However, I see some cards will call trigger for suspend in card suspend function. This may cause issue logically (Maybe the real cards are always OK). The codec has already powered off. So if trigger() operates with codec may cause issue? Even, if the codec is the master, maybe the clk is off before trigger() is called. I'm not sure this is always OK. (This is why I submitted the second patch.) Regards, Libin
On Thu, 10 Jan 2019 06:30:14 +0100, Yang, Libin wrote: > > > >-----Original Message----- > >From: Takashi Iwai [mailto:tiwai@suse.de] > >Sent: Wednesday, January 9, 2019 5:14 PM > >To: Yang, Libin <libin.yang@intel.com> > >Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; alsa- > >devel@alsa-project.org; broonie@kernel.org; > >liam.r.girdwood@linux.intel.com; Lin, Mengdong <mengdong.lin@intel.com> > >Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio > >suspend/resume > > > >On Wed, 09 Jan 2019 09:29:36 +0100, > >Takashi Iwai wrote: > >> > >> On Wed, 09 Jan 2019 09:16:34 +0100, > >> Yang, Libin wrote: > >> > > >> > >- > >> > >-static void hdmi_codec_complete(struct device *dev) > >> > >+#ifdef CONFIG_PM_SLEEP > >> > >+static int hdmi_codec_resume(struct device *dev) > >> > > { > >> > > struct hdac_device *hdev = dev_to_hdac_dev(dev); > >> > > struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); > >> > >+ int ret; > >> > > > >> > >- /* Power up afg */ > >> > >- snd_hdac_codec_read(hdev, hdev->afg, 0, > >> > > AC_VERB_SET_POWER_STATE, > >> > >- AC_PWRST_D0); > >> > >- > >> > >- hdac_hdmi_skl_enable_all_pins(hdev); > >> > >- hdac_hdmi_skl_enable_dp12(hdev); > >> > >- > >> > >+ ret = pm_runtime_force_resume(dev); > >> > > >> > The code hopes to call hdmi pm_runtime() whenever this resume() is > >> > called? If so, I'm afraid it will not work. > >> > pm_runtime_force_resume() calls pm_runtime with conditions. > >> > Sometimes pm_runtime will not be called. In my suspend/resume test, > >> > it shows that pm_runtime() is not called. This may cause the > >> > following function > >> > hdac_hdmi_present_sense_all_pins() not work well as there is no > >> > power on. > >> > >> Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins() > >> into runtime resume. It may be called unconditionally, but safer to > >> call it conditionally only after S3 or so... > > > >Now reading back the code again, no, it's correct to call > >hdac_hdmi_present_sense_all_pins() there. The function doesn't access any > >HD-audio codec verbs but only with the i915 audio component. > >Hence it's safe to call there no matter whether the device is opened (thus > >resumed) or not. And, it is even mandatory to call there; if a HDMI connection > >changed during suspend, we need to notify it right after the system resume. > > Yes. It seems this is safe. Thanks for clarification. SOF audio driver test passes. > No reply from our internal SST driver team. From SOF test, this patch can fix > the display power issue. OK, I'm going to queue this, but will delay the upstream merge for 5.0-rc3. > BTW: I have another question. Maybe it is not related to this topic, but > related the general audio power sequence. I'm not sure my understand > is correct or not. > Currently, codec device's parent is the card device, right? If so, the suspend > sequence is: > codec power off (child) > card power off (parent) > > However, I see some cards will call trigger for suspend in card suspend > function. This may cause issue logically (Maybe the real cards are always > OK). The codec has already powered off. So if trigger() operates with > codec may cause issue? Even, if the codec is the master, maybe the clk > is off before trigger() is called. I'm not sure this is always OK. > (This is why I submitted the second patch.) Indeed this looks like a problem, and it's specific to ASoC. The legacy HD-audio codec driver has the base resume code that calls snd_pcm_suspend_all() for the assigned PCM streams, while ASoC calls it in snd_soc_suspend() that is invoked from the machine driver in general. If a codec driver has a strong dependency with the PCM state (like HD-audio one), it'd need to stop in its suspend callback, something like below. thanks, Takashi --- diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index b19d7a3e7a2c..caf512d807f5 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -106,6 +106,7 @@ struct hdac_hdmi_pcm { struct list_head port_list; struct hdac_hdmi_cvt *cvt; struct snd_soc_jack *jack; + struct snd_pcm *snd_pcm; int stream_tag; int channels; int format; @@ -1792,6 +1793,7 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int device, mutex_init(&pcm->lock); INIT_LIST_HEAD(&pcm->port_list); snd_pcm = hdac_hdmi_get_pcm_from_id(dai->component->card, device); + pcm->snd_pcm = snd_pcm; if (snd_pcm) { err = snd_hdac_add_chmap_ctls(snd_pcm, device, &hdmi->chmap); if (err < 0) { @@ -2118,8 +2120,10 @@ static int hdac_hdmi_dev_remove(struct hdac_device *hdev) static int hdac_hdmi_runtime_suspend(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); + struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_bus *bus = hdev->bus; struct hdac_ext_link *hlink = NULL; + struct hdac_hdmi_pcm *pcm; dev_dbg(dev, "Enter: %s\n", __func__); @@ -2127,6 +2131,12 @@ static int hdac_hdmi_runtime_suspend(struct device *dev) if (!bus) return 0; + /* make sure all streams suspended before power down */ + list_for_each_entry(pcm, &hdmi->pcm_list, head) { + if (pcm->snd_pcm) + snd_pcm_suspend_all(pcm->snd_pcm); + } + /* * Power down afg. * codec_read is preferred over codec_write to set the power state.
>> >On Wed, 09 Jan 2019 09:29:36 +0100, >> >Takashi Iwai wrote: >> >> >> >> On Wed, 09 Jan 2019 09:16:34 +0100, Yang, Libin wrote: >> >> > >> >> > >- >> >> > >-static void hdmi_codec_complete(struct device *dev) >> >> > >+#ifdef CONFIG_PM_SLEEP >> >> > >+static int hdmi_codec_resume(struct device *dev) >> >> > > { >> >> > > struct hdac_device *hdev = dev_to_hdac_dev(dev); >> >> > > struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); >> >> > >+ int ret; >> >> > > >> >> > >- /* Power up afg */ >> >> > >- snd_hdac_codec_read(hdev, hdev->afg, 0, >> >> > > AC_VERB_SET_POWER_STATE, >> >> > >- > AC_PWRST_D0); >> >> > >- >> >> > >- hdac_hdmi_skl_enable_all_pins(hdev); >> >> > >- hdac_hdmi_skl_enable_dp12(hdev); >> >> > >- >> >> > >+ ret = pm_runtime_force_resume(dev); >> >> > >> >> > The code hopes to call hdmi pm_runtime() whenever this resume() >> >> > is called? If so, I'm afraid it will not work. >> >> > pm_runtime_force_resume() calls pm_runtime with conditions. >> >> > Sometimes pm_runtime will not be called. In my suspend/resume >> >> > test, it shows that pm_runtime() is not called. This may cause >> >> > the following function >> >> > hdac_hdmi_present_sense_all_pins() not work well as there is no >> >> > power on. >> >> >> >> Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins() >> >> into runtime resume. It may be called unconditionally, but safer >> >> to call it conditionally only after S3 or so... >> > >> >Now reading back the code again, no, it's correct to call >> >hdac_hdmi_present_sense_all_pins() there. The function doesn't >> >access any HD-audio codec verbs but only with the i915 audio component. >> >Hence it's safe to call there no matter whether the device is opened >> >(thus >> >resumed) or not. And, it is even mandatory to call there; if a HDMI >> >connection changed during suspend, we need to notify it right after the >system resume. >> >> Yes. It seems this is safe. Thanks for clarification. SOF audio driver test >passes. >> No reply from our internal SST driver team. From SOF test, this patch >> can fix the display power issue. > >OK, I'm going to queue this, but will delay the upstream merge for 5.0-rc3. That great! Thanks. > >> BTW: I have another question. Maybe it is not related to this topic, >> but related the general audio power sequence. I'm not sure my >> understand is correct or not. >> Currently, codec device's parent is the card device, right? If so, the >> suspend sequence is: >> codec power off (child) >> card power off (parent) >> >> However, I see some cards will call trigger for suspend in card >> suspend function. This may cause issue logically (Maybe the real cards >> are always OK). The codec has already powered off. So if trigger() >> operates with codec may cause issue? Even, if the codec is the master, >> maybe the clk is off before trigger() is called. I'm not sure this is always OK. >> (This is why I submitted the second patch.) > >Indeed this looks like a problem, and it's specific to ASoC. >The legacy HD-audio codec driver has the base resume code that calls >snd_pcm_suspend_all() for the assigned PCM streams, while ASoC calls it in >snd_soc_suspend() that is invoked from the machine driver in general. > >If a codec driver has a strong dependency with the PCM state (like HD-audio >one), it'd need to stop in its suspend callback, something like below. Yes, below patch should be able to fix my concern. I will test it in HDMI audio suspend/resume with playback after I fix another issue of hdmi audio suspend/resume of SOF. The below patch may have a small confliction that the trigger will be called twice as our SOF has already call snd_pcm_suspend() in card suspend. Anyway, this is a SOF specific issue. I will work on it later. Regards, Libin > > >thanks, > >Takashi > >--- >diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c >index b19d7a3e7a2c..caf512d807f5 100644 >--- a/sound/soc/codecs/hdac_hdmi.c >+++ b/sound/soc/codecs/hdac_hdmi.c >@@ -106,6 +106,7 @@ struct hdac_hdmi_pcm { > struct list_head port_list; > struct hdac_hdmi_cvt *cvt; > struct snd_soc_jack *jack; >+ struct snd_pcm *snd_pcm; > int stream_tag; > int channels; > int format; >@@ -1792,6 +1793,7 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int >device, > mutex_init(&pcm->lock); > INIT_LIST_HEAD(&pcm->port_list); > snd_pcm = hdac_hdmi_get_pcm_from_id(dai->component->card, >device); >+ pcm->snd_pcm = snd_pcm; > if (snd_pcm) { > err = snd_hdac_add_chmap_ctls(snd_pcm, device, &hdmi- >>chmap); > if (err < 0) { >@@ -2118,8 +2120,10 @@ static int hdac_hdmi_dev_remove(struct >hdac_device *hdev) static int hdac_hdmi_runtime_suspend(struct device >*dev) { > struct hdac_device *hdev = dev_to_hdac_dev(dev); >+ struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); > struct hdac_bus *bus = hdev->bus; > struct hdac_ext_link *hlink = NULL; >+ struct hdac_hdmi_pcm *pcm; > > dev_dbg(dev, "Enter: %s\n", __func__); > >@@ -2127,6 +2131,12 @@ static int hdac_hdmi_runtime_suspend(struct >device *dev) > if (!bus) > return 0; > >+ /* make sure all streams suspended before power down */ >+ list_for_each_entry(pcm, &hdmi->pcm_list, head) { >+ if (pcm->snd_pcm) >+ snd_pcm_suspend_all(pcm->snd_pcm); >+ } >+ > /* > * Power down afg. > * codec_read is preferred over codec_write to set the power state. > > >_______________________________________________ >Alsa-devel mailing list >Alsa-devel@alsa-project.org >http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Fri, 11 Jan 2019 06:20:23 +0100, Yang, Libin wrote: > > The below patch may have a small confliction that the trigger will be > called twice as our SOF has already call snd_pcm_suspend() in card > suspend. It should be no problem, snd_pcm_suspend() can be called multiple times. If it's already suspended, just nothing happens. thanks, Takashi
On Fri, 11 Jan 2019 14:34:34 +0100, Takashi Iwai wrote: > > On Fri, 11 Jan 2019 06:20:23 +0100, > Yang, Libin wrote: > > > > The below patch may have a small confliction that the trigger will be > > called twice as our SOF has already call snd_pcm_suspend() in card > > suspend. > > It should be no problem, snd_pcm_suspend() can be called multiple > times. If it's already suspended, just nothing happens. Thinking of this problem again, does a patch like below work instead? This looks like a better and more generic solution. What I'm not quite sure is whether the device suspend order between PCM device and HD-audio codec device is guaranteed. I guess yes, because the PCM device is registered always after the codec. But ASoC might have another weirdness :) If this patch works, basically we can get rid of all external callers of snd_pcm_suspend() & co. That'll be a nice cleanup. thanks, Takashi --- diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 01b9d62eef14..d8d57336b8b8 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -683,6 +683,25 @@ static inline int snd_pcm_substream_proc_done(struct snd_pcm_substream *substrea static const struct attribute_group *pcm_dev_attr_groups[]; +#ifdef CONFIG_PM_SLEEP +static int do_pcm_suspend(struct device *dev) +{ + struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev); + + snd_pcm_suspend_all(pstr->pcm); + return 0; +} +#endif + +static const struct dev_pm_ops pcm_dev_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL) +}; + +static const struct device_type pcm_dev_type = { + .name = "pcm", + .pm = &pcm_dev_pm_ops, +}; + /** * snd_pcm_new_stream - create a new PCM stream * @pcm: the pcm instance @@ -713,6 +732,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) snd_device_initialize(&pstr->dev, pcm->card); pstr->dev.groups = pcm_dev_attr_groups; + pstr->dev.type = &pcm_dev_type; dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device, stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
>-----Original Message----- >From: Takashi Iwai [mailto:tiwai@suse.de] >Sent: Friday, January 11, 2019 9:35 PM >To: Yang, Libin <libin.yang@intel.com> >Cc: liam.r.girdwood@linux.intel.com; Lin, Mengdong ><mengdong.lin@intel.com>; alsa-devel@alsa-project.org; broonie@kernel.org; >Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio >suspend/resume > >On Fri, 11 Jan 2019 06:20:23 +0100, >Yang, Libin wrote: >> >> The below patch may have a small confliction that the trigger will be >> called twice as our SOF has already call snd_pcm_suspend() in card >> suspend. > >It should be no problem, snd_pcm_suspend() can be called multiple times. If >it's already suspended, just nothing happens. Got it. Thanks. Regards, Libin > > >thanks, > >Takashi
On Fri, 11 Jan 2019 15:24:30 +0100, Takashi Iwai wrote: > > On Fri, 11 Jan 2019 14:34:34 +0100, > Takashi Iwai wrote: > > > > On Fri, 11 Jan 2019 06:20:23 +0100, > > Yang, Libin wrote: > > > > > > The below patch may have a small confliction that the trigger will be > > > called twice as our SOF has already call snd_pcm_suspend() in card > > > suspend. > > > > It should be no problem, snd_pcm_suspend() can be called multiple > > times. If it's already suspended, just nothing happens. > > Thinking of this problem again, does a patch like below work instead? > This looks like a better and more generic solution. > > What I'm not quite sure is whether the device suspend order between > PCM device and HD-audio codec device is guaranteed. I guess yes, > because the PCM device is registered always after the codec. But ASoC > might have another weirdness :) ... and further reading revealed that my afraid is right -- ASoC is another beast. Basically PM in ASoC requires *not* to be done by the usual device PM callbacks. snd_soc_suspend() does the whole suspend jobs sequentially. So, what you need for Intel ASoC driver is to do suspend/resume with the component callbacks instead of device PM ops. Currently the plumbing of hdac-hdmi driver is in a half-baked state. > If this patch works, basically we can get rid of all external callers > of snd_pcm_suspend() & co. That'll be a nice cleanup. OTOH, the statement above is still true for non-ASoC drivers. So we'd end up needing an extra flag or such to prevent the PCM device PM ops for ASoC, while using this for the rest. thanks, Takashi > > > thanks, > > Takashi > > --- > diff --git a/sound/core/pcm.c b/sound/core/pcm.c > index 01b9d62eef14..d8d57336b8b8 100644 > --- a/sound/core/pcm.c > +++ b/sound/core/pcm.c > @@ -683,6 +683,25 @@ static inline int snd_pcm_substream_proc_done(struct snd_pcm_substream *substrea > > static const struct attribute_group *pcm_dev_attr_groups[]; > > +#ifdef CONFIG_PM_SLEEP > +static int do_pcm_suspend(struct device *dev) > +{ > + struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev); > + > + snd_pcm_suspend_all(pstr->pcm); > + return 0; > +} > +#endif > + > +static const struct dev_pm_ops pcm_dev_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL) > +}; > + > +static const struct device_type pcm_dev_type = { > + .name = "pcm", > + .pm = &pcm_dev_pm_ops, > +}; > + > /** > * snd_pcm_new_stream - create a new PCM stream > * @pcm: the pcm instance > @@ -713,6 +732,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) > > snd_device_initialize(&pstr->dev, pcm->card); > pstr->dev.groups = pcm_dev_attr_groups; > + pstr->dev.type = &pcm_dev_type; > dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device, > stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c'); >
Hi Takashi, >> > >> > On Fri, 11 Jan 2019 06:20:23 +0100, >> > Yang, Libin wrote: >> > > >> > > The below patch may have a small confliction that the trigger will >> > > be called twice as our SOF has already call snd_pcm_suspend() in >> > > card suspend. >> > >> > It should be no problem, snd_pcm_suspend() can be called multiple >> > times. If it's already suspended, just nothing happens. >> >> Thinking of this problem again, does a patch like below work instead? >> This looks like a better and more generic solution. >> >> What I'm not quite sure is whether the device suspend order between >> PCM device and HD-audio codec device is guaranteed. I guess yes, >> because the PCM device is registered always after the codec. But ASoC >> might have another weirdness :) The suspend order is always a quite hard issue for me. I have to spend a lot time checking the parent-child relationship. And if they don't have such relationship, I don't know how to find the order. My initial idea to get rid of such dependency is: do the pcm suspend before suspend(), for example put it in prepare(). And set device clk off and power off in suspend(). This can make sure pcm is always properly suspended. But, I'm not sure prepare() is a right place because it seems prepare() is not designed to do such things and prepare() may impact the suspend/resume sequence based on its return value. If your below patch works, I think it will be a best solution which can handle such things in ALSA level. I think we may need to do a lot of test because the different cards drivers are extremely different. Regards, Libin > >... and further reading revealed that my afraid is right -- ASoC is another beast. >Basically PM in ASoC requires *not* to be done by the usual device PM >callbacks. snd_soc_suspend() does the whole suspend jobs sequentially. So, >what you need for Intel ASoC driver is to do suspend/resume with the >component callbacks instead of device PM ops. >Currently the plumbing of hdac-hdmi driver is in a half-baked state. > >> If this patch works, basically we can get rid of all external callers >> of snd_pcm_suspend() & co. That'll be a nice cleanup. > >OTOH, the statement above is still true for non-ASoC drivers. So we'd end up >needing an extra flag or such to prevent the PCM device PM ops for ASoC, >while using this for the rest. > > >thanks, > >Takashi > >> >> >> thanks, >> >> Takashi >> >> --- >> diff --git a/sound/core/pcm.c b/sound/core/pcm.c index >> 01b9d62eef14..d8d57336b8b8 100644 >> --- a/sound/core/pcm.c >> +++ b/sound/core/pcm.c >> @@ -683,6 +683,25 @@ static inline int >> snd_pcm_substream_proc_done(struct snd_pcm_substream *substrea >> >> static const struct attribute_group *pcm_dev_attr_groups[]; >> >> +#ifdef CONFIG_PM_SLEEP >> +static int do_pcm_suspend(struct device *dev) { >> + struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, >> +dev); >> + >> + snd_pcm_suspend_all(pstr->pcm); >> + return 0; >> +} >> +#endif >> + >> +static const struct dev_pm_ops pcm_dev_pm_ops = { >> + SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL) }; >> + >> +static const struct device_type pcm_dev_type = { >> + .name = "pcm", >> + .pm = &pcm_dev_pm_ops, >> +}; >> + >> /** >> * snd_pcm_new_stream - create a new PCM stream >> * @pcm: the pcm instance >> @@ -713,6 +732,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int >> stream, int substream_count) >> >> snd_device_initialize(&pstr->dev, pcm->card); >> pstr->dev.groups = pcm_dev_attr_groups; >> + pstr->dev.type = &pcm_dev_type; >> dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, >pcm->device, >> stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c'); >>
On Sat, 12 Jan 2019 02:46:33 +0100, Yang, Libin wrote: > > Hi Takashi, > > >> > > >> > On Fri, 11 Jan 2019 06:20:23 +0100, > >> > Yang, Libin wrote: > >> > > > >> > > The below patch may have a small confliction that the trigger will > >> > > be called twice as our SOF has already call snd_pcm_suspend() in > >> > > card suspend. > >> > > >> > It should be no problem, snd_pcm_suspend() can be called multiple > >> > times. If it's already suspended, just nothing happens. > >> > >> Thinking of this problem again, does a patch like below work instead? > >> This looks like a better and more generic solution. > >> > >> What I'm not quite sure is whether the device suspend order between > >> PCM device and HD-audio codec device is guaranteed. I guess yes, > >> because the PCM device is registered always after the codec. But ASoC > >> might have another weirdness :) > > The suspend order is always a quite hard issue for me. I have to spend > a lot time checking the parent-child relationship. And if they don't have > such relationship, I don't know how to find the order. > > My initial idea to get rid of such dependency is: do the pcm suspend before > suspend(), for example put it in prepare(). And set device clk off and power > off in suspend(). This can make sure pcm is always properly suspended. > But, I'm not sure prepare() is a right place because it seems prepare() is not > designed to do such things and prepare() may impact the suspend/resume > sequence based on its return value. If your below patch works, I think it will > be a best solution which can handle such things in ALSA level. I think we > may need to do a lot of test because the different cards drivers are > extremely different. As mentioned below, ASoC is another thing. Its PM sequence is found in snd_soc_suspend(). Takashi > > Regards, > Libin > > > > >... and further reading revealed that my afraid is right -- ASoC is another beast. > >Basically PM in ASoC requires *not* to be done by the usual device PM > >callbacks. snd_soc_suspend() does the whole suspend jobs sequentially. So, > >what you need for Intel ASoC driver is to do suspend/resume with the > >component callbacks instead of device PM ops. > >Currently the plumbing of hdac-hdmi driver is in a half-baked state. > > > >> If this patch works, basically we can get rid of all external callers > >> of snd_pcm_suspend() & co. That'll be a nice cleanup. > > > >OTOH, the statement above is still true for non-ASoC drivers. So we'd end up > >needing an extra flag or such to prevent the PCM device PM ops for ASoC, > >while using this for the rest. > > > > > >thanks, > > > >Takashi > > > >> > >> > >> thanks, > >> > >> Takashi > >> > >> --- > >> diff --git a/sound/core/pcm.c b/sound/core/pcm.c index > >> 01b9d62eef14..d8d57336b8b8 100644 > >> --- a/sound/core/pcm.c > >> +++ b/sound/core/pcm.c > >> @@ -683,6 +683,25 @@ static inline int > >> snd_pcm_substream_proc_done(struct snd_pcm_substream *substrea > >> > >> static const struct attribute_group *pcm_dev_attr_groups[]; > >> > >> +#ifdef CONFIG_PM_SLEEP > >> +static int do_pcm_suspend(struct device *dev) { > >> + struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, > >> +dev); > >> + > >> + snd_pcm_suspend_all(pstr->pcm); > >> + return 0; > >> +} > >> +#endif > >> + > >> +static const struct dev_pm_ops pcm_dev_pm_ops = { > >> + SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL) }; > >> + > >> +static const struct device_type pcm_dev_type = { > >> + .name = "pcm", > >> + .pm = &pcm_dev_pm_ops, > >> +}; > >> + > >> /** > >> * snd_pcm_new_stream - create a new PCM stream > >> * @pcm: the pcm instance > >> @@ -713,6 +732,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int > >> stream, int substream_count) > >> > >> snd_device_initialize(&pstr->dev, pcm->card); > >> pstr->dev.groups = pcm_dev_attr_groups; > >> + pstr->dev.type = &pcm_dev_type; > >> dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, > >pcm->device, > >> stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c'); > >> >
On 2019/1/12 下午3:43, Takashi Iwai wrote: > On Sat, 12 Jan 2019 02:46:33 +0100, > Yang, Libin wrote: >> >> Hi Takashi, >> >>>>> >>>>> On Fri, 11 Jan 2019 06:20:23 +0100, >>>>> Yang, Libin wrote: >>>>>> >>>>>> The below patch may have a small confliction that the trigger will >>>>>> be called twice as our SOF has already call snd_pcm_suspend() in >>>>>> card suspend. >>>>> >>>>> It should be no problem, snd_pcm_suspend() can be called multiple >>>>> times. If it's already suspended, just nothing happens. >>>> >>>> Thinking of this problem again, does a patch like below work instead? >>>> This looks like a better and more generic solution. >>>> >>>> What I'm not quite sure is whether the device suspend order between >>>> PCM device and HD-audio codec device is guaranteed. I guess yes, >>>> because the PCM device is registered always after the codec. But ASoC >>>> might have another weirdness :) >> >> The suspend order is always a quite hard issue for me. I have to spend >> a lot time checking the parent-child relationship. And if they don't have >> such relationship, I don't know how to find the order. >> >> My initial idea to get rid of such dependency is: do the pcm suspend before >> suspend(), for example put it in prepare(). And set device clk off and power >> off in suspend(). This can make sure pcm is always properly suspended. >> But, I'm not sure prepare() is a right place because it seems prepare() is not >> designed to do such things and prepare() may impact the suspend/resume >> sequence based on its return value. If your below patch works, I think it will >> be a best solution which can handle such things in ALSA level. I think we >> may need to do a lot of test because the different cards drivers are >> extremely different. > > As mentioned below, ASoC is another thing. Its PM sequence is found > in snd_soc_suspend(). > > > Takashi This is an interesting topic, it worth to do a surgery to remove those ASoC snd_pcm_suspend_all() invoking and do unified PCM suspend in ALSA PCM level? Thanks, ~Keyon
On Mon, 14 Jan 2019 02:16:02 +0100, Keyon Jie wrote: > > > > On 2019/1/12 下午3:43, Takashi Iwai wrote: > > On Sat, 12 Jan 2019 02:46:33 +0100, > > Yang, Libin wrote: > >> > >> Hi Takashi, > >> > >>>>> > >>>>> On Fri, 11 Jan 2019 06:20:23 +0100, > >>>>> Yang, Libin wrote: > >>>>>> > >>>>>> The below patch may have a small confliction that the trigger will > >>>>>> be called twice as our SOF has already call snd_pcm_suspend() in > >>>>>> card suspend. > >>>>> > >>>>> It should be no problem, snd_pcm_suspend() can be called multiple > >>>>> times. If it's already suspended, just nothing happens. > >>>> > >>>> Thinking of this problem again, does a patch like below work instead? > >>>> This looks like a better and more generic solution. > >>>> > >>>> What I'm not quite sure is whether the device suspend order between > >>>> PCM device and HD-audio codec device is guaranteed. I guess yes, > >>>> because the PCM device is registered always after the codec. But ASoC > >>>> might have another weirdness :) > >> > >> The suspend order is always a quite hard issue for me. I have to spend > >> a lot time checking the parent-child relationship. And if they don't have > >> such relationship, I don't know how to find the order. > >> > >> My initial idea to get rid of such dependency is: do the pcm suspend before > >> suspend(), for example put it in prepare(). And set device clk off and power > >> off in suspend(). This can make sure pcm is always properly suspended. > >> But, I'm not sure prepare() is a right place because it seems prepare() is not > >> designed to do such things and prepare() may impact the suspend/resume > >> sequence based on its return value. If your below patch works, I think it will > >> be a best solution which can handle such things in ALSA level. I think we > >> may need to do a lot of test because the different cards drivers are > >> extremely different. > > > > As mentioned below, ASoC is another thing. Its PM sequence is found > > in snd_soc_suspend(). > > > > > > Takashi > > This is an interesting topic, it worth to do a surgery to remove those > ASoC snd_pcm_suspend_all() invoking and do unified PCM suspend in ALSA > PCM level? This won't work easily, I suppose. Take a look at snd_soc_suspend() code. It's designed to run several procedures sequentially there, and each component is supposed to process the suspend/resume task via the ASoC component callback, not the driver PM ops that can be outside this call. HD-audio ASoC codec driver is an ASoC codec driver, hence it should follow that way. Takashi
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 3ab2949..782b323 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -1895,7 +1895,7 @@ static int hdmi_codec_prepare(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); - pm_runtime_get_sync(&hdev->dev); + snd_hdac_display_power(hdev->bus, hdev->addr, true); /* * Power down afg. @@ -1906,6 +1906,7 @@ static int hdmi_codec_prepare(struct device *dev) */ snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE, AC_PWRST_D3); + snd_hdac_display_power(hdev->bus, hdev->addr, false); return 0; } @@ -1915,6 +1916,7 @@ static void hdmi_codec_complete(struct device *dev) struct hdac_device *hdev = dev_to_hdac_dev(dev); struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); + snd_hdac_display_power(hdev->bus, hdev->addr, true); /* Power up afg */ snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE, AC_PWRST_D0); @@ -1930,7 +1932,7 @@ static void hdmi_codec_complete(struct device *dev) */ hdac_hdmi_present_sense_all_pins(hdev, hdmi, false); - pm_runtime_put_sync(&hdev->dev); + snd_hdac_display_power(hdev->bus, hdev->addr, false); } #else #define hdmi_codec_prepare NULL diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 60c9483..89f4d66 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -336,9 +336,6 @@ static int skl_suspend(struct device *dev) skl->skl_sst->fw_loaded = false; } - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false); - return 0; } @@ -350,10 +347,6 @@ static int skl_resume(struct device *dev) struct hdac_ext_link *hlink = NULL; int ret; - /* Turned OFF in HDMI codec driver after codec reconfiguration */ - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true); - /* * resume only when we are not in suspend active, otherwise need to * restore the device