[Intel-gfx] [RFC] i915: make the probe asynchronous

[Daniel Vetter]

On Wed, Jun 20, 2018 at 11:45:25AM +0200, Takashi Iwai wrote:
> On Wed, 20 Jun 2018 10:02:42 +0200,
> Daniel Vetter wrote:
> > 
> > On Wed, Jun 20, 2018 at 10:11:34AM +0300, Jani Nikula wrote:
> > > On Wed, 20 Jun 2018, Feng Tang <feng.tang at intel.com> wrote:
> > > > Hi Takashi, 
> > > >
> > > > On Wed, Jun 20, 2018 at 08:35:05AM +0200, Takashi Iwai wrote:
> > > >> On Wed, 20 Jun 2018 08:25:23 +0200,
> > > >> Feng Tang wrote:
> > > >> > 
> > > >> > Hi Jani/Chris/Takashi, 
> > > >> > 
> > > >> > On Wed, Jun 06, 2018 at 11:21:43AM +0300, Jani Nikula wrote:
> > > >> > > >> 
> > > >> > > >> http://patchwork.freedesktop.org/patch/msgid/20180323083048.13327-1-chris@chris-wilson.co.uk
> > > >> > > >
> > > >> > > > IIUC, you are waiting for the HDA audio driver to first handle the
> > > >> > > > i915 sync probel case?
> > > >> > > 
> > > >> > > I wouldn't hold my breath waiting. If you want to do i915 async probe,
> > > >> > > you'll probably have to figure hda out as well.
> > > >> > 
> > > >> > I made the following patch based on 4.18-rc1, and tested on my machine,
> > > >> > which basically works fine with Async probe taking effect (I tried
> > > >> > builtin and modules way).
> > > >> > 
> > > >> > Please review this initial version, and I'll separate to clean patches
> > > >> > if you think it's OK. thanks!
> > > >> 
> > > >> When you call an i915 function from HD-audio driver, you made already
> > > >> a hard dependency.  This is exactly what we want to avoid.
> > > >
> > > > Thanks for the info, I see your intension now.
> > > >
> > > > In previous discussion, Jani suggested to use a completion to indicate
> > > > the probe done, I can't figure out another way :)
> > > 
> > > I suggested you do that within hdac_i915.c. Wait in snd_hdac_i915_init()
> > > below request_module(), complete in hdac_component_master_bind().
> > 
> > I thgouth this kind of cross-driver dependency is supposed to be handled
> > using EPROBE_DEFER? Why do we need to hand-roll that here?
> > 
> > Or is this some other kind of synchronization need that needs a bespoke
> > solution?
> 
> The binding with i915 from HD-audio controller is done in an async
> work invoked from the probe callback.  It makes harder to deal with
> EPROBEDEFER.
> 
> IMO, a saner way would be to rather wait for the component binding.
> The component mechanism is there just for that purpose, after all.
> 
> Does a patch like below work (totally untested)?

Yeah this looks like a reasonable first step at least. Probably need to
thread the real errno value through, and at that point probably better to
just nuke the async worker (and maybe switch all of hda over to async
driver loading instead).
-Daniel

> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -23,6 +23,7 @@
>  #include <sound/hda_register.h>
>  
>  static struct i915_audio_component *hdac_acomp;
> +static DECLARE_COMPLETION(acomp_bound);
>  
>  /**
>   * snd_hdac_set_codec_wakeup - Enable / disable HDMI/DP codec wakeup
> @@ -284,6 +285,7 @@ static int hdac_component_master_bind(struct device *dev)
>  		goto out_unbind;
>  	}
>  
> +	complete_all(&acomp_bound);
>  	return 0;
>  
>  out_unbind:
> @@ -382,11 +384,8 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
>  	if (ret < 0)
>  		goto out_err;
>  
> -	/*
> -	 * Atm, we don't support deferring the component binding, so make sure
> -	 * i915 is loaded and that the binding successfully completes.
> -	 */
>  	request_module("i915");
> +	wait_for_completion_timeout(&acomp_bound, 10000); /* 10s timeout */
>  
>  	if (!acomp->ops) {
>  		ret = -ENODEV;

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch