ASoC: SOF: merge ipc and sdw irq by bardliao · Pull Request #1412 · thesofproject/linux

bardliao · 2019-10-31T09:18:47Z

So we can use single irq to handle both ipc and sdw interrupts.

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

bardliao · 2019-10-31T09:19:54Z

I did a basic function test and found no issue.

plbossart

Looks pretty good @bardliao
I don't fully get some of the changes, looks like the handler is now a thread, not sure if this is a good change?

plbossart · 2019-10-31T14:20:53Z

 						 HDA_DSP_REG_ADSPIC,
 						 HDA_DSP_ADSPIC_IPC, 0);
-		ret = IRQ_WAKE_THREAD;
+		spin_unlock(&sdev->hw_lock);


careful with the unlock. we could have an IPC and SoundWire interrupt at the same time (different registers used for status), so the two if cases are not mutually exclusive in corner cases

Thanks for the reminding, now I separate the handler and thread. So no problem here now.

plbossart · 2019-10-31T14:22:50Z

-	ret = request_threaded_irq(sdev->ipc_irq, hda_dsp_ipc_irq_handler,
-				   sof_ops(sdev)->irq_thread, IRQF_SHARED,
+	ret = request_threaded_irq(sdev->ipc_irq, NULL,
+				   hda_dsp_ipc_irq_handler, IRQF_SHARED,


sorry I don't get this one. this would mean we use a generic handler and the current handler becomes a thread?
Is this needed?

Yeah, I thought there are no other interrupt share the irq and want to do a quick test yesterday. Now I change it back as it is.

plbossart · 2019-10-31T14:24:09Z

-	return IRQ_NONE;
+	return false;
 }
+EXPORT_SYMBOL(is_sdw_intel_irq);


maybe a static inline instead?

plbossart · 2019-10-31T14:27:09Z

 						 HDA_DSP_ADSPIC_IPC, 0);
-		ret = IRQ_WAKE_THREAD;
+		spin_unlock(&sdev->hw_lock);
+		sof_ops(sdev)->irq_thread(irq, context);


I don't get the change here?

The sof_ops(sdev)->irq_thread was the irq thread in request_thread_irq(). So we have to call it in the irq thread.

RanderWang · 2019-11-01T01:58:45Z

@bardliao please check the interrupt design in Linux. handler is in interrupt context and the PCI
interrupt is disabled, and thread is in non-interrupt context with the PCI interrupt enabled. Another irq function for HDA stream is still done with handler & thread. It takes risk that ipc &sdw are done in thread function with the PCI interrupt enabled.

bardliao · 2019-11-01T08:21:15Z

@bardliao please check the interrupt design in Linux. handler is in interrupt context and the PCI
interrupt is disabled, and thread is in non-interrupt context with the PCI interrupt enabled. Another irq function for HDA stream is still done with handler & thread. It takes risk that ipc &sdw are done in thread function with the PCI interrupt enabled.

Thanks @RanderWang I separate the irq handler and thread in the updated version. :)

ranj063 · 2019-11-04T16:48:07Z

+		sof_ops(sdev)->irq_thread(irq, context);
+
+	if (sdev->irq_event & SOF_IRQ_SDW)
+		sdw_intel_thread(irq, hdev->sdw);


@bardliao I understand the intention here but the naming is incorrent. hda_dsp_irq_thread() is the IRQ thread, sof_ops(sdev)->irq_thread() and sdw_intel_thread() are not threads but just normal methods. So I think we should remove the "thread" from their names.

@bardliao I understand the intention here but the naming is incorrent. hda_dsp_irq_thread() is the IRQ thread, sof_ops(sdev)->irq_thread() and sdw_intel_thread() are not threads but just normal methods. So I think we should remove the "thread" from their names.

@ranj063 I agree with you. I have a concern regarding sof_ops(sdev)->irq_thread(). Is there anywhere use sof_ops(sdev)->irq_thread() except here? And it looks confusing, too if we remove the _thread suffix. i.e. sof_ops(sdev)->irq. irq_handler may be a better name, but we already have it. Do you have any suggestion for irq_thread?

ranj063 · 2019-11-04T16:50:14Z

@bardliao the commit message is very sparse. Why do we need to merge the 2 interrupts? What advantage does it bring?

plbossart · 2019-11-04T17:28:49Z

@bardliao the commit message is very sparse. Why do we need to merge the 2 interrupts? What advantage does it bring?

we still see IPC issues with SoundWire, that go way if we disable MSI. We did improve the results by merging 4 SoundWire handlers into one, this is an attempt to go further and have a single handler for all interrupts.

ranj063 · 2019-11-04T17:35:20Z

we still see IPC issues with SoundWire, that go way if we disable MSI. We did improve the results by merging 4 SoundWire handlers into one, this is an attempt to go further and have a single handler for all interrupts.

@plbossart what confuses me is that even if you had separate IRQ handlers for the SDW and IPC interrupts, they will both get invoked when an interrupt is raised. Why does it help to service them in one handler? Is there a race because of the order the 2 interrupt handlers get invoked?

plbossart · 2019-11-04T18:14:24Z

we still see IPC issues with SoundWire, that go way if we disable MSI. We did improve the results by merging 4 SoundWire handlers into one, this is an attempt to go further and have a single handler for all interrupts.

@plbossart what confuses me is that even if you had separate IRQ handlers for the SDW and IPC interrupts, they will both get invoked when an interrupt is raised. Why does it help to service them in one handler? Is there a race because of the order the 2 interrupt handlers get invoked?

We have no idea how edge interrupts like MSI are handled if you have multiple handlers. It just works better if we avoid this in the first place.

plbossart

@bardliao can you rebase on top on integration/soundwire-debug-fixes?

See also comments below, I don't think the use of the sdev->irq_event can work.

plbossart · 2019-11-07T20:17:16Z

+{
+	u32 int_status;
+
+	int_status = readl(ctx->mmio_base + 0x14);


HDA_DSP_REG_ADSPIS2

@plbossart HDA_DSP_REG_ADSPIS2 is defined at sound/soc/sof/intel/hda.h, but we don't include hda.h in soundwire driver. I am not sure include hda.h in sdw_intel.h is a good idea or not. If we just add the HDA_DSP_REG_ADSPIS2 define in sdw_intel.h, we will see redefine warning in compiling. Or we can define another name for HDA_DSP_REG_ADSPIS2 in sdw_intel.h?

just redefine them SDW_ADSPIC2 and SDW_ADSIS2. that's fine.

plbossart · 2019-11-07T20:17:31Z

+	u32 int_status;
+
+	int_status = readl(ctx->mmio_base + 0x14);
+	if (int_status & BIT(5))


HDA_DSP_REG_ADSPIC2_SNDW

plbossart · 2019-11-07T20:20:46Z

 		snd_sof_dsp_update_bits_unlocked(sdev, HDA_DSP_BAR,
 						 HDA_DSP_REG_ADSPIC,
 						 HDA_DSP_ADSPIC_IPC, 0);
+		sdev->irq_event |= SOF_IRQ_IPC;


the use of a field that's set in the handler is problematic. We know we can have new interrupts before the thread is scheduled (that's e.g. how SoundWire handled the commands before, with the wait_for_completion in the thread and the complete() bin the handler

So it's possible that while you have an SoundWire thread scheduled an IPC interrupt quicks in, and you would reset sdev->irq_event.

I don't think this can work...

So we can use single irq to handle both ipc and sdw interrupts. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao · 2019-11-08T05:33:38Z

replaced by #1451 which is on top of integration/soundwire-debug-fixes.

bardliao requested review from RanderWang, plbossart and ranj063 October 31, 2019 09:18

bardliao requested a review from lgirdwood as a code owner October 31, 2019 09:18

plbossart requested changes Oct 31, 2019

View reviewed changes

bardliao force-pushed the topic/sdw-one-irq branch from 18c255a to 259ceb3 Compare November 1, 2019 07:49

bardliao requested review from juimonen, keyonjie and lyakh as code owners November 1, 2019 07:49

bardliao force-pushed the topic/sdw-one-irq branch from 259ceb3 to 43ec27c Compare November 1, 2019 08:10

plbossart force-pushed the integration/soundwire-intel branch from f417f1c to fcf05e1 Compare November 4, 2019 16:40

ranj063 reviewed Nov 4, 2019

View reviewed changes

bardliao force-pushed the topic/sdw-one-irq branch from 43ec27c to 2c007d8 Compare November 5, 2019 05:43

YvonneYang2 mentioned this pull request Nov 6, 2019

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

Closed

plbossart force-pushed the integration/soundwire-intel branch 3 times, most recently from eff3c3c to 8085159 Compare November 6, 2019 19:08

plbossart requested changes Nov 7, 2019

View reviewed changes

ASoC: SOF: merge ipc and sdw irq

13f05a2

So we can use single irq to handle both ipc and sdw interrupts. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the topic/sdw-one-irq branch from 2c007d8 to 13f05a2 Compare November 8, 2019 02:11

bardliao closed this Nov 8, 2019

plbossart mentioned this pull request Nov 11, 2019

ASoC: SOF: merge ipc and sdw irq #1451

Closed

Conversation

bardliao commented Oct 31, 2019

Uh oh!

bardliao commented Oct 31, 2019

Uh oh!

plbossart left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Nov 1, 2019

Uh oh!

bardliao commented Nov 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Nov 4, 2019

Uh oh!

plbossart commented Nov 4, 2019

Uh oh!

ranj063 commented Nov 4, 2019

Uh oh!

plbossart commented Nov 4, 2019

Uh oh!

plbossart left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliao commented Nov 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants