Skip to content

copier: pipeline period recalculation #7626

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
tmleman wants to merge 1 commit into from
Closed

copier: pipeline period recalculation #7626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 36 additions & 0 deletions src/audio/copier/copier.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include <sof/audio/dai_copier.h>
#include <sof/audio/ipcgtw_copier.h>
#include <sof/audio/module_adapter/module/generic.h>
#include <sof/schedule/ll_schedule_domain.h>

#if CONFIG_ZEPHYR_NATIVE_DRIVERS
#include <zephyr/drivers/dai.h>
Expand Down Expand Up @@ -92,6 +93,41 @@ static int copier_init(struct processing_module *mod)
}

dev->pipeline = ipc_pipe->pipeline;
/* Calculation of the period in which the component should be scheduled based on
* the input buffer size.
*/
if (copier->gtw_cfg.dma_buffer_size > 0 && dev->pipeline->deep_buffering) {
/* Size of the single frame in bytes. */
uint32_t frame_size = copier->base.audio_fmt.channels_count *
(copier->base.audio_fmt.depth >> 3);
/* Size of the one second of audio data in bytes. */
uint32_t one_s = frame_size * copier->base.audio_fmt.sampling_frequency;
/* Size of the ten millisecond of audio data in bytes. */
uint32_t ten_ms = DIV_ROUND_UP(one_s, 100);
/* The number of ten milliseconds chunks of data that will fit into the buffer. */
uint32_t chunk_count = copier->gtw_cfg.dma_buffer_size / ten_ms;
Copy link
Collaborator

@lyakh lyakh Jun 21, 2023

Choose a reason for hiding this comment

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

just as an example to clarify why this won't quite calculate correct results and why you need to round up line 114:

one_s = 8090
ten_ms = 80
dma_buffer_size = 8000
chunk_count = 100

that will tell you, that you can fit 100 10ms long chunks in the buffer, so you could conclude that a full second of audio fits into the buffer, whereas it doesn't - 90 bytes don't fit. Whereas if you round up the first division in line 114, you get

one_s = 8090
ten_ms = 81
dma_buffer_size = 8000
chunk_count = 98

so you conclude, that 980ms of audio will fit into the buffer, which is correct - they will fit there and will occupy 0.98*8090 < 7983 < 8000 bytes.

Copy link
Contributor

@btian1 btian1 Jun 29, 2023

Choose a reason for hiding this comment

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

may need enumerate all the possible freqs and then divide can be avoided by look up table.
like is only support 44100 and 48000 input, then two divide can be avoided.

Copy link
Contributor Author

@tmleman tmleman Aug 1, 2023

Choose a reason for hiding this comment

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

I assumed that rounding is not needed because none of the formats we use will result in a value that is not divisible by 100. I added rounding anyway.

Copy link
Member

@lgirdwood lgirdwood Aug 2, 2023

Choose a reason for hiding this comment

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

Yeah, I dont think there are any rates that are not multiples of 100.

Copy link
Collaborator

@RanderWang RanderWang Aug 8, 2023

Choose a reason for hiding this comment

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

so we will not support 11.025 khz and 22.05 khz ?

enum ipc4_sampling_frequency {
	IPC4_FS_8000HZ = 8000,
	IPC4_FS_11025HZ = 11025,
	IPC4_FS_12000HZ = 12000, /**< Mp3, AAC, SRC only */
	IPC4_FS_16000HZ = 16000,
	IPC4_FS_18900HZ = 18900, /**< SRC only for 44100 */
	IPC4_FS_22050HZ = 22050,
	IPC4_FS_24000HZ = 24000, /**< Mp3, AAC, SRC only */
	IPC4_FS_32000HZ = 32000,
	IPC4_FS_37800HZ = 37800, /**< SRC only for 44100 */
	IPC4_FS_44100HZ = 44100,
	IPC4_FS_48000HZ = 48000, /**< Default */
	IPC4_FS_64000HZ = 64000, /**< AAC, SRC only */
	IPC4_FS_88200HZ = 88200, /**< AAC, SRC only */
	IPC4_FS_96000HZ = 96000, /**< AAC, SRC only */
	IPC4_FS_176400HZ = 176400, /**< SRC only */
	IPC4_FS_192000HZ = 192000, /**< SRC only */
	IPC4_FS_INVALID
};

Copy link
Member

@lgirdwood lgirdwood Aug 9, 2023

Choose a reason for hiding this comment

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

oh, good point @RanderWang - these are rare but in IPC4 and ALSA.
@tmleman looks like we need a switch here to deal with the non divisible corner cases.


/* If buffer can fit at more than ten milliseconds of data we can try to schedule
* pipe on periods bigger than one millisecond.
*/
if (chunk_count) {
Copy link
Collaborator

@lyakh lyakh Jun 21, 2023

Choose a reason for hiding this comment

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

so in my above example if buffer size is 80 bytes, you'll calculate chunk_count == 1 but in fact it won't fit full 10ms of audio

Copy link
Member

@lgirdwood lgirdwood Aug 2, 2023

Choose a reason for hiding this comment

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

Isn't buffer size validated prior to this logic ? @tmleman do we validate copier->base.audio_fmt.sampling_frequency, copier->base.audio_fmt.depth or copier->base.audio_fmt.channels_count to be non zero here otherwise we can divide by 0.

dev->deep_buffering = true;
Copy link
Contributor

@btian1 btian1 Jun 29, 2023

Choose a reason for hiding this comment

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

dev->pipeline->long_period same meaning with dev->deep_buffering

if (chunk_count == 1)
dev->period = (5 * LL_TIMER_PERIOD_US);
else
dev->period = (10 * LL_TIMER_PERIOD_US);

Copy link
Collaborator

@kv2019i kv2019i Jun 21, 2023

Choose a reason for hiding this comment

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

With so many inputs (both from runtime PCM config as well as topology), I think this warrants printing out the modified period size to FW log.

Copy link
Member

@lgirdwood lgirdwood Aug 2, 2023

Choose a reason for hiding this comment

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

ack, would be handy for debug.

/* If a component that may run on bigger periods has already been added to
* the pipeline, the pipe must run at the lowest possible value.
*/
if (!dev->pipeline->period || dev->pipeline->period < dev->period) {
dev->pipeline->period = dev->period;
comp_cl_warn(&comp_copier,
"changing the period for the pipe (new value %u)",
dev->pipeline->period);
}
}
}

node_id = copier->gtw_cfg.node_id;
/* copier is linked to gateway */
Expand Down
9 changes: 9 additions & 0 deletions src/audio/pipeline/pipeline-graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <sof/lib/uuid.h>
#include <sof/compiler_attributes.h>
#include <sof/list.h>
#include <sof/schedule/ll_schedule_domain.h>
#include <rtos/spinlock.h>
#include <rtos/string.h>
#include <rtos/clk.h>
Expand Down Expand Up @@ -302,6 +303,14 @@ int pipeline_complete(struct pipeline *p, struct comp_dev *source,
data.start = source;
data.p = p;

/* If a component has been added to the pipeline that cannot run on longer periods
* we need to set pipe period to one ms.
*/
if (!p->deep_buffering) {
p->period = LL_TIMER_PERIOD_US;
pipe_info(p, "setting pipe period to %llu us", LL_TIMER_PERIOD_US);
}

/* now walk downstream from source component and
* complete component task and pipeline initialization
*/
Expand Down
3 changes: 3 additions & 0 deletions src/include/sof/audio/component.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,9 @@ struct comp_dev {
bool is_shared; /**< indicates whether component is shared
* across cores
*/
bool deep_buffering; /**< indicates whether component is able to work
* deep buffering
*/
struct comp_ipc_config ipc_config; /**< Component IPC configuration */
struct tr_ctx tctx; /**< trace settings */

Expand Down
2 changes: 2 additions & 0 deletions src/include/sof/audio/pipeline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ struct pipeline {
bool aborted; /* STOP or PAUSE failed, stay active */
bool pending; /* trigger scheduled but not executed yet */
} trigger;
/* pipe can use long periods for scheduling in case of a deep buffering */
bool deep_buffering;
};

struct pipeline_walk_context {
Expand Down
21 changes: 19 additions & 2 deletions src/ipc/ipc4/helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include <sof/lib/mailbox.h>
#include <sof/list.h>
#include <sof/platform.h>
#include <sof/schedule/ll_schedule_domain.h>
#include <rtos/wait.h>

/* TODO: Remove platform-specific code, see https://github.com/thesofproject/sof/issues/7549 */
Expand Down Expand Up @@ -206,7 +205,8 @@ static int ipc4_create_pipeline(struct ipc4_pipeline_create *pipe_desc)
}

pipe->time_domain = SOF_TIME_DOMAIN_TIMER;
pipe->period = LL_TIMER_PERIOD_US;
pipe->period = 0;
pipe->deep_buffering = true;

/* sched_id is set in FW so initialize it to a invalid value */
pipe->sched_id = 0xFFFFFFFF;
Expand Down Expand Up @@ -869,6 +869,23 @@ int ipc4_add_comp_dev(struct comp_dev *dev)
/* add new component to the list */
list_item_append(&icd->list, &ipc->comp_list);

/* If new component added to the pipeline is not fitted for deep buffering we need to
* inform pipeline it cannot work on long periods.
*/
if (!dev->deep_buffering) {
if (!dev->pipeline) {
const uint32_t pipe_id = dev->ipc_config.pipeline_id;
struct ipc_comp_dev *ipc_comp =
ipc_get_comp_by_ppl_id(ipc,
COMP_TYPE_PIPELINE,
pipe_id);
Copy link
Collaborator

@lyakh lyakh Aug 15, 2023

Choose a reason for hiding this comment

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

are we sure, that ipc_comp cannot be NULL here? pipe_id comes from an IPC... BTW, looks like ipc4_add_comp_dev() can be static


dev->pipeline = ipc_comp->pipeline;
}

dev->pipeline->deep_buffering = false;
}

return IPC4_SUCCESS;
};

Expand Down