ASoC: SOF: ipc4/Intel: Place constraint on the PERIOD_TIME based on the DSP host buffer size (which is provided in ms) fro playback #4858

ujfalusi · 2024-03-13T13:10:40Z

Hi,

when a playback starts, the first DMA burst will fill the DSP host buffer.
If user space asks for shorter period size then this burst and starts a playback with less than the burst size of the DSP then it will immediately xrun (DMA copies uninitialized data to DSP).

Place a constraint on the minimum period time (which will be forcing safe period size) to avoid such issue.

The dsp_buffer_time_ms can be used to save the length of the DSP host buffer for platform code to place constraint using it to avoid user space requesting for smaller period size than the DMA burst between DSP and host. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

plbossart

hear hear, the ALSA period size must indeed be a multiple of the DSP buffer.

Thanks @ujfalusi, comments below.

plbossart · 2024-03-13T13:29:57Z

sound/soc/sof/ipc4-topology.c

for my education and a paranoia check, when is this sof_ipc4_widget_setup_pcm() routine called?

It needs to happen BEFORE the PCM hw_params, right?

It happens when the PCMs are created and yes, we need to record this information before any audio activity is started since we need to place the constraint in open so it is taken into account during hw_params

sound/soc/sof/ipc4-topology.c

ujfalusi

@plbossart, should we modify the PERIOD_STEPS constraint as well?
^ nevermind

ujfalusi · 2024-03-13T13:38:38Z

sound/soc/sof/ipc4-topology.c

It happens when the PCMs are created and yes, we need to record this information before any audio activity is started since we need to place the constraint in open so it is taken into account during hw_params

sound/soc/sof/ipc4-topology.c

When setting up the pcm widget, save the DSP buffer size (in ms) for platform code to place a constraint on playback. On playback the DMA will fill the buffer on start and if the period size is smaller it will immediately overrun. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

If the PCM have the dsp_buffer_time_ms set then place a constraint to limit the minimum period time to avoid smaller period sizes than the DMA burst. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ujfalusi · 2024-03-13T13:44:14Z

Changes since v1:

Fix misspelled word in commit 2

plbossart · 2024-03-13T13:59:13Z

sound/soc/sof/ipc4-topology.c

+				      swidget->num_tuples, sizeof(u32), 1);
+		/* Set default DMA buffer size if it is not specified in topology */
+		if (!sps->dsp_buffer_time_ms)
+			sps->dsp_buffer_time_ms = SOF_IPC4_MIN_DMA_BUFFER_SIZE;


ipc4-topology.h:#define SOF_IPC4_MIN_DMA_BUFFER_SIZE 2

Is this really what we want? Why would we use multiple of 2ms for the ALSA period size? I think this confuses buffer and period, no?

@plbossart, no, the default SOF host buffer is 2ms long (with 2x 1ms firmware periods). Initially the DMA will move 2ms on these PCMs with default buffer configuration.
The DeepBuffer PCM have by default 100ms size (with100x 1ms firmware periods), here also the DMA will move initially with the size of the DSP buffer.

That define must be renamed to SOF_IPC4_MIN_DMA_BUFFER_TIME_MS

In case of capture we can allow 1ms period sizes, the firmware will push 1ms chunks afaik and we don't have DeepBuffer for capture, again, afaik.

That doesn't answer to my question: Are we going to be limited to 2ms ALSA period sizes for playback? That is a fairly important limitation IMHO. Increases in latency are like temporary tax increases, in practice they remain permanent...

I also wonder if the firmware behavior is predicated on the use of SPIB, which prevents invalid values from being transmitted to the DSP. The problem is that SPIB is not enabled by default for PCM devices.

@plbossart, no, the default SOF host buffer is 2ms long (with 2x 1ms firmware periods). Initially the DMA will move 2ms on these PCMs with default buffer configuration. The DeepBuffer PCM have by default 100ms size (with100x 1ms firmware periods), here also the DMA will move initially with the size of the DSP buffer.

That define must be renamed to SOF_IPC4_MIN_DMA_BUFFER_TIME_MS

Is this really correct? The buffer size is 2ms long alright but why would the burst be 2ms also? Doesn;'t that mean the buffer will have to be twice as large ie 4ms large?

When we start a playback the whole host DMA buffer is filled regardless of it's size.

SOF internally uses 1ms 'periods'
'Default' PCMs are using 2ms long host DMA buffer (2x 1ms SOF periods)
'DeepBuffer' is using 100ms long host DMA buffer (100x 1ms SOF periods)
The host DMA buffer length is configurable via tplg, so you can have 2sec, 50ms, whatever size selected if you want.

Again, on playback start the whole host DMA buffer is filled and after that the DMA will fill it in SOF-period step bursts, so the DMA never moves by frames, it moves with multiple of 1ms. Always.
'Default' PCMs:
1st DMA burst: 2ms worth of data
2nd and onward: 1ms worth of data bursts (in 1ms intervals)

'DeepBuffer' PCMs:
1st DMA burst: 100ms worth of data
2nd and onward (checked on HW): 98ms worth of data bursts (in 98ms intervals - when we have 2ms still left in the host DMA buffer)

Afaik the DeepBuffer burst size is firmware calculated based on wake latency or something, so host does not know this.

So, what to make of this?

Initially the application must provide data at least the size of the host DMA buffer, but this is not the size of burst size that you will have later on.

@ranj063, if we set the constraint on the period time to be minimum of the host DMA buffer then the ALSA buffer must be at least twice as big (periods_min is 2)

kv2019i

A few "thinking aloud" comments inline, but I think this is a +1 from me and can help a great deal avoiding problems with app confusion.

kv2019i · 2024-03-13T15:08:14Z

sound/soc/sof/intel/hda-pcm.c

+	 * Set constraint on the period time to make sure that it is larger than
+	 * the DSP buffer size. This is the maximum DMA burst size that can
+	 * happen on stream start for example.
+	 * The period size must not be smaller than this.


This is not a completely simple statement to explain (why period size cannot be smaller). Purely by ALSA semantics, smaller period sizes can be allowed. The period size is just a reporting interval. So I think this patch is more to encode a practical lower limit as many existing applications are not prepared to handle the behaviour that will result if DMA burst size is longer than period size. But yeah, just nitpicking on why we have the "must" here. I think for purpose of this patch, this is ok -- there's no functional need for us to allow to such small sizes, so why not just set the constraint.

kv2019i · 2024-03-13T15:25:47Z

sound/soc/sof/intel/hda-pcm.c

+	if (spcm->stream[substream->stream].dsp_buffer_time_ms)
+		snd_pcm_hw_constraint_minmax(substream->runtime,
+			SNDRV_PCM_HW_PARAM_PERIOD_TIME,
+			spcm->stream[substream->stream].dsp_buffer_time_ms * USEC_PER_MSEC,


I was wondering if this can be cause unexpected isssues with large values (e.g. a 100ms buffer) with appes that want a specific amount of periods. But yeah, I think in all cases I thought through, it's better to fail early. On the other hand, I can see a lot more confusion with apps that set the period size to low and expect period wakeups to come at a steady rate.

Right, that is true. On the other hand those apps will have other problems if they want to use small periods when the DMA burst in a different magnitude...

ranj063 · 2024-03-13T17:27:03Z

sound/soc/sof/intel/hda-pcm.c

+			SNDRV_PCM_HW_PARAM_PERIOD_TIME,
+			spcm->stream[substream->stream].dsp_buffer_time_ms * USEC_PER_MSEC,
+			UINT_MAX);
+


@ujfalusi I understand that if you dont set this constraint you will see an underrun immediately when the period size is smaller than the burst size. But for my education, when is the period time set? I am confused about how setting this constraint during pcm open helps

I will revisit this as I think the PERIOD_TIME min is not the right one to constrain.
ALSA interface is flexible, applications can at the end do whatever they want, if we we force them to have min host DMA buffer size periods they can still happily copy fractions of that size into ALSA buffer and set the playback to start which will cause pretty big issues down the line.
I don't see anything that can provide hint to applications that they need to have at least this and that amount prepared before starting the stream. The start_threshold is a sw param which the driver cannot constraint and applications can happily ignore it if they want.

I think we will place the constraint on the min BUFFER_TIME with the dsp_buffer_time_ms, that is the absolute minimum size of buffer applications can have without spinning on it during the first burst.
From there, well, we can hope that applications will fill the buffer before starting the playback and keep the buffer full.

Hrm, but that will not work with DeepBuffer if the app takes the min buffer size and uses 2 periods? It is on the edge imho.

The constraint on BUFFER_TIME still leaves mpv broken, it picks 4800 (S16_LE, stereo, 48KHz = 100ms) buffer with 2 periods and it ends up in endless xrun loop.

The constraint must be on the period size, or we should make the buffer size to be at least twice of the host DMA buffer and leave the period alone?

At the end both will result the same buffer_size_min limit, I wonder which one would be better and more correct for applications.

"When we start a playback the whole host DMA buffer is filled regardless of it's size"

Isn't this the problem statement?
I wonder why the firmware has this requirement. There's a concept of periods on the DSP side as well, so it could perfectly well start processing as soon as the first period is filled.

ranj063 · 2024-03-13T17:31:08Z

sound/soc/sof/intel/hda-pcm.c

 		snd_pcm_hw_constraint_mask64(substream->runtime, SNDRV_PCM_HW_PARAM_FORMAT,
 					     SNDRV_PCM_FMTBIT_S16 | SNDRV_PCM_FMTBIT_S32);

+	/*


just a nit, can we not squash all these three patches into one?

I don't like patches touching core and platform code at the same time when it can be avoided.

ujfalusi · 2024-03-15T07:58:58Z

Merged to #4851 to have all delay related fixes in one place (for make it easier to pick them for upstream)

ujfalusi requested review from bardliao, dbaluta, kv2019i, lgirdwood, plbossart and ranj063 as code owners March 13, 2024 13:10

plbossart reviewed Mar 13, 2024

View reviewed changes

ujfalusi commented Mar 13, 2024

View reviewed changes

ujfalusi force-pushed the peter/sof/pr/ipc4_delay_reporting_batch_02 branch from 126bd23 to da9054c Compare March 13, 2024 13:43

ASoC: SOF: Intel: hda-pcm: Use dsp_buffer_time_ms to place constraint

da9054c

If the PCM have the dsp_buffer_time_ms set then place a constraint to limit the minimum period time to avoid smaller period sizes than the DMA burst. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

plbossart reviewed Mar 13, 2024

View reviewed changes

kv2019i approved these changes Mar 13, 2024

View reviewed changes

ranj063 reviewed Mar 13, 2024

View reviewed changes

ujfalusi closed this Mar 15, 2024

ujfalusi mentioned this pull request Mar 15, 2024

ASoC: SOF: ipc4/Intel: Improve pcm delay reporting #4851

Merged

ujfalusi deleted the peter/sof/pr/ipc4_delay_reporting_batch_02 branch December 13, 2024 08:02

ASoC: SOF: ipc4/Intel: Place constraint on the PERIOD_TIME based on the DSP host buffer size (which is provided in ms) fro playback #4858

ASoC: SOF: ipc4/Intel: Place constraint on the PERIOD_TIME based on the DSP host buffer size (which is provided in ms) fro playback #4858

Uh oh!

Conversation

ujfalusi commented Mar 13, 2024

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!

Uh oh!

ujfalusi left a comment • 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!

Uh oh!

ujfalusi commented Mar 13, 2024

Uh oh!

plbossart Mar 13, 2024 • 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!

ujfalusi Mar 13, 2024 • 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!

ranj063 Mar 13, 2024 • 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!

kv2019i 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!

ujfalusi Mar 14, 2024 • 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!

ujfalusi commented Mar 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ujfalusi left a comment •

edited

Loading

plbossart Mar 13, 2024 •

edited

Loading

ujfalusi Mar 13, 2024 •

edited

Loading

ranj063 Mar 13, 2024 •

edited

Loading

ujfalusi Mar 14, 2024 •

edited

Loading