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

Wed Jun 20 08:02:42 UTC 2018

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?
-Daniel

> 
> BR,
> Jani.
> 
> >
> > In my own debug patch, I had put a 
> > #ifndef CONFIG_DRM_I915
> > static inline int  wait_i915_probe_done() {return -ENODEV;}
> > #else
> > ....
> > #endif
> >
> > Is this fine?
> >
> > btw, for hdac_i915.c, if it is already bound with i915, can we make
> > it an separte module to dedpend on i915?
> >
> > Thanks,
> > Feng
> >
> >> 
> >> 
> >> thanks,
> >> 
> >> Takashi
> >> 
> >> > 
> >> > - Feng
> >> > 
> >> > 
> >> > ------
> >> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> >> > index 4364922..16a59ae 100644
> >> > --- a/drivers/gpu/drm/i915/i915_pci.c
> >> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> >> > @@ -671,6 +671,21 @@ static const struct pci_device_id pciidlist[] = {
> >> >  };
> >> >  MODULE_DEVICE_TABLE(pci, pciidlist);
> >> >  
> >> > +static struct completion i915_probe_done;
> >> > +
> >> > +int wait_i915_probe_done(int timeout)
> >> > +{
> >> > +	int ret;
> >> > +
> >> > +	ret = wait_for_completion_interruptible_timeout(&i915_probe_done, timeout);
> >> > +	if (ret <= 0)
> >> > +		return -ENODEV;
> >> > +
> >> > +	return 0;
> >> > +}
> >> > +EXPORT_SYMBOL_GPL(wait_i915_probe_done);
> >> > +
> >> > +
> >> >  static void i915_pci_remove(struct pci_dev *pdev)
> >> >  {
> >> >  	struct drm_device *dev = pci_get_drvdata(pdev);
> >> > @@ -717,6 +732,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >> >  		return err > 0 ? -ENOTTY : err;
> >> >  	}
> >> >  
> >> > +	complete_all(&i915_probe_done);
> >> >  	return 0;
> >> >  }
> >> >  
> >> > @@ -726,6 +742,7 @@ static struct pci_driver i915_pci_driver = {
> >> >  	.probe = i915_pci_probe,
> >> >  	.remove = i915_pci_remove,
> >> >  	.driver.pm = &i915_pm_ops,
> >> > +	.driver.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> >> >  };
> >> >  
> >> >  static int __init i915_init(void)
> >> > @@ -755,6 +772,8 @@ static int __init i915_init(void)
> >> >  		return 0;
> >> >  	}
> >> >  
> >> > +	init_completion(&i915_probe_done);
> >> > +
> >> >  	return pci_register_driver(&i915_pci_driver);
> >> >  }
> >> >  
> >> > diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> >> > index c9e5a66..adae172 100644
> >> > --- a/include/drm/i915_drm.h
> >> > +++ b/include/drm/i915_drm.h
> >> > @@ -36,6 +36,12 @@ extern bool i915_gpu_lower(void);
> >> >  extern bool i915_gpu_busy(void);
> >> >  extern bool i915_gpu_turbo_disable(void);
> >> >  
> >> > +/*
> >> > + * For use by HDA driver for now
> >> > + * Return 0 on i915 probe is done, and -ENODEV on error
> >> > + */
> >> > +extern int wait_i915_probe_done(int timeout);
> >> > +
> >> >  /* Exported from arch/x86/kernel/early-quirks.c */
> >> >  extern struct resource intel_graphics_stolen_res;
> >> >  
> >> > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> >> > index cbe818e..48e5039 100644
> >> > --- a/sound/hda/hdac_i915.c
> >> > +++ b/sound/hda/hdac_i915.c
> >> > @@ -17,6 +17,7 @@
> >> >  #include <linux/pci.h>
> >> >  #include <linux/component.h>
> >> >  #include <drm/i915_component.h>
> >> > +#include <drm/i915_drm.h>
> >> >  #include <sound/core.h>
> >> >  #include <sound/hdaudio.h>
> >> >  #include <sound/hda_i915.h>
> >> > @@ -357,6 +358,7 @@ static bool i915_gfx_present(void)
> >> >   *
> >> >   * Returns zero for success or a negative error code.
> >> >   */
> >> > +#define HDAC_WAIT_I915_LOAD_MS	3000
> >> >  int snd_hdac_i915_init(struct hdac_bus *bus)
> >> >  {
> >> >  	struct component_match *match = NULL;
> >> > @@ -386,7 +388,11 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
> >> >  	 * Atm, we don't support deferring the component binding, so make sure
> >> >  	 * i915 is loaded and that the binding successfully completes.
> >> >  	 */
> >> > -	request_module("i915");
> >> > +	ret = wait_i915_probe_done(HDAC_WAIT_I915_LOAD_MS);
> >> > +	if (ret) {
> >> > +		dev_info(dev, "failed to wait for i915 probe done\n");
> >> > +		goto out_master_del;
> >> > +	}
> >> >  
> >> >  	if (!acomp->ops) {
> >> >  		ret = -ENODEV;
> >> > 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

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