Abstract the DMA and Codec DAI ops for the BE DAI widgets by ranj063 · Pull Request #3972 · thesofproject/linux

ranj063 · 2022-10-30T00:02:46Z

Introduce SOF DAI ops to abstract the BE DAI widget DMA, codec_dai and dai_config ops. This change will allow adding the chained DMA mode or the DSP-less mode for the BE DAI widgets with a simple change like this:

diff --git a/sound/soc/sof/intel/hda-dai-ops.c b/sound/soc/sof/intel/hda-dai-ops.c
index 1d1e238d032e..1be992367a89 100644
--- a/sound/soc/sof/intel/hda-dai-ops.c
+++ b/sound/soc/sof/intel/hda-dai-ops.c
@@ -433,10 +433,19 @@ void hda_set_dai_widget_ops(struct snd_sof_dev *sdev, struct snd_sof_widget *swi
}
case SOF_INTEL_IPC4:
{
+ struct snd_sof_widget *pipe_widget = swidget->pipe_widget;
struct sof_ipc4_copier *ipc4_copier = sdai->private;
+ struct sof_ipc4_pipeline *pipeline = pipe_widget->private;
+ struct snd_sof_dai_dma_ops *dma_ops = sdai->ops->dma_ops;
+
+ if (ipc4_copier->dai_type != SOF_DAI_INTEL_HDA)
+ break;
+
+ if (pipeline->use_chain_dma) {
+ dma_ops->pre_trigger = NULL;
+ dma_ops->post_trigger = NULL;
+ }
- if (ipc4_copier->dai_type == SOF_DAI_INTEL_HDA)
- sdai->ops = &hda_ipc4_dai_ops;
break;
}
default:

plbossart

@ranj063 I think you're on to something with the .pre and .post trigger.

I added a bunch of comments to try and help clarify the code, most of the changes are confusing due to naming aliases to different concepts.

plbossart · 2022-10-31T16:25:32Z

 strstr(ops->drv[i].name, "Analog") ||
 strstr(ops->drv[i].name, "Digital"))
- ops->drv[i].ops = &ipc4_hda_dai_ops;
+ ops->drv[i].ops = &hda_dai_ops;


this is a bit confusion, if the ops are the same between IPC and IPC4 why do wee need the switch()?

for SSP and the NHLT for IPC4

plbossart · 2022-10-31T16:27:59Z

+ if (ops && ops->dma_ops && ops->dma_ops->post_trigger) {
+ ret = ops->dma_ops->post_trigger(sdev, cpu_dai,
+ hext_stream->link_substream,
+ SNDRV_PCM_TRIGGER_SUSPEND);


I didn't really follow how the hw_free and post-trigger were related. The naming points to different phases in the ALSA world.

For IPC3, post_trigger does what hda_dai_hw_free_ipc(). For IPC4 also, we were doing the same and it made no sense. SO this patch actually fixes it

bardliao · 2022-11-01T10:58:39Z

 	struct hdac_ext_link *hlink;
 	struct snd_sof_dev *sdev;
 	struct hdac_bus *bus;
+	unsigned int format_val, link_bps;


one variable per line?

they are not initialized, so this is fine.

it's still better to have one variable per line for consistency.

plbossart

LGTM, we should probably do more testing overnight to see if we have any regressions, this is a rather intrusive patch.

Nice work @ranj063, I think this will help a lot clarify the sequences which were diluted with different ops and branches left and right.

plbossart · 2022-11-17T01:05:24Z

@ujfalusi @RanderWang @bardliao @kv2019i @jsarha please prioritize this PR if you have a bit of time on Thursday 16, we are trying to complete this work before the Thanksgiving break. Friday will be too late for any interaction.

RanderWang · 2022-11-17T05:25:50Z

+	if (tplg_ops && tplg_ops->dai_config) {
+ ret = tplg_ops->dai_config(sdev, swidget, flags, data);
+ if (ret < 0) {
+ dev_err(sdev->dev, "DAI config with flags %x failed for widget %s\n",


%#x to print hex number ? It does not block me.

RanderWang · 2022-11-17T05:30:54Z

-	int stream = substream->stream;
+	struct snd_sof_dai_config_data data = { 0 };
+	unsigned int flags = SOF_DAI_CONFIG_FLAGS_HW_PARAMS;
 	int ret;


I like the layout
unsigned int flags = SOF_DAI_CONFIG_FLAGS_HW_PARAMS; struct snd_sof_dai_config_data data = { 0 }; struct hdac_ext_stream *hext_stream;

RanderWang · 2022-11-17T05:38:10Z

-	}
-	default:
- break;
+ ipc4_data->nhlt = intel_nhlt_init(sdev->dev);


why we need to init nhlt in hda_set_dai_drv_ops ? It is not a issue in this PR. we remove the switch for ipc version but leave one if for ipc version

RanderWang

LGTM

ujfalusi

@ranj063, this looks really nice!
The only thing holding me to Approve is that I can not follow optionality in struct hda_dai_widget_dma_ops.
assign_hext_stream is as mandatory as the get_hext_stream, which also brings the release_hext_stream as mandatory.
By the look, the other ops are really optional.

ujfalusi · 2022-11-17T08:00:50Z

-snd_hdac_ext_stream_decouple_locked(bus, res, true);
+	/* check if mandatory ops are set */
+if (!ops || !ops->get_hext_stream)
+	return NULL;


nitpick: I would print error when the get_ext_stream is missing to make it more obvious.

ujfalusi · 2022-11-17T08:08:24Z

+	if (!hext_stream) {
+ if (ops->assign_hext_stream)
+ hext_stream = ops->assign_hext_stream(sdev, cpu_dai, substream);
 	}


You got me here...
Only get_hext_stream is mandatory, which is to to retrieve the dma_data set by the optional assign_hext_stream.
If the assign_hext_stream is not set (optional!) and this is the first call here then the get_hext_stream is going to return with NULL and you are not going to get anything but NULL...
What I was saying is that most, if not all callbacks in struct hda_dai_widget_dma_ops are in fact mandatory, don't they?

also to note: you also must have the release_hext_stream defined if you have the assign_hext_stream, but you must need to have the assign_hext_stream in order to have working stack.

or you should not return with error in line 118?

The conclusion for this thread is that assign/release need to be optional because couple mode will not need them

ujfalusi · 2022-11-17T08:13:41Z

-	struct sof_ipc4_pipeline *pipeline;
+	const struct hda_dai_widget_dma_ops *ops = hda_dai_get_ops(substream, dai);
+	struct snd_sof_widget *swidget = w->dobj.private;
+	struct hdac_ext_stream *hext_stream;


nitpick: would look better two lines down..

ujfalusi

@ranj063, I'm OK with few fixups (checkpatch).
Let's see how this will behave in the wild and under the DSPless and ChainDMA cases.

Introduce a new ops structure for HDA DAI widget DMA ops and add a new field to struct snd_sof_dai that will be used to set the ops pointer for DAI widgets. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Define and set the get_hext_stream, assign_hext_stream and release_hext_stream DMA ops for HDA DAIs. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

…A ops Define and use the setup_hext_stream/reset_hext_stream DMA ops during link hw_params and cleanup. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Use the topology IPC dai_config to update the dai_config for HDA DAI widgets. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Define and use the SOF widget's DMA pre_trigger/trigger/post_trigger ops in ipc4_hda_dai_trigger(). Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Define the post_trigger DMA op for IPC3 and unify the DAI driver ops for IPC3 and IPC4 for HDA DAI's. Also, use the post_trigger op to stop the paused streams properly in the hda_dai_suspend() function. This fixes the suspend while paused case for IPC4 because previously we weren't resetting the pipeline when suspending the system with some paused streams. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Remove these functions and reuse hda_dai_config(). Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 requested review from RanderWang, ajitkupandey, bardliao, dbaluta, juimonen, kuanhsuncheng, kv2019i, lgirdwood, libinyang, lyakh, plbossart, ujfalusi and yaochunhung as code owners October 30, 2022 00:02

ranj063 force-pushed the fix/hda_dai_hw_params branch 7 times, most recently from 787fa0e to e945284Compare October 30, 2022 17:34

plbossart reviewed Oct 31, 2022
View reviewed changes

ranj063 force-pushed the fix/hda_dai_hw_params branch from e945284 to d56c49bCompare October 31, 2022 17:37

ranj063 requested a review from plbossart October 31, 2022 17:37

ranj063 force-pushed the fix/hda_dai_hw_params branch from d56c49b to ba07eb1Compare October 31, 2022 18:08

bardliao reviewed Nov 1, 2022
View reviewed changes

Comment threadsound/soc/sof/intel/hda-dai.c Outdated

bardliao reviewed Nov 1, 2022
View reviewed changes

Comment threadsound/soc/sof/intel/hda-dai-ops.c Outdated

RanderWang reviewed Nov 1, 2022
View reviewed changes

Comment threadsound/soc/sof/intel/hda-dai-ops.c

RanderWang reviewed Nov 1, 2022
View reviewed changes

Comment threadsound/soc/sof/intel/hda-dai.c Outdated

RanderWang reviewed Nov 1, 2022
View reviewed changes

Comment threadsound/soc/sof/sof-audio.h Outdated

plbossart previously approved these changes Nov 17, 2022
View reviewed changes

bardliao previously approved these changes Nov 17, 2022
View reviewed changes

RanderWang reviewed Nov 17, 2022
View reviewed changes

RanderWang previously approved these changes Nov 17, 2022
View reviewed changes

ujfalusi reviewed Nov 17, 2022
View reviewed changes

ujfalusi previously approved these changes Nov 17, 2022
View reviewed changes

ranj063 added 7 commits November 17, 2022 06:28

ASoC: SOF: Intel: hda-dai: Introduce DAI widget ops
d249846
Introduce a new ops structure for HDA DAI widget DMA ops and add a new field to struct snd_sof_dai that will be used to set the ops pointer for DAI widgets. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: Intel: hda-dai: Define and set the HDA DAI widget DMA ops
78b470f
Define and set the get_hext_stream, assign_hext_stream and release_hext_stream DMA ops for HDA DAIs. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: Intel: hda-dai: Add setup_hext_stream/reset_hext_stream DM…
b070e78
…A ops Define and use the setup_hext_stream/reset_hext_stream DMA ops during link hw_params and cleanup. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: hda-dai: Use the topology IPC dai_config op
bf8e962
Use the topology IPC dai_config to update the dai_config for HDA DAI widgets. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: Intel: hda-dai: Define DAI widget DMA trigger ops for IPC4
153dca4
Define and use the SOF widget's DMA pre_trigger/trigger/post_trigger ops in ipc4_hda_dai_trigger(). Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: Intel: hda: Remove hda_ctrl_dai_widget_setup/free()
ee710ae
Remove these functions and reuse hda_dai_config(). Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 dismissed stale reviews from ujfalusi, RanderWang, bardliao, and plbossart via ee710aeNovember 17, 2022 14:29

ranj063 force-pushed the fix/hda_dai_hw_params branch from 5c11a0f to ee710aeCompare November 17, 2022 14:29

bardliao approved these changes Nov 17, 2022
View reviewed changes

plbossart approved these changes Nov 17, 2022
View reviewed changes

plbossart requested review from RanderWang and ujfalusi November 17, 2022 14:33

ujfalusi approved these changes Nov 17, 2022
View reviewed changes

RanderWang approved these changes Nov 18, 2022
View reviewed changes

plbossart merged commit 1197769 into thesofproject:topic/sof-devNov 18, 2022

plbossart mentioned this pull request Nov 18, 2022
ASoC: SOF: (Intel HDA) Add support for DSPless debug mode #3962
Merged

ranj063 deleted the fix/hda_dai_hw_params branch November 19, 2022 00:54

Uh oh!

Conversation

ranj063 commented Oct 30, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063Oct 31, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart commented Nov 17, 2022

Uh oh!

RanderWangNov 17, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ranj063 commented Oct 30, 2022•
edited
Loading

ranj063Oct 31, 2022•
edited
Loading

RanderWangNov 17, 2022•
edited
Loading