[PATCH 00/14] ASoC: Intel/SOF: extend run-time driver selection to ACPI devices
The module snd-intel-dspcfg, suggested by Jaroslav last year, currently provide the means to select a PCI driver at run-time, based on quirks, recommendations or user selection via a kernel parameter. This capability removed a lot of confusions in distributions and removed the need for recompilations to select legacy HDaudio, SST or SOF drivers.
This patchset extends the concept to ACPI devices. This was driven by the desire to at some point deprecate the Atom/SST driver for Baytrail and Cherrytrail, which is no longer maintained by Intel. By having the SOF driver enabled by distributions for Baytrail/Cherrytrail, we can enable more end-user tests and make the transition easier for distributions (likely in 2021 at this point).
This patchset provides the same solution for Broadwell, mainly to have a single build for all Intel platforms. SOF on Broadwell remains an option not recommended for distributions, as long as the 'catpt' driver is maintained there is no burning desire to make SOF the default on the three Broadwell-based platforms with the DSP enabled.
Pierre-Louis Bossart (14): ASoC: Intel: broadwell: add missing pm_ops ASoC: Intel: bdw-rt5677: add missing pm_ops ALSA: hda: intel-dsp-config: add helper for ACPI DSP driver selection ASoC: soc-acpi: add helper to identify parent driver. ASoC: Intel: boards: byt/cht: set card and driver name at run time ASoC: Intel: byt/cht: set pm ops dynamically ASoC: SOF: acpi: add dynamic selection of DSP driver ASoC: Intel: Atom: add dynamic selection of DSP driver ASoC: SOF: Intel: allow for coexistence between SOF and Atom/SST drivers ALSA: hda: intel-dsp-config: add Broadwell ACPI DSP driver selection ASoC: Intel: broadwell: set card and driver name dynamically ASoC: Intel: catpt: add dynamic selection of DSP driver ASoC: SOF: Intel: allow for coexistence between SOF and catpt drivers ALSA: hda: intel-dsp-config: ignore dsp_driver parameter for PCI legacy devices
include/sound/intel-dsp-config.h | 7 ++ include/sound/soc-acpi.h | 6 + sound/hda/intel-dsp-config.c | 111 +++++++++++++++++++ sound/soc/intel/Kconfig | 2 + sound/soc/intel/atom/sst/sst_acpi.c | 8 ++ sound/soc/intel/boards/bdw-rt5650.c | 17 ++- sound/soc/intel/boards/bdw-rt5677.c | 18 ++- sound/soc/intel/boards/broadwell.c | 20 ++-- sound/soc/intel/boards/bytcht_cx2072x.c | 27 +++-- sound/soc/intel/boards/bytcht_da7213.c | 27 +++-- sound/soc/intel/boards/bytcht_es8316.c | 29 +++-- sound/soc/intel/boards/bytcr_rt5640.c | 30 +++-- sound/soc/intel/boards/bytcr_rt5651.c | 27 +++-- sound/soc/intel/boards/cht_bsw_max98090_ti.c | 29 +++-- sound/soc/intel/boards/cht_bsw_nau8824.c | 29 +++-- sound/soc/intel/boards/cht_bsw_rt5645.c | 38 ++++--- sound/soc/intel/boards/cht_bsw_rt5672.c | 29 +++-- sound/soc/intel/catpt/device.c | 12 ++ sound/soc/sof/intel/Kconfig | 33 +++--- sound/soc/sof/sof-acpi-dev.c | 14 ++- 20 files changed, 392 insertions(+), 121 deletions(-)
For some reason this ops is missing in 2 out of the 3 broadwell drivers. Add to make sure ASoC takes care of power management.
Tested-by: Cezary Rojewski cezary.rojewski@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/soc/intel/boards/broadwell.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/intel/boards/broadwell.c b/sound/soc/intel/boards/broadwell.c index 77c85f17aca6..69e0b13b47f4 100644 --- a/sound/soc/intel/boards/broadwell.c +++ b/sound/soc/intel/boards/broadwell.c @@ -318,6 +318,7 @@ static struct platform_driver broadwell_audio = { .remove = broadwell_audio_remove, .driver = { .name = "broadwell-audio", + .pm = &snd_soc_pm_ops }, };
On 2020-11-12 11:38 PM, Pierre-Louis Bossart wrote:
For some reason this ops is missing in 2 out of the 3 broadwell drivers. Add to make sure ASoC takes care of power management.
Changes provided with this patch are not connected to the overall idea behind your patchset. I suggest splitting pm-changes into a separate one.
The very first sentence in commit message isn't providing any value, guess one-liner will suffice.
Tested-by: Cezary Rojewski cezary.rojewski@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
sound/soc/intel/boards/broadwell.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/intel/boards/broadwell.c b/sound/soc/intel/boards/broadwell.c index 77c85f17aca6..69e0b13b47f4 100644 --- a/sound/soc/intel/boards/broadwell.c +++ b/sound/soc/intel/boards/broadwell.c @@ -318,6 +318,7 @@ static struct platform_driver broadwell_audio = { .remove = broadwell_audio_remove, .driver = { .name = "broadwell-audio",
.pm = &snd_soc_pm_ops
For some reason this ops is missing in 2 out of the 3 broadwell drivers. Add to make sure ASoC takes care of power management.
Tested-by: Cezary Rojewski cezary.rojewski@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/soc/intel/boards/bdw-rt5677.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c index 7a3e773d0a1c..9cdd4164e1fb 100644 --- a/sound/soc/intel/boards/bdw-rt5677.c +++ b/sound/soc/intel/boards/bdw-rt5677.c @@ -446,6 +446,7 @@ static struct platform_driver bdw_rt5677_audio = { .probe = bdw_rt5677_probe, .driver = { .name = "bdw-rt5677", + .pm = &snd_soc_pm_ops }, };
On 2020-11-12 11:38 PM, Pierre-Louis Bossart wrote:
For some reason this ops is missing in 2 out of the 3 broadwell drivers. Add to make sure ASoC takes care of power management.
Changes provided with this patch are not connected to the overall idea behind your patchset. I suggest splitting pm-changes into a separate one.
The very first sentence in commit message isn't providing any value, guess one-liner will suffice.
Tested-by: Cezary Rojewski cezary.rojewski@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com
sound/soc/intel/boards/bdw-rt5677.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c index 7a3e773d0a1c..9cdd4164e1fb 100644 --- a/sound/soc/intel/boards/bdw-rt5677.c +++ b/sound/soc/intel/boards/bdw-rt5677.c @@ -446,6 +446,7 @@ static struct platform_driver bdw_rt5677_audio = { .probe = bdw_rt5677_probe, .driver = { .name = "bdw-rt5677",
.pm = &snd_soc_pm_ops
Mirror capabilities provided for PCI devices, so that distributions can select which ACPI driver is loaded at run-time with kernel parameters and DMI tables instead of forcing a build-time selection.
The "legacy" option supported for HDaudio has no meaning here and will be ignored.
The 'SST' driver based on closed-source firmware has the priority to avoid any impact on users, and the choice to use SOF is strictly opt-in. This may change at some point when the 'SST' driver is deprecated on Baytrail/Cherrytrail.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- include/sound/intel-dsp-config.h | 7 +++ sound/hda/intel-dsp-config.c | 77 ++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+)
diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h index c36622bee3f8..d4609077c258 100644 --- a/include/sound/intel-dsp-config.h +++ b/include/sound/intel-dsp-config.h @@ -21,6 +21,7 @@ enum { #if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
int snd_intel_dsp_driver_probe(struct pci_dev *pci); +int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]);
#else
@@ -29,6 +30,12 @@ static inline int snd_intel_dsp_driver_probe(struct pci_dev *pci) return SND_INTEL_DSP_DRIVER_ANY; }
+static inline +int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]) +{ + return SND_INTEL_DSP_DRIVER_ANY; +} + #endif
#endif diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c index 1c5114dedda9..7e6b8571c138 100644 --- a/sound/hda/intel-dsp-config.c +++ b/sound/hda/intel-dsp-config.c @@ -29,6 +29,7 @@ MODULE_PARM_DESC(dsp_driver, "Force the DSP driver for Intel DSP (0=auto, 1=lega struct config_entry { u32 flags; u16 device; + u8 acpi_hid[ACPI_ID_LEN]; const struct dmi_system_id *dmi_table; };
@@ -433,6 +434,82 @@ int snd_intel_dsp_driver_probe(struct pci_dev *pci) } EXPORT_SYMBOL_GPL(snd_intel_dsp_driver_probe);
+/* + * configuration table + * - the order of similar ACPI ID entries is important! + * - the first successful match will win + */ +static const struct config_entry acpi_config_table[] = { +/* BayTrail */ +#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) + { + .flags = FLAG_SST, + .acpi_hid = "80860F28", + }, +#endif +#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) + { + .flags = FLAG_SOF, + .acpi_hid = "80860F28", + }, +#endif +/* CherryTrail */ +#if IS_ENABLED(CONFIG_SND_SST_ATOM_HIFI2_PLATFORM_ACPI) + { + .flags = FLAG_SST, + .acpi_hid = "808622A8", + }, +#endif +#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) + { + .flags = FLAG_SOF, + .acpi_hid = "808622A8", + }, +#endif +}; + +static const struct config_entry *snd_intel_acpi_dsp_find_config(const u8 acpi_hid[ACPI_ID_LEN], + const struct config_entry *table, + u32 len) +{ + for (; len > 0; len--, table++) { + if (memcmp(table->acpi_hid, acpi_hid, ACPI_ID_LEN)) + continue; + if (table->dmi_table && !dmi_check_system(table->dmi_table)) + continue; + return table; + } + return NULL; +} + +int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]) +{ + const struct config_entry *cfg; + + if (dsp_driver > SND_INTEL_DSP_DRIVER_LEGACY && dsp_driver <= SND_INTEL_DSP_DRIVER_LAST) + return dsp_driver; + + if (dsp_driver == SND_INTEL_DSP_DRIVER_LEGACY) { + dev_warn(dev, "dsp_driver parameter %d not supported, using automatic detection\n", + SND_INTEL_DSP_DRIVER_LEGACY); + } + + /* find the configuration for the specific device */ + cfg = snd_intel_acpi_dsp_find_config(acpi_hid, acpi_config_table, + ARRAY_SIZE(acpi_config_table)); + if (!cfg) + return SND_INTEL_DSP_DRIVER_ANY; + + if (cfg->flags & FLAG_SST) + return SND_INTEL_DSP_DRIVER_SST; + + if (cfg->flags & FLAG_SOF) + return SND_INTEL_DSP_DRIVER_SOF; + + return SND_INTEL_DSP_DRIVER_SST; +} +EXPORT_SYMBOL_GPL(snd_intel_acpi_dsp_driver_probe); + MODULE_LICENSE("GPL v2"); MODULE_DESCRIPTION("Intel DSP config driver"); MODULE_IMPORT_NS(SOUNDWIRE_INTEL_INIT);
Intel machine drivers are used by parent platform drivers based on closed-source firmware (Atom/SST and catpt) and SOF-based ones.
In some cases for ACPI-based platforms, the behavior of machine drivers needs to be modified depending on the parent type, typically for card names and power management.
An initial solution based on passing a boolean flag as a platform device parameter was tested earlier. Since it looked overkill, this patch suggests instead a simple string comparison to identify an SOF parent device/driver.
Suggested-by: Kai Vehmanen kai.vehmanen@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- include/sound/soc-acpi.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/include/sound/soc-acpi.h b/include/sound/soc-acpi.h index b16a844d16ef..9a43c44dcbbb 100644 --- a/include/sound/soc-acpi.h +++ b/include/sound/soc-acpi.h @@ -171,4 +171,10 @@ struct snd_soc_acpi_codecs { u8 codecs[SND_SOC_ACPI_MAX_CODECS][ACPI_ID_LEN]; };
+static inline bool snd_soc_acpi_sof_parent(struct device *dev) +{ + return dev->parent && dev->parent->driver && dev->parent->driver->name && + !strcmp(dev->parent->driver->name, "sof-audio-acpi"); +} + #endif
To avoid hard-coded variations between SOF and SST drivers, set the card name and driver dynamically depending on the parent type. This is the first pass required to let distributions select which drivers to use with kernel parameters instead of build-time selection.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/soc/intel/boards/bytcht_cx2072x.c | 20 +++++++++---- sound/soc/intel/boards/bytcht_da7213.c | 20 +++++++++---- sound/soc/intel/boards/bytcht_es8316.c | 22 +++++++++----- sound/soc/intel/boards/bytcr_rt5640.c | 22 +++++++++----- sound/soc/intel/boards/bytcr_rt5651.c | 20 +++++++++---- sound/soc/intel/boards/cht_bsw_max98090_ti.c | 22 +++++++++----- sound/soc/intel/boards/cht_bsw_nau8824.c | 22 +++++++++----- sound/soc/intel/boards/cht_bsw_rt5645.c | 31 +++++++++++++------- sound/soc/intel/boards/cht_bsw_rt5672.c | 22 +++++++++----- 9 files changed, 141 insertions(+), 60 deletions(-)
diff --git a/sound/soc/intel/boards/bytcht_cx2072x.c b/sound/soc/intel/boards/bytcht_cx2072x.c index 0b50b3646d55..762f09190f10 100644 --- a/sound/soc/intel/boards/bytcht_cx2072x.c +++ b/sound/soc/intel/boards/bytcht_cx2072x.c @@ -205,14 +205,12 @@ static struct snd_soc_dai_link byt_cht_cx2072x_dais[] = { }, };
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_NAME "bytcht cx2072x" /* card name will be 'sof-bytcht cx2072x' */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_NAME "bytcht cx2072x" /* card name will be 'sof-bytcht cx2072x' */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_NAME "bytcht-cx2072x" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
/* SoC card */ static struct snd_soc_card byt_cht_cx2072x_card = { @@ -236,6 +234,7 @@ static int snd_byt_cht_cx2072x_probe(struct platform_device *pdev) struct snd_soc_acpi_mach *mach; struct acpi_device *adev; int dai_index = 0; + bool sof_parent; int i, ret;
byt_cht_cx2072x_card.dev = &pdev->dev; @@ -265,6 +264,17 @@ static int snd_byt_cht_cx2072x_probe(struct platform_device *pdev) if (ret) return ret;
+ sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); + + /* set card and driver name */ + if (sof_parent) { + byt_cht_cx2072x_card.name = SOF_CARD_NAME; + byt_cht_cx2072x_card.driver_name = SOF_DRIVER_NAME; + } else { + byt_cht_cx2072x_card.name = CARD_NAME; + byt_cht_cx2072x_card.driver_name = DRIVER_NAME; + } + return devm_snd_soc_register_card(&pdev->dev, &byt_cht_cx2072x_card); }
diff --git a/sound/soc/intel/boards/bytcht_da7213.c b/sound/soc/intel/boards/bytcht_da7213.c index e1e46b4bbac5..ef6682226a85 100644 --- a/sound/soc/intel/boards/bytcht_da7213.c +++ b/sound/soc/intel/boards/bytcht_da7213.c @@ -205,14 +205,12 @@ static struct snd_soc_dai_link dailink[] = { }, };
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_NAME "bytcht da7213" /* card name will be 'sof-bytcht da7213' */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_NAME "bytcht da7213" /* card name will be 'sof-bytcht da7213' */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_NAME "bytcht-da7213" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
/* SoC card */ static struct snd_soc_card bytcht_da7213_card = { @@ -237,6 +235,7 @@ static int bytcht_da7213_probe(struct platform_device *pdev) struct snd_soc_acpi_mach *mach; const char *platform_name; struct acpi_device *adev; + bool sof_parent; int dai_index = 0; int ret_val = 0; int i; @@ -269,6 +268,17 @@ static int bytcht_da7213_probe(struct platform_device *pdev) if (ret_val) return ret_val;
+ sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); + + /* set card and driver name */ + if (sof_parent) { + bytcht_da7213_card.name = SOF_CARD_NAME; + bytcht_da7213_card.driver_name = SOF_DRIVER_NAME; + } else { + bytcht_da7213_card.name = CARD_NAME; + bytcht_da7213_card.driver_name = DRIVER_NAME; + } + ret_val = devm_snd_soc_register_card(&pdev->dev, card); if (ret_val) { dev_err(&pdev->dev, diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c index 7ed869bf1a92..fbb62bab9dcc 100644 --- a/sound/soc/intel/boards/bytcht_es8316.c +++ b/sound/soc/intel/boards/bytcht_es8316.c @@ -406,18 +406,14 @@ static int byt_cht_es8316_resume(struct snd_soc_card *card) return 0; }
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_NAME "bytcht es8316" /* card name will be 'sof-bytcht es8316' */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_NAME "bytcht es8316" /* card name will be 'sof-bytcht es8316' */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_NAME "bytcht-es8316" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
static struct snd_soc_card byt_cht_es8316_card = { - .name = CARD_NAME, - .driver_name = DRIVER_NAME, .owner = THIS_MODULE, .dai_link = byt_cht_es8316_dais, .num_links = ARRAY_SIZE(byt_cht_es8316_dais), @@ -472,6 +468,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) const char *platform_name; struct acpi_device *adev; struct device *codec_dev; + bool sof_parent; unsigned int cnt = 0; int dai_index = 0; int i; @@ -590,6 +587,17 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) byt_cht_es8316_card.long_name = long_name; #endif
+ sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); + + /* set card and driver name */ + if (sof_parent) { + byt_cht_es8316_card.name = SOF_CARD_NAME; + byt_cht_es8316_card.driver_name = SOF_DRIVER_NAME; + } else { + byt_cht_es8316_card.name = CARD_NAME; + byt_cht_es8316_card.driver_name = DRIVER_NAME; + } + /* register the soc card */ snd_soc_card_set_drvdata(&byt_cht_es8316_card, priv);
diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c index 9dadf6561444..574b489a3283 100644 --- a/sound/soc/intel/boards/bytcr_rt5640.c +++ b/sound/soc/intel/boards/bytcr_rt5640.c @@ -1136,18 +1136,14 @@ static int byt_rt5640_resume(struct snd_soc_card *card) return 0; }
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_NAME "bytcht rt5640" /* card name will be 'sof-bytcht rt5640' */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_NAME "bytcht rt5640" /* card name will be 'sof-bytcht rt5640' */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_NAME "bytcr-rt5640" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
static struct snd_soc_card byt_rt5640_card = { - .name = CARD_NAME, - .driver_name = DRIVER_NAME, .owner = THIS_MODULE, .dai_link = byt_rt5640_dais, .num_links = ARRAY_SIZE(byt_rt5640_dais), @@ -1173,6 +1169,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) struct snd_soc_acpi_mach *mach; const char *platform_name; struct acpi_device *adev; + bool sof_parent; int ret_val = 0; int dai_index = 0; int i; @@ -1336,6 +1333,17 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) if (ret_val) return ret_val;
+ sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); + + /* set card and driver name */ + if (sof_parent) { + byt_rt5640_card.name = SOF_CARD_NAME; + byt_rt5640_card.driver_name = SOF_DRIVER_NAME; + } else { + byt_rt5640_card.name = CARD_NAME; + byt_rt5640_card.driver_name = DRIVER_NAME; + } + ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card);
if (ret_val) { diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 64d3fc4a3225..69e1d84e4de3 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -827,14 +827,12 @@ static int byt_rt5651_resume(struct snd_soc_card *card) return 0; }
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_NAME "bytcht rt5651" /* card name will be 'sof-bytcht rt5651' */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_NAME "bytcht rt5651" /* card name will be 'sof-bytcht rt5651' */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_NAME "bytcr-rt5651" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
static struct snd_soc_card byt_rt5651_card = { .name = CARD_NAME, @@ -876,6 +874,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) const char *platform_name; struct acpi_device *adev; struct device *codec_dev; + bool sof_parent; bool is_bytcr = false; int ret_val = 0; int dai_index = 0; @@ -1093,6 +1092,17 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) if (ret_val) return ret_val;
+ sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); + + /* set card and driver name */ + if (sof_parent) { + byt_rt5651_card.name = SOF_CARD_NAME; + byt_rt5651_card.driver_name = SOF_DRIVER_NAME; + } else { + byt_rt5651_card.name = CARD_NAME; + byt_rt5651_card.driver_name = DRIVER_NAME; + } + ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5651_card);
if (ret_val) { diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c index 835e9bd6b52d..a1d456d7a9c2 100644 --- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c +++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c @@ -382,19 +382,15 @@ static struct snd_soc_dai_link cht_dailink[] = { }, };
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_NAME "bytcht max98090" /* card name will be 'sof-bytcht max98090 */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_NAME "bytcht max98090" /* card name will be 'sof-bytcht max98090 */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_NAME "chtmax98090" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
/* SoC card */ static struct snd_soc_card snd_soc_card_cht = { - .name = CARD_NAME, - .driver_name = DRIVER_NAME, .owner = THIS_MODULE, .dai_link = cht_dailink, .num_links = ARRAY_SIZE(cht_dailink), @@ -540,6 +536,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev) const char *mclk_name; struct snd_soc_acpi_mach *mach; const char *platform_name; + bool sof_parent;
drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL); if (!drv) @@ -602,6 +599,17 @@ static int snd_cht_mc_probe(struct platform_device *pdev) } }
+ sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); + + /* set card and driver name */ + if (sof_parent) { + snd_soc_card_cht.name = SOF_CARD_NAME; + snd_soc_card_cht.driver_name = SOF_DRIVER_NAME; + } else { + snd_soc_card_cht.name = CARD_NAME; + snd_soc_card_cht.driver_name = DRIVER_NAME; + } + ret_val = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_cht); if (ret_val) { dev_err(&pdev->dev, diff --git a/sound/soc/intel/boards/cht_bsw_nau8824.c b/sound/soc/intel/boards/cht_bsw_nau8824.c index 3e12bff15fed..f173793d867b 100644 --- a/sound/soc/intel/boards/cht_bsw_nau8824.c +++ b/sound/soc/intel/boards/cht_bsw_nau8824.c @@ -231,19 +231,15 @@ static struct snd_soc_dai_link cht_dailink[] = { }, };
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_NAME "bytcht nau8824" /* card name will be 'sof-bytcht nau8824 */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_NAME "bytcht nau8824" /* card name will be 'sof-bytcht nau8824 */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_NAME "chtnau8824" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
/* SoC card */ static struct snd_soc_card snd_soc_card_cht = { - .name = CARD_NAME, - .driver_name = DRIVER_NAME, .owner = THIS_MODULE, .dai_link = cht_dailink, .num_links = ARRAY_SIZE(cht_dailink), @@ -260,6 +256,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev) struct cht_mc_private *drv; struct snd_soc_acpi_mach *mach; const char *platform_name; + bool sof_parent; int ret_val;
drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL); @@ -277,6 +274,17 @@ static int snd_cht_mc_probe(struct platform_device *pdev) if (ret_val) return ret_val;
+ sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); + + /* set card and driver name */ + if (sof_parent) { + snd_soc_card_cht.name = SOF_CARD_NAME; + snd_soc_card_cht.driver_name = SOF_DRIVER_NAME; + } else { + snd_soc_card_cht.name = CARD_NAME; + snd_soc_card_cht.driver_name = DRIVER_NAME; + } + /* register the soc card */ ret_val = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_cht); if (ret_val) { diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c index b53c02481749..bdaf8d00fc6b 100644 --- a/sound/soc/intel/boards/cht_bsw_rt5645.c +++ b/sound/soc/intel/boards/cht_bsw_rt5645.c @@ -479,21 +479,17 @@ static struct snd_soc_dai_link cht_dailink[] = { }, };
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_RT5645_NAME "bytcht rt5645" /* card name 'sof-bytcht rt5645' */ -#define CARD_RT5650_NAME "bytcht rt5650" /* card name 'sof-bytcht rt5650' */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_RT5645_NAME "bytcht rt5645" /* card name 'sof-bytcht rt5645' */ +#define SOF_CARD_RT5650_NAME "bytcht rt5650" /* card name 'sof-bytcht rt5650' */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_RT5645_NAME "chtrt5645" #define CARD_RT5650_NAME "chtrt5650" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
/* SoC card */ static struct snd_soc_card snd_soc_card_chtrt5645 = { - .name = CARD_RT5645_NAME, - .driver_name = DRIVER_NAME, .owner = THIS_MODULE, .dai_link = cht_dailink, .num_links = ARRAY_SIZE(cht_dailink), @@ -506,8 +502,6 @@ static struct snd_soc_card snd_soc_card_chtrt5645 = { };
static struct snd_soc_card snd_soc_card_chtrt5650 = { - .name = CARD_RT5650_NAME, - .driver_name = DRIVER_NAME, .owner = THIS_MODULE, .dai_link = cht_dailink, .num_links = ARRAY_SIZE(cht_dailink), @@ -541,6 +535,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev) const char *platform_name; struct cht_mc_private *drv; struct acpi_device *adev; + bool sof_parent; bool found = false; bool is_bytcr = false; int dai_index = 0; @@ -680,6 +675,22 @@ static int snd_cht_mc_probe(struct platform_device *pdev) }
snd_soc_card_set_drvdata(card, drv); + + sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); + + /* set card and driver name */ + if (sof_parent) { + snd_soc_card_chtrt5645.name = SOF_CARD_RT5645_NAME; + snd_soc_card_chtrt5645.driver_name = SOF_DRIVER_NAME; + snd_soc_card_chtrt5650.name = SOF_CARD_RT5650_NAME; + snd_soc_card_chtrt5650.driver_name = SOF_DRIVER_NAME; + } else { + snd_soc_card_chtrt5645.name = CARD_RT5645_NAME; + snd_soc_card_chtrt5645.driver_name = DRIVER_NAME; + snd_soc_card_chtrt5650.name = CARD_RT5650_NAME; + snd_soc_card_chtrt5650.driver_name = DRIVER_NAME; + } + ret_val = devm_snd_soc_register_card(&pdev->dev, card); if (ret_val) { dev_err(&pdev->dev, diff --git a/sound/soc/intel/boards/cht_bsw_rt5672.c b/sound/soc/intel/boards/cht_bsw_rt5672.c index 8442be93eb1c..6c46bfc43b50 100644 --- a/sound/soc/intel/boards/cht_bsw_rt5672.c +++ b/sound/soc/intel/boards/cht_bsw_rt5672.c @@ -382,19 +382,15 @@ static int cht_resume_post(struct snd_soc_card *card) return 0; }
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_NAME "bytcht rt5672" /* card name will be 'sof-bytcht rt5672' */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_NAME "bytcht rt5672" /* card name will be 'sof-bytcht rt5672' */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_NAME "cht-bsw-rt5672" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
/* SoC card */ static struct snd_soc_card snd_soc_card_cht = { - .name = CARD_NAME, - .driver_name = DRIVER_NAME, .owner = THIS_MODULE, .dai_link = cht_dailink, .num_links = ARRAY_SIZE(cht_dailink), @@ -417,6 +413,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev) struct snd_soc_acpi_mach *mach = pdev->dev.platform_data; const char *platform_name; struct acpi_device *adev; + bool sof_parent; int i;
drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL); @@ -458,6 +455,17 @@ static int snd_cht_mc_probe(struct platform_device *pdev) } snd_soc_card_set_drvdata(&snd_soc_card_cht, drv);
+ sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); + + /* set card and driver name */ + if (sof_parent) { + snd_soc_card_cht.name = SOF_CARD_NAME; + snd_soc_card_cht.driver_name = SOF_DRIVER_NAME; + } else { + snd_soc_card_cht.name = CARD_NAME; + snd_soc_card_cht.driver_name = DRIVER_NAME; + } + /* register the soc card */ ret_val = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_cht); if (ret_val) {
What`s the mean? how to use kernel parameter at run time?
On 4/25/21 1:13 PM, youling257 wrote:
What`s the mean? how to use kernel parameter at run time?
see the options for the snd-intel-dspcfg module
For byt/cht, if you use dsp_driver=3 you will select SOF. if you use dsp_driver=2 or don't set the value at all, you will select the legacy Atom/sst driver, which is still the default
The Atom/SST driver does not rely on ASoC power management, but the SOF driver does. Rather than using a hard-coded build-time assignment, we can set this pm_ops dynamically depending on what the parent is. That will remove the last build-time dependency and allow for coexistence of both SST and SOF drivers for Baytrail/Cherrytrail.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/soc/intel/boards/bytcht_cx2072x.c | 7 ++++--- sound/soc/intel/boards/bytcht_da7213.c | 7 ++++--- sound/soc/intel/boards/bytcht_es8316.c | 7 ++++--- sound/soc/intel/boards/bytcr_rt5640.c | 8 +++++--- sound/soc/intel/boards/bytcr_rt5651.c | 7 ++++--- sound/soc/intel/boards/cht_bsw_max98090_ti.c | 7 ++++--- sound/soc/intel/boards/cht_bsw_nau8824.c | 7 ++++--- sound/soc/intel/boards/cht_bsw_rt5645.c | 7 ++++--- sound/soc/intel/boards/cht_bsw_rt5672.c | 7 ++++--- 9 files changed, 37 insertions(+), 27 deletions(-)
diff --git a/sound/soc/intel/boards/bytcht_cx2072x.c b/sound/soc/intel/boards/bytcht_cx2072x.c index 762f09190f10..2bfe3e4c696f 100644 --- a/sound/soc/intel/boards/bytcht_cx2072x.c +++ b/sound/soc/intel/boards/bytcht_cx2072x.c @@ -275,15 +275,16 @@ static int snd_byt_cht_cx2072x_probe(struct platform_device *pdev) byt_cht_cx2072x_card.driver_name = DRIVER_NAME; }
+ /* set pm ops */ + if (sof_parent) + pdev->dev.driver->pm = &snd_soc_pm_ops; + return devm_snd_soc_register_card(&pdev->dev, &byt_cht_cx2072x_card); }
static struct platform_driver snd_byt_cht_cx2072x_driver = { .driver = { .name = "bytcht_cx2072x", -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - .pm = &snd_soc_pm_ops, -#endif }, .probe = snd_byt_cht_cx2072x_probe, }; diff --git a/sound/soc/intel/boards/bytcht_da7213.c b/sound/soc/intel/boards/bytcht_da7213.c index ef6682226a85..cfeba27252ba 100644 --- a/sound/soc/intel/boards/bytcht_da7213.c +++ b/sound/soc/intel/boards/bytcht_da7213.c @@ -279,6 +279,10 @@ static int bytcht_da7213_probe(struct platform_device *pdev) bytcht_da7213_card.driver_name = DRIVER_NAME; }
+ /* set pm ops */ + if (sof_parent) + pdev->dev.driver->pm = &snd_soc_pm_ops; + ret_val = devm_snd_soc_register_card(&pdev->dev, card); if (ret_val) { dev_err(&pdev->dev, @@ -292,9 +296,6 @@ static int bytcht_da7213_probe(struct platform_device *pdev) static struct platform_driver bytcht_da7213_driver = { .driver = { .name = "bytcht_da7213", -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - .pm = &snd_soc_pm_ops, -#endif }, .probe = bytcht_da7213_probe, }; diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c index fbb62bab9dcc..892cf684216e 100644 --- a/sound/soc/intel/boards/bytcht_es8316.c +++ b/sound/soc/intel/boards/bytcht_es8316.c @@ -598,6 +598,10 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) byt_cht_es8316_card.driver_name = DRIVER_NAME; }
+ /* set pm ops */ + if (sof_parent) + dev->driver->pm = &snd_soc_pm_ops; + /* register the soc card */ snd_soc_card_set_drvdata(&byt_cht_es8316_card, priv);
@@ -623,9 +627,6 @@ static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev) static struct platform_driver snd_byt_cht_es8316_mc_driver = { .driver = { .name = "bytcht_es8316", -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - .pm = &snd_soc_pm_ops, -#endif }, .probe = snd_byt_cht_es8316_mc_probe, .remove = snd_byt_cht_es8316_mc_remove, diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c index 574b489a3283..18f668fbc64c 100644 --- a/sound/soc/intel/boards/bytcr_rt5640.c +++ b/sound/soc/intel/boards/bytcr_rt5640.c @@ -1163,6 +1163,7 @@ struct acpi_chan_package { /* ACPICA seems to require 64 bit integers */
static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) { + struct device *dev = &pdev->dev; static const char * const map_name[] = { "dmic1", "dmic2", "in1", "in3" }; const struct dmi_system_id *dmi_id; struct byt_rt5640_private *priv; @@ -1344,6 +1345,10 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) byt_rt5640_card.driver_name = DRIVER_NAME; }
+ /* set pm ops */ + if (sof_parent) + dev->driver->pm = &snd_soc_pm_ops; + ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card);
if (ret_val) { @@ -1358,9 +1363,6 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) static struct platform_driver snd_byt_rt5640_mc_driver = { .driver = { .name = "bytcr_rt5640", -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - .pm = &snd_soc_pm_ops, -#endif }, .probe = snd_byt_rt5640_mc_probe, }; diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 69e1d84e4de3..f289ec8563a1 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -1103,6 +1103,10 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) byt_rt5651_card.driver_name = DRIVER_NAME; }
+ /* set pm ops */ + if (sof_parent) + pdev->dev.driver->pm = &snd_soc_pm_ops; + ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5651_card);
if (ret_val) { @@ -1117,9 +1121,6 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) static struct platform_driver snd_byt_rt5651_mc_driver = { .driver = { .name = "bytcr_rt5651", -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - .pm = &snd_soc_pm_ops, -#endif }, .probe = snd_byt_rt5651_mc_probe, }; diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c index a1d456d7a9c2..131882378a59 100644 --- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c +++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c @@ -610,6 +610,10 @@ static int snd_cht_mc_probe(struct platform_device *pdev) snd_soc_card_cht.driver_name = DRIVER_NAME; }
+ /* set pm ops */ + if (sof_parent) + dev->driver->pm = &snd_soc_pm_ops; + ret_val = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_cht); if (ret_val) { dev_err(&pdev->dev, @@ -634,9 +638,6 @@ static int snd_cht_mc_remove(struct platform_device *pdev) static struct platform_driver snd_cht_mc_driver = { .driver = { .name = "cht-bsw-max98090", -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - .pm = &snd_soc_pm_ops, -#endif }, .probe = snd_cht_mc_probe, .remove = snd_cht_mc_remove, diff --git a/sound/soc/intel/boards/cht_bsw_nau8824.c b/sound/soc/intel/boards/cht_bsw_nau8824.c index f173793d867b..8131af1730f7 100644 --- a/sound/soc/intel/boards/cht_bsw_nau8824.c +++ b/sound/soc/intel/boards/cht_bsw_nau8824.c @@ -285,6 +285,10 @@ static int snd_cht_mc_probe(struct platform_device *pdev) snd_soc_card_cht.driver_name = DRIVER_NAME; }
+ /* set pm ops */ + if (sof_parent) + pdev->dev.driver->pm = &snd_soc_pm_ops; + /* register the soc card */ ret_val = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_cht); if (ret_val) { @@ -300,9 +304,6 @@ static int snd_cht_mc_probe(struct platform_device *pdev) static struct platform_driver snd_cht_mc_driver = { .driver = { .name = "cht-bsw-nau8824", -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - .pm = &snd_soc_pm_ops, -#endif }, .probe = snd_cht_mc_probe, }; diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c index bdaf8d00fc6b..6fea554cfed5 100644 --- a/sound/soc/intel/boards/cht_bsw_rt5645.c +++ b/sound/soc/intel/boards/cht_bsw_rt5645.c @@ -691,6 +691,10 @@ static int snd_cht_mc_probe(struct platform_device *pdev) snd_soc_card_chtrt5650.driver_name = DRIVER_NAME; }
+ /* set pm ops */ + if (sof_parent) + pdev->dev.driver->pm = &snd_soc_pm_ops; + ret_val = devm_snd_soc_register_card(&pdev->dev, card); if (ret_val) { dev_err(&pdev->dev, @@ -704,9 +708,6 @@ static int snd_cht_mc_probe(struct platform_device *pdev) static struct platform_driver snd_cht_mc_driver = { .driver = { .name = "cht-bsw-rt5645", -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - .pm = &snd_soc_pm_ops, -#endif }, .probe = snd_cht_mc_probe, }; diff --git a/sound/soc/intel/boards/cht_bsw_rt5672.c b/sound/soc/intel/boards/cht_bsw_rt5672.c index 6c46bfc43b50..10c88ef2f85d 100644 --- a/sound/soc/intel/boards/cht_bsw_rt5672.c +++ b/sound/soc/intel/boards/cht_bsw_rt5672.c @@ -466,6 +466,10 @@ static int snd_cht_mc_probe(struct platform_device *pdev) snd_soc_card_cht.driver_name = DRIVER_NAME; }
+ /* set pm ops */ + if (sof_parent) + pdev->dev.driver->pm = &snd_soc_pm_ops; + /* register the soc card */ ret_val = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_cht); if (ret_val) { @@ -480,9 +484,6 @@ static int snd_cht_mc_probe(struct platform_device *pdev) static struct platform_driver snd_cht_mc_driver = { .driver = { .name = "cht-bsw-rt5672", -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - .pm = &snd_soc_pm_ops, -#endif }, .probe = snd_cht_mc_probe, };
On Thu, Nov 12, 2020 at 04:38:17PM -0600, Pierre-Louis Bossart wrote:
- /* set pm ops */
- if (sof_parent)
pdev->dev.driver->pm = &snd_soc_pm_ops;
This might need revisiting in future since we should be able to have the driver PM ops be static const and hence unwritable but that's more of a robustness thing for the time being given that only a limited set of systems have this hardware and we know that there can't be multiple devices.
On 11/17/20 11:18 AM, Mark Brown wrote:
On Thu, Nov 12, 2020 at 04:38:17PM -0600, Pierre-Louis Bossart wrote:
- /* set pm ops */
- if (sof_parent)
pdev->dev.driver->pm = &snd_soc_pm_ops;This might need revisiting in future since we should be able to have the driver PM ops be static const and hence unwritable but that's more of a robustness thing for the time being given that only a limited set of systems have this hardware and we know that there can't be multiple devices.
FWIW it's been done in other places, e.g.
drivers/net/wireless/ti/wlcore/main.c: wl->dev->driver->pm = &wlcore_pm_ops; drivers/net/wireless/ti/wlcore/main.c: wl->dev->driver->pm = NULL;
The alternative would be to add an .ops whose callbacks conditionally call snd_soc_suspend/resume/poweroff. Not much cleaner, is it?
The check on the 'sof_parent' was not present in initial versions, I had an additional 'machine parameter' set by the SOF driver. But early reviewers suggested a check on the parent name was enough. It achieves the same thing in the end, make sure that we don't change anything for power management when the Atom/SST driver is used.
On Tue, Nov 17, 2020 at 11:39:48AM -0600, Pierre-Louis Bossart wrote:
On 11/17/20 11:18 AM, Mark Brown wrote:
On Thu, Nov 12, 2020 at 04:38:17PM -0600, Pierre-Louis Bossart wrote:
- /* set pm ops */
- if (sof_parent)
pdev->dev.driver->pm = &snd_soc_pm_ops;
This might need revisiting in future since we should be able to have the driver PM ops be static const and hence unwritable but that's more of a robustness thing for the time being given that only a limited set of systems have this hardware and we know that there can't be multiple devices.
FWIW it's been done in other places, e.g.
drivers/net/wireless/ti/wlcore/main.c: wl->dev->driver->pm = &wlcore_pm_ops; drivers/net/wireless/ti/wlcore/main.c: wl->dev->driver->pm = NULL;
The alternative would be to add an .ops whose callbacks conditionally call snd_soc_suspend/resume/poweroff. Not much cleaner, is it?
It's not really about cleanliness, it's about being able to mark the ops struct as const and therefore make it read only which the security people like.
The check on the 'sof_parent' was not present in initial versions, I had an additional 'machine parameter' set by the SOF driver. But early reviewers suggested a check on the parent name was enough. It achieves the same thing in the end, make sure that we don't change anything for power management when the Atom/SST driver is used.
The check is fine.
Follow PCI example and stop the probe when another driver is desired for the same ACPI HID.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/soc/sof/intel/Kconfig | 1 + sound/soc/sof/sof-acpi-dev.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index b233302f05e4..b593b29789d5 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -78,6 +78,7 @@ config SND_SOC_SOF_BAYTRAIL_SUPPORT config SND_SOC_SOF_BAYTRAIL tristate select SND_SOC_SOF_INTEL_ATOM_HIFI_EP + select SND_INTEL_DSP_CONFIG help This option is not user-selectable but automagically handled by 'select' statements at a higher level. diff --git a/sound/soc/sof/sof-acpi-dev.c b/sound/soc/sof/sof-acpi-dev.c index a78b76ef37b2..2a369c2c6551 100644 --- a/sound/soc/sof/sof-acpi-dev.c +++ b/sound/soc/sof/sof-acpi-dev.c @@ -12,6 +12,7 @@ #include <linux/firmware.h> #include <linux/module.h> #include <linux/pm_runtime.h> +#include <sound/intel-dsp-config.h> #include <sound/soc-acpi.h> #include <sound/soc-acpi-intel-match.h> #include <sound/sof.h> @@ -120,12 +121,23 @@ static void sof_acpi_probe_complete(struct device *dev) static int sof_acpi_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; + const struct acpi_device_id *id; const struct sof_dev_desc *desc; struct snd_sof_pdata *sof_pdata; const struct snd_sof_dsp_ops *ops; int ret;
- dev_dbg(&pdev->dev, "ACPI DSP detected"); + id = acpi_match_device(dev->driver->acpi_match_table, dev); + if (!id) + return -ENODEV; + + ret = snd_intel_acpi_dsp_driver_probe(dev, id->id); + if (ret != SND_INTEL_DSP_DRIVER_ANY && ret != SND_INTEL_DSP_DRIVER_SOF) { + dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n"); + return -ENODEV; + } + + dev_dbg(dev, "ACPI DSP detected");
sof_pdata = devm_kzalloc(dev, sizeof(*sof_pdata), GFP_KERNEL); if (!sof_pdata)
Follow PCI example and stop the probe when another driver is desired for the same ACPI HID.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/soc/intel/Kconfig | 1 + sound/soc/intel/atom/sst/sst_acpi.c | 8 ++++++++ 2 files changed, 9 insertions(+)
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index a5b446d5af19..4e9f910751a9 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -55,6 +55,7 @@ config SND_SST_ATOM_HIFI2_PLATFORM_ACPI depends on X86 && ACPI && PCI select SND_SST_ATOM_HIFI2_PLATFORM select SND_SOC_ACPI_INTEL_MATCH + select SND_INTEL_DSP_CONFIG select IOSF_MBI help If you have a Intel Baytrail or Cherrytrail platform with an I2S diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c index f943a0884976..2c1b8a2e3506 100644 --- a/sound/soc/intel/atom/sst/sst_acpi.c +++ b/sound/soc/intel/atom/sst/sst_acpi.c @@ -21,6 +21,7 @@ #include <linux/acpi.h> #include <asm/platform_sst_audio.h> #include <sound/core.h> +#include <sound/intel-dsp-config.h> #include <sound/soc.h> #include <sound/compress_driver.h> #include <acpi/acbuffer.h> @@ -246,6 +247,13 @@ static int sst_acpi_probe(struct platform_device *pdev) id = acpi_match_device(dev->driver->acpi_match_table, dev); if (!id) return -ENODEV; + + ret = snd_intel_acpi_dsp_driver_probe(dev, id->id); + if (ret != SND_INTEL_DSP_DRIVER_ANY && ret != SND_INTEL_DSP_DRIVER_SST) { + dev_dbg(dev, "SST ACPI driver not selected, aborting probe\n"); + return -ENODEV; + } + dev_dbg(dev, "for %s\n", id->id);
mach = (struct snd_soc_acpi_mach *)id->driver_data;
Now that we have all the support needed for coexistence between ACPI drivers for Baytrail and Cherrytrail, remove mutual exclusion in the Kconfig file. The selection is done by playing with the snd_intel_dsp module parameter.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/soc/sof/intel/Kconfig | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index b593b29789d5..59b35fa87e0e 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -61,17 +61,16 @@ if SND_SOC_SOF_INTEL_ACPI
config SND_SOC_SOF_BAYTRAIL_SUPPORT bool "SOF support for Baytrail, Braswell and Cherrytrail" - depends on SND_SST_ATOM_HIFI2_PLATFORM_ACPI=n help This adds support for Sound Open Firmware for Intel(R) platforms using the Baytrail, Braswell or Cherrytrail processors. - This option is mutually exclusive with the Atom/SST and Baytrail - legacy drivers. If you want to enable SOF on Baytrail/Cherrytrail, - you need to deselect those options first. - SOF does not support Baytrail-CR for now, so this option is not - recommended for distros. At some point all legacy drivers will be - deprecated but not before all userspace firmware/topology/UCM files - are made available to downstream distros. + This option can coexist in the same build with the Atom legacy + drivers, currently the default but which will be deprecated + at some point. + Existing firmware/topology binaries and UCM configurations + typically located in the root file system are already + compatible with both SOF or Atom/SST legacy drivers. + This is a recommended option for distributions. Say Y if you want to enable SOF on Baytrail/Cherrytrail. If unsure select "N".
Add ACPI IDs for Broadwell (and Haswell for consistency). This addition is required for dynamic selection of drivers on those devices.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/hda/intel-dsp-config.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c index 7e6b8571c138..0dc079ba02ff 100644 --- a/sound/hda/intel-dsp-config.c +++ b/sound/hda/intel-dsp-config.c @@ -466,6 +466,26 @@ static const struct config_entry acpi_config_table[] = { .acpi_hid = "808622A8", }, #endif +/* Broadwell */ +#if IS_ENABLED(CONFIG_SND_SOC_INTEL_CATPT) + { + .flags = FLAG_SST, + .acpi_hid = "INT3438" + }, +#endif +#if IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL) + { + .flags = FLAG_SOF, + .acpi_hid = "INT3438" + }, +#endif +/* Haswell - not supported by SOF but added for consistency */ +#if IS_ENABLED(CONFIG_SND_SOC_INTEL_CATPT) + { + .flags = FLAG_SST, + .acpi_hid = "INT33C8" + }, +#endif };
static const struct config_entry *snd_intel_acpi_dsp_find_config(const u8 acpi_hid[ACPI_ID_LEN],
Remove last hard-coded build-time dependency
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/soc/intel/boards/bdw-rt5650.c | 17 ++++++++++++----- sound/soc/intel/boards/bdw-rt5677.c | 17 ++++++++++++----- sound/soc/intel/boards/broadwell.c | 19 ++++++++++++------- 3 files changed, 36 insertions(+), 17 deletions(-)
diff --git a/sound/soc/intel/boards/bdw-rt5650.c b/sound/soc/intel/boards/bdw-rt5650.c index aa420b201848..c5122d3b0e6c 100644 --- a/sound/soc/intel/boards/bdw-rt5650.c +++ b/sound/soc/intel/boards/bdw-rt5650.c @@ -262,14 +262,12 @@ static struct snd_soc_dai_link bdw_rt5650_dais[] = { }, };
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_NAME "bdw rt5650" /* card name will be 'sof-bdw rt5650' */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_NAME "bdw rt5650" /* card name will be 'sof-bdw rt5650' */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_NAME "bdw-rt5650" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
/* ASoC machine driver for Broadwell DSP + RT5650 */ static struct snd_soc_card bdw_rt5650_card = { @@ -309,6 +307,15 @@ static int bdw_rt5650_probe(struct platform_device *pdev) if (ret) return ret;
+ /* set card and driver name */ + if (snd_soc_acpi_sof_parent(&pdev->dev)) { + bdw_rt5650_card.name = SOF_CARD_NAME; + bdw_rt5650_card.driver_name = SOF_DRIVER_NAME; + } else { + bdw_rt5650_card.name = CARD_NAME; + bdw_rt5650_card.driver_name = DRIVER_NAME; + } + snd_soc_card_set_drvdata(&bdw_rt5650_card, bdw_rt5650);
return devm_snd_soc_register_card(&pdev->dev, &bdw_rt5650_card); diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c index 9cdd4164e1fb..021bc59aac80 100644 --- a/sound/soc/intel/boards/bdw-rt5677.c +++ b/sound/soc/intel/boards/bdw-rt5677.c @@ -387,14 +387,12 @@ static int bdw_rt5677_resume_post(struct snd_soc_card *card) return 0; }
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_NAME "bdw rt5677" /* card name will be 'sof-bdw rt5677' */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_NAME "bdw rt5677" /* card name will be 'sof-bdw rt5677' */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_NAME "bdw-rt5677" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
/* ASoC machine driver for Broadwell DSP + RT5677 */ static struct snd_soc_card bdw_rt5677_card = { @@ -437,6 +435,15 @@ static int bdw_rt5677_probe(struct platform_device *pdev) if (ret) return ret;
+ /* set card and driver name */ + if (snd_soc_acpi_sof_parent(&pdev->dev)) { + bdw_rt5677_card.name = SOF_CARD_NAME; + bdw_rt5677_card.driver_name = SOF_DRIVER_NAME; + } else { + bdw_rt5677_card.name = CARD_NAME; + bdw_rt5677_card.driver_name = DRIVER_NAME; + } + snd_soc_card_set_drvdata(&bdw_rt5677_card, bdw_rt5677);
return devm_snd_soc_register_card(&pdev->dev, &bdw_rt5677_card); diff --git a/sound/soc/intel/boards/broadwell.c b/sound/soc/intel/boards/broadwell.c index 69e0b13b47f4..3c3aff9c61cc 100644 --- a/sound/soc/intel/boards/broadwell.c +++ b/sound/soc/intel/boards/broadwell.c @@ -262,19 +262,15 @@ static int broadwell_resume(struct snd_soc_card *card){ return 0; }
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL) /* use space before codec name to simplify card ID, and simplify driver name */ -#define CARD_NAME "bdw rt286" /* card name will be 'sof-bdw rt286' */ -#define DRIVER_NAME "SOF" -#else +#define SOF_CARD_NAME "bdw rt286" /* card name will be 'sof-bdw rt286' */ +#define SOF_DRIVER_NAME "SOF" + #define CARD_NAME "broadwell-rt286" #define DRIVER_NAME NULL /* card name will be used for driver name */ -#endif
/* broadwell audio machine driver for WPT + RT286S */ static struct snd_soc_card broadwell_rt286 = { - .name = CARD_NAME, - .driver_name = DRIVER_NAME, .owner = THIS_MODULE, .dai_link = broadwell_rt286_dais, .num_links = ARRAY_SIZE(broadwell_rt286_dais), @@ -303,6 +299,15 @@ static int broadwell_audio_probe(struct platform_device *pdev) if (ret) return ret;
+ /* set card and driver name */ + if (snd_soc_acpi_sof_parent(&pdev->dev)) { + broadwell_rt286.name = SOF_CARD_NAME; + broadwell_rt286.driver_name = SOF_DRIVER_NAME; + } else { + broadwell_rt286.name = CARD_NAME; + broadwell_rt286.driver_name = DRIVER_NAME; + } + return devm_snd_soc_register_card(&pdev->dev, &broadwell_rt286); }
Follow PCI example and stop the probe when another driver is desired for the same ACPI HID.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/soc/intel/Kconfig | 1 + sound/soc/intel/catpt/device.c | 12 ++++++++++++ 2 files changed, 13 insertions(+)
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index 4e9f910751a9..051687d97039 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -24,6 +24,7 @@ config SND_SOC_INTEL_CATPT depends on DMADEVICES && SND_DMA_SGBUF select DW_DMAC_CORE select SND_SOC_ACPI_INTEL_MATCH + select SND_INTEL_DSP_CONFIG help Enable support for Intel(R) Haswell and Broadwell platforms with I2S codec present. This is a recommended option. diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c index a70179959795..613585c3016f 100644 --- a/sound/soc/intel/catpt/device.c +++ b/sound/soc/intel/catpt/device.c @@ -19,6 +19,7 @@ #include <linux/pci.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <sound/intel-dsp-config.h> #include <sound/soc.h> #include <sound/soc-acpi.h> #include <sound/soc-acpi-intel-match.h> @@ -239,9 +240,20 @@ static int catpt_acpi_probe(struct platform_device *pdev) const struct catpt_spec *spec; struct catpt_dev *cdev; struct device *dev = &pdev->dev; + const struct acpi_device_id *id; struct resource *res; int ret;
+ id = acpi_match_device(dev->driver->acpi_match_table, dev); + if (!id) + return -ENODEV; + + ret = snd_intel_acpi_dsp_driver_probe(dev, id->id); + if (ret != SND_INTEL_DSP_DRIVER_ANY && ret != SND_INTEL_DSP_DRIVER_SST) { + dev_dbg(dev, "CATPT ACPI driver not selected, aborting probe\n"); + return -ENODEV; + } + spec = device_get_match_data(dev); if (!spec) return -ENODEV;
Now that we have all the support needed for coexistence between ACPI drivers for Broadwell, remove mutual exclusion in the Kconfig file. The selection is done by playing with the snd_intel_dspcfg module 'dsp_driver' parameter.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/soc/sof/intel/Kconfig | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index 59b35fa87e0e..084f5993f043 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -84,17 +84,18 @@ config SND_SOC_SOF_BAYTRAIL
config SND_SOC_SOF_BROADWELL_SUPPORT bool "SOF support for Broadwell" - depends on SND_SOC_INTEL_HASWELL=n + select SND_INTEL_DSP_CONFIG help This adds support for Sound Open Firmware for Intel(R) platforms using the Broadwell processors. - This option is mutually exclusive with the Haswell/Broadwell legacy - driver. If you want to enable SOF on Broadwell you need to deselect - the legacy driver first. - SOF does not fully support Broadwell yet, so this option is not - recommended for distros. At some point all legacy drivers will be - deprecated but not before all userspace firmware/topology/UCM files - are made available to downstream distros. + This option can coexist in the same build with the default 'catpt' + driver. + Existing firmware/topology binaries and UCM configurations typically + located in the root file system are already compatible with both SOF + or catpt drivers. + SOF does not fully support Broadwell and has limitations related to + DMA and suspend-resume, this is not a recommended option for + distributions. Say Y if you want to enable SOF on Broadwell. If unsure select "N".
On Thu, Nov 12, 2020 at 04:38:24PM -0600, Pierre-Louis Bossart wrote:
Now that we have all the support needed for coexistence between ACPI drivers for Broadwell, remove mutual exclusion in the Kconfig file. The selection is done by playing with the snd_intel_dspcfg module 'dsp_driver' parameter.
This breaks x86 allmodconfig builds for me:
/mnt/kernel/sound/soc/intel/atom/sst/sst_acpi.c: In function 'sst_acpi_probe': /mnt/kernel/sound/soc/intel/atom/sst/sst_acpi.c:251:8: error: implicit declaration of function 'snd_intel_acpi_dsp_driver_probe'; did you mean 'snd_intel_dsp_driver_probe'? [-Werror=implicit-function-declaration] ret = snd_intel_acpi_dsp_driver_probe(dev, id->id); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ snd_intel_dsp_driver_probe /mnt/kernel/sound/soc/intel/catpt/device.c: In function 'catpt_acpi_probe': /mnt/kernel/sound/soc/intel/catpt/device.c:251:8: error: implicit declaration of function 'snd_intel_acpi_dsp_driver_probe'; did you mean 'snd_intel_dsp_driver_probe'? [-Werror=implicit-function-declaration] ret = snd_intel_acpi_dsp_driver_probe(dev, id->id); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ snd_intel_dsp_driver_probe /mnt/kernel/sound/soc/sof/sof-acpi-dev.c: In function 'sof_acpi_probe': /mnt/kernel/sound/soc/sof/sof-acpi-dev.c:134:8: error: implicit declaration of function 'snd_intel_acpi_dsp_driver_probe'; did you mean 'snd_intel_dsp_driver_probe'? [-Werror=implicit-function-declaration] ret = snd_intel_acpi_dsp_driver_probe(dev, id->id); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ snd_intel_dsp_driver_probe
On 11/19/20 8:06 AM, Mark Brown wrote:
On Thu, Nov 12, 2020 at 04:38:24PM -0600, Pierre-Louis Bossart wrote:
Now that we have all the support needed for coexistence between ACPI drivers for Broadwell, remove mutual exclusion in the Kconfig file. The selection is done by playing with the snd_intel_dspcfg module 'dsp_driver' parameter.
This breaks x86 allmodconfig builds for me:
/mnt/kernel/sound/soc/intel/atom/sst/sst_acpi.c: In function 'sst_acpi_probe': /mnt/kernel/sound/soc/intel/atom/sst/sst_acpi.c:251:8: error: implicit declaration of function 'snd_intel_acpi_dsp_driver_probe'; did you mean 'snd_intel_dsp_driver_probe'? [-Werror=implicit-function-declaration] ret = snd_intel_acpi_dsp_driver_probe(dev, id->id); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ snd_intel_dsp_driver_probe /mnt/kernel/sound/soc/intel/catpt/device.c: In function 'catpt_acpi_probe': /mnt/kernel/sound/soc/intel/catpt/device.c:251:8: error: implicit declaration of function 'snd_intel_acpi_dsp_driver_probe'; did you mean 'snd_intel_dsp_driver_probe'? [-Werror=implicit-function-declaration] ret = snd_intel_acpi_dsp_driver_probe(dev, id->id); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ snd_intel_dsp_driver_probe /mnt/kernel/sound/soc/sof/sof-acpi-dev.c: In function 'sof_acpi_probe': /mnt/kernel/sound/soc/sof/sof-acpi-dev.c:134:8: error: implicit declaration of function 'snd_intel_acpi_dsp_driver_probe'; did you mean 'snd_intel_dsp_driver_probe'? [-Werror=implicit-function-declaration] ret = snd_intel_acpi_dsp_driver_probe(dev, id->id); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ snd_intel_dsp_driver_probe
Humm, I just tried and it works fine for me on top of your for-5.11 branch. That error across the board seems weird, there's even a fall-back with a static inline if the Kconfig is not selected.
Could it be that Patch3 was not applied somehow? That's where the prototype was added.
On Thu, Nov 19, 2020 at 11:52:22AM -0600, Pierre-Louis Bossart wrote:
Humm, I just tried and it works fine for me on top of your for-5.11 branch. That error across the board seems weird, there's even a fall-back with a static inline if the Kconfig is not selected.
Could it be that Patch3 was not applied somehow? That's where the prototype was added.
Looks like the patches got reordered because I had to manually add Takashi's acks since he added some spaces which confused b4.
On Haswell/Broadwell/Baytrail/Braswell, the DSP is not used for the HDMI/DP interface, and setting the dsp_driver parameter to a value > 1 has the side effect of preventing the HDaudio legacy driver from probing.
The DSP driver selection should really only handle cases where a DSP is actually used. This patch traps all known PCI devices and makes sure the HDaudio driver can always be probed.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com --- sound/hda/intel-dsp-config.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c index 0dc079ba02ff..6a0d070c60c9 100644 --- a/sound/hda/intel-dsp-config.c +++ b/sound/hda/intel-dsp-config.c @@ -379,6 +379,20 @@ int snd_intel_dsp_driver_probe(struct pci_dev *pci) if (pci->vendor != 0x8086) return SND_INTEL_DSP_DRIVER_ANY;
+ /* + * Legacy devices don't have a PCI-based DSP and use HDaudio + * for HDMI/DP support, ignore kernel parameter + */ + switch (pci->device) { + case 0x160c: /* Broadwell */ + case 0x0a0c: /* Haswell */ + case 0x0c0c: + case 0x0d0c: + case 0x0f04: /* Baytrail */ + case 0x2284: /* Braswell */ + return SND_INTEL_DSP_DRIVER_ANY; + } + if (dsp_driver > 0 && dsp_driver <= SND_INTEL_DSP_DRIVER_LAST) return dsp_driver;
On 2020-11-12 11:38 PM, Pierre-Louis Bossart wrote:
The module snd-intel-dspcfg, suggested by Jaroslav last year, currently provide the means to select a PCI driver at run-time, based on quirks, recommendations or user selection via a kernel parameter. This capability removed a lot of confusions in distributions and removed the need for recompilations to select legacy HDaudio, SST or SOF drivers.
This patchset extends the concept to ACPI devices. This was driven by the desire to at some point deprecate the Atom/SST driver for Baytrail and Cherrytrail, which is no longer maintained by Intel. By having the SOF driver enabled by distributions for Baytrail/Cherrytrail, we can enable more end-user tests and make the transition easier for distributions (likely in 2021 at this point).
This patchset provides the same solution for Broadwell, mainly to have a single build for all Intel platforms. SOF on Broadwell remains an option not recommended for distributions, as long as the 'catpt' driver is maintained there is no burning desire to make SOF the default on the three Broadwell-based platforms with the DSP enabled.
Hello,
Don't understand why I was omitted in CC for catpt-related patches.
I'll try to do proper review tomorrow as it's late here but for starters: why do we need any intel-dsp-config changes for non-HDA platforms, what's the technical reason behind these?
Czarek
On 2020-11-13 12:04 AM, Rojewski, Cezary wrote:
On 2020-11-12 11:38 PM, Pierre-Louis Bossart wrote:
The module snd-intel-dspcfg, suggested by Jaroslav last year, currently provide the means to select a PCI driver at run-time, based on quirks, recommendations or user selection via a kernel parameter. This capability removed a lot of confusions in distributions and removed the need for recompilations to select legacy HDaudio, SST or SOF drivers.
This patchset extends the concept to ACPI devices. This was driven by the desire to at some point deprecate the Atom/SST driver for Baytrail and Cherrytrail, which is no longer maintained by Intel. By having the SOF driver enabled by distributions for Baytrail/Cherrytrail, we can enable more end-user tests and make the transition easier for distributions (likely in 2021 at this point).
This patchset provides the same solution for Broadwell, mainly to have a single build for all Intel platforms. SOF on Broadwell remains an option not recommended for distributions, as long as the 'catpt' driver is maintained there is no burning desire to make SOF the default on the three Broadwell-based platforms with the DSP enabled.
Hello,
Don't understand why I was omitted in CC for catpt-related patches.
I'll try to do proper review tomorrow as it's late here but for starters: why do we need any intel-dsp-config changes for non-HDA platforms, what's the technical reason behind these?
Czarek
As almost all of the patches share the concerns, decided to provide entire output here so it's easier to navigate later on.
For a very long time upstream was filled with "flavors" of drivers for Intel solutions. Having three available for BYT is a very good example of that. The division of what goes where wasn't exactly explicit either. This all leads to confusion - while community and users may feel confused about what's recommended and what they should actually be using, surprisingly (unsurprisingly?) developers were too.
Latest changes provided by Intel employees were addressing exactly that. Removal of obsolete and redundant code. Together with fixing several issues that were bothering users of older solutions, net gain was: reduction of confusion and complexity factors.
Patchset presented here goes directly against that goal. We, Intel developers, are tasked with providing reliable, working solutions exposing best possible experience for our customers when dealing with our products. And thus solutions provided are called recommended. We don't deal with flavors and try-it-out-on-your-own-risk.
Moreover, intel-dsp-config module was created to address completely different problem - problem of selecting correct HDA driver given the circumstances. This is true as one cannot always rely on DSP-capability bit being enabled when HDA-legacy is meant to be the default solution on given platform. In future maybe we should revisit that subject once again as perhaps even the existing solution isn't resolving the problem completely.
Neither HSW/BDW nor atom-based platforms are HDA-based. Those are ACPI devices and you know upfront what we're dealing with. There is no DSP-capability bit to check for to know whether we're dealing with legacy solution or not. As these are explicit configurations, one needs not to bother with additional magic enums.
As long as Intel recommendation stays with /atom/ for atom-based products, so should linux kernel. Same for hsw/bdw - Intel recommends closed firmware solution and thus this should remain as the only available choice - leaving no place for confusion.
Once and if SOF is ready to support all available atom configuration, it should deprecate and replace it with the same fashion catpt replaced its predecessor. Until that moment, things should remain as they are. No additional quirks or magic, just plain simple ACPI-ID based selection.
Users that are making use of optional and opportunistic changes especially in regard to selecting not-recommended choices are experienced enough to rebuild their kernel and merge out-of-tree changes and they are free to do so if they want to. Upstream kernel, however, is not place for keeping such code.
Czarek
Once and if SOF is ready to support all available atom configuration, it should deprecate and replace it with the same fashion catpt replaced its predecessor. Until that moment, things should remain as they are. No additional quirks or magic, just plain simple ACPI-ID based selection.
We have lots of users for the existing legacy Baytrail/Cherrytrail driver and we cannot ask distributions to switch one sunny day.
I reached out to Hans and Jaroslav to understand how this deprecation might happen, and it has to be done in steps. First include the SOF/BYT-CHT driver in builds, experiment and test and after a successful test period switch over.
Keep in mind that Intel folks have only a very limited subset of the hardware based on BYT/CHT so we have to work with the community to get feedback, that's very different to Broadwell where there are only 3 platforms to be supported and an extremely limited number of users impacted by a switch since we are out of the ChromeOS support period.
In all cases, distributions and users, not Intel, make the call.
On Fri, Nov 13, 2020 at 01:06:48PM +0000, Rojewski, Cezary wrote:
For a very long time upstream was filled with "flavors" of drivers for Intel solutions. Having three available for BYT is a very good example of that. The division of what goes where wasn't exactly explicit either. This all leads to confusion - while community and users may feel confused about what's recommended and what they should actually be using, surprisingly (unsurprisingly?) developers were too.
...
Patchset presented here goes directly against that goal. We, Intel developers, are tasked with providing reliable, working solutions exposing best possible experience for our customers when dealing with our products. And thus solutions provided are called recommended. We don't deal with flavors and try-it-out-on-your-own-risk.
My feeling here was that this is helping with this goal in that it's not changing the defaults but is rather pushing the decision making process from build time to runtime. This means that distributions are able to ship the preferred implementations for all the platforms without causing any issues for the hopefully small set of users who need or want to work on a different firmware, if they've been doing something like sticking with an alternative firmware for old users since things were working they'll be able to smoothly transition over to the current recommended default, eg leaving old users on the old firmware by default. That's a bit of a niche use case but then hopefully all use cases for selecting a non-default firmware are niche.
It also means that people don't have to think about this so much at build time, they can just turn everything on and not worry they'll cause problems for people using the binary and still get the recommended runtime behaviour by default unless the user actively does something.
At any rate I'm not clear that I see this actively causing problems.
Hi,
On 11/13/20 5:49 PM, Mark Brown wrote:
On Fri, Nov 13, 2020 at 01:06:48PM +0000, Rojewski, Cezary wrote:
For a very long time upstream was filled with "flavors" of drivers for Intel solutions. Having three available for BYT is a very good example of that. The division of what goes where wasn't exactly explicit either. This all leads to confusion - while community and users may feel confused about what's recommended and what they should actually be using, surprisingly (unsurprisingly?) developers were too.
...
Patchset presented here goes directly against that goal. We, Intel developers, are tasked with providing reliable, working solutions exposing best possible experience for our customers when dealing with our products. And thus solutions provided are called recommended. We don't deal with flavors and try-it-out-on-your-own-risk.
My feeling here was that this is helping with this goal in that it's not changing the defaults but is rather pushing the decision making process from build time to runtime. This means that distributions are able to ship the preferred implementations for all the platforms without causing any issues for the hopefully small set of users who need or want to work on a different firmware, if they've been doing something like sticking with an alternative firmware for old users since things were working they'll be able to smoothly transition over to the current recommended default, eg leaving old users on the old firmware by default. That's a bit of a niche use case but then hopefully all use cases for selecting a non-default firmware are niche.
It also means that people don't have to think about this so much at build time, they can just turn everything on and not worry they'll cause problems for people using the binary and still get the recommended runtime behaviour by default unless the user actively does something
Exactly. As Pierre-Louis mentions the Intel team does not have most of the affected hardware. Since I've been working on making BYT and CHT based devices work better with Linux as a side project for the last couple of years I have been buying these kinda devices 2nd hand when ever I can get one cheap and I've built a big collection of these (one might say this has gotten out of hand a bit) see here:
https://github.com/jwrdegoede/sunxi-fedora-scripts/blob/master/x86-tablet-in... https://github.com/jwrdegoede/sunxi-fedora-scripts/blob/master/x86-codec-inf...
For my device collection (mostly the first link).
As Pierre-Louis mentioned I've already been working with him on getting ready to move everything BYT/CHT related over to SOF. I've already been testing SOF on various devices with mostly ok results so far. But this is a process not a switch which we can simply throw.
So I'm all in favor of this patch-set. With some luck we can switch the BYT/CHT default to SOF in Fedora for F34 beta (*), but doing that really sorta hinges on this patch-set so that users can easily try the old driver, both as a workaround for issues and to check if the problem is caused by the switch to SOF.
Talking about doing this for Fedora 34, I think that switching the default there is a good idea (and I can make this happen) what do others think about doing this?
Regards,
Hans
On 2020-11-13 6:06 PM, Hans de Goede wrote:
On 11/13/20 5:49 PM, Mark Brown wrote:
On Fri, Nov 13, 2020 at 01:06:48PM +0000, Rojewski, Cezary wrote:
For a very long time upstream was filled with "flavors" of drivers for Intel solutions. Having three available for BYT is a very good example of that. The division of what goes where wasn't exactly explicit either. This all leads to confusion - while community and users may feel confused about what's recommended and what they should actually be using, surprisingly (unsurprisingly?) developers were too.
...
Patchset presented here goes directly against that goal. We, Intel developers, are tasked with providing reliable, working solutions exposing best possible experience for our customers when dealing with our products. And thus solutions provided are called recommended. We don't deal with flavors and try-it-out-on-your-own-risk.
My feeling here was that this is helping with this goal in that it's not changing the defaults but is rather pushing the decision making process from build time to runtime. This means that distributions are able to ship the preferred implementations for all the platforms without causing any issues for the hopefully small set of users who need or want to work on a different firmware, if they've been doing something like sticking with an alternative firmware for old users since things were working they'll be able to smoothly transition over to the current recommended default, eg leaving old users on the old firmware by default. That's a bit of a niche use case but then hopefully all use cases for selecting a non-default firmware are niche.
It also means that people don't have to think about this so much at build time, they can just turn everything on and not worry they'll cause problems for people using the binary and still get the recommended runtime behaviour by default unless the user actively does something
Thanks for your input, Mark.
The ultimate goal is OK but the execution is not. Take a look at the following:
+static inline bool snd_soc_acpi_sof_parent(struct device *dev) +{ + return dev->parent && dev->parent->driver && dev->parent->driver->name && + !strcmp(dev->parent->driver->name, "sof-audio-acpi"); +} +
-and-
+ /* set pm ops */ + if (sof_parent) + pdev->dev.driver->pm = &snd_soc_pm_ops; +
-and-
+ /* set card and driver name */ + if (snd_soc_acpi_sof_parent(&pdev->dev)) { + bdw_rt5650_card.name = SOF_CARD_NAME; + bdw_rt5650_card.driver_name = SOF_DRIVER_NAME; + } else { + bdw_rt5650_card.name = CARD_NAME; + bdw_rt5650_card.driver_name = DRIVER_NAME; + } +
pieces that are appearing several times throughout the series. ASoC is a framework on its own. It is by all means an extension to an older, more general ALSA framework. With every passing month SOF code found in /sound/soc/sof gets closer to becoming yet another framework, one that is placed on top of ASoC. Samples taken from this series augment this statement. If ASoC framework is missing means for its child drivers to do specific things, it's better to update the framework than creating yet another one.
Explicit 'ifs' asking whether we're dealing with SOF or other solution is at best a code smell. I believe this is unnecessary complexity added to the code especially once you realize user needs to play with module parameters to switch between solutions. If we assume user is able to play with module parameters then why not simply make use of blacklist mechanism?
And last but not least: intel-dsp-config is (as already stated) a mean for solving the HDA-runtime-driver-selection problem. Mixing it with ACPI devices is another layer of confusion.
Exactly. As Pierre-Louis mentions the Intel team does not have most of the affected hardware. Since I've been working on making BYT and CHT based devices work better with Linux as a side project for the last couple of years I have been buying these kinda devices 2nd hand when ever
...
As Pierre-Louis mentioned I've already been working with him on getting ready to move everything BYT/CHT related over to SOF. I've already been testing SOF on various devices with mostly ok results so far. But this is a process not a switch which we can simply throw.
Hans, thanks for sharing your concerns.
I'm afraid it's basically impossible to be fully prepared as you and Pierre pointed out. Even when speaking about catpt and BDW, we too didn't have all the available production stuff.
And thus I don't believe there will ever be a "good moment" to switch. Once internal validation confirms driver is stable it's better to switch entirely to the new solution with CI and devs on standby - ready to address any upcoming reports. Don't believe /atom/ has clean slide anyway given the patches and issues being posted from time to time related to said solution.
I understand you're here for atom-based products yet the patchset also touches on catpt aka hsw/bdw. While to my knowledge old /atom/ has no active CI running - so the switch is desirable - the same cannot be said about catpt. Because of that, I don't see any reason for appending any catpt-related changes here. Leave no place for confusion. One solution for one architecture that's recommended and maintained.
Czarek
+static inline bool snd_soc_acpi_sof_parent(struct device *dev) +{
- return dev->parent && dev->parent->driver && dev->parent->driver->name &&
!strcmp(dev->parent->driver->name, "sof-audio-acpi");+}
-and-
- /* set pm ops */
- if (sof_parent)
pdev->dev.driver->pm = &snd_soc_pm_ops;
The legacy Baytrail/Cherrytrail driver uses its own power management flow instead of the ASoC one. This patch does not cause the problem, it recognizes instead that this legacy driver cannot use the same pm ops.
I wish we didn't have to do this but there's just no alternative.
Dynamically assigning the .pm ops is not illegal and has been done in other drivers.
-and-
- /* set card and driver name */
- if (snd_soc_acpi_sof_parent(&pdev->dev)) {
bdw_rt5650_card.name = SOF_CARD_NAME;bdw_rt5650_card.driver_name = SOF_DRIVER_NAME;- } else {
bdw_rt5650_card.name = CARD_NAME;bdw_rt5650_card.driver_name = DRIVER_NAME;- }
That is also intentional. We modified the card names based on Jaroslav's feedback, and this code change is just the mirror of what happened before with build-time code changes. Should we have changed the legacy card names? Maybe, but that might have added issues for users so we left the names untouched.
pieces that are appearing several times throughout the series. ASoC is a framework on its own. It is by all means an extension to an older, more general ALSA framework. With every passing month SOF code found in /sound/soc/sof gets closer to becoming yet another framework, one that is placed on top of ASoC. Samples taken from this series augment this statement. If ASoC framework is missing means for its child drivers to do specific things, it's better to update the framework than creating yet another one.
There are no ASoC devices or drivers, we use platform devices/drivers. I don't see any need for an ASoC extension here.
Explicit 'ifs' asking whether we're dealing with SOF or other solution is at best a code smell. I believe this is unnecessary complexity added to the code especially once you realize user needs to play with module parameters to switch between solutions. If we assume user is able to play with module parameters then why not simply make use of blacklist mechanism?
Been there, done that. We don't want to use either denylist of kernel parameters.
denylists create confusion for users, it's an endless stream of false errors and time lost in bug reports.
The use of the kernel parameter is ONLY for expert users who want to tinker with the system or debug issues, the average user should not have to play with either denylists or kernel parameters.
And last but not least: intel-dsp-config is (as already stated) a mean for solving the HDA-runtime-driver-selection problem. Mixing it with ACPI devices is another layer of confusion.
Why? Who said it was PCI only? We already take care of DMIC, SoundWire, Google Chromebooks, open platforms, why not ACPI? It's just one API to be used when more than one driver can register to the same device.
Exactly. As Pierre-Louis mentions the Intel team does not have most of the affected hardware. Since I've been working on making BYT and CHT based devices work better with Linux as a side project for the last couple of years I have been buying these kinda devices 2nd hand when ever
...
As Pierre-Louis mentioned I've already been working with him on getting ready to move everything BYT/CHT related over to SOF. I've already been testing SOF on various devices with mostly ok results so far. But this is a process not a switch which we can simply throw.
Hans, thanks for sharing your concerns.
I'm afraid it's basically impossible to be fully prepared as you and Pierre pointed out. Even when speaking about catpt and BDW, we too didn't have all the available production stuff.
And thus I don't believe there will ever be a "good moment" to switch. Once internal validation confirms driver is stable it's better to switch entirely to the new solution with CI and devs on standby - ready to address any upcoming reports. Don't believe /atom/ has clean slide anyway given the patches and issues being posted from time to time related to said solution.
You refer to tests made by Intel when we are talking about community based tests here. We precisely do not have 'CI and devs on standby', the transition will be made by distributions themselves.
Besides, CI cannot test jack detection and all the flavors of microphone/speaker placements, which are the source of 99% of the issues.
I understand you're here for atom-based products yet the patchset also touches on catpt aka hsw/bdw. While to my knowledge old /atom/ has no active CI running - so the switch is desirable - the same cannot be said about catpt. Because of that, I don't see any reason for appending any catpt-related changes here. Leave no place for confusion. One solution for one architecture that's recommended and maintained.
There is no confusion, the wording is explicit
" SOF does not fully support Broadwell and has limitations related to + DMA and suspend-resume, this is not a recommended option for + distributions. "
But there are niche users who prefer it for their own experiments, so what's the issue in making their life simpler?
On Mon, 16 Nov 2020 18:47:22 +0100, Pierre-Louis Bossart wrote:
Explicit 'ifs' asking whether we're dealing with SOF or other solution is at best a code smell. I believe this is unnecessary complexity added to the code especially once you realize user needs to play with module parameters to switch between solutions. If we assume user is able to play with module parameters then why not simply make use of blacklist mechanism?
Been there, done that. We don't want to use either denylist of kernel parameters.
denylists create confusion for users, it's an endless stream of false errors and time lost in bug reports.
The use of the kernel parameter is ONLY for expert users who want to tinker with the system or debug issues, the average user should not have to play with either denylists or kernel parameters.
I guess Cezary mean the modprobe blacklist? This was used in the early stage of ASoC Skylake driver development, but in the end, it's more cumbersome because user needs to change multiple places. The single module parameter was easier to handle.
And last but not least: intel-dsp-config is (as already stated) a mean for solving the HDA-runtime-driver-selection problem. Mixing it with ACPI devices is another layer of confusion.
Why? Who said it was PCI only? We already take care of DMIC, SoundWire, Google Chromebooks, open platforms, why not ACPI? It's just one API to be used when more than one driver can register to the same device.
Well, currently intel-dsp-config sits in sound/hda, which isn't really intuitive. Though, Intel driver file paths are already fairly scattered, so it doesn't matter too much :)
I don't mind to move it to another directory, but which one...? x86 might match, but shuffling the place won't help for maintenance.
I personally find this move good, at least it makes things easier for distros. There are small details like the above, but technically seen, I see no big problem.
thanks,
Takashi
On Tue, Nov 17, 2020 at 03:04:31PM +0100, Takashi Iwai wrote:
Pierre-Louis Bossart wrote:
Why? Who said it was PCI only? We already take care of DMIC, SoundWire, Google Chromebooks, open platforms, why not ACPI? It's just one API to be used when more than one driver can register to the same device.
Well, currently intel-dsp-config sits in sound/hda, which isn't really intuitive. Though, Intel driver file paths are already fairly scattered, so it doesn't matter too much :)
I don't mind to move it to another directory, but which one...? x86 might match, but shuffling the place won't help for maintenance.
Yeah, I don't have a strong opinion here on location. I'm not sure there is any decision which won't have downsides.
I personally find this move good, at least it makes things easier for distros. There are small details like the above, but technically seen, I see no big problem.
Indeed - this isn't ideal but that's more a product of the situation we're in, this seems to improve it so it seems like a win to me and the distros and other people dealing with end users seem happy in so far as they've spoken up. Would you be OK with me applying the ALSA patches in here?
On Tue, 17 Nov 2020 18:31:45 +0100, Mark Brown wrote:
On Tue, Nov 17, 2020 at 03:04:31PM +0100, Takashi Iwai wrote:
Pierre-Louis Bossart wrote:
Why? Who said it was PCI only? We already take care of DMIC, SoundWire, Google Chromebooks, open platforms, why not ACPI? It's just one API to be used when more than one driver can register to the same device.
Well, currently intel-dsp-config sits in sound/hda, which isn't really intuitive. Though, Intel driver file paths are already fairly scattered, so it doesn't matter too much :)
I don't mind to move it to another directory, but which one...? x86 might match, but shuffling the place won't help for maintenance.
Yeah, I don't have a strong opinion here on location. I'm not sure there is any decision which won't have downsides.
I personally find this move good, at least it makes things easier for distros. There are small details like the above, but technically seen, I see no big problem.
Indeed - this isn't ideal but that's more a product of the situation we're in, this seems to improve it so it seems like a win to me and the distros and other people dealing with end users seem happy in so far as they've spoken up. Would you be OK with me applying the ALSA patches in here?
Yes, feel free to take my ack: Acked-by: Takashi Iwai tiwai@suse.de
thanks,
Takashi
On 2020-11-17 3:04 PM, Takashi Iwai wrote:
On Mon, 16 Nov 2020 18:47:22 +0100, Pierre-Louis Bossart wrote:
Explicit 'ifs' asking whether we're dealing with SOF or other solution is at best a code smell. I believe this is unnecessary complexity added to the code especially once you realize user needs to play with module parameters to switch between solutions. If we assume user is able to play with module parameters then why not simply make use of blacklist mechanism?
Been there, done that. We don't want to use either denylist of kernel parameters.
denylists create confusion for users, it's an endless stream of false errors and time lost in bug reports.
The use of the kernel parameter is ONLY for expert users who want to tinker with the system or debug issues, the average user should not have to play with either denylists or kernel parameters.
I guess Cezary mean the modprobe blacklist? This was used in the early stage of ASoC Skylake driver development, but in the end, it's more cumbersome because user needs to change multiple places. The single module parameter was easier to handle.
Thanks for joining the discussion, Takashi.
If the switch of solution for atom-based products is imminent, why add code which becomes redundant soon after?
Yes, indeed I meant the modprobe blacklisting as it solves the problem without addition of any code. Doubt alsa-driver entries are scattered in /etc/modprobe.d/ so switching between one solution to another via blacklist becomes as easy as changing 'options intel-dsp-config <param>==<value>' entry.
In regard to catpt, solution is even simpler: just remove sound/soc/sof/intel/bdw.c as that code is not valid & recommended anyway and linux kernel is not place for such. There shouldn't be really any options for not recommended stuff. Leave the selection explicit.
And last but not least: intel-dsp-config is (as already stated) a mean for solving the HDA-runtime-driver-selection problem. Mixing it with ACPI devices is another layer of confusion.
Why? Who said it was PCI only? We already take care of DMIC, SoundWire, Google Chromebooks, open platforms, why not ACPI? It's just one API to be used when more than one driver can register to the same device.
Well, currently intel-dsp-config sits in sound/hda, which isn't really intuitive. Though, Intel driver file paths are already fairly scattered, so it doesn't matter too much :)
I don't mind to move it to another directory, but which one...? x86 might match, but shuffling the place won't help for maintenance.
I personally find this move good, at least it makes things easier for distros. There are small details like the above, but technically seen, I see no big problem.
Well, if non-Intel guys see the localization of code counter-intuitive then how about those who play with it daily.. The new "sof-parent" checks won't make maintaining any easier and I believe there are easier solutions as written above.
Czarek
On 11/17/20 4:13 PM, Rojewski, Cezary wrote:
On 2020-11-17 3:04 PM, Takashi Iwai wrote:
On Mon, 16 Nov 2020 18:47:22 +0100, Pierre-Louis Bossart wrote:
Explicit 'ifs' asking whether we're dealing with SOF or other solution is at best a code smell. I believe this is unnecessary complexity added to the code especially once you realize user needs to play with module parameters to switch between solutions. If we assume user is able to play with module parameters then why not simply make use of blacklist mechanism?
Been there, done that. We don't want to use either denylist of kernel parameters.
denylists create confusion for users, it's an endless stream of false errors and time lost in bug reports.
The use of the kernel parameter is ONLY for expert users who want to tinker with the system or debug issues, the average user should not have to play with either denylists or kernel parameters.
I guess Cezary mean the modprobe blacklist? This was used in the early stage of ASoC Skylake driver development, but in the end, it's more cumbersome because user needs to change multiple places. The single module parameter was easier to handle.
Thanks for joining the discussion, Takashi.
If the switch of solution for atom-based products is imminent, why add code which becomes redundant soon after?
To be clear: there is *no plan* to *remove* the Atom/sst code any time 'soon', only to *deprecate* it.
In the best case distributions would transition in 2021. Some distros are faster than others, neither you nor I have any control over this.
Removing code from the kernel is not something we can do unless there is demonstrated evidence that the number of impacted users is close to zero and distributions no longer support that code. The case of Baytrail legacy is telling, you removed it earlier this Fall but after a recommended alternative was provided for more than 3 years.
Again, there is no planned 'switch' but a gradual transition, and that patchset helps with the transition.
On 2020-11-17 11:53 PM, Pierre-Louis Bossart wrote:
On 11/17/20 4:13 PM, Rojewski, Cezary wrote:
On 2020-11-17 3:04 PM, Takashi Iwai wrote:
...
I guess Cezary mean the modprobe blacklist? This was used in the early stage of ASoC Skylake driver development, but in the end, it's more cumbersome because user needs to change multiple places. The single module parameter was easier to handle.
Thanks for joining the discussion, Takashi.
If the switch of solution for atom-based products is imminent, why add code which becomes redundant soon after?
To be clear: there is *no plan* to *remove* the Atom/sst code any time 'soon', only to *deprecate* it.
In the best case distributions would transition in 2021. Some distros are faster than others, neither you nor I have any control over this.
Removing code from the kernel is not something we can do unless there is demonstrated evidence that the number of impacted users is close to zero and distributions no longer support that code. The case of Baytrail legacy is telling, you removed it earlier this Fall but after a recommended alternative was provided for more than 3 years.
Again, there is no planned 'switch' but a gradual transition, and that patchset helps with the transition.
Hmm, then maybe I misunderstood Hans. Given his feedback it seemed like Fedora is about to switch to SOF right now.
Indeed, before I've sent patches removing Baytrail, basically every support-team had been asked about its usage and the answers were negative. /atom/ was covering basically every case anyway like you pointed out so /baytrail/ solution felt more like a duplication.
As SOF is the desired solution for atom-based products, I can see the need for some sort of selection mechanism. The same cannot be said for hsw/bdw though. Let's leave catpt out this, shall we?
Czarek
I guess Cezary mean the modprobe blacklist? This was used in the early stage of ASoC Skylake driver development, but in the end, it's more cumbersome because user needs to change multiple places. The single module parameter was easier to handle.
Thanks for joining the discussion, Takashi.
If the switch of solution for atom-based products is imminent, why add code which becomes redundant soon after?
To be clear: there is *no plan* to *remove* the Atom/sst code any time 'soon', only to *deprecate* it.
In the best case distributions would transition in 2021. Some distros are faster than others, neither you nor I have any control over this.
Removing code from the kernel is not something we can do unless there is demonstrated evidence that the number of impacted users is close to zero and distributions no longer support that code. The case of Baytrail legacy is telling, you removed it earlier this Fall but after a recommended alternative was provided for more than 3 years.
Again, there is no planned 'switch' but a gradual transition, and that patchset helps with the transition.
Hmm, then maybe I misunderstood Hans. Given his feedback it seemed like Fedora is about to switch to SOF right now.
Indeed, before I've sent patches removing Baytrail, basically every support-team had been asked about its usage and the answers were negative. /atom/ was covering basically every case anyway like you pointed out so /baytrail/ solution felt more like a duplication.
As SOF is the desired solution for atom-based products, I can see the need for some sort of selection mechanism. The same cannot be said for hsw/bdw though. Let's leave catpt out this, shall we?
It helps everyone to have a single build, e.g. 'make allmodconfig' or 'make allyesconfig' would select all possible drivers and bots can run wild.
On 2020-11-18 9:25 PM, Pierre-Louis Bossart wrote:
...
Hmm, then maybe I misunderstood Hans. Given his feedback it seemed like Fedora is about to switch to SOF right now.
Indeed, before I've sent patches removing Baytrail, basically every support-team had been asked about its usage and the answers were negative. /atom/ was covering basically every case anyway like you pointed out so /baytrail/ solution felt more like a duplication.
As SOF is the desired solution for atom-based products, I can see the need for some sort of selection mechanism. The same cannot be said for hsw/bdw though. Let's leave catpt out this, shall we?
It helps everyone to have a single build, e.g. 'make allmodconfig' or 'make allyesconfig' would select all possible drivers and bots can run wild.
Why should bots care about not recommended code? I'm against adding external dependency (intel-dsp-config) for catpt for reasons I'd mentioned several times already.
Czarek
On Fri, Nov 20, 2020 at 03:40:21PM +0000, Rojewski, Cezary wrote:
On 2020-11-18 9:25 PM, Pierre-Louis Bossart wrote:
It helps everyone to have a single build, e.g. 'make allmodconfig' or 'make allyesconfig' would select all possible drivers and bots can run wild.
Why should bots care about not recommended code? I'm against adding external dependency (intel-dsp-config) for catpt for reasons I'd mentioned several times already.
People care about any code that's in the kernel, especially people doing anything treewide. The fewer configurations people need to build to get code coverage the better.
On 2020-11-20 5:48 PM, Mark Brown wrote:
On Fri, Nov 20, 2020 at 03:40:21PM +0000, Rojewski, Cezary wrote:
On 2020-11-18 9:25 PM, Pierre-Louis Bossart wrote:
It helps everyone to have a single build, e.g. 'make allmodconfig' or 'make allyesconfig' would select all possible drivers and bots can run wild.
Why should bots care about not recommended code? I'm against adding external dependency (intel-dsp-config) for catpt for reasons I'd mentioned several times already.
People care about any code that's in the kernel, especially people doing anything treewide. The fewer configurations people need to build to get code coverage the better.
Sure, but in this particular case there really shouldn't be "another option". If catpt is the sole option, why add intel-dsp-config dependency? The alternative shouldn't even exist in the kernel and be instead removed just like /haswell/ and /baytrail/ were.
Czarek
On Fri, Nov 20, 2020 at 05:10:30PM +0000, Rojewski, Cezary wrote:
On 2020-11-20 5:48 PM, Mark Brown wrote:
People care about any code that's in the kernel, especially people doing anything treewide. The fewer configurations people need to build to get code coverage the better.
Sure, but in this particular case there really shouldn't be "another option". If catpt is the sole option, why add intel-dsp-config dependency? The alternative shouldn't even exist in the kernel and be instead removed just like /haswell/ and /baytrail/ were.
If all the alternatives actually get removed then there'd be no need for it, while they're there it is reasonable to have it - it does make it easier for people like distros to try converting, it means they can deploy the recommended setup without needing to ship new binaries to people who run into trouble. Besides TBH while there's several DSP implementations in the tree having the code there makes it obvious that this case works the same way as all the others to anyone looking at the code.
On 2020-11-20 7:06 PM, Mark Brown wrote:
On Fri, Nov 20, 2020 at 05:10:30PM +0000, Rojewski, Cezary wrote:
On 2020-11-20 5:48 PM, Mark Brown wrote:
People care about any code that's in the kernel, especially people doing anything treewide. The fewer configurations people need to build to get code coverage the better.
Sure, but in this particular case there really shouldn't be "another option". If catpt is the sole option, why add intel-dsp-config dependency? The alternative shouldn't even exist in the kernel and be instead removed just like /haswell/ and /baytrail/ were.
If all the alternatives actually get removed then there'd be no need for it, while they're there it is reasonable to have it - it does make it easier for people like distros to try converting, it means they can deploy the recommended setup without needing to ship new binaries to people who run into trouble. Besides TBH while there's several DSP implementations in the tree having the code there makes it obvious that this case works the same way as all the others to anyone looking at the code.
I can understand that in atom's case. That's why I'm fine with the section mechanism being applied there. At the beginning I'd thought SOF is already prepared to take over /atom/. As that's not the case, to ease the transition, dynamic switch could prove useful.
There are no circumstances under which Intel recommends distros to try to convert out of catpt though. Don't believe aligning all the drivers to some general idea just for the sake of aligning is a good move. That's why drivers have their own specifics in the first place - their complexity and performance could have been negatively impacted otherwise.
Czarek
On Fri, Nov 20, 2020 at 09:02:24PM +0000, Rojewski, Cezary wrote:
There are no circumstances under which Intel recommends distros to try to convert out of catpt though. Don't believe aligning all the drivers to some general idea just for the sake of aligning is a good move. That's why drivers have their own specifics in the first place - their complexity and performance could have been negatively impacted otherwise.
It could equally be that someone has stuck with the older, now deprecated, implementations due to compatibility fears and this could help them deploy the catpt implementation without worrying so much about breaking things for users.
On 2020-11-23 6:35 PM, Mark Brown wrote:
On Fri, Nov 20, 2020 at 09:02:24PM +0000, Rojewski, Cezary wrote:
There are no circumstances under which Intel recommends distros to try to convert out of catpt though. Don't believe aligning all the drivers to some general idea just for the sake of aligning is a good move. That's why drivers have their own specifics in the first place - their complexity and performance could have been negatively impacted otherwise.
It could equally be that someone has stuck with the older, now deprecated, implementations due to compatibility fears and this could help them deploy the catpt implementation without worrying so much about breaking things for users.
Except that it (i.e.: patchset) doesn't touch old _HASWELL kconfig at all as the code behind it is already removed.
Believe we are desync'ed here.
What the patchset presents catpt vs SOF. /sof/ runs through SOF firmware so it cannot be account as old-implementation. It's a mix of not recommended fw + incorrect sw flow. As old /haswell/ is no more, there is no worrying about catpt deployment - it's your only option. As there is no userspace involved (lack of topology files), base firmware binary remains the same and amixer kcontrols behave 1:1 when compared to its predecessor, compatibility is left intact.
That's exactly why we should be explicit in driver selection. Pretty sure hsw/bdw case is still mistakenly addressed to as if it was atom-based platform.
Czarek
On Tue, Nov 24, 2020 at 11:56:36AM +0000, Rojewski, Cezary wrote:
What the patchset presents catpt vs SOF. /sof/ runs through SOF firmware so it cannot be account as old-implementation. It's a mix of not recommended fw + incorrect sw flow. As old /haswell/ is no more, there is no worrying about catpt deployment - it's your only option. As there is no userspace involved (lack of topology files), base firmware binary remains the same and amixer kcontrols behave 1:1 when compared to its predecessor, compatibility is left intact.
That's exactly why we should be explicit in driver selection. Pretty sure hsw/bdw case is still mistakenly addressed to as if it was atom-based platform.
It's not just the userspace interface that worries people, it's also any board specific quirks that might turn up. A good chunk of the work with x86 sound support is quirking around platform specifics - look at all the patches Hans sends for example. In an ideal world this would just be people worrying too much but the general history with getting generic code working well on a wide range of x86 hardware it's hard to blame anyone for being conservative about substantial changes in the software stack.
On Tue, 24 Nov 2020 15:01:19 +0100, Mark Brown wrote:
On Tue, Nov 24, 2020 at 11:56:36AM +0000, Rojewski, Cezary wrote:
What the patchset presents catpt vs SOF. /sof/ runs through SOF firmware so it cannot be account as old-implementation. It's a mix of not recommended fw + incorrect sw flow. As old /haswell/ is no more, there is no worrying about catpt deployment - it's your only option. As there is no userspace involved (lack of topology files), base firmware binary remains the same and amixer kcontrols behave 1:1 when compared to its predecessor, compatibility is left intact.
That's exactly why we should be explicit in driver selection. Pretty sure hsw/bdw case is still mistakenly addressed to as if it was atom-based platform.
It's not just the userspace interface that worries people, it's also any board specific quirks that might turn up. A good chunk of the work with x86 sound support is quirking around platform specifics - look at all the patches Hans sends for example. In an ideal world this would just be people worrying too much but the general history with getting generic code working well on a wide range of x86 hardware it's hard to blame anyone for being conservative about substantial changes in the software stack.
I guess Cezary's point is that CATPT is the only driver for Haswell, hence the intel-dsp-config is useless for it.
But I thought CATPT also covers Broadwell, and Broadwell can be supported by both CATPT and SOF? If so, the dynamic switching makes sense.
Takashi
On 2020-11-24 3:15 PM, Takashi Iwai wrote:
On Tue, 24 Nov 2020 15:01:19 +0100, Mark Brown wrote:
On Tue, Nov 24, 2020 at 11:56:36AM +0000, Rojewski, Cezary wrote:
What the patchset presents catpt vs SOF. /sof/ runs through SOF firmware so it cannot be account as old-implementation. It's a mix of not recommended fw + incorrect sw flow. As old /haswell/ is no more, there is no worrying about catpt deployment - it's your only option. As there is no userspace involved (lack of topology files), base firmware binary remains the same and amixer kcontrols behave 1:1 when compared to its predecessor, compatibility is left intact.
That's exactly why we should be explicit in driver selection. Pretty sure hsw/bdw case is still mistakenly addressed to as if it was atom-based platform.
It's not just the userspace interface that worries people, it's also any board specific quirks that might turn up. A good chunk of the work with x86 sound support is quirking around platform specifics - look at all the patches Hans sends for example. In an ideal world this would just be people worrying too much but the general history with getting generic code working well on a wide range of x86 hardware it's hard to blame anyone for being conservative about substantial changes in the software stack.
Mark, there is not a single word I don't agree with in your statement.
In regard to quirks - I was surprised how much detail Hans found out regarding atom platforms. That's a lot of good input. And that's probably one of the key reasons why atom is properly supported in linux.
My point has more "basic" nature.
I guess Cezary's point is that CATPT is the only driver for Haswell, hence the intel-dsp-config is useless for it.
This! and..
But I thought CATPT also covers Broadwell, and Broadwell can be supported by both CATPT and SOF? If so, the dynamic switching makes sense.
..more. Dynamic selection made sense if you're in transition period as it is the case for atoms. There is no transition period for hsw/bdw. BDW as "supported" by SOF would be a strong claim. There is no commitment and Intel does not recommend using it for hsw/bdw for any scenario. And as such, selection-subject does not apply here.
Believe removal of /sof/intel/bdw.c is in order?
Czarek
On Tue, 24 Nov 2020 17:07:19 +0100, Rojewski, Cezary wrote:
On 2020-11-24 3:15 PM, Takashi Iwai wrote:
On Tue, 24 Nov 2020 15:01:19 +0100, Mark Brown wrote:
On Tue, Nov 24, 2020 at 11:56:36AM +0000, Rojewski, Cezary wrote:
What the patchset presents catpt vs SOF. /sof/ runs through SOF firmware so it cannot be account as old-implementation. It's a mix of not recommended fw + incorrect sw flow. As old /haswell/ is no more, there is no worrying about catpt deployment - it's your only option. As there is no userspace involved (lack of topology files), base firmware binary remains the same and amixer kcontrols behave 1:1 when compared to its predecessor, compatibility is left intact.
That's exactly why we should be explicit in driver selection. Pretty sure hsw/bdw case is still mistakenly addressed to as if it was atom-based platform.
It's not just the userspace interface that worries people, it's also any board specific quirks that might turn up. A good chunk of the work with x86 sound support is quirking around platform specifics - look at all the patches Hans sends for example. In an ideal world this would just be people worrying too much but the general history with getting generic code working well on a wide range of x86 hardware it's hard to blame anyone for being conservative about substantial changes in the software stack.
Mark, there is not a single word I don't agree with in your statement.
In regard to quirks - I was surprised how much detail Hans found out regarding atom platforms. That's a lot of good input. And that's probably one of the key reasons why atom is properly supported in linux.
My point has more "basic" nature.
I guess Cezary's point is that CATPT is the only driver for Haswell, hence the intel-dsp-config is useless for it.
This! and..
But I thought CATPT also covers Broadwell, and Broadwell can be supported by both CATPT and SOF? If so, the dynamic switching makes sense.
..more. Dynamic selection made sense if you're in transition period as it is the case for atoms. There is no transition period for hsw/bdw. BDW as "supported" by SOF would be a strong claim. There is no commitment and Intel does not recommend using it for hsw/bdw for any scenario. And as such, selection-subject does not apply here.
Believe removal of /sof/intel/bdw.c is in order?
This requires the agreement rather in Intel side at first.
The upstream will follow that decision, and eventually drop the dynamic selection for HSW/BDW, too.
Takashi
On Tue, Nov 24, 2020 at 03:15:18PM +0100, Takashi Iwai wrote:
I guess Cezary's point is that CATPT is the only driver for Haswell, hence the intel-dsp-config is useless for it.
But I thought CATPT also covers Broadwell, and Broadwell can be supported by both CATPT and SOF? If so, the dynamic switching makes sense.
Right, like I said previously if there's only one option upstream then we can drop this stuff entirely - like you say I understood that there were some SoCs that could use both.
On Tue, 17 Nov 2020 23:13:13 +0100, Rojewski, Cezary wrote:
On 2020-11-17 3:04 PM, Takashi Iwai wrote:
On Mon, 16 Nov 2020 18:47:22 +0100, Pierre-Louis Bossart wrote:
Explicit 'ifs' asking whether we're dealing with SOF or other solution is at best a code smell. I believe this is unnecessary complexity added to the code especially once you realize user needs to play with module parameters to switch between solutions. If we assume user is able to play with module parameters then why not simply make use of blacklist mechanism?
Been there, done that. We don't want to use either denylist of kernel parameters.
denylists create confusion for users, it's an endless stream of false errors and time lost in bug reports.
The use of the kernel parameter is ONLY for expert users who want to tinker with the system or debug issues, the average user should not have to play with either denylists or kernel parameters.
I guess Cezary mean the modprobe blacklist? This was used in the early stage of ASoC Skylake driver development, but in the end, it's more cumbersome because user needs to change multiple places. The single module parameter was easier to handle.
Thanks for joining the discussion, Takashi.
If the switch of solution for atom-based products is imminent, why add code which becomes redundant soon after?
Yes, indeed I meant the modprobe blacklisting as it solves the problem without addition of any code. Doubt alsa-driver entries are scattered in /etc/modprobe.d/ so switching between one solution to another via blacklist becomes as easy as changing 'options intel-dsp-config
<param>==<value>' entry.
Ideally blacklist would work well, but practically it can be more problematic. When you *switch* between multiple drivers via blacklist, you'll have to mask one of them while keeping another untouched, so either: blacklist A or blacklist B
Now, imagine that distro sets "blacklist A" to choose B as the default. What user has to do? They have to modify "blacklist A" line with "blacklist B". But it can't be done with an additional modprobe.d/*.config file; otherwise this blacklist remains. It means they have to scratch the system configuration file itself -- which might be again overridden by a package update or whatever.
This will be more complex if there are more than three choices, of course.
Admittedly, the situation with the system config file be same for module option if distro sets the option in modprobe.d/*, too. But, there is another difference: the default option value can be set in the kernel code, while the blacklist approach is to let all open and choose via blacklist. IOW, devs have some control for choosing the default value for the module option but for blacklist they are all done by user-space side.
In regard to catpt, solution is even simpler: just remove sound/soc/sof/intel/bdw.c as that code is not valid & recommended anyway and linux kernel is not place for such. There shouldn't be really any options for not recommended stuff. Leave the selection explicit.
And last but not least: intel-dsp-config is (as already stated) a mean for solving the HDA-runtime-driver-selection problem. Mixing it with ACPI devices is another layer of confusion.
Why? Who said it was PCI only? We already take care of DMIC, SoundWire, Google Chromebooks, open platforms, why not ACPI? It's just one API to be used when more than one driver can register to the same device.
Well, currently intel-dsp-config sits in sound/hda, which isn't really intuitive. Though, Intel driver file paths are already fairly scattered, so it doesn't matter too much :)
I don't mind to move it to another directory, but which one...? x86 might match, but shuffling the place won't help for maintenance.
I personally find this move good, at least it makes things easier for distros. There are small details like the above, but technically seen, I see no big problem.
Well, if non-Intel guys see the localization of code counter-intuitive then how about those who play with it daily..
I play it and maintain it daily, that's why I find unintuitive :) I guess most users don't notice the file path, as the module loading or option is done only by the module name.
The new "sof-parent" checks won't make maintaining any easier and I believe there are easier solutions as written above.
If you find a good way to overcome the disadvantage, that's great. Let's see.
thanks,
Takashi
On 2020-11-18 8:49 AM, Takashi Iwai wrote:
On Tue, 17 Nov 2020 23:13:13 +0100, Rojewski, Cezary wrote:
...
Thanks for joining the discussion, Takashi.
If the switch of solution for atom-based products is imminent, why add code which becomes redundant soon after?
Yes, indeed I meant the modprobe blacklisting as it solves the problem without addition of any code. Doubt alsa-driver entries are scattered in /etc/modprobe.d/ so switching between one solution to another via blacklist becomes as easy as changing 'options intel-dsp-config
<param>==<value>' entry.
Ideally blacklist would work well, but practically it can be more problematic. When you *switch* between multiple drivers via blacklist, you'll have to mask one of them while keeping another untouched, so either: blacklist A or blacklist B
Now, imagine that distro sets "blacklist A" to choose B as the default. What user has to do? They have to modify "blacklist A" line with "blacklist B". But it can't be done with an additional modprobe.d/*.config file; otherwise this blacklist remains. It means they have to scratch the system configuration file itself -- which might be again overridden by a package update or whatever.
This will be more complex if there are more than three choices, of course.
Admittedly, the situation with the system config file be same for module option if distro sets the option in modprobe.d/*, too. But, there is another difference: the default option value can be set in the kernel code, while the blacklist approach is to let all open and choose via blacklist. IOW, devs have some control for choosing the default value for the module option but for blacklist they are all done by user-space side.
I agree, module param is ultimately easier to handle than denylist. The reason I had mentioned that is: if user is capable of changing value for module param, then we might as well assume he or she is the experienced one and playing with denylist isn't a problem either.
And hopefully we don't reach a point in time where again 3 flavours for atom-based products are available : )
In regard to catpt, solution is even simpler: just remove sound/soc/sof/intel/bdw.c as that code is not valid & recommended anyway and linux kernel is not place for such. There shouldn't be really any options for not recommended stuff. Leave the selection explicit.
...
Well, if non-Intel guys see the localization of code counter-intuitive then how about those who play with it daily..
I play it and maintain it daily, that's why I find unintuitive :) I guess most users don't notice the file path, as the module loading or option is done only by the module name.
Perhaps I misworded my previous statement. What I meant is: you don't have access to all the stuff we - Intel employees - have like specs, firmware documentation, hardware specifics and yet you see the problem. And this tells me there's still a lot to be done.
The new "sof-parent" checks won't make maintaining any easier and I believe there are easier solutions as written above.
If you find a good way to overcome the disadvantage, that's great. Let's see.
Well, the disadvantage is: weight of maintenance of newly added code. All in all, as mentioned in other reply, we could settle with selection mechanism for atom while leaving hsw/bdw out of it.
Czarek
On Thu, 12 Nov 2020 16:38:11 -0600, Pierre-Louis Bossart wrote:
The module snd-intel-dspcfg, suggested by Jaroslav last year, currently provide the means to select a PCI driver at run-time, based on quirks, recommendations or user selection via a kernel parameter. This capability removed a lot of confusions in distributions and removed the need for recompilations to select legacy HDaudio, SST or SOF drivers.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[01/14] ASoC: Intel: broadwell: add missing pm_ops commit: 7998c168a94de9c593ab07455924e827ad5f1bd7 [02/14] ASoC: Intel: bdw-rt5677: add missing pm_ops commit: cf7f4a5320cda6fc533ae96601b4ce767d1af0f8 [03/14] ALSA: hda: intel-dsp-config: add helper for ACPI DSP driver selection commit: b5682305297db24b456e55ba209574cb8f9318f9 [04/14] ASoC: soc-acpi: add helper to identify parent driver. commit: 644eebdbbf1154c995d6319c133d7d5b898c5ed2 [05/14] ASoC: Intel: boards: byt/cht: set card and driver name at run time commit: 41656c3dc2acfe2aef3d7c4e1cd2b92f49b6e3a7 [06/14] ASoC: Intel: byt/cht: set pm ops dynamically commit: 05ff312badb6079f18c0b05d89e21733a9dafe32 [07/14] ASoC: SOF: acpi: add dynamic selection of DSP driver commit: f7313f9fc28781ad0801d8b9c692222445e664ca [08/14] ASoC: Intel: Atom: add dynamic selection of DSP driver commit: df5f5edaef4b653fa731dcf3753e71766f95c2cd [09/14] ASoC: SOF: Intel: allow for coexistence between SOF and Atom/SST drivers commit: b405b4318c77db061fdf1c8c4b9329ea30e807ee [10/14] ALSA: hda: intel-dsp-config: add Broadwell ACPI DSP driver selection commit: 803e591337e6f7953350e0f56284ebbabb600808 [11/14] ASoC: Intel: broadwell: set card and driver name dynamically commit: 8643e85aab878fe0d8031ae4622b40cfb78d4172 [12/14] ASoC: Intel: catpt: add dynamic selection of DSP driver commit: ec8a15d3a7c7d6e9acd2e0637d2020ac17fb7820 [13/14] ASoC: SOF: Intel: allow for coexistence between SOF and catpt drivers commit: d512ef22d77b0779e9b0e9a91a63b291357079f9 [14/14] ALSA: hda: intel-dsp-config: ignore dsp_driver parameter for PCI legacy devices commit: 0e5cc22162e55c19255f4e25dadf9fda76eac11c
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (6)
-
Hans de Goede -
Mark Brown -
Pierre-Louis Bossart -
Rojewski, Cezary -
Takashi Iwai -
youling257