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

[Shi, Yang A]

>-----Original Message-----
>From: De Marchi, Lucas
>Sent: Wednesday, April 11, 2018 9:37 AM
>To: Jani Nikula <jani.nikula at linux.intel.com>
>Cc: Shi, Yang A <yang.a.shi at intel.com>; Chris Wilson <chris at chris-wilson.co.uk>;
>intel-gfx at lists.freedesktop.org; He, Bo <bo.he at intel.com>; Deak, Imre
><imre.deak at intel.com>
>Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915: move audio component intialization
>before audio driver use it
>
>On Tue, Apr 10, 2018 at 10:58:28AM -0300, Jani Nikula wrote:
>> 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.
>
>You should not really rely on "module is loaded, I can use it". It may not be bound to
>the device yet in case of async probe for example.
>You should rather wait for the service you want to use from the driver to be ready. See
>documentation of __request_module().
>
>> >>>> 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.
>
>Humn... I fail to see how the usleep() would cause any of this. If the probe is
>synchronous (see below) we an eventual call to request_module() will/should wait for
>the first probe to finish nonetheless.
>To me it seems the error is elsewhere.
>
>> >>>>
>> >>>> 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
>
>This should not be relevant in this case. Even if the module is already mid-probe,
>request_module() will only return after modprobe returns (since request_module forces
>UMH_WAIT_PROC).
>
This issue is not related to request_module. When issue happened, request_module get i915
Correctly and return 0 successfully.
It just be caused by acomp->ops is null in function snd_hdac_i915_init after it called 
Component_master_add_with_match.

If intel_audio_init can be called before it check acomp->ops. Everything will goes well.
But intel_audio_init is called too late because i915 driver go to usleep.

BR.
Yang.


>If a second call races with the first, it will try to find the module in the module list and
>end up waiting for it to complete. See add_unformed_module().
>
>> > be mid-probe. You don't even check or log request_module() return value.
>
>Checking the return value of request_module() is absolutely the first thing to do. It's
>plain broken otherwise... there are many reasons why a call to request_module() could
>fail. If your driver is being loaded from a work queue, if the modprobe binary is not
>where it should be, if there's a bug in modprobe, etc etc etc.
>
>> >
>> > 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.
>
>You can, except if the pci probe is async... In this case modprobe would return before
>the rest of the initialization. I'm not totally sure about this without giving it a deeper
>look. But then, moving the call anywhere in that function should produce the same
>results with enough tries.
>
>> >
>> > 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].
>
>Yep, this might explain as well. But only if i915 is builtin rather than a module. Is this
>the case here?
>
>Lucas De Marchi
>
>>
>> BR,
>> Jani.
>>
>>
>> [1]
>> https://github.com/lucasdemarchi/kmod/commit/fd44a98ae2eb5eb3216108895
>> 4ab21e58e19dfc4
>>
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center