diff mbox

[RFC] i915: make the probe asynchronous

Message ID s5h4lhxokp6.wl-tiwai@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Iwai June 20, 2018, 9:45 a.m. UTC
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< --

Comments

Feng Tang June 21, 2018, 6:52 a.m. UTC | #1
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;
Daniel Vetter June 25, 2018, 3:36 p.m. UTC | #2
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;
Feng Tang June 26, 2018, 2:29 a.m. UTC | #3
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
Feng Tang July 12, 2018, 1:29 a.m. UTC | #4
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
Daniel Vetter July 12, 2018, 6:54 a.m. UTC | #5
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
Takashi Iwai July 12, 2018, 6:56 a.m. UTC | #6
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
Daniel Vetter July 12, 2018, 7:37 a.m. UTC | #7
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
Feng Tang July 12, 2018, 7:51 a.m. UTC | #8
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
Feng Tang Aug. 14, 2018, 6:54 a.m. UTC | #9
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
>
Daniel Vetter Aug. 14, 2018, 9:34 a.m. UTC | #10
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
Takashi Iwai Aug. 14, 2018, 9:39 a.m. UTC | #11
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
Feng Tang Aug. 16, 2018, 7:40 a.m. UTC | #12
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
Brian Norris Oct. 28, 2022, 9:55 p.m. UTC | #13
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
diff mbox

Patch

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