[Intel-gfx] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team

[Takashi Iwai]

At Mon, 29 Apr 2013 08:02:19 -0700,
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.

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