Skip to content

stop all connected pipelines on XRUNs #4562

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 3 commits into from
Sep 7, 2021
Merged

stop all connected pipelines on XRUNs #4562

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
ee4a550
zephyr: (cosmetic) fix indentation
lyakh Sep 1, 2021
9e30120
host: (cosmetic) simplify function return flow
lyakh Jul 20, 2021
fc57e8d
pipeline: stop all connected pipelines on xrun
lyakh Jul 20, 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
8 changes: 3 additions & 5 deletions src/audio/host.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ static int host_copy_normal(struct comp_dev *dev)
struct host_data *hd = comp_get_drvdata(dev);
uint32_t copy_bytes = 0;
uint32_t flags = 0;
int ret = 0;
int ret;

comp_dbg(dev, "host_copy_normal()");

Expand All @@ -408,13 +408,11 @@ static int host_copy_normal(struct comp_dev *dev)

copy_bytes = host_get_copy_bytes_normal(dev);
if (!copy_bytes)
return ret;
Copy link
Contributor

@mrajwa mrajwa Jul 28, 2021

Choose a reason for hiding this comment

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

Hmm I like to return "ret" more TBH this way we are consistent of always returning the same variable and not magic numbers.

Copy link
Collaborator Author

@lyakh lyakh Jul 28, 2021

Choose a reason for hiding this comment

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

I usually approach this firstly from the PoV of minimalism, secondly of locality. You don't do

int f(void)
{
	int ret = 0;
	return ret;
}

you just do

int f(void)
{
	return 0;
}

I think this is similar. Locality - when I look at that line I immediately know what happens there. With the previous version I have to check 9 lines up to see where ret comes from and whether it was modified in between. So, sorry, no, I prefer this version.

Copy link
Contributor

@mrajwa mrajwa Jul 30, 2021

Choose a reason for hiding this comment

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

Both approaches can be considered better or worse... I think we should create a macro or enum which maps 0 with some meaningful success constant like in CAVS we have ADSP_SUCCESS. Anyway, if you feel like returning 0 instead of defined ret is more appropriate then OK, this is not critically important tbh.

return 0;

ret = dma_copy(hd->chan, copy_bytes, flags);
if (ret < 0) {
if (ret < 0)
comp_err(dev, "host_copy_normal(): dma_copy() failed, ret = %u", ret);
return ret;
}

return ret;
}
Expand Down
11 changes: 11 additions & 0 deletions src/audio/pipeline/pipeline-schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,17 @@ void pipeline_schedule_triggered(struct pipeline_walk_context *ctx,
}
pipeline_schedule_copy(p, 0);
}
break;
case COMP_TRIGGER_XRUN:
list_for_item(tlist, &ctx->pipelines) {
p = container_of(tlist, struct pipeline, list);
if (!p->xrun_bytes)
/*
* the exact number of xrun bytes is unused,
* just make it non-0
*/
p->xrun_bytes = 1;
Copy link
Contributor

@mrajwa mrajwa Jul 28, 2021

Choose a reason for hiding this comment

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

magic number

Copy link
Collaborator Author

@lyakh lyakh Jul 28, 2021

Choose a reason for hiding this comment

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

any non-zero should do, the value isn't used anyway. I can add a comment.

Copy link
Contributor

@mrajwa mrajwa Jul 30, 2021

Choose a reason for hiding this comment

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

I see but can't we just assign exact XRUN_BYTES here? Even though currently we treat it as a boolean value at some point we may need exact value - this veriable was defined as 32 bit one so this suggest me that was the original intention.

}
}

irq_local_enable(flags);
Expand Down
52 changes: 51 additions & 1 deletion src/audio/pipeline/pipeline-stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,53 @@ static int pipeline_trigger_list(struct pipeline *p, struct comp_dev *host, int
return ret;
}

static void pipeline_trigger_xrun(struct pipeline *p, struct comp_dev **host)
{
/*
* XRUN can happen on a pipeline, not directly attached to the host,
* find the original one
*/
do {
/* Check the opposite direction */
int dir = (*host)->direction == PPL_DIR_DOWNSTREAM ? PPL_DIR_UPSTREAM :
PPL_DIR_DOWNSTREAM;
struct list_item *buffer_list = comp_buffer_list(*host, dir);
struct list_item *clist;
bool found = false;

if (list_is_empty(buffer_list))
/* Reached the original host */
break;

list_for_item(clist, buffer_list) {
struct comp_buffer *buffer = buffer_from_list(clist,
struct comp_buffer, dir);
struct comp_dev *buffer_comp = buffer_get_comp(buffer, dir);

switch (buffer_comp->pipeline->status) {
case COMP_STATE_ACTIVE:
case COMP_STATE_PREPARE:
found = true;
break;
}

if (found) {
*host = (*host)->direction == PPL_DIR_DOWNSTREAM ?
buffer_comp->pipeline->source_comp :
buffer_comp->pipeline->sink_comp;
break;
}
}

if (!found) {
/* No active pipeline found! Should never occur. */
pipe_err(p, "No active pipeline found to link to pipeline %u!",
(*host)->pipeline->pipeline_id);
break;
}
} while (true);
}

/* trigger pipeline in IPC context */
int pipeline_trigger(struct pipeline *p, struct comp_dev *host, int cmd)
{
Expand All @@ -268,9 +315,12 @@ int pipeline_trigger(struct pipeline *p, struct comp_dev *host, int cmd)
/* Execute immediately */
ret = pipeline_trigger_run(p, host, cmd);
return ret == PPL_STATUS_PATH_STOP ? 0 : ret;
case COMP_TRIGGER_XRUN:
pipeline_trigger_xrun(p, &host);
COMPILER_FALLTHROUGH;
case COMP_TRIGGER_PRE_RELEASE:
case COMP_TRIGGER_PRE_START:
/* Add all connected pipelines to the list and schedule them all */
/* Add all connected pipelines to the list and trigger them all */
ret = pipeline_trigger_list(p, host, cmd);
if (ret < 0)
return ret;
Expand Down
2 changes: 1 addition & 1 deletion src/audio/pipeline/pipeline-xrun.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ void pipeline_xrun(struct pipeline *p, struct comp_dev *dev,
return;

/* notify all pipeline comps we are in XRUN, and stop copying */
ret = pipeline_trigger_run(p, p->source_comp, COMP_TRIGGER_XRUN);
ret = pipeline_trigger(p, p->source_comp, COMP_TRIGGER_XRUN);
if (ret < 0)
pipe_err(p, "pipeline_xrun(): Pipelines notification about XRUN failed, ret = %d",
ret);
Expand Down
4 changes: 2 additions & 2 deletions src/include/sof/audio/component_ext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ struct comp_driver_list {

/** \brief Retrieves the component device buffer list. */
#define comp_buffer_list(comp, dir) \
((dir) == PPL_DIR_DOWNSTREAM ? &comp->bsink_list : \
&comp->bsource_list)
((dir) == PPL_DIR_DOWNSTREAM ? &(comp)->bsink_list : \
&(comp)->bsource_list)

/** See comp_ops::new */
struct comp_dev *comp_new(struct sof_ipc_comp *comp);
Expand Down
4 changes: 2 additions & 2 deletions zephyr/edf_schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ static int schedule_edf_task(void *data, struct task *task, uint64_t start,
k_timeout_t start_time = K_USEC(start + EDF_SCHEDULE_DELAY);

k_work_reschedule_for_queue(&edf_workq,
&task->z_delayed_work,
start_time);
&task->z_delayed_work,
start_time);

task->state = SOF_TASK_STATE_QUEUED;
return 0;
Expand Down