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

[Takashi Iwai]

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.
> 
> 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.

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