diff mbox series

[v3,04/10] ASoC: SOF: Intel: add support for snd-hda-codec-hdmi

Message ID 20190910182916.29693-5-kai.vehmanen@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series adapt SOF to use snd-hda-codec-hdmi | expand

Commit Message

Kai Vehmanen Sept. 10, 2019, 6:29 p.m. UTC
Add support to implement HDMI/DP audio by using the common
snd-hda-codec-hdmi driver.

Change of codec driver affects user-space as the the two
drivers expose different mixer controls. A new kernel
module option "use_common_hdmi" is added to user-space
to indicate which interface should be used. The default
driver can be selected via a Kconfig option.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 sound/soc/sof/intel/Kconfig     | 10 ++++++++++
 sound/soc/sof/intel/hda-codec.c | 19 +++++++++++++++----
 sound/soc/sof/intel/hda.c       |  6 ++++++
 sound/soc/sof/intel/hda.h       |  6 ++++--
 4 files changed, 35 insertions(+), 6 deletions(-)

Comments

Pierre-Louis Bossart Sept. 10, 2019, 8:52 p.m. UTC | #1
On 9/10/19 1:29 PM, Kai Vehmanen wrote:
> Add support to implement HDMI/DP audio by using the common
> snd-hda-codec-hdmi driver.
> 
> Change of codec driver affects user-space as the the two
> drivers expose different mixer controls. A new kernel
> module option "use_common_hdmi" is added to user-space
> to indicate which interface should be used. The default
> driver can be selected via a Kconfig option.
> 
> Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> ---
>   sound/soc/sof/intel/Kconfig     | 10 ++++++++++
>   sound/soc/sof/intel/hda-codec.c | 19 +++++++++++++++----
>   sound/soc/sof/intel/hda.c       |  6 ++++++
>   sound/soc/sof/intel/hda.h       |  6 ++++--
>   4 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
> index 479ba249e219..a8f4e69b044d 100644
> --- a/sound/soc/sof/intel/Kconfig
> +++ b/sound/soc/sof/intel/Kconfig
> @@ -273,6 +273,16 @@ config SND_SOC_SOF_HDA_AUDIO_CODEC
>   	  Say Y if you want to enable HDAudio codecs with SOF.
>   	  If unsure select "N".
>   
> +config SND_SOC_SOF_HDA_COMMON_HDMI_CODEC
> +	bool "SOF common HDA HDMI codec driver"
> +	depends on SND_SOC_SOF_HDA_LINK
> +	depends on SND_HDA_CODEC_HDMI
> +	help
> +	  This adds support for HDMI audio by using the common HDA
> +	  HDMI/DisplayPort codec driver.
> +	  Say Y if you want to use the common codec driver with SOF.
> +	  If unsure select "Y".
> +
>   endif ## SND_SOC_SOF_HDA_COMMON
>   
>   config SND_SOC_SOF_HDA_LINK_BASELINE
> diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
> index 3ca6795a89ba..817ebba00b47 100644
> --- a/sound/soc/sof/intel/hda-codec.c
> +++ b/sound/soc/sof/intel/hda-codec.c
> @@ -84,6 +84,8 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address)
>   {
>   #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
>   	struct hdac_hda_priv *hda_priv;
> +	struct snd_soc_acpi_mach_params *mach_params = 0;
> +	struct snd_sof_pdata *pdata = sdev->pdata;
>   #endif
>   	struct hda_bus *hbus = sof_to_hbus(sdev);
>   	struct hdac_device *hdev;
> @@ -113,8 +115,16 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address)
>   	if (ret < 0)
>   		return ret;
>   
> -	/* use legacy bus only for HDA codecs, idisp uses ext bus */
> -	if ((resp & 0xFFFF0000) != IDISP_VID_INTEL) {
> +	if (pdata->machine)
> +		mach_params = (struct snd_soc_acpi_mach_params *)
> +			&pdata->machine->mach_params;
> +
> +	/*
> +	 * if common HDMI codec driver is not used, codec load
> +	 * is skipped here and hdac_hdmi is used instead
> +	 */
> +	if ((mach_params && mach_params->common_hdmi_codec_drv) ||
> +	    (resp & 0xFFFF0000) != IDISP_VID_INTEL) {
>   		hdev->type = HDA_DEV_LEGACY;
>   		hda_codec_load_module(&hda_priv->codec);

This part is might be problematic. For SoundWire stuff, I had to move 
all the machine detection part out of hda_init_caps() and at the end of 
hda_dsp_probe. It's not final since I am still trying to figure out what 
the earliest time I can power-up the SoundWire IP is, but it would help 
if you don't make strong assumptions on when mach_params is set. The 
fact that all this code is currently in hda_init_caps() is not really by 
design, more because of incremental code changes.


>   	}
> @@ -155,7 +165,8 @@ int hda_codec_probe_bus(struct snd_sof_dev *sdev)
>   }
>   EXPORT_SYMBOL(hda_codec_probe_bus);
>   
> -#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)
> +#if IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI) || \
> +	IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)
>   
>   void hda_codec_i915_get(struct snd_sof_dev *sdev)
>   {
> @@ -204,6 +215,6 @@ int hda_codec_i915_exit(struct snd_sof_dev *sdev)
>   }
>   EXPORT_SYMBOL(hda_codec_i915_exit);
>   
> -#endif /* CONFIG_SND_SOC_HDAC_HDMI */
> +#endif
>   
>   MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
> index c72e9a09eee1..ee742157516e 100644
> --- a/sound/soc/sof/intel/hda.c
> +++ b/sound/soc/sof/intel/hda.c
> @@ -54,6 +54,11 @@ MODULE_PARM_DESC(use_msi, "SOF HDA use PCI MSI mode");
>   static int hda_dmic_num = -1;
>   module_param_named(dmic_num, hda_dmic_num, int, 0444);
>   MODULE_PARM_DESC(dmic_num, "SOF HDA DMIC number");
> +
> +static bool hda_codec_use_common_hdmi =
> +	IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_COMMON_HDMI_CODEC);
> +module_param_named(use_common_hdmi, hda_codec_use_common_hdmi, bool, 0444);
> +MODULE_PARM_DESC(use_common_hdmi, "SOF HDA use common HDMI codec driver");
>   #endif
>   
>   static const struct hda_dsp_msg_code hda_dsp_rom_msg[] = {
> @@ -458,6 +463,7 @@ static int hda_init_caps(struct snd_sof_dev *sdev)
>   			&pdata->machine->mach_params;
>   		mach_params->codec_mask = bus->codec_mask;
>   		mach_params->platform = dev_name(sdev->dev);
> +		mach_params->common_hdmi_codec_drv = hda_codec_use_common_hdmi;
>   	}
>   
>   	/* create codec instances */
> diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
> index 5591841a1b6f..28640a29e1b6 100644
> --- a/sound/soc/sof/intel/hda.h
> +++ b/sound/soc/sof/intel/hda.h
> @@ -562,7 +562,9 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev);
>   
>   #endif /* CONFIG_SND_SOC_SOF_HDA */
>   
> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) && IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) && \
> +	(IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI) || \
> +	 IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
>   
>   void hda_codec_i915_get(struct snd_sof_dev *sdev);
>   void hda_codec_i915_put(struct snd_sof_dev *sdev);
> @@ -576,7 +578,7 @@ static inline void hda_codec_i915_put(struct snd_sof_dev *sdev)  { }
>   static inline int hda_codec_i915_init(struct snd_sof_dev *sdev) { return 0; }
>   static inline int hda_codec_i915_exit(struct snd_sof_dev *sdev) { return 0; }
>   
> -#endif /* CONFIG_SND_SOC_SOF_HDA && CONFIG_SND_SOC_HDAC_HDMI */
> +#endif
>   
>   /*
>    * Trace Control.
>
Kai Vehmanen Sept. 12, 2019, 12:06 p.m. UTC | #2
Hey,

On Tue, 10 Sep 2019, Pierre-Louis Bossart wrote:
> > --- a/sound/soc/sof/intel/hda-codec.c
> > +++ b/sound/soc/sof/intel/hda-codec.c
> > @@ -84,6 +84,8 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int
[...]
> > +	/*
> > +	 * if common HDMI codec driver is not used, codec load
> > +	 * is skipped here and hdac_hdmi is used instead
> > +	 */
> > +	if ((mach_params && mach_params->common_hdmi_codec_drv) ||
> > +	    (resp & 0xFFFF0000) != IDISP_VID_INTEL) {
> >   		hdev->type = HDA_DEV_LEGACY;
> >   		hda_codec_load_module(&hda_priv->codec);
> 
> This part is might be problematic. For SoundWire stuff, I had to move all the
> machine detection part out of hda_init_caps() and at the end of hda_dsp_probe.
> It's not final since I am still trying to figure out what the earliest time I
> can power-up the SoundWire IP is, but it would help if you don't make strong
> assumptions on when mach_params is set. The fact that all this code is
> currently in hda_init_caps() is not really by design, more because of
> incremental code changes.

hmm. Currently the settings part of 'common_hdmi_codec_drv' is in in 
init_caps, just before call to hda_codec_probe_bus() (which uses the 
flag). There are also other fields set there (like 
mach_params->codec_mask), so issues should apply to codec_mask passing as 
well. I.e. if functionality is moved out from init_caps, the call to bus 
probe should be moved as well.

Other options to pass the flag do not seem good. We don't want to add hw 
specific stuff like this to snd_sof_dev or hdac_bus (latter used to pass 
"codec_mask"). If this becomes an issue, maybe we just need an explicit 
parameter to hda_codec_probe_bus(), and/or move the module parameter 
directly to intel/hda-codec.c. Both are a bit ugly, but at least contained 
in the "sof/intel/" subfolder.

Br, Kai
Pierre-Louis Bossart Sept. 12, 2019, 1:29 p.m. UTC | #3
On 9/12/19 7:06 AM, Kai Vehmanen wrote:
> Hey,
> 
> On Tue, 10 Sep 2019, Pierre-Louis Bossart wrote:
>>> --- a/sound/soc/sof/intel/hda-codec.c
>>> +++ b/sound/soc/sof/intel/hda-codec.c
>>> @@ -84,6 +84,8 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int
> [...]
>>> +	/*
>>> +	 * if common HDMI codec driver is not used, codec load
>>> +	 * is skipped here and hdac_hdmi is used instead
>>> +	 */
>>> +	if ((mach_params && mach_params->common_hdmi_codec_drv) ||
>>> +	    (resp & 0xFFFF0000) != IDISP_VID_INTEL) {
>>>    		hdev->type = HDA_DEV_LEGACY;
>>>    		hda_codec_load_module(&hda_priv->codec);
>>
>> This part is might be problematic. For SoundWire stuff, I had to move all the
>> machine detection part out of hda_init_caps() and at the end of hda_dsp_probe.
>> It's not final since I am still trying to figure out what the earliest time I
>> can power-up the SoundWire IP is, but it would help if you don't make strong
>> assumptions on when mach_params is set. The fact that all this code is
>> currently in hda_init_caps() is not really by design, more because of
>> incremental code changes.
> 
> hmm. Currently the settings part of 'common_hdmi_codec_drv' is in in
> init_caps, just before call to hda_codec_probe_bus() (which uses the
> flag). There are also other fields set there (like
> mach_params->codec_mask), so issues should apply to codec_mask passing as
> well. I.e. if functionality is moved out from init_caps, the call to bus
> probe should be moved as well.
> 
> Other options to pass the flag do not seem good. We don't want to add hw
> specific stuff like this to snd_sof_dev or hdac_bus (latter used to pass
> "codec_mask"). If this becomes an issue, maybe we just need an explicit
> parameter to hda_codec_probe_bus(), and/or move the module parameter
> directly to intel/hda-codec.c. Both are a bit ugly, but at least contained
> in the "sof/intel/" subfolder.

never mind, with the SoundWire plumbing rework we can do all the machine 
driver checks in hda_init_caps. I split the ACPI scan, driver probe and 
startup in 3 steps so we have more freedom. Before this rework I was 
constrained to detect the ACPI stuff only when the hardware was powered, 
which was after init_caps.
In short don't worry about me/SoundWire.
diff mbox series

Patch

diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
index 479ba249e219..a8f4e69b044d 100644
--- a/sound/soc/sof/intel/Kconfig
+++ b/sound/soc/sof/intel/Kconfig
@@ -273,6 +273,16 @@  config SND_SOC_SOF_HDA_AUDIO_CODEC
 	  Say Y if you want to enable HDAudio codecs with SOF.
 	  If unsure select "N".
 
+config SND_SOC_SOF_HDA_COMMON_HDMI_CODEC
+	bool "SOF common HDA HDMI codec driver"
+	depends on SND_SOC_SOF_HDA_LINK
+	depends on SND_HDA_CODEC_HDMI
+	help
+	  This adds support for HDMI audio by using the common HDA
+	  HDMI/DisplayPort codec driver.
+	  Say Y if you want to use the common codec driver with SOF.
+	  If unsure select "Y".
+
 endif ## SND_SOC_SOF_HDA_COMMON
 
 config SND_SOC_SOF_HDA_LINK_BASELINE
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index 3ca6795a89ba..817ebba00b47 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -84,6 +84,8 @@  static int hda_codec_probe(struct snd_sof_dev *sdev, int address)
 {
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
 	struct hdac_hda_priv *hda_priv;
+	struct snd_soc_acpi_mach_params *mach_params = 0;
+	struct snd_sof_pdata *pdata = sdev->pdata;
 #endif
 	struct hda_bus *hbus = sof_to_hbus(sdev);
 	struct hdac_device *hdev;
@@ -113,8 +115,16 @@  static int hda_codec_probe(struct snd_sof_dev *sdev, int address)
 	if (ret < 0)
 		return ret;
 
-	/* use legacy bus only for HDA codecs, idisp uses ext bus */
-	if ((resp & 0xFFFF0000) != IDISP_VID_INTEL) {
+	if (pdata->machine)
+		mach_params = (struct snd_soc_acpi_mach_params *)
+			&pdata->machine->mach_params;
+
+	/*
+	 * if common HDMI codec driver is not used, codec load
+	 * is skipped here and hdac_hdmi is used instead
+	 */
+	if ((mach_params && mach_params->common_hdmi_codec_drv) ||
+	    (resp & 0xFFFF0000) != IDISP_VID_INTEL) {
 		hdev->type = HDA_DEV_LEGACY;
 		hda_codec_load_module(&hda_priv->codec);
 	}
@@ -155,7 +165,8 @@  int hda_codec_probe_bus(struct snd_sof_dev *sdev)
 }
 EXPORT_SYMBOL(hda_codec_probe_bus);
 
-#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)
+#if IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI) || \
+	IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)
 
 void hda_codec_i915_get(struct snd_sof_dev *sdev)
 {
@@ -204,6 +215,6 @@  int hda_codec_i915_exit(struct snd_sof_dev *sdev)
 }
 EXPORT_SYMBOL(hda_codec_i915_exit);
 
-#endif /* CONFIG_SND_SOC_HDAC_HDMI */
+#endif
 
 MODULE_LICENSE("Dual BSD/GPL");
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index c72e9a09eee1..ee742157516e 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -54,6 +54,11 @@  MODULE_PARM_DESC(use_msi, "SOF HDA use PCI MSI mode");
 static int hda_dmic_num = -1;
 module_param_named(dmic_num, hda_dmic_num, int, 0444);
 MODULE_PARM_DESC(dmic_num, "SOF HDA DMIC number");
+
+static bool hda_codec_use_common_hdmi =
+	IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_COMMON_HDMI_CODEC);
+module_param_named(use_common_hdmi, hda_codec_use_common_hdmi, bool, 0444);
+MODULE_PARM_DESC(use_common_hdmi, "SOF HDA use common HDMI codec driver");
 #endif
 
 static const struct hda_dsp_msg_code hda_dsp_rom_msg[] = {
@@ -458,6 +463,7 @@  static int hda_init_caps(struct snd_sof_dev *sdev)
 			&pdata->machine->mach_params;
 		mach_params->codec_mask = bus->codec_mask;
 		mach_params->platform = dev_name(sdev->dev);
+		mach_params->common_hdmi_codec_drv = hda_codec_use_common_hdmi;
 	}
 
 	/* create codec instances */
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 5591841a1b6f..28640a29e1b6 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -562,7 +562,9 @@  void hda_codec_jack_check(struct snd_sof_dev *sdev);
 
 #endif /* CONFIG_SND_SOC_SOF_HDA */
 
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) && IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) && \
+	(IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI) || \
+	 IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
 
 void hda_codec_i915_get(struct snd_sof_dev *sdev);
 void hda_codec_i915_put(struct snd_sof_dev *sdev);
@@ -576,7 +578,7 @@  static inline void hda_codec_i915_put(struct snd_sof_dev *sdev)  { }
 static inline int hda_codec_i915_init(struct snd_sof_dev *sdev) { return 0; }
 static inline int hda_codec_i915_exit(struct snd_sof_dev *sdev) { return 0; }
 
-#endif /* CONFIG_SND_SOC_SOF_HDA && CONFIG_SND_SOC_HDAC_HDMI */
+#endif
 
 /*
  * Trace Control.