[alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Wang, Xingchao]

Hi Jaroslav,

> -----Original Message-----
> From: alsa-devel-bounces at alsa-project.org
> [mailto:alsa-devel-bounces at alsa-project.org] On Behalf Of Jaroslav Kysela
> Sent: Monday, May 13, 2013 4:56 PM
> To: David Henningsson
> Cc: alsa-devel at alsa-project.org; Girdwood, Liam R; tiwai at suse.de; Lin,
> Mengdong; intel-gfx at lists.freedesktop.org; Wang Xingchao; Li, Jocelyn;
> Barnes, Jesse; daniel at ffwll.ch; Zanoni, Paulo R
> Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API
> existense before calling
> 
> Date 13.5.2013 10:28, David Henningsson wrote:
> > 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
> >
> > 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?
> 
> Looking to the code, if the drm code requires a callback to the audio code, I
> would just register it in the audio driver init phase and unregister it when
> snd-hda-intel is unloaded. This cross module loading dependency looks really
> bad. Or it is not allowed to load the audio driver later than DRM one?

I think the dependency here is not real. In case the i915 loaded first, the callback
doesnot exist and do nothing, it's safe. In case snd-hda-intel loaded first, and need
pause there to wait i915 module loading, this callback is helpful to notify snd-hda-intel
the proper time to check symbol again.

Thanks
--xingchao