Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Ville Syrjälä]

On Mon, Oct 02, 2023 at 09:38:44PM +0200, maarten.lankhorst@linux.intel.com wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Now that we can use -EPROBE_DEFER, it's no longer required to spin off
> the snd_hdac_i915_init into a workqueue.
> 
> Use the -EPROBE_DEFER mechanism instead, which must be returned in the
> probe function.

This completely broke i915 audio!

I also can't see any trace of this stuff ever being posted to 
intel-gfx so it never went through the CI.

Please fix or revert ASAP.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  sound/pci/hda/hda_intel.c | 54 +++++++++++++++++++--------------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index e42ad0e816e1..9dad3607596a 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -2135,6 +2135,33 @@ static int azx_probe(struct pci_dev *pci,
>  
>  	pci_set_drvdata(pci, card);
>  
> +#ifdef CONFIG_SND_HDA_I915
> +	/* bind with i915 if needed */
> +	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
> +		err = snd_hdac_i915_init(azx_bus(chip), false);
> +		if (err < 0) {
> +			/* if the controller is bound only with HDMI/DP
> +			 * (for HSW and BDW), we need to abort the probe;
> +			 * for other chips, still continue probing as other
> +			 * codecs can be on the same link.
> +			 */
> +			if (HDA_CONTROLLER_IN_GPU(pci)) {
> +				goto out_free;
> +			} else {
> +				/* don't bother any longer */
> +				chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT;
> +			}
> +		}
> +
> +		/* HSW/BDW controllers need this power */
> +		if (HDA_CONTROLLER_IN_GPU(pci))
> +			hda->need_i915_power = true;
> +	}
> +#else
> +	if (HDA_CONTROLLER_IN_GPU(pci))
> +		dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n");
> +#endif
> +
>  	err = register_vga_switcheroo(chip);
>  	if (err < 0) {
>  		dev_err(card->dev, "Error registering vga_switcheroo client\n");
> @@ -2162,11 +2189,6 @@ static int azx_probe(struct pci_dev *pci,
>  	}
>  #endif /* CONFIG_SND_HDA_PATCH_LOADER */
>  
> -#ifndef CONFIG_SND_HDA_I915
> -	if (HDA_CONTROLLER_IN_GPU(pci))
> -		dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n");
> -#endif
> -
>  	if (schedule_probe)
>  		schedule_delayed_work(&hda->probe_work, 0);
>  
> @@ -2264,28 +2286,6 @@ static int azx_probe_continue(struct azx *chip)
>  	to_hda_bus(bus)->bus_probing = 1;
>  	hda->probe_continued = 1;
>  
> -	/* bind with i915 if needed */
> -	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
> -		err = snd_hdac_i915_init(bus, true);
> -		if (err < 0) {
> -			/* if the controller is bound only with HDMI/DP
> -			 * (for HSW and BDW), we need to abort the probe;
> -			 * for other chips, still continue probing as other
> -			 * codecs can be on the same link.
> -			 */
> -			if (HDA_CONTROLLER_IN_GPU(pci)) {
> -				goto out_free;
> -			} else {
> -				/* don't bother any longer */
> -				chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT;
> -			}
> -		}
> -
> -		/* HSW/BDW controllers need this power */
> -		if (HDA_CONTROLLER_IN_GPU(pci))
> -			hda->need_i915_power = true;
> -	}
> -
>  	/* Request display power well for the HDA controller or codec. For
>  	 * Haswell/Broadwell, both the display HDA controller and codec need
>  	 * this power. For other platforms, like Baytrail/Braswell, only the
> -- 
> 2.40.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Jani Nikula]

On Tue, 14 Nov 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Oct 02, 2023 at 09:38:44PM +0200, maarten.lankhorst@linux.intel.com wrote:
>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> 
>> Now that we can use -EPROBE_DEFER, it's no longer required to spin off
>> the snd_hdac_i915_init into a workqueue.
>> 
>> Use the -EPROBE_DEFER mechanism instead, which must be returned in the
>> probe function.
>
> This completely broke i915 audio!
>
> I also can't see any trace of this stuff ever being posted to 
> intel-gfx so it never went through the CI.
>
> Please fix or revert ASAP.

Cc: Jani, Suresh

Ville, please file a bug at gitlab so we can track this, thanks.

BR,
Jani.



>
>> 
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
>> ---
>>  sound/pci/hda/hda_intel.c | 54 +++++++++++++++++++--------------------
>>  1 file changed, 27 insertions(+), 27 deletions(-)
>> 
>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>> index e42ad0e816e1..9dad3607596a 100644
>> --- a/sound/pci/hda/hda_intel.c
>> +++ b/sound/pci/hda/hda_intel.c
>> @@ -2135,6 +2135,33 @@ static int azx_probe(struct pci_dev *pci,
>>  
>>  	pci_set_drvdata(pci, card);
>>  
>> +#ifdef CONFIG_SND_HDA_I915
>> +	/* bind with i915 if needed */
>> +	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
>> +		err = snd_hdac_i915_init(azx_bus(chip), false);
>> +		if (err < 0) {
>> +			/* if the controller is bound only with HDMI/DP
>> +			 * (for HSW and BDW), we need to abort the probe;
>> +			 * for other chips, still continue probing as other
>> +			 * codecs can be on the same link.
>> +			 */
>> +			if (HDA_CONTROLLER_IN_GPU(pci)) {
>> +				goto out_free;
>> +			} else {
>> +				/* don't bother any longer */
>> +				chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT;
>> +			}
>> +		}
>> +
>> +		/* HSW/BDW controllers need this power */
>> +		if (HDA_CONTROLLER_IN_GPU(pci))
>> +			hda->need_i915_power = true;
>> +	}
>> +#else
>> +	if (HDA_CONTROLLER_IN_GPU(pci))
>> +		dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n");
>> +#endif
>> +
>>  	err = register_vga_switcheroo(chip);
>>  	if (err < 0) {
>>  		dev_err(card->dev, "Error registering vga_switcheroo client\n");
>> @@ -2162,11 +2189,6 @@ static int azx_probe(struct pci_dev *pci,
>>  	}
>>  #endif /* CONFIG_SND_HDA_PATCH_LOADER */
>>  
>> -#ifndef CONFIG_SND_HDA_I915
>> -	if (HDA_CONTROLLER_IN_GPU(pci))
>> -		dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n");
>> -#endif
>> -
>>  	if (schedule_probe)
>>  		schedule_delayed_work(&hda->probe_work, 0);
>>  
>> @@ -2264,28 +2286,6 @@ static int azx_probe_continue(struct azx *chip)
>>  	to_hda_bus(bus)->bus_probing = 1;
>>  	hda->probe_continued = 1;
>>  
>> -	/* bind with i915 if needed */
>> -	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
>> -		err = snd_hdac_i915_init(bus, true);
>> -		if (err < 0) {
>> -			/* if the controller is bound only with HDMI/DP
>> -			 * (for HSW and BDW), we need to abort the probe;
>> -			 * for other chips, still continue probing as other
>> -			 * codecs can be on the same link.
>> -			 */
>> -			if (HDA_CONTROLLER_IN_GPU(pci)) {
>> -				goto out_free;
>> -			} else {
>> -				/* don't bother any longer */
>> -				chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT;
>> -			}
>> -		}
>> -
>> -		/* HSW/BDW controllers need this power */
>> -		if (HDA_CONTROLLER_IN_GPU(pci))
>> -			hda->need_i915_power = true;
>> -	}
>> -
>>  	/* Request display power well for the HDA controller or codec. For
>>  	 * Haswell/Broadwell, both the display HDA controller and codec need
>>  	 * this power. For other platforms, like Baytrail/Braswell, only the
>> -- 
>> 2.40.1

-- 
Jani Nikula, Intel

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Maarten Lankhorst]

Hey,

Den 2023-11-14 kl. 13:35, skrev Jani Nikula:
> On Tue, 14 Nov 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Mon, Oct 02, 2023 at 09:38:44PM +0200, maarten.lankhorst@linux.intel.com wrote:
>>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>
>>> Now that we can use -EPROBE_DEFER, it's no longer required to spin off
>>> the snd_hdac_i915_init into a workqueue.
>>>
>>> Use the -EPROBE_DEFER mechanism instead, which must be returned in the
>>> probe function.
>> This completely broke i915 audio!
>>
>> I also can't see any trace of this stuff ever being posted to
>> intel-gfx so it never went through the CI.
>>
>> Please fix or revert ASAP.
> Cc: Jani, Suresh
>
> Ville, please file a bug at gitlab so we can track this, thanks.

I've originally tested this on TGL and DG2, so can you be more specific 
on what broke?

Cheers,

~Maarten

>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>>> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
>>> ---
>>>   sound/pci/hda/hda_intel.c | 54 +++++++++++++++++++--------------------
>>>   1 file changed, 27 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>>> index e42ad0e816e1..9dad3607596a 100644
>>> --- a/sound/pci/hda/hda_intel.c
>>> +++ b/sound/pci/hda/hda_intel.c
>>> @@ -2135,6 +2135,33 @@ static int azx_probe(struct pci_dev *pci,
>>>   
>>>   	pci_set_drvdata(pci, card);
>>>   
>>> +#ifdef CONFIG_SND_HDA_I915
>>> +	/* bind with i915 if needed */
>>> +	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
>>> +		err = snd_hdac_i915_init(azx_bus(chip), false);
>>> +		if (err < 0) {
>>> +			/* if the controller is bound only with HDMI/DP
>>> +			 * (for HSW and BDW), we need to abort the probe;
>>> +			 * for other chips, still continue probing as other
>>> +			 * codecs can be on the same link.
>>> +			 */
>>> +			if (HDA_CONTROLLER_IN_GPU(pci)) {
>>> +				goto out_free;
>>> +			} else {
>>> +				/* don't bother any longer */
>>> +				chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT;
>>> +			}
>>> +		}
>>> +
>>> +		/* HSW/BDW controllers need this power */
>>> +		if (HDA_CONTROLLER_IN_GPU(pci))
>>> +			hda->need_i915_power = true;
>>> +	}
>>> +#else
>>> +	if (HDA_CONTROLLER_IN_GPU(pci))
>>> +		dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n");
>>> +#endif
>>> +
>>>   	err = register_vga_switcheroo(chip);
>>>   	if (err < 0) {
>>>   		dev_err(card->dev, "Error registering vga_switcheroo client\n");
>>> @@ -2162,11 +2189,6 @@ static int azx_probe(struct pci_dev *pci,
>>>   	}
>>>   #endif /* CONFIG_SND_HDA_PATCH_LOADER */
>>>   
>>> -#ifndef CONFIG_SND_HDA_I915
>>> -	if (HDA_CONTROLLER_IN_GPU(pci))
>>> -		dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n");
>>> -#endif
>>> -
>>>   	if (schedule_probe)
>>>   		schedule_delayed_work(&hda->probe_work, 0);
>>>   
>>> @@ -2264,28 +2286,6 @@ static int azx_probe_continue(struct azx *chip)
>>>   	to_hda_bus(bus)->bus_probing = 1;
>>>   	hda->probe_continued = 1;
>>>   
>>> -	/* bind with i915 if needed */
>>> -	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
>>> -		err = snd_hdac_i915_init(bus, true);
>>> -		if (err < 0) {
>>> -			/* if the controller is bound only with HDMI/DP
>>> -			 * (for HSW and BDW), we need to abort the probe;
>>> -			 * for other chips, still continue probing as other
>>> -			 * codecs can be on the same link.
>>> -			 */
>>> -			if (HDA_CONTROLLER_IN_GPU(pci)) {
>>> -				goto out_free;
>>> -			} else {
>>> -				/* don't bother any longer */
>>> -				chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT;
>>> -			}
>>> -		}
>>> -
>>> -		/* HSW/BDW controllers need this power */
>>> -		if (HDA_CONTROLLER_IN_GPU(pci))
>>> -			hda->need_i915_power = true;
>>> -	}
>>> -
>>>   	/* Request display power well for the HDA controller or codec. For
>>>   	 * Haswell/Broadwell, both the display HDA controller and codec need
>>>   	 * this power. For other platforms, like Baytrail/Braswell, only the
>>> -- 
>>> 2.40.1

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Saarinen, Jani]

Hi, 
> -----Original Message-----
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Sent: Tuesday, November 14, 2023 3:30 PM
> To: Jani Nikula <jani.nikula@linux.intel.com>; Ville Syrjälä
> <ville.syrjala@linux.intel.com>
> Cc: Takashi Iwai <tiwai@suse.de>; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org; alsa-devel@alsa-project.org; Saarinen, Jani
> <jani.saarinen@intel.com>; Kurmi, Suresh Kumar
> <suresh.kumar.kurmi@intel.com>
> Subject: Re: [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init
> to before probe_work.
> 
> Hey,
> 
> Den 2023-11-14 kl. 13:35, skrev Jani Nikula:
> > On Tue, 14 Nov 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> On Mon, Oct 02, 2023 at 09:38:44PM +0200,
> maarten.lankhorst@linux.intel.com wrote:
> >>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>
> >>> Now that we can use -EPROBE_DEFER, it's no longer required to spin
> >>> off the snd_hdac_i915_init into a workqueue.
> >>>
> >>> Use the -EPROBE_DEFER mechanism instead, which must be returned in
> >>> the probe function.
> >> This completely broke i915 audio!
> >>
> >> I also can't see any trace of this stuff ever being posted to
> >> intel-gfx so it never went through the CI.
> >>
> >> Please fix or revert ASAP.
> > Cc: Jani, Suresh
> >
> > Ville, please file a bug at gitlab so we can track this, thanks.
> 
> I've originally tested this on TGL and DG2, so can you be more specific on what
> broke?
Was this series tested on CI  ever as Ville saying no? How this got merged? 

> 
> Cheers,
> 
> ~Maarten
> 
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> >>> Reviewed-by: Pierre-Louis Bossart
> >>> <pierre-louis.bossart@linux.intel.com>
> >>> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> >>> ---
> >>>   sound/pci/hda/hda_intel.c | 54 +++++++++++++++++++--------------------
> >>>   1 file changed, 27 insertions(+), 27 deletions(-)
> >>>
> >>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> >>> index e42ad0e816e1..9dad3607596a 100644
> >>> --- a/sound/pci/hda/hda_intel.c
> >>> +++ b/sound/pci/hda/hda_intel.c
> >>> @@ -2135,6 +2135,33 @@ static int azx_probe(struct pci_dev *pci,
> >>>
> >>>   	pci_set_drvdata(pci, card);
> >>>
> >>> +#ifdef CONFIG_SND_HDA_I915
> >>> +	/* bind with i915 if needed */
> >>> +	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
> >>> +		err = snd_hdac_i915_init(azx_bus(chip), false);
> >>> +		if (err < 0) {
> >>> +			/* if the controller is bound only with HDMI/DP
> >>> +			 * (for HSW and BDW), we need to abort the probe;
> >>> +			 * for other chips, still continue probing as other
> >>> +			 * codecs can be on the same link.
> >>> +			 */
> >>> +			if (HDA_CONTROLLER_IN_GPU(pci)) {
> >>> +				goto out_free;
> >>> +			} else {
> >>> +				/* don't bother any longer */
> >>> +				chip->driver_caps &=
> ~AZX_DCAPS_I915_COMPONENT;
> >>> +			}
> >>> +		}
> >>> +
> >>> +		/* HSW/BDW controllers need this power */
> >>> +		if (HDA_CONTROLLER_IN_GPU(pci))
> >>> +			hda->need_i915_power = true;
> >>> +	}
> >>> +#else
> >>> +	if (HDA_CONTROLLER_IN_GPU(pci))
> >>> +		dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in
> >>> +CONFIG_SND_HDA_I915\n"); #endif
> >>> +
> >>>   	err = register_vga_switcheroo(chip);
> >>>   	if (err < 0) {
> >>>   		dev_err(card->dev, "Error registering vga_switcheroo client\n");
> >>> @@ -2162,11 +2189,6 @@ static int azx_probe(struct pci_dev *pci,
> >>>   	}
> >>>   #endif /* CONFIG_SND_HDA_PATCH_LOADER */
> >>>
> >>> -#ifndef CONFIG_SND_HDA_I915
> >>> -	if (HDA_CONTROLLER_IN_GPU(pci))
> >>> -		dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in
> CONFIG_SND_HDA_I915\n");
> >>> -#endif
> >>> -
> >>>   	if (schedule_probe)
> >>>   		schedule_delayed_work(&hda->probe_work, 0);
> >>>
> >>> @@ -2264,28 +2286,6 @@ static int azx_probe_continue(struct azx *chip)
> >>>   	to_hda_bus(bus)->bus_probing = 1;
> >>>   	hda->probe_continued = 1;
> >>>
> >>> -	/* bind with i915 if needed */
> >>> -	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
> >>> -		err = snd_hdac_i915_init(bus, true);
> >>> -		if (err < 0) {
> >>> -			/* if the controller is bound only with HDMI/DP
> >>> -			 * (for HSW and BDW), we need to abort the probe;
> >>> -			 * for other chips, still continue probing as other
> >>> -			 * codecs can be on the same link.
> >>> -			 */
> >>> -			if (HDA_CONTROLLER_IN_GPU(pci)) {
> >>> -				goto out_free;
> >>> -			} else {
> >>> -				/* don't bother any longer */
> >>> -				chip->driver_caps &=
> ~AZX_DCAPS_I915_COMPONENT;
> >>> -			}
> >>> -		}
> >>> -
> >>> -		/* HSW/BDW controllers need this power */
> >>> -		if (HDA_CONTROLLER_IN_GPU(pci))
> >>> -			hda->need_i915_power = true;
> >>> -	}
> >>> -
> >>>   	/* Request display power well for the HDA controller or codec. For
> >>>   	 * Haswell/Broadwell, both the display HDA controller and codec need
> >>>   	 * this power. For other platforms, like Baytrail/Braswell, only
> >>> the
> >>> --
> >>> 2.40.1

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Ville Syrjälä]

On Tue, Nov 14, 2023 at 02:35:10PM +0200, Jani Nikula wrote:
> On Tue, 14 Nov 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Oct 02, 2023 at 09:38:44PM +0200, maarten.lankhorst@linux.intel.com wrote:
> >> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> 
> >> Now that we can use -EPROBE_DEFER, it's no longer required to spin off
> >> the snd_hdac_i915_init into a workqueue.
> >> 
> >> Use the -EPROBE_DEFER mechanism instead, which must be returned in the
> >> probe function.
> >
> > This completely broke i915 audio!
> >
> > I also can't see any trace of this stuff ever being posted to 
> > intel-gfx so it never went through the CI.
> >
> > Please fix or revert ASAP.
> 
> Cc: Jani, Suresh
> 
> Ville, please file a bug at gitlab so we can track this, thanks.

https://gitlab.freedesktop.org/drm/intel/-/issues/9671

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Maarten Lankhorst]

[-- Attachment #1: Type: text/plain, Size: 1509 bytes --]

Hey,

Den 2023-11-14 kl. 14:39, skrev Ville Syrjälä:
> On Tue, Nov 14, 2023 at 02:35:10PM +0200, Jani Nikula wrote:
>> On Tue, 14 Nov 2023, Ville Syrjälä<ville.syrjala@linux.intel.com>  wrote:
>>> On Mon, Oct 02, 2023 at 09:38:44PM +0200,maarten.lankhorst@linux.intel.com  wrote:
>>>> From: Maarten Lankhorst<maarten.lankhorst@linux.intel.com>
>>>>
>>>> Now that we can use -EPROBE_DEFER, it's no longer required to spin off
>>>> the snd_hdac_i915_init into a workqueue.
>>>>
>>>> Use the -EPROBE_DEFER mechanism instead, which must be returned in the
>>>> probe function.
>>> This completely broke i915 audio!
>>>
>>> I also can't see any trace of this stuff ever being posted to
>>> intel-gfx so it never went through the CI.
>>>
>>> Please fix or revert ASAP.
>> Cc: Jani, Suresh
>>
>> Ville, please file a bug at gitlab so we can track this, thanks.
> https://gitlab.freedesktop.org/drm/intel/-/issues/9671

Looks like a simple patch should be enough, can you test below?

----

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 058f6e6491f9a..946aaa487f200 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2145,7 +2145,8 @@ static int azx_probe(struct pci_dev *pci,
  			 * for other chips, still continue probing as other
  			 * codecs can be on the same link.
  			 */
-			if (HDA_CONTROLLER_IN_GPU(pci)) {
+			if (HDA_CONTROLLER_IN_GPU(pci) ||
+			    err == -EPROBE_DEFER) {
  				goto out_free;
  			} else {
  				/* don't bother any longer */

[-- Attachment #2: Type: text/html, Size: 2661 bytes --]

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Takashi Iwai]

On Tue, 14 Nov 2023 12:06:45 +0100,
Ville Syrjälä wrote:
> 
> On Mon, Oct 02, 2023 at 09:38:44PM +0200, maarten.lankhorst@linux.intel.com wrote:
> > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > 
> > Now that we can use -EPROBE_DEFER, it's no longer required to spin off
> > the snd_hdac_i915_init into a workqueue.
> > 
> > Use the -EPROBE_DEFER mechanism instead, which must be returned in the
> > probe function.
> 
> This completely broke i915 audio!
> 
> I also can't see any trace of this stuff ever being posted to 
> intel-gfx so it never went through the CI.
> 
> Please fix or revert ASAP.

It's hard to judge without any log nor h/w details (as a typical bug
report from a developer :)

As a blind shot, does the patch below fix your case?


thanks,

Takashi

-- 8< --
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2141,6 +2141,9 @@ static int azx_probe(struct pci_dev *pci,
 	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
 		err = snd_hdac_i915_init(azx_bus(chip));
 		if (err < 0) {
+			if (err == -EPROBE_DEFER)
+				goto out_free;
+
 			/* if the controller is bound only with HDMI/DP
 			 * (for HSW and BDW), we need to abort the probe;
 			 * for other chips, still continue probing as other

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Takashi Iwai]

On Tue, 14 Nov 2023 15:39:16 +0100,
Maarten Lankhorst wrote:
> 
> 
> Hey,
> 
> Den 2023-11-14 kl. 14:39, skrev Ville Syrjälä:
> 
>     On Tue, Nov 14, 2023 at 02:35:10PM +0200, Jani Nikula wrote:
>     
>         On Tue, 14 Nov 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>         
>             On Mon, Oct 02, 2023 at 09:38:44PM +0200, maarten.lankhorst@linux.intel.com wrote:
>             
>                 From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>                 
>                 Now that we can use -EPROBE_DEFER, it's no longer required to spin off
>                 the snd_hdac_i915_init into a workqueue.
>                 
>                 Use the -EPROBE_DEFER mechanism instead, which must be returned in the
>                 probe function.
>                 
>             This completely broke i915 audio!
>             
>             I also can't see any trace of this stuff ever being posted to 
>             intel-gfx so it never went through the CI.
>             
>             Please fix or revert ASAP.
>             
>         Cc: Jani, Suresh
>         
>         Ville, please file a bug at gitlab so we can track this, thanks.
>         
>     https://gitlab.freedesktop.org/drm/intel/-/issues/9671
>     
> Looks like a simple patch should be enough, can you test below?

Seems that we reached to the same conclusion :)

I took a quick look at other code paths, and sound/pci/hda/hda_intel.c
is the only place that needs the correction.  Other (ASoC) drivers are
either simply always returning the error or dealing only with -ENODEV
case for skipping the HDMI codec.


thanks,

Takashi

> 
> ----
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 058f6e6491f9a..946aaa487f200 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -2145,7 +2145,8 @@ static int azx_probe(struct pci_dev *pci,
>                                                                    * for other chips, still continue probing as other
>                                                                    * codecs can be on the same link.
>                                                                    */
> -                                                                 if (HDA_CONTROLLER_IN_GPU(pci)) {
> +                                                                 if (HDA_CONTROLLER_IN_GPU(pci) ||
> +                                                                     err == -EPROBE_DEFER) {
>                                                                                         goto out_free;
>                                                                   } else {
>                                                                                         /* don't bother any longer */
> 

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Maarten Lankhorst]

Hey,

Den 2023-11-14 kl. 16:50, skrev Takashi Iwai:
> On Tue, 14 Nov 2023 15:39:16 +0100,
> Maarten Lankhorst wrote:
>>
>> Hey,
>>
>> Den 2023-11-14 kl. 14:39, skrev Ville Syrjälä:
>>
>>      On Tue, Nov 14, 2023 at 02:35:10PM +0200, Jani Nikula wrote:
>>      
>>          On Tue, 14 Nov 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>>          
>>              On Mon, Oct 02, 2023 at 09:38:44PM +0200, maarten.lankhorst@linux.intel.com wrote:
>>              
>>                  From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>                  
>>                  Now that we can use -EPROBE_DEFER, it's no longer required to spin off
>>                  the snd_hdac_i915_init into a workqueue.
>>                  
>>                  Use the -EPROBE_DEFER mechanism instead, which must be returned in the
>>                  probe function.
>>                  
>>              This completely broke i915 audio!
>>              
>>              I also can't see any trace of this stuff ever being posted to
>>              intel-gfx so it never went through the CI.
>>              
>>              Please fix or revert ASAP.
>>              
>>          Cc: Jani, Suresh
>>          
>>          Ville, please file a bug at gitlab so we can track this, thanks.
>>          
>>      https://gitlab.freedesktop.org/drm/intel/-/issues/9671
>>      
>> Looks like a simple patch should be enough, can you test below?
> Seems that we reached to the same conclusion :)
>
> I took a quick look at other code paths, and sound/pci/hda/hda_intel.c
> is the only place that needs the correction.  Other (ASoC) drivers are
> either simply always returning the error or dealing only with -ENODEV
> case for skipping the HDMI codec.

Yeah, the original comment in the code confused me and led me to not 
handle -EPROBE_DEFER in the first place there.

Cheers,

Maarten

> thanks,
>
> Takashi
>
>> ----
>>
>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>> index 058f6e6491f9a..946aaa487f200 100644
>> --- a/sound/pci/hda/hda_intel.c
>> +++ b/sound/pci/hda/hda_intel.c
>> @@ -2145,7 +2145,8 @@ static int azx_probe(struct pci_dev *pci,
>>                                                                     * for other chips, still continue probing as other
>>                                                                     * codecs can be on the same link.
>>                                                                     */
>> -                                                                 if (HDA_CONTROLLER_IN_GPU(pci)) {
>> +                                                                 if (HDA_CONTROLLER_IN_GPU(pci) ||
>> +                                                                     err == -EPROBE_DEFER) {
>>                                                                                          goto out_free;
>>                                                                    } else {
>>                                                                                          /* don't bother any longer */
>>

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Takashi Iwai]

On Tue, 14 Nov 2023 14:31:25 +0100,
Saarinen, Jani wrote:
> 
> Hi, 
> > -----Original Message-----
> > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Sent: Tuesday, November 14, 2023 3:30 PM
> > To: Jani Nikula <jani.nikula@linux.intel.com>; Ville Syrjälä
> > <ville.syrjala@linux.intel.com>
> > Cc: Takashi Iwai <tiwai@suse.de>; intel-gfx@lists.freedesktop.org; intel-
> > xe@lists.freedesktop.org; alsa-devel@alsa-project.org; Saarinen, Jani
> > <jani.saarinen@intel.com>; Kurmi, Suresh Kumar
> > <suresh.kumar.kurmi@intel.com>
> > Subject: Re: [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init
> > to before probe_work.
> > 
> > Hey,
> > 
> > Den 2023-11-14 kl. 13:35, skrev Jani Nikula:
> > > On Tue, 14 Nov 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > >> On Mon, Oct 02, 2023 at 09:38:44PM +0200,
> > maarten.lankhorst@linux.intel.com wrote:
> > >>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >>>
> > >>> Now that we can use -EPROBE_DEFER, it's no longer required to spin
> > >>> off the snd_hdac_i915_init into a workqueue.
> > >>>
> > >>> Use the -EPROBE_DEFER mechanism instead, which must be returned in
> > >>> the probe function.
> > >> This completely broke i915 audio!
> > >>
> > >> I also can't see any trace of this stuff ever being posted to
> > >> intel-gfx so it never went through the CI.
> > >>
> > >> Please fix or revert ASAP.
> > > Cc: Jani, Suresh
> > >
> > > Ville, please file a bug at gitlab so we can track this, thanks.
> > 
> > I've originally tested this on TGL and DG2, so can you be more specific on what
> > broke?
> Was this series tested on CI  ever as Ville saying no? How this got merged? 

Hm, I somehow believed that patches have been tested by intel people,
as they came from intel.  (Also thought that drm-tip or whatever tree
also drags the recent changes from sound.git tree for relevant part.)

In anyway, the whole patches were touching only sound/* files and no
changes in DRM side at all, hence they weren't sent to intel-gfx, I
suppose.


Takashi

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Jani Nikula]

On Tue, 14 Nov 2023, Takashi Iwai <tiwai@suse.de> wrote:
> (Also thought that drm-tip or whatever tree also drags the recent
> changes from sound.git tree for relevant part.)

We've dropped the sound branches from drm-tip, mostly because they were
repeatedly rebased on Linus' master (as opposed to tags) during the
merge window.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [Intel-gfx] [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

[Kai Vehmanen]

Hi,

On Tue, 14 Nov 2023, Takashi Iwai wrote:

> On Tue, 14 Nov 2023 14:31:25 +0100, Saarinen, Jani wrote:
> > Was this series tested on CI  ever as Ville saying no? How this got merged? 
> 
> Hm, I somehow believed that patches have been tested by intel people,
> as they came from intel.  (Also thought that drm-tip or whatever tree
> also drags the recent changes from sound.git tree for relevant part.)

ack, this is squarely on us Intel folks to ensure the test runs get done. 
This series did go through a number of Intel test setups, but 
unfortunately, as it now seems, not through the i915 CI setup that covers 
the affected configuration (audio DSP not used, HDA controller on the PCH, 
tests on the display audio codec).

Reproduced the problem now one one local setup and can confirm the fix 
Takashi and Maarten posted, does work.
 
Br, Kai