diff mbox series

ASoC: amd:Replacing MSI with Legacy IRQ model

Message ID 20201222115929.11222-1-Vishnuvardhanrao.Ravulapati@amd.com
State Accepted
Commit a523e1538fdd5f00ea3289cc0b3c6c1785b89814
Headers show
Series ASoC: amd:Replacing MSI with Legacy IRQ model | expand

Commit Message

RAVULAPATI, VISHNU VARDHAN RAO Dec. 22, 2020, 11:59 a.m. UTC 
When we try to play and capture simultaneously we see that
interrupts are genrated but our handler is not being acknowledged,
After investigating further more in detail on this issue we found
that IRQ delivery via MSI from the ACP IP is unreliable and so sometimes
interrupt generated will not be acknowledged so MSI model shouldn't be used
and using legacy IRQs will resolve interrupt handling issue.

This patch replaces MSI interrupt handling with legacy IRQ model.

Issue can be reproduced easily by running below python script:

import subprocess
import time
import threading

def do2():
  cmd = 'aplay -f dat -D hw:2,1 /dev/zero -d 1'
    subprocess.call(cmd, stdin=subprocess.PIPE,
			stderr=subprocess.PIPE, shell=True)
    print('Play Done')

def run():
	for i in range(1000):
		do2()

def do(i):
    cmd = 'arecord -f dat -D hw:2,2 /dev/null -d 1'
    subprocess.call(cmd, stdout=subprocess.PIPE,
			stderr=subprocess.PIPE, shell=True)
    print(datetime.datetime.now(), i)

t = threading.Thread(target=run)
t.start()
for i in range(1000):
	do(i)

t.join()

After applying this patch issue is resolved.

Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com>
---
 sound/soc/amd/raven/pci-acp3x.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

Comments

Takashi Iwai Dec. 22, 2020, 2:15 p.m. UTC | #1 
On Tue, 22 Dec 2020 12:59:18 +0100,
Ravulapati Vishnu vardhan rao wrote:
> 
> When we try to play and capture simultaneously we see that
> interrupts are genrated but our handler is not being acknowledged,
> After investigating further more in detail on this issue we found
> that IRQ delivery via MSI from the ACP IP is unreliable and so sometimes
> interrupt generated will not be acknowledged so MSI model shouldn't be used
> and using legacy IRQs will resolve interrupt handling issue.
> 
> This patch replaces MSI interrupt handling with legacy IRQ model.
> 
> Issue can be reproduced easily by running below python script:
> 
> import subprocess
> import time
> import threading
> 
> def do2():
>   cmd = 'aplay -f dat -D hw:2,1 /dev/zero -d 1'
>     subprocess.call(cmd, stdin=subprocess.PIPE,
> 			stderr=subprocess.PIPE, shell=True)
>     print('Play Done')
> 
> def run():
> 	for i in range(1000):
> 		do2()
> 
> def do(i):
>     cmd = 'arecord -f dat -D hw:2,2 /dev/null -d 1'
>     subprocess.call(cmd, stdout=subprocess.PIPE,
> 			stderr=subprocess.PIPE, shell=True)
>     print(datetime.datetime.now(), i)
> 
> t = threading.Thread(target=run)
> t.start()
> for i in range(1000):
> 	do(i)
> 
> t.join()
> 
> After applying this patch issue is resolved.
> 
> Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com>

Is this specific to Raven, i.e. Renoir doesn't need the same fix?
If so, it should be mentioned in the patch description.


thanks,

Takashi
RAVULAPATI, VISHNU VARDHAN RAO Dec. 25, 2020, noon UTC | #2 
[AMD Official Use Only - Internal Distribution Only]



On Tue, 22 Dec 2020 12:59:18 +0100,
Ravulapati Vishnu vardhan rao wrote:
>
> When we try to play and capture simultaneously we see that
> interrupts are genrated but our handler is not being acknowledged,
> After investigating further more in detail on this issue we found
> that IRQ delivery via MSI from the ACP IP is unreliable and so sometimes
> interrupt generated will not be acknowledged so MSI model shouldn't be used
> and using legacy IRQs will resolve interrupt handling issue.
>
> This patch replaces MSI interrupt handling with legacy IRQ model.
>
> Issue can be reproduced easily by running below python script:
>
> import subprocess
> import time
> import threading
>
> def do2():
>   cmd = 'aplay -f dat -D hw:2,1 /dev/zero -d 1'
>     subprocess.call(cmd, stdin=subprocess.PIPE,
>                        stderr=subprocess.PIPE, shell=True)
>     print('Play Done')
>
> def run():
>        for i in range(1000):
>                do2()
>
> def do(i):
>     cmd = 'arecord -f dat -D hw:2,2 /dev/null -d 1'
>     subprocess.call(cmd, stdout=subprocess.PIPE,
>                        stderr=subprocess.PIPE, shell=True)
>     print(datetime.datetime.now(), i)
>
> t = threading.Thread(target=run)
> t.start()
> for i in range(1000):
>        do(i)
>
> t.join()
>
> After applying this patch issue is resolved.
>
> Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com>

>Is this specific to Raven, i.e. Renoir doesn't need >the same fix?
>If so, it should be mentioned in the patch >description.

We will send this fix as separate patch for Renoir.

Thanks,
Vishnu
Jaroslav Kysela Dec. 27, 2020, 6:03 p.m. UTC | #3 
Dne 22. 12. 20 v 12:59 Ravulapati Vishnu vardhan rao napsal(a):
> When we try to play and capture simultaneously we see that
> interrupts are genrated but our handler is not being acknowledged,
> After investigating further more in detail on this issue we found
> that IRQ delivery via MSI from the ACP IP is unreliable and so sometimes
> interrupt generated will not be acknowledged so MSI model shouldn't be used
> and using legacy IRQs will resolve interrupt handling issue.

What is the real culprit? It's hw bug? If not, it would be better to fix the
PCI code or the irq handler.

					Jaroslav
Takashi Iwai Dec. 27, 2020, 6:46 p.m. UTC | #4 
On Sun, 27 Dec 2020 19:03:28 +0100,
Jaroslav Kysela wrote:
> 
> Dne 22. 12. 20 v 12:59 Ravulapati Vishnu vardhan rao napsal(a):
> > When we try to play and capture simultaneously we see that
> > interrupts are genrated but our handler is not being acknowledged,
> > After investigating further more in detail on this issue we found
> > that IRQ delivery via MSI from the ACP IP is unreliable and so sometimes
> > interrupt generated will not be acknowledged so MSI model shouldn't be used
> > and using legacy IRQs will resolve interrupt handling issue.
> 
> What is the real culprit? It's hw bug? If not, it would be better to fix the
> PCI code or the irq handler.

I'm not surprised if it's a hardware "feature".  There are lots of
deny list and quirks about disabling MSI in HD-audio controller code,
after all.


Takashi
Mark Brown Dec. 28, 2020, 4:12 p.m. UTC | #5 
On Tue, 22 Dec 2020 17:29:18 +0530, Ravulapati Vishnu vardhan rao wrote:
> When we try to play and capture simultaneously we see that
> interrupts are genrated but our handler is not being acknowledged,
> After investigating further more in detail on this issue we found
> that IRQ delivery via MSI from the ACP IP is unreliable and so sometimes
> interrupt generated will not be acknowledged so MSI model shouldn't be used
> and using legacy IRQs will resolve interrupt handling issue.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: amd:Replacing MSI with Legacy IRQ model
      commit: a523e1538fdd5f00ea3289cc0b3c6c1785b89814

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark
diff mbox series

Patch

diff --git a/sound/soc/amd/raven/pci-acp3x.c b/sound/soc/amd/raven/pci-acp3x.c
index 8c138e490f0c..d3536fd6a124 100644
--- a/sound/soc/amd/raven/pci-acp3x.c
+++ b/sound/soc/amd/raven/pci-acp3x.c
@@ -140,21 +140,14 @@  static int snd_acp3x_probe(struct pci_dev *pci,
 		goto release_regions;
 	}
 
-	/* check for msi interrupt support */
-	ret = pci_enable_msi(pci);
-	if (ret)
-		/* msi is not enabled */
-		irqflags = IRQF_SHARED;
-	else
-		/* msi is enabled */
-		irqflags = 0;
+	irqflags = IRQF_SHARED;
 
 	addr = pci_resource_start(pci, 0);
 	adata->acp3x_base = devm_ioremap(&pci->dev, addr,
 					pci_resource_len(pci, 0));
 	if (!adata->acp3x_base) {
 		ret = -ENOMEM;
-		goto disable_msi;
+		goto release_regions;
 	}
 	pci_set_master(pci);
 	pci_set_drvdata(pci, adata);
@@ -162,7 +155,7 @@  static int snd_acp3x_probe(struct pci_dev *pci,
 	adata->pme_en = rv_readl(adata->acp3x_base + mmACP_PME_EN);
 	ret = acp3x_init(adata);
 	if (ret)
-		goto disable_msi;
+		goto release_regions;
 
 	val = rv_readl(adata->acp3x_base + mmACP_I2S_PIN_CONFIG);
 	switch (val) {
@@ -251,8 +244,6 @@  static int snd_acp3x_probe(struct pci_dev *pci,
 de_init:
 	if (acp3x_deinit(adata->acp3x_base))
 		dev_err(&pci->dev, "ACP de-init failed\n");
-disable_msi:
-	pci_disable_msi(pci);
 release_regions:
 	pci_release_regions(pci);
 disable_pci:
@@ -311,7 +302,6 @@  static void snd_acp3x_remove(struct pci_dev *pci)
 		dev_err(&pci->dev, "ACP de-init failed\n");
 	pm_runtime_forbid(&pci->dev);
 	pm_runtime_get_noresume(&pci->dev);
-	pci_disable_msi(pci);
 	pci_release_regions(pci);
 	pci_disable_device(pci);
 }