Re: [alsa-devel] [PATCH] [sound] hdac-codec runtime suspended at PM:Suspend.
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
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:
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?
In kernel traces this hdac hdmi codec device appear with name "HDA Codec ehdaudio0D2", and it seems to be a leaf device do not have any child. So it would be ok to use pm_runtime_disable to avoid any 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);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?
Idea of this patch to remove the PCI 00:1f:3 (hdac controller) direct complete blocker, as hdac hdmi codec is resuming it from runtime suspend in its prepare callback to set the hdac hdmi codec in low power state. (https://elixir.bootlin.com/linux/v4.16-rc5/source/sound/soc/codecs/hdac_hdmi...) So if hdac hdmi codec in active state it require to resume hdac controller from runtime suspend.
/*@@ -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.
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.
/* Power up afg */ snd_hdac_codec_read(hdac, hdac->afg, 0, AC_VERB_SET_POWER_STATE, AC_PWRST_D0);-- 2.7.4
participants (2)
-
Anshuman Gupta -
Rafael J. Wysocki