On 04/30/2013 04:41 PM, Liam Girdwood wrote: > 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 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. >> > > 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. Ok, I stand corrected. > >>> + >>> 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. Nobody objected, so I wrote a very draft patch, which is attached to this email. It probably does not even compile, but should show how I envision things could be mended together. Do you think it could work? Also, I tried to find the patch set for the i915 side, but couldn't find any while looking on the intel-gfx mailinglist (which I'm not subscribed to) - only comments on those patches. Where can the latest version of the i915 patches be found? -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-ALSA-hda-implement-i915-power-well-callbacks.patch Type: text/x-patch Size: 6181 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20130502/36d76be4/attachment-0001.bin>
