From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Henningsson Subject: Re: [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team Date: Tue, 30 Apr 2013 12:29:37 +0200 Message-ID: <517F9D11.3020905@canonical.com> References: <1B1032E88FFFDA4898B04D530F28DEDB53CAB7@SHSMSX101.ccr.corp.intel.com> <1B1032E88FFFDA4898B04D530F28DEDB53CB7C@SHSMSX101.ccr.corp.intel.com> <20130426145708.GN6169@phenom.ffwll.local> <20130426154207.GP6169@phenom.ffwll.local> <20130426171737.GR6169@phenom.ffwll.local> <46B810F6945F7C4788E11DCE57EC4890109D3608@SHSMSX102.ccr.corp.intel.com> <20130427113529.GT6169@phenom.ffwll.local> <20130429080219.05108857@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by alsa0.perex.cz (Postfix) with ESMTP id 7B132264EFA for ; Tue, 30 Apr 2013 12:29:40 +0200 (CEST) In-Reply-To: <20130429080219.05108857@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Jesse Barnes Cc: "alsa-devel@alsa-project.org" , "Zanoni, Paulo R" , Takashi Iwai , Daniel Vetter , "intel-gfx@lists.freedesktop.org" , "Wysocki, Rafael J" , Wang Xingchao , "Wang, Xingchao" , "Li, Jocelyn" , "Hindman, Gavin" , Daniel Vetter , "Girdwood, Liam R" , "Lin, Mengdong" , "ville.syrjala@linux.intel.com" List-Id: alsa-devel@alsa-project.org On 04/29/2013 05:02 PM, Jesse Barnes wrote: > On Sat, 27 Apr 2013 13:35:29 +0200 > Daniel Vetter 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