[RFC] Use virtual bus implementation in SOF by ranj063 · Pull Request #1841 · thesofproject/linux

ranj063 · 2020-02-28T19:18:36Z

This PR just shows the use virtual bus devices in SOF. The virtbus dev is encapsulated within a sof_client_dev to enable extending the ops supported by the virtbus_dev to include SOF client specific ops and data. This PR is now updated with the complete implementation for the dummy IPC client.

Please note that the first patch is still under review here:
http://patchwork.ozlabs.org/patch/1247882/

plbossart

Looks like a good start to me. Thanks @ranj063

lgirdwood

I do need to read the virtio bus patches, but some questions that may impact the client PM.

kv2019i

Looks ok so far. Biggest thing seems to be how we manage the sof_virtbus_dev objects as part of sdev and how we link back.

ranj063 · 2020-03-05T21:48:03Z

@plbossart @lgirdwood @lyakh @kv2019i this PR is now updated to include the second step ir registering a client drv and probing client devices after FW boot is done. The client device probe still does nothing atm but once we agree upon the registration model, I will add that to this PR.

lyakh · 2020-03-06T09:31:27Z

+
+	mutex_lock(&sof_client_mutex);
+	list_for_each_entry_safe(cdev, _cdev, &client_list, list) {
+ if (cdev->drv == drv) {


Your client_list is global, and as far as I understand we can in theory at some point get multiple DSPs. Would they then possibly use the same driver? Then this matching would break?

@lyakh I'm not sure about this case. But my guess would be that if there are multiple DSP, they possibly should not share the same driver. @lgirdwood could you please chime in?

the same driver can be used for multiple devices, but each would need to have its own context and not a shared variable.

@plbossart How would that work though? If you look at sof_client_drv_register() today, we create a client device for each client drv that is registered. If multiple devices share the same driver, then we probably need some kind of id table just like the virtbus driver, right?

We can create a new global context (which can hold lists etc) per detected DSP device, i.e. on PCI/ACPI probe.

plbossart · 2020-03-09T20:27:18Z

@ranj063 I must be missing something here.

If I look at the IPC, there is first layer that probes/removed the virtbus driver. And then the probe registers an SOF client, which results in a second level probe.

Is this really necessary? seems to me you are bolting a bus on top of a bus?

ranj063 · 2020-03-10T15:59:11Z

@ranj063 I must be missing something here.
If I look at the IPC, there is first layer that probes/removed the virtbus driver. And then the probe registers an SOF client, which results in a second level probe.
Is this really necessary? seems to me you are bolting a bus on top of a bus?

@plbossart the virtbus devices are actually only virtual devices. They need to be probed first before they can register with SOF for IPC. And we can probe the client devices only after the SOF core device has finished booting the DSP. So I cant imagine how we can avoid the 2-level probe that we have here.
Besides, for a non-virtbus device, we will be calling just the client_drv_register() when it is probed and then handle all client device initialization in the client_dev_probe().

plbossart · 2020-03-10T20:51:45Z

@ranj063 I must be missing something here.
If I look at the IPC, there is first layer that probes/removed the virtbus driver. And then the probe registers an SOF client, which results in a second level probe.
Is this really necessary? seems to me you are bolting a bus on top of a bus?
@plbossart the virtbus devices are actually only virtual devices. They need to be probed first before they can register with SOF for IPC. And we can probe the client devices only after the SOF core device has finished booting the DSP. So I cant imagine how we can avoid the 2-level probe that we have here.
Besides, for a non-virtbus device, we will be calling just the client_drv_register() when it is probed and then handle all client device initialization in the client_dev_probe().

It was my understanding that all clients would be represented with a device that's a child of the PCI/ACPI/DT parent, which would by construction guarantee that the parent remains active as long as any of its children is active.
Your last sentence 'for a non-virtbus device' suggests otherwise?

ranj063 · 2020-03-23T21:49:00Z

@lgirdwood @plbossart I've updated this PR based on our discussions last week. The last 3 patches are the ones relevant for SOF. Please let me know what you think

plbossart

Thanks @ranj063 I must be missing a bit of context on the need for a list and the pointer v. structure nesting.

ranj063 · 2020-04-23T20:33:56Z

@plbossart @kv2019i I'm goin to hold off on this PR for a week. The virtbus authors are working on a new version and I want to take that in.

plbossart · 2020-04-23T20:40:10Z

@plbossart @kv2019i I'm goin to hold off on this PR for a week. The virtbus authors are working on a new version and I want to take that in.

ack, also busy on my side.

lyakh · 2020-05-12T06:31:29Z

@ranj063 I see you haven't yet changed the SOF_CLIENT_IPC enum, are you still in the process of making changes on this PR or is it just a single comment that you missed?

fredoh9 · 2020-05-12T19:02:13Z

@lyakh This is not final yet. virtual bus commit was updated in a few days ago and they are working again to address some comments. I need to rebase again when they update it. I will address your comment also.

plbossart

couple of comments below @ranj063

plbossart

minor comments on alignment, for renames I don't know if we really want virtualbus everywhere. virtbus sounds good enough to me, not sure if there is really any risk of confusion with virtualization as @andy-shev was asking.

plbossart · 2020-05-28T18:19:27Z

+ One example is SOF (Sound Open Firmware) clients such as audio cards
+ or test tools needing to connect with the SOF core. Another example
+ is the irdma driver needing to connect with various PCI LAN drivers
+ to request resources (queues) to be able to perform its function.


alignment seems off.

hmm this looks fine on my editor. Is it maybe a github display issue?

@ranj063 No, it's TABs vs. spaces

plbossart · 2020-05-28T18:26:40Z

-struct virtbus_device_id *virtbus_match_id(const struct virtbus_device_id *id,
- struct virtbus_device *vdev)
+static const struct virtualbus_device_id *
+virtbus_match_id(const struct virtualbus_device_id *id,


then also rename the virtbus_ functions

plbossart · 2020-05-28T18:30:58Z

 	struct virtbus_device *vdev = to_virtbus_dev(dev);

-	if (add_uevent_var(env, "MODALIAS=%s%s", VIRTBUS_MODULE_PREFIX,
+	if (add_uevent_var(env, "MODALIAS=%s%s", VIRTUALBUS_MODULE_PREFIX,


if you rename on line 45 this should also be virtualbus_device = to_virtualbus_dev

ranj063 · 2020-05-28T18:42:25Z

minor comments on alignment, for renames I don't know if we really want virtualbus everywhere. virtbus sounds good enough to me, not sure if there is really any risk of confusion with virtualization as @andy-shev was asking.

OK then, maybe I drop that commit to rename.

plbossart · 2020-05-28T18:45:24Z

minor comments on alignment, for renames I don't know if we really want virtualbus everywhere. virtbus sounds good enough to me, not sure if there is really any risk of confusion with virtualization as @andy-shev was asking.
OK then, maybe I drop that commit to rename.

best to see what @andy-shev thinks of these really long virtual_bus_device names.

andy-shev · 2020-05-28T21:02:05Z

minor comments on alignment, for renames I don't know if we really want virtualbus everywhere. virtbus sounds good enough to me, not sure if there is really any risk of confusion with virtualization as @andy-shev was asking.
OK then, maybe I drop that commit to rename.
best to see what @andy-shev thinks of these really long virtual_bus_device names.

Can you elaborate a bit? Any example where these long names appear and how do they look like?

kv2019i

Looks fine to me now.

lyakh

I prvided a couple of comments, but none of them are critical IMHO, I leave it to the author and other reviewers to decide whether a new iteration is needed

lyakh · 2020-06-02T06:48:20Z

+{
+	int ret;
+
+	if (WARN_ON(!vdev->release))


not sure this kind of an error deserves a full blown stack dump and stuff, wouldn't a dev_err() or similar be enough?

lyakh · 2020-06-02T07:29:54Z

+	.name = "sof-ipc-test-client-drv",
+	.virtbus_drv = {
+ .driver = {
+ .name = "sof-ipc-test-virtbus-drv",


both driver names seem a bit too verbose to me, at least the "-drv" part seems unnecessary?

kv2019i

Looks good @ranj063 . Really the only thing in the documentation was use of MFD versus PCI multifunction. Otherwise seems good.

plbossart

Sorry for chiming in late, the PCI part is just completely wrong.

shirazsaleem · 2020-06-03T17:41:55Z

+
+Virtual bus devices are created and registered by a multi-function PCI device.
+Therefore, they are typically encapsulated within a structure defined by
+parent PCI device which contains the virtual bus device and any associated


defined by the device or driver?

shirazsaleem · 2020-06-03T17:42:17Z

+ void *data;
+ };
+
+The parent PCI device would then register the virtual bus device by calling


The 'core' driver does the registration...

kv2019i

A few minor notes on doc, but looks good. At least this now explicitly discussed the platform bus and MFD and why virtbus is added.

plbossart

Nice improvement on the documentation @ranj063
a couple of comments below to make it even nicer

This is the initial implementation of the Virtual Bus, virtbus_device and virtbus_driver. The virtual bus is a software based bus intended to support splitting the functionality of a subsystem's core device into orthogonal parts. This bus is also meant to be generic and be available to be used by any type of parent device (PCI/ACPI/DT). Signed-off-by: Dave Ertman <david.m.ertman@intel.com> Signed-off-by: Kiran Patil <kiran.patil@intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Tested-by: Andrew Bowers <andrewx.bowers@intel.com> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

A client in the SOF (Sound Open Firmware) context is a device 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> Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Create an SOF client driver for IPC flood test. This driver is used to set up the debugfs entries and the read/write ops for initiating the IPC flood test that would be used to measure the min/max/avg response times for sending IPCs to the DSP. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add a new op for registering clients. The clients to be registered depend on the DSP capabilities and the ACPI/DT information. For now, we only add 2 IPC test clients that will be used for run tandem IPC flood tests for all Intel platforms. For ACPI platforms, change the Kconfig to select SND_SOC_SOF_PROBE_WORK_QUEUE to allow the virtbus driver to probe when the client is registered. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Remove the IPC flood test support in the SOF core as it is now added in the IPC flood test client. Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

lyakh

if you decide to fix the presumable '?' typo, this would be a perfect case for a "fixup" patch.

lyakh · 2020-06-05T07:16:40Z

+
+Virtbus drivers register themselves with the bus by calling
+virtbus_register_driver(). The id_table contains the names of virtbus devices
+that a driver can bind with?


why a question mark? A typo?

I think it was my suggestion with a question mark that was copied verbatim...

yes, sorry about that. The documentation is so verbose now that I missed it :(

kv2019i · 2020-10-22T09:01:28Z

@ranj063 Can we close this, seems obsolete now...?

ranj063 requested review from dbaluta, juimonen, keyonjie, kv2019i, lgirdwood, libinyang, lyakh and plbossart as code owners February 28, 2020 19:18

plbossart reviewed Feb 28, 2020
View reviewed changes

Comment threadsound/soc/sof/core.c Outdated
Comment threadsound/soc/sof/Makefile Outdated
Comment threadsound/soc/sof/sof-ipc-test.c Outdated
Comment threadsound/soc/sof/sof-ipc-test.c Outdated

lyakh requested changes Mar 2, 2020
View reviewed changes

Comment threadsound/soc/sof/core.c Outdated
Comment threadsound/soc/sof/core.c Outdated
Comment threadsound/soc/sof/core.c Outdated

lgirdwood reviewed Mar 2, 2020
View reviewed changes

Comment threadsound/soc/sof/core.c Outdated

kv2019i reviewed Mar 2, 2020
View reviewed changes

ranj063 force-pushed the virtual-bus-sof branch from 80aeb71 to b071bc4Compare March 5, 2020 21:46

lyakh requested changes Mar 6, 2020
View reviewed changes

ranj063 force-pushed the virtual-bus-sof branch 6 times, most recently from 145d738 to c0251feCompare March 6, 2020 18:33

ranj063 force-pushed the virtual-bus-sof branch from c0251fe to b7cfa48Compare March 23, 2020 21:44

ranj063 force-pushed the virtual-bus-sof branch from b7cfa48 to 0e1b090Compare March 23, 2020 22:13

plbossart reviewed Mar 23, 2020
View reviewed changes

Comment threadsound/soc/sof/sof-client.c Outdated
Comment threadsound/soc/sof/sof-client.c
Comment threadsound/soc/sof/sof-client.h Outdated
Comment threadsound/soc/sof/sof-client.h Outdated

ranj063 force-pushed the virtual-bus-sof branch 2 times, most recently from 48cd91f to d120705Compare March 23, 2020 23:13

plbossart reviewed May 27, 2020
View reviewed changes

lyakh reviewed May 28, 2020
View reviewed changes

Comment threadsound/soc/sof/Kconfig Outdated
Comment threadsound/soc/sof/sof-client.h Outdated

plbossart requested changes May 28, 2020
View reviewed changes

lyakh requested changes May 29, 2020
View reviewed changes

Comment threaddrivers/bus/virtual_bus.c
Comment threaddrivers/bus/virtual_bus.c Outdated
Comment threaddrivers/bus/virtual_bus.c
Comment threadsound/soc/sof/Makefile Outdated
Comment threadsound/soc/sof/sof-ipc-test-client.c

kv2019i previously approved these changes Jun 1, 2020
View reviewed changes

lyakh previously approved these changes Jun 2, 2020
View reviewed changes

bardliao reviewed Jun 3, 2020
View reviewed changes

Comment threadDocumentation/driver-api/virtual_bus.rst Outdated

kv2019i reviewed Jun 3, 2020
View reviewed changes

Comment threadDocumentation/driver-api/virtual_bus.rst Outdated
Comment threadDocumentation/driver-api/virtual_bus.rst Outdated

plbossart requested changes Jun 3, 2020
View reviewed changes

Comment threadDocumentation/driver-api/virtual_bus.rst Outdated
Comment threadDocumentation/driver-api/virtual_bus.rst Outdated
Comment threadDocumentation/driver-api/virtual_bus.rst Outdated

shirazsaleem reviewed Jun 3, 2020
View reviewed changes

Comment threadDocumentation/driver-api/virtual_bus.rst Outdated

shirazsaleem reviewed Jun 3, 2020
View reviewed changes

kv2019i reviewed Jun 4, 2020
View reviewed changes

Comment threadDocumentation/driver-api/virtual_bus.rst
Comment threadDocumentation/driver-api/virtual_bus.rst Outdated
Comment threadDocumentation/driver-api/virtual_bus.rst Outdated

plbossart requested changes Jun 4, 2020
View reviewed changes

dmertmanand others added 5 commits June 4, 2020 13:03

ASoC: SOF: debug: Remove IPC flood test support in SOF core
61a5a04
Remove the IPC flood test support in the SOF core as it is now added in the IPC flood test client. Signed-off-by: Fred Oh <fred.oh@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

lyakh approved these changes Jun 5, 2020
View reviewed changes

fredoh9 mentioned this pull request Jul 9, 2020
[RFC] Use Ancillary bus implementation in SOF #2265
Closed

marc-hb mentioned this pull request Aug 13, 2024
Auxiliary devices in SOF break check-ipc-flood.sh (IPC3) thesofproject/sof-test#467
Closed

Uh oh!

Conversation

ranj063 commented Feb 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Mar 5, 2020

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!

Uh oh!

Uh oh!

plbossart commented Mar 9, 2020

Uh oh!

ranj063 commented Mar 10, 2020

Uh oh!

plbossart commented Mar 10, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranj063 commented Mar 23, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ranj063 commented Apr 23, 2020

Uh oh!

plbossart commented Apr 23, 2020

Uh oh!

lyakh commented May 12, 2020

Uh oh!

fredoh9 commented May 12, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Feb 28, 2020•
edited
Loading

plbossart commented Mar 10, 2020•
edited
Loading

plbossart commented May 28, 2020•
edited
Loading