| Message ID | 20230718084522.116952-1-maarten.lankhorst@linux.intel.com |
|---|---|
| Headers | show |
| Series | sound: Use -EPROBE_DEFER instead of i915 module loading. | expand |
On Tue, 18 Jul 2023 19:04:41 +0200, Kai Vehmanen wrote: > > > diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c > > index f1fd5b44aaac9..344b61576c0e3 100644 > > --- a/sound/soc/sof/intel/hda-codec.c > > +++ b/sound/soc/sof/intel/hda-codec.c > > @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) > > return 0; > > > > /* i915 exposes a HDA codec for HDMI audio */ > > - ret = snd_hdac_i915_init(bus, true); > > + ret = snd_hdac_i915_init(bus, false); > > if (ret < 0) > > return ret; > > My only bigger concern is corner cases where the display PCI device is on > the bus and visible to kernel, but for some reason there is no working > driver in the system or it is disabled. > > With this patch, not having a workign display driver means that there is > also no audio in the system as the SOF driver will never get probed. > > In current mainline, one will get the 60sec timeout warning and then > audio driver will proceed to probe and you'll have audio support (minus > HDMI/DP). Yeah, that was a concern in the past, too. e.g. when you pass "nomodeset" boot option, the driver will become unusable, even if the bus is used generically for both analog and HDMI codecs. > This is mostly an issue with very new hardware (e.g. hw is still > behind force_probe flag in xe/i915 driver), but we've had some odd > cases with e.g. systems with both Intel IGFX and other vendors' DGPU. > Audio drivers see the Intel VGA controller in system and will > call snd_hdac_i915_init(), but the audio component bind will never > succeed if the the Intel IGFX is not in actual use. > > Will need a bit of time to think about possible scenarios. Possibly this > is not an issue outside early development systems. In theory if IGFX is > disabled in BIOS, and not visible to OS, we are good, and if it's visible, > the i915/xe driver should be loaded, so we are good again. The 60 seconds timeout is a thing "better than complete disablement", so it's not ideal, either. Maybe we can add something like the following: - Check when the deferred probe takes too long, and warn it - Provide some runtime option to disable the component binding, so that user can work around it if needed thanks, Takashi
Hey, On 2023-07-19 08:01, Takashi Iwai wrote: > On Tue, 18 Jul 2023 19:04:41 +0200, > Kai Vehmanen wrote: >>> diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c >>> index f1fd5b44aaac9..344b61576c0e3 100644 >>> --- a/sound/soc/sof/intel/hda-codec.c >>> +++ b/sound/soc/sof/intel/hda-codec.c >>> @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) >>> return 0; >>> >>> /* i915 exposes a HDA codec for HDMI audio */ >>> - ret = snd_hdac_i915_init(bus, true); >>> + ret = snd_hdac_i915_init(bus, false); >>> if (ret < 0) >>> return ret; >> My only bigger concern is corner cases where the display PCI device is on >> the bus and visible to kernel, but for some reason there is no working >> driver in the system or it is disabled. >> >> With this patch, not having a workign display driver means that there is >> also no audio in the system as the SOF driver will never get probed. >> >> In current mainline, one will get the 60sec timeout warning and then >> audio driver will proceed to probe and you'll have audio support (minus >> HDMI/DP). > Yeah, that was a concern in the past, too. e.g. when you pass > "nomodeset" boot option, the driver will become unusable, even if the > bus is used generically for both analog and HDMI codecs. Yeah, I have no answer for this. My guess is that in an ideal world, the optional features related to HDMI outputs would be put in a separate sub-driver, which could -EPROBE_DEFER. Only when this driver loads, features related to display will work, but the main audio driver could still load. >> This is mostly an issue with very new hardware (e.g. hw is still >> behind force_probe flag in xe/i915 driver), but we've had some odd >> cases with e.g. systems with both Intel IGFX and other vendors' DGPU. >> Audio drivers see the Intel VGA controller in system and will >> call snd_hdac_i915_init(), but the audio component bind will never >> succeed if the the Intel IGFX is not in actual use. >> >> Will need a bit of time to think about possible scenarios. Possibly this >> is not an issue outside early development systems. In theory if IGFX is >> disabled in BIOS, and not visible to OS, we are good, and if it's visible, >> the i915/xe driver should be loaded, so we are good again. > The 60 seconds timeout is a thing "better than complete disablement", > so it's not ideal, either. Maybe we can add something like the > following: > > - Check when the deferred probe takes too long, and warn it > - Provide some runtime option to disable the component binding, so > that user can work around it if needed A module option to snd_hdac_i915_init would probably be the least of all evils here. I see the removal of the 60 second timeout as a good thing regardless. :-) Usually when nomodeset is used, it's just for safe mode. With the addition of the xe driver, blindly modprobing i915 will fall apart regardless. ~Maarten
Hi, On Wed, 19 Jul 2023, Maarten Lankhorst wrote: > On Tue, 18 Jul 2023 19:04:41 +0200, Kai Vehmanen wrote: >> My only bigger concern is corner cases where the display PCI device is on >> the bus and visible to kernel, but for some reason there is no working >> driver in the system or it is disabled. > > Yeah, I have no answer for this. My guess is that in an ideal world, the optional features > related to HDMI outputs would be put in a separate sub-driver, which could -EPROBE_DEFER. > Only when this driver loads, features related to display will work, but the main audio driver > could still load. in longer term, we have ongoing work in SOF to allow exposing multiple cards (e.g. to have a separate card for HDMI/DP PCM devices), and we are continuously working at improving the data we get from ACPI to have less guesswork in the driver. But this really doesn't help in the shortterm and/or cover all scenarios. So for now, this is legacy we just need to deal with. OTOH, I do agree that... > A module option to snd_hdac_i915_init would probably be the least of all evils here. > > I see the removal of the 60 second timeout as a good thing regardless. :-) Usually when nomodeset is used, it's just for safe > mode. > > With the addition of the xe driver, blindly modprobing i915 will fall apart regardless. The modprobing of i915 from the audio driver, has always felt a bit out-of-place, and with the xe driver, this simply won't scale anymore. The test results so far look good and this patchset works ok even if some of the more complex multi-GPU configurations we have, so I think with a module option to snd_hdac_i915, I'd be ready to go with this. Br, Kai