[alsa-devel] [PATCH] ASoC: Skylake SST driver - blacklist the PCI device IDs for the auto probe

[Pierre-Louis Bossart]

On 9/24/19 12:29 PM, Jaroslav Kysela wrote:
> Dne 24. 09. 19 v 15:41 Pierre-Louis Bossart napsal(a):
>>
>>
>> On 9/24/19 1:46 AM, Jaroslav Kysela wrote:
>>> Dne 24. 09. 19 v 1:34 Pierre-Louis Bossart napsal(a):
>>>> On 9/23/19 4:21 PM, Takashi Iwai wrote:
>>>>> On Mon, 23 Sep 2019 22:35:14 +0200,
>>>>> Jaroslav Kysela wrote:
>>>>>>
>>>>>> Dne 23. 09. 19 v 20:24 Pierre-Louis Bossart napsal(a):
>>>>>>> On 9/23/19 11:57 AM, Jaroslav Kysela wrote:
>>>>>>>> There are basically three drivers for the PCI devices for
>>>>>>>> the recent Intel hardware with the build-in DSPs. The legacy HDA
>>>>>>>> driver has dmic_detect module option for the auto detection
>>>>>>>> of the platforms with the digital microphone. Because the SOF
>>>>>>>> driver is preferred, just skip PCI probe in the Skylake SST
>>>>>>>> driver when the PCI device ID clashes by default. The user
>>>>>>>> can override the auto behaviour with the pci_binding
>>>>>>>> module parameter.
>>>>>>>
>>>>>>> Thanks Jaroslav for re-opening this mutual-exclusion issue.
>>>>>>>
>>>>>>> I think we want to deal with this in two alternate ways
>>>>>>> 1. static built-time exclusion based on Kconfigs
>>>>>>
>>>>>> Unfortunately, that's really an issue for the universal distros.
>>>>>
>>>>> Right.  The Kconfig of Intel audio is already too messy even for now.
>>>>> We don't want more complexity just for covering some very corner
>>>>> case.
>>>>>
>>>>> Practically seen, if SOF Kconfig is enabled, we may assume that SOF is
>>>>> preferred in general.  I don't think of any big need of yet another
>>>>> static configuration.
>>>>>
>>>>>>> 2. probe-time exclusion based on quirks (CPU ID + DMI)
>>>>>>>
>>>>>>> For example with a SKL/KBL/APL chromebook w/ DMIC we'd want to use the
>>>>>>> SST driver and for GLK+ we want to use SOF. For any device with
>>>>>>> HDAudio+DMIC we'd want SOF, same for any device with SoundWire when it's
>>>>>>> fully supported.
>>>>>>>
>>>>>>> I can't recall if I shared the patches I worked on a couple of months
>>>>>>> ago, but they are still at https://github.com/thesofproject/linux/pull/927
>>>>>>
>>>>>> Thanks for pointing me to this. It does not address the legacy HDA, but it's a
>>>>>> step forward.
>>>>>
>>>>> The legacy HD-audio stuff is resolved with the recent DMIC detection
>>>>> on 5.4, I suppose?
>>>
>>> Unfortunately not completely. The Broxton and Coffelake PCI IDs are also
>>> shared by the SST / Legacy HDA drivers, so the universal kernel will be
>>> confused (at least snd_hda_intel will be loaded as first).
>>
>> I don't see any issues with this. The use of the DSP is only required
>> when DMICs/I2S/SoundWire are used, or the firmware contains processing.
>> Using the firmware is passthrough mode brings no added value.
>>
>> If the only thing that's done is manage an HDaudio link, the legacy is
>> just fine.
> 
> Yes, but the dependancy on the PCI probe order specified just by the module
> name is not really nice at all. We are just lucky that the legacy
> snd_hda_intel is first.
> 
>>>>>>> the first part essentially does the same thing as this patch, the second
>>>>>>> relies on quirks. I've been busy with other things but indeed it's high
>>>>>>> time we closed this for distributions.
>>>>>>
>>>>>> Yes, and I have to say, it's too late for the hardware vendors right now. I
>>>>>> will probably apply my patch to our distribution (I don't care too much about
>>>>>> chromebooks - the user can change the module/driver behaviour manually) until
>>>>>> we have a better code.
>>>>>>
>>>>>>>> Boot log from Lenovo Carbon X1 (7th gen) with the default settings:
>>>>>>>>
>>>>>>>>       snd_hda_intel 0000:00:1f.3: Digital mics found on Skylake+ platform, aborting probe
>>>>>>>>       snd_soc_skl 0000:00:1f.3: SOF driver is preferred on this platform, aborting probe
>>>>>>>>       sof-audio-pci 0000:00:1f.3: warning: No matching ASoC machine driver found
>>>>>>>>       sof-audio-pci 0000:00:1f.3: DSP detected with PCI class/subclass/prog-if 0x040380
>>>>>>>>       ....
>>>>>>>>
>>>>>>>> Perhaps, it may be more wise to create one shared module and all
>>>>>>>> three drivers should call the driver detection routine(s) from one
>>>>>>>> place.
>>>>>>>
>>>>>>> We did look into this and it's a bit complicated in terms of plumbing.
>>>>>>
>>>>>> Could you elaborate more here? I believe that for the runtime environment
>>>>>> where all drivers are compiled in the kernel, it might make sense to have this
>>>>>> code at one place and installed only once for all three (or may be four in the
>>>>>> soundwire future) drivers.
>>>>>>
>>>>>> We should have one straight way which driver/module is used. The separate
>>>>>> conditions in the mentioned drivers will cause problems. Also, it will
>>>>>> simplify things for the end user. One module parameter (in the driver selector
>>>>>> library) is better than three or four to make things working (if the DMI /
>>>>>> whatever table is not preset correctly for the new hardware).
>>>>>
>>>>> Well, one question is where to put this option.  I thought of HD-audio
>>>>> core in the past, but it's not always the common place any longer.
>>>>> We may introduce yet another common module just for an option, but it
>>>>> sounds little appealing to me in comparison with the needed
>>>>> resources.
>>>>>
>>>>> Basically the deployment of SST is only for the already existing
>>>>> devices, and all newer should go for SOF.  And, the pattern Pierre
>>>>> mentioned should cover almost all use cases.  This made me believing
>>>>> that a simple switch is no mandatory request.
>>>>>
>>>>> In anyway, Jaroslav's patch looks like a good starting point.  We can
>>>>> build up a few more exceptions (SKl/KBl/APL Chromebooks with DMIC) on
>>>>> top of it, then we've done mostly, right?
>>>>
>>>> Yes, there are only a handful of quirks really.
>>>
>>> It seems like a not very rubust solution for me. If you don't like to have
>>> a standalone module, the code might be included to all drivers, but we losethe
>>> possibility to control the auto detection behaviour from the one place
>>> (one module parameter).
>>>
>>> Perhaps, we can rename snd_intel_nhlt() module and put the auto-detection code
>>> there. It's required by all mentioned drivers, so the extra resources required
>>> for this new code are minimal.
>>
>> it's kinda what my patches were about, there was a central set of quirks
>> in sound/soc/intel/common called by SOF/SST drivers.
> 
> Yes, but no legacy HDA... I can expect that we want to use DSP to enable some
> effects or decoders on hardware which has this co-processor in the near future.

I don't disagree, but I have more modest goals for the near future. 
There are quite a few intermediate steps to make before enabling fancy 
processing, with the transition to HDAudio+DMICs there are multiple gaps 
in the stack, e.g. the dependency on UCM, support for mute buttons and 
LEDs, hardware volumes handled by PulseAudio, firmware/topologies pushed 
to distributions with packages, etc.