Re: [Intel-gfx] [RFC] i915: make the probe asynchronous
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
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: [Intel-gfx] [RFC] i915: make the probe asynchronous
On Thu, Jul 12, 2018 at 09:37:41AM +0200, Daniel Vetter wrote:
> On Thu, Jul 12, 2018 at 8:56 AM, Takashi Iwai 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
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(_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(_bound, 1); /* 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: [Intel-gfx] [RFC] i915: make the probe asynchronous
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: [Intel-gfx] [RFC] i915: make the probe asynchronous
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: [Intel-gfx] [RFC] i915: make the probe asynchronous
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: [Intel-gfx] [RFC] i915: make the probe asynchronous
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
>
> 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(_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(_bound, 1); /* 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: [Intel-gfx] [RFC] i915: make the probe asynchronous
Hi Jani,
On Wed, Jun 20, 2018 at 10:11:34AM +0300, Jani Nikula wrote:
> On Wed, 20 Jun 2018, Feng Tang 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-ch...@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(_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(_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 = _pm_ops,
> >> > +.driver.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> >> > };
> >> >
> >> > static int __init i915_init(void)
> >> > @@ -755,6 +772,8 @@ static int __init i915_init(void)
> >>
Re: [Intel-gfx] [RFC] i915: make the probe asynchronous
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-ch...@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(_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(_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 = _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(_probe_done);
> > +
> > return pci_register_driver(_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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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: [Intel-gfx] [RFC] i915: make the probe asynchronous
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-ch...@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(_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(_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 = _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(_probe_done);
+
return pci_register_driver(_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
#include
#include
+#include
#include
#include
#include
@@ -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: [Intel-gfx] [RFC] i915: make the probe asynchronous
Hi Jani, On Tue, Jun 05, 2018 at 11:18:46AM +0300, Jani Nikula wrote: > On Tue, 05 Jun 2018, Feng Tang wrote: > > Hi Jani, > > > > On Tue, Jun 05, 2018 at 10:41:04AM +0300, Jani Nikula wrote: > >> On Mon, 04 Jun 2018, Feng Tang 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-ch...@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: [Intel-gfx] [RFC] i915: make the probe asynchronous
Hi Jani, On Tue, Jun 05, 2018 at 10:41:04AM +0300, Jani Nikula wrote: > On Mon, 04 Jun 2018, Feng Tang 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
[Intel-gfx] [RFC] i915: make the probe asynchronous
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
Cc: Daniel Vetter
Cc: Jani Nikula
Cc: Ville Syrjälä
Cc: Chris Wilson
---
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 = _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
Re: [Intel-gfx] [PATCH] drm/dp: add module parameter for the dpcd access max retries
On Wed, May 09, 2018 at 12:28:15PM +0300, Jani Nikula wrote: > On Wed, 09 May 2018, Feng Tang <feng.t...@intel.com> wrote: > > On Wed, May 09, 2018 at 10:53:53AM +0300, Jani Nikula wrote: > >> On Wed, 09 May 2018, Feng Tang <feng.t...@intel.com> wrote: > >> > On Tue, May 08, 2018 at 10:30:19PM +0300, Jani Nikula wrote: > >> >> On Wed, 09 May 2018, Feng Tang <feng.t...@intel.com> wrote: > >> >> >> Well if it's edp probing, then atm we do need to block since we have > >> >> >> no support for panel hotplugging. And userspace generally no > >> >> >> expectations that built-in panels come async_synchronize_full > >> >> >> making our fbdev code less async than desired is kinda a different > >> >> >> story I think here. First step would be trying to figure out why we > >> >> >> even bother with edp probing on this platform, when the thing isn't > >> >> >> there. Sounds like broken VBT. > >> >> > > >> >> > Hi Daniel, > >> >> > > >> >> > Here are some of the VBT and DPCD related logs on my A3900 (bxt + > >> >> > GEN9 LP) > >> >> > based NUC. but I don't have the knowledge to tell if the VBT is > >> >> > broken :) > >> >> > >> >> Please run current upstream drm-tip when you're suggesting changes to > >> >> upstream code. Looks like you're running at most v4.14. This can't be > >> >> emphasized enough. We can't and won't merge the changes unless they make > >> >> sense with current code. > >> > > >> > Yes, I understand, the patch posted was created right after git-pulling > >> > Linus' tree, and back-ported to test with 4.14 kernel. > >> > > >> >> > >> >> As to vbt, please send me /sys/kernel/debug/dri/0/i915_vbt. > >> > > >> > Sure. as attached > >> > >> Your VBT claims the device has an eDP panel. Does it have one or not? > > > > After asking around, our platform (BXT NUC) does have a eDP interface > > (someone > > has tested with a eDP screen), but most of our platforms are connected > > with 2 HDMI LCD monitors. > > Sounds like you should have a different VBT for the cases where you ship > with/without eDP connected. As you've noticed, we generally try pretty Yep, this is a good idea. Currently I'm not able to change VBT, so I hacked the code to simulating a no-eDP VBT like: --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1261,7 +1261,8 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port, is_crt = child->device_type & DEVICE_TYPE_ANALOG_OUTPUT; is_hdmi = is_dvi && (child->device_type & DEVICE_TYPE_NOT_HDMI_OUTPUT) == 0; - is_edp = is_dp && (child->device_type & DEVICE_TYPE_INTERNAL_CONNECTOR); + is_edp = 0; And it does cut the boottime a lot!! as avoiding the dpcd access. And later i915_hpd_poll_init_work() will still call intel_dp_detect() and call the time-eater drm_dp_dpcd_access() finally, but the situation is better as it runs in an async way at this point. Thanks, Feng ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/dp: add module parameter for the dpcd access max retries
On Wed, May 09, 2018 at 10:53:53AM +0300, Jani Nikula wrote: > On Wed, 09 May 2018, Feng Tang <feng.t...@intel.com> wrote: > > On Tue, May 08, 2018 at 10:30:19PM +0300, Jani Nikula wrote: > >> On Wed, 09 May 2018, Feng Tang <feng.t...@intel.com> wrote: > >> >> Well if it's edp probing, then atm we do need to block since we have > >> >> no support for panel hotplugging. And userspace generally no > >> >> expectations that built-in panels come async_synchronize_full > >> >> making our fbdev code less async than desired is kinda a different > >> >> story I think here. First step would be trying to figure out why we > >> >> even bother with edp probing on this platform, when the thing isn't > >> >> there. Sounds like broken VBT. > >> > > >> > Hi Daniel, > >> > > >> > Here are some of the VBT and DPCD related logs on my A3900 (bxt + GEN9 > >> > LP) > >> > based NUC. but I don't have the knowledge to tell if the VBT is broken :) > >> > >> Please run current upstream drm-tip when you're suggesting changes to > >> upstream code. Looks like you're running at most v4.14. This can't be > >> emphasized enough. We can't and won't merge the changes unless they make > >> sense with current code. > > > > Yes, I understand, the patch posted was created right after git-pulling > > Linus' tree, and back-ported to test with 4.14 kernel. > > > >> > >> As to vbt, please send me /sys/kernel/debug/dri/0/i915_vbt. > > > > Sure. as attached > > Your VBT claims the device has an eDP panel. Does it have one or not? After asking around, our platform (BXT NUC) does have a eDP interface (someone has tested with a eDP screen), but most of our platforms are connected with 2 HDMI LCD monitors. Thanks, Feng ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/dp: add module parameter for the dpcd access max retries
On Tue, May 08, 2018 at 10:30:19PM +0300, Jani Nikula wrote: > On Wed, 09 May 2018, Feng Tang <feng.t...@intel.com> wrote: > > >> > > > On Mon, May 07, 2018 at 11:40:45AM +0100, Chris Wilson wrote: > >> >> > > > > Quoting Feng Tang (2018-05-07 11:36:09) > >> >> > > > > > To fulfil the Dell 4K monitor, the dpcd max retries has been > >> >> > > > > > bumped > >> >> > > > > > from 7 to 32, which may hurt the boot/init time for some > >> >> > > > > > platforms, > >> >> > > > > > as the 32 retries may take hundreds of ms. > >> >> > > > > > >> >> > > > > If we need that many retries, so be it. No modparam, the driver > >> >> > > > > just has > >> >> > > > > to work. > >> >> > > > > >> >> > > > I understand your point. The retry numer was originally 7, and > >> >> > > > worked > >> >> > > > fine untill the Dell 4K monitor which changes to 32. According > >> >> > > > to my test, > >> >> > > > each retry will take about 8ms on the A3960 based NUC. > >> >> > > > > >> >> > > > One of our product need to boot up within a given time limit, this > >> >> > > > 32 retries will take about 1/3 of the budget (about 270ms), that's > >> >> > > > why I would try to make it a parameter. > >> >> > > > >> >> > > The essence is that probing whether a monitor is connected should > >> >> > > not be > >> >> > > blocking boot. If an async probe tries and fails to find a monitor, > >> >> > > fine - no one will notice. If it does take 270ms to find a monitor, > >> >> > > it > >> >> > > turns on 200ms after userspace kicks in, just like any other > >> >> > > hotplug. > >> >> > > >> >> > Yeah, the fix here is to get the probing out of the driver load path, > >> >> > not > >> >> > to break the driver for some funky monitors. And definitely not using > >> >> > a > >> >> > modparam. > >> >> > >> >> Hi Chris and Daniel, > >> >> > >> >> After reading the i915 modeset init code, I did some experiments: > >> >> 1. make the intel_modeset_init() function async (here async means > >> >>creating a async func wrapper and call async_schedul() for it) > >> >> 2. make the intel_setup_outpus()+intel_modeset_setup_hw_state() async > >> > > >> > You could just set i915_pci_driver.driver.prove_type = > >> > PROBE_PREFER_ASYNCHRONOUS for the same result, or set i915.async_probe=1 > >> > for testing. > >> > > >> > However, if it's blocking on async_synchronize_full(), that will be no > >> > improvement. So if it is only an existing async_sychronize_full() > >> > causing the fbdev config to be waited on before userspace, we need to > >> > stop using the async mechanism and just use an ordinary worker and > >> > manual flushing. If it's the fbdev probing blocking you, why are you > >> > using fbdev? > >> > >> Well if it's edp probing, then atm we do need to block since we have > >> no support for panel hotplugging. And userspace generally no > >> expectations that built-in panels come async_synchronize_full > >> making our fbdev code less async than desired is kinda a different > >> story I think here. First step would be trying to figure out why we > >> even bother with edp probing on this platform, when the thing isn't > >> there. Sounds like broken VBT. > > > > Hi Daniel, > > > > Here are some of the VBT and DPCD related logs on my A3900 (bxt + GEN9 LP) > > based NUC. but I don't have the knowledge to tell if the VBT is broken :) > > Please run current upstream drm-tip when you're suggesting changes to > upstream code. Looks like you're running at most v4.14. This can't be > emphasized enough. We can't and won't merge the changes unless they make > sense with current code. Yes, I understand, the patch posted was created right after git-pulling Linus' tree, and back-ported to test with 4.14 kernel. > > As to vbt, please send me /sys/kernel/debug/dri/0/i915_vbt. Sure. as attached Thanks, Feng apl_nuc_i915_vbt Description: Binary data ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/dp: add module parameter for the dpcd access max retries
>> > > > On Mon, May 07, 2018 at 11:40:45AM +0100, Chris Wilson wrote: > >> > > > > Quoting Feng Tang (2018-05-07 11:36:09) > >> > > > > > To fulfil the Dell 4K monitor, the dpcd max retries has been > >> > > > > > bumped > >> > > > > > from 7 to 32, which may hurt the boot/init time for some > >> > > > > > platforms, > >> > > > > > as the 32 retries may take hundreds of ms. > >> > > > > > >> > > > > If we need that many retries, so be it. No modparam, the driver > >> > > > > just has > >> > > > > to work. > >> > > > > >> > > > I understand your point. The retry numer was originally 7, and worked > >> > > > fine untill the Dell 4K monitor which changes to 32. According to > >> > > > my test, > >> > > > each retry will take about 8ms on the A3960 based NUC. > >> > > > > >> > > > One of our product need to boot up within a given time limit, this > >> > > > 32 retries will take about 1/3 of the budget (about 270ms), that's > >> > > > why I would try to make it a parameter. > >> > > > >> > > The essence is that probing whether a monitor is connected should not > >> > > be > >> > > blocking boot. If an async probe tries and fails to find a monitor, > >> > > fine - no one will notice. If it does take 270ms to find a monitor, it > >> > > turns on 200ms after userspace kicks in, just like any other hotplug. > >> > > >> > Yeah, the fix here is to get the probing out of the driver load path, not > >> > to break the driver for some funky monitors. And definitely not using a > >> > modparam. > >> > >> Hi Chris and Daniel, > >> > >> After reading the i915 modeset init code, I did some experiments: > >> 1. make the intel_modeset_init() function async (here async means > >>creating a async func wrapper and call async_schedul() for it) > >> 2. make the intel_setup_outpus()+intel_modeset_setup_hw_state() async > > > > You could just set i915_pci_driver.driver.prove_type = > > PROBE_PREFER_ASYNCHRONOUS for the same result, or set i915.async_probe=1 > > for testing. > > > > However, if it's blocking on async_synchronize_full(), that will be no > > improvement. So if it is only an existing async_sychronize_full() > > causing the fbdev config to be waited on before userspace, we need to > > stop using the async mechanism and just use an ordinary worker and > > manual flushing. If it's the fbdev probing blocking you, why are you > > using fbdev? > > Well if it's edp probing, then atm we do need to block since we have > no support for panel hotplugging. And userspace generally no > expectations that built-in panels come async_synchronize_full > making our fbdev code less async than desired is kinda a different > story I think here. First step would be trying to figure out why we > even bother with edp probing on this platform, when the thing isn't > there. Sounds like broken VBT. Hi Daniel, Here are some of the VBT and DPCD related logs on my A3900 (bxt + GEN9 LP) based NUC. but I don't have the knowledge to tell if the VBT is broken :) [0.545231] [drm:intel_bios_init] Set default to SSC at 10 kHz [0.545237] [drm:intel_bios_init] VBT signature "$VBT BROXTON", BDB version 207 [0.545241] [drm:intel_bios_init] BDB_GENERAL_FEATURES int_tv_support 0 int_crt_support 0 lvds_use_ssc 0 lvds_ssc_freq 12 display_clock_mode 1 fdi_rx_polarity_inverted 0 [0.545245] [drm:intel_bios_init] crt_ddc_bus_pin: 2 [0.545255] [drm:intel_opregion_get_panel_type] Failed to get panel details from OpRegion (-19) [0.545257] [drm:intel_bios_init] Panel type: 0 (VBT) [0.545260] [drm:intel_bios_init] DRRS supported mode is seamless [0.545275] [drm:intel_bios_init] Found panel mode in BIOS VBT tables: [0.545281] [drm:drm_mode_debug_printmodeline] Modeline 0:"1920x1080" 0 148500 1920 2008 2052 2200 1080 1084 1089 1125 0x8 0xa [0.545292] [drm:intel_bios_init] VBT backlight PWM modulation frequency 200 Hz, active high, min brightness 0, level 180, controller 1 [0.545301] [drm:intel_bios_init] Unsupported child device size for SDVO mapping. [0.545305] [drm:intel_bios_init] Expected child device config size for VBT version 207 not known; assuming 38 [0.545323] [drm:intel_bios_init] DRRS State Enabled:1 [0.545334] [drm:intel_bios_init] Port A VBT info: DP:1 HDMI:0 DV
Re: [Intel-gfx] [PATCH] drm/dp: add module parameter for the dpcd access max retries
On Mon, May 07, 2018 at 05:09:21PM +0200, Daniel Vetter wrote: > On Mon, May 07, 2018 at 02:33:25PM +0100, Chris Wilson wrote: > > Quoting Feng Tang (2018-05-07 22:26:34) > > > Hi Chris, > > > > > > Thanks for the prompt review! > > > > > > On Mon, May 07, 2018 at 11:40:45AM +0100, Chris Wilson wrote: > > > > Quoting Feng Tang (2018-05-07 11:36:09) > > > > > To fulfil the Dell 4K monitor, the dpcd max retries has been bumped > > > > > from 7 to 32, which may hurt the boot/init time for some platforms, > > > > > as the 32 retries may take hundreds of ms. > > > > > > > > If we need that many retries, so be it. No modparam, the driver just has > > > > to work. > > > > > > I understand your point. The retry numer was originally 7, and worked > > > fine untill the Dell 4K monitor which changes to 32. According to my > > > test, > > > each retry will take about 8ms on the A3960 based NUC. > > > > > > One of our product need to boot up within a given time limit, this > > > 32 retries will take about 1/3 of the budget (about 270ms), that's > > > why I would try to make it a parameter. > > > > The essence is that probing whether a monitor is connected should not be > > blocking boot. If an async probe tries and fails to find a monitor, > > fine - no one will notice. If it does take 270ms to find a monitor, it > > turns on 200ms after userspace kicks in, just like any other hotplug. > > Yeah, the fix here is to get the probing out of the driver load path, not > to break the driver for some funky monitors. And definitely not using a > modparam. Hi Chris and Daniel, After reading the i915 modeset init code, I did some experiments: 1. make the intel_modeset_init() function async (here async means creating a async func wrapper and call async_schedul() for it) 2. make the intel_setup_outpus()+intel_modeset_setup_hw_state() async But both of them will trigger kernel panic (log msg pasted in the end), did I made some mistakes, or maybe the i915 codes following these functions has dependency over them? IIUC the dpcd access first happens in i915_driver_load --> i915_load_modeset_init --> intel_modeset_init --> intel_setup_outputs --> intel_ddi_init --> intel_edp_init_connector --> intel_edp_init_dpcd (to check if DPCD exist) Should we postpone it to later phase or even after user space kick in? Thanks, Feng --- Error msg for my async experiment: [0.715706] No backend configured for hyper_dmabuf in kernel config [0.716079] Hyper_dmabuf: no backend found [0.736361] intel_powerclamp: CPU does not support MWAIT [0.737643] [drm:wait_panel_status] *ERROR* PPS state mismatch [0.741381] genirq: Setting trigger mode 3 for irq 127 failed (intel_gpio_irq_type+0x0/0x110) [0.743244] dmi-sysfs: dmi entry is absent. [0.765116] [edp_panel_vdd_on()]: exit [0.765360] BUG: unable to handle kernel NULL pointer dereference at (null) [0.765809] IP: (null) [0.766005] PGD 0 P4D 0 [0.766168] Oops: 0010 [#1] PREEMPT SMP [0.766401] Modules linked in: [0.766592] CPU: 0 PID: 28 Comm: kworker/u8:1 Tainted: G U W 4.14.39-sos+ #26 [0.767075] Workqueue: events_unbound async_run_entry_fn [0.767392] task: 88027433c240 task.stack: 88027434 [0.767743] RIP: 0010: (null) [0.767969] RSP: :880274343ab8 EFLAGS: 00010246 [0.768281] RAX: RBX: 2d4003ff RCX: 0001 [0.768701] RDX: 8000 RSI: RDI: 880272f21100 [0.769121] RBP: R08: 2d4003ff R09: 0001 [0.769541] R10: 880274343a60 R11: 82e7fe0d R12: 0001 [0.769961] R13: 880273038000 R14: 0004 R15: 880272f21100 [0.770383] FS: () GS:88027dc0() knlGS: [0.770858] CS: 0010 DS: ES: CR0: 80050033 [0.771199] CR2: CR3: 02613000 CR4: 003426f0 [0.771619] Call Trace: [0.771779] ? intel_dp_aux_ch+0x1a7/0x770 [0.772031] ? remove_wait_queue+0x60/0x60 [0.772281] ? intel_dp_aux_transfer+0xa6/0x200 [0.772556] ? drm_dp_dpcd_access+0x9d/0x150 [0.772815] ? drm_dp_dpcd_read+0x2c/0x60 [0.773059] ? drm_dp_read_desc+0x43/0xf0 [0.773303] ? intel_dp_detect+0x346/0x6a0 [0.773554] ? drm_helper_probe_single_connector_modes+0xcd/0x6b0 [0.773920] ? _raw_spin_unlock+0x14/0x30 [0.774165] ? vt_console_print+0x22a/0x3d0 [0.774420] ? preempt_count_add+0x56/0xa0 [0.774669] ? _raw_spin_lock_irqsave+0x32/0x40 [0.774944] ? drm_setup_crtcs+0x143/0x9e0 [0.775195]
Re: [Intel-gfx] [PATCH] drm/dp: add module parameter for the dpcd access max retries
On Mon, May 07, 2018 at 05:09:21PM +0200, Daniel Vetter wrote: > On Mon, May 07, 2018 at 02:33:25PM +0100, Chris Wilson wrote: > > Quoting Feng Tang (2018-05-07 22:26:34) > > > Hi Chris, > > > > > > Thanks for the prompt review! > > > > > > On Mon, May 07, 2018 at 11:40:45AM +0100, Chris Wilson wrote: > > > > Quoting Feng Tang (2018-05-07 11:36:09) > > > > > To fulfil the Dell 4K monitor, the dpcd max retries has been bumped > > > > > from 7 to 32, which may hurt the boot/init time for some platforms, > > > > > as the 32 retries may take hundreds of ms. > > > > > > > > If we need that many retries, so be it. No modparam, the driver just has > > > > to work. > > > > > > I understand your point. The retry numer was originally 7, and worked > > > fine untill the Dell 4K monitor which changes to 32. According to my > > > test, > > > each retry will take about 8ms on the A3960 based NUC. > > > > > > One of our product need to boot up within a given time limit, this > > > 32 retries will take about 1/3 of the budget (about 270ms), that's > > > why I would try to make it a parameter. > > > > The essence is that probing whether a monitor is connected should not be > > blocking boot. If an async probe tries and fails to find a monitor, > > fine - no one will notice. If it does take 270ms to find a monitor, it > > turns on 200ms after userspace kicks in, just like any other hotplug. > > Yeah, the fix here is to get the probing out of the driver load path, not > to break the driver for some funky monitors. And definitely not using a > modparam. Thank you both for the suggestion! Will try to make the probing async first, though I'm not familiar with the whole drm yet :) - Feng ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/dp: add module parameter for the dpcd access max retries
Hi Chris, Thanks for the prompt review! On Mon, May 07, 2018 at 11:40:45AM +0100, Chris Wilson wrote: > Quoting Feng Tang (2018-05-07 11:36:09) > > To fulfil the Dell 4K monitor, the dpcd max retries has been bumped > > from 7 to 32, which may hurt the boot/init time for some platforms, > > as the 32 retries may take hundreds of ms. > > If we need that many retries, so be it. No modparam, the driver just has > to work. I understand your point. The retry numer was originally 7, and worked fine untill the Dell 4K monitor which changes to 32. According to my test, each retry will take about 8ms on the A3960 based NUC. One of our product need to boot up within a given time limit, this 32 retries will take about 1/3 of the budget (about 270ms), that's why I would try to make it a parameter. > > > This patch makes no functional change, but make the max retries > > adjustable, so that concerned users/platforms can have an option > > to short the wait time. > > > > On a Intel Atom Processor A3960 based NUC, the i915_init() time could > > be reduced from 450ms to 200ms. > > > > retries = 3: > > [0.457806] calling i915_init+0x0/0x51 @ 1 > > [0.662307] initcall i915_init+0x0/0x51 returned 0 after 199694 > > usecs > > > > retries = 32: > > [0.465818] calling i915_init+0x0/0x51 @ 1 > > [0.925831] initcall i915_init+0x0/0x51 returned 0 after 449219 > > usecs > > Why is this synchronous anyway? I don't know the reason, maybe some of kernel components depends on the GFX module. But even we can put it to async, it is still the longest init one on our platform. Thanks, Feng ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/dp: add module parameter for the dpcd access max retries
To fulfil the Dell 4K monitor, the dpcd max retries has been bumped
from 7 to 32, which may hurt the boot/init time for some platforms,
as the 32 retries may take hundreds of ms.
This patch makes no functional change, but make the max retries
adjustable, so that concerned users/platforms can have an option
to short the wait time.
On a Intel Atom Processor A3960 based NUC, the i915_init() time could
be reduced from 450ms to 200ms.
retries = 3:
[0.457806] calling i915_init+0x0/0x51 @ 1
[0.662307] initcall i915_init+0x0/0x51 returned 0 after 199694 usecs
retries = 32:
[0.465818] calling i915_init+0x0/0x51 @ 1
[0.925831] initcall i915_init+0x0/0x51 returned 0 after 449219 usecs
Signed-off-by: Feng Tang <feng.t...@intel.com>
---
drivers/gpu/drm/drm_dp_helper.c | 22 +++---
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index ffe14ec..6054067 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -171,6 +171,20 @@ EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
#define AUX_RETRY_INTERVAL 500 /* us */
+/*
+ * The specification doesn't give any recommendation on how often to
+ * retry native transactions. We used to retry 7 times like for
+ * aux i2c transactions but real world devices this wasn't
+ * sufficient, bump to 32 which makes Dell 4k monitors happier.
+ *
+ * Since the retry may take quite some boot time, we make it a
+ * adjustable parameter for possible shorter retry time.
+ */
+static int dp_dpcd_max_retries __read_mostly = 32;
+module_param_unsafe(dp_dpcd_max_retries, int, 0644);
+MODULE_PARM_DESC(dp_dpcd_max_retries,
+"Max retry number for DPCD access (default 32)");
+
/**
* DOC: dp helpers
*
@@ -198,13 +212,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8
request,
mutex_lock(>hw_mutex);
- /*
-* The specification doesn't give any recommendation on how often to
-* retry native transactions. We used to retry 7 times like for
-* aux i2c transactions but real world devices this wasn't
-* sufficient, bump to 32 which makes Dell 4k monitors happier.
-*/
- for (retry = 0; retry < 32; retry++) {
+ for (retry = 0; retry < dp_dpcd_max_retries; retry++) {
if (ret != 0 && ret != -ETIMEDOUT) {
usleep_range(AUX_RETRY_INTERVAL,
AUX_RETRY_INTERVAL + 100);
--
2.7.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
