Skip to content

ASoC: SOF: discard sdev->waitq and sdev->code_loading#1189

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
aiChaoSONG wants to merge 2 commits into from
Closed

ASoC: SOF: discard sdev->waitq and sdev->code_loading #1189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c8bb56
ASoC: SOF: Intel: hda: ignore spurious replies such as ROM messages
Sep 17, 2019
f77dd19
ASoC: SOF: Intel: hda: remove unnecessary waitq when loading firmware
Sep 17, 2019
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
11 changes: 4 additions & 7 deletions sound/soc/sof/intel/cnl.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -59,13 +59,10 @@ static irqreturn_t cnl_ipc_irq_thread(int irq, void *context)

spin_lock_irq(&sdev->ipc_lock);

/* handle immediate reply from DSP core */
hda_dsp_ipc_get_reply(sdev);
snd_sof_ipc_reply(sdev, msg);

if (sdev->code_loading) {
sdev->code_loading = 0;
wake_up(&sdev->waitq);
/* do not check reply if no message was initiated */
Comment thread
plbossart marked this conversation as resolved.
if (sdev->msg) {
hda_dsp_ipc_get_reply(sdev);
snd_sof_ipc_reply(sdev, msg);
}

Comment thread
plbossart marked this conversation as resolved.
cnl_ipc_dsp_done(sdev);
Expand Down
16 changes: 2 additions & 14 deletions sound/soc/sof/intel/hda-ipc.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -121,12 +121,6 @@ void hda_dsp_ipc_get_reply(struct snd_sof_dev *sdev)

}

static bool hda_dsp_ipc_is_sof(uint32_t msg)
{
return (msg & (HDA_DSP_IPC_PURGE_FW | 0xf << 9)) != msg ||
(msg & HDA_DSP_IPC_PURGE_FW) != HDA_DSP_IPC_PURGE_FW;
}

/* IPC handler thread */
irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context)
{
Expand DownExpand Up@@ -172,18 +166,12 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context)
*/
spin_lock_irq(&sdev->ipc_lock);

/* handle immediate reply from DSP core - ignore ROM messages */
if (hda_dsp_ipc_is_sof(msg)) {
/* do not check reply if no message was initiated */
if (sdev->msg) {
Comment thread
plbossart marked this conversation as resolved.
hda_dsp_ipc_get_reply(sdev);
snd_sof_ipc_reply(sdev, msg);
}

/* wake up sleeper if we are loading code */
if (sdev->code_loading) {
sdev->code_loading = 0;
wake_up(&sdev->waitq);
}

/* set the done bit */
hda_dsp_ipc_dsp_done(sdev);

Expand Down
3 changes: 0 additions & 3 deletions sound/soc/sof/intel/hda-loader.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -167,9 +167,6 @@ static int cl_trigger(struct snd_sof_dev *sdev,
/* code loader is special case that reuses stream ops */
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
wait_event_timeout(sdev->waitq, !sdev->code_loading,
HDA_DSP_CL_TRIGGER_TIMEOUT);
Comment thread
plbossart marked this conversation as resolved.

@lyakhlyakhOct 14, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we can also remove the define for that timeout now?

@lyakhlyakhOct 14, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If this is correct, I'd be all for this - removing redundant synchronisation controls simplifies the code. But like others I cannot be completely certain that this is safe on all platforms. This assumes, that by the time we arrive here code_loading is already cleared and we don't wait anyway. So, can we add wait with this commit and add a

WARN_ON(sdev->code_loading);

here and run with that code for some time? After we've verified that that warning never triggers, we should be more confident, that it's safe to remove the wait queue. When removing it we still can keep the WARN().

@aiChaoSONGaiChaoSONGOct 14, 2019•
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We have 2 threads in the boot process in parallel, one is the boot thread, the other is the irq thread. once interrupt is initialized in boot thread, "PURGE FW IPC" initiated by previous purge FW request will propagate to irq thread. This IPC happens really early, even before interrupt initialization. but we have to deal with it(call hda_dsp_ipc_dsp_done to clear some bit), or further ipc will not work. Some later polls in cl_dsp_init(hda_loader.c) cause the execution of irq thread ahead of TRIGGER START, that's the reason fw still boot after removing boot_loading. Actually, we are depending on linux scheduler, and what I am really afraid is that if the irq thread not get executed before TRIGGER START, FW surely boot failed!!!(didn't see this till now) but boot_loading guaranteed the execution order. @keyonjie@plbossart btw, I think deal with ROM IPC with FW IRQ thread is not a good practice.


snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTCTL,
1 << hstream->index,
1 << hstream->index);
Expand Down
3 changes: 0 additions & 3 deletions sound/soc/sof/intel/hda.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -644,9 +644,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
hda_dsp_ctrl_ppcap_enable(sdev, true);
hda_dsp_ctrl_ppcap_int_enable(sdev, true);

/* initialize waitq for code loading */
init_waitqueue_head(&sdev->waitq);

/* set default mailbox offset for FW ready message */
sdev->dsp_box.offset = HDA_DSP_MBOX_UPLINK_OFFSET;

Expand Down
3 changes: 0 additions & 3 deletions sound/soc/sof/loader.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -427,9 +427,6 @@ int snd_sof_load_firmware_raw(struct snd_sof_dev *sdev)
const char *fw_filename;
int ret;

/* set code loading condition to true */
sdev->code_loading = 1;

/* Don't request firmware again if firmware is already requested */
if (plat_data->fw)
return 0;
Expand Down
4 changes: 0 additions & 4 deletions sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -424,10 +424,6 @@ struct snd_sof_dev {
int ipc_timeout;
int boot_timeout;

/* Wait queue for code loading */
wait_queue_head_t waitq;
int code_loading;

/* DMA for Trace */
struct snd_dma_buffer dmatb;
struct snd_dma_buffer dmatp;
Expand Down