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

[David Henningsson]

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://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20130502/36d76be4/attachment-0001.bin>