| Message ID | 20231127130245.24295-2-peter.ujfalusi@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA/ASoC: hdmi/hdac_hda: Conditionally register dais | expand |
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@linux.intel.com/ 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@linux.intel.com/ 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@linux.intel.com/ > > 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@linux.intel.com/ >> >> 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@linux.intel.com/ > >> > >> 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
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");