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

[Shi, Yang A]

>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
>Sent: Tuesday, April 10, 2018 6:57 PM
>To: Shi, Yang A <yang.a.shi at intel.com>; Chris Wilson <chris at chris-wilson.co.uk>;
>intel-gfx at lists.freedesktop.org
>Cc: He, Bo <bo.he at intel.com>; De Marchi, Lucas <lucas.demarchi at intel.com>
>Subject: RE: [Intel-gfx] [PATCH 1/1] drm/i915: move audio component intialization
>before audio driver use it
>
>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.
i915_load_modeset_init->intel_power_domains_init_hw->bxt_display_core_init->bxt_init_cdclk
dev_priv->cdclk.hw.cdclk will be initialized here.
It happened before driver enter into intel_modeset_init.
So it should be safe to be used from audio driver.

>
>> 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.
>
No, display driver consume too much time to do initialization when it don't connect any display
Output device.It influence whole audio driver and cause whole audio driver can't works well.
This don't make sense.

>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.
This is the issue in audio driver itself. Need be checked by audio driver owner.

BR.
Yang.

Yang Shi, Intel Open Source Technology Center
>
>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.
>
>BR,
>Jani.
>
>--
>Jani Nikula, Intel Open Source Technology Center