[RFC] i915: make the probe asynchronous

[RFC] i915: make the probe asynchronous

[Feng Tang]

i915 driver's probe is one of the longest of pci devices, which takes
about hundreds of microseconds or more, make the probe async will help
much on the kernel boot time, as different driver's probe can go async.

This have been limited verified on several platforms of mine, don't
know if it will have other side effects and drawbacks, so I would
throw it out for reviews and comments, thanks

Signed-off-by: Feng Tang <feng.tang@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 062e91b39085..5db3080101be 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -725,6 +725,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)
-- 
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for i915: make the probe asynchronous

[Patchwork]

== Series Details ==

Series: i915: make the probe asynchronous
URL   : https://patchwork.freedesktop.org/series/44159/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4275 -> Patchwork_9181 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/44159/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9181 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-cnl-y3:          PASS -> INCOMPLETE (fdo#105086)

    igt@gem_exec_suspend@basic-s4-devices:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#105719)

    igt@kms_flip@basic-flip-vs-dpms:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000, fdo#106097)

    igt@prime_vgem@basic-fence-flip:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000)

    
    ==== Possible fixes ====

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       DMESG-WARN (fdo#106000) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS
      fi-cnl-psr:         DMESG-WARN (fdo#104951) -> PASS

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#104951 https://bugs.freedesktop.org/show_bug.cgi?id=104951
  fdo#105086 https://bugs.freedesktop.org/show_bug.cgi?id=105086
  fdo#105719 https://bugs.freedesktop.org/show_bug.cgi?id=105719
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097


== Participating hosts (40 -> 38) ==

  Additional (2): fi-hsw-peppy fi-skl-guc 
  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4275 -> Patchwork_9181

  CI_DRM_4275: 8fdb62e0511e81fa935059c274a2457361fdb679 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4505: 8a8f0271a71e2e0d2a2caa4d41f4ad1d9c89670e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9181: 49fc278d0b790d95a8ce5027baf63e8cf646e4c5 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

49fc278d0b79 i915: make the probe asynchronous

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9181/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for i915: make the probe asynchronous

[Patchwork]

== Series Details ==

Series: i915: make the probe asynchronous
URL   : https://patchwork.freedesktop.org/series/44159/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4275_full -> Patchwork_9181_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9181_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9181_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/44159/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9181_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-blt:
      shard-kbl:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_9181_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_objects:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          PASS -> FAIL (fdo#106509, fdo#105454)

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#102887)

    igt@kms_flip@2x-plain-flip-ts-check:
      shard-hsw:          PASS -> FAIL (fdo#103928)
      shard-glk:          PASS -> FAIL (fdo#100368)

    igt@kms_flip_tiling@flip-x-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      shard-hsw:          PASS -> FAIL (fdo#103481)

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    igt@pm_rpm@debugfs-forcewake-user:
      shard-glk:          PASS -> INCOMPLETE (fdo#103359, k.org#198133) +1

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@gem_eio@suspend:
      shard-snb:          DMESG-FAIL -> PASS

    igt@kms_flip@2x-dpms-vs-vblank-race:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      shard-hsw:          FAIL (fdo#103928) -> PASS

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS +1

    igt@kms_flip@plain-flip-ts-check:
      shard-hsw:          FAIL (fdo#100368) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4275 -> Patchwork_9181

  CI_DRM_4275: 8fdb62e0511e81fa935059c274a2457361fdb679 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4505: 8a8f0271a71e2e0d2a2caa4d41f4ad1d9c89670e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9181: 49fc278d0b790d95a8ce5027baf63e8cf646e4c5 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9181/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Jani Nikula]

On Mon, 04 Jun 2018, Feng Tang <feng.tang@intel.com> wrote:
> i915 driver's probe is one of the longest of pci devices, which takes
> about hundreds of microseconds or more, make the probe async will help
> much on the kernel boot time, as different driver's probe can go async.
>
> This have been limited verified on several platforms of mine, don't
> know if it will have other side effects and drawbacks, so I would
> throw it out for reviews and comments, thanks

See the previous discussion [1].

BR,
Jani.

[1] http://mid.mail-archive.com/20180323083048.13327-1-chris@chris-wilson.co.uk

>
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_pci.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 062e91b39085..5db3080101be 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -725,6 +725,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)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

Hi Jani,

On Tue, Jun 05, 2018 at 10:41:04AM +0300, Jani Nikula wrote:
> On Mon, 04 Jun 2018, Feng Tang <feng.tang@intel.com> wrote:
> > i915 driver's probe is one of the longest of pci devices, which takes
> > about hundreds of microseconds or more, make the probe async will help
> > much on the kernel boot time, as different driver's probe can go async.
> >
> > This have been limited verified on several platforms of mine, don't
> > know if it will have other side effects and drawbacks, so I would
> > throw it out for reviews and comments, thanks
> 
> See the previous discussion [1].

Thanks! But cannot access the link :( 

I've googled, but without luck, is there other link? or some other
keywords so that I can google with?

Thanks,
Feng

> 
> BR,
> Jani.
> 
> [1] http://mid.mail-archive.com/20180323083048.13327-1-chris@chris-wilson.co.uk
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Jani Nikula]

On Tue, 05 Jun 2018, Feng Tang <feng.tang@intel.com> wrote:
> Hi Jani,
>
> On Tue, Jun 05, 2018 at 10:41:04AM +0300, Jani Nikula wrote:
>> On Mon, 04 Jun 2018, Feng Tang <feng.tang@intel.com> wrote:
>> > i915 driver's probe is one of the longest of pci devices, which takes
>> > about hundreds of microseconds or more, make the probe async will help
>> > much on the kernel boot time, as different driver's probe can go async.
>> >
>> > This have been limited verified on several platforms of mine, don't
>> > know if it will have other side effects and drawbacks, so I would
>> > throw it out for reviews and comments, thanks
>> 
>> See the previous discussion [1].
>
> Thanks! But cannot access the link :( 
>
> I've googled, but without luck, is there other link? or some other
> keywords so that I can google with?

Works for me, here's another:

http://patchwork.freedesktop.org/patch/msgid/20180323083048.13327-1-chris@chris-wilson.co.uk

BR,
Jani.


>
> Thanks,
> Feng
>
>> 
>> BR,
>> Jani.
>> 
>> [1] http://mid.mail-archive.com/20180323083048.13327-1-chris@chris-wilson.co.uk
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

Hi Jani,

On Tue, Jun 05, 2018 at 11:18:46AM +0300, Jani Nikula wrote:
> On Tue, 05 Jun 2018, Feng Tang <feng.tang@intel.com> wrote:
> > Hi Jani,
> >
> > On Tue, Jun 05, 2018 at 10:41:04AM +0300, Jani Nikula wrote:
> >> On Mon, 04 Jun 2018, Feng Tang <feng.tang@intel.com> wrote:
> >> > i915 driver's probe is one of the longest of pci devices, which takes
> >> > about hundreds of microseconds or more, make the probe async will help
> >> > much on the kernel boot time, as different driver's probe can go async.
> >> >
> >> > This have been limited verified on several platforms of mine, don't
> >> > know if it will have other side effects and drawbacks, so I would
> >> > throw it out for reviews and comments, thanks
> >> 
> >> See the previous discussion [1].
> >
> > Thanks! But cannot access the link :( 
> >
> > I've googled, but without luck, is there other link? or some other
> > keywords so that I can google with?
> 
> Works for me, here's another:
> 
> http://patchwork.freedesktop.org/patch/msgid/20180323083048.13327-1-chris@chris-wilson.co.uk

thanks, this works now.

IIUC, you are waiting for the HDA audio driver to first handle the
i915 sync probel case?

Thanks,
Feng
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Jani Nikula]

On Wed, 06 Jun 2018, Feng Tang <feng.tang@intel.com> wrote:
> Hi Jani,
>
> On Tue, Jun 05, 2018 at 11:18:46AM +0300, Jani Nikula wrote:
>> On Tue, 05 Jun 2018, Feng Tang <feng.tang@intel.com> wrote:
>> > Hi Jani,
>> >
>> > On Tue, Jun 05, 2018 at 10:41:04AM +0300, Jani Nikula wrote:
>> >> On Mon, 04 Jun 2018, Feng Tang <feng.tang@intel.com> wrote:
>> >> > i915 driver's probe is one of the longest of pci devices, which takes
>> >> > about hundreds of microseconds or more, make the probe async will help
>> >> > much on the kernel boot time, as different driver's probe can go async.
>> >> >
>> >> > This have been limited verified on several platforms of mine, don't
>> >> > know if it will have other side effects and drawbacks, so I would
>> >> > throw it out for reviews and comments, thanks
>> >> 
>> >> See the previous discussion [1].
>> >
>> > Thanks! But cannot access the link :( 
>> >
>> > I've googled, but without luck, is there other link? or some other
>> > keywords so that I can google with?
>> 
>> Works for me, here's another:
>> 
>> http://patchwork.freedesktop.org/patch/msgid/20180323083048.13327-1-chris@chris-wilson.co.uk
>
> thanks, this works now.
>
> 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.

BR,
Jani.



>
> Thanks,
> Feng

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

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!

- 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;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Takashi Iwai]

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

✗ Fi.CI.CHECKPATCH: warning for i915: make the probe asynchronous (rev2)

[Patchwork]

== Series Details ==

Series: i915: make the probe asynchronous (rev2)
URL   : https://patchwork.freedesktop.org/series/44159/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
23766ec5a70a i915: make the probe asynchronous
-:10: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#10: 
> >> http://patchwork.freedesktop.org/patch/msgid/20180323083048.13327-1-chris@chris-wilson.co.uk

-:51: CHECK:LINE_SPACING: Please don't use multiple blank lines
#51: FILE: drivers/gpu/drm/i915/i915_pci.c:689:
+
+

-:92: CHECK:AVOID_EXTERNS: extern prototypes should be avoided in .h files
#92: FILE: include/drm/i915_drm.h:43:
+extern int wait_i915_probe_done(int timeout);

-:129: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)

total: 1 errors, 1 warnings, 2 checks, 81 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

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

✗ Fi.CI.BAT: failure for i915: make the probe asynchronous (rev2)

[Patchwork]

== Series Details ==

Series: i915: make the probe asynchronous (rev2)
URL   : https://patchwork.freedesktop.org/series/44159/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4344 -> Patchwork_9364 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9364 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9364, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/44159/revisions/2/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9364:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_module_reload@basic-no-display:
      fi-elk-e7500:       PASS -> FAIL +1
      fi-snb-2520m:       PASS -> FAIL +1
      {fi-whl-u}:         PASS -> FAIL +1
      fi-ivb-3520m:       PASS -> FAIL +1
      fi-pnv-d510:        PASS -> FAIL +1
      fi-kbl-7567u:       PASS -> FAIL +1
      fi-glk-j4005:       PASS -> FAIL
      fi-bwr-2160:        PASS -> FAIL +1
      fi-bdw-5557u:       PASS -> FAIL +1
      fi-hsw-peppy:       PASS -> FAIL +1
      fi-hsw-4770r:       PASS -> FAIL +1
      fi-skl-6260u:       PASS -> FAIL +1
      fi-snb-2600:        PASS -> FAIL +1
      fi-skl-6700k2:      PASS -> FAIL +1
      fi-skl-6770hq:      PASS -> FAIL +1
      fi-byt-n2820:       PASS -> FAIL +1
      fi-byt-j1900:       PASS -> FAIL +1
      fi-kbl-7560u:       PASS -> FAIL +1
      fi-skl-6600u:       PASS -> FAIL +1
      fi-bxt-j4205:       PASS -> FAIL +1

    igt@drv_module_reload@basic-reload:
      fi-skl-guc:         PASS -> FAIL +1
      fi-bdw-gvtdvm:      PASS -> FAIL +1
      fi-kbl-r:           PASS -> FAIL +1
      fi-gdg-551:         PASS -> FAIL +1
      fi-cfl-8700k:       PASS -> FAIL +1
      fi-bxt-dsi:         NOTRUN -> FAIL +1
      fi-hsw-4770:        PASS -> FAIL +1
      fi-cfl-guc:         PASS -> FAIL +1
      fi-ilk-650:         PASS -> FAIL +1
      fi-bsw-n3050:       PASS -> FAIL +1
      fi-ivb-3770:        PASS -> FAIL +1
      fi-cfl-s3:          PASS -> FAIL +1
      fi-skl-6700hq:      PASS -> FAIL +1
      fi-kbl-7500u:       PASS -> FAIL +1
      fi-kbl-guc:         PASS -> FAIL +1
      fi-blb-e6850:       PASS -> FAIL +1
      fi-skl-gvtdvm:      PASS -> FAIL +1

    igt@drv_module_reload@basic-reload-inject:
      fi-skl-6260u:       PASS -> INCOMPLETE
      fi-snb-2600:        PASS -> INCOMPLETE
      fi-kbl-7560u:       PASS -> INCOMPLETE
      fi-skl-6770hq:      PASS -> INCOMPLETE
      fi-bwr-2160:        PASS -> INCOMPLETE
      fi-kbl-r:           PASS -> INCOMPLETE
      fi-cfl-s3:          PASS -> INCOMPLETE
      fi-gdg-551:         PASS -> INCOMPLETE
      fi-ilk-650:         PASS -> INCOMPLETE
      fi-bsw-n3050:       PASS -> INCOMPLETE
      fi-kbl-7567u:       PASS -> INCOMPLETE
      fi-ivb-3770:        PASS -> INCOMPLETE
      {fi-whl-u}:         PASS -> INCOMPLETE
      fi-skl-6700hq:      PASS -> INCOMPLETE
      fi-kbl-7500u:       PASS -> INCOMPLETE
      fi-ivb-3520m:       PASS -> INCOMPLETE
      fi-hsw-4770:        PASS -> INCOMPLETE
      fi-bdw-5557u:       PASS -> INCOMPLETE
      fi-cfl-8700k:       PASS -> INCOMPLETE
      fi-hsw-peppy:       PASS -> INCOMPLETE
      fi-pnv-d510:        PASS -> INCOMPLETE
      fi-hsw-4770r:       PASS -> INCOMPLETE
      fi-blb-e6850:       PASS -> INCOMPLETE
      fi-skl-6700k2:      PASS -> INCOMPLETE

    
    ==== Warnings ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       DMESG-WARN (fdo#106248, fdo#106725) -> FAIL

    
== Known issues ==

  Here are the changes found in Patchwork_9364 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload-inject:
      fi-byt-j1900:       PASS -> INCOMPLETE (fdo#102657)
      fi-kbl-guc:         PASS -> INCOMPLETE (fdo#106693)
      fi-byt-n2820:       PASS -> INCOMPLETE (fdo#102657)
      fi-bxt-j4205:       PASS -> INCOMPLETE (fdo#103927)
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)
      fi-bdw-gvtdvm:      PASS -> INCOMPLETE (fdo#105600)
      fi-glk-j4005:       PASS -> INCOMPLETE (fdo#103359, k.org#198133)
      fi-bxt-dsi:         NOTRUN -> INCOMPLETE (fdo#103927)
      fi-skl-gvtdvm:      PASS -> INCOMPLETE (fdo#105600)
      fi-skl-6600u:       PASS -> INCOMPLETE (fdo#104108)
      fi-skl-guc:         PASS -> INCOMPLETE (fdo#106693)
      fi-cfl-guc:         PASS -> INCOMPLETE (fdo#106693)
      fi-elk-e7500:       PASS -> INCOMPLETE (fdo#103989)

    igt@gem_exec_gttfill@basic:
      fi-byt-n2820:       PASS -> FAIL (fdo#106744)

    igt@kms_flip@basic-flip-vs-dpms:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-glk-j4005:       DMESG-WARN (fdo#106238) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102657 https://bugs.freedesktop.org/show_bug.cgi?id=102657
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105600 https://bugs.freedesktop.org/show_bug.cgi?id=105600
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106238 https://bugs.freedesktop.org/show_bug.cgi?id=106238
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106693 https://bugs.freedesktop.org/show_bug.cgi?id=106693
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (43 -> 37) ==

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-cnl-psr 


== Build changes ==

    * Linux: CI_DRM_4344 -> Patchwork_9364

  CI_DRM_4344: 922a029a1d0ecf5c7e5c86a372a5d3df3fd35483 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4524: 9ab9268fa7eeda0a7ea6eb2ab02bb6c5b9c91ba0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9364: 23766ec5a70ac05299aa421a9a92c511b39686ec @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

23766ec5a70a i915: make the probe asynchronous

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9364/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Jani Nikula]

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

Re: [RFC] i915: make the probe asynchronous

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Takashi Iwai]

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

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

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

✗ Fi.CI.CHECKPATCH: warning for i915: make the probe asynchronous (rev3)

[Patchwork]

== Series Details ==

Series: i915: make the probe asynchronous (rev3)
URL   : https://patchwork.freedesktop.org/series/44159/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
5792ad56bb06 i915: make the probe asynchronous
-:21: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#21: 
> > >> > > >> http://patchwork.freedesktop.org/patch/msgid/20180323083048.13327-1-chris@chris-wilson.co.uk

-:100: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)

total: 1 errors, 1 warnings, 0 checks, 26 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for i915: make the probe asynchronous (rev3)

[Patchwork]

== Series Details ==

Series: i915: make the probe asynchronous (rev3)
URL   : https://patchwork.freedesktop.org/series/44159/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4346 -> Patchwork_9367 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/44159/revisions/3/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9367 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@gem_exec_gttfill@basic:
      fi-byt-n2820:       FAIL (fdo#106744) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       FAIL (fdo#100368) -> PASS

    igt@kms_flip@basic-plain-flip:
      fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744


== Participating hosts (42 -> 37) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4346 -> Patchwork_9367

  CI_DRM_4346: 92d7e0feac3acdf911aacfcc8a5df92756e2d11a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4525: 578c645406d59138029fa6ef343fcc87c2d95d4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9367: 5792ad56bb06cea462ea9be53eff6879debada8b @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

5792ad56bb06 i915: make the probe asynchronous

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9367/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Jani Nikula]

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.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for i915: make the probe asynchronous (rev3)

[Patchwork]

== Series Details ==

Series: i915: make the probe asynchronous (rev3)
URL   : https://patchwork.freedesktop.org/series/44159/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4346_full -> Patchwork_9367_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9367_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9367_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9367_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-blt:
      shard-kbl:          SKIP -> PASS +1

    
== Known issues ==

  Here are the changes found in Patchwork_9367_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_gtt:
      shard-kbl:          PASS -> FAIL (fdo#105347)

    igt@gem_exec_schedule@pi-ringfull-blt:
      shard-kbl:          NOTRUN -> FAIL (fdo#103158)

    igt@kms_draw_crc@draw-method-rgb565-blt-xtiled:
      shard-glk:          PASS -> WARN (fdo#106974)

    igt@kms_flip@plain-flip-fb-recreate:
      shard-glk:          PASS -> FAIL (fdo#100368)

    igt@kms_flip_tiling@flip-y-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-render:
      shard-snb:          PASS -> FAIL (fdo#103167, fdo#104724)

    igt@kms_plane_scaling@pipe-b-scaler-with-clipping-clamping:
      shard-glk:          PASS -> DMESG-WARN (fdo#105763) +1

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)
      shard-kbl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS

    igt@kms_cursor_legacy@cursorb-vs-flipb-toggle:
      shard-glk:          DMESG-WARN (fdo#106538, fdo#105763) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105189) -> PASS

    igt@kms_flip@flip-vs-expired-vblank:
      shard-glk:          FAIL (fdo#102887) -> PASS

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          FAIL (fdo#103822, fdo#104724) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106974 https://bugs.freedesktop.org/show_bug.cgi?id=106974
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4346 -> Patchwork_9367

  CI_DRM_4346: 92d7e0feac3acdf911aacfcc8a5df92756e2d11a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4525: 578c645406d59138029fa6ef343fcc87c2d95d4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9367: 5792ad56bb06cea462ea9be53eff6879debada8b @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9367/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

Hi,

On Wed, Jun 20, 2018 at 11:45:25AM +0200, Takashi Iwai wrote:
> > > >
> > > > 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)?

When I was testing this patch, I further checked the kernel module code,
and found that: we may need NOT to add any new codes to prepare for
i915's async probe feature!

Say when i915 module is being loader due to HDA's request_module() call,
in the callchain, do_init_module() has such code: 

    if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
            async_synchronize_full();

This will garantee the asynced probe is done before it returns.

I have just tested and this seems to be enough. If I am not wrong, then
we can take the i915 async patch directly. What do you think?

Thanks,
Feng


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

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

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

On Mon, Jun 25, 2018 at 05:36:32PM +0200, Daniel Vetter wrote:
 > 
> > 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

Hi Daneil/Jani/Takashi,

When I was testing this patch from Takashi, I further checked the kernel
module code, and found that: we may need NOT to add any new codes to
prepare for i915's async probe feature!

Say when i915 module is being loader due to HDA's request_module() call,
in the callchain, do_init_module() has such code:

    if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
                async_synchronize_full();

This will garantee the asynced probe is done before it returns.

I have just tested and this seems to be enough. If I am not wrong, then
we can take the i915 async patch directly. What do you think?

Thanks,
Feng

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

On Tue, Jun 26, 2018 at 10:29:16AM +0800, Feng Tang wrote:
> On Mon, Jun 25, 2018 at 05:36:32PM +0200, Daniel Vetter wrote:
 
> Hi Daneil/Jani/Takashi,
> 
> When I was testing this patch from Takashi, I further checked the kernel
> module code, and found that: we may need NOT to add any new codes to
> prepare for i915's async probe feature!
> 
> Say when i915 module is being loader due to HDA's request_module() call,
> in the callchain, do_init_module() has such code:
> 
>     if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
>                 async_synchronize_full();
> 
> This will garantee the asynced probe is done before it returns.
> 
> I have just tested and this seems to be enough. If I am not wrong, then
> we can take the i915 async patch directly. What do you think?

Ping for comments, thanks!

- Feng
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Daniel Vetter]

On Thu, Jul 12, 2018 at 09:29:01AM +0800, Feng Tang wrote:
> On Tue, Jun 26, 2018 at 10:29:16AM +0800, Feng Tang wrote:
> > On Mon, Jun 25, 2018 at 05:36:32PM +0200, Daniel Vetter wrote:
>  
> > Hi Daneil/Jani/Takashi,
> > 
> > When I was testing this patch from Takashi, I further checked the kernel
> > module code, and found that: we may need NOT to add any new codes to
> > prepare for i915's async probe feature!
> > 
> > Say when i915 module is being loader due to HDA's request_module() call,
> > in the callchain, do_init_module() has such code:
> > 
> >     if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
> >                 async_synchronize_full();
> > 
> > This will garantee the asynced probe is done before it returns.
> > 
> > I have just tested and this seems to be enough. If I am not wrong, then
> > we can take the i915 async patch directly. What do you think?
> 
> Ping for comments, thanks!

Ram (who's working on the hdcp2 code) just learned the hard way that if
i915 registration gets delayed then audio fails to load. So if you want to
make i915 fully async, then you _must_ fix the audio load stuff.

The above code just shows that if you're loading things with
request_module(), then there's not actually any async probing going on.
Which kinda defeats the point.

So yeah, I still think we need to fix this properly, or it's pointless.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Takashi Iwai]

On Thu, 12 Jul 2018 08:54:34 +0200,
Daniel Vetter wrote:
> 
> On Thu, Jul 12, 2018 at 09:29:01AM +0800, Feng Tang wrote:
> > On Tue, Jun 26, 2018 at 10:29:16AM +0800, Feng Tang wrote:
> > > On Mon, Jun 25, 2018 at 05:36:32PM +0200, Daniel Vetter wrote:
> >  
> > > Hi Daneil/Jani/Takashi,
> > > 
> > > When I was testing this patch from Takashi, I further checked the kernel
> > > module code, and found that: we may need NOT to add any new codes to
> > > prepare for i915's async probe feature!
> > > 
> > > Say when i915 module is being loader due to HDA's request_module() call,
> > > in the callchain, do_init_module() has such code:
> > > 
> > >     if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
> > >                 async_synchronize_full();
> > > 
> > > This will garantee the asynced probe is done before it returns.
> > > 
> > > I have just tested and this seems to be enough. If I am not wrong, then
> > > we can take the i915 async patch directly. What do you think?
> > 
> > Ping for comments, thanks!
> 
> Ram (who's working on the hdcp2 code) just learned the hard way that if
> i915 registration gets delayed then audio fails to load. So if you want to
> make i915 fully async, then you _must_ fix the audio load stuff.

Does my component completion patch help for that scenario?


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Daniel Vetter]

On Thu, Jul 12, 2018 at 8:56 AM, Takashi Iwai <tiwai@suse.de> wrote:
> On Thu, 12 Jul 2018 08:54:34 +0200,
> Daniel Vetter wrote:
>>
>> On Thu, Jul 12, 2018 at 09:29:01AM +0800, Feng Tang wrote:
>> > On Tue, Jun 26, 2018 at 10:29:16AM +0800, Feng Tang wrote:
>> > > On Mon, Jun 25, 2018 at 05:36:32PM +0200, Daniel Vetter wrote:
>> >
>> > > Hi Daneil/Jani/Takashi,
>> > >
>> > > When I was testing this patch from Takashi, I further checked the kernel
>> > > module code, and found that: we may need NOT to add any new codes to
>> > > prepare for i915's async probe feature!
>> > >
>> > > Say when i915 module is being loader due to HDA's request_module() call,
>> > > in the callchain, do_init_module() has such code:
>> > >
>> > >     if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
>> > >                 async_synchronize_full();
>> > >
>> > > This will garantee the asynced probe is done before it returns.
>> > >
>> > > I have just tested and this seems to be enough. If I am not wrong, then
>> > > we can take the i915 async patch directly. What do you think?
>> >
>> > Ping for comments, thanks!
>>
>> Ram (who's working on the hdcp2 code) just learned the hard way that if
>> i915 registration gets delayed then audio fails to load. So if you want to
>> make i915 fully async, then you _must_ fix the audio load stuff.
>
> Does my component completion patch help for that scenario?

Hm, must have missed it. Do you have a patchwork link?

Also adding Ram so he can test this out.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

Hi Daniel,

On Thu, Jul 12, 2018 at 08:54:34AM +0200, Daniel Vetter wrote:
> On Thu, Jul 12, 2018 at 09:29:01AM +0800, Feng Tang wrote:
> > On Tue, Jun 26, 2018 at 10:29:16AM +0800, Feng Tang wrote:
> > > On Mon, Jun 25, 2018 at 05:36:32PM +0200, Daniel Vetter wrote:
> >  
> > > Hi Daneil/Jani/Takashi,
> > > 
> > > When I was testing this patch from Takashi, I further checked the kernel
> > > module code, and found that: we may need NOT to add any new codes to
> > > prepare for i915's async probe feature!
> > > 
> > > Say when i915 module is being loader due to HDA's request_module() call,
> > > in the callchain, do_init_module() has such code:
> > > 
> > >     if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
> > >                 async_synchronize_full();
> > > 
> > > This will garantee the asynced probe is done before it returns.
> > > 
> > > I have just tested and this seems to be enough. If I am not wrong, then
> > > we can take the i915 async patch directly. What do you think?
> > 
> > Ping for comments, thanks!
> 
> Ram (who's working on the hdcp2 code) just learned the hard way that if
> i915 registration gets delayed then audio fails to load. So if you want to
> make i915 fully async, then you _must_ fix the audio load stuff.

Thanks for sharing this info, this is a real concern. I just did a quick
search of intel-gfx mail list archive, but failed to find the details :(

> 
> The above code just shows that if you're loading things with
> request_module(), then there's not actually any async probing going on.
> Which kinda defeats the point.
> 
> So yeah, I still think we need to fix this properly, or it's pointless.

Agree, this has to be fixed. Can we just do as the hdac_i915.c to explicitly
claim this dependency by using the similar request_module("i915"), as there
may be similar dependency on i915 in the future.

Thanks,
Feng

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

On Thu, Jul 12, 2018 at 09:37:41AM +0200, Daniel Vetter wrote:
> On Thu, Jul 12, 2018 at 8:56 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Thu, 12 Jul 2018 08:54:34 +0200,
> > Daniel Vetter wrote:
> >>
> >> On Thu, Jul 12, 2018 at 09:29:01AM +0800, Feng Tang wrote:
> >> > On Tue, Jun 26, 2018 at 10:29:16AM +0800, Feng Tang wrote:
> >> > > On Mon, Jun 25, 2018 at 05:36:32PM +0200, Daniel Vetter wrote:
> >> >
> >> > > Hi Daneil/Jani/Takashi,
> >> > >
> >> > > When I was testing this patch from Takashi, I further checked the kernel
> >> > > module code, and found that: we may need NOT to add any new codes to
> >> > > prepare for i915's async probe feature!
> >> > >
> >> > > Say when i915 module is being loader due to HDA's request_module() call,
> >> > > in the callchain, do_init_module() has such code:
> >> > >
> >> > >     if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
> >> > >                 async_synchronize_full();
> >> > >
> >> > > This will garantee the asynced probe is done before it returns.
> >> > >
> >> > > I have just tested and this seems to be enough. If I am not wrong, then
> >> > > we can take the i915 async patch directly. What do you think?
> >> >
> >> > Ping for comments, thanks!
> >>
> >> Ram (who's working on the hdcp2 code) just learned the hard way that if
> >> i915 registration gets delayed then audio fails to load. So if you want to
> >> make i915 fully async, then you _must_ fix the audio load stuff.
> >
> > Does my component completion patch help for that scenario?
> 
> Hm, must have missed it. Do you have a patchwork link?
> 
> Also adding Ram so he can test this out.

Here is Iwai's patch that I found in my inbox:

-----

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

✗ Fi.CI.CHECKPATCH: warning for i915: make the probe asynchronous (rev4)

[Patchwork]

== Series Details ==

Series: i915: make the probe asynchronous (rev4)
URL   : https://patchwork.freedesktop.org/series/44159/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
9a02b4dfc2e6 i915: make the probe asynchronous
-:17: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#17: 
> >> > > When I was testing this patch from Takashi, I further checked the kernel

-:80: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)

total: 1 errors, 1 warnings, 0 checks, 26 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for i915: make the probe asynchronous (rev4)

[Patchwork]

== Series Details ==

Series: i915: make the probe asynchronous (rev4)
URL   : https://patchwork.freedesktop.org/series/44159/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4474 -> Patchwork_9624 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/44159/revisions/4/mbox/


== Changes ==

  No changes found


== Participating hosts (45 -> 41) ==

  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4474 -> Patchwork_9624

  CI_DRM_4474: c298e0700edde3d016ae9b16d0ce2a098bee1022 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4549: e19fd5549e9cf603251704117fc64f4068be5016 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9624: 9a02b4dfc2e647a6710bd75947e207b4094aaddc @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

9a02b4dfc2e6 i915: make the probe asynchronous

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9624/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

Hi Jani, Daniel

Could you help to comment? thanks,

- Feng

On Thu, Jul 12, 2018 at 03:51:34PM +0800, Feng Tang wrote:
> Hi Daniel,
> 
> On Thu, Jul 12, 2018 at 08:54:34AM +0200, Daniel Vetter wrote:
> > On Thu, Jul 12, 2018 at 09:29:01AM +0800, Feng Tang wrote:
> > > On Tue, Jun 26, 2018 at 10:29:16AM +0800, Feng Tang wrote:
> > > > On Mon, Jun 25, 2018 at 05:36:32PM +0200, Daniel Vetter wrote:
> > >  
> > > > Hi Daneil/Jani/Takashi,
> > > > 
> > > > When I was testing this patch from Takashi, I further checked the kernel
> > > > module code, and found that: we may need NOT to add any new codes to
> > > > prepare for i915's async probe feature!
> > > > 
> > > > Say when i915 module is being loader due to HDA's request_module() call,
> > > > in the callchain, do_init_module() has such code:
> > > > 
> > > >     if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
> > > >                 async_synchronize_full();
> > > > 
> > > > This will garantee the asynced probe is done before it returns.
> > > > 
> > > > I have just tested and this seems to be enough. If I am not wrong, then
> > > > we can take the i915 async patch directly. What do you think?
> > > 
> > > Ping for comments, thanks!
> > 
> > Ram (who's working on the hdcp2 code) just learned the hard way that if
> > i915 registration gets delayed then audio fails to load. So if you want to
> > make i915 fully async, then you _must_ fix the audio load stuff.
> 
> Thanks for sharing this info, this is a real concern. I just did a quick
> search of intel-gfx mail list archive, but failed to find the details :(
> 
> > 
> > The above code just shows that if you're loading things with
> > request_module(), then there's not actually any async probing going on.
> > Which kinda defeats the point.
> > 
> > So yeah, I still think we need to fix this properly, or it's pointless.
> 
> Agree, this has to be fixed. Can we just do as the hdac_i915.c to explicitly
> claim this dependency by using the similar request_module("i915"), as there
> may be similar dependency on i915 in the future.
> 
> Thanks,
> Feng
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Daniel Vetter]

On Tue, Aug 14, 2018 at 02:54:31PM +0800, Feng Tang wrote:
> Hi Jani, Daniel
> 
> Could you help to comment? thanks,
> 
> - Feng
> 
> On Thu, Jul 12, 2018 at 03:51:34PM +0800, Feng Tang wrote:
> > Hi Daniel,
> > 
> > On Thu, Jul 12, 2018 at 08:54:34AM +0200, Daniel Vetter wrote:
> > > On Thu, Jul 12, 2018 at 09:29:01AM +0800, Feng Tang wrote:
> > > > On Tue, Jun 26, 2018 at 10:29:16AM +0800, Feng Tang wrote:
> > > > > On Mon, Jun 25, 2018 at 05:36:32PM +0200, Daniel Vetter wrote:
> > > >  
> > > > > Hi Daneil/Jani/Takashi,
> > > > > 
> > > > > When I was testing this patch from Takashi, I further checked the kernel
> > > > > module code, and found that: we may need NOT to add any new codes to
> > > > > prepare for i915's async probe feature!
> > > > > 
> > > > > Say when i915 module is being loader due to HDA's request_module() call,
> > > > > in the callchain, do_init_module() has such code:
> > > > > 
> > > > >     if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
> > > > >                 async_synchronize_full();
> > > > > 
> > > > > This will garantee the asynced probe is done before it returns.
> > > > > 
> > > > > I have just tested and this seems to be enough. If I am not wrong, then
> > > > > we can take the i915 async patch directly. What do you think?
> > > > 
> > > > Ping for comments, thanks!
> > > 
> > > Ram (who's working on the hdcp2 code) just learned the hard way that if
> > > i915 registration gets delayed then audio fails to load. So if you want to
> > > make i915 fully async, then you _must_ fix the audio load stuff.
> > 
> > Thanks for sharing this info, this is a real concern. I just did a quick
> > search of intel-gfx mail list archive, but failed to find the details :(

Sorry this wa all irc discussions around the hdcp2 patches from
Ramalingam. There's hacks in his latest patch series to work around the
audio issues.

> > > The above code just shows that if you're loading things with
> > > request_module(), then there's not actually any async probing going on.
> > > Which kinda defeats the point.
> > > 
> > > So yeah, I still think we need to fix this properly, or it's pointless.
> > 
> > Agree, this has to be fixed. Can we just do as the hdac_i915.c to explicitly
> > claim this dependency by using the similar request_module("i915"), as there
> > may be similar dependency on i915 in the future.

Erm, the entire point of this discussion was the request_module() doesn't
work. request_module() does _not_ make dependencies explicit at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Takashi Iwai]

On Tue, 14 Aug 2018 11:34:07 +0200,
Daniel Vetter wrote:
> 
> On Tue, Aug 14, 2018 at 02:54:31PM +0800, Feng Tang wrote:
> > Hi Jani, Daniel
> > 
> > Could you help to comment? thanks,
> > 
> > - Feng
> > 
> > On Thu, Jul 12, 2018 at 03:51:34PM +0800, Feng Tang wrote:
> > > Hi Daniel,
> > > 
> > > On Thu, Jul 12, 2018 at 08:54:34AM +0200, Daniel Vetter wrote:
> > > > On Thu, Jul 12, 2018 at 09:29:01AM +0800, Feng Tang wrote:
> > > > > On Tue, Jun 26, 2018 at 10:29:16AM +0800, Feng Tang wrote:
> > > > > > On Mon, Jun 25, 2018 at 05:36:32PM +0200, Daniel Vetter wrote:
> > > > >  
> > > > > > Hi Daneil/Jani/Takashi,
> > > > > > 
> > > > > > When I was testing this patch from Takashi, I further checked the kernel
> > > > > > module code, and found that: we may need NOT to add any new codes to
> > > > > > prepare for i915's async probe feature!
> > > > > > 
> > > > > > Say when i915 module is being loader due to HDA's request_module() call,
> > > > > > in the callchain, do_init_module() has such code:
> > > > > > 
> > > > > >     if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
> > > > > >                 async_synchronize_full();
> > > > > > 
> > > > > > This will garantee the asynced probe is done before it returns.
> > > > > > 
> > > > > > I have just tested and this seems to be enough. If I am not wrong, then
> > > > > > we can take the i915 async patch directly. What do you think?
> > > > > 
> > > > > Ping for comments, thanks!
> > > > 
> > > > Ram (who's working on the hdcp2 code) just learned the hard way that if
> > > > i915 registration gets delayed then audio fails to load. So if you want to
> > > > make i915 fully async, then you _must_ fix the audio load stuff.
> > > 
> > > Thanks for sharing this info, this is a real concern. I just did a quick
> > > search of intel-gfx mail list archive, but failed to find the details :(
> 
> Sorry this wa all irc discussions around the hdcp2 patches from
> Ramalingam. There's hacks in his latest patch series to work around the
> audio issues.
> 
> > > > The above code just shows that if you're loading things with
> > > > request_module(), then there's not actually any async probing going on.
> > > > Which kinda defeats the point.
> > > > 
> > > > So yeah, I still think we need to fix this properly, or it's pointless.
> > > 
> > > Agree, this has to be fixed. Can we just do as the hdac_i915.c to explicitly
> > > claim this dependency by using the similar request_module("i915"), as there
> > > may be similar dependency on i915 in the future.
> 
> Erm, the entire point of this discussion was the request_module() doesn't
> work. request_module() does _not_ make dependencies explicit at all.

FYI, the upcoming 4.19 will have the completion in audio side binding,
so this problem should be solved there.


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] i915: make the probe asynchronous

[Feng Tang]

On Tue, Aug 14, 2018 at 11:39:48AM +0200, Takashi Iwai wrote:
> On Tue, 14 Aug 2018 11:34:07 +0200,
> Daniel Vetter wrote:
> > 
> > On Tue, Aug 14, 2018 at 02:54:31PM +0800, Feng Tang wrote:
> > > Hi Jani, Daniel
> > > 
> > > Could you help to comment? thanks,
> > > 
> > > - Feng
> > > 
> > > On Thu, Jul 12, 2018 at 03:51:34PM +0800, Feng Tang wrote:
> > > > Hi Daniel,
> > > > 
> > > > On Thu, Jul 12, 2018 at 08:54:34AM +0200, Daniel Vetter wrote:
> > > > > On Thu, Jul 12, 2018 at 09:29:01AM +0800, Feng Tang wrote:
> > > > > > On Tue, Jun 26, 2018 at 10:29:16AM +0800, Feng Tang wrote:
> > > > > > > On Mon, Jun 25, 2018 at 05:36:32PM +0200, Daniel Vetter wrote:
> > > > > >  
> > > > > > > Hi Daneil/Jani/Takashi,
> > > > > > > 
> > > > > > > When I was testing this patch from Takashi, I further checked the kernel
> > > > > > > module code, and found that: we may need NOT to add any new codes to
> > > > > > > prepare for i915's async probe feature!
> > > > > > > 
> > > > > > > Say when i915 module is being loader due to HDA's request_module() call,
> > > > > > > in the callchain, do_init_module() has such code:
> > > > > > > 
> > > > > > >     if (!mod->async_probe_requested && (current->flags & PF_USED_ASYNC))
> > > > > > >                 async_synchronize_full();
> > > > > > > 
> > > > > > > This will garantee the asynced probe is done before it returns.
> > > > > > > 
> > > > > > > I have just tested and this seems to be enough. If I am not wrong, then
> > > > > > > we can take the i915 async patch directly. What do you think?
> > > > > > 
> > > > > > Ping for comments, thanks!
> > > > > 
> > > > > Ram (who's working on the hdcp2 code) just learned the hard way that if
> > > > > i915 registration gets delayed then audio fails to load. So if you want to
> > > > > make i915 fully async, then you _must_ fix the audio load stuff.
> > > > 
> > > > Thanks for sharing this info, this is a real concern. I just did a quick
> > > > search of intel-gfx mail list archive, but failed to find the details :(
> > 
> > Sorry this wa all irc discussions around the hdcp2 patches from
> > Ramalingam. There's hacks in his latest patch series to work around the
> > audio issues.
> > 
> > > > > The above code just shows that if you're loading things with
> > > > > request_module(), then there's not actually any async probing going on.
> > > > > Which kinda defeats the point.
> > > > > 
> > > > > So yeah, I still think we need to fix this properly, or it's pointless.
> > > > 
> > > > Agree, this has to be fixed. Can we just do as the hdac_i915.c to explicitly
> > > > claim this dependency by using the similar request_module("i915"), as there
> > > > may be similar dependency on i915 in the future.
> > 
> > Erm, the entire point of this discussion was the request_module() doesn't
> > work. request_module() does _not_ make dependencies explicit at all.
> 
> FYI, the upcoming 4.19 will have the completion in audio side binding,
> so this problem should be solved there.

Really a great news! thanks for sharing

Thanks,
Feng
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Brian Norris]

Hi,

On Thu, Aug 16, 2018 at 03:40:38PM +0800, Feng Tang wrote:
> On Tue, Aug 14, 2018 at 11:39:48AM +0200, Takashi Iwai wrote:
> > FYI, the upcoming 4.19 will have the completion in audio side binding,
> > so this problem should be solved there.
> 
> Really a great news! thanks for sharing

For the record: that was merged as:

  f9b54e1961c7 ("ALSA: hda/i915: Allow delayed i915 audio component binding")

I'm also poking here in case somebody still had reason we shouldn't do
this now. I wrote up my own patch, and the looked for past discussions
like this one. Feel free to comment here if there's still a problem:

  [PATCH] drm/i915: Set PROBE_PREFER_ASYNCHRONOUS
  https://lore.kernel.org/lkml/20221028145319.1.I87b119c576d486ad139faf1b7278d97e158aabe4@changeid/

Thanks,
Brian