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

[Wang, Xingchao]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Wednesday, July 24, 2013 8:24 PM
> To: David Henningsson
> Cc: Wang, Xingchao; Wang Xingchao; alsa-devel at 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 <xingchao.wang at 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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ALSA-hda-Jack-hotplug-support-in-D3.patch
Type: application/octet-stream
Size: 5837 bytes
Desc: 0001-ALSA-hda-Jack-hotplug-support-in-D3.patch
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20130725/6b1cf12d/attachment-0001.obj>