[RFC] ASoC: SOF: merge ipc and stream irq by bardliao · Pull Request #1483 · thesofproject/linux

bardliao · 2019-11-14T07:22:01Z

We may lost interrupt on MSI mode if two interrupts are triggered at
the same time. Use single handler and check both interrupts can avoid
that.

Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com

RanderWang · 2019-11-14T09:43:49Z

+irqreturn_t hda_dsp_interrupt_handler(int irq, void *context)
+{
+	if ((hda_dsp_ipc_irq_handler(irq, context) == IRQ_WAKE_THREAD) ||
+	    (hda_dsp_stream_interrupt(irq, context) == IRQ_WAKE_THREAD))


how about to change hda_dsp_stream_interrupt to hda_dsp_stream_irq_handler like hda_dsp_ipc_irq_handler

Thanks @RanderWang . I renamed them to is_stream_irq() and is_ipc_irq()

plbossart

Thanks @bardliao, looks mostly good, I suggested some improvements to make the code more readable.

plbossart · 2019-11-14T14:06:20Z

+irqreturn_t hda_dsp_interrupt_handler(int irq, void *context)
+{
+	if ((hda_dsp_ipc_irq_handler(irq, context) == IRQ_WAKE_THREAD) ||
+	    (hda_dsp_stream_interrupt(irq, context) == IRQ_WAKE_THREAD))


I would change the return type of those functions to a boolean, e.g.

if (hda_dsp_check_ipc_irq(irq, context) || hda_dsp_check_stream_irq(irq, context)) return IRQ_WAKE_THREAD;

@plbossart Done. But I add a new function instead of changing hda_dsp_ipc_irq_handler() since it is used in somewhere else.

plbossart · 2019-11-14T14:10:55Z


-	spin_lock(&sdev->hw_lock);
+	/* The function can be called at irq thread, so use spin_lock_irq */
+	spin_lock_irq(&sdev->hw_lock);


question:
I can never remember the rules for spin_locks.

Do we need to protect with the _irq suffix only here, or also in the thread?

I believe it has to be protected with _irq in the thread and not necessarily in the IRQ handler (you'd use _irq if you're not 100% certain the code runs only in IRQ handlers). IIRC, that is.

Thanks @paulstelian97 for the explanation . @plbossart hda_dsp_ipc_irq_handler() will be called in the irq thread that's why I add the _irq suffix here. But actually I don't know if we do need the spin_lock or not. @RanderWang discussed it with me about it, but we didn't have a conclusion.

ghost · 2019-11-15T02:28:49Z

I use work in hda_dsp_interrupt_threaded() since we want to handle different interrupt in parallel. And it can prevent one is blocked by the other.

ghost · 2019-11-18T21:47:23Z

-	struct hdac_bus *bus = context;
-	int ret = IRQ_WAKE_THREAD;
+	struct hdac_bus *bus = sof_to_bus(sdev);
+	int ret = true;


ghost · 2019-11-18T21:48:38Z


+bool is_ipc_irq(struct snd_sof_dev *sdev)
+{
+	if (hda_dsp_ipc_irq_handler(0, sdev) == IRQ_WAKE_THREAD)


can't you remove this function and do all the work here?
Why is a wrapper needed with a first parameter 0 that seems useless?

Oh, I just want to reuse the code. Yes, I can remove that function.

ghost · 2019-11-18T21:51:18Z

+	struct snd_sof_dev *sdev =
+		container_of(work, struct snd_sof_dev, stream_work);
+
+	hda_dsp_stream_threaded_handler(0, sdev);


this is really odd, you are not using the first argument 0?

No, the first argument is never used.

@bardliao a general comment about the naming of the functions. We should only call the hard interrupt handler as somthing_thread_handler(), the thread itself should just be called irq_thread or the stream_thread

ghost · 2019-11-18T21:53:43Z

+	struct snd_sof_dev *sdev =
+		container_of(work, struct snd_sof_dev, ipc_work);
+
+	sof_ops(sdev)->irq_thread(0, sdev);


is this meant to handle the APL/CNL difference only?

Yes, currently, hda_dsp_probe() is for APL/CNL only and they have different .irq_thread.

ghost · 2019-11-19T08:28:21Z

+	if (is_ipc_irq(sdev))
+		schedule_work(&sdev->ipc_work);
+	if (is_stream_irq(sdev))
+		schedule_work(&sdev->stream_work);


@bardliao why we need to process interrupt in second level scheduled work ? hda_dsp_interrupt_threaded is in a work scheduled by interrupt framework.

@RanderWang hda_dsp_stream_threaded_handler() could take a long time, and we don't wan't one threaded handler to wait for the other. Besides, we may want to use loop on each threaded handler in the future.

@RanderWang has a point though.what would happen if we didn't not use a threaded handler, and called schedule_work directly from the handler?

@bardliao I'm a bit concerned about this as well. We are essentially adding two mandatory context switches for every interrupt. First from hard-irq to irq thread, and then from irq thread to work-queue. Do you have concrete logs showing that the threaded handler is taking a lot of time?

I also agree that this seems a bit of an overkill. Instead we should just wake the appropriate thread in the handler based on is_ipc_irq() or is_stream_irq() is true.

Thank you all. Yes, it would be better to wake the appropriate thread in the handler.

@RanderWang hda_dsp_stream_threaded_handler() could take a long time, and we don't wan't one threaded handler to wait for the other. Besides, we may want to use loop on each threaded handler in the future.

Actually, the two works are still scheduled on the same CPU because scheduler is designed to utilize cache data (please check scheduler documents), so this can't save time and spend extra time to schedule work.

@RanderWang has a point though.what would happen if we didn't not use a threaded handler, and called schedule_work directly from the handler?

We should follow the requirement of request_threaded_irq
please check the comments for request_threaded_irq in kernel.

* If you want to set up a threaded irq handler for your device * then you need to supply @handler and @thread_fn. @handler is * still called in hard interrupt context and has to check * whether the interrupt originates from the device. If yes it * needs to disable the interrupt on the device and return * IRQ_WAKE_THREAD which will wake up the handler thread and run * @thread_fn. This split handler design is necessary to support * shared interrupts.

yes, this was a completely dumb comment from me. Oops :-)
I think what I was trying to say if whether the same handler can trigger multiple threads, but that seems like asking for trouble.

ghost

As overall design, I think this is the way to go. I have some concerns on the specific implementation, see comments inline.

ghost · 2019-11-19T15:58:46Z

+		ret = false;

-	spin_unlock(&bus->reg_lock);
+	spin_unlock_irq(&bus->reg_lock);


This is a bit asymmetric in that is_ipc() function disabled IPC interrupts in this check, while this function does not disable the interrupt source? Why is this?

Good question. I think we should disable it as well, but we can do it in another PR.

yes, please check the comments for request_threaded_irq

If yes it needs to disable the interrupt on the device and return IRQ_WAKE_THREAD which will wake up the handler thread and run @thread_fn. This split handler design is necessary to support shared interrupts.

Ack @bardliao and @RanderWang This is matching existing code, so I'm ok to change in another PR. I do have a comment on naming the is_ipc_irq() functions (see my other comment).

ghost · 2019-11-19T16:01:44Z

-				   "AudioDSP", sdev);
+	ret = request_threaded_irq(sdev->ipc_irq, hda_dsp_interrupt_handler,
+				   hda_dsp_interrupt_threaded,
+				   IRQF_SHARED, "AudioDSP", sdev);


@bardliao Shouldn't this patch remove hda_dsp_ipc_irq_handler as well?

hda_dsp_ipc_irq_handler is assigned to cnl and apl's irq_handler ops. But no one uses it. So, yes, we can remove hda_dsp_ipc_irq_handler as well.

ghost · 2019-11-19T16:04:42Z

+	if (is_ipc_irq(sdev))
+		schedule_work(&sdev->ipc_work);
+	if (is_stream_irq(sdev))
+		schedule_work(&sdev->stream_work);


@bardliao I'm a bit concerned about this as well. We are essentially adding two mandatory context switches for every interrupt. First from hard-irq to irq thread, and then from irq thread to work-queue. Do you have concrete logs showing that the threaded handler is taking a lot of time?

ghost · 2019-11-19T16:51:28Z

+	u32 irq_status;
+
+	/* The function can be called at irq thread, so use spin_lock_irq */
+	spin_lock_irq(&sdev->hw_lock);


@bardliao I'm not sure about this. I dont think we need to use spin_lock_irq(). Back when we're still upstreaming the SOF driver, IIRC the comment upstream was that spin_lock_irq() should only be in the hard interrupt handler.

Thanks. I will double check it.

@ranj063 I can see spin_lock() is called twice without spin_unlock() if I test it with spin_lock() instead of spin_lock_irq(). I think the reason is that the irq did not be disabled when we use spin_lock() and the irq handler could be triggered when the irq thread is still running. Also, spin_lock_irq() is using in hda_dsp_stream_threaded_handler() which is a irq thread actually. So I believe that we should use spin_lock_irq()

@bardliao @ranj063 You definitely need to use the "_irq()" variant here. These is_xxx_irq() functions are called both from hard-irq and softirq context, so plain spin_lock() will not work.

ghost · 2019-11-19T16:53:17Z

-		hdev->irq = pci_irq_vector(pci, 0);
-		/* ipc irq number is the same of hda irq */
-		sdev->ipc_irq = hdev->irq;
+		sdev->ipc_irq = pci_irq_vector(pci, 0);


@bardliao if this is a common irq, we should consider renaming it to maybe just irq?

Yeah, I also think about it. However, for those platforms without HDA, the irq is still for ipc only. That's why I leave it as it is for now.

@bardliao even in that case it makes sense to rename it to something else. Maybe in another PR

ghost · 2019-11-19T17:03:41Z

@bardliao there was specific change we made in the stream interrupt handler to handled unsolicited responses from the HDA codecs (ie looping 10 times) and checking INTSTS. This is was fixed the IPC timeouts for us. Now, in this PR, you are suggesting moving it to the IRQ thread. I'm a bit concerned this could cause regressions though.

ghost · 2019-11-20T00:09:11Z

@bardliao there was specific change we made in the stream interrupt handler to handled unsolicited responses from the HDA codecs (ie looping 10 times) and checking INTSTS. This is was fixed the IPC timeouts for us. Now, in this PR, you are suggesting moving it to the IRQ thread. I'm a bit concerned this could cause regressions though.

that was already done in the interrupt thread, but now it'd be moved to a workqueue and indeed this could impact the results

Since we are moving to a single thread, I wonder if the simple thing to do is to sequentially deal with IPC, then stream, then SoundWire (without using schedule_work as suggested so far). In the latter case, we actually use a work-queue already so there wouldn't be any change performance-wise. Also the peak times for interrupts don't overlap. For the firmware setup, we have a slew of IPC, but they happen before SoundWire kicks in. then we have stream interrupts while the flows are setup. It's not completely random, the control parts peak before the streaming.

The only change would come from the serialization of IPC and stream interrupts, but that doesn't look so bad to me.

ghost · 2019-11-20T00:15:40Z

that was already done in the interrupt thread, but now it'd be moved to a workqueue and indeed this could impact the results

@plbossart I am talking about this part:

linux/sound/soc/sof/intel/hda-stream.c

Line 617 in ecc776a

for (i = 0, active = true; i < 10 && active; i++) {

This is still done in the handler and not in the thread. But with @bardliao 's changes this would be moved to the workqueue or if he changes it to the interrupt thread, either way I feel like this could cause a regression

ghost · 2019-11-20T00:25:54Z

that was already done in the interrupt thread, but now it'd be moved to a workqueue and indeed this could impact the results

@plbossart I am talking about this part:

linux/sound/soc/sof/intel/hda-stream.c

Line 617 in ecc776a

for (i = 0, active = true; i < 10 && active; i++) {

This is still done in the handler and not in the thread. But with @bardliao 's changes this would be moved to the workqueue or if he changes it to the interrupt thread, either way I feel like this could cause a regression

nope, the handler is here:

linux/sound/soc/sof/intel/hda-stream.c

Line 552 in ecc776a

irqreturn_t hda_dsp_stream_interrupt(int irq, void *context)

ghost · 2019-11-20T00:51:43Z

that was already done in the interrupt thread, but now it'd be moved to a workqueue and indeed this could impact the results

@plbossart I am talking about this part:

linux/sound/soc/sof/intel/hda-stream.c

Line 617 in ecc776a

for (i = 0, active = true; i < 10 && active; i++) {

This is still done in the handler and not in the thread. But with @bardliao 's changes this would be moved to the workqueue or if he changes it to the interrupt thread, either way I feel like this could cause a regression

nope, the handler is here:

linux/sound/soc/sof/intel/hda-stream.c

Line 552 in ecc776a

irqreturn_t hda_dsp_stream_interrupt(int irq, void *context)

duh! yes you are correct @plbossart. the name threw me off a bit. In this case, if we dont use the workqueue, we should be fine I think

ghost · 2019-11-20T05:25:38Z

duh! yes you are correct @plbossart. the name threw me off a bit. In this case, if we dont use the workqueue, we should be fine I think

yes, I was also confused. handler and thread are official name for request_threaded_irq, no thread- handler

ghost · 2019-11-20T05:39:40Z

duh! yes you are correct @plbossart. the name threw me off a bit. In this case, if we dont use the workqueue, we should be fine I think

yes, I was also confused. handler and thread are official name for request_threaded_irq, no thread- handler

@RanderWang @ranj063 We can rename it in another patch :)

ghost · 2019-11-20T05:48:34Z

@bardliao there was specific change we made in the stream interrupt handler to handled unsolicited responses from the HDA codecs (ie looping 10 times) and checking INTSTS. This is was fixed the IPC timeouts for us. Now, in this PR, you are suggesting moving it to the IRQ thread. I'm a bit concerned this could cause regressions though.

that was already done in the interrupt thread, but now it'd be moved to a workqueue and indeed this could impact the results

Since we are moving to a single thread, I wonder if the simple thing to do is to sequentially deal with IPC, then stream, then SoundWire (without using schedule_work as suggested so far). In the latter case, we actually use a work-queue already so there wouldn't be any change performance-wise. Also the peak times for interrupts don't overlap. For the firmware setup, we have a slew of IPC, but they happen before SoundWire kicks in. then we have stream interrupts while the flows are setup. It's not completely random, the control parts peak before the streaming.

The only change would come from the serialization of IPC and stream interrupts, but that doesn't look so bad to me.

ok. Let's go with sequentially dealing with IPC, stream, and SoundWire first and see if everything works fine. :)

ghost · 2019-11-20T05:55:37Z

There is a potential risk to schedule work in thread function. For kernel, if thread function returns, interrupt frame work assumes all job has been done so another thread maybe invoked. But are the scheduled work done ? Do we need to sync all the scheduled works ? can anyone answer this question?

ghost

Starts to look good. I don't have any major concerns open. I'd like to hear your comments on the function naming, but otherwise I'm ready to press approve on this.

ghost · 2019-11-20T11:53:49Z

+	u32 irq_status;
+
+	/* The function can be called at irq thread, so use spin_lock_irq */
+	spin_lock_irq(&sdev->hw_lock);


@bardliao @ranj063 You definitely need to use the "_irq()" variant here. These is_xxx_irq() functions are called both from hard-irq and softirq context, so plain spin_lock() will not work.

ghost · 2019-11-20T12:09:45Z

+{
+	struct snd_sof_dev *sdev = context;
+
+	if (is_ipc_irq(sdev))


@bardliao is_ipc_irq() is called in both hardirq and softirq. This is otherwise ok, but it's a bit odd that is_ipc_irq() also has side-effects in that it clears HDA_DSP_ADSPIC_IPC. I'd prefer to call the function something else than "is_foo_irq()" as to me (maybe just ipc_irq()). To me, a function with is_something(), indicates the function does not have side-effects. The functions here do more.

I guess this still works as the ADSPIS status bit remains high despite ADSPIC set low. And this matches with the current logic we have.

@kv2019i I had the same feeling but it's hard to move the mask outside of the spinlock, and taking the spinlock twice, once to read ADSPIC2 and once to clear ADSPIS2 is not that great either.

maybe adding a comment would help explain the hidden side effect.
or rename from is_ipc to check_ipc, which is let of a status return

Thanks @kv2019i @plbossart I will rename it and add a comment.

@kv2019i @plbossart done. BTW, do you know how to disable stream irq? Or we can leave it as it is for now?

ghost · 2019-11-20T12:10:54Z

+		ret = false;

-	spin_unlock(&bus->reg_lock);
+	spin_unlock_irq(&bus->reg_lock);


Ack @bardliao and @RanderWang This is matching existing code, so I'm ok to change in another PR. I do have a comment on naming the is_ipc_irq() functions (see my other comment).

…andlers The existing code uses two handlers for a shared edge-based MSI interrupts. In corner cases, interrupts are lost, leading to IPC timeouts. Those timeouts do not appear in legacy mode. This patch merges the two handlers into a single one. Follow-up patches will extend this work to SoundWire interrupts which are also shared with IPC and stream interrupts. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ghost · 2019-11-21T02:39:33Z

+		hda_dsp_stream_threaded_handler(irq, sdev);
+
+	return IRQ_HANDLED;
+}


@bardliao this looks bad. You call check_ipc() in the handler, and then wake up the thread and then check_ipc() again and do the same thing over?

@ranj063 Yes, that's odd, but there are more than one interrupt could be trigger and one may be triggered after the handler is checked. So we need to recheck it on the thread. Actually, I think we can have a flag that store the interrupt status which is checked on the handler. So we won't need to double check it on the thread again. Please see hda_dsp_irq_thread

Why not just return IRQ_WAKE_THREAD in hda_dsp_interrupt_handler() and then handle all the checks in the thread?

Why not just return IRQ_WAKE_THREAD in hda_dsp_interrupt_handler() and then handle all the checks in the thread?

@ranj063 We can only do it when there is not a shared irq. i.e. No one else could trigger the irq. Probably, we can do that after ipc/stream/sdw irq are merged. And we don't need the handler if it return IRQ_WAKE_THREAD always.

ghost · 2019-11-21T09:38:36Z

Run both function and stress tests on below devices. No side effect found.

BYT MinnowBoard	BYT MinnowBoard	APL UP2	APL UP2	GLK	CML Chrome	CML_Mantis	ICL_RVP
nocodec	I2S RT5682	nocodec	I2S PCM512x	I2S-DA7219	I2S RT5682	HDA ALC3204	HDA ALC700

ghost

Minor note on the naming. Can be changed in a follow-up patch is other issues are cleared.

ghost · 2019-11-21T14:37:30Z

 			     struct snd_dma_buffer *dmab,
 			     struct hdac_stream *stream);
+bool check_ipc(struct snd_sof_dev *sdev);
+bool is_stream_irq(struct snd_sof_dev *sdev);


@bardliao I'd have uniform names to this (even if stream irq function is changed in a later patch). E.g. something like:
check_ipc_irq()
check_stream_irq()

agree, I'll add an update to fix the naming but we should merge this today since it'll give us one working day to fix things before the week-end.

ghost · 2019-11-21T16:12:00Z

SOFCI TEST

ghost · 2019-11-21T16:12:51Z

@fredoh9 can you help here, CI seems to be stuck?

ghost · 2019-11-21T16:14:16Z

-	struct hdac_bus *bus = context;
-	int ret = IRQ_WAKE_THREAD;
+	struct hdac_bus *bus = sof_to_bus(sdev);
+	bool ret = true;


@bardliao @plbossart I find this odd the default return value is true. The logic is correct but typically we always assume its false and return true when conditions are favorable.

the initial code assumed an IRQ_WAKE_THREAD as default, so it's acceptable to me.

ghost · 2019-11-21T16:20:48Z

@plbossart @bardliao this patch looks OK to merge given that its critical for testing but a follow up with minor changes will be needed.

ghost · 2019-11-21T18:34:37Z

ok, so no CI today, so will assume things work and move on.

bardliao requested review from RanderWang, lyakh, plbossart and ranj063 November 14, 2019 07:22

bardliao requested a review from lgirdwood as a code owner November 14, 2019 07:22

RanderWang reviewed Nov 14, 2019

View reviewed changes

plbossart requested changes Nov 14, 2019

View reviewed changes

bardliao force-pushed the merge-ipc-stream-irq branch from 325917a to 6c13c11 Compare November 15, 2019 02:18

bardliao requested review from juimonen and keyonjie as code owners November 15, 2019 02:18

plbossart requested changes Nov 18, 2019

View reviewed changes

bardliao force-pushed the merge-ipc-stream-irq branch from 6c13c11 to 905d11a Compare November 19, 2019 05:42

RanderWang reviewed Nov 19, 2019

View reviewed changes

mengdonglin mentioned this pull request Nov 19, 2019

ASoC: SOF: merge ipc and sdw irq #1451

Closed

kv2019i requested changes Nov 19, 2019

View reviewed changes

ranj063 reviewed Nov 19, 2019

View reviewed changes

bardliao force-pushed the merge-ipc-stream-irq branch from 905d11a to 74e85a9 Compare November 20, 2019 07:27

bardliao requested review from plbossart and ranj063 November 20, 2019 07:30

bardliao requested review from RanderWang and kv2019i November 20, 2019 07:30

kv2019i reviewed Nov 20, 2019

View reviewed changes

bardliao force-pushed the merge-ipc-stream-irq branch from 74e85a9 to 93d20ac Compare November 21, 2019 02:05

ranj063 reviewed Nov 21, 2019

View reviewed changes

bardliao mentioned this pull request Nov 21, 2019

Use single irq for IPC, stream, and sdw interrupts #1525

Closed

kv2019i reviewed Nov 21, 2019

View reviewed changes

ranj063 reviewed Nov 21, 2019

View reviewed changes

plbossart merged commit dbf4806 into thesofproject:topic/sof-dev Nov 21, 2019

This was referenced Nov 25, 2019

[BUG][SDW]Shows ipc timed out for 0x30010000 size 76 when run system boot up stress test. #1395

Closed

[SDW][BUG]ipc timed out for 0x40080000 size 32 occurred when run reboot stress test with legacy IRQ mode #1468

Closed

bardliao deleted the merge-ipc-stream-irq branch October 12, 2022 12:19

Conversation

bardliao commented Nov 14, 2019

Uh oh!

RanderWang Nov 14, 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!

bardliao Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

bardliao Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

plbossart Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

paulstelian97 Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

bardliao Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Nov 15, 2019

Uh oh!

ghost Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 20, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 20, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 20, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Nov 20, 2019 • edited by plbossart Loading Uh oh! There was an error while loading. Please reload this page.

RanderWang Nov 14, 2019 •

edited

Loading

ghost Nov 20, 2019 •

edited by plbossart

Loading

ghost commented Nov 20, 2019 •

edited by plbossart

Loading