[PATCH 0/2] ALSA/ASoC: hdmi/hdac_hda: Conditionally register dais
Hi,
Each instance of the hdac_hda registers DAIs to be used for analog and HDMI audio. When the system have more than one HDA codec - like most devices then the kernel log will include the following warning prints:
snd_hda_codec_realtek ehdaudio0D0: ASoC: sink widget AIF1TX overwritten snd_hda_codec_realtek ehdaudio0D0: ASoC: source widget AIF1RX overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget hifi3 overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget hifi2 overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget hifi1 overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: source widget Codec Output Pin1 overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget Codec Input Pin1 overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget Analog Codec Playback overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget Digital Codec Playback overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget Alt Analog Codec Playback overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: source widget Analog Codec Capture overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: source widget Digital Codec Capture overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: source widget Alt Analog Codec Capture overwritten
These are printed when the second instance of the hdac_hda is probing since it is re-registering the DAIs with the same name. Using some clever way of prefixing might solve the issue but that would need non backwards compatible changes to topology files.
Pragmatically the hdac_hda instance fro the normal codec should not register DAIs for HDMI and the hdac_hda instance for the HDMI should not register DAIs for the analog codec - for example on a SDW device where we have HDA-HDMI and sdw codecs.
To keep the hdac_hda vendor neutral (altrough it is only used on Intel platforms afaik) add a helper function to answer the question: is this codec HDMI? Use the new helper to decide which sets of DAIs to register for the probing hdac_hda instance.
Regards, Peter --- Peter Ujfalusi (2): ALSA: hda/hdmi: Add helper function to check if a device is HDMI codec ASoC: hdac_hda: Conditionally register dais for HDMI and Analog
include/sound/hdaudio.h | 10 ++++++++++ sound/pci/hda/patch_hdmi.c | 13 +++++++++++++ sound/soc/codecs/hdac_hda.c | 22 +++++++++++++++++++--- 3 files changed, 42 insertions(+), 3 deletions(-)
The snd_hda_device_is_hdmi() can be used in drivers to check if the hdev belongs to a display audio device.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- include/sound/hdaudio.h | 10 ++++++++++ sound/pci/hda/patch_hdmi.c | 13 +++++++++++++ 2 files changed, 23 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index dd7c87bbc613..cf5483d6b5b7 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -158,6 +158,16 @@ bool snd_hdac_check_power_state(struct hdac_device *hdac, hda_nid_t nid, unsigned int target_state); unsigned int snd_hdac_sync_power_state(struct hdac_device *hdac, hda_nid_t nid, unsigned int target_state); + +#if IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI) +bool snd_hda_device_is_hdmi(struct hdac_device *hdev); +#else +static inline bool snd_hda_device_is_hdmi(struct hdac_device *hdev) +{ + return false; +} +#endif + /** * snd_hdac_read_parm - read a codec parameter * @codec: the codec object diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 1cde2a69bdb4..d6943575c8c7 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -4645,6 +4645,19 @@ HDA_CODEC_ENTRY(HDA_CODEC_ID_GENERIC_HDMI, "Generic HDMI", patch_generic_hdmi), }; MODULE_DEVICE_TABLE(hdaudio, snd_hda_id_hdmi);
+bool snd_hda_device_is_hdmi(struct hdac_device *hdev) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(snd_hda_id_hdmi); i++) { + if (snd_hda_id_hdmi[i].vendor_id == hdev->vendor_id) + return true; + } + + return false; +} +EXPORT_SYMBOL_GPL(snd_hda_device_is_hdmi); + MODULE_LICENSE("GPL"); MODULE_DESCRIPTION("HDMI HD-audio codec"); MODULE_ALIAS("snd-hda-codec-intelhdmi");
On Mon, 27 Nov 2023 14:02:44 +0100, Peter Ujfalusi wrote:
The snd_hda_device_is_hdmi() can be used in drivers to check if the hdev belongs to a display audio device.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com
include/sound/hdaudio.h | 10 ++++++++++ sound/pci/hda/patch_hdmi.c | 13 +++++++++++++ 2 files changed, 23 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index dd7c87bbc613..cf5483d6b5b7 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -158,6 +158,16 @@ bool snd_hdac_check_power_state(struct hdac_device *hdac, hda_nid_t nid, unsigned int target_state); unsigned int snd_hdac_sync_power_state(struct hdac_device *hdac, hda_nid_t nid, unsigned int target_state);
+#if IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI) +bool snd_hda_device_is_hdmi(struct hdac_device *hdev); +#else +static inline bool snd_hda_device_is_hdmi(struct hdac_device *hdev) +{
- return false;
+} +#endif
/**
- snd_hdac_read_parm - read a codec parameter
- @codec: the codec object
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 1cde2a69bdb4..d6943575c8c7 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -4645,6 +4645,19 @@ HDA_CODEC_ENTRY(HDA_CODEC_ID_GENERIC_HDMI, "Generic HDMI", patch_generic_hdmi), }; MODULE_DEVICE_TABLE(hdaudio, snd_hda_id_hdmi);
+bool snd_hda_device_is_hdmi(struct hdac_device *hdev) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(snd_hda_id_hdmi); i++) {
if (snd_hda_id_hdmi[i].vendor_id == hdev->vendor_id)return true;- }
- return false;
+} +EXPORT_SYMBOL_GPL(snd_hda_device_is_hdmi);
I'm afraid that this will bring unnecessary dependency on HDMI codec driver.
IMO, it's better to add a bool flag is_hdmi to struct hdac_device, and let the HDMI codec driver setting it at the probe, instead. Then we can save an extra exported symbol, too.
thanks,
Takashi
On 27/11/2023 15:18, Takashi Iwai wrote:
+bool snd_hda_device_is_hdmi(struct hdac_device *hdev) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(snd_hda_id_hdmi); i++) {
if (snd_hda_id_hdmi[i].vendor_id == hdev->vendor_id)return true;- }
- return false;
+} +EXPORT_SYMBOL_GPL(snd_hda_device_is_hdmi);
I'm afraid that this will bring unnecessary dependency on HDMI codec driver.
For HDMI support we anyways need HDMI code?
IMO, it's better to add a bool flag is_hdmi to struct hdac_device, and let the HDMI codec driver setting it at the probe, instead. Then we can save an extra exported symbol, too.
We only use a combined generic codec driver which just supports 'HDA' codecs regardless of what type they are.
When things probed via ASoC/SOF I just could not find any other way to access to this table inside of patch_hdmi.c.
We could sort of 'cheat' and look for a specific (I'm not sure if it is Intel or HDA generic) mask: 0x4 It is used in Intel drivers to identify the display codec, so the HDMI/DP. If the 0x4 is universal among all HDA platforms for HDMI/DP then I'm more than happy to drop this patch, but I'm not sure about that.
On Mon, 27 Nov 2023 15:12:51 +0100, Péter Ujfalusi wrote:
On 27/11/2023 15:18, Takashi Iwai wrote:
+bool snd_hda_device_is_hdmi(struct hdac_device *hdev) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(snd_hda_id_hdmi); i++) {
if (snd_hda_id_hdmi[i].vendor_id == hdev->vendor_id)return true;- }
- return false;
+} +EXPORT_SYMBOL_GPL(snd_hda_device_is_hdmi);
I'm afraid that this will bring unnecessary dependency on HDMI codec driver.
For HDMI support we anyways need HDMI code?
But the ASoC hdac-hda driver isn't specifically bound with HDMI, I thought?
With your patch, now it becomes a hard-dependency. It'll be even build failure when HDMI codec driver isn't enabled in Kconfig.
Takashi
On 27/11/2023 16:31, Takashi Iwai wrote:
On Mon, 27 Nov 2023 15:12:51 +0100, Péter Ujfalusi wrote:
On 27/11/2023 15:18, Takashi Iwai wrote:
+bool snd_hda_device_is_hdmi(struct hdac_device *hdev) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(snd_hda_id_hdmi); i++) {
if (snd_hda_id_hdmi[i].vendor_id == hdev->vendor_id)return true;- }
- return false;
+} +EXPORT_SYMBOL_GPL(snd_hda_device_is_hdmi);
I'm afraid that this will bring unnecessary dependency on HDMI codec driver.
For HDMI support we anyways need HDMI code?
But the ASoC hdac-hda driver isn't specifically bound with HDMI, I thought?
With your patch, now it becomes a hard-dependency. It'll be even build failure when HDMI codec driver isn't enabled in Kconfig.
The change in hdaudio.h handles the config dependency, if CONFIG_SND_HDA_CODEC_HDMI is not enabled in Kconfig then snd_hda_device_is_hdmi() will return false.
On Mon, 27 Nov 2023 15:45:54 +0100, Péter Ujfalusi wrote:
On 27/11/2023 16:31, Takashi Iwai wrote:
On Mon, 27 Nov 2023 15:12:51 +0100, Péter Ujfalusi wrote:
On 27/11/2023 15:18, Takashi Iwai wrote:
+bool snd_hda_device_is_hdmi(struct hdac_device *hdev) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(snd_hda_id_hdmi); i++) {
if (snd_hda_id_hdmi[i].vendor_id == hdev->vendor_id)return true;- }
- return false;
+} +EXPORT_SYMBOL_GPL(snd_hda_device_is_hdmi);
I'm afraid that this will bring unnecessary dependency on HDMI codec driver.
For HDMI support we anyways need HDMI code?
But the ASoC hdac-hda driver isn't specifically bound with HDMI, I thought?
With your patch, now it becomes a hard-dependency. It'll be even build failure when HDMI codec driver isn't enabled in Kconfig.
The change in hdaudio.h handles the config dependency, if CONFIG_SND_HDA_CODEC_HDMI is not enabled in Kconfig then snd_hda_device_is_hdmi() will return false.
OK, that's at least good. But I still find it not ideal to bring the hard dependency there unnecessarily.
With the introduction of a flag in hdac_device, the necessary change would be even smaller like below.
Takashi
--- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -95,6 +95,7 @@ struct hdac_device { bool lazy_cache:1; /* don't wake up for writes */ bool caps_overwriting:1; /* caps overwrite being in process */ bool cache_coef:1; /* cache COEF read/write too */ + bool is_hdmi:1; /* a HDMI/DP codec */ unsigned int registered:1; /* codec was registered */ };
--- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2597,6 +2597,7 @@ static int patch_generic_hdmi(struct hda_codec *codec) }
generic_hdmi_init_per_pins(codec); + codec->core.is_hdmi = true; return 0; }
@@ -3472,6 +3473,7 @@ static int patch_simple_hdmi(struct hda_codec *codec, spec->pcm_playback = simple_pcm_playback;
codec->patch_ops = simple_hdmi_patch_ops; + codec->core.is_hdmi = true;
return 0; }
On 27/11/2023 17:20, Takashi Iwai wrote:
On Mon, 27 Nov 2023 15:45:54 +0100, Péter Ujfalusi wrote:
On 27/11/2023 16:31, Takashi Iwai wrote:
On Mon, 27 Nov 2023 15:12:51 +0100, Péter Ujfalusi wrote:
On 27/11/2023 15:18, Takashi Iwai wrote:
+bool snd_hda_device_is_hdmi(struct hdac_device *hdev) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(snd_hda_id_hdmi); i++) {
if (snd_hda_id_hdmi[i].vendor_id == hdev->vendor_id)return true;- }
- return false;
+} +EXPORT_SYMBOL_GPL(snd_hda_device_is_hdmi);
I'm afraid that this will bring unnecessary dependency on HDMI codec driver.
For HDMI support we anyways need HDMI code?
But the ASoC hdac-hda driver isn't specifically bound with HDMI, I thought?
With your patch, now it becomes a hard-dependency. It'll be even build failure when HDMI codec driver isn't enabled in Kconfig.
The change in hdaudio.h handles the config dependency, if CONFIG_SND_HDA_CODEC_HDMI is not enabled in Kconfig then snd_hda_device_is_hdmi() will return false.
OK, that's at least good. But I still find it not ideal to bring the hard dependency there unnecessarily.
With the introduction of a flag in hdac_device, the necessary change would be even smaller like below.
Takashi
--- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -95,6 +95,7 @@ struct hdac_device { bool lazy_cache:1; /* don't wake up for writes */ bool caps_overwriting:1; /* caps overwrite being in process */ bool cache_coef:1; /* cache COEF read/write too */
- bool is_hdmi:1; /* a HDMI/DP codec */ unsigned int registered:1; /* codec was registered */
};
--- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2597,6 +2597,7 @@ static int patch_generic_hdmi(struct hda_codec *codec) }
generic_hdmi_init_per_pins(codec);
- codec->core.is_hdmi = true; return 0;
}
@@ -3472,6 +3473,7 @@ static int patch_simple_hdmi(struct hda_codec *codec, spec->pcm_playback = simple_pcm_playback;
codec->patch_ops = simple_hdmi_patch_ops;
codec->core.is_hdmi = true;
return 0;
}
I see, so this is what I was not sure how to do ;) I will rework the series and resend tomorrow.
Thanks for the code snippet, I will make you as author of it, if it is OK for you.
On Mon, 27 Nov 2023 16:40:57 +0100, Péter Ujfalusi wrote:
On 27/11/2023 17:20, Takashi Iwai wrote:
On Mon, 27 Nov 2023 15:45:54 +0100, Péter Ujfalusi wrote:
On 27/11/2023 16:31, Takashi Iwai wrote:
On Mon, 27 Nov 2023 15:12:51 +0100, Péter Ujfalusi wrote:
On 27/11/2023 15:18, Takashi Iwai wrote:
> +bool snd_hda_device_is_hdmi(struct hdac_device *hdev) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(snd_hda_id_hdmi); i++) { > + if (snd_hda_id_hdmi[i].vendor_id == hdev->vendor_id) > + return true; > + } > + > + return false; > +} > +EXPORT_SYMBOL_GPL(snd_hda_device_is_hdmi);
I'm afraid that this will bring unnecessary dependency on HDMI codec driver.
For HDMI support we anyways need HDMI code?
But the ASoC hdac-hda driver isn't specifically bound with HDMI, I thought?
With your patch, now it becomes a hard-dependency. It'll be even build failure when HDMI codec driver isn't enabled in Kconfig.
The change in hdaudio.h handles the config dependency, if CONFIG_SND_HDA_CODEC_HDMI is not enabled in Kconfig then snd_hda_device_is_hdmi() will return false.
OK, that's at least good. But I still find it not ideal to bring the hard dependency there unnecessarily.
With the introduction of a flag in hdac_device, the necessary change would be even smaller like below.
Takashi
--- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -95,6 +95,7 @@ struct hdac_device { bool lazy_cache:1; /* don't wake up for writes */ bool caps_overwriting:1; /* caps overwrite being in process */ bool cache_coef:1; /* cache COEF read/write too */
- bool is_hdmi:1; /* a HDMI/DP codec */ unsigned int registered:1; /* codec was registered */
};
--- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2597,6 +2597,7 @@ static int patch_generic_hdmi(struct hda_codec *codec) }
generic_hdmi_init_per_pins(codec);
- codec->core.is_hdmi = true; return 0;
}
@@ -3472,6 +3473,7 @@ static int patch_simple_hdmi(struct hda_codec *codec, spec->pcm_playback = simple_pcm_playback;
codec->patch_ops = simple_hdmi_patch_ops;
codec->core.is_hdmi = true;
return 0;
}
I see, so this is what I was not sure how to do ;) I will rework the series and resend tomorrow.
Thanks for the code snippet, I will make you as author of it, if it is OK for you.
Sure, no problem.
Takashi
On 27/11/2023 17:43, Takashi Iwai wrote:
On Mon, 27 Nov 2023 16:40:57 +0100,
--- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -95,6 +95,7 @@ struct hdac_device { bool lazy_cache:1; /* don't wake up for writes */ bool caps_overwriting:1; /* caps overwrite being in process */ bool cache_coef:1; /* cache COEF read/write too */
- bool is_hdmi:1; /* a HDMI/DP codec */ unsigned int registered:1; /* codec was registered */
};
--- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2597,6 +2597,7 @@ static int patch_generic_hdmi(struct hda_codec *codec) }
generic_hdmi_init_per_pins(codec);
- codec->core.is_hdmi = true; return 0;
}
@@ -3472,6 +3473,7 @@ static int patch_simple_hdmi(struct hda_codec *codec, spec->pcm_playback = simple_pcm_playback;
codec->patch_ops = simple_hdmi_patch_ops;
codec->core.is_hdmi = true;
return 0;
}
I see, so this is what I was not sure how to do ;) I will rework the series and resend tomorrow.
Thanks for the code snippet, I will make you as author of it, if it is OK for you.
Sure, no problem.
The flag does not work with SOF stack which uses the hdac_hda codec driver:
patch_generic_hdmi() and patch_simple_hdmi() is not entered at all, so the flag is not set.
The codec driver have is_hdmi_codec() helper to check the struct hda_pcm.pcm_type, but that is not set early enough either.
The is HDMI or not needs to be known in hdac_hda_dev_probe(), I think this was one of the reason I have opted to have the exported function. We just don't have other information at the dev probe time.
# dmesg | grep peter [ 3.810841] [peter] hdac_hda_dev_probe: is_hdmi_codec(): 0 [ 3.810846] [peter] hdac_hda_dev_probe: hdev->is_hdmi: 0 [ 3.810848] [peter] hdac_hda_dev_probe: snd_hda_device_is_hdmi(): 0 ... [ 3.814497] [peter] hdac_hda_dev_probe: is_hdmi_codec(): 0 [ 3.814499] [peter] hdac_hda_dev_probe: hdev->is_hdmi: 0 [ 3.814500] [peter] hdac_hda_dev_probe: snd_hda_device_is_hdmi(): 1 ... [ 3.986610] [peter] generic_hdmi_build_pcms: ENTER [ 3.986627] [peter] hdac_hda_codec_probe: is_hdmi_codec(): 1 ... [ 3.996383] [peter] snd_hda_parse_pin_defcfg: ENTER [ 4.001562] [peter] hdac_hda_codec_probe: is_hdmi_codec(): 0
On Tue, 28 Nov 2023 10:10:48 +0100, Péter Ujfalusi wrote:
On 27/11/2023 17:43, Takashi Iwai wrote:
On Mon, 27 Nov 2023 16:40:57 +0100,
--- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -95,6 +95,7 @@ struct hdac_device { bool lazy_cache:1; /* don't wake up for writes */ bool caps_overwriting:1; /* caps overwrite being in process */ bool cache_coef:1; /* cache COEF read/write too */
- bool is_hdmi:1; /* a HDMI/DP codec */ unsigned int registered:1; /* codec was registered */
};
--- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2597,6 +2597,7 @@ static int patch_generic_hdmi(struct hda_codec *codec) }
generic_hdmi_init_per_pins(codec);
- codec->core.is_hdmi = true; return 0;
}
@@ -3472,6 +3473,7 @@ static int patch_simple_hdmi(struct hda_codec *codec, spec->pcm_playback = simple_pcm_playback;
codec->patch_ops = simple_hdmi_patch_ops;
codec->core.is_hdmi = true;
return 0;
}
I see, so this is what I was not sure how to do ;) I will rework the series and resend tomorrow.
Thanks for the code snippet, I will make you as author of it, if it is OK for you.
Sure, no problem.
The flag does not work with SOF stack which uses the hdac_hda codec driver:
patch_generic_hdmi() and patch_simple_hdmi() is not entered at all, so the flag is not set.
The codec driver have is_hdmi_codec() helper to check the struct hda_pcm.pcm_type, but that is not set early enough either.
The is HDMI or not needs to be known in hdac_hda_dev_probe(), I think this was one of the reason I have opted to have the exported function. We just don't have other information at the dev probe time.
# dmesg | grep peter [ 3.810841] [peter] hdac_hda_dev_probe: is_hdmi_codec(): 0 [ 3.810846] [peter] hdac_hda_dev_probe: hdev->is_hdmi: 0 [ 3.810848] [peter] hdac_hda_dev_probe: snd_hda_device_is_hdmi(): 0 ... [ 3.814497] [peter] hdac_hda_dev_probe: is_hdmi_codec(): 0 [ 3.814499] [peter] hdac_hda_dev_probe: hdev->is_hdmi: 0 [ 3.814500] [peter] hdac_hda_dev_probe: snd_hda_device_is_hdmi(): 1 ... [ 3.986610] [peter] generic_hdmi_build_pcms: ENTER [ 3.986627] [peter] hdac_hda_codec_probe: is_hdmi_codec(): 1 ... [ 3.996383] [peter] snd_hda_parse_pin_defcfg: ENTER [ 4.001562] [peter] hdac_hda_codec_probe: is_hdmi_codec(): 0
Hm... I still find it's a bad move to use an exported symbol from another codec driver.
And, I wonder what if you have a system that has only one HDMI codec without analog one? Would it still work with your change?
Takashi
On 28/11/2023 11:39, Takashi Iwai wrote:
Hm... I still find it's a bad move to use an exported symbol from another codec driver.
The other option is to check for 0x4 (or address 2), but I'm not sure if this is Intel only or universally true for HDMI codecs.
And, I wonder what if you have a system that has only one HDMI codec without analog one? Would it still work with your change?
Yes, it works with only HDMI codec (for example on SoundWire laptops) or with UP2 board which only have HDMI audio support by default.
It also works if we disable HDMI and only have analog codec.
On Tue, 28 Nov 2023 10:53:56 +0100, Péter Ujfalusi wrote:
On 28/11/2023 11:39, Takashi Iwai wrote:
Hm... I still find it's a bad move to use an exported symbol from another codec driver.
The other option is to check for 0x4 (or address 2), but I'm not sure if this is Intel only or universally true for HDMI codecs.
And, I wonder what if you have a system that has only one HDMI codec without analog one? Would it still work with your change?
Yes, it works with only HDMI codec (for example on SoundWire laptops) or with UP2 board which only have HDMI audio support by default.
Interesting. With your patch 2, hdac_hda_hdmi_codec is without the DAPM definitions, and even if that's the only one that is registered, it will still work? So it means that it works without DAPM definitions at all?
Takashi
On 28/11/2023 12:02, Takashi Iwai wrote:
On Tue, 28 Nov 2023 10:53:56 +0100, Péter Ujfalusi wrote:
On 28/11/2023 11:39, Takashi Iwai wrote:
Hm... I still find it's a bad move to use an exported symbol from another codec driver.
The other option is to check for 0x4 (or address 2), but I'm not sure if this is Intel only or universally true for HDMI codecs.
And, I wonder what if you have a system that has only one HDMI codec without analog one? Would it still work with your change?
Yes, it works with only HDMI codec (for example on SoundWire laptops) or with UP2 board which only have HDMI audio support by default.
Interesting. With your patch 2, hdac_hda_hdmi_codec is without the DAPM definitions, and even if that's the only one that is registered, it will still work? So it means that it works without DAPM definitions at all?
Well, it is a bit more 'interesting' from that angle. for patch two we needed: https://lore.kernel.org/linux-sound/20231124124015.15878-1-peter.ujfalusi@li...
The reason is that prior to patch 2 the analog codec would create the DAPM widgets for the HDMI also and the DAPM route would be available but the HDMI would still not work: we had PCMs for HDMI but non operational ones.
If we had a codec+HDMI then we would have the warnings that the second codec is registering the same DAPM widgets again.
With the linked patch plus this series we will not register the DAPM widgets and the machine driver would drop the routes. The PCMs will be still created and they will be still non functional but we will have no warning when the system have two codecs.
The core HDA+patch_hdmi is needed for the hdac_hda to have working HDMI audio, but the patch init is after the codec driver's probe:
# dmesg | grep peter [ 4088.698111] [peter] hdac_hda_dev_probe: is_hdmi_codec(): 0 [ 4088.698112] [peter] hdac_hda_dev_probe: hdev->is_hdmi: 0 [ 4088.698114] [peter] hdac_hda_dev_probe: snd_hda_device_is_hdmi(): 1 [ 4088.862882] [peter] patch_i915_tgl_hdmi: ENTER [ 4088.862886] [peter] alloc_intel_hdmi: ENTER [ 4088.872269] [peter] generic_hdmi_build_pcms: ENTER [ 4088.872279] [peter] hdac_hda_codec_probe: is_hdmi_codec(): 1
We need to know if the codec is HDMI or not in hdac_hda_dev_probe()
I would rather not risk to move the hdac_hda as Intel only using address 2 as HDMI indication - which I'm still not sure if it is Intel only or generic HDA convention.
On Tue, 28 Nov 2023 11:16:00 +0100, Péter Ujfalusi wrote:
On 28/11/2023 12:02, Takashi Iwai wrote:
On Tue, 28 Nov 2023 10:53:56 +0100, Péter Ujfalusi wrote:
On 28/11/2023 11:39, Takashi Iwai wrote:
Hm... I still find it's a bad move to use an exported symbol from another codec driver.
The other option is to check for 0x4 (or address 2), but I'm not sure if this is Intel only or universally true for HDMI codecs.
And, I wonder what if you have a system that has only one HDMI codec without analog one? Would it still work with your change?
Yes, it works with only HDMI codec (for example on SoundWire laptops) or with UP2 board which only have HDMI audio support by default.
Interesting. With your patch 2, hdac_hda_hdmi_codec is without the DAPM definitions, and even if that's the only one that is registered, it will still work? So it means that it works without DAPM definitions at all?
Well, it is a bit more 'interesting' from that angle. for patch two we needed: https://lore.kernel.org/linux-sound/20231124124015.15878-1-peter.ujfalusi@li...
Ouch, this kind of information has to be mentioned in the patch description. Otherwise one would take only this series and face a problem easily. I can imagine such a problem on the stable tree.
The reason is that prior to patch 2 the analog codec would create the DAPM widgets for the HDMI also and the DAPM route would be available but the HDMI would still not work: we had PCMs for HDMI but non operational ones.
If we had a codec+HDMI then we would have the warnings that the second codec is registering the same DAPM widgets again.
With the linked patch plus this series we will not register the DAPM widgets and the machine driver would drop the routes. The PCMs will be still created and they will be still non functional but we will have no warning when the system have two codecs.
The core HDA+patch_hdmi is needed for the hdac_hda to have working HDMI audio, but the patch init is after the codec driver's probe:
# dmesg | grep peter [ 4088.698111] [peter] hdac_hda_dev_probe: is_hdmi_codec(): 0 [ 4088.698112] [peter] hdac_hda_dev_probe: hdev->is_hdmi: 0 [ 4088.698114] [peter] hdac_hda_dev_probe: snd_hda_device_is_hdmi(): 1 [ 4088.862882] [peter] patch_i915_tgl_hdmi: ENTER [ 4088.862886] [peter] alloc_intel_hdmi: ENTER [ 4088.872269] [peter] generic_hdmi_build_pcms: ENTER [ 4088.872279] [peter] hdac_hda_codec_probe: is_hdmi_codec(): 1
We need to know if the codec is HDMI or not in hdac_hda_dev_probe()
I would rather not risk to move the hdac_hda as Intel only using address 2 as HDMI indication - which I'm still not sure if it is Intel only or generic HDA convention.
Sure, it doesn't sound right, either.
Can we then add DAPM widgets and routes later conditionally instead of having it in component driver definition?
Takashi
On 28/11/2023 12:48, Takashi Iwai wrote:
Well, it is a bit more 'interesting' from that angle. for patch two we needed: https://lore.kernel.org/linux-sound/20231124124015.15878-1-peter.ujfalusi@li...
Ouch, this kind of information has to be mentioned in the patch description. Otherwise one would take only this series and face a problem easily. I can imagine such a problem on the stable tree.
OK, I will update the commit message
I would rather not risk to move the hdac_hda as Intel only using address 2 as HDMI indication - which I'm still not sure if it is Intel only or generic HDA convention.
Sure, it doesn't sound right, either.
Can we then add DAPM widgets and routes later conditionally instead of having it in component driver definition?
The issue is with the DAIs. If I remove the dai registering from hdac_hda_dev_probe() to be done in hdac_hda_codec_probe() then the probe will not happen since we do not have the needed components/DAIs to probe the card.
If we don't have HDMI then the machine driver will substitute it with dummy-dai, but if we have HDMI then we are not going to probe at all.
It is a sort of chicken and egg situation, right?
On 28/11/2023 13:58, Péter Ujfalusi wrote:
On 28/11/2023 12:48, Takashi Iwai wrote:
Well, it is a bit more 'interesting' from that angle. for patch two we needed: https://lore.kernel.org/linux-sound/20231124124015.15878-1-peter.ujfalusi@li...
Ouch, this kind of information has to be mentioned in the patch description. Otherwise one would take only this series and face a problem easily. I can imagine such a problem on the stable tree.
OK, I will update the commit message
I would rather not risk to move the hdac_hda as Intel only using address 2 as HDMI indication - which I'm still not sure if it is Intel only or generic HDA convention.
Sure, it doesn't sound right, either.
Can we then add DAPM widgets and routes later conditionally instead of having it in component driver definition?
The issue is with the DAIs. If I remove the dai registering from hdac_hda_dev_probe() to be done in hdac_hda_codec_probe() then the probe will not happen since we do not have the needed components/DAIs to probe the card.
If we don't have HDMI then the machine driver will substitute it with dummy-dai, but if we have HDMI then we are not going to probe at all.
It is a sort of chicken and egg situation, right?
I think I have found a workaround without the need to export a function, it is going to be a single patch and should be OK for non Intel platforms in the future.
struct hdac_hda_priv *hda_pvt = dev_get_drvdata(&hdev->dev); .. if (hda_pvt->need_display_power) /* HDMI/DP */ else /* Non HDMI */
On Tue, 28 Nov 2023 13:16:29 +0100, Péter Ujfalusi wrote:
On 28/11/2023 13:58, Péter Ujfalusi wrote:
On 28/11/2023 12:48, Takashi Iwai wrote:
Well, it is a bit more 'interesting' from that angle. for patch two we needed: https://lore.kernel.org/linux-sound/20231124124015.15878-1-peter.ujfalusi@li...
Ouch, this kind of information has to be mentioned in the patch description. Otherwise one would take only this series and face a problem easily. I can imagine such a problem on the stable tree.
OK, I will update the commit message
I would rather not risk to move the hdac_hda as Intel only using address 2 as HDMI indication - which I'm still not sure if it is Intel only or generic HDA convention.
Sure, it doesn't sound right, either.
Can we then add DAPM widgets and routes later conditionally instead of having it in component driver definition?
The issue is with the DAIs. If I remove the dai registering from hdac_hda_dev_probe() to be done in hdac_hda_codec_probe() then the probe will not happen since we do not have the needed components/DAIs to probe the card.
If we don't have HDMI then the machine driver will substitute it with dummy-dai, but if we have HDMI then we are not going to probe at all.
It is a sort of chicken and egg situation, right?
I think I have found a workaround without the need to export a function, it is going to be a single patch and should be OK for non Intel platforms in the future.
struct hdac_hda_priv *hda_pvt = dev_get_drvdata(&hdev->dev); .. if (hda_pvt->need_display_power) /* HDMI/DP */ else /* Non HDMI */
Looks like a saner approach, yeah.
thanks,
Takashi
The current driver is registering the same dais for each hdev found in the system which results duplicated widgets to be registered and the kernel log contains similar prints: snd_hda_codec_realtek ehdaudio0D0: ASoC: sink widget AIF1TX overwritten snd_hda_codec_realtek ehdaudio0D0: ASoC: source widget AIF1RX overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget hifi3 overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget hifi2 overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget hifi1 overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: source widget Codec Output Pin1 overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget Codec Input Pin1 overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget Analog Codec Playback overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget Digital Codec Playback overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: sink widget Alt Analog Codec Playback overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: source widget Analog Codec Capture overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: source widget Digital Codec Capture overwritten skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: source widget Alt Analog Codec Capture overwritten
To avoid such issue, split the dai array into HDMI and non HDMI array and register them conditionally: for HDMI hdev only register the dais needed for HDMI for non HDMI hdev do not register the HDMI dais.
Link: https://github.com/thesofproject/linux/issues/4509 Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/soc/codecs/hdac_hda.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c index 355f30779a34..72916a387ad6 100644 --- a/sound/soc/codecs/hdac_hda.c +++ b/sound/soc/codecs/hdac_hda.c @@ -132,6 +132,9 @@ static struct snd_soc_dai_driver hdac_hda_dais[] = { .sig_bits = 24, }, }, +}; + +static struct snd_soc_dai_driver hdac_hda_hdmi_dais[] = { { .id = HDAC_HDMI_0_DAI_ID, .name = "intel-hdmi-hifi1", @@ -607,6 +610,13 @@ static const struct snd_soc_component_driver hdac_hda_codec = { .endianness = 1, };
+static const struct snd_soc_component_driver hdac_hda_hdmi_codec = { + .probe = hdac_hda_codec_probe, + .remove = hdac_hda_codec_remove, + .idle_bias_on = false, + .endianness = 1, +}; + static int hdac_hda_dev_probe(struct hdac_device *hdev) { struct hdac_ext_link *hlink; @@ -621,9 +631,15 @@ static int hdac_hda_dev_probe(struct hdac_device *hdev) snd_hdac_ext_bus_link_get(hdev->bus, hlink);
/* ASoC specific initialization */ - ret = devm_snd_soc_register_component(&hdev->dev, - &hdac_hda_codec, hdac_hda_dais, - ARRAY_SIZE(hdac_hda_dais)); + if (snd_hda_device_is_hdmi(hdev)) + ret = devm_snd_soc_register_component(&hdev->dev, + &hdac_hda_hdmi_codec, hdac_hda_hdmi_dais, + ARRAY_SIZE(hdac_hda_hdmi_dais)); + else + ret = devm_snd_soc_register_component(&hdev->dev, + &hdac_hda_codec, hdac_hda_dais, + ARRAY_SIZE(hdac_hda_dais)); + if (ret < 0) { dev_err(&hdev->dev, "failed to register HDA codec %d\n", ret); return ret;
participants (2)
-
Peter Ujfalusi -
Takashi Iwai