Re: [RFC] i915: make the probe asynchronous

From: Takashi Iwai <tiwai@suse.de>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Feng Tang <feng.tang@intel.com>,
	Jani Nikula <jani.nikula@intel.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	intel-gfx@lists.freedesktop.org, alek.du@intel.com
Subject: Re: [RFC] i915: make the probe asynchronous
Date: Wed, 20 Jun 2018 11:45:25 +0200	[thread overview]
Message-ID: <s5h4lhxokp6.wl-tiwai@suse.de> (raw)
In-Reply-To: <20180620080242.GB7186@phenom.ffwll.local>

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@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)?


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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
