[PATCH 0/2] ASoC/SoundWire: Intel: add sdw BE dai trigger
For SOF IPC4, we need to set pipeline state in BE DAI trigger. @Vinod, Could you ack if the soundwire patch looks good to you? And we can go through the ASoC tree since the change is mainly on ASoC.
Bard Liao (2): soundwire: Intel: add trigger callback ASoC: SOF: Intel: add trigger callback into sdw_callback
drivers/soundwire/intel.c | 8 ++++++++ include/linux/soundwire/sdw_intel.h | 1 + sound/soc/sof/intel/hda-dai.c | 15 ++++++++++++--- sound/soc/sof/intel/hda.c | 2 +- sound/soc/sof/intel/hda.h | 1 + 5 files changed, 23 insertions(+), 4 deletions(-)
When a pipeline is split into FE and BE parts, the BE pipeline may need to be triggered separately in the BE trigger op. So add the trigger callback in the link_res ops that will be invoked during BE DAI trigger.
Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- drivers/soundwire/intel.c | 8 ++++++++ include/linux/soundwire/sdw_intel.h | 1 + 2 files changed, 9 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 505c5ef061e3..2e7c27d303b4 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1004,9 +1004,17 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct sn { struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_intel *sdw = cdns_to_intel(cdns); + struct sdw_intel_link_res *res = sdw->link_res; struct sdw_cdns_dma_data *dma; int ret = 0;
+ /* The .trigger callback is used to send required IPC to audio + * firmware. The .free_stream callback will still be called + * by intel_free_stream() in the TRIGGER_SUSPEND case. + */ + if (res->ops && res->ops->trigger) + res->ops->trigger(dai, cmd, substream->stream); + dma = snd_soc_dai_get_dma_data(dai, substream); if (!dma) { dev_err(dai->dev, "failed to get dma data in %s\n", diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index 67e0d3e750b5..f638707fd712 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -119,6 +119,7 @@ struct sdw_intel_ops { struct sdw_intel_stream_params_data *params_data); int (*free_stream)(struct device *dev, struct sdw_intel_stream_free_data *free_data); + int (*trigger)(struct snd_soc_dai *dai, int cmd, int stream); };
/**
For IPC4, we need to set pipeline state in BE DAI trigger.
Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-dai.c | 15 ++++++++++++--- sound/soc/sof/intel/hda.c | 2 +- sound/soc/sof/intel/hda.h | 1 + 3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 228079a52c3d..6ed99fdc5793 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -713,8 +713,7 @@ static const struct snd_soc_dai_ops ipc3_ssp_dai_ops = { .shutdown = ssp_dai_shutdown, };
-static int ipc4_be_dai_trigger(struct snd_pcm_substream *substream, - int cmd, struct snd_soc_dai *dai) +static int ipc4_be_dai_common_trigger(struct snd_soc_dai *dai, int cmd, int stream) { struct snd_sof_widget *pipe_widget; struct sof_ipc4_pipeline *pipeline; @@ -723,7 +722,7 @@ static int ipc4_be_dai_trigger(struct snd_pcm_substream *substream, struct snd_sof_dev *sdev; int ret;
- w = snd_soc_dai_get_widget(dai, substream->stream); + w = snd_soc_dai_get_widget(dai, stream); swidget = w->dobj.private; pipe_widget = swidget->pipe_widget; pipeline = pipe_widget->private; @@ -758,6 +757,12 @@ static int ipc4_be_dai_trigger(struct snd_pcm_substream *substream, return 0; }
+static int ipc4_be_dai_trigger(struct snd_pcm_substream *substream, + int cmd, struct snd_soc_dai *dai) +{ + return ipc4_be_dai_common_trigger(dai, cmd, substream->stream); +} + static const struct snd_soc_dai_ops ipc4_dmic_dai_ops = { .trigger = ipc4_be_dai_trigger, }; @@ -809,6 +814,10 @@ void hda_set_dai_drv_ops(struct snd_sof_dev *sdev, struct snd_sof_dsp_ops *ops) if (!hda_use_tplg_nhlt) ipc4_data->nhlt = intel_nhlt_init(sdev->dev);
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE) + sdw_callback.trigger = ipc4_be_dai_common_trigger; +#endif + break; } default: diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index bc07df1fc39f..cdd3601e84f5 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -147,7 +147,7 @@ static int sdw_free_stream(struct device *dev, return hda_ctrl_dai_widget_free(w, SOF_DAI_CONFIG_FLAGS_NONE, &data); }
-static const struct sdw_intel_ops sdw_callback = { +struct sdw_intel_ops sdw_callback = { .params_stream = sdw_params_stream, .free_stream = sdw_free_stream, }; diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index f4e4cd7d7406..ec7a2d947eb6 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -771,5 +771,6 @@ irqreturn_t cnl_ipc4_irq_thread(int irq, void *context); int cnl_ipc4_send_msg(struct snd_sof_dev *sdev, struct snd_sof_ipc_msg *msg); irqreturn_t hda_dsp_ipc4_irq_thread(int irq, void *context); int hda_dsp_ipc4_send_msg(struct snd_sof_dev *sdev, struct snd_sof_ipc_msg *msg); +extern struct sdw_intel_ops sdw_callback;
#endif
On Tue, Jun 14, 2022 at 03:08:17PM +0800, Bard Liao wrote:
For IPC4, we need to set pipeline state in BE DAI trigger.
Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com
sound/soc/sof/intel/hda-dai.c | 15 ++++++++++++--- sound/soc/sof/intel/hda.c | 2 +- sound/soc/sof/intel/hda.h | 1 + 3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 228079a52c3d..6ed99fdc5793 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -713,8 +713,7 @@ static const struct snd_soc_dai_ops ipc3_ssp_dai_ops = { .shutdown = ssp_dai_shutdown, };
-static int ipc4_be_dai_trigger(struct snd_pcm_substream *substream,
int cmd, struct snd_soc_dai *dai)+static int ipc4_be_dai_common_trigger(struct snd_soc_dai *dai, int cmd, int stream) { struct snd_sof_widget *pipe_widget; struct sof_ipc4_pipeline *pipeline; @@ -723,7 +722,7 @@ static int ipc4_be_dai_trigger(struct snd_pcm_substream *substream, struct snd_sof_dev *sdev; int ret;
- w = snd_soc_dai_get_widget(dai, substream->stream);
- w = snd_soc_dai_get_widget(dai, stream); swidget = w->dobj.private; pipe_widget = swidget->pipe_widget; pipeline = pipe_widget->private;
@@ -758,6 +757,12 @@ static int ipc4_be_dai_trigger(struct snd_pcm_substream *substream, return 0; }
+static int ipc4_be_dai_trigger(struct snd_pcm_substream *substream,
int cmd, struct snd_soc_dai *dai)+{
- return ipc4_be_dai_common_trigger(dai, cmd, substream->stream);
+}
static const struct snd_soc_dai_ops ipc4_dmic_dai_ops = { .trigger = ipc4_be_dai_trigger, }; @@ -809,6 +814,10 @@ void hda_set_dai_drv_ops(struct snd_sof_dev *sdev, struct snd_sof_dsp_ops *ops) if (!hda_use_tplg_nhlt) ipc4_data->nhlt = intel_nhlt_init(sdev->dev);
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE)
sdw_callback.trigger = ipc4_be_dai_common_trigger;+#endif
#if should not be in .c files if at all possible. Surely there's a better way here...
thanks,
greg k-h
@@ -809,6 +814,10 @@ void hda_set_dai_drv_ops(struct snd_sof_dev *sdev, struct snd_sof_dsp_ops *ops) if (!hda_use_tplg_nhlt) ipc4_data->nhlt = intel_nhlt_init(sdev->dev);
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE)
sdw_callback.trigger = ipc4_be_dai_common_trigger;+#endif
#if should not be in .c files if at all possible. Surely there's a better way here...
we could use
if (IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE)) sdw_callback.trigger = ipc4_be_dai_common_trigger;
would that work?
We try to keep this driver configurable, not all platforms require SoundWire or HDaudio, and that 'sdw_callback' ops structure is conditionally declared.
On Tue, Jun 14, 2022 at 09:55:41AM -0500, Pierre-Louis Bossart wrote:
@@ -809,6 +814,10 @@ void hda_set_dai_drv_ops(struct snd_sof_dev *sdev, struct snd_sof_dsp_ops *ops) if (!hda_use_tplg_nhlt) ipc4_data->nhlt = intel_nhlt_init(sdev->dev);
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE)
sdw_callback.trigger = ipc4_be_dai_common_trigger;+#endif
#if should not be in .c files if at all possible. Surely there's a better way here...
we could use
if (IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE)) sdw_callback.trigger = ipc4_be_dai_common_trigger;
would that work?
It's more readable, right? Also easier to maintain over time.
We try to keep this driver configurable, not all platforms require SoundWire or HDaudio, and that 'sdw_callback' ops structure is conditionally declared.
Perhaps don't conditionally declare that? How much does it really save to do that?
thanks,
greg k-h
@@ -809,6 +814,10 @@ void hda_set_dai_drv_ops(struct snd_sof_dev *sdev, struct snd_sof_dsp_ops *ops) if (!hda_use_tplg_nhlt) ipc4_data->nhlt = intel_nhlt_init(sdev->dev);
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE)
sdw_callback.trigger = ipc4_be_dai_common_trigger;+#endif
#if should not be in .c files if at all possible. Surely there's a better way here...
we could use
if (IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE)) sdw_callback.trigger = ipc4_be_dai_common_trigger;
would that work?
It's more readable, right? Also easier to maintain over time.
yes, it's more readable, and that's no problem to update.
There's another #if IS_ENABLED( that we can replace by a if (IS_ENABLED in the same routine, that would make this routine less of an eyesore. I am not sure why we added all these #if, we can cleanup.
case SOF_INTEL_IPC4: { struct sof_ipc4_fw_data *ipc4_data = sdev->private;
for (i = 0; i < ops->num_drv; i++) { if (strstr(ops->drv[i].name, "DMIC")) { ops->drv[i].ops = &ipc4_dmic_dai_ops; continue; } if (strstr(ops->drv[i].name, "SSP")) { ops->drv[i].ops = &ipc4_ssp_dai_ops; continue; } #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) if (strstr(ops->drv[i].name, "iDisp") || strstr(ops->drv[i].name, "Analog") || strstr(ops->drv[i].name, "Digital")) ops->drv[i].ops = &ipc4_hda_dai_ops; #endif }
if (!hda_use_tplg_nhlt) ipc4_data->nhlt = intel_nhlt_init(sdev->dev);
#if IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE) sdw_callback.trigger = ipc4_be_dai_common_trigger; #endif
break; } default: break; } }
We try to keep this driver configurable, not all platforms require SoundWire or HDaudio, and that 'sdw_callback' ops structure is conditionally declared.
Perhaps don't conditionally declare that? How much does it really save to do that?
It would force us to expose additional things that are only relevant for SoundWire, see the large block of code in hda.c
https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/intel/hda.c#L10...
We've been burned in the past by having too many things in a single configuration, so we try to allow for minimal builds that don't depend on other modules in sound/soc/codecs/, sound/pci/hda and drivers/soundwire - it also forces us to get the Kconfig dependencies right.
if (IS_ENABLED()) is less invasive in that case.
Thanks for your feedback -Pierre
participants (3)
-
Bard Liao -
Greg KH -
Pierre-Louis Bossart