[PATCH v2 0/4] ALSA/ASoC: Conditionally skip i915 init and cleanups

[PATCH v2 0/4] ALSA/ASoC: Conditionally skip i915 init and cleanups

[Cezary Rojewski]

A small set of changes to improve initialization of the audio stack on
HDAudio devices and pair of cleanups.

As the first change is the most important one here, following is the
technical background for it:

Commit 78f613ba1efb ("drm/i915: finish removal of CNL") and its friends
removed support for i915 for all CNL-based platforms. HDAudio library,
however, still treats such platforms as valid candidates for i915
binding. Update query mechanism to reflect changes made in drm tree.

At the same time, i915 support for LKF-based platforms has not been
provided so remove them from valid binding candidates.

The snd_soc_hda change is a follow up for the above and the cleanup
patches do not bring any functional changes.

Changes in v2:
- list of problematic VGA devices is now declared locally, no more
  touching drm stuff

Cezary Rojewski (4):
  ALSA: hda: Skip i915 initialization on CNL/LKF-based platforms
  ASoC: codecs: hda: Skip HDMI/DP registration if i915 is missing
  ASoC: codecs: hda: Cleanup error messages
  ALSA: hda: Reuse for_each_pcm_streams()

 sound/hda/hdac_i915.c     | 32 +++++++++++++++++++++++++++++---
 sound/pci/hda/hda_codec.c |  2 +-
 sound/soc/codecs/hda.c    | 15 ++++++++++-----
 3 files changed, 40 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] ALSA: hda: Skip i915 initialization on CNL/LKF-based platforms

[Cezary Rojewski]

Commit 78f613ba1efb ("drm/i915: finish removal of CNL") and its friends
removed support for i915 for all CNL-based platforms. HDAudio library,
however, still treats such platforms as valid candidates for i915
binding. Update query mechanism to reflect changes made in drm tree.

At the same time, i915 support for LKF-based platforms has not been
provided so remove them from valid binding candidates.

Link: https://lore.kernel.org/all/20210728215946.1573015-1-lucas.demarchi@intel.com/
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/hda/hdac_i915.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 365c36fdf205..afee87bd0f2e 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -127,15 +127,41 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
 /* check whether Intel graphics is present and reachable */
 static int i915_gfx_present(struct pci_dev *hdac_pci)
 {
+	/* List of known platforms with no i915 support. */
+	static struct pci_device_id denylist[] = {
+		/* CNL */
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a40), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a41), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a42), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a44), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a49), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a4a), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a4c), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a50), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a51), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a52), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a54), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a59), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a5a), 0x030000, 0xff0000 },
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a5c), 0x030000, 0xff0000 },
+		/* LKF */
+		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x9840), 0x030000, 0xff0000 },
+		{ 0 }
+	};
 	struct pci_dev *display_dev = NULL;
 
 	if (!gpu_bind || (gpu_bind < 0 && video_firmware_drivers_only()))
 		return false;
 
 	for_each_pci_dev(display_dev) {
-		if (display_dev->vendor == PCI_VENDOR_ID_INTEL &&
-		    (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&
-		    connectivity_check(display_dev, hdac_pci)) {
+		if (display_dev->vendor != PCI_VENDOR_ID_INTEL ||
+		    (display_dev->class >> 16) != PCI_BASE_CLASS_DISPLAY)
+			continue;
+
+		if (pci_match_id(denylist, display_dev))
+			continue;
+
+		if (connectivity_check(display_dev, hdac_pci)) {
 			pci_dev_put(display_dev);
 			return true;
 		}
-- 
2.25.1

Re: [PATCH v2 1/4] ALSA: hda: Skip i915 initialization on CNL/LKF-based platforms

[Rodrigo Vivi]

On Fri, Feb 23, 2024 at 12:46:23PM +0100, Cezary Rojewski wrote:
> Commit 78f613ba1efb ("drm/i915: finish removal of CNL") and its friends
> removed support for i915 for all CNL-based platforms. HDAudio library,
> however, still treats such platforms as valid candidates for i915
> binding. Update query mechanism to reflect changes made in drm tree.
> 
> At the same time, i915 support for LKF-based platforms has not been
> provided so remove them from valid binding candidates.
> 
> Link: https://lore.kernel.org/all/20210728215946.1573015-1-lucas.demarchi@intel.com/
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  sound/hda/hdac_i915.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 365c36fdf205..afee87bd0f2e 100644
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -127,15 +127,41 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
>  /* check whether Intel graphics is present and reachable */
>  static int i915_gfx_present(struct pci_dev *hdac_pci)
>  {
> +	/* List of known platforms with no i915 support. */
> +	static struct pci_device_id denylist[] = {
> +		/* CNL */
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a40), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a41), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a42), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a44), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a49), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a4a), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a4c), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a50), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a51), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a52), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a54), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a59), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a5a), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a5c), 0x030000, 0xff0000 },
> +		/* LKF */
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x9840), 0x030000, 0xff0000 },
> +		{ 0 }
> +	};
>  	struct pci_dev *display_dev = NULL;
>  
>  	if (!gpu_bind || (gpu_bind < 0 && video_firmware_drivers_only()))
>  		return false;
>  
>  	for_each_pci_dev(display_dev) {
> -		if (display_dev->vendor == PCI_VENDOR_ID_INTEL &&
> -		    (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&
> -		    connectivity_check(display_dev, hdac_pci)) {
> +		if (display_dev->vendor != PCI_VENDOR_ID_INTEL ||
> +		    (display_dev->class >> 16) != PCI_BASE_CLASS_DISPLAY)
> +			continue;
> +
> +		if (pci_match_id(denylist, display_dev))
> +			continue;
> +
> +		if (connectivity_check(display_dev, hdac_pci)) {
>  			pci_dev_put(display_dev);
>  			return true;
>  		}
> -- 
> 2.25.1
> 

Re: [PATCH v2 1/4] ALSA: hda: Skip i915 initialization on CNL/LKF-based platforms

[Jani Nikula]

On Fri, 23 Feb 2024, Cezary Rojewski <cezary.rojewski@intel.com> wrote:
> Commit 78f613ba1efb ("drm/i915: finish removal of CNL") and its friends
> removed support for i915 for all CNL-based platforms. HDAudio library,
> however, still treats such platforms as valid candidates for i915
> binding. Update query mechanism to reflect changes made in drm tree.
>
> At the same time, i915 support for LKF-based platforms has not been
> provided so remove them from valid binding candidates.
>
> Link: https://lore.kernel.org/all/20210728215946.1573015-1-lucas.demarchi@intel.com/
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  sound/hda/hdac_i915.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 365c36fdf205..afee87bd0f2e 100644
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -127,15 +127,41 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
>  /* check whether Intel graphics is present and reachable */
>  static int i915_gfx_present(struct pci_dev *hdac_pci)
>  {
> +	/* List of known platforms with no i915 support. */
> +	static struct pci_device_id denylist[] = {

This should be const to place it in rodata, it doesn't need to be
mutable.

> +		/* CNL */
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a40), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a41), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a42), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a44), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a49), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a4a), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a4c), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a50), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a51), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a52), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a54), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a59), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a5a), 0x030000, 0xff0000 },
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a5c), 0x030000, 0xff0000 },
> +		/* LKF */
> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x9840), 0x030000, 0xff0000 },
> +		{ 0 }

Nitpick, prefer {} over { 0 }.

BR,
Jani.

> +	};
>  	struct pci_dev *display_dev = NULL;
>  
>  	if (!gpu_bind || (gpu_bind < 0 && video_firmware_drivers_only()))
>  		return false;
>  
>  	for_each_pci_dev(display_dev) {
> -		if (display_dev->vendor == PCI_VENDOR_ID_INTEL &&
> -		    (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&
> -		    connectivity_check(display_dev, hdac_pci)) {
> +		if (display_dev->vendor != PCI_VENDOR_ID_INTEL ||
> +		    (display_dev->class >> 16) != PCI_BASE_CLASS_DISPLAY)
> +			continue;
> +
> +		if (pci_match_id(denylist, display_dev))
> +			continue;
> +
> +		if (connectivity_check(display_dev, hdac_pci)) {
>  			pci_dev_put(display_dev);
>  			return true;
>  		}

-- 
Jani Nikula, Intel

Re: [PATCH v2 1/4] ALSA: hda: Skip i915 initialization on CNL/LKF-based platforms

[Cezary Rojewski]

On 2024-02-26 11:40 AM, Jani Nikula wrote:
> On Fri, 23 Feb 2024, Cezary Rojewski <cezary.rojewski@intel.com> wrote:
>> Commit 78f613ba1efb ("drm/i915: finish removal of CNL") and its friends
>> removed support for i915 for all CNL-based platforms. HDAudio library,
>> however, still treats such platforms as valid candidates for i915
>> binding. Update query mechanism to reflect changes made in drm tree.
>>
>> At the same time, i915 support for LKF-based platforms has not been
>> provided so remove them from valid binding candidates.
>>
>> Link: https://lore.kernel.org/all/20210728215946.1573015-1-lucas.demarchi@intel.com/
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   sound/hda/hdac_i915.c | 32 +++++++++++++++++++++++++++++---
>>   1 file changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
>> index 365c36fdf205..afee87bd0f2e 100644
>> --- a/sound/hda/hdac_i915.c
>> +++ b/sound/hda/hdac_i915.c
>> @@ -127,15 +127,41 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
>>   /* check whether Intel graphics is present and reachable */
>>   static int i915_gfx_present(struct pci_dev *hdac_pci)
>>   {
>> +	/* List of known platforms with no i915 support. */
>> +	static struct pci_device_id denylist[] = {
> 
> This should be const to place it in rodata, it doesn't need to be
> mutable.
> 
>> +		/* CNL */
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a40), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a41), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a42), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a44), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a49), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a4a), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a4c), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a50), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a51), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a52), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a54), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a59), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a5a), 0x030000, 0xff0000 },
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x5a5c), 0x030000, 0xff0000 },
>> +		/* LKF */
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x9840), 0x030000, 0xff0000 },
>> +		{ 0 }
> 
> Nitpick, prefer {} over { 0 }.

Ack for both suggestions. Thank you.

Kind regards,
Czarek

[PATCH v2 2/4] ASoC: codecs: hda: Skip HDMI/DP registration if i915 is missing

[Cezary Rojewski]

If i915 does not support given platform but the hardware i.e.: HDAudio
codec is still there, the codec-probing procedure will succeed for such
device but the follow up initialization will always end up with -ENODEV.

While bus could filter out address '2' which Intel's HDMI/DP codecs
always enumerate on, more robust approach is to check for i915 presence
before registering display codecs.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/codecs/hda.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/soc/codecs/hda.c b/sound/soc/codecs/hda.c
index d2117e36ddd1..d9e7cd8aada2 100644
--- a/sound/soc/codecs/hda.c
+++ b/sound/soc/codecs/hda.c
@@ -350,6 +350,11 @@ static int hda_hdev_attach(struct hdac_device *hdev)
 	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
 	struct snd_soc_component_driver *comp_drv;
 
+	if (hda_codec_is_display(codec) && !hdev->bus->audio_component) {
+		dev_dbg(&hdev->dev, "no i915, skip registration for 0x%08x\n", hdev->vendor_id);
+		return 0;
+	}
+
 	comp_drv = devm_kzalloc(&hdev->dev, sizeof(*comp_drv), GFP_KERNEL);
 	if (!comp_drv)
 		return -ENOMEM;
-- 
2.25.1

Re: [PATCH v2 2/4] ASoC: codecs: hda: Skip HDMI/DP registration if i915 is missing

[Takashi Iwai]

On Fri, 23 Feb 2024 12:46:24 +0100,
Cezary Rojewski wrote:
> 
> If i915 does not support given platform but the hardware i.e.: HDAudio
> codec is still there, the codec-probing procedure will succeed for such
> device but the follow up initialization will always end up with -ENODEV.
> 
> While bus could filter out address '2' which Intel's HDMI/DP codecs
> always enumerate on, more robust approach is to check for i915 presence
> before registering display codecs.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  sound/soc/codecs/hda.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/sound/soc/codecs/hda.c b/sound/soc/codecs/hda.c
> index d2117e36ddd1..d9e7cd8aada2 100644
> --- a/sound/soc/codecs/hda.c
> +++ b/sound/soc/codecs/hda.c
> @@ -350,6 +350,11 @@ static int hda_hdev_attach(struct hdac_device *hdev)
>  	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
>  	struct snd_soc_component_driver *comp_drv;
>  
> +	if (hda_codec_is_display(codec) && !hdev->bus->audio_component) {
> +		dev_dbg(&hdev->dev, "no i915, skip registration for 0x%08x\n", hdev->vendor_id);
> +		return 0;

Should we return success here, or would it better with -ENODEV?
IIUC, the code path is from the early hda_codec_driver_probe() hook,
so returning an error can work.


Takashi

Re: [PATCH v2 2/4] ASoC: codecs: hda: Skip HDMI/DP registration if i915 is missing

[Cezary Rojewski]

On 2024-02-23 3:44 PM, Takashi Iwai wrote:
> On Fri, 23 Feb 2024 12:46:24 +0100,
> Cezary Rojewski wrote:
>>
>> If i915 does not support given platform but the hardware i.e.: HDAudio
>> codec is still there, the codec-probing procedure will succeed for such
>> device but the follow up initialization will always end up with -ENODEV.
>>
>> While bus could filter out address '2' which Intel's HDMI/DP codecs
>> always enumerate on, more robust approach is to check for i915 presence
>> before registering display codecs.
>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   sound/soc/codecs/hda.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/sound/soc/codecs/hda.c b/sound/soc/codecs/hda.c
>> index d2117e36ddd1..d9e7cd8aada2 100644
>> --- a/sound/soc/codecs/hda.c
>> +++ b/sound/soc/codecs/hda.c
>> @@ -350,6 +350,11 @@ static int hda_hdev_attach(struct hdac_device *hdev)
>>   	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
>>   	struct snd_soc_component_driver *comp_drv;
>>   
>> +	if (hda_codec_is_display(codec) && !hdev->bus->audio_component) {
>> +		dev_dbg(&hdev->dev, "no i915, skip registration for 0x%08x\n", hdev->vendor_id);
>> +		return 0;
> 
> Should we return success here, or would it better with -ENODEV?
> IIUC, the code path is from the early hda_codec_driver_probe() hook,
> so returning an error can work.

Good suggestion. Indeed attach() is called by probe() which treats 
-ENODEV just fine.

There is a consequence to that though. Logs from LKF show:

snd_soc_hda_codec:hda_hdev_attach: snd_hda_codec_hdmi hdaudioB0D2: no 
i915, skip registration for 0x80862811
snd_soc_hda_codec:hda_hdev_attach: snd_hda_codec_generic hdaudioB0D2: no 
i915, skip registration for 0x80862811
snd_soc_hda_codec:hda_hdev_attach: snd_hda_codec_generic hdaudioB0D2: no 
i915, skip registration for 0x80862811
snd_hda_codec:snd_hda_codec_configure: hdaudio hdaudioB0D2: Unable to 
bind the codec
snd_soc_avs 0000:00:1f.3: failed to config codec -19
snd_soc_avs 0000:00:1f.3: Codec #2 probe error; disabling it...

i.e.: three attempts. One for HDMI via codec_bind_module() and two from 
codec_bind_generic(). Situation on CNL-based is different - two logs, no 
error. The reason for this is lack of codec->preset in LKF case.

Should I add stub entries for ICL-HP/LKF to patch_hdmi.c to address this?

HDA_CODEC_ENTRY(0x80862810, "Icelake-P HDMI",  NULL),
HDA_CODEC_ENTRY(0x80862811, "Lakefield HDMI",  NULL),


Czarek

Re: [PATCH v2 2/4] ASoC: codecs: hda: Skip HDMI/DP registration if i915 is missing

[Takashi Iwai]

On Fri, 23 Feb 2024 18:09:59 +0100,
Cezary Rojewski wrote:
> 
> On 2024-02-23 3:44 PM, Takashi Iwai wrote:
> > On Fri, 23 Feb 2024 12:46:24 +0100,
> > Cezary Rojewski wrote:
> >> 
> >> If i915 does not support given platform but the hardware i.e.: HDAudio
> >> codec is still there, the codec-probing procedure will succeed for such
> >> device but the follow up initialization will always end up with -ENODEV.
> >> 
> >> While bus could filter out address '2' which Intel's HDMI/DP codecs
> >> always enumerate on, more robust approach is to check for i915 presence
> >> before registering display codecs.
> >> 
> >> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> >> ---
> >>   sound/soc/codecs/hda.c | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >> 
> >> diff --git a/sound/soc/codecs/hda.c b/sound/soc/codecs/hda.c
> >> index d2117e36ddd1..d9e7cd8aada2 100644
> >> --- a/sound/soc/codecs/hda.c
> >> +++ b/sound/soc/codecs/hda.c
> >> @@ -350,6 +350,11 @@ static int hda_hdev_attach(struct hdac_device *hdev)
> >>   	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
> >>   	struct snd_soc_component_driver *comp_drv;
> >>   +	if (hda_codec_is_display(codec) &&
> >> !hdev->bus->audio_component) {
> >> +		dev_dbg(&hdev->dev, "no i915, skip registration for 0x%08x\n", hdev->vendor_id);
> >> +		return 0;
> > 
> > Should we return success here, or would it better with -ENODEV?
> > IIUC, the code path is from the early hda_codec_driver_probe() hook,
> > so returning an error can work.
> 
> Good suggestion. Indeed attach() is called by probe() which treats
> -ENODEV just fine.
> 
> There is a consequence to that though. Logs from LKF show:
> 
> snd_soc_hda_codec:hda_hdev_attach: snd_hda_codec_hdmi hdaudioB0D2: no
> i915, skip registration for 0x80862811
> snd_soc_hda_codec:hda_hdev_attach: snd_hda_codec_generic hdaudioB0D2:
> no i915, skip registration for 0x80862811
> snd_soc_hda_codec:hda_hdev_attach: snd_hda_codec_generic hdaudioB0D2:
> no i915, skip registration for 0x80862811
> snd_hda_codec:snd_hda_codec_configure: hdaudio hdaudioB0D2: Unable to
> bind the codec
> snd_soc_avs 0000:00:1f.3: failed to config codec -19
> snd_soc_avs 0000:00:1f.3: Codec #2 probe error; disabling it...

Yeah the latter two are basically fallbacks, and I guess we can
disable them for ASoC case?  An additional patch like below.


thanks,

Takashi

--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -279,6 +279,10 @@ static int codec_bind_generic(struct hda_codec *codec)
 	if (codec->probe_id)
 		return -ENODEV;
 
+	/* no generic fallback for ASoC binding */
+	if (codec->bus->core.ext_ops)
+		return -ENODEV;
+
 	if (is_likely_hdmi_codec(codec)) {
 		codec->probe_id = HDA_CODEC_ID_GENERIC_HDMI;
 		request_codec_module(codec);

Re: [PATCH v2 2/4] ASoC: codecs: hda: Skip HDMI/DP registration if i915 is missing

[Cezary Rojewski]

On 2024-02-26 12:09 PM, Takashi Iwai wrote:
> On Fri, 23 Feb 2024 18:09:59 +0100,
> Cezary Rojewski wrote:
>>
>> On 2024-02-23 3:44 PM, Takashi Iwai wrote:
>>> On Fri, 23 Feb 2024 12:46:24 +0100,
>>> Cezary Rojewski wrote:
>>>>
>>>> If i915 does not support given platform but the hardware i.e.: HDAudio
>>>> codec is still there, the codec-probing procedure will succeed for such
>>>> device but the follow up initialization will always end up with -ENODEV.
>>>>
>>>> While bus could filter out address '2' which Intel's HDMI/DP codecs
>>>> always enumerate on, more robust approach is to check for i915 presence
>>>> before registering display codecs.
>>>>
>>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>>> ---
>>>>    sound/soc/codecs/hda.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/sound/soc/codecs/hda.c b/sound/soc/codecs/hda.c
>>>> index d2117e36ddd1..d9e7cd8aada2 100644
>>>> --- a/sound/soc/codecs/hda.c
>>>> +++ b/sound/soc/codecs/hda.c
>>>> @@ -350,6 +350,11 @@ static int hda_hdev_attach(struct hdac_device *hdev)
>>>>    	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
>>>>    	struct snd_soc_component_driver *comp_drv;
>>>>    +	if (hda_codec_is_display(codec) &&
>>>> !hdev->bus->audio_component) {
>>>> +		dev_dbg(&hdev->dev, "no i915, skip registration for 0x%08x\n", hdev->vendor_id);
>>>> +		return 0;
>>>
>>> Should we return success here, or would it better with -ENODEV?
>>> IIUC, the code path is from the early hda_codec_driver_probe() hook,
>>> so returning an error can work.
>>
>> Good suggestion. Indeed attach() is called by probe() which treats
>> -ENODEV just fine.
>>
>> There is a consequence to that though. Logs from LKF show:
>>
>> snd_soc_hda_codec:hda_hdev_attach: snd_hda_codec_hdmi hdaudioB0D2: no
>> i915, skip registration for 0x80862811
>> snd_soc_hda_codec:hda_hdev_attach: snd_hda_codec_generic hdaudioB0D2:
>> no i915, skip registration for 0x80862811
>> snd_soc_hda_codec:hda_hdev_attach: snd_hda_codec_generic hdaudioB0D2:
>> no i915, skip registration for 0x80862811
>> snd_hda_codec:snd_hda_codec_configure: hdaudio hdaudioB0D2: Unable to
>> bind the codec
>> snd_soc_avs 0000:00:1f.3: failed to config codec -19
>> snd_soc_avs 0000:00:1f.3: Codec #2 probe error; disabling it...
> 
> Yeah the latter two are basically fallbacks, and I guess we can
> disable them for ASoC case?  An additional patch like below.

Thanks for feedback, Takashi. One of the design philosophies for the 
avs-driver is to avoid any 'if (bus->ext_ops)' statements if possible. 
This is to keep runtime flow of snd_hda_intel and its DSP equivalent as 
close as possible.

What I propose is: address the problem directly in the avs-driver by 
ignoring codecs for which probe_codec() returns -ENODEV. The TLDR of my 
previous message is: we do not want to do any kind of recovery e.g.: 
HDAudio controller reset if there is no supporting driver for given 
codec. Will sent a separate patch as part of v3.

> --- a/sound/pci/hda/hda_bind.c
> +++ b/sound/pci/hda/hda_bind.c
> @@ -279,6 +279,10 @@ static int codec_bind_generic(struct hda_codec *codec)
>   	if (codec->probe_id)
>   		return -ENODEV;
>   
> +	/* no generic fallback for ASoC binding */
> +	if (codec->bus->core.ext_ops)
> +		return -ENODEV;
> +
>   	if (is_likely_hdmi_codec(codec)) {
>   		codec->probe_id = HDA_CODEC_ID_GENERIC_HDMI;
>   		request_codec_module(codec);

[PATCH v2 3/4] ASoC: codecs: hda: Cleanup error messages

[Cezary Rojewski]

Be cohesive and use same pattern in each error message.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/codecs/hda.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/hda.c b/sound/soc/codecs/hda.c
index d9e7cd8aada2..8a9d0674555e 100644
--- a/sound/soc/codecs/hda.c
+++ b/sound/soc/codecs/hda.c
@@ -198,19 +198,19 @@ static int hda_codec_probe(struct snd_soc_component *component)
 	ret = snd_hda_codec_device_new(codec->bus, component->card->snd_card, hdev->addr, codec,
 				       false);
 	if (ret < 0) {
-		dev_err(&hdev->dev, "create hda codec failed: %d\n", ret);
+		dev_err(&hdev->dev, "codec create failed: %d\n", ret);
 		goto device_new_err;
 	}
 
 	ret = snd_hda_codec_set_name(codec, codec->preset->name);
 	if (ret < 0) {
-		dev_err(&hdev->dev, "name failed %s\n", codec->preset->name);
+		dev_err(&hdev->dev, "set name: %s failed: %d\n", codec->preset->name, ret);
 		goto err;
 	}
 
 	ret = snd_hdac_regmap_init(&codec->core);
 	if (ret < 0) {
-		dev_err(&hdev->dev, "regmap init failed\n");
+		dev_err(&hdev->dev, "regmap init failed: %d\n", ret);
 		goto err;
 	}
 
@@ -223,13 +223,13 @@ static int hda_codec_probe(struct snd_soc_component *component)
 
 	ret = patch(codec);
 	if (ret < 0) {
-		dev_err(&hdev->dev, "patch failed %d\n", ret);
+		dev_err(&hdev->dev, "codec init failed: %d\n", ret);
 		goto err;
 	}
 
 	ret = snd_hda_codec_parse_pcms(codec);
 	if (ret < 0) {
-		dev_err(&hdev->dev, "unable to map pcms to dai %d\n", ret);
+		dev_err(&hdev->dev, "unable to map pcms to dai: %d\n", ret);
 		goto parse_pcms_err;
 	}
 
-- 
2.25.1

[PATCH v2 4/4] ALSA: hda: Reuse for_each_pcm_streams()

[Cezary Rojewski]

Use the macro to improve readability.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/pci/hda/hda_codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 12f02cdc9659..2cac337f5263 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -3313,7 +3313,7 @@ int snd_hda_codec_parse_pcms(struct hda_codec *codec)
 	list_for_each_entry(cpcm, &codec->pcm_list_head, list) {
 		int stream;
 
-		for (stream = 0; stream < 2; stream++) {
+		for_each_pcm_streams(stream) {
 			struct hda_pcm_stream *info = &cpcm->stream[stream];
 
 			if (!info->substreams)
-- 
2.25.1

Re: [PATCH v2 0/4] ALSA/ASoC: Conditionally skip i915 init and cleanups

[Takashi Iwai]

On Fri, 23 Feb 2024 12:46:22 +0100,
Cezary Rojewski wrote:
> 
> A small set of changes to improve initialization of the audio stack on
> HDAudio devices and pair of cleanups.
> 
> As the first change is the most important one here, following is the
> technical background for it:
> 
> Commit 78f613ba1efb ("drm/i915: finish removal of CNL") and its friends
> removed support for i915 for all CNL-based platforms. HDAudio library,
> however, still treats such platforms as valid candidates for i915
> binding. Update query mechanism to reflect changes made in drm tree.
> 
> At the same time, i915 support for LKF-based platforms has not been
> provided so remove them from valid binding candidates.
> 
> The snd_soc_hda change is a follow up for the above and the cleanup
> patches do not bring any functional changes.
> 
> Changes in v2:
> - list of problematic VGA devices is now declared locally, no more
>   touching drm stuff
> 
> Cezary Rojewski (4):
>   ALSA: hda: Skip i915 initialization on CNL/LKF-based platforms
>   ASoC: codecs: hda: Skip HDMI/DP registration if i915 is missing
>   ASoC: codecs: hda: Cleanup error messages
>   ALSA: hda: Reuse for_each_pcm_streams()

As far as I see, all those 4 patches are rather individually
applicable, right?
The essential change is the first patch, and the patch 2 is rather an
improvement (the driver gives -ENODEV as of now).
And the rest two are merely cleanups.


Takashi