Re: [PATCH 1/2] ALSA: HD-Audio: SKL+: abort probe if DSP is present and Skylake driver selected

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 12/10/18 10:06 AM, Takashi Iwai wrote:
> On Mon, 10 Dec 2018 17:05:00 +0100,
> Takashi Iwai wrote:
>> On Mon, 10 Dec 2018 16:57:49 +0100,
>> Pierre-Louis Bossart wrote:
>>>
>>> On 12/10/18 9:08 AM, Takashi Iwai wrote:
>>>> On Mon, 10 Dec 2018 15:31:08 +0100,
>>>> Pierre-Louis Bossart wrote:
>>>>> On 12/8/18 1:56 AM, Takashi Iwai wrote:
>>>>>> On Sat, 08 Dec 2018 01:00:38 +0100,
>>>>>> Pierre-Louis Bossart wrote:
>>>>>>> Now that the SST/Skylake driver supports per platform selectors, we
>>>>>>> can add logic to automatically select the right driver.
>>>>>>>
>>>>>>> If the Skylake driver is selected, and the DSP is enable, the legacy
>>>>>>> HDaudio driver aborts the probe. This will result in a single driver
>>>>>>> probing and remove the need for modprobe blacklists.
>>>>>>>
>>>>>>> Follow-up patches will add a module parameter to bypass the logic if
>>>>>>> this automatic detection fails, or if the Skylake driver is unable to
>>>>>>> actually support the platform (firmware authentication, missing
>>>>>>> topology file, hardware issue, etc).
>>>>>>>
>>>>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>>>>> ---
>>>>>>>     sound/pci/hda/Kconfig          | 46 ++++++++++++++++++++++++++++++++++
>>>>>>>     sound/pci/hda/hda_controller.h |  2 +-
>>>>>>>     sound/pci/hda/hda_intel.c      | 34 +++++++++++++++++++------
>>>>>>>     sound/soc/intel/Kconfig        |  6 +++++
>>>>>>>     4 files changed, 80 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
>>>>>>> index 4235907b7858..634b7fe6a936 100644
>>>>>>> --- a/sound/pci/hda/Kconfig
>>>>>>> +++ b/sound/pci/hda/Kconfig
>>>>>>> @@ -226,6 +226,52 @@ config SND_HDA_POWER_SAVE_DEFAULT
>>>>>>>     	  The default time-out value in seconds for HD-audio automatic
>>>>>>>     	  power-save mode.  0 means to disable the power-save mode.
>>>>>>>     +if SND_HDA_INTEL
>>>>>>> +
>>>>>>> +config SND_HDA_INTEL_DISABLE_SKL
>>>>>>> +	bool
>>>>>>> +	help
>>>>>>> +	  This option disables HD-audio legacy for
>>>>>>> +	  Skylake machines
>>>>>> I'm not sure whether we need the selection of this disablement for
>>>>>> each model.  Distros would choose these unlikely, and individual users
>>>>>> don't have to select multiple of them but only for their machine's
>>>>>> model.  So, in the end, the choice would be either yes or no.
>>>>> Ah yes, maybe I wasn't clear. This wasn't intended to be selected by
>>>>> the user, but selected when when the SND_SOC_INTEL_KBL or
>>>>> SND_SOC_SOF_CNL options are set. See the conditions below.
>>>>>
>>>>> The main idea what to only deal with the conflict resolution when we
>>>>> indeed have a conflict. I also introduced this option on the
>>>>> sound/pci/hda side so that SOF can use the same mechanisms, i.e. it's
>>>>> the legacy driver doesn't need to know if the conflict happens with
>>>>> the SST/Skylake or SOF driver.
>>>> OK, that makes sense.
>>>>
>>>> But then better to rephrase the help texts there for avoiding
>>>> confusion.  Currently it sounds as if the kconfig always disables the
>>>> support of the given chipset.  But the actual behavior is to disable
>>>> the binding with the legacy driver *only if* the PCI device class is
>>>> declared for Intel DSP.
>>> ok, will respin the help text.
>>>
>>> I was wondering if my email client ate your answers, was is the only
>>> change you wanted? In reply to the cover letter you mentioned "some
>>> comments" but I only see this one that needs an update, and no
>>> comments for the initial series of Skylake-specific patches.
>> Maybe you missed my comments for the second and later hunks of
>> patch#2?  It was about some dev_warn() and dev_err() usages, which I
>> suggested to degrade to dev_info().
Ah yes, sorry. I knew I was missing something.
> Oh, BTW, did the fallback mechanism work properly with your patches on
> the actual machines?
>
> At the last time I tried on SKL, the fallback failed by some reason,
> the driver core didn't try to load and bind two drivers.

I tested on two platforms, one WHL with DSP and one without (Skylake HP 
device), and the 3 values and didn't see any errors. lspci -vvv reports 
the two drivers registered but only the one I wanted as 'in use'.

I'll run more experiments on KBL and APL NUCs.

>
>
> Takashi