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

[PATCH 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. While I did touch i915_pciids.h
header which is part of drm, I'd like the changes to go through Mark's
tree entirely - being cohesive with the rest of the code by adding LKF
IDs where they belong rather than hiding them within the sound tree.

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.

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()

 include/drm/i915_pciids.h |  4 ++++
 sound/hda/hdac_i915.c     | 18 +++++++++++++++---
 sound/pci/hda/hda_codec.c |  2 +-
 sound/soc/codecs/hda.c    | 15 ++++++++++-----
 4 files changed, 30 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH 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>
---
 include/drm/i915_pciids.h |  4 ++++
 sound/hda/hdac_i915.c     | 18 +++++++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
index fcf1849aa47c..04172b541ee7 100644
--- a/include/drm/i915_pciids.h
+++ b/include/drm/i915_pciids.h
@@ -582,6 +582,10 @@
 	INTEL_VGA_DEVICE(0x8A51, info), \
 	INTEL_VGA_DEVICE(0x8A5D, info)
 
+/* LKF */
+#define INTEL_LKF_IDS(info) \
+	INTEL_VGA_DEVICE(0x9840, info)
+
 /* EHL */
 #define INTEL_EHL_IDS(info) \
 	INTEL_VGA_DEVICE(0x4541, info), \
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 365c36fdf205..07861f9fc491 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -6,6 +6,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <drm/i915_pciids.h>
 #include <sound/core.h>
 #include <sound/hdaudio.h>
 #include <sound/hda_i915.h>
@@ -127,15 +128,26 @@ 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[] = {
+		INTEL_CNL_IDS(NULL),
+		INTEL_LKF_IDS(NULL),
+		{ 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

[PATCH 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

[PATCH 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 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 1/4] ALSA: hda: Skip i915 initialization on CNL/LKF-based platforms

[Ville Syrjälä]

On Thu, Feb 22, 2024 at 06:06:11PM +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>
> ---
>  include/drm/i915_pciids.h |  4 ++++
>  sound/hda/hdac_i915.c     | 18 +++++++++++++++---
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
> index fcf1849aa47c..04172b541ee7 100644
> --- a/include/drm/i915_pciids.h
> +++ b/include/drm/i915_pciids.h
> @@ -582,6 +582,10 @@
>  	INTEL_VGA_DEVICE(0x8A51, info), \
>  	INTEL_VGA_DEVICE(0x8A5D, info)
>  
> +/* LKF */
> +#define INTEL_LKF_IDS(info) \
> +	INTEL_VGA_DEVICE(0x9840, info)
> +
>  /* EHL */
>  #define INTEL_EHL_IDS(info) \
>  	INTEL_VGA_DEVICE(0x4541, info), \
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 365c36fdf205..07861f9fc491 100644
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -6,6 +6,7 @@
>  #include <linux/init.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <drm/i915_pciids.h>
>  #include <sound/core.h>
>  #include <sound/hdaudio.h>
>  #include <sound/hda_i915.h>
> @@ -127,15 +128,26 @@ 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[] = {
> +		INTEL_CNL_IDS(NULL),
> +		INTEL_LKF_IDS(NULL),
> +		{ 0 }
> +	};

I thought these don't actually exist in the wild?

>  	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

-- 
Ville Syrjälä
Intel

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

[Cezary Rojewski]

On 2024-02-22 6:24 PM, Ville Syrjälä wrote:
> On Thu, Feb 22, 2024 at 06:06:11PM +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.

...

>> @@ -127,15 +128,26 @@ 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[] = {
>> +		INTEL_CNL_IDS(NULL),
>> +		INTEL_LKF_IDS(NULL),
>> +		{ 0 }
>> +	};
> 
> I thought these don't actually exist in the wild?

To my knowledge the opposite is true - while LKFs were shipped in 
limited number, they still were. I did ask few weeks ago my friends from 
Windows side about the support and they're still running full-scopes on 
HDMI endpoints on LKF platforms in their CIs. It seems the drm support 
is there though. Once you re-boot to linux we get -19 during probe().

In regard to CNL, the commit removing CNL-support left the IDs intact 
what's very handy to us - we have a lot of spare CNL boards for our 
validation purposes - CNL-based AudioDSP spans multiple platforms, e.g.: 
CNL/CFL/WHL/CML. The number of newer boards is lower, unfortunately.


Kind regards,
Czarek

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

[Rodrigo Vivi]

On Thu, Feb 22, 2024 at 06:53:12PM +0100, Cezary Rojewski wrote:
> On 2024-02-22 6:24 PM, Ville Syrjälä wrote:
> > On Thu, Feb 22, 2024 at 06:06:11PM +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.
> 
> ...
> 
> > > @@ -127,15 +128,26 @@ 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[] = {
> > > +		INTEL_CNL_IDS(NULL),
> > > +		INTEL_LKF_IDS(NULL),
> > > +		{ 0 }
> > > +	};
> > 
> > I thought these don't actually exist in the wild?
> 
> To my knowledge the opposite is true - while LKFs were shipped in limited
> number, they still were. I did ask few weeks ago my friends from Windows
> side about the support and they're still running full-scopes on HDMI
> endpoints on LKF platforms in their CIs. It seems the drm support is there
> though. Once you re-boot to linux we get -19 during probe().
> 
> In regard to CNL, the commit removing CNL-support left the IDs intact what's

I would prefer to go the other way around and remove the unused/unsupported
IDs entirely and for good.

> very handy to us - we have a lot of spare CNL boards for our validation
> purposes - CNL-based AudioDSP spans multiple platforms, e.g.:
> CNL/CFL/WHL/CML. The number of newer boards is lower, unfortunately.

Well, I do see your point here and you are not asking for us to add gfx
support back, but only help to have this protection here.

However I'm afraid that these entries in the list would only cause
further confusion. Couldn't they get defined inside your .c directly as
a const deny_list? so when we go there and remove the missing bits
of CNL we don't conflict or cause undersired issues to you.

> 
> 
> Kind regards,
> Czarek

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

[Takashi Iwai]

On Thu, 22 Feb 2024 21:54:42 +0100,
Rodrigo Vivi wrote:
> 
> On Thu, Feb 22, 2024 at 06:53:12PM +0100, Cezary Rojewski wrote:
> > On 2024-02-22 6:24 PM, Ville Syrjälä wrote:
> > > On Thu, Feb 22, 2024 at 06:06:11PM +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.
> > 
> > ...
> > 
> > > > @@ -127,15 +128,26 @@ 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[] = {
> > > > +		INTEL_CNL_IDS(NULL),
> > > > +		INTEL_LKF_IDS(NULL),
> > > > +		{ 0 }
> > > > +	};
> > > 
> > > I thought these don't actually exist in the wild?
> > 
> > To my knowledge the opposite is true - while LKFs were shipped in limited
> > number, they still were. I did ask few weeks ago my friends from Windows
> > side about the support and they're still running full-scopes on HDMI
> > endpoints on LKF platforms in their CIs. It seems the drm support is there
> > though. Once you re-boot to linux we get -19 during probe().
> > 
> > In regard to CNL, the commit removing CNL-support left the IDs intact what's
> 
> I would prefer to go the other way around and remove the unused/unsupported
> IDs entirely and for good.
> 
> > very handy to us - we have a lot of spare CNL boards for our validation
> > purposes - CNL-based AudioDSP spans multiple platforms, e.g.:
> > CNL/CFL/WHL/CML. The number of newer boards is lower, unfortunately.
> 
> Well, I do see your point here and you are not asking for us to add gfx
> support back, but only help to have this protection here.
> 
> However I'm afraid that these entries in the list would only cause
> further confusion. Couldn't they get defined inside your .c directly as
> a const deny_list? so when we go there and remove the missing bits
> of CNL we don't conflict or cause undersired issues to you.

That makes sense.  Maybe drm people would get rid of the dead CNL*()
definitions from the header as a cleanup in near future, and we'll hit
a trouble.


thanks,

Takashi

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

[Cezary Rojewski]

On 2024-02-23 9:47 AM, Takashi Iwai wrote:
> On Thu, 22 Feb 2024 21:54:42 +0100,
> Rodrigo Vivi wrote:

...

>>>>> @@ -127,15 +128,26 @@ 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[] = {
>>>>> +		INTEL_CNL_IDS(NULL),
>>>>> +		INTEL_LKF_IDS(NULL),
>>>>> +		{ 0 }
>>>>> +	};
>>>>
>>>> I thought these don't actually exist in the wild?
>>>
>>> To my knowledge the opposite is true - while LKFs were shipped in limited
>>> number, they still were. I did ask few weeks ago my friends from Windows
>>> side about the support and they're still running full-scopes on HDMI
>>> endpoints on LKF platforms in their CIs. It seems the drm support is there
>>> though. Once you re-boot to linux we get -19 during probe().
>>>
>>> In regard to CNL, the commit removing CNL-support left the IDs intact what's
>>
>> I would prefer to go the other way around and remove the unused/unsupported
>> IDs entirely and for good.
>>
>>> very handy to us - we have a lot of spare CNL boards for our validation
>>> purposes - CNL-based AudioDSP spans multiple platforms, e.g.:
>>> CNL/CFL/WHL/CML. The number of newer boards is lower, unfortunately.
>>
>> Well, I do see your point here and you are not asking for us to add gfx
>> support back, but only help to have this protection here.
>>
>> However I'm afraid that these entries in the list would only cause
>> further confusion. Couldn't they get defined inside your .c directly as
>> a const deny_list? so when we go there and remove the missing bits
>> of CNL we don't conflict or cause undersired issues to you.
> 
> That makes sense.  Maybe drm people would get rid of the dead CNL*()
> definitions from the header as a cleanup in near future, and we'll hit
> a trouble.

Another, more robust solution could be to expose list of recognized 
devices by drivers/gpu/drm/i915/i915_pci.c in a header. Rather than 
guess, as we do in i915_gfx_present(), we would just loop over IDs in 
such list.

As I'm unsure if such solution is viable, what I'll do for now is: send 
v2 with relevant IDs moved over to sound/ tree, leaving the i915 header 
alone. Incremental update can be provided later if a neater solution 
appears on the horizon.


Kind regards,
Czarek