Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

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

On 07/24/2013 12:48 PM, Takashi Iwai wrote:
> At Tue, 23 Jul 2013 08:09:02 +0000,
> Wang, Xingchao wrote:
>>
>>>>
>>>> Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
>>>> ---
>>>>    sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
>>>>    1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>>>> index 8860dd5..a7ac7fd 100644
>>>> --- a/sound/pci/hda/hda_intel.c
>>>> +++ b/sound/pci/hda/hda_intel.c
>>>> @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct device
>>> *dev)
>>>>    {
>>>>    	struct snd_card *card = dev_get_drvdata(dev);
>>>>    	struct azx *chip = card->private_data;
>>>> +	int status;
>>>> +
>>>> +	/* enable controller wake up event */
>>>> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
>>>> +		  STATESTS_INT_MASK);
>>>>
>>>>    	azx_stop_chip(chip);
>>>>    	azx_enter_link_reset(chip);
>>>> @@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct device
>>> *dev)
>>>>    {
>>>>    	struct snd_card *card = dev_get_drvdata(dev);
>>>>    	struct azx *chip = card->private_data;
>>>> +	struct hda_bus *bus;
>>>> +	struct hda_codec *codec;
>>>> +	int status;
>>>>
>>>>    	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
>>>>    		hda_display_power(true);
>>>> +
>>>> +	/* Read STATESTS before controller reset */
>>>> +	status = azx_readw(chip, STATESTS);
>>>> +
>>>>    	azx_init_pci(chip);
>>>>    	azx_init_chip(chip, 1);
>>>> +
>>>> +	bus = chip->bus;
>>>> +	if (status && bus) {
>>>> +		list_for_each_entry(codec, &bus->codec_list, list)
>>>> +			if (status & (1 << codec->addr))
>>>> +				queue_delayed_work(codec->bus->workq,
>>>> +						   &codec->jackpoll_work,
>>> codec->jackpoll_interval);
>>>
>>> Is there a reason you want to move the jack detection to a delayed work, i e,
>>> why can't we just call:
>>>
>>>    	snd_hda_jack_set_dirty_all(codec);
>>>    	snd_hda_jack_poll_all(codec);
>>>
>>> ...from here?
>>
>> In fact it doesnot work for me, It will again call runtime_resume() and caused dead loop.
>
> It means that the power refcount is messed up by this way.
>
> When a verb is executed, the codec goes to a full resume mode, which
> turns up the controller, eventually calling azx_power_notify().
> In azx_power_notify(), pm_runtime_{get|put}_sync() is called
> accordingly, which again goes to runtime resume.  A quick fix would be
> to add a flag or something to avoid reentrace.
>
> But, calling the jackpoll in the resume is basically superfluous.
> As already mentioned, issuing a verb itself triggers the full resume,
> and this already includes the update of the whole jack status.
> Thus, executing jackpoll at that place means to perform the jack
> polling twice.
>
> In other words, what's we need to achieve is just to call
> snd_hda_resume() appropriately in the runtime resume case.
> Of course, this will need more fixes for avoiding reentrance, etc.
>
> But, it leads to another question: do we need a full resume just for
> jack detection and user-space notification?  Just reading the pin
> detect state should be able to run even in D3 (for chips that are
> capable of it), and the notification itself doesn't need any audio
> hardware action.

The STATESTS register only indicates which codec requested wakeup, not 
which pin. So you need to issue the get_pin_sense verb for all pins on 
the codec, which means that the codec - controller link must be powered up.

So that's half of the resume procedure already. Are you proposing we 
introduce some kind of "half-resumed" mode that we would be in when we 
wait for the response from get_pin_sense? It sounds like additional 
complexity for very little gain in power.

Or am I missing something here?


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