Re: [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Jaroslav Kysela <perex@perex.cz>]

Date 13.5.2013 10:59, Takashi Iwai wrote:
> At Mon, 13 May 2013 10:55:46 +0200,
> Jaroslav Kysela wrote:
>>
>> 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?
> 
> It's not allowed.  The drm/i915 must be initialized before the audio.
> And yet, we don't want this dependency in a hard way in the hda
> driver, because the driver is not only for i915 but for other vendor's
> controllers, too.
> 
> In the previous meeting, I suggested to split snd-hda-intel for
> Haswell as an alternative solution.  But this has obvious
> disadvantages, and since the dynamic symbol lookup is already used in
> a few other kernel codes, we decided to try this first.

Yes, I agree here, but I was talking about the proposed
intel_gpu_audio_init() function which creates a back-dependency to the
audio driver. It should be done using a callback to the DRM code without
adding a new cross module. The other stuff (request_module, symbol_get)
looks good, but it should be integrated to the main snd-hda-intel module
as David proposed.

						Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.