SOF auxiliary bus - Part 1 by ujfalusi · Pull Request #2836 · thesofproject/linux

ujfalusi · 2021-04-09T12:31:28Z

Hi,

This is the first part of the reworked pull last sent by @fredoh9.
I have decided to split up the patches into at least two batch to make it easier to review and then fix up based on the comments.

The first part adds the basic support for the auxbus and introduces the term 'sof client'
The Kconfig has been changed so the the client support is selected by the user to keep things easier for the users.
It also includes the conversion of the first client: IPC flood test.
This is a generic test client so instead of doing the registration per architecture as it was in the first series we do it in the generic code. This will allow ARM to keep using it (it was available or them as well).
The Kconfig remained the same, but we now have option to have more than one IPC flood running at the same time. I have also made sure that we don't break testing scripts via debugfs.

Looking forward: the probe test will be converted along the same line as the IPC flood test as that one is also a generic test and it was (well, it is) available on ARM also.
On the Intel side we might drop most of the code for now until we can introduce the clients (HDMI, etc).
I believe the gdb and firmware debugging support might be also platform independent, but I need to check that and plan accordingly.

After addressing the surely coming comments I will include the probe test client into v2.

dbaluta · 2021-04-09T14:14:45Z

+
+	/* Register platform independent client devices */
+	if (IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST)) {
+		for (i = 0; i < CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_NUM; i++) {


Maybe create a function snd_sof_register_flood_clients and make it empty if SOF_DEBUG_IPC_FLOOD_TEST is not enabled. Otherwise, snd_sof_register_clients will be very hard to read.

@dbaluta , yes that's an option. I'm not sure how it will look like if we pile up more platform independent clients like this.
The error handling might be much simpler with a separate function for each selectable client.

@dbaluta, you are right, it is going to make it much cleaner, I'll update for v2.

It's probably also worth a comment stating the future flow i.e. that capabilities will be read from FW manifest and devices enumerated as found in the manifest.

I would add the comment when we have the code which does that

kv2019i

The one checkpatch CHECK issue plus I noticed one weird code comment (probably has been there for long), but otherwise, this looks good to go!

kv2019i · 2021-04-12T09:59:58Z

 	if (plat_data->sof_probe_complete)
 		plat_data->sof_probe_complete(sdev->dev);

+	/* If registering certain clients */


@ranj063 @fredoh9 @ujfalusi Not sure I noticed this in original review, but what does this comment mean? What is a "certain client" ?

There is no need for this comment, imho. I'll remove it.

it's a truncated version of an original comment, should be fixed, yes

I dont remember adding this either :)

I should have just removed the old comment instead, the original comment was:
/* If registering certain clients fails, unregister the previously registered clients. */
and it is no longer valid.

lyakh

some minor stuff, looks rather good to me otherwise

lyakh · 2021-04-12T10:24:20Z

 	if (plat_data->sof_probe_complete)
 		plat_data->sof_probe_complete(sdev->dev);

+	/* If registering certain clients */


it's a truncated version of an original comment, should be fixed, yes

lgirdwood · 2021-04-12T14:25:18Z

+
+	/* Register platform independent client devices */
+	if (IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST)) {
+		for (i = 0; i < CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_NUM; i++) {


It's probably also worth a comment stating the future flow i.e. that capabilities will be read from FW manifest and devices enumerated as found in the manifest.

A client in the SOF (Sound Open Firmware) context is a driver that needs to communicate with the DSP via IPC messages. The SOF core is responsible for serializing the IPC messages to the DSP from the different clients. One example of an SOF client would be an IPC test client that floods the DSP with test IPC messages to validate if the serialization works as expected. Multi-client support will also add the ability to split the existing audio cards into multiple ones, so as to e.g. to deal with HDMI with a dedicated client instead of adding HDMI to all cards. This patch introduces descriptors for SOF client driver and SOF client device along with APIs for registering and unregistering a SOF client driver, sending IPCs from a client device and accessing the SOF core debugfs root entry. Along with this, add a couple of new members to struct snd_sof_dev that will be used for maintaining the list of clients. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Co-developed-by: Fred Oh <fred.oh@linux.intel.com> Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Signed-off-by: Dave Ertman <david.m.ertman@intel.com> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Add new ops for registering/unregistering clients based on DSP capabilities and/or DT information. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Dave Ertman <david.m.ertman@intel.com> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Move the IPC flood test code out from the debug file as separate SOF client driver. Based on the kernel configuration, the device registration for the new IPC flood test is going to happen in the core. With the separate client driver it is going to be possible to run multiple flood tests in parallel to increase the stress, the new Kconfig option can be used to select this (defaults to 1). In order to preserve backward compatibility with existing SW/scripts, the first IPC flood test's debugfs files have been linked to the old files. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Co-developed-by: Fred Oh <fred.oh@linux.intel.com> Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Signed-off-by: Dave Ertman <david.m.ertman@intel.com> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

plbossart

bear with me, some comments are based on my flaky memory of code from last Summer, I've forgotten quite a bit of context there.

plbossart · 2021-04-13T16:26:22Z

 	  If unsure, select "N".

+config SND_SOC_SOF_CLIENT
+	bool


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?

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

plbossart · 2021-04-13T16:27:25Z

+ifneq ($(CONFIG_SND_SOC_SOF_CLIENT),)
+snd-sof-objs += sof-client.o
+endif
+


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

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.

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.

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.

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.

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.

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.

plbossart · 2021-04-13T16:29:01Z

+/*
+ * Copyright(c) 2021 Intel Corporation. All rights reserved.
+ * Author: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
+ */


Mark Brown says "The whole block in C++ style please"

we'll have to adjust older files but let's follow his convention.

OK, If Mark said, then let's do that. Other subsystems require this style....

plbossart · 2021-04-13T16:35:44Z

+	return cdev;
+}
+
+int sof_client_dev_register(struct snd_sof_dev *sdev, const char *name, u32 id)


I must admit I don't recall what the id is needed for.
A comment wouldn't hurt or better some kernel-doc to make sure there's no ambiguity on the parameters.

Edit: now I think I remember, each id is handled by the parent but is only valid within the scope of the 'name', e.g the name can be ipc-test, and there can be multiple instances identified by an id. the uniqueness of the id is handled by the parent. Right?

Yes, the ID is to have unique dev name for the devices. The name is auxbus_name + device_name + id.

plbossart · 2021-04-13T16:36:41Z

+	struct auxiliary_device auxdev;
+	struct snd_sof_dev *sdev;
+	struct list_head list;	/* item in SOF core client dev list */
+	void *data;


cdata?

There should be some comment here, or kernel-doc to explain what this is used for.

or priv. I think most of the struct lacks the proper documentation.

plbossart · 2021-04-13T16:37:38Z

+
+/* client-specific ops, all optional */
+struct sof_client_ops {
+	int (*client_ipc_rx)(struct sof_client_dev *cdev, u32 msg_cmd);


this doesn't really look optional if the tx ipc function is used?

the TX is sync and the client_ipc_rx is not used, if I'm not mistaken.
I'll check and fix the comment if needed.

plbossart · 2021-04-13T16:40:17Z

+	struct list_head client_list;
+
+	/* mutex to protect client list */
+	struct mutex client_mutex;


I am start to wonder if we should start using stronger naming conventions with 'ipc_client' as a prefix, e.g. sof_ipc_client, ipc_client_mutex, etc.

Can the client do anything other than IPCs?

It must not do anything else than IPC as we say the a client in SOF is a functionality on the DSP side or a feature over the IPC.
I agree that we should aim for easily recognizable naming convention. This is why I renamed the ipc test to sof-client-ipc-flood.

plbossart · 2021-04-13T16:45:01Z

+config SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_NUM
+	int "Number of IPC flood test clients"
+	range 1 32
+	default 1


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?

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.

plbossart · 2021-04-13T16:48:06Z

 	if (sof_ops(sdev) && sof_ops(sdev)->unregister_clients)
 		sof_ops(sdev)->unregister_clients(sdev);
+
+	snd_sof_unregister_ipcflood_test(sdev);


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

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?

...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

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

ujfalusi · 2021-06-23T10:39:13Z

A new take on the auxiliary bus support (SOF client drivers) will be pushed soon, closing this one, thank you for the feedbacks, most if not all will be addressed by the take2

ujfalusi requested review from fredoh9, keyonjie, kv2019i, lgirdwood, lyakh, plbossart and ranj063 April 9, 2021 12:31

ujfalusi requested review from bardliao, dbaluta, juimonen and xiulipan as code owners April 9, 2021 12:31

ujfalusi commented Apr 9, 2021

View reviewed changes

Comment thread sound/soc/sof/sof-client-ipc-test.c Outdated

dbaluta reviewed Apr 9, 2021

View reviewed changes

kv2019i requested changes Apr 12, 2021

View reviewed changes

lyakh requested changes Apr 12, 2021

View reviewed changes

lgirdwood requested changes Apr 12, 2021

View reviewed changes

ranj063 added 3 commits April 13, 2021 08:33

ujfalusi force-pushed the peter/sof/auxiliary_bus_part1 branch from 092ed2f to d5e064b Compare April 13, 2021 08:04

ujfalusi requested review from dbaluta, kv2019i, lgirdwood and lyakh April 13, 2021 08:05

plbossart requested changes Apr 13, 2021

View reviewed changes

dbaluta approved these changes Apr 14, 2021

View reviewed changes

ujfalusi closed this Jun 23, 2021

ujfalusi mentioned this pull request Jun 23, 2021

SOF client support (auxiliary bus) #3007

Closed

Uh oh!

Conversation

ujfalusi commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented Apr 9, 2021 •

edited

Loading