Some clean ups for the way we parse the available audio formats in topology #4163

ranj063 · 2023-01-31T07:26:07Z

This PR makes the audio format tokens extensible for non-zero pins as well. Depends on thesofproject/sof#7014

plbossart

IPC4 is becoming rocket science without a user manual or textbook. I can't review this, sorry.

sound/soc/sof/ipc4-topology.h

plbossart

Now that I understand the direction, it's indeed quite an improvement @ranj063

Still a couple of unclear parts, see comments below.

sound/soc/sof/ipc4-topology.c

sound/soc/sof/ipc4-topology.h

sound/soc/sof/ipc4-topology.c

plbossart · 2023-01-31T19:25:44Z

sound/soc/sof/ipc4-pcm.c

I am not able to follow the original code or the suggested change. In both cases, input_audio_fmts can be an array of "@audio_fmt_num: Number of available audio formats" elements, so why take the first element only?

in other places you use &available_fmt->input_audio_fmts[i].audio_fmt, so not sure why the index 0 is used?

This needs to be revisited later @plbossart . I do not know the intention for the original code. This change is needed though because the sampling_frequency is in a different field

this was added by @RanderWang in bed4045. Can you comment Rander?

sof_ipc4_pcm_dai_link_fixup is called for BE and then FE is processed, at this function ipc4_copier is not prepared, so we can only get sample frequency from available_fmt (we can't use current params for SRC case). Use the first entry since dai copier is fixed with one sampling rate.

Erm, what @RanderWang? @kv2019i is precisely trying to add more rates to the BT pipelines, so I am not sure how your assumption that dai copiers have a single sampling rate could hold

yes, my assumption is stale.

sound/soc/sof/ipc4-topology.h

sound/soc/sof/ipc4-topology.c

plbossart

couple of nit-picks below.

sound/soc/sof/ipc4-topology.h

include/uapi/sound/sof/tokens.h

sound/soc/sof/ipc4-topology.c

ranj063 · 2023-02-01T19:05:48Z

The CI device test shows all fails because the PR depends on the SOF PR 7014. The results with the SOF PR are available in plan 20422

sound/soc/sof/ipc4-topology.c

include/uapi/sound/sof/tokens.h

ujfalusi · 2023-01-31T07:41:42Z

sound/soc/sof/ipc4-topology.c

I wounder if this should be done in the similar way the out_format is done, with local temp struct.
Should we also NULL out the available_fmt->input_audio_fmts just for safety?
Add a new label to jump?

I wounder if this should be done in the similar way the out_format is done, with local temp struct. Should we also NULL out the available_fmt->input_audio_fmts just for safety? Add a new label to jump?

@ranj063, any thought?

sound/soc/sof/ipc4-topology.c

ujfalusi · 2023-01-31T08:42:08Z

sound/soc/sof/ipc4-topology.c

How did the cpc and is_pages made it's way to these base_configs? I can not see it so there is a chance that it is overridden from the other base_configs?

sound/soc/sof/ipc4-topology.c

ujfalusi · 2023-01-31T09:04:09Z

sound/soc/sof/ipc4-topology.c

If we can save on memory allocation when it is clearly not needed to be done, then why we don't?

sound/soc/sof/ipc4-topology.h

ujfalusi · 2023-02-02T15:37:42Z

SOFCI TEST

ujfalusi · 2023-02-02T15:38:16Z

SOFCI TEST

The tplg dependency is merged, let's re-run and see how it behaves.

Currently we use input/output and sink/source pins interchangeably. Remove the references to sink/source pins and replace with input/output pins everywhere for consistency and clarity. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

plbossart

couple more comments @ranj063.

plbossart · 2023-02-07T18:00:18Z

sound/soc/sof/topology.c

-		dev_err(scomp->dev, "invalid pins for %s: [sink: %d, src: %d]\n",
-			swidget->widget->name, swidget->num_sink_pins, swidget->num_source_pins);
+	if (swidget->num_input_pins > SOF_WIDGET_MAX_NUM_PINS ||
+	    swidget->num_output_pins > SOF_WIDGET_MAX_NUM_PINS) {


should we check that the sum is < SOF_WIDGET_MAX_NUM_PINS

the definition of MAX_NUM_PINS applies individually for input and output pins. So I dont think a check for sum is required.

didn't we have an array where we first have the input pins and then the output pins?

I am pretty sure we discussed this with @aiChaoSONG but I don't know if this applies in this context.
This could be addressed as a separate patch if needed.

yes, this is documented here:
cf6e9a5#diff-3aa89cf77fc07290f36500245dad57c983ae8628a8fb4b43804391a01ca4f520R444

But I am not sure what the binding array size might be.

sound/soc/sof/ipc4-topology.h

sound/soc/sof/ipc4-topology.c

…e_audio_format Add a new field, input_audio_fmts, in struct sof_ipc4_available_audio_format and parse all the available input audio formats into this new field and not into the base_config field. This is preparation to remove the base_config array from the struct sof_ipc4_available_audio_format. This simplifies the sof_ipc4_init_audio_fmt() function as the reference audio format for matching with input params has the same size. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

plbossart

LGTM @ranj063

plbossart · 2023-02-07T19:36:16Z

sound/soc/sof/ipc4-topology.c

nit-pick , you could remove the if/else and do

copier_data->gtw_cfg.dma_buffer_size = min( 2, deep_buffer_dma_ms) * copier_data->base_config.ibs;

can be a fixup if agreed

@plbossart min(2, deep_buffer_dma_ms) would always result in 2 even with deep_buffer_dma_ms > 2 right?

should be max, fixed now

Yes, I meant max. brain typo!

Do not parse the SOF_TKN_CAVS_AUDIO_FORMAT_DMA_BUFFER_SIZE token as the dma_buffer_size can be derived from the input/output buffer size and the type of widget during copier prepare. For the deep buffer case, introduce a new token that will be used to get the deep buffer DMA size for the host copier from topology. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

There is no need to parse the cpc and is_pages values multiple times. It is enough to parse the 2 tokens directly into the base_config field in each module's init_instance IPC payload. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Parse the output formats available in topology always. Whether the output format is sent in the init instance payload or not is decided when sof_ipc4_init_audio_fmt() is invoked. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

…ormats Introduce a new struct sof_ipc4_pin_format which contains the pin index and the buffer size. Replace the type of available input/output audio formats in struct sof_ipc4_available_audio_format with this new struct type and rename them to input_pin_fmts and output_pin_fmts. Also, add a new token, SOF_TKN_CAVS_AUDIO_FORMAT_PIN_INDEX that will be used to parse the pin index for the audio format from topology. Currently we only set the audio format for Pin 0 in topology, so the default value will be 0 for all audio formats. Finally, parse the pin_index and the input/output buffer sizes along with audio formats into the pin_format arrays in struct sof_ipc4_available_audio_format. This makes the base_config array in struct sof_ipc4_available_audio_format redundant. So remove it. This change will allow the addition of audio formats for the non-zero pins in topology transparent to the topology parser in the kernel. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

…_fmt() Only the copier needs to set the output format in its IPC payload. So move the code to set the output format inside sof_ipc4_prepare_copier_module() and modify the signature of sof_ipc4_init_audio_fmt() to remove the out_format argument. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Remove the field in struct sof_ipc4_available_audio_format and pass the format list to be searched as an argument to sof_ipc4_init_audio_fmt() directly. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

…count In preparation for handling processing modules with different input/output pin counts, introduce two new tokens for input/output audio format counts. Use these token values to parse all the available audio formats from topology. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Base config only contains the input/output audio formats for pin 0. So match only the pin 0 formats during runtime format selection. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

RanderWang

LGTM

lgirdwood · 2023-02-08T11:16:03Z

@ujfalusi @ranj063 pls review/merge.

ujfalusi

@ranj063, one failure in PR test which I can not connect to the content of this:
[ 825.361247] kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc timed out for 0x13000004|0x1

I can not see anything standing out.

plbossart · 2023-02-08T14:57:34Z

@ranj063, one failure in PR test which I can not connect to the content of this: [ 825.361247] kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc timed out for 0x13000004|0x1

Known issue unrelated to this.

plbossart

LGTM @ranj063 , except for the restriction to 2ms. That's not something that we should encode in the kernel. Clearly that would prevent low-latency usages that are possible with a lower system tick - it's been done before.

This can be updated later, so approving for now.

plbossart · 2023-02-08T15:01:34Z

sound/soc/sof/ipc4-topology.h

 #define SOF_IPC4_INVALID_NODE_ID	0xffffffff

+/* FW requires minimum 2ms DMA buffer size */
+#define SOF_IPC4_MIN_DMA_BUFFER_SIZE	2


is this really 2ms or 2x the system tick?

I see no reason why 2ms would be required, there's nothing in hardware that requires this.

can be updated with a fixup

bardliao previously approved these changes Jan 31, 2023

View reviewed changes

ranj063 marked this pull request as ready for review January 31, 2023 15:18

ranj063 requested review from ajitkupandey, dbaluta, juimonen, kuanhsuncheng, kv2019i, lgirdwood, lyakh, plbossart, singalsu, ujfalusi and yaochunhung as code owners January 31, 2023 15:18

plbossart reviewed Jan 31, 2023

View reviewed changes

sound/soc/sof/ipc4-topology.h Outdated Show resolved Hide resolved

ranj063 dismissed bardliao’s stale review via 0f5f9d9 January 31, 2023 18:18

ranj063 force-pushed the fix/available_fmts_new branch from 91438c0 to 0f5f9d9 Compare January 31, 2023 18:18

plbossart reviewed Jan 31, 2023

View reviewed changes

ranj063 force-pushed the fix/available_fmts_new branch 2 times, most recently from 4c6ad25 to aebe3d0 Compare January 31, 2023 21:09

plbossart reviewed Jan 31, 2023

View reviewed changes

sound/soc/sof/ipc4-topology.h Outdated Show resolved Hide resolved

include/uapi/sound/sof/tokens.h Outdated Show resolved Hide resolved

sound/soc/sof/ipc4-topology.c Outdated Show resolved Hide resolved

ranj063 force-pushed the fix/available_fmts_new branch 2 times, most recently from 4ca911d to 8f2876f Compare February 1, 2023 05:56

bardliao reviewed Feb 2, 2023

View reviewed changes

sound/soc/sof/ipc4-topology.c Outdated Show resolved Hide resolved

ranj063 force-pushed the fix/available_fmts_new branch from 8f2876f to a203f7f Compare February 2, 2023 04:03

bardliao previously approved these changes Feb 2, 2023

View reviewed changes

RanderWang reviewed Feb 2, 2023

View reviewed changes

sound/soc/sof/ipc4-topology.c Outdated Show resolved Hide resolved

ujfalusi reviewed Feb 2, 2023

View reviewed changes

ranj063 force-pushed the fix/available_fmts_new branch from 4aaeaad to c21c54c Compare February 7, 2023 17:40

ranj063 force-pushed the fix/available_fmts_new branch from c21c54c to ef8b8ea Compare February 7, 2023 17:57

plbossart requested changes Feb 7, 2023

View reviewed changes

ranj063 force-pushed the fix/available_fmts_new branch from ef8b8ea to cad1a16 Compare February 7, 2023 18:55

ranj063 requested review from kv2019i and plbossart February 7, 2023 18:55

plbossart previously approved these changes Feb 7, 2023

View reviewed changes

ranj063 dismissed plbossart’s stale review via b671382 February 7, 2023 22:42

ranj063 force-pushed the fix/available_fmts_new branch 2 times, most recently from b671382 to cffe1cc Compare February 7, 2023 23:52

ranj063 added 8 commits February 7, 2023 17:54

ASoC: SOF: ipc4-topology: Remove the ref_audio_fmt field

698e985

Remove the field in struct sof_ipc4_available_audio_format and pass the format list to be searched as an argument to sof_ipc4_init_audio_fmt() directly. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: ipc4-topology: Search only pin 0 formats

e9ce8b9

Base config only contains the input/output audio formats for pin 0. So match only the pin 0 formats during runtime format selection. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/available_fmts_new branch from cffe1cc to e9ce8b9 Compare February 8, 2023 01:54

RanderWang approved these changes Feb 8, 2023

View reviewed changes

bardliao approved these changes Feb 8, 2023

View reviewed changes

lgirdwood added the P1 Blocker bugs or important features label Feb 8, 2023

ujfalusi approved these changes Feb 8, 2023

View reviewed changes

plbossart approved these changes Feb 8, 2023

View reviewed changes

plbossart merged commit cfd8c7d into thesofproject:topic/sof-dev Feb 8, 2023

libinyang mentioned this pull request Feb 14, 2023

Sanity check available_fmt->input_pin_fmts and available_fmt->output_pin_fmts before using them #4190

Closed

Some clean ups for the way we parse the available audio formats in topology #4163

Some clean ups for the way we parse the available audio formats in topology #4163

Uh oh!

Conversation

ranj063 commented Jan 31, 2023 • 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!

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!

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!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ranj063 commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ujfalusi commented Feb 2, 2023

Uh oh!

ujfalusi commented Feb 2, 2023

Uh oh!

plbossart left a comment

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!

plbossart left a comment

Choose a reason for hiding this comment

ranj063 commented Jan 31, 2023 •

edited

Loading

ranj063 commented Feb 1, 2023 •

edited

Loading