Re: [RFC] i915: make the probe asynchronous

From: Jani Nikula <jani.nikula@intel.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: feng.tang@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 10:11:34 +0300	[thread overview]
Message-ID: <87bmc6vsnt.fsf@intel.com> (raw)
In-Reply-To: <20180620064701.nsa4nw56ro5dirp6@shbuild888>

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().

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