Skip to content

testbench: improvements for valgrind and debug#4408

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

Merged
lgirdwood merged 5 commits into from
Jun 29, 2021
Merged

testbench: improvements for valgrind and debug #4408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
29453c3
testbench: squash some valgrind warnings.
lrgirdwo Jun 26, 2021
5c27ab0
testbench: add more helpful output for options parsing.
lrgirdwo Jun 26, 2021
a44ddb6
testbench: free components and pipeline after test results.
lrgirdwo Jun 26, 2021
ab08c34
testbench: fix crash when file has no . extension
lrgirdwo Jun 26, 2021
65c2b3e
testbench: add a option for specifying the number of copy() iterations
lrgirdwo Jun 26, 2021
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
34 changes: 33 additions & 1 deletion tools/testbench/common_test.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -56,6 +56,38 @@ int tb_pipeline_setup(struct sof *sof)
return 0;
}

struct ipc_data {
struct ipc_data_host_buffer dh_buffer;
};

void tb_pipeline_free(struct sof *sof)
{
struct schedule_data *sch;
struct schedulers **schedulers;
struct list_item *slist, *_slist;
struct notify **notify = arch_notify_get();
struct ipc_data *iipc;

free(sof->sa);
free(*notify);

/* free all scheduler data */
schedule_free();
schedulers = arch_schedulers_get();
list_for_item_safe(slist, _slist, &(*schedulers)->list) {
sch = container_of(slist, struct schedule_data, list);
free(sch);
}
free(*arch_schedulers_get());

/* free IPC data */
iipc = sof->ipc->private;
free(sof->ipc->comp_data);
free(iipc->dh_buffer.page_table);
free(iipc);
free(sof->ipc);
}

/* set up pcm params, prepare and trigger pipeline */
int tb_pipeline_start(struct ipc *ipc, struct pipeline *p,
struct testbench_prm *tp)
Expand DownExpand Up@@ -100,7 +132,7 @@ int tb_pipeline_params(struct ipc *ipc, struct pipeline *p,
{
struct ipc_comp_dev *pcm_dev;
struct comp_dev *cd;
struct sof_ipc_pcm_params params;
struct sof_ipc_pcm_params params = {0};

@marc-hbmarc-hbJun 28, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks like the initialization of some field(s) is missing below and this is merely hiding the issue.

@lgirdwoodlgirdwoodJun 28, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

ditto as above.

char message[DEBUG_MSG_LEN];
int fs_period;
int period;
Expand Down
3 changes: 3 additions & 0 deletions tools/testbench/file.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -407,6 +407,9 @@ static enum file_format get_file_format(char *filename)
{
char *ext = strrchr(filename, '.');

if (!ext)
return FILE_RAW;

if (!strcmp(ext, ".txt"))
return FILE_TEXT;

Expand Down
4 changes: 4 additions & 0 deletions tools/testbench/include/testbench/common_test.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -8,6 +8,7 @@

#include <stdint.h>
#include <stddef.h>
#include <stdbool.h>
#include <time.h>
#include <stdio.h>
#include <sof/sof.h>
Expand DownExpand Up@@ -45,6 +46,8 @@ struct testbench_prm {
int sched_id;
int max_pipeline_id;
enum sof_ipc_frame frame_fmt;
int copy_iterations;
bool copy_check;
};

struct shared_lib_table {
Expand All@@ -67,6 +70,7 @@ void sys_comp_file_init(void);
void sys_comp_filewrite_init(void);

int tb_pipeline_setup(struct sof *sof);
void tb_pipeline_free(struct sof *sof);

int tb_pipeline_start(struct ipc *ipc, struct pipeline *p,
struct testbench_prm *tp);
Expand Down
71 changes: 50 additions & 21 deletions tools/testbench/testbench.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -113,9 +113,8 @@ static int parse_libraries(char *libs)

/* get shared library index from library table */
index = get_index_by_name(token1, lib_table);

if (index < 0) {
fprintf(stderr, "error: unsupported comp type\n");
fprintf(stderr, "error: unsupported comp type %s\n", token1);
return -EINVAL;
}

Expand DownExpand Up@@ -146,6 +145,7 @@ static void print_usage(char *executable)
printf("%s -i in.txt -o out.txt -t test.tplg ", executable);
printf("-r 48000 -R 96000 -c 2");
printf("-b S16_LE -a vol=libsof_volume.so\n");
printf("-C number of copy() iterations\n");
}

/* free components */
Expand All@@ -159,20 +159,13 @@ static void free_comps(void)
icd = container_of(clist, struct ipc_comp_dev, list);
switch (icd->type) {
case COMP_TYPE_COMPONENT:
comp_free(icd->cd);
list_item_del(&icd->list);
rfree(icd);
ipc_comp_free(sof_get()->ipc, icd->id);
break;
case COMP_TYPE_BUFFER:
rfree(icd->cb->stream.addr);
rfree(icd->cb);
list_item_del(&icd->list);
rfree(icd);
ipc_buffer_free(sof_get()->ipc, icd->id);
break;
default:
rfree(icd->pipeline);
list_item_del(&icd->list);
rfree(icd);
ipc_pipeline_free(sof_get()->ipc, icd->id);
break;
}
}
Expand All@@ -183,7 +176,7 @@ static void parse_input_args(int argc, char **argv, struct testbench_prm *tp)
int option = 0;
int ret = 0;

while ((option = getopt(argc, argv, "hdi:o:t:b:a:r:R:c:")) != -1) {
while ((option = getopt(argc, argv, "hdi:o:t:b:a:r:R:c:C:")) != -1) {
switch (option) {
/* input sample file */
case 'i':
Expand DownExpand Up@@ -231,9 +224,17 @@ static void parse_input_args(int argc, char **argv, struct testbench_prm *tp)
debug = 1;
break;

/* number of pipeline copy() iterations */
case 'C':
tp->copy_iterations = atoi(optarg);
tp->copy_check = true;
break;

/* print usage */
case 'h':
default:
fprintf(stderr, "unknown option %c\n", option);
__attribute__ ((fallthrough));
case 'h':
print_usage(argv[0]);
exit(EXIT_FAILURE);
}
Expand DownExpand Up@@ -268,13 +269,32 @@ int main(int argc, char **argv)
tp.output_file_num = 0;
tp.channels = TESTBENCH_NCH;
tp.max_pipeline_id = 0;
tp.copy_check = false;

/* command line arguments*/
parse_input_args(argc, argv, &tp);

/* check args */
if (!tp.tplg_file || !tp.input_file || !tp.output_file_num ||
!tp.bits_in) {
/* check mandatory args */
if (!tp.tplg_file) {
fprintf(stderr, "topology file not specified, use -t file.tplg\n");
print_usage(argv[0]);
exit(EXIT_FAILURE);
}

if (!tp.input_file) {
fprintf(stderr, "input audio file not specified, use -i file\n");
print_usage(argv[0]);
exit(EXIT_FAILURE);
}

if (!tp.output_file_num) {
fprintf(stderr, "output files not specified, use -o file1,file2\n");
print_usage(argv[0]);
exit(EXIT_FAILURE);
}

if (!tp.bits_in) {
fprintf(stderr, "input format not specified, use -b format\n");
print_usage(argv[0]);
exit(EXIT_FAILURE);
}
Expand DownExpand Up@@ -346,9 +366,15 @@ int main(int argc, char **argv)
pipeline_schedule_copy(curr_p, 0);
}
}

/* are we bailing out after a fixed number of iterations ? */
if (tp.copy_check) {
if (--tp.copy_iterations == 0)
break;
}
}

if (!frcd->fs.reached_eof)
if (!frcd->fs.reached_eof && !tp.copy_check)
printf("warning: possible pipeline xrun\n");

/* reset and free pipeline */
Expand All@@ -366,9 +392,6 @@ int main(int argc, char **argv)
t_exec = (double)(toc - tic) / CLOCKS_PER_SEC;
c_realtime = (double)n_out / tp.channels / tp.fs_out / t_exec;

/* free all components/buffers in pipeline */
free_comps();

/* print test summary */
printf("==========================================================\n");
printf(" Test Summary\n");
Expand All@@ -387,6 +410,12 @@ int main(int argc, char **argv)
printf("Total execution time: %.2f us, %.2f x realtime\n",
1e3 * t_exec, c_realtime);

/* free all components/buffers in pipeline */
free_comps();

/* free other core FW services */
tb_pipeline_free(sof_get());

/* free all other data */
free(tp.bits_in);
free(tp.input_file);
Expand Down
2 changes: 1 addition & 1 deletion tools/testbench/topology.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -137,7 +137,7 @@ int load_buffer(void *dev, int comp_id, int pipeline_id,
struct snd_soc_tplg_dapm_widget *widget)
{
struct sof *sof = (struct sof *)dev;
struct sof_ipc_buffer buffer;
struct sof_ipc_buffer buffer = {0};

@marc-hbmarc-hbJun 28, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Isn't the job of tplg_load_buffer() to initialize buffer? What error does this silence?

@lgirdwoodlgirdwoodJun 28, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

testbench does not use all the data members as some are specific to HW config. This was only missing from the two "IPCs" in this PR.

@marc-hbmarc-hbJun 28, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't understand sorry, I don't have the full picture. What was the error reported and in which function?

@lgirdwoodlgirdwoodJun 28, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Use of uninitialized data in FW buffer logic due to testbench not initializing it. Not an issue with kernel as it clears. Testbench bug.

@marc-hbmarc-hbJun 28, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I understood that the error was testbench specific, my concern is that this change hides any "real" initialization bug in tplg_load_buffer in the future.

@lgirdwoodlgirdwoodJun 28, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Got you now, yep the userspace tplg parser is not setting any variables if it cant find tokens for them. I will fix this as another PR as it means removing more testbench code here and also testing other tools that used the shared tplg parser.

int size = widget->priv.size;
int ret;

Expand Down