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

[Takashi Iwai <tiwai@suse.de>]

At Wed, 24 Jul 2013 15:52:16 +0200,
Takashi Iwai wrote:
> 
> At Wed, 24 Jul 2013 13:40:15 +0000,
> Wang, Xingchao wrote:
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, July 24, 2013 6:49 PM
> > > To: Wang, Xingchao
> > > Cc: David Henningsson; Wang Xingchao; alsa-devel@alsa-project.org;
> > > Girdwood, Liam R
> > > Subject: Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm
> > > 
> > > 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.
> > 
> > Thanks for clarification on the internal logic, Takashi.
> > I will modify the patch and do some test tormorrow when I'm at office.
> 
> Well, it might be that reusing the jackpoll would be simpler in the
> end.  But, even if it's so, give a better description in the first
> patch.  It doesn't clarify _why_ it's needed.  And need to clarify
> that it doesn't give any ill-effect, too.
> 
> (And actually there will be an impact by that change; patch_via.c
>  changes the jackpoll_interval dynamically, and the work will be still
>  pending with jackpoll_interval=0.  This code should be modified
>  before your patch, i.e. clearing the jackpoll_interval before
>  cancel_work_sync().)

BTW, one thing I'm not sure after reading the patch is how the runtime
power refcount is managed after this wakeup is handled.  That is, the
device goes down to the runtime suspend properly again after the pin
detection and notification is done?


Takashi