Skip to content
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

ASoC: SOF: ipc4-topology: Introduce new token for period size #4664

Closed
wants to merge 1 commit into from
Closed

ASoC: SOF: ipc4-topology: Introduce new token for period size #4664

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
3 changes: 3 additions & 0 deletions include/uapi/sound/sof/tokens.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,4 +213,7 @@
#define SOF_TKN_AMD_ACPI2S_CH 1701
#define SOF_TKN_AMD_ACPI2S_TDM_MODE 1702

/* Process */
#define SOF_TKN_PROCESS_BUFFER_PERIOD_SIZE 2000

#endif
22 changes: 22 additions & 0 deletions sound/soc/sof/ipc4-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,12 @@ static const struct sof_topology_token src_tokens[] = {
offsetof(struct sof_ipc4_src_data, sink_rate)},
};

/* Process tokens */
static const struct sof_topology_token process_tokens[] = {
{SOF_TKN_PROCESS_BUFFER_PERIOD_SIZE, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32,
offsetof(struct sof_ipc4_process, buffer_period_size)},
};

static const struct sof_token_info ipc4_token_list[SOF_TOKEN_COUNT] = {
[SOF_DAI_TOKENS] = {"DAI tokens", dai_tokens, ARRAY_SIZE(dai_tokens)},
[SOF_PIPELINE_TOKENS] = {"Pipeline tokens", pipeline_tokens, ARRAY_SIZE(pipeline_tokens)},
Expand All @@ -165,6 +171,7 @@ static const struct sof_token_info ipc4_token_list[SOF_TOKEN_COUNT] = {
ipc4_audio_fmt_num_tokens, ARRAY_SIZE(ipc4_audio_fmt_num_tokens)},
[SOF_GAIN_TOKENS] = {"Gain tokens", gain_tokens, ARRAY_SIZE(gain_tokens)},
[SOF_SRC_TOKENS] = {"SRC tokens", src_tokens, ARRAY_SIZE(src_tokens)},
[SOF_PROCESS_TOKENS] = {"Process tokens", process_tokens, ARRAY_SIZE(process_tokens)},
};

struct snd_sof_widget *sof_ipc4_find_swidget_by_ids(struct snd_sof_dev *sdev,
Expand Down Expand Up @@ -906,6 +913,15 @@ static int sof_ipc4_widget_setup_comp_process(struct snd_sof_widget *swidget)
if (ret)
goto err;

/* set default period size to 1 and override it with the topology value if set */
process->buffer_period_size = 1;
ret = sof_update_ipc_object(scomp, process, SOF_PROCESS_TOKENS, swidget->tuples,
swidget->num_tuples, sizeof(*process), 1);
if (ret) {
dev_err(scomp->dev, "parse process tokens failed %d\n", ret);
goto err;
}

/* parse process init module payload config type from module info */
fw_module = swidget->module_info;
process->init_config = FIELD_GET(SOF_IPC4_MODULE_INIT_CONFIG_MASK,
Expand Down Expand Up @@ -2012,6 +2028,7 @@ sof_ipc4_process_set_pin_formats(struct snd_sof_widget *swidget, int pin_type)

if (pin_format_item->pin_index == i - pin_format_offset) {
*pin_format = *pin_format_item;
pin_format->buffer_size *= process->buffer_period_size;
Copy link
Member

Choose a reason for hiding this comment

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

naming is awful. suggest "buffer_size_multiplier". It doesn't help to refer to the period and is't not a buffer or period_size in the first place.

Copy link
Author

Choose a reason for hiding this comment

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

Ack, will update.

break;
}
}
Expand Down Expand Up @@ -2075,6 +2092,10 @@ static int sof_ipc4_prepare_process_module(struct snd_sof_widget *swidget,
return output_fmt_index;
}

/* adjust the input/output buffer size based on the period size set in topology */
process->base_config.ibs *= process->buffer_period_size;
process->base_config.obs *= process->buffer_period_size;
Copy link
Member

Choose a reason for hiding this comment

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

is this correct to multiply the pin_format->buffer_size and ibs/obs? Isn't the pin_format->buffer_size derived from IBS/OBS?

Looks like a double multiplication to me...

Copy link
Author

Choose a reason for hiding this comment

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

Hi, @RanderWang , could you help to double check above? sorry I am not really familiar this part.

Copy link

Choose a reason for hiding this comment

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

this change should be made in topology since driver just pass it to fw.
please check audio_format.conf. you can override it for DP module

      # math expression for computing input/put buffer sizes
        # for 11.025 22.05, 44.1, 88.2 and 176.4khz, we need to round it to ceiling value
        ibs "$[($in_channels * ($[($in_rate + 999)] / 1000)) * ($in_bit_depth / 8)]"
        obs "$[($out_channels * ($[($out_rate + 999)] / 1000)) * ($out_bit_depth / 8)]"

Copy link

Choose a reason for hiding this comment

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

And if the process->buffer_period_size is used to build IBS or OBS only, we can do it in topology and no need to change driver ?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @RanderWang , updated the thesofproject/sof#8403, hope to have same net results :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR can be closed then?


/* copy Pin 0 output format */
if (available_fmt->num_output_formats &&
output_fmt_index < available_fmt->num_output_formats &&
Expand Down Expand Up @@ -3042,6 +3063,7 @@ static enum sof_tokens process_token_list[] = {
SOF_IN_AUDIO_FORMAT_TOKENS,
SOF_OUT_AUDIO_FORMAT_TOKENS,
SOF_COMP_EXT_TOKENS,
SOF_PROCESS_TOKENS,
};

static const struct sof_ipc_tplg_widget_ops tplg_ipc4_widget_ops[SND_SOC_DAPM_TYPE_COUNT] = {
Expand Down
1 change: 1 addition & 0 deletions sound/soc/sof/ipc4-topology.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,7 @@ struct sof_ipc4_process {
struct sof_ipc4_msg msg;
u32 base_config_ext_size;
u32 init_config;
u32 buffer_period_size;
Copy link
Member

Choose a reason for hiding this comment

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

commit message: you're describing a solution without explaining what the issue is. Why do we need this in the first place? In what context does this help?

Copy link
Author

Choose a reason for hiding this comment

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

This is intended to handle DP module that requires larger buffer than 1ms.

Copy link

@marcinszkudlinski marcinszkudlinski Oct 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

DP modules scheduling is based on their IBS and OBS

In general:

  1. the module is ready to run and will be scheduled if it has at least IBS number of bytes available on each input pin and at least OBS number of bytes free on each output pin
    please note that each pin may have its own format - number of channels, sampling frequency, etc. so each pin may have its own IBS/OBS

IBS and OBS are as declared in module bind IPC.

  1. calculation of module period - required for synchronization with LL modules and EDF scheduling - is also based on IBS(IBSes)

special case like variable bitrate: the module may override both 1 and 2 if really needed.

so - for DP modules IBS/OBS values should be precise, it is critical for proper data flow.

example: if a module has 2 input pins, one 16000/16/2, second 48000/16/2 and one output pin 48000/32/4 and is designed to process 10ms data chunks:

in pin1 IBS should be 16(samples/1ms) * 2 (bytes per sample) * 2 (channels) * 10(ms) = 640
in pin2 IBS should be 48(samples/1ms) * 2 (bytes per sample) * 2 (channels) * 10(ms) = 1920
out pin1 OBS should be 48(samples/1ms) * 4 (bytes per sample) * 4 (channels) * 10(ms) = 7680

another note - bigger OBS won't hurt the processing but is a memory waste - so should be avoided.

};

bool sof_ipc4_copier_is_single_format(struct snd_sof_dev *sdev,
Expand Down