SOF client support (auxiliary bus)

[ujfalusi]

Hi,

this series is relatively big departure from previous versions (the latest was #2836 from me).
The high level concept is:

• SOF client are auxiliary devices / drivers (sof_client_drv level is dropped)
• The API used by clients are going to be sof_client_*
  • we will add new ones as the need emerges
• platform dependent and dynamic information can be passed to the client driver via standard device platform_data
• platform dependent static information, parameters which can be local to the client driver can be propagated via match with different device name.
• In case the platform needs host side software control like HDA does, the platform_data is the place to give the ops to the client driver along with a parameter that the callbacks should be called with (void *host_data) and it is up to the host side code to cast it to what it is using internally.
  • see the sof-client-probes and hda-probes pair.

Dropped most of the reviewed-by, tested-by tags since not much is intact from the original versions, but I have kept @ranj063 as author where she was and @fredoh9 as co-author.
I have tested the IPC flood test with 3 instance and the probes on my TGL-H HDA device, all works correctly.

Reasons for a draft status:

• the sound card created by the probes takes card0 with a single compr, the normal audio card is enumerated as card1
  • I want to find a way to keep the audio card as card0 and have the probes at card1
  • I'm not sure if this actually matters.
• I have tried to split up the probes conversion patch (last one), but I just don't see how it can be done in a bisectable way
• I might have missed some of the comments for the previous approach, but I have tried, some of them no longer applicable
• I want to see if I can also move the trace support out as a client as next step.

Next steps:

• split up the snd_sof_dsp_ops into chunks for the really core DSP/Firmware management, communication and audio, trace to prepare for the audio support client conversion.
• abstract away all platform specific members from snd_sof_pdata, sof_dev_desc, snd_sof_dev and other structures under a void * and make them only visible for the platform they are actually used (whenever it is possible)
• IO function abstraction break up
• work towards full client based setup of SOF.

[ujfalusi]

changes since v1:

• sparse issues fixed
• checkpatch report addressed.

[ujfalusi]

Note: The checkpatch warnings are all about new, renamed files versus MAINTAINERS file.

[lyakh]

could just do

if (remaining <= 0)
	break;

[ujfalusi]

True, but my intention was to go without any functional changes, this is coming from the merged two sof/probe.c and sof/compress.c file.

[plbossart]

let's add @lyakh's suggestion in a follow-up patch

[plbossart]

Can we push the following commits to a cleanup PR?

ASoC: SOF: ipc: Add probe message logging to ipc_log_header()
ASoC: SOF: pcm: Remove non existent CONFIG_SND_SOC_SOF_COMPRESS refer…
ASoC: SOF: compress: move and export sof_probe_compr_ops
ASoC: SOF: probe: Merge and clean up the probe and compress files
ASoC: SOF: intel: Rename hda-compress.c to hda-probes.c

That would make the rest easier to review.

[ujfalusi]

Can we push the following commits to a cleanup PR?

ASoC: SOF: ipc: Add probe message logging to ipc_log_header()
ASoC: SOF: pcm: Remove non existent CONFIG_SND_SOC_SOF_COMPRESS refer…
ASoC: SOF: compress: move and export sof_probe_compr_ops
ASoC: SOF: probe: Merge and clean up the probe and compress files
ASoC: SOF: intel: Rename hda-compress.c to hda-probes.c

That would make the rest easier to review.

@plbossart, yes, I think we can do that. I have re-arranged the commits in my wip branch as per your suggestion and the PR is created: #3014

[ujfalusi]

I have a generic file naming convention question.
In this series I have the SOF client driver files named with sof-client prefix. Do we want to do that?
Would it be simpler and easier for the future when we will add clients spanning to multiple files to drop prefixing at all?
We could have some convention to add sof prefix for files which implements more like SOF features than generic things, like:
sof-client-ipc-test.c -> ipc-flood-test.c as it is a generic IPC stress test module
sof-client-probes.c/h -> sof-probes.c/h as the probes are SOF feature (and HDA limited atm) to extract audio from the firmware.

We could also remove the sof-client prefixing from names/structs within the client drivers.
I'm not sure about the sof-client.c/h at this point. Should it be renamed as ipc-client.c/h? Would it trigger objection to be kept under sound/soc/sof/ if it is renamed?

@lgirdwood, @plbossart, @ranj063, @kv2019i, @lyakh what is your opinion on the matter?

[lyakh]

We could have some convention to add sof prefix for files which implements more like SOF features than generic things, like:
sof-client-ipc-test.c -> ipc-flood-test.c as it is a generic IPC stress test module
sof-client-probes.c/h -> sof-probes.c/h as the probes are SOF feature (and HDA limited atm) to extract audio from the firmware.

@ujfalusi No prefix has the advantage of saving the redundancy, while having a prefix makes "find" easier - I've just found 14 dev.c, 41 dma.c, 85 irq.c and 65 init.c in the kernel... So, personally I wouldn't mind using sof- prefix for all our files. As for type / function / variable names: the way I work having prefixes makes it easier. Try to find definition of bus_type or something similar in the kernel :-)

[ujfalusi]

Hi,

Changes since v2:

• rebased on sof-dev, the only the four core patch remained in the series
• the default number of IPC flood tests is now set to 2
• Kconfig change as suggested by @plbossart
• the callback function for platforms to implement for client registration is renamed from ipc_register_clients to register_ipc_clients and the unregister in a same way (@lyakh)

[plbossart]

LGTM, just one question below for @ujfalusi and @ranj063

[plbossart]

@ujfalusi any explanation for the across-the-board CI failures? Could this be an sof-test issue or is this a regression?

[ujfalusi]

@ujfalusi any explanation for the across-the-board CI failures? Could this be an sof-test issue or is this a regression?

@plbossart, I certainly don't have any explanation. The previous version (which was identical) had no such failures.

Oh, I think this gives some clue for some of the test failures:
aplay on PCM hw:0,0 failed at 1/1.

If the probes are enabled then card0 is the probes card (with only compress capture), card1 is the audio card.
This is something I'm trying to solve, to have a way to 'hint' the card index for probe to be 20 or something high enough.
I think UCM and PA PipeWire should be able to handle this, but aplay/arecord is a bit harder.

If the probes are used by tests then the debugfs files are moved under sof/probes from sof

[plbossart]

@ujfalusi any explanation for the across-the-board CI failures? Could this be an sof-test issue or is this a regression?

@plbossart, I certainly don't have any explanation. The previous version (which was identical) had no such failures.

Oh, I think this gives some clue for some of the test failures:
aplay on PCM hw:0,0 failed at 1/1.

If the probes are enabled then card0 is the probes card (with only compress capture), card1 is the audio card.
This is something I'm trying to solve, to have a way to 'hint' the card index for probe to be 20 or something high enough.
I think UCM and PA PipeWire should be able to handle this, but aplay/arecord is a bit harder.

If the probes are used by tests then the debugfs files are moved under sof/probes from sof

I don't know why our CI requires the use of card0, they can filter based on the 'sof' prefix.

another workaround is to use what distros use in alsa-base.conf

# Prevent abnormal drivers from grabbing index 0
options bt87x index=-2
options cx88_alsa index=-2
options saa7134-alsa index=-2
options snd-atiixp-modem index=-2

and

# Keep snd-usb-audio from beeing loaded as first soundcard
options snd-usb-audio index=-2

[ujfalusi]

I don't know why our CI requires the use of card0, they can filter based on the 'sof' prefix.

another workaround is to use what distros use in alsa-base.conf

# Prevent abnormal drivers from grabbing index 0
options bt87x index=-2
options cx88_alsa index=-2
options saa7134-alsa index=-2
options snd-atiixp-modem index=-2

and

# Keep snd-usb-audio from beeing loaded as first soundcard
options snd-usb-audio index=-2

This will not work, ASoC does not have support for custom IDX, it will use SNDRV_DEFAULT_IDX1 unconditionally. There is no way to pass a custom idx, we have tried to get a support for it but the whole idea was rejected. fwiw this is the core patch:
https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/sound/soc/soc-core.c?h=ti-linux-5.4.y&id=dafbe78ee0b747aad82d508936e2ad3ee63a09b6

What works via module parameter is:

options snd slots=,,,snd_sof_probes

This will move the probes card to card3:

# cat /proc/asound/cards 
 0 [sofhdadsp      ]: sof-hda-dsp - sof-hda-dsp
                      DellInc.-LatitudeABCDEF
 3 [sofprobe       ]: sof-probe - sof-probe
                      DellInc.-LatitudeABCDEF

But this is static assignment and if a dynamic card got the slot 3 first then it will fail.
We would need to pass 0xffff fffe (-2) as idx instead SNDRV_DEFAULT_IDX1 to get the dynamic slot assignment working but exclude the slot0.

We could try to add idx_mask to struct snd_soc_card and do similar thing but:

idx = card->idx_mask ? : SNDRV_DEFAULT_IDX1;
ret = snd_card_new(card->dev, idx, SNDRV_DEFAULT_STR1, card->owner, 0, &card->snd_card);

Then let card drivers to deal with the idx_mask if they want, but we will lack the support for requesting a specific card number (0 can not be asked directly).

[ujfalusi]

Changes since v14:

• rebased on sof-dev
• dma-trace remains supported on i.MX after the client conversion (FYI @dbaluta)
• sof_client_core_module_get() / sof_client_core_module_put() is removed from debugfs file open calls
• New sdev->fw_state is introduced: SOF_FW_CRASHED which indicates that the firmware crashed after it has booted
• the client panic event is replaced with fw_state change notification
• along with the change I have introduced API to query the fw_state by the clients
• clients now block their use if the firmware is in SOF_FW_CRASHED state as the firmware is known to be dead
• probes now have no user facing Kconfig option, selected by SND_SOC_SOF_HDA_COMMON

Opps:

• the DMA_TRACE ( or FIRMWARE_TRACE) Kconfig got mixed up, I need to revisit it. It might make sense to drop also the user facing Kconfig and just build it?

Squashing patches is still postponed.

[plbossart]

@ujfalusi I see this series has grown to 32 commits and there are really a lot of opens left and right that could be IMHO addressed later.

What if e.g. we only enable IPC flood and probes for SOF clients? The DMA trace seems to be problematic but it doesn't really bring any value to move as a client, does it?

I think we should really look at realistic/smaller objectives and later on add more complicated scenarios/client types.

[plbossart]

not sure about this one. In the 'sof_tear_down_pipelines' below we free some widgets. We wouldn't be able to restart with a clean slate out of D3 if we skip this part.

@ranj063 you'll have to chime in on this change,

[ujfalusi]

@plbossart, you are right, my thinking was that since the firmware has crashed there is no point of trying to do a clean shutdown of it by sending messages. The firmware is in essence in a non initialized state. However I see that we do cleanup on the linux side and the path ignores IPC errors (which we will have).

I'll drop this change.

[plbossart]

I don't understand this part either.

[ujfalusi]

If the firmware crashed we want the DSP to be hard reset. In this case we will not check if the platform would be capable of S0 state.

[plbossart]

question: Why is this different from the regular components?

int snd_soc_component_module_get(struct snd_soc_component *component,
				 struct snd_pcm_substream *substream,
				 int upon_open)
{
	int ret = 0;

	if (component->driver->module_get_upon_open == !!upon_open &&
	    !try_module_get(component->dev->driver->owner))
		ret = -ENODEV;

	/* mark substream if succeeded */
	if (ret == 0)
		soc_component_mark_push(component, substream, module);

	return soc_component_ret(component, ret);
}

[ujfalusi]

The snd_soc_component_module_get() is used for compressed only via snd_soc_component_module_get_when_probe() wrapper where the substream parameter is NULL. We can not use the snd_soc_component_module_get_when_open() with compress stream as:

int snd_soc_component_module_get(struct snd_soc_component *component,
				 struct snd_pcm_substream *substream,
				 int upon_open);

expects snd_pcm_substream and not snd_compr_stream.

I should have used snd_soc_component_compr_module_get_when_open() and snd_soc_component_compr_module_put_when_close() to make it a cleaner

It might worth adding another mark to struct snd_soc_component, like struct snd_compr_stream *mark_compr_module;
or
convert the struct snd_pcm_substream *mark_module; to void *mark_module;
or
convert all marks to void *

IN the case of the later two the snd_soc_component_module_get() should have void *for the substream, probably named asmark`

[ujfalusi]

Should we try to send it upstream to get feedback which way this should be?
I have now the renamed version locally, introducing snd_soc_component_compr_module_get_when_open() and snd_soc_component_compr_module_put_when_close()

We piggyback on compr_open mark to manage the module_get/put to avoid introducing bigger changes for the framework and at the end the same thing is done for the normal components.

[plbossart]

I don't get the concept of passing a void pointer without any indication of size to the client. How would they know how much data they can read from the 'full_msg' area?

[ujfalusi]

It is the full (and valid IPC message) and the handlers should cast it to the struct they use internally. The size of the message is in the message itself, in the header.
We could use struct sof_ipc_cmd_hdr *full_msg, but it is not really the full message or we can say that it is a pointer to a header, but then it has to be documented that the header pointer is pointing to the beginning of the full message.

I prefer it to be void and let the handlers decide how to interpret the message they are given, in the 'core' we don't really care about any of the details.

[plbossart]

squash this change in previous patch?

[ujfalusi]

The main reason for moving this is the introduction of the notification for clients which uses sdev->client_event_handler_mutex and sdev->fw_state_handler_list. We can not set the fw_state before those are initialized.
This is why this change is in this commit.

[plbossart]

I don't get why you are transposing the logic by doing ipc_msg_data() and then testing the callback.
Couldn't this done in an earlier patch if this is necessary so that this patch only adds the state notification?

[ujfalusi]

Before this patch I only allocated memory for the full_msg and retrieved the message from firmware if we had a handler for it. True, that it is unlikely, but it sounded the right thing to do. I could have done it with goto like this:

+ if (!rx_callback)
+ 	goto out;

+ /* read the full message as there we have rx handler for it */
+ full_msg = kmalloc(hdr.size, GFP_KERNEL);
+ if (!full_msg)
+ 	return;

+ err = snd_sof_ipc_msg_data(sdev, NULL, full_msg, hdr.size);
+ if (err < 0)
+ 	dev_err(sdev->dev, "failed to read message\n");
+ else
+ 	rx_callback(sdev, full_msg);

+ kfree(full_msg);

+ out:
ipc_log_header(sdev->dev, "ipc rx done", hdr.cmd);

if you prefer that.

Here we always need to allocate the full_msg as we don't know if there is a client waiting for the exact notification.

[plbossart]

I still don't get why this is necessary now that you provide a state information to the clients.

Or maybe you need to add a FW_SUSPEND/RESUME state and provide it clients which cannot do power management on their own.

[ujfalusi]

Right, I think clients should be able to use the entering to SOF_FW_BOOT_NOT_STARTED as suspend and entering to SOF_FW_BOOT_COMPLETE as resume.

I think we can drop the sof_suspend_client()/ sof_resume_client() and related infra. Most likely, I need to check to be sure.

[ujfalusi]

Right, I think clients should be able to use the entering to SOF_FW_BOOT_NOT_STARTED as suspend and entering to SOF_FW_BOOT_COMPLETE as resume.

I think we can drop the sof_suspend_client()/ sof_resume_client() and related infra. Most likely, I need to check to be sure.

@plbossart, I was too optimistic on this. Relying solely on the fw_state change instead resume/suspend auxiliary bus callbacks does not work well.
There are several issues right know, I was able to fix only one:
[1] SOF_FW_BOOT_COMPLETE state is set at a wrong point in time
It is set directly when the SOF_IPC_FW_READY is received and it is OK, however the DSP is not yet ready. We need the snd_sof_dsp_post_fw_run() to be run on HDA platforms.
I have fixed this with a new SOF_FW_BOOT_READY_OK which is set in ipc.c, then after the snd_sof_dsp_post_fw_run() we move to SOF_FW_BOOT_COMPLETE.
[2] On suspend we change the fw_state to SOF_FW_BOOT_NOT_STARTED after the DSP is powered off, clients can not clean and close things up at this point and causes lockups and DSP/HDA to crash completely
Moving the state change before the snd_sof_dsp_runtime_suspend() / snd_sof_dsp_suspend() is kind of fixes it, but it is not a valid place to say the the DSP is off as it is not yet.
[3] With workaround for [1] and [2] there is a race which manifests on resume (runtime) that we are trying to send IPC message while we are still processing the SOF_IPC_FW_READY - the ipc rx done: 0x70000000: FW_READY is not printed before the dma-trace tries to re-enable the trace via the callback from state change.

Introducing another set of fw_states (SOF_FW_SUSPENDING/RESUMING) might give us a tool to work around some of the issues but the state handling around SOF must be revisited as SUSPENDING is essentially BOOT_COMPLETE during it's time, but RESUMING is a bit trickier. It is also a question on when to set the SUSPENDING state, most likely in the middle of the suspend function, which is not nice...

[plbossart]

what happened before this test was introduced? Did we get an error or a kernel oops?

[ujfalusi]

The first IPC tx would time-out (error prints from the core) and we aborted the test (error print in the flood test).
We could print in dmesg that the test can not be run due to the fact that the firmware is thought to be crashed.

There is definitely no point of trying to execute the test in this case.

[ujfalusi]

@ujfalusi I see this series has grown to 32 commits and there are really a lot of opens left and right that could be IMHO addressed later.

I got requests for new features and I have kept patches as separate for easier review (new functionalities are like this), I can squash things down quite a bit, but new core features that we need to have for the SOF client infra to be operational are piling up. They could be split to separate PR (like the new fw_state).

I also have tidy-up patches for the converted features, kept as separate to allow the conversion patch to be mostly a code move.

What if e.g. we only enable IPC flood and probes for SOF clients? The DMA trace seems to be problematic but it doesn't really bring any value to move as a client, does it?

At the end what we want is to have a core and clients. The core would provide interface for the clients to implement support for features within the SOF firmware or to communicate with the firmware. In this sense the dma-trace should not be in the core and I think with the state notification we no longer have anything big against it.
The PCM would be the next one in line to be moved as a client, then the real compressed stream support could follow, but there might be bigger implications with those (DAI/DAPM/topology and the card)

I think we should really look at realistic/smaller objectives and later on add more complicated scenarios/client types.

What I fear is that if we have only plan for the easy ones we would end up rewriting things to support the complicated components. The dma-trace was a good exercise to bring out some of the things we never would have needed for the ipc-flood and the probes, they are different types of clients.
It think with the three we are covering the feature set of SOF clients to come.

[ujfalusi]

Closing this as I have moved to #3136 due to the changes caused by squashing and new patch ordering.