[v6] sound: Use -EPROBE_DEFER instead of i915 module loading.

[PATCH v6 11/12] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

Posted by Maarten Lankhorst 2 years, 3 months ago

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.

The previously added probe_early can be used for this,
and we also use the newly added remove_late for unbinding afterwards.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/hda-common-ops.c |  1 +
 sound/soc/sof/intel/hda.c            | 18 ++++++------------
 sound/soc/sof/intel/hda.h            |  1 +
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/sound/soc/sof/intel/hda-common-ops.c b/sound/soc/sof/intel/hda-common-ops.c
index 1cc18fb2b75b..26105d8f1bdc 100644
--- a/sound/soc/sof/intel/hda-common-ops.c
+++ b/sound/soc/sof/intel/hda-common-ops.c
@@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = {
 	.probe_early	= hda_dsp_probe_early,
 	.probe		= hda_dsp_probe,
 	.remove		= hda_dsp_remove,
+	.remove_late	= hda_dsp_remove_late,
 
 	/* Register IO uses direct mmio */
 
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index 86a2571488bc..4eb7f04b8ae1 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev)
 		return -ENOMEM;
 	sdev->pdata->hw_pdata = hdev;
 	hdev->desc = chip;
+	ret = hda_init(sdev);
 
 err:
 	return ret;
@@ -1169,7 +1170,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
 {
 	struct pci_dev *pci = to_pci_dev(sdev->dev);
 	struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata;
-	struct hdac_bus *bus;
 	int ret = 0;
 
 	hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec",
@@ -1193,12 +1193,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
 	if (sdev->dspless_mode_selected)
 		hdev->no_ipc_position = 1;
 
-	/* set up HDA base */
-	bus = sof_to_bus(sdev);
-	ret = hda_init(sdev);
-	if (ret < 0)
-		goto hdac_bus_unmap;
-
 	if (sdev->dspless_mode_selected)
 		goto skip_dsp_setup;
 
@@ -1307,8 +1301,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
 		iounmap(sdev->bar[HDA_DSP_BAR]);
 hdac_bus_unmap:
 	platform_device_unregister(hdev->dmic_dev);
-	iounmap(bus->remap_addr);
-	hda_codec_i915_exit(sdev);
 
 	return ret;
 }
@@ -1317,7 +1309,6 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
 {
 	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
 	const struct sof_intel_dsp_desc *chip = hda->desc;
-	struct hdac_bus *bus = sof_to_bus(sdev);
 	struct pci_dev *pci = to_pci_dev(sdev->dev);
 	struct nhlt_acpi_table *nhlt = hda->nhlt;
 
@@ -1368,10 +1359,13 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
 	if (!sdev->dspless_mode_selected)
 		iounmap(sdev->bar[HDA_DSP_BAR]);
 
-	iounmap(bus->remap_addr);
+	return 0;
+}
 
+int hda_dsp_remove_late(struct snd_sof_dev *sdev)
+{
+	iounmap(sof_to_bus(sdev)->remap_addr);
 	sof_hda_bus_exit(sdev);
-
 	hda_codec_i915_exit(sdev);
 
 	return 0;
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index e13cdc933ca6..8e846684279e 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -576,6 +576,7 @@ struct sof_intel_hda_stream {
 int hda_dsp_probe_early(struct snd_sof_dev *sdev);
 int hda_dsp_probe(struct snd_sof_dev *sdev);
 int hda_dsp_remove(struct snd_sof_dev *sdev);
+int hda_dsp_remove_late(struct snd_sof_dev *sdev);
 int hda_dsp_core_power_up(struct snd_sof_dev *sdev, unsigned int core_mask);
 int hda_dsp_core_run(struct snd_sof_dev *sdev, unsigned int core_mask);
 int hda_dsp_enable_core(struct snd_sof_dev *sdev, unsigned int core_mask);
-- 
2.40.1

Re: [PATCH v6 11/12] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

Posted by Kai Vehmanen 2 years, 3 months ago

Hi,

I'm good with rest of the series, but one patch requires work.

On Wed, 4 Oct 2023, Maarten Lankhorst wrote:

> 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.
> 
> The previously added probe_early can be used for this,
> and we also use the newly added remove_late for unbinding afterwards.
[...]
> --- a/sound/soc/sof/intel/hda-common-ops.c
> +++ b/sound/soc/sof/intel/hda-common-ops.c
> @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = {
>  	.probe_early	= hda_dsp_probe_early,
>  	.probe		= hda_dsp_probe,
>  	.remove		= hda_dsp_remove,
> +	.remove_late	= hda_dsp_remove_late,
>  
>  	/* Register IO uses direct mmio */
>  
> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
> index 86a2571488bc..4eb7f04b8ae1 100644
> --- a/sound/soc/sof/intel/hda.c
> +++ b/sound/soc/sof/intel/hda.c
> @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev)
>  		return -ENOMEM;
>  	sdev->pdata->hw_pdata = hdev;
>  	hdev->desc = chip;
> +	ret = hda_init(sdev);
>  
>  err:
>  	return ret;

I don't think this works. The hda_codec_i915_init() errors are ignored in 
hda_init() so this never returns -EPROBE_DEFER.

So something like this is needed on top (tested quickly on one SOF 
machine and this blocks SOF load until i915 or xe driver is loaded):

--cut--
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index 9025bfaf6a7e..8b17c82dcc89 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -863,13 +863,20 @@ static int hda_init(struct snd_sof_dev *sdev)
        /* init i915 and HDMI codecs */
        ret = hda_codec_i915_init(sdev);
        if (ret < 0)
-               dev_warn(sdev->dev, "init of i915 and HDMI codec 
failed\n");
+               dev_warn(sdev->dev, "init of i915 and HDMI codec failed 
(%d)\n", ret);
+
+       if (ret < 0 && ret != -ENODEV)
+               goto out;
 
        /* get controller capabilities */
        ret = hda_dsp_ctrl_get_caps(sdev);
        if (ret < 0)
                dev_err(sdev->dev, "error: get caps error\n");
 
+out:
+       if (ret < 0)
+               iounmap(sof_to_bus(sdev)->remap_addr);
+
        return ret;
 }
--cut--

Br, Kai

Re: [PATCH v6 11/12] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

Posted by Péter Ujfalusi 2 years, 2 months ago


On 04/10/2023 19:59, Kai Vehmanen wrote:
> Hi,
> 
> I'm good with rest of the series, but one patch requires work.
> 
> On Wed, 4 Oct 2023, Maarten Lankhorst wrote:
> 
>> 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.
>>
>> The previously added probe_early can be used for this,
>> and we also use the newly added remove_late for unbinding afterwards.
> [...]
>> --- a/sound/soc/sof/intel/hda-common-ops.c
>> +++ b/sound/soc/sof/intel/hda-common-ops.c
>> @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = {
>>  	.probe_early	= hda_dsp_probe_early,
>>  	.probe		= hda_dsp_probe,
>>  	.remove		= hda_dsp_remove,
>> +	.remove_late	= hda_dsp_remove_late,
>>  
>>  	/* Register IO uses direct mmio */
>>  
>> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
>> index 86a2571488bc..4eb7f04b8ae1 100644
>> --- a/sound/soc/sof/intel/hda.c
>> +++ b/sound/soc/sof/intel/hda.c
>> @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev)
>>  		return -ENOMEM;
>>  	sdev->pdata->hw_pdata = hdev;
>>  	hdev->desc = chip;
>> +	ret = hda_init(sdev);
>>  
>>  err:
>>  	return ret;
> 
> I don't think this works. The hda_codec_i915_init() errors are ignored in 
> hda_init() so this never returns -EPROBE_DEFER.
> 
> So something like this is needed on top (tested quickly on one SOF 
> machine and this blocks SOF load until i915 or xe driver is loaded):
> 
> --cut--
> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
> index 9025bfaf6a7e..8b17c82dcc89 100644
> --- a/sound/soc/sof/intel/hda.c
> +++ b/sound/soc/sof/intel/hda.c
> @@ -863,13 +863,20 @@ static int hda_init(struct snd_sof_dev *sdev)
>         /* init i915 and HDMI codecs */
>         ret = hda_codec_i915_init(sdev);
>         if (ret < 0)
> -               dev_warn(sdev->dev, "init of i915 and HDMI codec 
> failed\n");
> +               dev_warn(sdev->dev, "init of i915 and HDMI codec failed 
> (%d)\n", ret);

we should not print anything or maximum dev_dbg in case of EPROBE_DEFER.

> +
> +       if (ret < 0 && ret != -ENODEV)
> +               goto out;
>  
>         /* get controller capabilities */
>         ret = hda_dsp_ctrl_get_caps(sdev);
>         if (ret < 0)
>                 dev_err(sdev->dev, "error: get caps error\n");
>  
> +out:
> +       if (ret < 0)
> +               iounmap(sof_to_bus(sdev)->remap_addr);
> +
>         return ret;
>  }
> --cut--
> 
> Br, Kai

-- 
Péter

Re: [PATCH v6 11/12] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

Posted by Maarten Lankhorst 2 years, 2 months ago


On 2023-10-05 12:58, Péter Ujfalusi wrote:
> 
> 
> On 04/10/2023 19:59, Kai Vehmanen wrote:
>> Hi,
>>
>> I'm good with rest of the series, but one patch requires work.
>>
>> On Wed, 4 Oct 2023, Maarten Lankhorst wrote:
>>
>>> 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.
>>>
>>> The previously added probe_early can be used for this,
>>> and we also use the newly added remove_late for unbinding afterwards.
>> [...]
>>> --- a/sound/soc/sof/intel/hda-common-ops.c
>>> +++ b/sound/soc/sof/intel/hda-common-ops.c
>>> @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = {
>>>   	.probe_early	= hda_dsp_probe_early,
>>>   	.probe		= hda_dsp_probe,
>>>   	.remove		= hda_dsp_remove,
>>> +	.remove_late	= hda_dsp_remove_late,
>>>   
>>>   	/* Register IO uses direct mmio */
>>>   
>>> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
>>> index 86a2571488bc..4eb7f04b8ae1 100644
>>> --- a/sound/soc/sof/intel/hda.c
>>> +++ b/sound/soc/sof/intel/hda.c
>>> @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev)
>>>   		return -ENOMEM;
>>>   	sdev->pdata->hw_pdata = hdev;
>>>   	hdev->desc = chip;
>>> +	ret = hda_init(sdev);
>>>   
>>>   err:
>>>   	return ret;
>>
>> I don't think this works. The hda_codec_i915_init() errors are ignored in
>> hda_init() so this never returns -EPROBE_DEFER.
>>
>> So something like this is needed on top (tested quickly on one SOF
>> machine and this blocks SOF load until i915 or xe driver is loaded):
>>
>> --cut--
>> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
>> index 9025bfaf6a7e..8b17c82dcc89 100644
>> --- a/sound/soc/sof/intel/hda.c
>> +++ b/sound/soc/sof/intel/hda.c
>> @@ -863,13 +863,20 @@ static int hda_init(struct snd_sof_dev *sdev)
>>          /* init i915 and HDMI codecs */
>>          ret = hda_codec_i915_init(sdev);
>>          if (ret < 0)
>> -               dev_warn(sdev->dev, "init of i915 and HDMI codec
>> failed\n");
>> +               dev_warn(sdev->dev, "init of i915 and HDMI codec failed
>> (%d)\n", ret);
> 
> we should not print anything or maximum dev_dbg in case of EPROBE_DEFER.
There's dev_err_probe, which is dev_err on error, or sets the reason for 
deferred probe to the arguments if the error is -EPROBE_DEFER.

~Maarten

Re: [PATCH v6 11/12] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

Posted by Takashi Iwai 2 years, 2 months ago

On Thu, 05 Oct 2023 13:26:18 +0200,
Maarten Lankhorst wrote:
> 
> 
> 
> On 2023-10-05 12:58, Péter Ujfalusi wrote:
> > 
> > 
> > On 04/10/2023 19:59, Kai Vehmanen wrote:
> >> Hi,
> >> 
> >> I'm good with rest of the series, but one patch requires work.
> >> 
> >> On Wed, 4 Oct 2023, Maarten Lankhorst wrote:
> >> 
> >>> 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.
> >>> 
> >>> The previously added probe_early can be used for this,
> >>> and we also use the newly added remove_late for unbinding afterwards.
> >> [...]
> >>> --- a/sound/soc/sof/intel/hda-common-ops.c
> >>> +++ b/sound/soc/sof/intel/hda-common-ops.c
> >>> @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = {
> >>>   	.probe_early	= hda_dsp_probe_early,
> >>>   	.probe		= hda_dsp_probe,
> >>>   	.remove		= hda_dsp_remove,
> >>> +	.remove_late	= hda_dsp_remove_late,
> >>>     	/* Register IO uses direct mmio */
> >>>   diff --git a/sound/soc/sof/intel/hda.c
> >>> b/sound/soc/sof/intel/hda.c
> >>> index 86a2571488bc..4eb7f04b8ae1 100644
> >>> --- a/sound/soc/sof/intel/hda.c
> >>> +++ b/sound/soc/sof/intel/hda.c
> >>> @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev)
> >>>   		return -ENOMEM;
> >>>   	sdev->pdata->hw_pdata = hdev;
> >>>   	hdev->desc = chip;
> >>> +	ret = hda_init(sdev);
> >>>     err:
> >>>   	return ret;
> >> 
> >> I don't think this works. The hda_codec_i915_init() errors are ignored in
> >> hda_init() so this never returns -EPROBE_DEFER.
> >> 
> >> So something like this is needed on top (tested quickly on one SOF
> >> machine and this blocks SOF load until i915 or xe driver is loaded):
> >> 
> >> --cut--
> >> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
> >> index 9025bfaf6a7e..8b17c82dcc89 100644
> >> --- a/sound/soc/sof/intel/hda.c
> >> +++ b/sound/soc/sof/intel/hda.c
> >> @@ -863,13 +863,20 @@ static int hda_init(struct snd_sof_dev *sdev)
> >>          /* init i915 and HDMI codecs */
> >>          ret = hda_codec_i915_init(sdev);
> >>          if (ret < 0)
> >> -               dev_warn(sdev->dev, "init of i915 and HDMI codec
> >> failed\n");
> >> +               dev_warn(sdev->dev, "init of i915 and HDMI codec failed
> >> (%d)\n", ret);
> > 
> > we should not print anything or maximum dev_dbg in case of EPROBE_DEFER.
> There's dev_err_probe, which is dev_err on error, or sets the reason
> for deferred probe to the arguments if the error is -EPROBE_DEFER.

I expect you'll respin v7 for addressing this?

I'd love to merge the series for 6.7, and the time ticks...


thanks,

Takashi

Re: [PATCH v6 11/12] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

Posted by Maarten Lankhorst 2 years, 2 months ago

Hey,

On 2023-10-09 08:23, Takashi Iwai wrote:
> On Thu, 05 Oct 2023 13:26:18 +0200,
> Maarten Lankhorst wrote:
>>
>>
>>
>> On 2023-10-05 12:58, Péter Ujfalusi wrote:
>>>
>>>
>>> On 04/10/2023 19:59, Kai Vehmanen wrote:
>>>> Hi,
>>>>
>>>> I'm good with rest of the series, but one patch requires work.
>>>>
>>>> On Wed, 4 Oct 2023, Maarten Lankhorst wrote:
>>>>
>>>>> 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.
>>>>>
>>>>> The previously added probe_early can be used for this,
>>>>> and we also use the newly added remove_late for unbinding afterwards.
>>>> [...]
>>>>> --- a/sound/soc/sof/intel/hda-common-ops.c
>>>>> +++ b/sound/soc/sof/intel/hda-common-ops.c
>>>>> @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = {
>>>>>    	.probe_early	= hda_dsp_probe_early,
>>>>>    	.probe		= hda_dsp_probe,
>>>>>    	.remove		= hda_dsp_remove,
>>>>> +	.remove_late	= hda_dsp_remove_late,
>>>>>      	/* Register IO uses direct mmio */
>>>>>    diff --git a/sound/soc/sof/intel/hda.c
>>>>> b/sound/soc/sof/intel/hda.c
>>>>> index 86a2571488bc..4eb7f04b8ae1 100644
>>>>> --- a/sound/soc/sof/intel/hda.c
>>>>> +++ b/sound/soc/sof/intel/hda.c
>>>>> @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev)
>>>>>    		return -ENOMEM;
>>>>>    	sdev->pdata->hw_pdata = hdev;
>>>>>    	hdev->desc = chip;
>>>>> +	ret = hda_init(sdev);
>>>>>      err:
>>>>>    	return ret;
>>>>
>>>> I don't think this works. The hda_codec_i915_init() errors are ignored in
>>>> hda_init() so this never returns -EPROBE_DEFER.
>>>>
>>>> So something like this is needed on top (tested quickly on one SOF
>>>> machine and this blocks SOF load until i915 or xe driver is loaded):
>>>>
>>>> --cut--
>>>> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
>>>> index 9025bfaf6a7e..8b17c82dcc89 100644
>>>> --- a/sound/soc/sof/intel/hda.c
>>>> +++ b/sound/soc/sof/intel/hda.c
>>>> @@ -863,13 +863,20 @@ static int hda_init(struct snd_sof_dev *sdev)
>>>>           /* init i915 and HDMI codecs */
>>>>           ret = hda_codec_i915_init(sdev);
>>>>           if (ret < 0)
>>>> -               dev_warn(sdev->dev, "init of i915 and HDMI codec
>>>> failed\n");
>>>> +               dev_warn(sdev->dev, "init of i915 and HDMI codec failed
>>>> (%d)\n", ret);
>>>
>>> we should not print anything or maximum dev_dbg in case of EPROBE_DEFER.
>> There's dev_err_probe, which is dev_err on error, or sets the reason
>> for deferred probe to the arguments if the error is -EPROBE_DEFER.
> 
> I expect you'll respin v7 for addressing this?
> 
> I'd love to merge the series for 6.7, and the time ticks...
Done, added the error handling early in the series as a bugfix.

Cheers,
~Maarten