Skip to content

ASoC: SOF: IPC: remove tx/rx work and do sending/receiving directly #581

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
keyonjie wants to merge 1 commit into from
Closed

ASoC: SOF: IPC: remove tx/rx work and do sending/receiving directly #581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
89 changes: 34 additions & 55 deletions sound/soc/sof/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,10 @@ static void ipc_stream_message(struct snd_sof_dev *sdev, u32 msg_cmd);
struct snd_sof_ipc {

@plbossart plbossart Jan 23, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

adding this here since GitHub will not let me comment on the commit message:
Is there any reason why we had workqueues in the first place? I find it hard to believe that this was added for no good reason. @lgirdwood can you comment?

@keyonjie keyonjie Jan 24, 2019

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.

Maybe we hoped that IPC sending can be more responsive? User only call tx function and we add it to our list, schedule task and then we can return rapidly. We had no_wait mode for sending IPCs which is never used in current drivers.

@lgirdwood can you chime in?

struct snd_sof_dev *sdev;

/* TX message work and status */
/* TX message status */
wait_queue_head_t wait_txq;
struct work_struct tx_kwork;
u32 msg_pending;

/* Rx Message work and status */
struct work_struct rx_kwork;

/* lists */
struct list_head tx_list;
struct list_head reply_list;
Expand Down Expand Up @@ -191,6 +187,34 @@ static inline void ipc_log_header(struct device *dev, u8 *text, u32 cmd)
}
#endif

/* caller need hold ipc_lock, send next IPC message in list */
static void ipc_tx_next_msg_unlock(struct snd_sof_ipc *ipc)
{
struct snd_sof_dev *sdev = ipc->sdev;

@dbaluta dbaluta Jan 28, 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.

So, we either call this ipc_tx_next_msg_unlocked or __ipc_tx_next_msg. The "unlock" at the end suggests that this function will "unlock" something.

@keyonjie keyonjie Jan 28, 2019

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.

"unlock" means this function itself will not hold the lock, the caller please hold the ipc lock.

struct snd_sof_ipc_msg *msg;

/* send message if HW read and message in TX list */
if (list_empty(&ipc->tx_list) || !snd_sof_dsp_is_ready(sdev))
return;

/* send first message in TX list */
msg = list_first_entry(&ipc->tx_list, struct snd_sof_ipc_msg, list);
list_move(&msg->list, &ipc->reply_list);
snd_sof_dsp_send_msg(sdev, msg);

ipc_log_header(sdev->dev, "ipc tx", msg->header);
}

/* send next IPC message in list */
static void ipc_tx_next_msg(struct snd_sof_ipc *ipc)
{
struct snd_sof_dev *sdev = ipc->sdev;

spin_lock_irq(&sdev->ipc_lock);
ipc_tx_next_msg_unlock(ipc);
spin_unlock_irq(&sdev->ipc_lock);
}

/* wait for IPC message reply */
static int tx_wait_done(struct snd_sof_ipc *ipc, struct snd_sof_ipc_msg *msg,
void *reply_data)
Expand Down Expand Up @@ -235,8 +259,8 @@ static int tx_wait_done(struct snd_sof_ipc *ipc, struct snd_sof_ipc_msg *msg,

snd_sof_dsp_cmd_done(sdev, SOF_IPC_DSP_REPLY);

/* continue to schedule any remaining messages... */
snd_sof_ipc_msgs_tx(sdev);
/* continue to send any remaining messages... */
ipc_tx_next_msg(sdev->ipc);
Comment thread
keyonjie marked this conversation as resolved.
Outdated

return ret;
}
Expand Down Expand Up @@ -270,9 +294,8 @@ int sof_ipc_tx_message(struct snd_sof_ipc *ipc, u32 header,
/* add message to transmit list */
list_add_tail(&msg->list, &ipc->tx_list);

/* schedule the message if not busy */
if (snd_sof_dsp_is_ready(sdev))
schedule_work(&ipc->tx_kwork);
/* try to send the message */
ipc_tx_next_msg_unlock(ipc);

@dbaluta dbaluta Jan 28, 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.

So, what happens if ipc_tx_next_msg_unlock fails to send the message, because DSP is already busy with another message?

Will tx_wait_done return an error right?

@keyonjie keyonjie Jan 28, 2019

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.

the next success at tx_wait_done will call ipc_tx_next_msg() to transmit next message in tx_list.


spin_unlock_irq(&sdev->ipc_lock);

Expand All @@ -281,30 +304,6 @@ int sof_ipc_tx_message(struct snd_sof_ipc *ipc, u32 header,
}
EXPORT_SYMBOL(sof_ipc_tx_message);

/* send next IPC message in list */
static void ipc_tx_next_msg(struct work_struct *work)
{
struct snd_sof_ipc *ipc =
container_of(work, struct snd_sof_ipc, tx_kwork);
struct snd_sof_dev *sdev = ipc->sdev;
struct snd_sof_ipc_msg *msg;

spin_lock_irq(&sdev->ipc_lock);

/* send message if HW read and message in TX list */
if (list_empty(&ipc->tx_list) || !snd_sof_dsp_is_ready(sdev))
goto out;

/* send first message in TX list */
msg = list_first_entry(&ipc->tx_list, struct snd_sof_ipc_msg, list);
list_move(&msg->list, &ipc->reply_list);
snd_sof_dsp_send_msg(sdev, msg);

ipc_log_header(sdev->dev, "ipc tx", msg->header);
out:
spin_unlock_irq(&sdev->ipc_lock);
}

/* find original TX message from DSP reply */
static struct snd_sof_ipc_msg *sof_ipc_reply_find_msg(struct snd_sof_ipc *ipc,
u32 header)
Expand Down Expand Up @@ -374,11 +373,8 @@ int snd_sof_ipc_reply(struct snd_sof_dev *sdev, u32 msg_id)
EXPORT_SYMBOL(snd_sof_ipc_reply);

/* DSP firmware has sent host a message */
static void ipc_msgs_rx(struct work_struct *work)
void snd_sof_ipc_msgs_rx(struct snd_sof_dev *sdev)
{
struct snd_sof_ipc *ipc =
container_of(work, struct snd_sof_ipc, rx_kwork);
struct snd_sof_dev *sdev = ipc->sdev;
struct sof_ipc_cmd_hdr hdr;
u32 cmd, type;
int err = -EINVAL;
Expand Down Expand Up @@ -433,19 +429,6 @@ static void ipc_msgs_rx(struct work_struct *work)
/* tell DSP we are done */
snd_sof_dsp_cmd_done(sdev, SOF_IPC_HOST_REPLY);
}

/* schedule work to transmit any IPC in queue */
void snd_sof_ipc_msgs_tx(struct snd_sof_dev *sdev)
{
schedule_work(&sdev->ipc->tx_kwork);
}
EXPORT_SYMBOL(snd_sof_ipc_msgs_tx);

/* schedule work to handle IPC from DSP */
void snd_sof_ipc_msgs_rx(struct snd_sof_dev *sdev)

@RanderWang RanderWang Jan 28, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

snd_sof_ipc_msgs_rx is called by IRQ function. Now with this change, there is a lot of work done in IRQ function. It is not a good idea. We should do some work in another thread to make IRQ function simpler

@keyonjie keyonjie Jan 28, 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.

This is a good point, let me do an update for it before vacation.

@dbaluta dbaluta Jan 28, 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.

This would also be helpful for my i.MX implementation. Anyhow, particularly for Intel snd_sof_ipc_msgs_rx is called from IRQ thread not from IRQ handler. So, would creating another thread make sense? I'm not sure for threaded interrupts what is the priority of the interrupt thread comparing with a normal thread.

{
schedule_work(&sdev->ipc->rx_kwork);
}
EXPORT_SYMBOL(snd_sof_ipc_msgs_rx);

/*
Expand Down Expand Up @@ -769,8 +752,6 @@ struct snd_sof_ipc *snd_sof_ipc_init(struct snd_sof_dev *sdev)
INIT_LIST_HEAD(&ipc->reply_list);
INIT_LIST_HEAD(&ipc->empty_list);
init_waitqueue_head(&ipc->wait_txq);
INIT_WORK(&ipc->tx_kwork, ipc_tx_next_msg);
INIT_WORK(&ipc->rx_kwork, ipc_msgs_rx);
ipc->sdev = sdev;

/* pre-allocate messages */
Expand Down Expand Up @@ -803,7 +784,5 @@ EXPORT_SYMBOL(snd_sof_ipc_init);

void snd_sof_ipc_free(struct snd_sof_dev *sdev)
{
cancel_work_sync(&sdev->ipc->tx_kwork);
cancel_work_sync(&sdev->ipc->rx_kwork);
}
EXPORT_SYMBOL(snd_sof_ipc_free);
1 change: 0 additions & 1 deletion sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,6 @@ struct snd_sof_ipc *snd_sof_ipc_init(struct snd_sof_dev *sdev);
void snd_sof_ipc_free(struct snd_sof_dev *sdev);
int snd_sof_ipc_reply(struct snd_sof_dev *sdev, u32 msg_id);
void snd_sof_ipc_msgs_rx(struct snd_sof_dev *sdev);
void snd_sof_ipc_msgs_tx(struct snd_sof_dev *sdev);
int snd_sof_ipc_stream_pcm_params(struct snd_sof_dev *sdev,
struct sof_ipc_pcm_params *params);
int snd_sof_dsp_mailbox_init(struct snd_sof_dev *sdev, u32 dspbox,
Expand Down