Skip to content

ASoC: SOF: Intel: sort pipeline based on priority #4593

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
ranj063 merged 2 commits into from
Oct 3, 2023
Merged

ASoC: SOF: Intel: sort pipeline based on priority #4593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c0770b
ASoC: SOF: IPC4: get pipeline priority from topology
RanderWang Sep 15, 2023
b7d0610
ASoC: SOF: IPC4: sort pipeline based on priority
RanderWang Sep 15, 2023
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
55 changes: 46 additions & 9 deletions sound/soc/sof/ipc4-pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,37 @@ int sof_ipc4_set_pipeline_state(struct snd_sof_dev *sdev, u32 instance_id, u32 s
}
EXPORT_SYMBOL(sof_ipc4_set_pipeline_state);

static void sof_ipc4_add_pipeline_by_priority(struct ipc4_pipeline_set_state_data *trigger_list,
Copy link
Collaborator

@ranj063 ranj063 Sep 26, 2023

Choose a reason for hiding this comment

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

@RanderWang I had a though about this second patch. Instead of sorting the pipelines based on priority in the kernel while ending the IPC, can we handle this in the FW? When the pipeline is set up, the FW is already aware of the priority. We can use that information in the firmware to trigger in decreasing order of priorities, right?

Copy link
Author

@RanderWang RanderWang Sep 27, 2023

Choose a reason for hiding this comment

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

(1) We can do it in FW, but we need some search operation to find the priority in FW. As you know, host X86 is much faster than dsp core. (5GHZ vs 400MHZ , DDR4800 vs DDR400).
(2) According to above discussing, we should fully align with REF implementation which does it in driver and it should be good for REF FW also.

Copy link

@yongzhi1 yongzhi1 Sep 27, 2023

Choose a reason for hiding this comment

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

Verified working fine with randomly start/stop spk playback and DMIC0 RTC AEC.

struct snd_sof_widget *pipe_widget,
s8 *pipe_priority, bool ascend)
{
struct sof_ipc4_pipeline *pipeline = pipe_widget->private;
int i, j;

for (i = 0; i < trigger_list->count; i++) {
/* add pipeline from low priority to high */
if (ascend && pipeline->priority < pipe_priority[i])
break;
/* add pipeline from high priority to low */
else if (!ascend && pipeline->priority > pipe_priority[i])
break;
}

for (j = trigger_list->count - 1; j >= i; j--) {
trigger_list->pipeline_instance_ids[j + 1] = trigger_list->pipeline_instance_ids[j];
pipe_priority[j + 1] = pipe_priority[j];
Copy link
Collaborator

@ujfalusi ujfalusi Sep 29, 2023

Choose a reason for hiding this comment

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

trigger_list->count - 1 + 1 == trigger_list->count
trigger_list->pipeline_instance_ids[trigger_list->count] and pipe_priority[trigger_list->count] is out of bounds access?

Copy link
Collaborator

@ranj063 ranj063 Sep 29, 2023

Choose a reason for hiding this comment

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

i had the same suspicion when I looked at the code. But the array size is pipeline_list->count not trigger_list->count. So I think it is safe here

Copy link
Collaborator

@ujfalusi ujfalusi Oct 2, 2023

Choose a reason for hiding this comment

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

At the end we are not going to put all pipelines in the trigger list? Right, when we add the last pipeline the trigger_list->count is pipeline_list->count, I guess it is OK, but super confusing.

}

trigger_list->pipeline_instance_ids[i] = pipe_widget->instance_id;
trigger_list->count++;
pipe_priority[i] = pipeline->priority;
}

static void
sof_ipc4_add_pipeline_to_trigger_list(struct snd_sof_dev *sdev, int state,
struct snd_sof_pipeline *spipe,
struct ipc4_pipeline_set_state_data *trigger_list)
struct ipc4_pipeline_set_state_data *trigger_list,
s8 *pipe_priority)
{
struct snd_sof_widget *pipe_widget = spipe->pipe_widget;
struct sof_ipc4_pipeline *pipeline = pipe_widget->private;
Expand All @@ -80,20 +107,20 @@ sof_ipc4_add_pipeline_to_trigger_list(struct snd_sof_dev *sdev, int state,
* for the first time
*/
if (spipe->started_count == spipe->paused_count)
trigger_list->pipeline_instance_ids[trigger_list->count++] =
pipe_widget->instance_id;
sof_ipc4_add_pipeline_by_priority(trigger_list, pipe_widget, pipe_priority,
false);
break;
case SOF_IPC4_PIPE_RESET:
/* RESET if the pipeline is neither running nor paused */
if (!spipe->started_count && !spipe->paused_count)
trigger_list->pipeline_instance_ids[trigger_list->count++] =
pipe_widget->instance_id;
sof_ipc4_add_pipeline_by_priority(trigger_list, pipe_widget, pipe_priority,
true);
break;
case SOF_IPC4_PIPE_PAUSED:
/* Pause the pipeline only when its started_count is 1 more than paused_count */
if (spipe->paused_count == (spipe->started_count - 1))
trigger_list->pipeline_instance_ids[trigger_list->count++] =
pipe_widget->instance_id;
sof_ipc4_add_pipeline_by_priority(trigger_list, pipe_widget, pipe_priority,
true);
Copy link
Member

@plbossart plbossart Sep 21, 2023

Choose a reason for hiding this comment

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

can you clarify why the order needs to be different between PAUSED and RESET?

Copy link
Author

@RanderWang RanderWang Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I also spent time to check it. For REF FW, it doesn't care about the order for PAUSED or RESET and only care about START since START can result to XRUN. Although REF FW doesn't care about PAUSED or RESET, it still use the reverse order compared to START. This behavior is commented in REF driver.

For SOF, we always pause pipeline first then reset. After all the pipeline are paused or are just ready, the order of RESET is unimportant. I follow the current logic. @ranj063 can give more idea.

	/*
	 * IPC4 requires pipelines to be triggered in order starting at the sink and
	 * walking all the way to the source. So traverse the pipeline_list in the order
	 * sink->source when starting PCM's and in the reverse order to pause/stop PCM's.
	 * Skip the pipelines that have their skip_during_fe_trigger flag set. If there is a fork
	 * in the pipeline, the order of triggering between the left/right paths will be
	 * indeterministic. But the sink->source trigger order sink->source would still be
	 * guaranteed for each fork independently.
	 */
	if (state == SOF_IPC4_PIPE_RUNNING || state == SOF_IPC4_PIPE_RESET)
		for (i = pipeline_list->count - 1; i >= 0; i--) {
			spipe = pipeline_list->pipelines[i];
			sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list,
							      pipe_priority);
		}
	else
		for (i = 0; i < pipeline_list->count; i++) {
			spipe = pipeline_list->pipelines[i];
			sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list,
							      pipe_priority);
		}

Copy link
Member

@plbossart plbossart Sep 22, 2023

Choose a reason for hiding this comment

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

i am completely lost. The answer contains two conflicting explanations

A) the reference IPC4 implementation uses the same order for pause and reset
B) the suggested code uses a different order for pause and reset - even though the order for reset does not matter.

if the intent was to follow the reference to the letter, then obviously something went sideways.

I give up at this point, someone else needs to review.

Copy link
Author

@RanderWang RanderWang Sep 25, 2023

Choose a reason for hiding this comment

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

@plbossart let's fully follow the reference. Adopt the same order for pause & resume case.

break;
default:
break;
Expand Down Expand Up @@ -288,6 +315,7 @@ static int sof_ipc4_trigger_pipelines(struct snd_soc_component *component,
struct sof_ipc4_pipeline *pipeline;
struct snd_sof_pipeline *spipe;
struct snd_sof_pcm *spcm;
u8 *pipe_priority;
int ret;
int i;

Expand Down Expand Up @@ -320,6 +348,12 @@ static int sof_ipc4_trigger_pipelines(struct snd_soc_component *component,
if (!trigger_list)
return -ENOMEM;

pipe_priority = kzalloc(pipeline_list->count, GFP_KERNEL);
if (!pipe_priority) {
kfree(trigger_list);
return -ENOMEM;
}

mutex_lock(&ipc4_data->pipeline_state_mutex);

/*
Expand All @@ -334,12 +368,14 @@ static int sof_ipc4_trigger_pipelines(struct snd_soc_component *component,
if (state == SOF_IPC4_PIPE_RUNNING || state == SOF_IPC4_PIPE_RESET)
for (i = pipeline_list->count - 1; i >= 0; i--) {
spipe = pipeline_list->pipelines[i];
sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list);
sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list,
pipe_priority);
}
else
for (i = 0; i < pipeline_list->count; i++) {
spipe = pipeline_list->pipelines[i];
sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list);
sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list,
pipe_priority);
}

/* return if all pipelines are in the requested state already */
Expand Down Expand Up @@ -389,6 +425,7 @@ static int sof_ipc4_trigger_pipelines(struct snd_soc_component *component,
free:
mutex_unlock(&ipc4_data->pipeline_state_mutex);
kfree(trigger_list);
kfree(pipe_priority);
return ret;
}

Expand Down
5 changes: 2 additions & 3 deletions sound/soc/sof/ipc4-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ static const struct sof_topology_token ipc4_sched_tokens[] = {
offsetof(struct sof_ipc4_pipeline, use_chain_dma)},
{SOF_TKN_SCHED_CORE, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32,
offsetof(struct sof_ipc4_pipeline, core_id)},
{SOF_TKN_SCHED_PRIORITY, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32,
offsetof(struct sof_ipc4_pipeline, priority)},
};

static const struct sof_topology_token pipeline_tokens[] = {
Expand Down Expand Up @@ -683,9 +685,6 @@ static int sof_ipc4_widget_setup_comp_pipeline(struct snd_sof_widget *swidget)
goto err;
}

/* TODO: Get priority from topology */
pipeline->priority = 0;

dev_dbg(scomp->dev, "pipeline '%s': id %d, pri %d, core_id %u, lp mode %d\n",
swidget->widget->name, swidget->pipeline_id,
pipeline->priority, pipeline->core_id, pipeline->lp_mode);
Expand Down