| Message ID | 20230807090045.198993-9-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sound: Use -EPROBE_DEFER instead of i915 module loading. | expand |
On Mon, 07 Aug 2023 16:26:53 +0200, Pierre-Louis Bossart wrote: > > > > On 8/7/23 04:00, 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. > > I don't think this patch is aligned with the previous discussions. What > we agreed on is that snd_hdac_i915_init() would be called from and not > from the workqueue. > > But this patch also moves all codec initialization out of the workqueue. > > I think we need two callbacks for device-specific initilization, one > that is called from the probe function and one from the workqueue, > otherwise we'll have a structure that differs from the snd-hda-intel - > which would be rather silly in terms of support/debug. > > I realize there's quite a bit of surgery involved, and most likely the > SOF folks should provide this patch for you to build on. So this patch looks like the only significant concern in the whole patch set. Can we reach to some agreement for merging to 6.6 in time? thanks, Takashi
Ping on this? On 2023-08-12 10:17, Takashi Iwai wrote: > On Mon, 07 Aug 2023 16:26:53 +0200, > Pierre-Louis Bossart wrote: >> >> >> >> On 8/7/23 04:00, 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. >> >> I don't think this patch is aligned with the previous discussions. What >> we agreed on is that snd_hdac_i915_init() would be called from and not >> from the workqueue. >> >> But this patch also moves all codec initialization out of the workqueue. >> >> I think we need two callbacks for device-specific initilization, one >> that is called from the probe function and one from the workqueue, >> otherwise we'll have a structure that differs from the snd-hda-intel - >> which would be rather silly in terms of support/debug. >> >> I realize there's quite a bit of surgery involved, and most likely the >> SOF folks should provide this patch for you to build on. > > So this patch looks like the only significant concern in the whole > patch set. Can we reach to some agreement for merging to 6.6 in time? > > > thanks, > > Takashi
On Mon, 14 Aug 2023 16:26:01 +0200, Maarten Lankhorst wrote: > > Ping on this? Pierre? Does one of your recent patch sets achieves the suggested thing? Or do we need another rewrite/respin of this series? Currently it's blocking the merge for 6.6. Takashi > On 2023-08-12 10:17, Takashi Iwai wrote: > > On Mon, 07 Aug 2023 16:26:53 +0200, > > Pierre-Louis Bossart wrote: > >> > >> > >> > >> On 8/7/23 04:00, 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. > >> > >> I don't think this patch is aligned with the previous discussions. What > >> we agreed on is that snd_hdac_i915_init() would be called from and not > >> from the workqueue. > >> > >> But this patch also moves all codec initialization out of the workqueue. > >> > >> I think we need two callbacks for device-specific initilization, one > >> that is called from the probe function and one from the workqueue, > >> otherwise we'll have a structure that differs from the snd-hda-intel - > >> which would be rather silly in terms of support/debug. > >> > >> I realize there's quite a bit of surgery involved, and most likely the > >> SOF folks should provide this patch for you to build on. > > > > So this patch looks like the only significant concern in the whole > > patch set. Can we reach to some agreement for merging to 6.6 in time? > > > > > > thanks, > > > > Takashi >
Hi, On Fri, 18 Aug 2023, Takashi Iwai wrote: > On Mon, 14 Aug 2023 16:26:01 +0200, > Maarten Lankhorst wrote: > > > > Ping on this? > > Pierre? Does one of your recent patch sets achieves the suggested > thing? Or do we need another rewrite/respin of this series? > Currently it's blocking the merge for 6.6. this is likely to require another spin. Pierre did a draft of the new ops at https://github.com/thesofproject/linux/pull/4527 and Maarten is looking to adapt the series to this. Br, Kai
On Fri, 18 Aug 2023 14:24:14 +0200, Kai Vehmanen wrote: > > Hi, > > On Fri, 18 Aug 2023, Takashi Iwai wrote: > > > On Mon, 14 Aug 2023 16:26:01 +0200, > > Maarten Lankhorst wrote: > > > > > > Ping on this? > > > > Pierre? Does one of your recent patch sets achieves the suggested > > thing? Or do we need another rewrite/respin of this series? > > Currently it's blocking the merge for 6.6. > > this is likely to require another spin. Pierre did a draft of > the new ops at https://github.com/thesofproject/linux/pull/4527 > and Maarten is looking to adapt the series to this. OK, then it can be too late for 6.6, unfortunately. Takashi
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 30db685cc5f4..cd4d06d1800b 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -188,13 +188,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) struct snd_sof_pdata *plat_data = sdev->pdata; int ret; - /* probe the DSP hardware */ - ret = snd_sof_probe(sdev); - if (ret < 0) { - dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret); - goto probe_err; - } - sof_set_fw_state(sdev, SOF_FW_BOOT_PREPARE); /* check machine info */ @@ -325,10 +318,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) dbg_err: snd_sof_free_debug(sdev); dsp_err: - snd_sof_remove(sdev); -probe_err: - sof_ops_free(sdev); - /* all resources freed, update state to match */ sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED); sdev->first_boot = true; @@ -436,6 +425,12 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data) sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED); + ret = snd_sof_probe(sdev); + if (ret) { + dev_err_probe(sdev->dev, ret, "failed to probe DSP\n"); + return ret; + } + if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) { INIT_WORK(&sdev->probe_work, sof_probe_work); schedule_work(&sdev->probe_work); @@ -485,9 +480,9 @@ int snd_sof_device_remove(struct device *dev) snd_sof_ipc_free(sdev); snd_sof_free_debug(sdev); - snd_sof_remove(sdev); } + snd_sof_remove(sdev); sof_ops_free(sdev); /* release firmware */ diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index f1fd5b44aaac..344b61576c0e 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) return 0; /* i915 exposes a HDA codec for HDMI audio */ - ret = snd_hdac_i915_init(bus, true); + ret = snd_hdac_i915_init(bus, false); if (ret < 0) return ret;
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. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- sound/soc/sof/core.c | 19 +++++++------------ sound/soc/sof/intel/hda-codec.c | 2 +- 2 files changed, 8 insertions(+), 13 deletions(-)