On 05/13/2013 03:50 PM, Wang, Xingchao wrote:
-----Original Message-----
From: David Henningsson [mailto:david.hennings...@canonical.com]
Sent: Monday, May 13, 2013 8:13 PM
To: Wang, Xingchao
Cc: Wang Xingchao; alsa-de...@alsa-project.org; dan...@ffwll.ch;
ti...@suse.de; Lin, Mengdong; intel-gfx@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 01:55 PM, Wang, Xingchao wrote:
Hi David,
-----Original Message-----
From: alsa-devel-boun...@alsa-project.org
[mailto:alsa-devel-boun...@alsa-project.org] On Behalf Of David
Henningsson
Sent: Monday, May 13, 2013 4:29 PM
To: Wang Xingchao
Cc: alsa-de...@alsa-project.org; dan...@ffwll.ch; ti...@suse.de; Lin,
Mengdong; intel-gfx@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
Or perhaps even better
static inline void hda_i915_init(azx *chip) {}
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.
Could you explain in more detail, what you mean with "tolerant delay"
and what will happen if you exceed that delay?
You said " If the i915 module is not loaded at that point, we must wait for it to
load".
I'm not clear about the routine, how long will snd-hda-intel wait? What would
happen if loading i915 too long time?
How will snd-hda-intel get to know the i915 loading finished?
I'm not experienced in module loading either, but I believe
"request_module" load the i915 driver and not return until i915 has
finished loading?
Note: You should get rid of the backwards reference (as Jaroslav pointed
out), because not only is it not needed, it could potentially cause a
deadlock if the i915 and snd-hda-intel modules both try to load each other.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
