[Intel-gfx] [PATCH 1/1] drm/i915: move audio component intialization before audio driver use it

[Jani Nikula]

On Tue, 10 Apr 2018, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> On Tue, 10 Apr 2018, "Shi, Yang A" <yang.a.shi at intel.com> wrote:
>>>On Tue, 10 Apr 2018, "Shi, Yang A" <yang.a.shi at intel.com> wrote:
>>>> issue: snd_soc_skl meet "failed to add i915 component master (-19)"
>>>> when platform don't connect any display output.
>>>>
>>>> i915 do initialization before than skl_probe, but if there is no
>>>> display output connect, in function drm_dp_dpcd_access, there is a 32
>>>> retry for aux i2c transactions. It will meet timeout and do usleep.
>>>> Then skl_probe function will be scheduled. It will call
>>>> snd_hdac_i915_init, and it will meet "failed to add i915 component
>>>> master" error.  And whole snd_soc_skl initialization will be failed,
>>>> audio can't work normally anymore.
>>>>
>>>> So i915 driver need to move intel_audio_init at the beginning of
>>>> intel_modeset_init. This will make sure i915_audio_component_init
>>>> process before snd_hdac_i915_init call it.
>>>
>>>We do intel_audio_init() and register the audio component when we are
>>>ready to handle the audio component calls. We are ready at
>>>i915_driver_register(). We are not ready at intel_modeset_init().
>>>
>>>BR,
>>>Jani.
>>
>> Thanks to comments my patch.
>> After I check the whole driver code, I think all ops in
>> i915_audio_component_ops should be ready at the beginning of function
>> intel_modeset_init. So can we move intel_audio_init as early as we
>> can.
>
> No, that's not true. Just as an example, dev_priv->cdclk.hw.cdclk hasn't
> been initialized.
>
>> Would you like to suggest a better place to do intel_audio_init?
>
> I think the call is already where it is supposed to be. We expose
> ourselves to the rest of the system when we are ready. If it takes long,
> it takes long. I think you have a race in your driver, and you need to
> deal with it properly in your driver.
>
> In snd_hdac_i915_init(), I don't think there are any guarantees that the
> request_module() call is the one actually probing i915. We might already
> be mid-probe. You don't even check or log request_module() return value.
>
> I'm also not 100% sure at what point of driver loading request_module()
> returns. I think it's when the module init hook returns, which should be
> all right, but again, I don't think you can count on that if it isn't
> your request_module() that actually probes i915.
>
> I think the patch at hand is a hack that reduces the window for the
> race, and not a real fix. Moreover, it makes the i915 audio component
> code fragile by introducing tricky probe order dependencies that we've
> been systematically trying to reduce by placing the call where it is
> now.
>
> Cc: Lucas for any further input on module probing.

Apparently there was also a bug in some version of kmod/modprobe which
could have lead to what you're experiencing. Are you running the fixed
version? See [1].

BR,
Jani.


[1] https://github.com/lucasdemarchi/kmod/commit/fd44a98ae2eb5eb32161088954ab21e58e19dfc4


-- 
Jani Nikula, Intel Open Source Technology Center