On 04/29/2013 05:02 PM, Jesse Barnes wrote: > On Sat, 27 Apr 2013 13:35:29 +0200 > Daniel Vetter <daniel at 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 :-) -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
