Re: ALSA: hda - Add power-welll support for haswell HDA

Re: ALSA: hda - Add power-welll support for haswell HDA

[Dave Jones]

On Tue, Jul 09, 2013 at 11:36:26PM +0000, Linux Kernel wrote:
 > Gitweb:     http://git.kernel.org/linus/;a=commit;h=99a2008d0b32d72dfc2a54e7be1eb698dd2e3bd6
 > Commit:     99a2008d0b32d72dfc2a54e7be1eb698dd2e3bd6
 > Parent:     5c90680e42b08a1e4a6800ca02e75ad201f8037f
 > Author:     Wang Xingchao <xingchao.wang@linux.intel.com>
 > AuthorDate: Thu May 30 22:07:10 2013 +0800
 > Committer:  Daniel Vetter <daniel.vetter@ffwll.ch>
 > CommitDate: Thu Jun 6 17:31:56 2013 +0200
 > 
 >     ALSA: hda - Add power-welll support for haswell HDA
 >     
 >     For Intel Haswell chip, HDA controller and codec have
 >     power well dependency from GPU side. This patch added support
 >     to request/release power well in audio driver. Power save
 >     feature should be enabled to get runtime power saving.
 >     
 >     There's deadlock when request_module(i915) in azx_probe.
 >     It looks like:
 >     device_lock(audio pci device) -> azx_probe -> module_request
 >     (or symbol_request) -> modprobe (userspace) -> i915 init ->
 >     drm_pci_init -> pci_register_driver -> bus_add_driver -> driver_attach ->
 >     which in turn tries all locks on pci bus, and when it tries the one on the
 >     audio device, it will deadlock.
 >     
 >     This patch introduce a work to store remaining probe stuff, and let
 >     request_module run in safe work context.

That deadlock sounds nasty, but..

 > +#ifdef CONFIG_SND_HDA_I915
 > +		probe_now = false;
 > +		schedule_work(&chip->probe_work);
 > +#else
 > +		snd_printk(KERN_ERR SFX "Haswell must build in CONFIG_SND_HDA_I915\n");
 > +#endif

runtime surprises like this are also a pain. There has to be a way in Kconfig
to express this (through addition of a new option). Though having to build this
in for distro kernels is also unfortunate. This means we'll also have to build-in
all the ALSA stuff even for users who never use it.

	Dave

Re: ALSA: hda - Add power-welll support for haswell HDA

[Takashi Iwai]

At Mon, 29 Jul 2013 16:12:08 -0400,
Dave Jones wrote:
> 
> On Tue, Jul 09, 2013 at 11:36:26PM +0000, Linux Kernel wrote:
>  > Gitweb:     http://git.kernel.org/linus/;a=commit;h=99a2008d0b32d72dfc2a54e7be1eb698dd2e3bd6
>  > Commit:     99a2008d0b32d72dfc2a54e7be1eb698dd2e3bd6
>  > Parent:     5c90680e42b08a1e4a6800ca02e75ad201f8037f
>  > Author:     Wang Xingchao <xingchao.wang@linux.intel.com>
>  > AuthorDate: Thu May 30 22:07:10 2013 +0800
>  > Committer:  Daniel Vetter <daniel.vetter@ffwll.ch>
>  > CommitDate: Thu Jun 6 17:31:56 2013 +0200
>  > 
>  >     ALSA: hda - Add power-welll support for haswell HDA
>  >     
>  >     For Intel Haswell chip, HDA controller and codec have
>  >     power well dependency from GPU side. This patch added support
>  >     to request/release power well in audio driver. Power save
>  >     feature should be enabled to get runtime power saving.
>  >     
>  >     There's deadlock when request_module(i915) in azx_probe.
>  >     It looks like:
>  >     device_lock(audio pci device) -> azx_probe -> module_request
>  >     (or symbol_request) -> modprobe (userspace) -> i915 init ->
>  >     drm_pci_init -> pci_register_driver -> bus_add_driver -> driver_attach ->
>  >     which in turn tries all locks on pci bus, and when it tries the one on the
>  >     audio device, it will deadlock.
>  >     
>  >     This patch introduce a work to store remaining probe stuff, and let
>  >     request_module run in safe work context.
> 
> That deadlock sounds nasty, but..
> 
>  > +#ifdef CONFIG_SND_HDA_I915
>  > +		probe_now = false;
>  > +		schedule_work(&chip->probe_work);
>  > +#else
>  > +		snd_printk(KERN_ERR SFX "Haswell must build in CONFIG_SND_HDA_I915\n");
>  > +#endif
> 
> runtime surprises like this are also a pain. There has to be a way in Kconfig
> to express this (through addition of a new option).

Well, sound/pci/hda/Kconfig gives a warning:
  Note that this option must be enabled for Intel Haswell C+ stepping
  machines, otherwise the GPU audio controller/codecs will not be
  initialized or damaged when exit from S3 mode.

I guess "damaged" is a bit too strong word, though.  So the driver
still continues with just an error message.  It can be changed to stop
loading and return the error, too, if such behavior is preferred.


> Though having to build this
> in for distro kernels is also unfortunate. This means we'll also have to build-in
> all the ALSA stuff even for users who never use it.

We discussed about it, too, and that's the reason why the driver tries
to bind dynamically with symbol_request().  The glue code is fairly
small (found in hda_i915.c).


thanks,

Takashi