Skip to content

ASoC: SOF: topology: refine multi-core core status management #888

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
keyonjie wants to merge 6 commits into from
Closed

ASoC: SOF: topology: refine multi-core core status management #888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dd1853b
ASoC: SOF: add ref counts for DSP cores
keyonjie May 21, 2019
0ebb026
ASoC: SOF: Add cores_status_mutex to protect core status
keyonjie May 13, 2019
9939e84
ASoC: SOF: update ref counts of cores at probe, suspend, and FW booted
keyonjie May 21, 2019
bc4c608
ASoC: SOF: ipc: add an interface for DSP core enable
keyonjie May 24, 2019
afcf5da
ASoC: SOF: ops: add snd_sof_dsp_core_get/put interfaces
keyonjie May 24, 2019
3c0f719
ASoC: SOF: topology: refine DSP multi-core support
keyonjie May 24, 2019
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
5 changes: 5 additions & 0 deletions sound/soc/sof/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,7 @@ static void sof_probe_work(struct work_struct *work)
int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
{
struct snd_sof_dev *sdev;
int i;

sdev = devm_kzalloc(dev, sizeof(*sdev), GFP_KERNEL);
if (!sdev)
Expand All @@ -477,6 +478,10 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
INIT_LIST_HEAD(&sdev->route_list);
spin_lock_init(&sdev->ipc_lock);
spin_lock_init(&sdev->hw_lock);
mutex_init(&sdev->cores_status_mutex);

for (i = 0; i < ARRAY_SIZE(sdev->core_refs); i++)
sdev->core_refs[i] = 0;

if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE))
INIT_WORK(&sdev->probe_work, sof_probe_work);
Expand Down
24 changes: 24 additions & 0 deletions sound/soc/sof/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,30 @@ int snd_sof_ipc_set_get_comp_data(struct snd_sof_ipc *ipc,
}
EXPORT_SYMBOL(snd_sof_ipc_set_get_comp_data);

/* DSP core enable/disable IPC according to enabled_cores_mask */
int snd_sof_ipc_core_enable(struct snd_sof_dev *sdev)
{
struct sof_ipc_pm_core_config pm_core_config;
int err;

memset(&pm_core_config, 0, sizeof(pm_core_config));
pm_core_config.enable_mask = sdev->enabled_cores_mask;

/* configure CORE_ENABLE ipc message */
pm_core_config.hdr.size = sizeof(pm_core_config);
pm_core_config.hdr.cmd = SOF_IPC_GLB_PM_MSG | SOF_IPC_PM_CORE_ENABLE;

/* send ipc */
err = sof_ipc_tx_message(sdev->ipc, pm_core_config.hdr.cmd,
&pm_core_config, sizeof(pm_core_config),
&pm_core_config, sizeof(pm_core_config));
if (err < 0)
dev_err(sdev->dev, "error: core enable ipc failure\n");

return err;
}
EXPORT_SYMBOL(snd_sof_ipc_core_enable);

/*
* IPC layer enumeration.
*/
Expand Down
11 changes: 10 additions & 1 deletion sound/soc/sof/loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ EXPORT_SYMBOL(snd_sof_load_firmware);
int snd_sof_run_firmware(struct snd_sof_dev *sdev)
{
int ret;
int i = 0;
int init_core_mask;

init_waitqueue_head(&sdev->boot_wait);
Expand Down Expand Up @@ -394,8 +395,16 @@ int snd_sof_run_firmware(struct snd_sof_dev *sdev)
return ret;
}

/* fw boot is complete. Update the active cores mask */
mutex_lock(&sdev->cores_status_mutex);
/* fw booted, update the active cores mask and ref counts */
sdev->enabled_cores_mask = init_core_mask;
while (init_core_mask && (i < SND_SOF_CORE_MAX)) {
if (init_core_mask & BIT(0))
sdev->core_refs[i] = 1;
init_core_mask >>= 1;
i++;
}
Copy link
Member

@plbossart plbossart May 21, 2019

Choose a reason for hiding this comment

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

You would need all changes to the refcounts in the same patch, otherwise it'll be difficult to bisect

Copy link
Author

@keyonjie keyonjie May 22, 2019

Choose a reason for hiding this comment

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

OK, let me merge them.

Copy link
Member

@plbossart plbossart May 31, 2019

Choose a reason for hiding this comment

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

@keyonjie this is not resolved.

mutex_unlock(&sdev->cores_status_mutex);

return 0;
}
Expand Down
80 changes: 80 additions & 0 deletions sound/soc/sof/ops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,83 @@ void snd_sof_dsp_panic(struct snd_sof_dev *sdev, u32 offset)
snd_sof_trace_notify_for_error(sdev);
}
EXPORT_SYMBOL(snd_sof_dsp_panic);

/* This is for getting ref count for a DSP core and power on it if needed */
int snd_sof_dsp_core_get(struct snd_sof_dev *sdev, u32 core_idx)
{
int ret;

mutex_lock(&sdev->cores_status_mutex);

/* already powered on, return */
if (sdev->core_refs[core_idx] > 0) {
sdev->core_refs[core_idx]++;
dev_vdbg(sdev->dev, "core_get: core_refs[%d] %d, no need power up\n",
core_idx, sdev->core_refs[core_idx]);
mutex_unlock(&sdev->cores_status_mutex);
return 0;/* core already enabled, return */
}

dev_vdbg(sdev->dev, "core_get: core_refs[%d] %d, powering it up...\n",
core_idx, sdev->core_refs[core_idx]);
/* power up the core that this pipeline is scheduled on */
ret = snd_sof_dsp_core_power_up(sdev, BIT(core_idx));
if (ret < 0) {
dev_err(sdev->dev, "error: powering up pipeline schedule core %d\n",
core_idx);
mutex_unlock(&sdev->cores_status_mutex);
return ret;
}

/* update core ref count and enabled_cores_mask */
sdev->core_refs[core_idx]++;
sdev->enabled_cores_mask |= BIT(core_idx);

/* Now notify DSP that the core power status changed */
snd_sof_ipc_core_enable(sdev);

mutex_unlock(&sdev->cores_status_mutex);

return 0;
}
EXPORT_SYMBOL(snd_sof_dsp_core_get);

/* This is for putting ref count for a DSP core and power off it if needed */
int snd_sof_dsp_core_put(struct snd_sof_dev *sdev, u32 core_idx)
{
int ret;

mutex_lock(&sdev->cores_status_mutex);

/* return if the core is still in use */
if (sdev->core_refs[core_idx] > 1) {
sdev->core_refs[core_idx]--;
dev_vdbg(sdev->dev, "core_put: core_refs[%d] %d, no need power down\n",
core_idx, sdev->core_refs[core_idx]);
mutex_unlock(&sdev->cores_status_mutex);
return 0;
}

dev_vdbg(sdev->dev, "core_put: core_refs[%d] %d, powering it down...\n",
core_idx, sdev->core_refs[core_idx]);
/* power down the pipeline schedule core */
ret = snd_sof_dsp_core_power_down(sdev, BIT(core_idx));
if (ret < 0) {
dev_err(sdev->dev, "error: powering down pipeline schedule core %d\n",
core_idx);
mutex_unlock(&sdev->cores_status_mutex);
return ret;
}

/* update core ref count and enabled_cores_mask */
sdev->core_refs[core_idx]--;
sdev->enabled_cores_mask &= ~BIT(core_idx);

/* Now notify DSP that the core power status changed */
snd_sof_ipc_core_enable(sdev);
Copy link
Collaborator

@lyakh lyakh May 24, 2019

Choose a reason for hiding this comment

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

Isn't this an ABI change? Does the firmware already understand, that this IPC means a "change," not a "power up?" We weren't sending this IPC on power-down before, right?

Copy link
Author

@keyonjie keyonjie May 24, 2019

Choose a reason for hiding this comment

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

No ABI change is needed, the FW already understand it like that, it requires a cores_mask sent, check for all cores with each IPC, and power on/off every core if needed. This is not efficiency, but as I stated in the commit message, changing that requiring FW and ABI change, and it is not the purpose for this PR.

wrt the power-down IPCs, you are right, we didn't send them before, but doing it like that was incorrect, here it is fix to that. Once we introduce real multi-core support in topology file, the cores will never be freed by FW with previous version, maybe @ranj063 can confirm this.

static int ipc_pm_core_enable(uint32_t header)
{
	struct sof_ipc_pm_core_config pm_core_config;
	int i = 0;

	/* copy message with ABI safe method */
	IPC_COPY_CMD(pm_core_config, _ipc->comp_data);

	trace_ipc("ipc: pm core mask 0x%x -> enable",
		  pm_core_config.enable_mask);

	for (i = 0; i < PLATFORM_CORE_COUNT; i++) {
		if (i != PLATFORM_MASTER_CORE_ID) {
			if (pm_core_config.enable_mask & (1 << i))
				cpu_enable_core(i);
			else
				cpu_disable_core(i);
		}
	}

	return 0;
}


mutex_unlock(&sdev->cores_status_mutex);

return 0;
}
EXPORT_SYMBOL(snd_sof_dsp_core_put);
4 changes: 4 additions & 0 deletions sound/soc/sof/ops.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,4 +446,8 @@ int snd_sof_dsp_register_poll(struct snd_sof_dev *sdev, u32 bar, u32 offset,
u32 interval_us);

void snd_sof_dsp_panic(struct snd_sof_dev *sdev, u32 offset);

int snd_sof_dsp_core_get(struct snd_sof_dev *sdev, u32 core_idx);
int snd_sof_dsp_core_put(struct snd_sof_dev *sdev, u32 core_idx);

#endif
7 changes: 7 additions & 0 deletions sound/soc/sof/pm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
{
struct snd_sof_dev *sdev = dev_get_drvdata(dev);
int ret;
int i;

/* do nothing if dsp suspend callback is not set */
if (!sof_ops(sdev)->suspend)
Expand Down Expand Up @@ -385,6 +386,12 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
"error: failed to power down DSP during suspend %d\n",
ret);

mutex_lock(&sdev->cores_status_mutex);
/* reset ref counts of DSP cores */
for (i = 0; i < ARRAY_SIZE(sdev->core_refs); i++)
sdev->core_refs[i] = 0;
Copy link
Collaborator

@lyakh lyakh May 24, 2019

Choose a reason for hiding this comment

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

Is the mutex held when this function is called?

Copy link
Author

@keyonjie keyonjie May 28, 2019

Choose a reason for hiding this comment

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

No actually.
Let me add this to make it symmetric with what we do in run_firmware, thanks.

Copy link
Member

@plbossart plbossart May 31, 2019

Choose a reason for hiding this comment

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

can you clarify if this is fixed or not

mutex_unlock(&sdev->cores_status_mutex);

return ret;
}

Expand Down
9 changes: 9 additions & 0 deletions sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
/* max BARs mmaped devices can use */
#define SND_SOF_BARS 8

/* max core number */
#define SND_SOF_CORE_MAX 8

/* time in ms for runtime suspend delay */
#define SND_SOF_SUSPEND_DELAY_MS 2000

Expand Down Expand Up @@ -412,6 +415,9 @@ struct snd_sof_dev {
struct list_head route_list;
struct snd_soc_component *component;
u32 enabled_cores_mask; /* keep track of enabled cores */
int core_refs[SND_SOF_CORE_MAX];
/* protects enabled_cores_mask & core_refs */
struct mutex cores_status_mutex;

/* FW configuration */
struct sof_ipc_dma_buffer_data *info_buffer;
Expand Down Expand Up @@ -535,6 +541,9 @@ int snd_sof_ipc_set_get_comp_data(struct snd_sof_ipc *ipc,
enum sof_ipc_ctrl_cmd ctrl_cmd,
bool send);

/* DSP core enable/disable IPC */
int snd_sof_ipc_core_enable(struct snd_sof_dev *sdev);
Copy link
Collaborator

@lyakh lyakh May 24, 2019

Choose a reason for hiding this comment

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

I think Pierre already suggested that - would be good to also switch over to using this function in the same patch


/*
* Topology.
* There is no snd_sof_free_topology since topology components will
Expand Down
44 changes: 4 additions & 40 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1351,7 +1351,6 @@ int sof_load_pipeline_ipc(struct snd_sof_dev *sdev,
struct sof_ipc_pipe_new *pipeline,
struct sof_ipc_comp_reply *r)
{
struct sof_ipc_pm_core_config pm_core_config;
int ret;

ret = sof_ipc_tx_message(sdev->ipc, pipeline->hdr.cmd, pipeline,
Expand All @@ -1361,36 +1360,8 @@ int sof_load_pipeline_ipc(struct snd_sof_dev *sdev,
return ret;
}

/* power up the core that this pipeline is scheduled on */
ret = snd_sof_dsp_core_power_up(sdev, 1 << pipeline->core);
if (ret < 0) {
dev_err(sdev->dev, "error: powering up pipeline schedule core %d\n",
pipeline->core);
return ret;
}

/* update enabled cores mask */
sdev->enabled_cores_mask |= 1 << pipeline->core;

/*
* Now notify DSP that the core that this pipeline is scheduled on
* has been powered up
*/
memset(&pm_core_config, 0, sizeof(pm_core_config));
pm_core_config.enable_mask = sdev->enabled_cores_mask;

/* configure CORE_ENABLE ipc message */
pm_core_config.hdr.size = sizeof(pm_core_config);
pm_core_config.hdr.cmd = SOF_IPC_GLB_PM_MSG | SOF_IPC_PM_CORE_ENABLE;

/* send ipc */
ret = sof_ipc_tx_message(sdev->ipc, pm_core_config.hdr.cmd,
&pm_core_config, sizeof(pm_core_config),
&pm_core_config, sizeof(pm_core_config));
if (ret < 0)
dev_err(sdev->dev, "error: core enable ipc failure\n");

return ret;
/* increase ref count of the DSP core */
return snd_sof_dsp_core_get(sdev, pipeline->core);
Copy link
Member

@plbossart plbossart May 31, 2019

Choose a reason for hiding this comment

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

this doesn't look like a very sensible power management idea. You power-up all the cores when the topology is loaded and release them when we unload the topology. They should be power-up when they are used!

Copy link
Author

@keyonjie keyonjie Jun 3, 2019

Choose a reason for hiding this comment

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

That's true, powering up cores as late as possible and powering down them as soon as possible sounds fantastic, but I am not sure we can support this inside FW ATM, we even haven't verified that configuring a pipeline run on a non-0 core via topology can work as we expected(@tlauda please correct me if I am wrong).

To me, the changes of where we are calling snd_sof_dsp_core_get() is relative simple, we can change that when it is aligned on FW and driver and verified work.

Copy link
Member

@plbossart plbossart Jun 3, 2019

Choose a reason for hiding this comment

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

@keyonjie your write-up suggests that you have not tested this patch in a multi-core configuration?
Also I don't see why we should first do something and then change it because it's a bad design. If you've tested multi-core then you can do the change directly. If you haven't tested and this is an enabler patch then test it further before we merge it.

Copy link
Author

@keyonjie keyonjie Jun 11, 2019

Choose a reason for hiding this comment

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

@keyonjie your write-up suggests that you have not tested this patch in a multi-core configuration?
Also I don't see why we should first do something and then change it because it's a bad design. If you've tested multi-core then you can do the change directly. If you haven't tested and this is an enabler patch then test it further before we merge it.

Agree, let me close this PR and revisit it when multi-core feature is required and verified work on FW side.

}

static int sof_widget_load_pipeline(struct snd_soc_component *scomp,
Expand Down Expand Up @@ -2149,17 +2120,10 @@ static int sof_widget_unload(struct snd_soc_component *scomp,
}
break;
case snd_soc_dapm_scheduler:

/* power down the pipeline schedule core */
pipeline = swidget->private;
ret = snd_sof_dsp_core_power_down(sdev, 1 << pipeline->core);
if (ret < 0)
dev_err(sdev->dev, "error: powering down pipeline schedule core %d\n",
pipeline->core);

/* update enabled cores mask */
sdev->enabled_cores_mask &= ~(1 << pipeline->core);

/* decrease ref count of the DSP core */
ret = snd_sof_dsp_core_put(sdev, pipeline->core);
break;
default:
break;
Expand Down