> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Wednesday, July 24, 2013 8:24 PM > To: David Henningsson > Cc: Wang, Xingchao; Wang Xingchao; alsa-devel@alsa-project.org; Girdwood, > Liam R > Subject: Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm > > At Wed, 24 Jul 2013 13:36:32 +0200, > David Henningsson wrote: > > > > 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 > > >>>> --- > > >>>> 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. Takashi, I donot find description about reading pin state in D3 in HD-Audio spec. Am I missing something? > > > > 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. > > Yes, as Takashi mentioned above, if each pin's jack state could be read in D3, it's not a problem anymore. I work out a new patch to remember old pin_sense and report to user-space when receive wake-up event. This could avoid polling jack state and send command, I thought it could avoid waking up audio controller/codec, but not... (for detail changes please find in attached patch file) > > So that's half of the resume procedure already. > > The rest half is rather a longer run; it executes many verbs to restore the whole > codec widget states. And, it's done just to run a few GET_PIN_SENSE verbs. > For reading GET_PIN_SENSE, we even don't have to power up the codec. > > > 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. > > Yeah, the complexity question is still valid. > > However, in this runtime PM suspend case, the system doesn't react to change > the configuration for auto-mute, etc, at this point. It just needs to send a > notification to the user-space. So, it sends a few GET_PIN_SENSE verbs, then > updates the kctl / input-jack stuff -- that's all it needs. > > For example, suppose that we have a function to execute the verb without the > power up/down sequence, e.g. snd_hda_codec_read_no_pm(). > Then change snd_hda_codec_read() call with this one if codec->epss is true. > > Of course, it won't suffice to assure that it's executed in the proper power > state, so the end result might become too complex. Needs more > investigations. > > > Takashi