From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Henningsson Subject: Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm Date: Wed, 24 Jul 2013 13:36:32 +0200 Message-ID: <51EFBC40.5040403@canonical.com> References: <1374477558-14074-1-git-send-email-xingchao.wang@linux.intel.com> <1374477558-14074-2-git-send-email-xingchao.wang@linux.intel.com> <51EE327E.1070407@canonical.com> <46B810F6945F7C4788E11DCE57EC489011850663@SHSMSX104.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by alsa0.perex.cz (Postfix) with ESMTP id 762942652C6 for ; Wed, 24 Jul 2013 13:36:35 +0200 (CEST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: "alsa-devel@alsa-project.org" , Wang Xingchao , "Girdwood, Liam R" , "Wang, Xingchao" List-Id: alsa-devel@alsa-project.org 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. 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