[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 10:02 PM
> To: Wang, Xingchao
> Cc: David Henningsson; 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 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 at suse.de]
> > > > Sent: Wednesday, July 24, 2013 6:49 PM
> > > > To: Wang, Xingchao
> > > > Cc: David Henningsson; 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 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.
> > >
> > > 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?
> 
Yes, once Jack event reported to userspace, audio driver enter runtime_suspend quickly.
But I did not track the power refcount changing in the test.

thanks
--xingchao
> 
> Takashi