Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Jaroslav Kysela <perex@perex.cz>
To: Takashi Iwai <tiwai@suse.de>
Cc: Cezary Rojewski <cezary.rojewski@intel.com>,
	ALSA development <alsa-devel@alsa-project.org>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Subject: Re: [alsa-devel] [PATCH v3] ALSA: hda: add Intel DSP configuration / probe code
Date: Mon, 7 Oct 2019 18:13:59 +0200
Message-ID: <faa55989-4e04-3463-384f-1bb35e5d3816@perex.cz> (raw)
In-Reply-To: <s5h5zl1jpcx.wl-tiwai@suse.de>

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

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-06 15:22 Jaroslav Kysela
2019-10-07  1:50 ` Takashi Iwai
2019-10-07 16:13   ` Jaroslav Kysela [this message]
2019-10-07 14:05 ` Pierre-Louis Bossart
2019-10-07 16:03   ` Jaroslav Kysela
2019-10-07 16:44     ` Pierre-Louis Bossart
2019-10-08 18:44     ` Pierre-Louis Bossart
2019-10-09  6:26       ` Jaroslav Kysela

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=faa55989-4e04-3463-384f-1bb35e5d3816@perex.cz \
    --to=perex@perex.cz \
    --cc=alsa-devel@alsa-project.org \
    --cc=cezary.rojewski@intel.com \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=tiwai@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git