>-----Original Message----- >From: De Marchi, Lucas >Sent: Wednesday, April 11, 2018 9:37 AM >To: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> >Cc: Shi, Yang A <yang.a.shi@xxxxxxxxx>; Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>; >intel-gfx@xxxxxxxxxxxxxxxxxxxxx; He, Bo <bo.he@xxxxxxxxx>; Deak, Imre ><imre.deak@xxxxxxxxx> >Subject: Re: [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@xxxxxxxxxxxxxxx> wrote: >> > On Tue, 10 Apr 2018, "Shi, Yang A" <yang.a.shi@xxxxxxxxx> wrote: >> >>>On Tue, 10 Apr 2018, "Shi, Yang A" <yang.a.shi@xxxxxxxxx> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
