Skip to content

SOF client support (auxiliary bus) #3007

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 32 commits into from
Closed

SOF client support (auxiliary bus) #3007

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
a87a2b5
ASoC: SOF: Move the definition of enum snd_sof_fw_state to global header
ujfalusi Aug 13, 2021
4d512ec
ASoC: SOF: Introduce a macro to set the firmware state
ujfalusi Aug 16, 2021
44b2a43
ASoC: SOF: debug: Print out the fw_state along with the DSP dump
ujfalusi Aug 18, 2021
9115f5b
ASoC: SOF: Introduce new firmware state: SOF_FW_CRASHED
ujfalusi Aug 17, 2021
476d4c0
ASoC: soc-component: Add support module get/put on open for compresse…
ujfalusi Jul 23, 2021
7274b1c
ASoC: SOF: Introduce IPC SOF client support
ranj063 Oct 22, 2020
6a772ce
ASoC: SOF: core/ops: Add support for client registration
ranj063 Oct 22, 2020
a858cfa
ASoC: SOF: core: Unregister machine driver before IPC and debugfs
ujfalusi Jul 23, 2021
49abfdc
ASoC: SOF: ipc: Read and pass the whole message to handlers for IPC e…
ujfalusi Jul 6, 2021
11f3b3a
ASoC: SOF: clients: Add support for IPC rx and firmware state change
ujfalusi Jul 6, 2021
20c9323
ASoC: SOF: clients: Add support for auxdev suspend/resume handling
ujfalusi Jul 9, 2021
9e32894
ASoC: SOF: clients: Add API to get the SOF firmware version
ujfalusi Jul 7, 2021
bcaad35
ASoC: SOF: clients: Add API to manage the module refcount of SOF core
ujfalusi Jul 21, 2021
92a48c2
ASoC: SOF: intel: hda-trace: Pass the dma buffer pointer to hda_dsp_t…
ujfalusi Jul 9, 2021
9e675f2
ASoC: SOF: Split up utils.c into sof-utils and iomem-utils
ujfalusi Jul 15, 2021
deb5a0c
ASoC: SOF: Convert the generic IPC flood test into SOF client
ranj063 Oct 22, 2020
cba8df9
ASoC: SOF: sof-client-ipc-test: Protection against removal while in use
ujfalusi Jul 22, 2021
64a91b8
ASoC: SOF: sof-client-ipc-test: Block the test if the firmware has cr…
ujfalusi Aug 20, 2021
5f585a8
ASoC: SOF: Convert the generic probe support to SOF client
ujfalusi Jun 9, 2021
d6a888d
ASoC: SOF: sof-client-probes: Add module parameter to enable probes s…
ujfalusi Aug 9, 2021
6677eec
ASoC: SOF: sof-client-probes: Protection against removal while in use
ujfalusi Jul 22, 2021
68d6c63
ASoC: SOF: sof-client-probes: Block the capture if the firmware is cr…
ujfalusi Aug 20, 2021
39459f7
ASoC: SOF: Add optional SOF client for dma-trace support
ujfalusi Jul 6, 2021
9e6b984
ASoC: SOF: imx: Enable SOF client version of dma-trace
ujfalusi Aug 18, 2021
e8bab41
ASoC: SOF: Switch to the client driver for dma-trace support
ujfalusi Jul 19, 2021
5c75bc5
ASoC: SOF: sof-client-dma-trace: Add protection against file/module r…
ujfalusi Jul 21, 2021
b3089af
ASoC: SOF: sof-client-dma-trace: Block the dtrace if the firmware has…
ujfalusi Aug 20, 2021
b269552
ASoC: SOF: sof-client-dma-trace: Simplify count adjustment in trace_read
ujfalusi Jul 16, 2021
fd4a66c
ASoC: SOF: sof-client-dma-trace: Coding style cleanups
ujfalusi Jul 16, 2021
5e3f492
ASoC: SOF: sof-client-ipc-test: Code cleanup for consistency
ujfalusi Jul 22, 2021
f763eea
ASoC: SOF: sof-client-probes: Code cleanup for consistency
ujfalusi Jul 22, 2021
3bd20bb
ASoC: SOF: sof-client-dma-trace: Code cleanup for consistency
ujfalusi Jul 22, 2021
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
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 @@ -165,6 +165,9 @@ static int sof_resume(struct device *dev, bool runtime_resume)
return ret;
}

/* Notify clients about core resume */
sof_resume_clients(sdev);
Copy link
Copy Markdown
Member

@plbossart plbossart Aug 24, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi Aug 26, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi Aug 27, 2021

Choose a reason for hiding this comment

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

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


/* notify DSP of system resume */
ret = sof_send_pm_ctx_ipc(sdev, SOF_IPC_PM_CTX_RESTORE);
if (ret < 0)
Expand All @@ -178,6 +181,7 @@ static int sof_resume(struct device *dev, bool runtime_resume)
static int sof_suspend(struct device *dev, bool runtime_suspend)
{
struct snd_sof_dev *sdev = dev_get_drvdata(dev);
pm_message_t pm_state = {};
u32 target_state = 0;
int ret;

Expand Down Expand Up @@ -216,6 +220,9 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
/* release trace */
snd_sof_release_trace(sdev);

/* Notify clients about core suspend */
sof_suspend_clients(sdev, pm_state);
Comment thread
plbossart marked this conversation as resolved.

#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_ENABLE_DEBUGFS_CACHE)
/* cache debugfs contents during runtime suspend */
if (runtime_suspend)
Expand Down
46 changes: 46 additions & 0 deletions sound/soc/sof/sof-client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,52 @@ int sof_client_ipc_tx_message(struct sof_client_dev *cdev, void *ipc_msg,
}
EXPORT_SYMBOL_NS_GPL(sof_client_ipc_tx_message, SND_SOC_SOF_CLIENT);
Comment thread
plbossart marked this conversation as resolved.

int sof_suspend_clients(struct snd_sof_dev *sdev, pm_message_t state)
Copy link
Copy Markdown
Collaborator

@lyakh lyakh Jul 12, 2021

Choose a reason for hiding this comment

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

It seems strange to me that these are needed, that seems like something that the generic PM should be handling.

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi Jul 23, 2021

Choose a reason for hiding this comment

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

For some clients the pm_runtime() is the way to go, but for some it is not possible to use it, it is the dma-trace.
Let me replay to @plbossart with an explanation.

{
struct auxiliary_driver *adrv;
struct sof_client_dev *cdev;

mutex_lock(&sdev->ipc_client_mutex);

list_for_each_entry(cdev, &sdev->ipc_client_list, list) {
/* Skip devices without loaded driver */
if (!cdev->auxdev.dev.driver)
continue;

adrv = to_auxiliary_drv(cdev->auxdev.dev.driver);
if (adrv->suspend)
adrv->suspend(&cdev->auxdev, state);
Copy link
Copy Markdown
Member

@plbossart plbossart Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should do this, it's better to make a request_resume or request_suspend() than explicitly using the pointers. Let the PM frameworks work for you, don't use the suspend callback directly.

Copy link
Copy Markdown
Member

@plbossart plbossart Jul 22, 2021

Choose a reason for hiding this comment

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

still not aligned on this use of ->suspend...

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi Jul 23, 2021

Choose a reason for hiding this comment

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

right, let me try to explain on the resume side.

}

mutex_unlock(&sdev->ipc_client_mutex);

return 0;
}
EXPORT_SYMBOL_NS_GPL(sof_suspend_clients, SND_SOC_SOF_CLIENT);

int sof_resume_clients(struct snd_sof_dev *sdev)
{
struct auxiliary_driver *adrv;
struct sof_client_dev *cdev;

mutex_lock(&sdev->ipc_client_mutex);

list_for_each_entry(cdev, &sdev->ipc_client_list, list) {
/* Skip devices without loaded driver */
if (!cdev->auxdev.dev.driver)
continue;

adrv = to_auxiliary_drv(cdev->auxdev.dev.driver);
if (adrv->resume)
adrv->resume(&cdev->auxdev);
Copy link
Copy Markdown
Member

@plbossart plbossart Jul 21, 2021

Choose a reason for hiding this comment

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

same, don't use the PM callback directly, ask the PM framework to resume the device for you.

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi Jul 23, 2021

Choose a reason for hiding this comment

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

@plbossart, @lyakh, let me try to gather my thoughts and what I have faced with...

The reason why we need the auxiliary level of suspend/resume over the runtime_pm calls is coming from the dma-trace.
It is not an active feature, it does not need (and must not) keep the core powered on while we are using it. It shall only take data out from the DSP when it is powered and it sent a notification that the position moved.
The DSP will only send the notification about the position when it is powered up and there are traces coming from SOF.
The dma-trace is not a separate device, it is a feature of the firmware.
The dma-trace must not use any pm_runtime calls on it's device because if it does it will have effect on the parent's device: if the dma-trace does a pm_runtime_get_sync() on it's device then the parent will be also woken up and it is not going to be put to suspend unless the client (and all of them) put their device to suspend.

So if at any point the dam-trace 'wakes' up to do one pm_runtime_get_sync() then what is the point when it shall do pm_runtime_put()? The core will not go to suspend as the dma-trace is keeping it powered, the dma-trace would have no idea when it should do that.
Another issue is that if the client is gone to suspend and the core also, then a core resume will not trigger a resume on the client. We need some event to take the client device out from suspend and the runtime_pm is not propagating upwards, afaik.

We might be able to walk through the auxdevs and call pm_runtime_get_sync() / pm_runtime_put_sync() on them when the core is woken up.

But we have the other type of clients, the active ones.
They do manage their own runtime_pm state to force the core powered and let it suspend when they don't need it.
Probes and the ipc-test is such a client and the audio is going to be another one. When you open the compress/test/audio they would get_sync() and the core would woke up so they can perform their tasks.

But, if we do pm_runtime_get_sync() / pm_runtime_put_sync() from the core to all clients then an IPC test would trigger a resume for the probes, dma-trace, audio and all clients which does not make much sense...

So we have two types of clients and they need two types of handling.

Copy link
Copy Markdown
Collaborator

@ranj063 ranj063 Jul 23, 2021

Choose a reason for hiding this comment

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

@ujfalusi why do we need to suppose suspend/resume for the trace client at all? The only time trace sends IPC's is during init/free which are done during probe and remove and the core will always be active at that point no?

I dont think suspend/resuming clients from the core looks right at all

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi Jul 23, 2021

Choose a reason for hiding this comment

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

we need to send IPC when the DSP resumes also.

I can not find other ways to to handle the dma-trace feature of the SOF. It is a passive feature, it can not enforce PM state, it must follow the core and act on the change.

Do you see any other way to handle a feature of SOF which should be following and not dictating (or enforcing) power state for the DSP?
I think I have covered most of the points in my comment above yours for @plbossart and @lyakh, did I missed something?

Copy link
Copy Markdown
Collaborator

@ranj063 ranj063 Jul 24, 2021

Choose a reason for hiding this comment

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

What IPC does the trace client need to send for DSP resume? The core needs to send the CTX_RESTORE when the DSp is resumed. But I cant imagine the client having to send anything

Copy link
Copy Markdown
Collaborator

@ranj063 ranj063 Jul 24, 2021

Choose a reason for hiding this comment

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

Please help me understand, Peter. why does the trace client need PM. It's job is just to do debugfs read/write isnt it? and a write is performed when an IPC arrives from the DSP in which case the DSP is already in D0. The core simply needs to forward the ipC RX to the trace client. In the case of read, we dont really need the DSP to be in D0 right?

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi Jul 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The client conversion of dma-trace follows how the current code is written.
in sound/soc/sof/pm.c we have:

linux/sound/soc/sof/pm.c

Lines 150 to 157 in d54b780

/* resume DMA trace, only need send ipc */
ret = snd_sof_init_trace_ipc(sdev);
if (ret < 0) {
/* non fatal */
dev_warn(sdev->dev,
"warning: failed to init trace after resume %d\n",
ret);
}

And indeed, the snd_sof_init_trace_ipc() sends IPC.
If it is not needed then it must be removed (and rigorously tested) prior to client conversion.

The struct auxiliary_driver have the suspend / resume callbacks since the initial commit, no extended information available, only this:
The bus supports probe/remove shutdown and suspend/resume callbacks.
I'm certain that they are added for a reason and I would not be surprised if the reason is for exactly the same use case as we are using them.

let's theorize: we want to have a simple client driver which would light up an LED when the DSP is powered on. Nothing more, nothing less. How it can be done?

Copy link
Copy Markdown
Member

@plbossart plbossart Aug 11, 2021

Choose a reason for hiding this comment

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

isn't this the case of a 'no callback device' described in https://www.kernel.org/doc/html/latest/power/runtime_pm.html?highlight=pm_runtime#no-callback-devices

I think you need some sort of signaling between the SOF core and the DMA trace or your LED thingy - using a loop on resume doesn't seem quite right here.

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi Aug 13, 2021

Choose a reason for hiding this comment

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

@plbossart, yes it is and the dtrace client sets pm_runtime_no_callbacks()

I would love to use device_for_each_child() instead of our list, but sof_dev have other children than the clients.

There must be a reason why the auxdev have suspend and resume callbacks defined and I can not think of other use cases for them than this one.. In other places where the auxiliary bus is used have only one child so they obviously not doing any looping, but we are pushing this to it's limits. imho.

}

mutex_unlock(&sdev->ipc_client_mutex);

return 0;
}
EXPORT_SYMBOL_NS_GPL(sof_resume_clients, SND_SOC_SOF_CLIENT);

struct dentry *sof_client_get_debugfs_root(struct sof_client_dev *cdev)
{
return cdev->sdev->debugfs_root;
Expand Down
12 changes: 12 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 @@ -650,6 +650,8 @@ int sof_register_clients(struct snd_sof_dev *sdev);
void sof_unregister_clients(struct snd_sof_dev *sdev);
void sof_client_ipc_rx_dispatcher(struct snd_sof_dev *sdev, void *full_msg);
void sof_client_fw_state_dispatcher(struct snd_sof_dev *sdev);
int sof_suspend_clients(struct snd_sof_dev *sdev, pm_message_t state);
int sof_resume_clients(struct snd_sof_dev *sdev);
#else /* CONFIG_SND_SOC_SOF_CLIENT */
static inline int sof_client_dev_register(struct snd_sof_dev *sdev, const char *name,
u32 id, const void *data, size_t size)
Expand Down Expand Up @@ -679,6 +681,16 @@ static inline void sof_client_ipc_rx_dispatcher(struct snd_sof_dev *sdev,
static inline void sof_client_fw_state_dispatcher(struct snd_sof_dev *sdev)
{
}

static inline int sof_suspend_clients(struct snd_sof_dev *sdev, pm_message_t state)
{
return 0;
}

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

#endif