Re: [alsa-devel] [PATCH v3] ALSA: hda: add Intel DSP configuration / probe code

[Jaroslav Kysela <perex@perex.cz>]

Dne 07. 10. 19 v 3:50 Takashi Iwai napsal(a):
> On Sun, 06 Oct 2019 17:22:32 +0200,
> Jaroslav Kysela wrote:
>>
>> For distributions, we need one place where we can decide
>> which driver will be activated for the auto-configation of the
>> Intel's HDA hardware with DSP. Actually, we cover three drivers:
>>
>> * Legacy HDA
>> * Intel SST
>> * Intel Sound Open Firmware (SOF)
>>
>> All those drivers registers similar PCI IDs, so the first
>> driver probed from the PCI stack can win. But... it is not
>> guaranteed that the correct driver wins.
>>
>> This commit changes Intel's NHLT ACPI module to a common
>> DSP probe module for the Intel's hardware. All above sound
>> drivers calls this code. The user can force another behaviour
>> using the module parameter 'dsp_driver' located in
>> the 'snd-intel-dspcfg' module.
>>
>> This change allows to add specific dmi checks for the specific
>> systems. The examples are taken from the pull request:
>>
>>   https://github.com/thesofproject/linux/pull/927
>>
>> Tested on Lenovo Carbon X1 7th gen.
>>
>> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
>> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Cc: Cezary Rojewski <cezary.rojewski@intel.com>
> 
> Since I've been still traveling, just a few nitpicking:
> 
>> --- a/sound/hda/Makefile
>> +++ b/sound/hda/Makefile
>> @@ -14,5 +14,8 @@ obj-$(CONFIG_SND_HDA_CORE) += snd-hda-core.o
>>  #extended hda
>>  obj-$(CONFIG_SND_HDA_EXT_CORE) += ext/
>>  
>> -snd-intel-nhlt-objs := intel-nhlt.o
>> -obj-$(CONFIG_SND_INTEL_NHLT) += snd-intel-nhlt.o
>> +snd-intel-dspcfg-objs := intel-dsp-config.o
>> +ifneq ($(CONFIG_ACPI),)
>> +  snd-intel-dspcfg-objs += intel-nhlt.o
>> +endif
> 
> This can be simplified by
>   snd-intel-dspcfg-$(CONFIG_SND_INTEL_NHLT) += intel-nhlt.o

Yes, I added the ACPI condition later and didn't return back to Makefile.
Fixed in v4 (will post after comments from Pierre-Louis).

>> --- /dev/null
>> +++ b/sound/hda/intel-dsp-config.c
>> @@ -0,0 +1,247 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2019 Jaroslav Kysela <perex@perex.cz>
>> +
>> +#include <sound/core.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/dmi.h>
>> +#include <sound/intel-nhlt.h>
>> +#include <sound/intel-dsp-config.h>
> 
> Please put sound/core.h after linux/*.h inclusions.
> Also in general the inclusions are arranged in the alphabetical order
> nowadays.

Uff. Rules and rules for rules. Will fix that in v4.

>> +int snd_intel_dsp_driver_probe(struct pci_dev *pci)
>> +{
>> +	const struct config_entry *cfg;
>> +
>> +	if (dsp_driver > 0 && dsp_driver <= SND_INTEL_DSP_DRIVER_LAST)
>> +		return dsp_driver;
>> +
>> +	/* Intel vendor only */
>> +	if (snd_BUG_ON(pci->vendor != 0x8086))
>> +		return SND_INTEL_DSP_DRIVER_ANY;
>> +
>> +	/*
>> +	 * detect DSP by checking class/subclass/prog-id information
>> +	 * class=04 subclass 03 prog-if 00: no DSP, use legacy driver
>> +	 * class=04 subclass 01 prog-if 00: DSP is present
>> +	 *  (and may be required e.g. for DMIC or SSP support)
>> +	 * class=04 subclass 03 prog-if 80: use DSP or legacy mode
>> +	 */
>> +	if (pci->class == 0x040300)
>> +		return SND_INTEL_DSP_DRIVER_LEGACY;
>> +	if (pci->class != 0x040100 && pci->class != 0x040380) {
>> +		dev_err(&pci->dev, "Unknown PCI class/subclass/prog-if information (0x%06x) found, selecting HDA legacy driver\n", pci->class);
>> +		return SND_INTEL_DSP_DRIVER_LEGACY;
>> +	}
> 
> If we treat this as an error, we should provide a code to work around
> this (e.g. quirk entry or such), so that the error can be avoided
> later.
It' s really an error. This path should not be executed with the known,
functional hardware. The user might use dsp_driver module parameter to change
the behaviour.

> 
>> --- a/sound/pci/hda/hda_intel.c
>> +++ b/sound/pci/hda/hda_intel.c
> ....
>> @@ -124,7 +124,7 @@ static char *patch[SNDRV_CARDS];
>>  static bool beep_mode[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] =
>>  					CONFIG_SND_HDA_INPUT_BEEP_MODE};
>>  #endif
>> -static bool dmic_detect = IS_ENABLED(CONFIG_SND_HDA_INTEL_DETECT_DMIC);
>> +static bool no_dsp_driver;
>>  
>>  module_param_array(index, int, NULL, 0444);
>>  MODULE_PARM_DESC(index, "Index value for Intel HD audio interface.");
>> @@ -159,8 +159,8 @@ module_param_array(beep_mode, bool, NULL, 0444);
>>  MODULE_PARM_DESC(beep_mode, "Select HDA Beep registration mode "
>>  			    "(0=off, 1=on) (default=1).");
>>  #endif
>> -module_param(dmic_detect, bool, 0444);
>> -MODULE_PARM_DESC(dmic_detect, "DMIC detect on SKL+ platforms");
>> +module_param(no_dsp_driver, bool, 0444);
>> +MODULE_PARM_DESC(no_dsp_driver, "Force this driver to be used and bypass the DSP driver selection");
> 
> A negative option is often confusing, e.g. you may pass like
> no_dsp_driver=no.  Better to use a positive form if possible.

I just tried to copy the dmic_detect value, but yes, it might be confusion. I
will change that in v4.

				Jaroslav


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel