Fix IPC4 pipeline trigger by ranj063 · Pull Request #3917 · thesofproject/linux

ranj063 · 2022-10-11T21:37:28Z

FIrst patch is from the previous PR #3914

ujfalusi · 2022-10-13T13:40:11Z

+	for (i = 0; i < pipeline_info.count; i++) {
+ pipeline_widget = pipeline_info.pipelines[i];
+ pipeline = pipeline_widget->private;
+ pipeline->state = state;


So, we still have the separate BE (DAI) pipeline trigger and we handle the non BE (DAI) pipelines in batch mode?

no if there are multiple BE DAI's, their pipelines will get triggered in the respective BE trigger callbacks. There wouldnt be a batch mode there

In here it does not matter if we have triggered it or not like in case of the pause?

it does matter and if it is triggered it will be in the right state already no?

I meant the ones newly triggered will need an update to the state and the others will be the same state already. So, no need to check here

ujfalusi · 2022-10-13T13:57:15Z

@ranj063, I have started internal IPC4 daily test plan: 16213

plbossart

This looks much better @ranj063, but I still have a number of questions.

plbossart · 2022-10-19T01:18:39Z

 	case SOF_DAI_INTEL_HDA:
 gtw_attr = ipc4_copier->gtw_attr;
 gtw_attr->lp_buffer_alloc = pipeline->lp_mode;
+ pipeline->skip_during_fe_trigger = true;


I am still in the dark on why we need to skip only HDAudio pipelines?

because they're the only ones left with BE DAI trigger ops

Can't we detect this rather than add a flag that duplicates information that can be discovered?

I can think of doing this with a very convoluted check by accessing the snd_soc_dai from the snd_sof_widget using the sname and then checking if the dai->drv-ops are set. But the flag is a lot simpler I think

Don't we have a snd_sof_dai -> snd_soc_dai helper? I don't really understand why we don't have a 1:1 mapping.

we don't have a 1:1 mapping, Pierre. I can add this in a follow up maybe

That would be good indeed

These will be used to perform IPC-specific PCM setup/free. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ujfalusi · 2022-10-19T11:33:54Z

 };

+/**
+ * struct snd_sof_pipeline_trigger_info - multi pipeline trigger IPC info


How this struct is 'multi pipeline trigger IPC info'?
It is more like pipeline path or pipeline list.
In any case, I find it increasingly confusing that snd_sof_widget is sometimes a widget, module, component or a pipeline.
Probably it is really a snd_sof_pipeline_trigger_path? Certainly not IPC info.

And this does not necessarily needs to be valid for paths which consists of more than one pipeline (we always have at least 2 pipelines with IPC4, are they also multi-pipeline?)

yes 2 is multi a well.

In any case, I find it increasingly confusing that snd_sof_widget is sometimes a widget, module, component or a pipeline.
a widget is never a pipeline but widget/module/component are all used intrerchageably. There's a pipeline widget which is a widget of type snd_soc_dapm_scheduler.

How this struct is 'multi pipeline trigger IPC info'?
I changed the name to snd_sof_pcm_stream_trigger_info now. hope that makes more sense.

Define the pcm_setup/pcm_free ops for IPC4. Define a new struct snd_sof_pcm_stream_trigger_info and add a new field trigger_info of this type to struct snd_sof_pcm_stream. This will be used to save the list of pipelines that need to be triggered. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

…trigger Add a new flag, skip_during_fe_trigger, to struct sof_ipc4_pipeline to skip triggering pipelines in the FE DAI trigger. Set this flag for the HDA DAI BE pipelines so that their BE pipeline will not be triggered in the FE DAI trigger. Also, move the trigger handling for all commands include START/PAUSE_RELEASE for the HDA DAI's to the backend DAI trigger ops. For the SSP/DMIC/SDW cases, remove the BE DAI trigger as they involve no DMA operations and can be triggered in the FE DAI trigger. This is in preparation to perform batch triggering of all pipelines for the non-HDA case. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Populate the pipeline_info for the PCM stream with the list of pipeline widgets that need to be handled during the PCM trigger. This will be used in the IPC-specific PCM trigger op to trigger the pipelines. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Use the list of pipelines in the PCM stream's pipeline info to trigger the pipelines in the right order. Add a helper for triggering pipelines in batch mode that will be used to trigger multiple pipelines at the same time. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

bardliao

This PR LGTM and test result looks good

libinyang

LGTM

ujfalusi · 2022-10-19T12:04:13Z

+ return 0;
+
+	/* allocate data for the pipeline ID's and the count of pipelines to be triggered */
+	data = kzalloc(sizeof(u32) * (pipeline_info->count + 1), GFP_KERNEL);


data=kzalloc(struct_size(data, pipeline_ids, pipeline_info->count), GFP_KERNEL);

this too, can I fix in a fixup?

ujfalusi · 2022-10-20T11:20:39Z

+ */
+struct snd_sof_pcm_stream_trigger_info {
+	uint32_t count;
+	struct snd_sof_widget **pipeline_list;


I'm not good at naming, but would
struct snd_sof_pcm_stream_pipeline_list { uint32_t count; struct snd_sof_widget **pipelines; };
or
struct snd_sof_pipelines_in_pcm_stream { uint32_t count; struct snd_sof_widget **pipeline_list; };
would describe this better?
The reason is that I don't see anything trigger_infoish in the struct

@ujfalusi pipeline_list is the list of pipelines to be triggered. Isn't that good enough? And it is only used in the trigger callback.

plbossart

This is good enough for me if there is a fixup coming to take @bardliao and @ujfalusi 's suggestions into account.

@ujfalusi you get to choose if a new version is needed or if we handle change requests with a fixup.

ujfalusi

@ranj063, overall this looks great, amazing actually.
I think it is fine to do fixups for the smaller issues since the functionality is not going to be changed, let's get this merged if everyone is OK with it.

plbossart · 2022-10-20T17:43:36Z

merging

ranj063 requested review from RanderWang, ajitkupandey, bardliao, dbaluta, kuanhsuncheng, kv2019i, lgirdwood, libinyang, plbossart, ujfalusi and yaochunhung as code owners October 11, 2022 21:37

ranj063 force-pushed the fix/intermediate-pipelines branch from b0b45f0 to 7641581Compare October 11, 2022 23:11

RanderWang reviewed Oct 12, 2022
View reviewed changes

Comment threadsound/soc/sof/ipc4-pcm.c Outdated

ranj063 force-pushed the fix/intermediate-pipelines branch from 7641581 to 768141bCompare October 12, 2022 03:01

ranj063 requested a review from RanderWang October 12, 2022 03:01

ranj063 force-pushed the fix/intermediate-pipelines branch 2 times, most recently from 71a5e88 to b001899Compare October 12, 2022 03:36

RanderWang reviewed Oct 12, 2022
View reviewed changes

Comment threadsound/soc/sof/ipc4-topology.h Outdated

RanderWang reviewed Oct 12, 2022
View reviewed changes

Comment threadsound/soc/sof/ipc4-pcm.c Outdated

bardliao reviewed Oct 12, 2022
View reviewed changes

Comment threadsound/soc/sof/ipc4-pcm.c Outdated

RanderWang reviewed Oct 12, 2022
View reviewed changes

Comment threadsound/soc/sof/ipc4-pcm.c Outdated

bardliao previously approved these changes Oct 12, 2022
View reviewed changes

ranj063 mentioned this pull request Oct 12, 2022
Skip triggering BE pipelines in FE trigger #3914
Closed

RanderWang reviewed Oct 13, 2022
View reviewed changes

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

ujfalusi reviewed Oct 13, 2022
View reviewed changes

ranj063 dismissed bardliao’s stale review via a11c35bOctober 13, 2022 15:40

ranj063 force-pushed the fix/intermediate-pipelines branch from b001899 to a11c35bCompare October 13, 2022 15:40

ranj063 requested review from bardliao and ujfalusi October 13, 2022 15:41

ranj063 force-pushed the fix/intermediate-pipelines branch from a8c8e8a to 8b063a4Compare October 19, 2022 01:20

plbossart reviewed Oct 19, 2022
View reviewed changes

ranj063 force-pushed the fix/intermediate-pipelines branch 5 times, most recently from 67622a0 to dcab26aCompare October 19, 2022 02:20

ASoC: SOF: Introduce PCM setup/free PCM IPC ops
64d7cf4
These will be used to perform IPC-specific PCM setup/free. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/intermediate-pipelines branch from dcab26a to 462da65Compare October 19, 2022 05:28

ujfalusi reviewed Oct 19, 2022
View reviewed changes

RanderWang reviewed Oct 19, 2022
View reviewed changes

Comment threadsound/soc/sof/ipc4-pcm.c

ranj063 added 3 commits October 19, 2022 07:41

plbossart reviewed Oct 19, 2022
View reviewed changes

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

ranj063 force-pushed the fix/intermediate-pipelines branch from 462da65 to dd63b11Compare October 19, 2022 15:02

ranj063 requested review from RanderWang and ujfalusi October 19, 2022 16:13

ranj063 force-pushed the fix/intermediate-pipelines branch from 52285a3 to 4abe644Compare October 20, 2022 01:54

RanderWang approved these changes Oct 20, 2022
View reviewed changes

bardliao reviewed Oct 20, 2022
View reviewed changes

Comment threadsound/soc/sof/ipc4-pcm.c

bardliao approved these changes Oct 20, 2022
View reviewed changes

libinyang approved these changes Oct 20, 2022
View reviewed changes

ujfalusi reviewed Oct 20, 2022
View reviewed changes

plbossart approved these changes Oct 20, 2022
View reviewed changes

Comment threadsound/soc/sof/ipc4-pcm.c

ujfalusi approved these changes Oct 20, 2022
View reviewed changes

plbossart requested a review from kv2019i October 20, 2022 16:54

plbossart mentioned this pull request Oct 24, 2022
[BUG][TGL] ipc tx error for 0x60040000(GLB_STREAM_MSG: TRIG_START) on TGL-HDA platform #3910
Closed

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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 commented Oct 13, 2022

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

ranj063Oct 19, 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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bardliao left a comment

Choose a reason for hiding this comment

Uh oh!

libinyang left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Oct 11, 2022•
edited
Loading

ranj063Oct 19, 2022•
edited
Loading