Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ujfalusi wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a37bd1b
ASoC: SOF: Add dsp_buffer_time_ms member to snd_sof_pcm_stream
ujfalusi Mar 8, 2024
b75fef1
ASoC: SOF: ipc4-topology: Save the DMA buffer time for playback PCMs
ujfalusi Mar 8, 2024
da9054c
ASoC: SOF: Intel: hda-pcm: Use dsp_buffer_time_ms to place constraint
ujfalusi Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions sound/soc/sof/intel/hda-pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,18 @@ int hda_dsp_pcm_open(struct snd_sof_dev *sdev,
snd_pcm_hw_constraint_mask64(substream->runtime, SNDRV_PCM_HW_PARAM_FORMAT,
SNDRV_PCM_FMTBIT_S16 | SNDRV_PCM_FMTBIT_S32);

/*
Copy link
Collaborator

@ranj063 ranj063 Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@ujfalusi ujfalusi Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* 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.
Copy link
Collaborator

@kv2019i kv2019i Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
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,
Copy link
Collaborator

@kv2019i kv2019i Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@ujfalusi ujfalusi Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

UINT_MAX);

Copy link
Collaborator

@ranj063 ranj063 Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator Author

@ujfalusi ujfalusi Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@ujfalusi ujfalusi Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@plbossart plbossart Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

/* binding pcm substream to hda stream */
substream->runtime->private_data = &dsp_stream->hstream;
return 0;
Expand Down
16 changes: 15 additions & 1 deletion sound/soc/sof/ipc4-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,8 +412,9 @@ static int sof_ipc4_widget_setup_pcm(struct snd_sof_widget *swidget)
struct sof_ipc4_available_audio_format *available_fmt;
struct snd_soc_component *scomp = swidget->scomp;
struct sof_ipc4_copier *ipc4_copier;
struct snd_sof_pcm *spcm;
int node_type = 0;
int ret;
int ret, dir;

ipc4_copier = kzalloc(sizeof(*ipc4_copier), GFP_KERNEL);
if (!ipc4_copier)
Expand Down Expand Up @@ -447,6 +448,19 @@ static int sof_ipc4_widget_setup_pcm(struct snd_sof_widget *swidget)
}
dev_dbg(scomp->dev, "host copier '%s' node_type %u\n", swidget->widget->name, node_type);

spcm = snd_sof_find_spcm_comp(scomp, swidget->comp_id, &dir);
if (spcm && dir == SNDRV_PCM_STREAM_PLAYBACK) {
struct snd_sof_pcm_stream *sps = &spcm->stream[dir];

sof_update_ipc_object(scomp, &sps->dsp_buffer_time_ms,
SOF_COPIER_DEEP_BUFFER_TOKENS,
swidget->tuples,
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;
Copy link
Member

@plbossart plbossart Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@ujfalusi ujfalusi Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@plbossart plbossart Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@ujfalusi ujfalusi Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator Author

@ujfalusi ujfalusi Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@plbossart plbossart Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@ranj063 ranj063 Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator Author

@ujfalusi ujfalusi Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@ujfalusi ujfalusi Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

}

skip_gtw_cfg:
ipc4_copier->gtw_attr = kzalloc(sizeof(*ipc4_copier->gtw_attr), GFP_KERNEL);
if (!ipc4_copier->gtw_attr) {
Expand Down
1 change: 1 addition & 0 deletions sound/soc/sof/sof-audio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ struct snd_sof_pcm_stream {
struct work_struct period_elapsed_work;
struct snd_soc_dapm_widget_list *list; /* list of connected DAPM widgets */
bool d0i3_compatible; /* DSP can be in D0I3 when this pcm is opened */
unsigned int dsp_buffer_time_ms; /* The size of the DSP pcm buffer in ms */
/*
* flag to indicate that the DSP pipelines should be kept
* active or not while suspending the stream
Expand Down