| Message ID | 20180620062523.xpkqeknrklgrihpl@shbuild888 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, 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; >
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 :) 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; > >
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; >> >
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? -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
Hi Jani, 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(). Sorry, I'm not familiar with the i915 HDAC detailed connection, but seems that the hdac_component_master_bind() is a synchronous call (per my test), how can we add a completion inside that? Thanks, Feng > 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
On Wed, 20 Jun 2018, Feng Tang <feng.tang@intel.com> wrote: > On Wed, Jun 20, 2018 at 10:11:34AM +0300, Jani Nikula wrote: >> I suggested you do that within hdac_i915.c. Wait in snd_hdac_i915_init() >> below request_module(), complete in hdac_component_master_bind(). > > Sorry, I'm not familiar with the i915 HDAC detailed connection, but seems that > the hdac_component_master_bind() is a synchronous call (per my test), how > can we add a completion inside that? See Takashi's patch. BR, Jani.
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;