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

[David Henningsson <david.henningsson@canonical.com>]

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 :-)


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic