[alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
Wang, Xingchao
xingchao.wang at intel.com
Mon May 13 13:55:14 CEST 2013
Hi David,
> -----Original Message-----
> From: alsa-devel-bounces at alsa-project.org
> [mailto:alsa-devel-bounces at alsa-project.org] On Behalf Of David Henningsson
> Sent: Monday, May 13, 2013 4:29 PM
> To: Wang Xingchao
> Cc: alsa-devel at alsa-project.org; daniel at ffwll.ch; tiwai at suse.de; Lin,
> Mengdong; intel-gfx at lists.freedesktop.org; Li, Jocelyn; Barnes, Jesse;
> Girdwood, Liam R; Zanoni, Paulo R
> Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API
> existense before calling
>
> On 05/13/2013 09:37 AM, Wang Xingchao wrote:
> > I915 module maybe loaded after snd_hda_intel, the power-well API
> > doesnot exist in such case. This patch intended to avoid loading
> > dependency between snd-hda-intel and i915 module.
>
> Hi Xingchao and thanks for working on this.
>
> This patch seems to re-do some of the work done in other patches (a lot of lines
> removed), so it's a little hard to follow. But I'll try to write some overall
> comments on how I have envisioned things...
>
> 1. I don't think there's any need to create an additional kernel module, we can
> just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if
> DRM_I915 is defined.
>
> 2. We don't need an IS_HSW macro on the audio side. Instead declare a new
> AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
>
> 3. Somewhere in the beginning of the probing in hda_intel.c, we'll need
> something like:
>
> if (driver_caps & AZX_DCAPS_NEED_I915_POWER)
> hda_i915_init(chip);
>
> 4. The hda_i915_init does not need to be exported (they're now both in the
> same module). hda_i915.h could have something like:
>
> #ifdef DRM_I915
> void hda_i915_init(chip);
> #else
> #define hda_i915_init(chip) do {} while(0) #endif
Thanks your suggestions. Will change them in next version.
>
> 5. You're saying this patch is about avoid loading dependency between
> snd-hda-intel and i915 module. That does not make sense to me, since the
> powerwell API is in the i915 module, snd-hda-intel must load it when it wants to
> enable power on haswell, right? Thus there is a loading dependency. If the i915
> module is not loaded at that point, we must wait for it to load, so we can have
> proper power, instead of continuing probing without the power well?
>
If i915 module not loaded, snd-had-intel will load it in current code.
The question is the tolerant delay of waiting for i915 loading.
Continuing probing would immediately fail.
Thanks
--xingchao
> >
> > Signed-off-by: Wang Xingchao <xingchao.wang at linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_dma.c | 3 ++
> > drivers/gpu/drm/i915/intel_drv.h | 2 ++
> > drivers/gpu/drm/i915/intel_pm.c | 12 +++++++
> > include/drm/i915_powerwell.h | 1 +
> > sound/pci/hda/hda_i915.c | 69
> ++++++++++++++++++++++++--------------
> > sound/pci/hda/hda_i915.h | 5 +++
> > sound/pci/hda/hda_intel.c | 8 +++--
> > 7 files changed, 72 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c index a1648eb..307ca4b 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1671,6 +1671,9 @@ int i915_driver_load(struct drm_device *dev,
> unsigned long flags)
> > if (IS_GEN5(dev))
> > intel_gpu_ips_init(dev_priv);
> >
> > + if (IS_HASWELL(dev))
> > + intel_gpu_audio_init();
> > +
> > return 0;
> >
> > out_gem_unload:
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index dfcf546..f159f12 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -758,6 +758,8 @@ extern void intel_update_fbc(struct drm_device
> *dev);
> > /* IPS */
> > extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
> > extern void intel_gpu_ips_teardown(void);
> > +/* Display audio */
> > +extern void intel_gpu_audio_init(void);
> >
> > extern bool intel_using_power_well(struct drm_device *dev);
> > extern void intel_init_power_well(struct drm_device *dev); diff
> > --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c index 642c4b6..8c1df21 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -29,6 +29,7 @@
> > #include "i915_drv.h"
> > #include "intel_drv.h"
> > #include "../../../platform/x86/intel_ips.h"
> > +#include "../../../../sound/pci/hda/hda_i915.h"
> > #include <linux/module.h>
> >
> > #define FORCEWAKE_ACK_TIMEOUT_MS 2
> > @@ -4393,6 +4394,17 @@ struct i915_power_well_user {
> > struct list_head list;
> > };
> >
> > +void intel_gpu_audio_init(void)
> > +{
> > + void (*link)(void);
> > +
> > + link = symbol_get(audio_link_to_i915_driver);
> > + if (link) {
> > + link();
> > + symbol_put(audio_link_to_i915_driver);
> > + }
> > +}
> > +
> > void i915_request_power_well(const char *name)
> > {
> > struct i915_power_well_user *user;
> > diff --git a/include/drm/i915_powerwell.h
> > b/include/drm/i915_powerwell.h index 87e0a2e..de03dc8 100644
> > --- a/include/drm/i915_powerwell.h
> > +++ b/include/drm/i915_powerwell.h
> > @@ -33,6 +33,7 @@
> > #ifndef _I915_POWERWELL_H_
> > #define _I915_POWERWELL_H_
> >
> > +/* For use by hda_i915 driver */
> > extern void i915_request_power_well(const char *name);
> > extern void i915_release_power_well(const char *name);
> >
> > diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index
> > edf1a08..d11f255 100644
> > --- a/sound/pci/hda/hda_i915.c
> > +++ b/sound/pci/hda/hda_i915.c
> > @@ -22,54 +22,71 @@
> > #include <drm/i915_powerwell.h>
> > #include "hda_i915.h"
> >
> > -const char *name = "i915";
> > -static void (*get_power)(const char *name); -static void
> > (*put_power)(const char *name);
> > +char *module_name = "i915";
> > +void (*get_power)(const char *);
> > +void (*put_power)(const char *);
> > +static bool hsw_i915_load;
> > +
> > +void audio_link_to_i915_driver(void)
> > +{
> > + /* it's time to try getting the symbols again.*/
> > + hsw_i915_load = true;
> > +}
> > +EXPORT_SYMBOL_GPL(audio_link_to_i915_driver);
> >
> > /* Power well has impact on Haswell controller and codecs */
> > void hda_display_power(bool enable)
> > {
> > - snd_printk(KERN_INFO "HDA display power %s \n", enable ? "Enable" :
> "Disable");
> > -
> > - if (!get_power || !put_power)
> > - return;
> > + if (!get_power || !put_power) {
> > + if (hsw_i915_load) {
> > + get_power = i915_request_power_well;
> > + put_power = i915_release_power_well;
> > + } else
> > + return;
> > + }
> >
> > + snd_printk(KERN_DEBUG "HDA display power %s \n",
> > + enable ? "Enable" : "Disable");
> > if (enable)
> > get_power("hda");
> > else
> > put_power("hda");
> > }
> > -EXPORT_SYMBOL(hda_display_power);
> > +EXPORT_SYMBOL_GPL(hda_display_power);
> >
> > -static int __init hda_i915_init(void)
> > +/* In case i915 module loaded first, the APIs are there.
> > + * Otherwise wait until i915 module notify us. */ int
> > +hda_i915_init(void)
> > {
> > - struct module *i915;
> > - mutex_lock(&module_mutex);
> > - i915 = find_module(name);
> > - mutex_unlock(&module_mutex);
> > + struct module *i915;
> >
> > - if (!i915)
> > - request_module_nowait(name);
> > + mutex_lock(&module_mutex);
> > + i915 = find_module(module_name);
> > + mutex_unlock(&module_mutex);
> >
> > - if (!try_module_get(i915))
> > - return -EFAULT;
> > + if (!i915)
> > + request_module_nowait(module_name);
> >
> > get_power = symbol_get(i915_request_power_well);
> > + if (!get_power)
> > + goto out;
> > +
> > put_power = symbol_get(i915_release_power_well);
> > + if (!put_power)
> > + goto out;
> >
> > - module_put(i915);
> > + snd_printk(KERN_DEBUG "HDA driver get symbol successfully from i915
> > +module\n");
> > +out:
> > return 0;
> > }
> > +EXPORT_SYMBOL_GPL(hda_i915_init);
> >
> > -#if 0
> > -static void __exit hda_i915_exit(void)
> > +int hda_i915_exit(void)
> > {
> > - drm_pci_exit(&driver, &i915_pci_driver);
> > + symbol_put(i915_request_power_well);
> > + symbol_put(i915_release_power_well);
> > }
> > +EXPORT_SYMBOL_GPL(hda_i915_exit);
> >
> > -module_init(hda_i915_init);
> > -module_exit(hda_i915_exit);
> > -#endif
> > -module_init(hda_i915_init);
> > -MODULE_DESCRIPTION("HDA power well");
> > +MODULE_DESCRIPTION("HDA power well For Haswell");
> > MODULE_LICENSE("GPL");
> > diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h index
> > a7e5324..b0516ab 100644
> > --- a/sound/pci/hda/hda_i915.h
> > +++ b/sound/pci/hda/hda_i915.h
> > @@ -3,8 +3,13 @@
> >
> > #ifdef CONFIG_SND_HDA_I915
> > void hda_display_power(bool enable);
> > +int hda_i915_init(void);
> > +int hda_i915_exit(void);
> > #else
> > void hda_display_power(bool enable) {}
> > +int hda_i915_init(void) {}
> > +int hda_i915_exit(void) {}
> > #endif
> >
> > +void audio_link_to_i915_driver(void);
> > #endif
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 8bb6075..431027d 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -3684,8 +3684,10 @@ static int azx_probe(struct pci_dev *pci,
> > return -ENOENT;
> > }
> >
> > - if (IS_HSW(pci))
> > + if (IS_HSW(pci)) {
> > + hda_i915_init();
> > hda_display_power(true);
> > + }
> >
> > err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, &card);
> > if (err < 0) {
> > @@ -3822,8 +3824,10 @@ static void azx_remove(struct pci_dev *pci)
> > if (card)
> > snd_card_free(card);
> > pci_set_drvdata(pci, NULL);
> > - if (IS_HSW(pci))
> > + if (IS_HSW(pci)) {
> > hda_display_power(false);
> > + hda_i915_exit();
> > + }
> > }
> >
> > /* PCI IDs */
> >
>
>
>
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
More information about the Alsa-devel
mailing list