[01/11] drm/i915: Fix runtime PM for LPE audio
diff mbox

Message ID 20170427160231.13337-2-ville.syrjala@linux.intel.com
State New
Headers show

Commit Message

Ville Syrjälä April 27, 2017, 4:02 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Not calling pm_runtime_enable() means that runtime PM can't be
enabled at all via sysfs. So we definitely need to call it
from somewhere.

Calling it from the driver seems like a bad idea because it
would have to be paired with a pm_runtime_disable() at driver
unload time, otherwise the core gets upset. Also if there's
no LPE audio driver loaded then we couldn't runtime suspend
i915 either.

So it looks like a better plan is to call it from i915 when
we register the platform device. That seems to match how
pci generally does things. I cargo culted the
pm_runtime_forbid() and pm_runtime_set_active() calls from
pci as well.

The exposed runtime PM API is massive an thorougly misleading, so
I don't actually know if this is how you're supposed to use the API
or not. But it seems to work. I can now runtime suspend i915 again
with or without the LPE audio driver loaded, and reloading the
LPE audio driver also seems to work.

Note that powertop won't auto-tune runtime PM for platform devices,
which is a little annoying. So I'm not sure that leaving runtime
PM in "on" mode by default is the best choice here. But I've left
it like that for now at least.

Also remove the comment about there not being much benefit from
LPE audio runtime PM. Not allowing runtime PM blocks i915 runtime
PM, which will also block s0ix, and that could have a measurable
impact on power consumption.

Cc: Takashi Iwai <tiwai@suse.de>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Fixes: 0b6b524f3915 ("ALSA: x86: Don't enable runtime PM as default")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lpe_audio.c | 5 +++++
 sound/x86/intel_hdmi_audio.c           | 4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Takashi Iwai April 28, 2017, 8:15 a.m. UTC | #1
On Thu, 27 Apr 2017 18:02:20 +0200,
ville.syrjala@linux.intel.com wrote:
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Not calling pm_runtime_enable() means that runtime PM can't be
> enabled at all via sysfs. So we definitely need to call it
> from somewhere.
> 
> Calling it from the driver seems like a bad idea because it
> would have to be paired with a pm_runtime_disable() at driver
> unload time, otherwise the core gets upset. Also if there's
> no LPE audio driver loaded then we couldn't runtime suspend
> i915 either.
> 
> So it looks like a better plan is to call it from i915 when
> we register the platform device. That seems to match how
> pci generally does things. I cargo culted the
> pm_runtime_forbid() and pm_runtime_set_active() calls from
> pci as well.
> 
> The exposed runtime PM API is massive an thorougly misleading, so
> I don't actually know if this is how you're supposed to use the API
> or not. But it seems to work. I can now runtime suspend i915 again
> with or without the LPE audio driver loaded, and reloading the
> LPE audio driver also seems to work.
> 
> Note that powertop won't auto-tune runtime PM for platform devices,
> which is a little annoying. So I'm not sure that leaving runtime
> PM in "on" mode by default is the best choice here. But I've left
> it like that for now at least.

The reason I didn't proactively turn on the runtime PM was that it
often caused a few seconds of pause to the A/V receivers before
actually starting playing.

There is a planned feature to keep sending the silent stream even
after stopping the stream, but it's not implemented yet.

> Also remove the comment about there not being much benefit from
> LPE audio runtime PM. Not allowing runtime PM blocks i915 runtime
> PM, which will also block s0ix, and that could have a measurable
> impact on power consumption.
> 
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Fixes: 0b6b524f3915 ("ALSA: x86: Don't enable runtime PM as default")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Takashi Iwai <tiwai@suse.de>

IMO, this should be tagged with Cc to stable.


thanks,

Takashi

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
index 25d8e76489e4..668f00480d97 100644
--- a/drivers/gpu/drm/i915/intel_lpe_audio.c
+++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
@@ -63,6 +63,7 @@ 
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/pci.h>
+#include <linux/pm_runtime.h>
 
 #include "i915_drv.h"
 #include <linux/delay.h>
@@ -121,6 +122,10 @@  lpe_audio_platdev_create(struct drm_i915_private *dev_priv)
 
 	kfree(rsc);
 
+	pm_runtime_forbid(&platdev->dev);
+	pm_runtime_set_active(&platdev->dev);
+	pm_runtime_enable(&platdev->dev);
+
 	return platdev;
 
 err:
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c
index c505b019e09c..bfac6f21ae5e 100644
--- a/sound/x86/intel_hdmi_audio.c
+++ b/sound/x86/intel_hdmi_audio.c
@@ -1809,10 +1809,6 @@  static int hdmi_lpe_audio_probe(struct platform_device *pdev)
 	pdata->notify_pending = false;
 	spin_unlock_irq(&pdata->lpe_audio_slock);
 
-	/* runtime PM isn't enabled as default, since it won't save much on
-	 * BYT/CHT devices; user who want the runtime PM should adjust the
-	 * power/ontrol and power/autosuspend_delay_ms sysfs entries instead
-	 */
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);