ASoC: SOF: IPC: remove tx/rx work and do sending/receiving directly by keyonjie · Pull Request #581 · thesofproject/linux

keyonjie · 2019-01-23T06:37:14Z

This is aimed to fix #580

It is verified on APL UP2 with simultaneous pipelines stress test, both Tx and Rx(dma trace) messages works fine at that scenario.

It is not necessary to schedule another tx/rx thread to handle the
messages sending/receiving as our messages handlings are actually
happening in sequence, and ipc_lock can already handle any possible race
condition issues, Here remove tx/rx work from snd_sof_ipc structure and
do the related cleanup.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

plbossart

@keyonjie I really could use more explanations on such an intrusive patch.

why did we have workqueues in the first place? they may not be strictly necessary but are there any side effects.
what is the impact of removing them? lower latency?
Also I'd like to request a validation on more than one platform, we have 3 IPC types (baytrail, APL, CNL) so we should really test on all 3.

plbossart · 2019-01-23T15:59:22Z

so what happens if the DSP is busy? can we clarify what happens here?

If the DSP is busy(means transiting another ipc is in progress), we don't send ipc here, it may have chance to be sent after the one in transiting is done(at the bottom of wait_done()).

And, this logic stay no changed by my PR.

plbossart · 2019-01-23T16:01:38Z

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?

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?

ranj063 · 2019-01-23T21:29:38Z

@plbossart @keyonjie I have a couple of points that stand for this change and one that stands against

For:

I think the workqueues were added with the assumption that the ipc could be re-scheduled at a later time if the dsp is busy. But this never worked as intended.
Given that the IPC's are sequentially processed and we're queuing up the ipC's in a list, there's no need for a workqueue here.

Against:

If there is no workqueue, what would be our plan for a re-try mechanism that is currently put on hold?

keyonjie · 2019-01-24T07:57:21Z

@plbossart @keyonjie I have a couple of points that stand for this change and one that stands against

For:

I think the workqueues were added with the assumption that the ipc could be re-scheduled at a later time if the dsp is busy. But this never worked as intended.

Given that the IPC's are sequentially processed and we're queuing up the ipC's in a list, there's no need for a workqueue here.

Against:

If there is no workqueue, what would be our plan for a re-try mechanism that is currently put on hold?

We are removing work, but those un-delivered IPCs are still in our tx_list, we can re-try(just call tx function) at any place needed(e.g. we do it in wait_done()), so no problem here?

It is not necessary to schedule another tx/rx thread to handle the messages sending/receiving as our messages handlings are actually happening in sequence, and ipc_lock can already handle any possible race condition issues, Here remove tx/rx work from snd_sof_ipc structure and do the related cleanup. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie · 2019-01-24T10:40:28Z

@plbossart updated with comments changed.

plbossart · 2019-01-24T14:42:12Z

As I said yesterday, I am not going to touch any IPC-related changes until I see an explanation of how the IPC is supposed to work. I am done with incremental changes and legacy things that haven't been removed, let's document once and forall what the design is, and check that it flies before doing the changes. @keyonjie and @lgirdwood you are the de-facto owners of this work given that no one else has the historical context.

wenqingfu · 2019-01-27T16:18:34Z

I think there is a point here to clarify what we wanted IPC to be and why it "never worked as intended". Maybe it's a beautiful wish at the beginning to code the IPC in that way, but the complication of reality made us giving up?

Asynchronous communication sounds like a right thing for inter-processor communication, but literally all the tx callers in the code seems must wait to get a response from the other end before they go. Do they have to? Should the enhancement be on those callers to see if the tx call just return, thus the main thread don't have to sleep wait? The Linux kernel work queue is low priority task among all kinds of kernel threads and a shared runner, was it the purpose to employ it originally so that we are the good citizen in the system? What about the worst case of its latency? would it be a problem for audio?

More importantly, we should use caution to change the core code that have been proven it works, at least for now, unless there is a good reason to do so. To prove it's wrong is like a snap, but to prove it's the right thing you expected , it takes lots of stress testing...

RanderWang · 2019-01-28T08:38:34Z

This PR is not very complex and not very terrible.

Last year, I spent much time to prove to Liam that IPC msg should be serialized.
rx_list doesn't work as the one designed at first. There is only one rx msg now.
So we should refine the algorithm about rx_list.

For schedule(tx_list), I don't know why it is used. I don't see any benefit.
So I prefer this PR.

RanderWang · 2019-01-28T08:47:32Z

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


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

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

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.

keyonjie · 2019-01-28T09:07:47Z

Though codes are always changing and our time is limited, documentation is a perpetual topic, we all like codes documented well. The purpose of this PR is to try to make things simpler and our life easier, I want to shout and call for test with it on byt/bdw/cnl, any helps on that are greatly appreciated.

dbaluta · 2019-01-28T09:12:07Z

+/* 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;


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.

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

dbaluta · 2019-01-28T09:14:28Z

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


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?

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

plbossart · 2019-01-28T17:24:15Z

Last year, I spent much time to prove to Liam that IPC msg should be serialized.
rx_list doesn't work as the one designed at first. There is only one rx msg now.
So we should refine the algorithm about rx_list.

@RanderWang You've mentioned several times that RX doesn't work and we "should refine the algorithm" and I personally have no idea what you are referring to. I'd really like things to be explained, otherwise we will not collectively progress.

plbossart · 2019-01-28T17:26:22Z

Though codes are always changing and our time is limited, documentation is a perpetual topic, we all like codes documented well. The purpose of this PR is to try to make things simpler and our life easier, I want to shout and call for test with it on byt/bdw/cnl, any helps on that are greatly appreciated.

This PR does not make my life easier as a maintainer, it raises a number of questions and leaves me scratching my head as to why the whole thing works and what are the benefits/issues of the current solution and the one you are pushing.

lgirdwood · 2019-01-31T21:39:47Z

@keyonjie we should not be removing the Tx/Rx work Qs and message lists.

RX thread. Used to process DSP replies and notifications in non atomic context. Standard Linux IRQ handling.

TX thread. We can have multiple async message initiators (MFD in the future) so using a sender thread that checks the HW state and message Q means all initiators can sleep & block whilst the thread processes the TX Q. Things to check.

Locking. List access, mailbox and doorbell state operations must all be atomic.
Serialization. All IO must be serialized and in order.

lgirdwood · 2019-02-01T00:05:29Z

@keyonjie here's an issue that causes an IPC timeout

send new IPC, but DSP is busy with previous IPC so work is not scheduled.
current IPC completes.
IPC from 1 still not sent and times out.

@plbossart @ranj063 can we try something like this where we check the Tx Q after current Tx IPC completes.

diff --git a/sound/soc/sof/intel/hda-ipc.c b/sound/soc/sof/intel/hda-ipc.c
index 9ddfa9e1f968..fa5830c73872 100644
--- a/sound/soc/sof/intel/hda-ipc.c
+++ b/sound/soc/sof/intel/hda-ipc.c
@@ -203,6 +203,8 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context)
                                        HDA_DSP_ADSPIC_IPC, HDA_DSP_ADSPIC_IPC);
        }
 
+       snd_sof_ipc_msgs_tx(sdev);
+
        /* wake up sleeper if we are loading code */
        if (sdev->code_loading) {
                sdev->code_loading = 0;

ranj063 · 2019-02-01T00:34:14Z

@keyonjie here's an issue that causes an IPC timeout

send new IPC, but DSP is busy with previous IPC so work is not scheduled.

current IPC completes.

IPC from 1 still not sent and times out.

@lgirdwood I have always suspected this problem and even filed an issue earlier to track this but closed it because I couldnt show proof or logs. let me look into it once I solve my PM and module load/unload problems.

@plbossart @ranj063 can we try something like this where we check the Tx Q after current Tx IPC completes.

diff --git a/sound/soc/sof/intel/hda-ipc.c b/sound/soc/sof/intel/hda-ipc.c
index 9ddfa9e1f968..fa5830c73872 100644
--- a/sound/soc/sof/intel/hda-ipc.c
+++ b/sound/soc/sof/intel/hda-ipc.c
@@ -203,6 +203,8 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context)
                                        HDA_DSP_ADSPIC_IPC, HDA_DSP_ADSPIC_IPC);
        }
 
+       snd_sof_ipc_msgs_tx(sdev);
+
        /* wake up sleeper if we are loading code */
        if (sdev->code_loading) {
                sdev->code_loading = 0;

wenqingfu · 2019-02-01T03:55:10Z

I think this is even called a "normal timeout" - it's a timeout, but it'll be "re-try" at the end of tx_wait_done. I think this is what @keyonjie meant in early reply. In the inter-processor communication, there must be a prepare for timeout because one never knows what's going to happen in the other end.

however, I see sending a DONE message to DSP in this situation could be problematic and have consequence on future IPC transactions.

In a more complicated corner case, say there are two tasks of 'aplay' run, A and B. Either one of them sends a tx message into DSP, say A send one called AT, and DSP returns a reply, say AR.
Task A was waken by the IPC irq thread. However, due to kernel scheduler, task A won't necessarily be scheduled to run instantly. there is no grantee of when. The reply stays in the mailbox to be read. If one of the timeout scenario above happened and it kicks in randomly, a DONE message would lift the bar of tx list and now DSP can be tested as 'ready'. If this time a task B pushes a tx message into DSP, DSP returns a reply, which can override the mailbox that A would read when it's finally loaded into CPU to run... possible?

Also, I think using the msg header as 'token' to match rx with tx message is limited. Why not use a few bits in the message header as a rolling sequence number for this purpose?

keyonjie · 2019-02-20T02:51:51Z

@keyonjie we should not be removing the Tx/Rx work Qs and message lists.

RX thread. Used to process DSP replies and notifications in non atomic context. Standard Linux IRQ handling.

I did PR to remove RX thread at the same time as:

Our handling to replies is rapid: we only mask msg->ipc_complete to be true and wakeup waitq and return.
With respect to handling to notification(fw_ready, trace/stream position update, ..., new added async notification for Keyword Detection), we handle position update rapidly, but for fw_ready and especially the new added notification for KD, it may take noticeable time to handle them, so now I am open to keep this RX thread for those cases.

TX thread. We can have multiple async message initiators (MFD in the future) so using a sender thread that checks the HW state and message Q means all initiators can sleep & block whilst the thread

IMO, multiple async message initiators doesn't necessarily result in using a separated TX thread, with my PR applied, we still support multiple async message initiators.

processes the TX Q. Things to check.

Locking. List access, mailbox and doorbell state operations must all be atomic.

Yes, now the mailbox and doorbell state operations are atomic, but the tx list access isn't atomic with others together, List access including adding and removing, now we do it like this:

atomic 1: (initiator) add tx message to tx list.
atomic 2: check dsp ready, send message(mailbox and doorbell), and removing the message from tx list.

We don't make "atomic 1 + atomic 2 = a big atomic" yet, this allow different initiator adding messages to tx list before previous ones are done, but this is an open IMO, if we are all aligned to do "atomic 1 + atomic 2 = a big atomic", then we actually don't need the TX list -- we won't add a new message before the previous one finished.

Serialization. All IO must be serialized and in order.

keyonjie · 2019-02-20T03:10:02Z

@keyonjie here's an issue that causes an IPC timeout

send new IPC, but DSP is busy with previous IPC so work is not scheduled.

current IPC completes.

@lgirdwood we already have tx next logic at current IPC completes:

	/* continue to send any remaining messages... */
	ipc_tx_next_msg(sdev->ipc);

So, here after 2 done, IPC from 1 should be sent soon.

IPC from 1 still not sent and times out.

@ranj063 mentioned it is better to refine that the time we used to calculate for timeout:
#395 (comment)

This could be an enhancement, but I don't think it will lead to IPC timeout in normal cases.

@plbossart @ranj063 can we try something like this where we check the Tx Q after current Tx IPC completes.

diff --git a/sound/soc/sof/intel/hda-ipc.c b/sound/soc/sof/intel/hda-ipc.c
index 9ddfa9e1f968..fa5830c73872 100644
--- a/sound/soc/sof/intel/hda-ipc.c
+++ b/sound/soc/sof/intel/hda-ipc.c
@@ -203,6 +203,8 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context)
                                        HDA_DSP_ADSPIC_IPC, HDA_DSP_ADSPIC_IPC);
        }
 
+       snd_sof_ipc_msgs_tx(sdev);
+
        /* wake up sleeper if we are loading code */
        if (sdev->code_loading) {
                sdev->code_loading = 0;

plbossart · 2019-03-05T21:54:36Z

closing since @lyakh is working on a better IPC

keyonjie requested review from RanderWang, lgirdwood, plbossart and ranj063 January 23, 2019 06:37

plbossart reviewed Jan 23, 2019

View reviewed changes

keyonjie force-pushed the ipc_work_remove branch from d2bf166 to de810c9 Compare January 24, 2019 10:38

RanderWang reviewed Jan 28, 2019

View reviewed changes

dbaluta reviewed Jan 28, 2019

View reviewed changes

mengdonglin added the upstream blocker Mandatory for upstream label Jan 31, 2019

keyonjie mentioned this pull request Feb 20, 2019

[DO NOT MERGE] Ipc reply list fixes #584

Closed

plbossart closed this Mar 5, 2019

Conversation

keyonjie commented Jan 23, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

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!

ranj063 commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keyonjie commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keyonjie commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Jan 24, 2019

Uh oh!

wenqingfu commented Jan 27, 2019

Uh oh!

RanderWang commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented Jan 28, 2019

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!

plbossart commented Jan 28, 2019

Uh oh!

plbossart commented Jan 28, 2019

Uh oh!

lgirdwood commented Jan 31, 2019

Uh oh!

lgirdwood commented Feb 1, 2019

Uh oh!

ranj063 commented Feb 1, 2019 • edited by wenqingfu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wenqingfu commented Feb 1, 2019

Uh oh!

keyonjie commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keyonjie commented Feb 20, 2019

Uh oh!

plbossart commented Mar 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ranj063 commented Jan 23, 2019 •

edited

Loading

keyonjie commented Jan 24, 2019 •

edited

Loading

keyonjie commented Jan 24, 2019 •

edited

Loading

RanderWang commented Jan 28, 2019 •

edited

Loading

keyonjie Jan 28, 2019 •

edited

Loading

ranj063 commented Feb 1, 2019 •

edited by wenqingfu

Loading

keyonjie commented Feb 20, 2019 •

edited

Loading