Skip to content

SOF auxiliary bus - Part 1 #2836

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
ujfalusi wants to merge 3 commits into from
Closed

SOF auxiliary bus - Part 1 #2836

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
04def7a
ASoC: SOF: Introduce SOF client support
ranj063 Oct 22, 2020
08722f4
ASoC: SOF: core/ops: Add support for client registration
ranj063 Oct 22, 2020
d5e064b
ASoC: SOF: Convert the generic IPC flood test into SOF client
ranj063 Oct 22, 2020
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
24 changes: 21 additions & 3 deletions sound/soc/sof/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,13 @@ config SND_SOC_SOF_DEBUG_PROBES
Say Y if you want to enable probes.
If unsure, select "N".

config SND_SOC_SOF_CLIENT
bool
Copy link
Member

@plbossart plbossart Apr 13, 2021

Choose a reason for hiding this comment

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

is there a reason not to make this a tristate? This will force the auxiliary bus to be built-in, it's not really necessary, is it?

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 13, 2021

Choose a reason for hiding this comment

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

I use it as a flag to include the sof-client support or not.

select AUXILIARY_BUS
help
This option is not user-selectable but automagically handled by
'select' statements at a higher level.

config SND_SOC_SOF_DEVELOPER_SUPPORT
bool "SOF developer options support"
depends on EXPERT
Expand Down Expand Up @@ -181,13 +188,24 @@ config SND_SOC_SOF_DEBUG_ENABLE_FIRMWARE_TRACE
If unsure, select "N".

config SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST
bool "SOF enable IPC flood test"
tristate "SOF enable IPC flood test"
select SND_SOC_SOF_CLIENT
help
This option enables the IPC flood test which can be used to flood
the DSP with test IPCs and gather stats about response times.
This option enables a separate client device for IPC flood test
which can be used to flood the DSP with test IPCs and gather stats
about response times.
Say Y if you want to enable IPC flood test.
If unsure, select "N".

if SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST
config SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_NUM
int "Number of IPC flood test clients"
range 1 32
default 1
Copy link
Member

@plbossart plbossart Apr 13, 2021

Choose a reason for hiding this comment

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

I can't recall why we said 1 is the default. If we want to test IPC concurrency 2 seems like a natural number, no?

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 13, 2021

Choose a reason for hiding this comment

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

I don't want to introduce any functionality change. The original code provided one IPC flood test, we do the same by default but later we can change the default.

help
Select the number of IPC flood test clients to be created.
endif

config SND_SOC_SOF_DEBUG_RETAIN_DSP_CONTEXT
bool "SOF retain DSP context on any FW exceptions"
help
Expand Down
8 changes: 8 additions & 0 deletions sound/soc/sof/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@

snd-sof-objs := core.o ops.o loader.o ipc.o pcm.o pm.o debug.o topology.o\
control.o trace.o utils.o sof-audio.o
ifneq ($(CONFIG_SND_SOC_SOF_CLIENT),)
snd-sof-objs += sof-client.o
endif

Copy link
Member

@plbossart plbossart Apr 13, 2021

Choose a reason for hiding this comment

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

do we need to do this, or can we have a cleaner cut and have sof-client be a separate module?

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 13, 2021

Choose a reason for hiding this comment

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

The problem with a separate .ko is that we will have circular dependency due to the fact that the generic, no platform clients are registered from the core.
snd-sof-client.ko uses symbols from snd-sof.ko and snd-sof.ko uses symbols from snd-sof-client.ko
This was not an issue with the original series as there the generic, platform independent sof clients were registered from platform code (intel code and intel code only).
ipc flood and probes are generic clients and can be used by other architectures.

Copy link
Member

@plbossart plbossart Apr 13, 2021

Choose a reason for hiding this comment

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

that's not completely true. I believe the probes are Intel HDaudio only, or at least they cannot be enabled on legacy platforms so don't belong in the core.

As for the circular dependency for ipc, I am usure why 'snd-sof.ko uses symbols from snd-sof-client.ko'. that seems wrong.

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 13, 2021

Choose a reason for hiding this comment

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

right, we only registered the probe for HDA, true, but afaik the probe code on the firmware is not tied to HDA or is it?

Well, the sof-core is the one which manages and branches out the auxbus for the SOF clients.
One option is to relocate the auxbus device registration calls to the file where most of the IPC core code is, I think that might solve the circular dependency issue and it might be even better place.

Copy link
Member

@plbossart plbossart Apr 13, 2021

Choose a reason for hiding this comment

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

One option is to relocate the auxbus device registration calls to the file where most of the IPC core code is, I think that might solve the circular dependency issue and it might be even better place.

good suggestion.

Copy link
Member

@plbossart plbossart Apr 13, 2021

Choose a reason for hiding this comment

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

right, we only registered the probe for HDA, true, but afaik the probe code on the firmware is not tied to HDA or is it?

I think the probes only exist on platforms that have HDA. The DMA also assumes a circular list (BDL) that doesn't exist with the GP-DMA on legacy platforms.

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 13, 2021

Choose a reason for hiding this comment

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

OK, in the draft pull I create the aux device for probe only in case of HDA.
I hope.

The code for the probe was in the generic, core part of SOF, that's why I have kept it there and the original series did the same as well.

snd-sof-$(CONFIG_SND_SOC_SOF_DEBUG_PROBES) += probe.o compress.o

snd-sof-pci-objs := sof-pci-dev.o
snd-sof-acpi-objs := sof-acpi-dev.o
snd-sof-of-objs := sof-of-dev.o

snd-sof-ipc-test-objs := sof-client-ipc-test.o

snd-sof-nocodec-objs := nocodec.o

obj-$(CONFIG_SND_SOC_SOF) += snd-sof.o
Expand All @@ -18,6 +24,8 @@ obj-$(CONFIG_SND_SOC_SOF_ACPI_DEV) += snd-sof-acpi.o
obj-$(CONFIG_SND_SOC_SOF_OF) += snd-sof-of.o
obj-$(CONFIG_SND_SOC_SOF_PCI_DEV) += snd-sof-pci.o

obj-$(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST) += snd-sof-ipc-test.o

obj-$(CONFIG_SND_SOC_SOF_INTEL_TOPLEVEL) += intel/
obj-$(CONFIG_SND_SOC_SOF_IMX_TOPLEVEL) += imx/
obj-$(CONFIG_SND_SOC_SOF_XTENSA) += xtensa/
82 changes: 82 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 @@ -12,6 +12,7 @@
#include <linux/module.h>
#include <sound/soc.h>
#include <sound/sof.h>
#include "sof-client.h"
#include "sof-priv.h"
#include "ops.h"
#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
Expand Down Expand Up @@ -52,6 +53,77 @@ static const struct sof_panic_msg panic_msg[] = {
{SOF_IPC_PANIC_ASSERT, "assertion failed"},
};

#if IS_ENABLED(CONFIG_SND_SOC_SOF_CLIENT)
static int snd_sof_register_ipcflood_test(struct snd_sof_dev *sdev)
{
int i;
int ret;

if (!IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST))
return 0;

for (i = 0; i < CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_NUM; i++) {
ret = sof_client_dev_register(sdev, "ipc_test", i);
if (ret < 0)
break;
}

if (ret) {
for (; i >= 0; --i)
sof_client_dev_unregister(sdev, "ipc_test", i);
}

return ret;
}

static void snd_sof_unregister_ipcflood_test(struct snd_sof_dev *sdev)
{
int i;

if (!IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST))
return;

for (i = 0; i < CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_NUM; i++)
sof_client_dev_unregister(sdev, "ipc_test", i);
}

static int snd_sof_register_clients(struct snd_sof_dev *sdev)
{
int ret;

/* Register platform independent client devices */
ret = snd_sof_register_ipcflood_test(sdev);
if (ret) {
dev_err(sdev->dev, "error: IPC flood test client registration failed\n");
return ret;
}

/* Platform depndent client device registration */
if (sof_ops(sdev) && sof_ops(sdev)->register_clients)
ret = sof_ops(sdev)->register_clients(sdev);

if (ret)
snd_sof_unregister_ipcflood_test(sdev);

return ret;
}

static void snd_sof_unregister_clients(struct snd_sof_dev *sdev)
{
if (sof_ops(sdev) && sof_ops(sdev)->unregister_clients)
sof_ops(sdev)->unregister_clients(sdev);

snd_sof_unregister_ipcflood_test(sdev);
Copy link
Member

@plbossart plbossart Apr 13, 2021

Choose a reason for hiding this comment

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

why is this all in core.c? This doesn't seem right to me.

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 13, 2021

Choose a reason for hiding this comment

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

It is a generic client and does not belong to platform code, it has to be in the core.
We had it in the debug.c

We have introduced a callback for client registration and I think it is better to keep all client device creation, destruction in one place to minimize the chances of random ordering issues.

We could move the generic debug feature registrations to debug.c, if that makes more sense?

Copy link
Collaborator

@lyakh lyakh Apr 14, 2021

Choose a reason for hiding this comment

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

...or to sof-client.c, maybe even move snd_sof_register_clients() and snd_sof_unregister_clients() there. Maybe even move all generic IPC-flood (de)registration to sof-client-ipc-test.c

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 14, 2021

Choose a reason for hiding this comment

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

right, let me see how that will look like, thanks.

}
#else /* CONFIG_SND_SOC_SOF_CLIENT */
static inline int snd_sof_register_clients(struct snd_sof_dev *sdev)
{
return 0;
}

static inline void snd_sof_unregister_clients(struct snd_sof_dev *sdev) { }
#endif /* CONFIG_SND_SOC_SOF_CLIENT */

/*
* helper to be called from .dbg_dump callbacks. No error code is
* provided, it's left as an exercise for the caller of .dbg_dump
Expand Down Expand Up @@ -249,6 +321,12 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
if (plat_data->sof_probe_complete)
plat_data->sof_probe_complete(sdev->dev);

ret = snd_sof_register_clients(sdev);
if (ret < 0) {
dev_err(sdev->dev, "error: failed to register clients %d\n", ret);
goto fw_trace_err;
}

sdev->probe_completed = true;

return 0;
Expand Down Expand Up @@ -322,9 +400,11 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
INIT_LIST_HEAD(&sdev->widget_list);
INIT_LIST_HEAD(&sdev->dai_list);
INIT_LIST_HEAD(&sdev->route_list);
INIT_LIST_HEAD(&sdev->client_list);
spin_lock_init(&sdev->ipc_lock);
spin_lock_init(&sdev->hw_lock);
mutex_init(&sdev->power_state_access);
mutex_init(&sdev->client_mutex);

if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE))
INIT_WORK(&sdev->probe_work, sof_probe_work);
Expand Down Expand Up @@ -371,6 +451,7 @@ int snd_sof_device_remove(struct device *dev)
dev_warn(dev, "error: %d failed to prepare DSP for device removal",
ret);

snd_sof_unregister_clients(sdev);
snd_sof_fw_unload(sdev);
snd_sof_ipc_free(sdev);
snd_sof_free_debug(sdev);
Expand Down Expand Up @@ -419,3 +500,4 @@ MODULE_AUTHOR("Liam Girdwood");
MODULE_DESCRIPTION("Sound Open Firmware (SOF) Core");
MODULE_LICENSE("Dual BSD/GPL");
MODULE_ALIAS("platform:sof-audio");
MODULE_IMPORT_NS(SND_SOC_SOF_CLIENT);
Loading