Re: [PATCH 6/7] ASoC: SOF: Intel: Remove deferred probe for SOF

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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