| Message ID | 20130429080219.05108857@jbarnes-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/29/2013 05:02 PM, Jesse Barnes wrote: > On Sat, 27 Apr 2013 13:35:29 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > >> On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote: >>> Let me throw a basic proposal on Audio driver side, please give your comments freely. >>> >>> it contains the power well control usage points: >>> #1: audio request power well at boot up. >>> I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. >>> #2: audio request power on resume >>> After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. >>> #3: audio release power well control at suspend >>> Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. >>> #4: audio release power well when module unload >>> Audio release power well at remove callback to let i915 know. >> >> I miss the power well grab/dropping at runtime from the audio side. If the >> audio driver forces the power well to be on the entire time it's loaded, >> that's not good, since the power well stuff is very much for runtime PM. >> We _must_ be able to switch off the power well whenever possible. > > Xingchao, I'm not an audio developer so I'm probably way off. > > But what we really need is a very small and targeted set of calls into > the i915 driver from say the HDMI driver in HDA. It looks like the > prepare/cleanup pair in the pcm_ops structure might be the right place > to put things? If that's too fine grained, you could do it at > open/close time I guess, but the danger there is that some app will > keep the device open even while it's not playing. > > If that won't work, maybe calling i915 from hda_power_work in the > higher level code would be better? > > For detecting whether to call i915 at all, you can filter on the PCI > IDs (just look for an Intel graphics device and if present, try to get > the i915 symbols for the power functions). > > --- a/sound/pci/hda/hda_codec.c > +++ b/sound/pci/hda/hda_codec.c > @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code > codec->patch_ops.suspend(codec); > hda_cleanup_all_streams(codec); > state = hda_set_power_state(codec, AC_PWRST_D3); > + if (i915_shared_power_well) > + i915_put_power_well(codec->i915_data); > /* Cancel delayed work if we aren't currently running from it. */ > if (!in_wq) > cancel_delayed_work_sync(&codec->power_work); > @@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo > return; > spin_unlock(&codec->power_lock); > > + if (i915_shared_power_well) > + i915_get_power_well(codec->i915_data); Is it wise that a _get function actually has side effects? Perhaps _push and _pop or something else would be better semantics. > + > cancel_delayed_work_sync(&codec->power_work); > > spin_lock(&codec->power_lock); > > With some code at init time to get the i915 symbols you need to call > and whether or not the shared power well is present... > > Takashi, any other ideas? > > The high level goal here should be for the audio driver to call into > i915 with get/put power well around the sequences where it needs the > power to be up (reading/writing registers, playing audio), but not > across the whole time the driver is loaded, just like you already do > with the powersave work functions, e.g. hda_call_codec_suspend. I think this sounds about right. The question is how to avoid a dependency on the i915 driver when it's not necessary, such as when the HDMI codec is AMD or Nvidia. The most obvious way to me seems to be to create a new snd-hda-codec-hdmi-haswell module (that depends on both i915 and snd-hda-codec-hdmi), and then let that call into i915 and codec-hdmi drivers as necessary, e g using the set_power_state callback for the i915 stuff. But maybe there's something smarter to do here, as I'm not experienced in mending kernel pieces together :-)
On Tue, 30 Apr 2013 12:29:37 +0200 David Henningsson <david.henningsson@canonical.com> wrote: > On 04/29/2013 05:02 PM, Jesse Barnes wrote: > > On Sat, 27 Apr 2013 13:35:29 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > >> On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote: > >>> Let me throw a basic proposal on Audio driver side, please give your comments freely. > >>> > >>> it contains the power well control usage points: > >>> #1: audio request power well at boot up. > >>> I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. > >>> #2: audio request power on resume > >>> After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. > >>> #3: audio release power well control at suspend > >>> Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. > >>> #4: audio release power well when module unload > >>> Audio release power well at remove callback to let i915 know. > >> > >> I miss the power well grab/dropping at runtime from the audio side. If the > >> audio driver forces the power well to be on the entire time it's loaded, > >> that's not good, since the power well stuff is very much for runtime PM. > >> We _must_ be able to switch off the power well whenever possible. > > > > Xingchao, I'm not an audio developer so I'm probably way off. > > > > But what we really need is a very small and targeted set of calls into > > the i915 driver from say the HDMI driver in HDA. It looks like the > > prepare/cleanup pair in the pcm_ops structure might be the right place > > to put things? If that's too fine grained, you could do it at > > open/close time I guess, but the danger there is that some app will > > keep the device open even while it's not playing. > > > > If that won't work, maybe calling i915 from hda_power_work in the > > higher level code would be better? > > > > For detecting whether to call i915 at all, you can filter on the PCI > > IDs (just look for an Intel graphics device and if present, try to get > > the i915 symbols for the power functions). > > > > --- a/sound/pci/hda/hda_codec.c > > +++ b/sound/pci/hda/hda_codec.c > > @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code > > codec->patch_ops.suspend(codec); > > hda_cleanup_all_streams(codec); > > state = hda_set_power_state(codec, AC_PWRST_D3); > > + if (i915_shared_power_well) > > + i915_put_power_well(codec->i915_data); > > /* Cancel delayed work if we aren't currently running from it. */ > > if (!in_wq) > > cancel_delayed_work_sync(&codec->power_work); > > @@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo > > return; > > spin_unlock(&codec->power_lock); > > > > + if (i915_shared_power_well) > > + i915_get_power_well(codec->i915_data); > > Is it wise that a _get function actually has side effects? Perhaps _push > and _pop or something else would be better semantics. We have other get/put functions like this, so I thought it fit (vblank irq, PCI runtime PM). > > > + > > cancel_delayed_work_sync(&codec->power_work); > > > > spin_lock(&codec->power_lock); > > > > With some code at init time to get the i915 symbols you need to call > > and whether or not the shared power well is present... > > > > Takashi, any other ideas? > > > > The high level goal here should be for the audio driver to call into > > i915 with get/put power well around the sequences where it needs the > > power to be up (reading/writing registers, playing audio), but not > > across the whole time the driver is loaded, just like you already do > > with the powersave work functions, e.g. hda_call_codec_suspend. > > I think this sounds about right. The question is how to avoid a > dependency on the i915 driver when it's not necessary, such as when the > HDMI codec is AMD or Nvidia. > > The most obvious way to me seems to be to create a new > snd-hda-codec-hdmi-haswell module (that depends on both i915 and > snd-hda-codec-hdmi), and then let that call into i915 and codec-hdmi > drivers as necessary, e g using the set_power_state callback for the > i915 stuff. > > But maybe there's something smarter to do here, as I'm not experienced > in mending kernel pieces together :-) Yeah a separate module would be ok too. But I was hoping the i915 dependencies could be pretty well contained so as not to affect readability in the HDA driver, while having all the conditionality needed for whether i915 is loaded, or loaded first, or needed at all on a given platform. Thanks, Jesse
On Tue, 2013-04-30 at 12:29 +0200, David Henningsson wrote: > On 04/29/2013 05:02 PM, Jesse Barnes wrote: > > On Sat, 27 Apr 2013 13:35:29 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > >> On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote: > >>> Let me throw a basic proposal on Audio driver side, please give your comments freely. > >>> > >>> it contains the power well control usage points: > >>> #1: audio request power well at boot up. > >>> I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. > >>> #2: audio request power on resume > >>> After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. > >>> #3: audio release power well control at suspend > >>> Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. > >>> #4: audio release power well when module unload > >>> Audio release power well at remove callback to let i915 know. > >> > >> I miss the power well grab/dropping at runtime from the audio side. If the > >> audio driver forces the power well to be on the entire time it's loaded, > >> that's not good, since the power well stuff is very much for runtime PM. > >> We _must_ be able to switch off the power well whenever possible. > > > > Xingchao, I'm not an audio developer so I'm probably way off. > > > > But what we really need is a very small and targeted set of calls into > > the i915 driver from say the HDMI driver in HDA. It looks like the > > prepare/cleanup pair in the pcm_ops structure might be the right place > > to put things? If that's too fine grained, you could do it at > > open/close time I guess, but the danger there is that some app will > > keep the device open even while it's not playing. > > > > If that won't work, maybe calling i915 from hda_power_work in the > > higher level code would be better? > > > > For detecting whether to call i915 at all, you can filter on the PCI > > IDs (just look for an Intel graphics device and if present, try to get > > the i915 symbols for the power functions). > > > > --- a/sound/pci/hda/hda_codec.c > > +++ b/sound/pci/hda/hda_codec.c > > @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code > > codec->patch_ops.suspend(codec); > > hda_cleanup_all_streams(codec); > > state = hda_set_power_state(codec, AC_PWRST_D3); > > + if (i915_shared_power_well) > > + i915_put_power_well(codec->i915_data); > > /* Cancel delayed work if we aren't currently running from it. */ > > if (!in_wq) > > cancel_delayed_work_sync(&codec->power_work); > > @@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo > > return; > > spin_unlock(&codec->power_lock); > > > > + if (i915_shared_power_well) > > + i915_get_power_well(codec->i915_data); > > Is it wise that a _get function actually has side effects? Perhaps _push > and _pop or something else would be better semantics. > I think the intention here is to model on the PM runtime subsystem where we can get/put the reference count on a PM resource. I'd expect with this API that i915_get_power_well() will increment the count and prevent the shared PM resource from being powered OFF. > > + > > cancel_delayed_work_sync(&codec->power_work); > > > > spin_lock(&codec->power_lock); > > > > With some code at init time to get the i915 symbols you need to call > > and whether or not the shared power well is present... > > > > Takashi, any other ideas? > > > > The high level goal here should be for the audio driver to call into > > i915 with get/put power well around the sequences where it needs the > > power to be up (reading/writing registers, playing audio), but not > > across the whole time the driver is loaded, just like you already do > > with the powersave work functions, e.g. hda_call_codec_suspend. > > I think this sounds about right. The question is how to avoid a > dependency on the i915 driver when it's not necessary, such as when the > HDMI codec is AMD or Nvidia. > > The most obvious way to me seems to be to create a new > snd-hda-codec-hdmi-haswell module (that depends on both i915 and > snd-hda-codec-hdmi), and then let that call into i915 and codec-hdmi > drivers as necessary, e g using the set_power_state callback for the > i915 stuff. > > But maybe there's something smarter to do here, as I'm not experienced > in mending kernel pieces together :-) > Interesting idea, we could have something similar to the AC97 ad-hoc device support where we could load other HW specific AC97 modules (like touchscreen controllers) without breaking the generic nature of the base driver. e.g. the WM9712 AC97 touchscreen driver could connect to the generic AC97 audio driver (get it's device struct ac97 *) and perform AC97 register IO, ac97 API calls etc. Regards Liam
> > On 04/29/2013 05:02 PM, Jesse Barnes wrote: > > > On Sat, 27 Apr 2013 13:35:29 +0200 > > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > The high level goal here should be for the audio driver to call into > > > i915 with get/put power well around the sequences where it needs the > > > power to be up (reading/writing registers, playing audio), but not > > > across the whole time the driver is loaded, just like you already do > > > with the powersave work functions, e.g. hda_call_codec_suspend. Hi Daniel, We can modify the patch so that the audio driver will not request the power well across the whole time the HD-A driver is loaded, but request/release the power well at runtime. The HD-Audio driver can support runtime PM. When the codec is idle, it can suspend the codec and further suspend the controller, thus the power well is no longer needed. So we can add haswell-specific code to release the power well in HD-A runtime suspend callback azx_suspend(), and request the power well in audio runtime resume callback azx_resume(). Thanks Mengdong
Hi Jesse, > -----Original Message----- > From: Barnes, Jesse > Sent: Monday, April 29, 2013 11:02 PM > To: Daniel Vetter > Cc: Wang, Xingchao; Takashi Iwai; Li, Jocelyn; Daniel Vetter; Zanoni, Paulo R; > ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; > intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; > Wysocki, Rafael J; Hindman, Gavin > Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- > alignment between graphic team and audio team > > On Sat, 27 Apr 2013 13:35:29 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote: > > > Let me throw a basic proposal on Audio driver side, please give your > comments freely. > > > > > > it contains the power well control usage points: > > > #1: audio request power well at boot up. > > > I915 may shut down power well after bootup initialization, as there's no > monitor connected outside or only eDP on pipe A. > > > #2: audio request power on resume > > > After exit from D3 mode, audio driver quest power on. This may happen at > normal resume or runtime resume. > > > #3: audio release power well control at suspend Audio driver will > > > let i915 know it doensot need power well anymore as it's going to suspend. > This may happened at normal suspend or runtime suspend point. > > > #4: audio release power well when module unload Audio release power > > > well at remove callback to let i915 know. > > > > I miss the power well grab/dropping at runtime from the audio side. If > > the audio driver forces the power well to be on the entire time it's > > loaded, that's not good, since the power well stuff is very much for runtime > PM. > > We _must_ be able to switch off the power well whenever possible. > > Xingchao, I'm not an audio developer so I'm probably way off. > > But what we really need is a very small and targeted set of calls into the i915 > driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair > in the pcm_ops structure might be the right place to put things? If that's too > fine grained, you could do it at open/close time I guess, but the danger there is > that some app will keep the device open even while it's not playing. > > If that won't work, maybe calling i915 from hda_power_work in the higher level > code would be better? > > For detecting whether to call i915 at all, you can filter on the PCI IDs (just look > for an Intel graphics device and if present, try to get the i915 symbols for the > power functions). > > --- a/sound/pci/hda/hda_codec.c > +++ b/sound/pci/hda/hda_codec.c > @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct > hda_code > codec->patch_ops.suspend(codec); > hda_cleanup_all_streams(codec); > state = hda_set_power_state(codec, AC_PWRST_D3); > + if (i915_shared_power_well) > + i915_put_power_well(codec->i915_data); > /* Cancel delayed work if we aren't currently running from it. */ > if (!in_wq) > cancel_delayed_work_sync(&codec->power_work); > @@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec > *codec, bo > return; > spin_unlock(&codec->power_lock); > > + if (i915_shared_power_well) > + i915_get_power_well(codec->i915_data); > + > cancel_delayed_work_sync(&codec->power_work); > > spin_lock(&codec->power_lock); > > With some code at init time to get the i915 symbols you need to call and > whether or not the shared power well is present... > > Takashi, any other ideas? > > The high level goal here should be for the audio driver to call into > i915 with get/put power well around the sequences where it needs the power > to be up (reading/writing registers, playing audio), but not across the whole > time the driver is loaded, just like you already do with the powersave work > functions, e.g. hda_call_codec_suspend. That's the point. I missed the background about power well it's part of runtime power feature. Audio driver should not always keep power well on even it's not active, I guess you were misunderstood by the power well request point at azx_probe(). azx_probe() is very initial point for power well request, and it will switch to runtime pm soon(please check Mengdong's reply in another email), so the power well will be release if the codec is inactive, that match the initial idea of power well. Thanks --xingchao
At Mon, 29 Apr 2013 08:02:19 -0700, Jesse Barnes wrote: > > On Sat, 27 Apr 2013 13:35:29 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote: > > > Let me throw a basic proposal on Audio driver side, please give your comments freely. > > > > > > it contains the power well control usage points: > > > #1: audio request power well at boot up. > > > I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. > > > #2: audio request power on resume > > > After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. > > > #3: audio release power well control at suspend > > > Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. > > > #4: audio release power well when module unload > > > Audio release power well at remove callback to let i915 know. > > > > I miss the power well grab/dropping at runtime from the audio side. If the > > audio driver forces the power well to be on the entire time it's loaded, > > that's not good, since the power well stuff is very much for runtime PM. > > We _must_ be able to switch off the power well whenever possible. > > Xingchao, I'm not an audio developer so I'm probably way off. > > But what we really need is a very small and targeted set of calls into > the i915 driver from say the HDMI driver in HDA. It looks like the > prepare/cleanup pair in the pcm_ops structure might be the right place > to put things? If that's too fine grained, you could do it at > open/close time I guess, but the danger there is that some app will > keep the device open even while it's not playing. Well, the question is what impact the power well on/off has against the audio. Do we need to resume the HD-audio controller / codec fully from the scratch? I guess not, but I have no certain idea. If the impact of the change (i.e. the procedure needed to resume) is small, somehow limited to the targeted converter/pin, it can be implemented in the prepare/cleanup callback of the codec driver, yes. Though, the easiest path would be to add i915_get/put_power_well() in the codec probe, suspend, resume, and free callbacks, as you pointed out below. > If that won't work, maybe calling i915 from hda_power_work in the > higher level code would be better? > > For detecting whether to call i915 at all, you can filter on the PCI > IDs (just look for an Intel graphics device and if present, try to get > the i915 symbols for the power functions). > > --- a/sound/pci/hda/hda_codec.c > +++ b/sound/pci/hda/hda_codec.c > @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code > codec->patch_ops.suspend(codec); > hda_cleanup_all_streams(codec); > state = hda_set_power_state(codec, AC_PWRST_D3); > + if (i915_shared_power_well) > + i915_put_power_well(codec->i915_data); > /* Cancel delayed work if we aren't currently running from it. */ > if (!in_wq) > cancel_delayed_work_sync(&codec->power_work); > @@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo > return; > spin_unlock(&codec->power_lock); > > + if (i915_shared_power_well) > + i915_get_power_well(codec->i915_data); > + > cancel_delayed_work_sync(&codec->power_work); > > spin_lock(&codec->power_lock); > > With some code at init time to get the i915 symbols you need to call > and whether or not the shared power well is present... > > Takashi, any other ideas? > > The high level goal here should be for the audio driver to call into > i915 with get/put power well around the sequences where it needs the > power to be up (reading/writing registers, playing audio), but not > across the whole time the driver is loaded, just like you already do > with the powersave work functions, e.g. hda_call_codec_suspend. I guess controlling the suspend/resume path would be practically working well. If a system is really power-conscious, it should use a sound backend like PulseAudio, which closes the unused PCM devices frequently enough, and the power_save option should be changed by the power management tool on the fly. If such mechanisms aren't used, it means that user doesn't care about power, after all. thanks, Takashi
Hi Takashi, > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Friday, May 03, 2013 10:27 PM > To: Barnes, Jesse > Cc: Daniel Vetter; Wang, Xingchao; Li, Jocelyn; Daniel Vetter; Zanoni, Paulo R; > ville.syrjala@linux.intel.com; Lin, Mengdong; Girdwood, Liam R; > intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Wang Xingchao; > Wysocki, Rafael J; Hindman, Gavin > Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- > alignment between graphic team and audio team > > At Mon, 29 Apr 2013 08:02:19 -0700, > Jesse Barnes wrote: > > > > On Sat, 27 Apr 2013 13:35:29 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Sat, Apr 27, 2013 at 09:20:39AM +0000, Wang, Xingchao wrote: > > > > Let me throw a basic proposal on Audio driver side, please give your > comments freely. > > > > > > > > it contains the power well control usage points: > > > > #1: audio request power well at boot up. > > > > I915 may shut down power well after bootup initialization, as there's no > monitor connected outside or only eDP on pipe A. > > > > #2: audio request power on resume > > > > After exit from D3 mode, audio driver quest power on. This may happen at > normal resume or runtime resume. > > > > #3: audio release power well control at suspend Audio driver will > > > > let i915 know it doensot need power well anymore as it's going to suspend. > This may happened at normal suspend or runtime suspend point. > > > > #4: audio release power well when module unload Audio release > > > > power well at remove callback to let i915 know. > > > > > > I miss the power well grab/dropping at runtime from the audio side. > > > If the audio driver forces the power well to be on the entire time > > > it's loaded, that's not good, since the power well stuff is very much for > runtime PM. > > > We _must_ be able to switch off the power well whenever possible. > > > > Xingchao, I'm not an audio developer so I'm probably way off. > > > > But what we really need is a very small and targeted set of calls into > > the i915 driver from say the HDMI driver in HDA. It looks like the > > prepare/cleanup pair in the pcm_ops structure might be the right place > > to put things? If that's too fine grained, you could do it at > > open/close time I guess, but the danger there is that some app will > > keep the device open even while it's not playing. > > Well, the question is what impact the power well on/off has against the audio. > Do we need to resume the HD-audio controller / codec fully from the scratch? > I guess not, but I have no certain idea. Both the display H-DA controller and codec are under control by power well. When the power well is off, for H-DA controller, the MMIO space is off, the PCI Registers are in on well. The codec could not access anymore. > > If the impact of the change (i.e. the procedure needed to resume) is small, > somehow limited to the targeted converter/pin, it can be implemented in the > prepare/cleanup callback of the codec driver, yes. > > Though, the easiest path would be to add i915_get/put_power_well() in the > codec probe, suspend, resume, and free callbacks, as you pointed out below. Yes, and Jesse should get the background that, even power well is requested at probe, It will not take long time to waste power. The controller/codec will enter power save mode If runtime pm enabled. Thanks --xingchao > > > If that won't work, maybe calling i915 from hda_power_work in the > > higher level code would be better? > > > > For detecting whether to call i915 at all, you can filter on the PCI > > IDs (just look for an Intel graphics device and if present, try to get > > the i915 symbols for the power functions). > > > > --- a/sound/pci/hda/hda_codec.c > > +++ b/sound/pci/hda/hda_codec.c > > @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct > hda_code > > codec->patch_ops.suspend(codec); > > hda_cleanup_all_streams(codec); > > state = hda_set_power_state(codec, AC_PWRST_D3); > > + if (i915_shared_power_well) > > + i915_put_power_well(codec->i915_data); > > /* Cancel delayed work if we aren't currently running from it. */ > > if (!in_wq) > > cancel_delayed_work_sync(&codec->power_work); > > @@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct > hda_codec *codec, bo > > return; > > spin_unlock(&codec->power_lock); > > > > + if (i915_shared_power_well) > > + i915_get_power_well(codec->i915_data); > > + > > cancel_delayed_work_sync(&codec->power_work); > > > > spin_lock(&codec->power_lock); > > > > With some code at init time to get the i915 symbols you need to call > > and whether or not the shared power well is present... > > > > Takashi, any other ideas? > > > > The high level goal here should be for the audio driver to call into > > i915 with get/put power well around the sequences where it needs the > > power to be up (reading/writing registers, playing audio), but not > > across the whole time the driver is loaded, just like you already do > > with the powersave work functions, e.g. hda_call_codec_suspend. > > I guess controlling the suspend/resume path would be practically working well. > If a system is really power-conscious, it should use a sound backend like > PulseAudio, which closes the unused PCM devices frequently enough, and the > power_save option should be changed by the power management tool on the > fly. > > If such mechanisms aren't used, it means that user doesn't care about power, > after all. > > > thanks, > > Takashi
--- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3); + if (i915_shared_power_well) + i915_put_power_well(codec->i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(&codec->power_work); @@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo return; spin_unlock(&codec->power_lock); + if (i915_shared_power_well) + i915_get_power_well(codec->i915_data); + cancel_delayed_work_sync(&codec->power_work); spin_lock(&codec->power_lock);