[PATCH 4/4] sounds/hda/radeon: Disable 64-bit DMA on radeon

At Wed, 01 Oct 2014 12:09:53 +1000,

Post by Benjamin Herrenschmidt
The chipset has a limitation in the number of address bits it
can generate. The graphics portion uses a specific mask of
40 or 48 bits depending on the generation. For audio, it's a bit
less of an issue, so just mark them as no-64bit for now.
Without this, it crashes on POWER machines which can use high bits
in the DMA address to distinguish between DMA windows.
---
sound/pci/hda/hda_intel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 3e6d22d..2b679d5 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -297,7 +297,7 @@ enum {
/* quirks for ATI/AMD HDMI */
#define AZX_DCAPS_PRESET_ATI_HDMI \
(AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_POSFIX_LPIB|\
- AZX_DCAPS_NO_MSI64)
+ AZX_DCAPS_NO_MSI64 | AZX_DCAPS_NO_64BIT)

The only concern is that this will disable 64bit DMA also on x86 where
it has been working fine. Can we add an ifdef CONFIG_PPC for this?

Takashi

Benjamin Herrenschmidt

2014-10-01 07:41:29 UTC

Post by Benjamin Herrenschmidt
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 3e6d22d..2b679d5 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -297,7 +297,7 @@ enum {
/* quirks for ATI/AMD HDMI */
#define AZX_DCAPS_PRESET_ATI_HDMI \
(AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_POSFIX_LPIB|\
- AZX_DCAPS_NO_MSI64)
+ AZX_DCAPS_NO_MSI64 | AZX_DCAPS_NO_64BIT)

The only concern is that this will disable 64bit DMA also on x86 where
it has been working fine. Can we add an ifdef CONFIG_PPC for this?

I don't like that approach because technically the chip doesn't do
64-bit DMA ... it does something like 40 or 48 (might actually depend on
the chip version) and for all I know it will break on future x86 with
more memory or other platforms with similar address encodings as
powerpc...

The right thing might be to get the exact number of bits and do the
appropriate dma_set_mask() like the graphics driver does, but that's a
bit tricky unless we add a DMA mask field in that big array of chips in
there...

Cheers,
Ben.

Benjamin Herrenschmidt

2014-10-01 07:48:02 UTC

Post by Benjamin Herrenschmidt
I don't like that approach because technically the chip doesn't do
64-bit DMA ... it does something like 40 or 48 (might actually depend on
the chip version) and for all I know it will break on future x86 with
more memory or other platforms with similar address encodings as
powerpc...
The right thing might be to get the exact number of bits and do the
appropriate dma_set_mask() like the graphics driver does, but that's a
bit tricky unless we add a DMA mask field in that big array of chips in
there...

Note that I made the assumption that in the grand scheme of things, HDMI
audio isn't a very big throughput device and thus the extra iommu
mappings (or swiotlb) shouldn't hurt that much. But I may be wrong...

So by all means, let me know what you want done here, but I don't think
CONFIG_PPC is the right thing to do here, especially since this is not
even something that affects all PPCs, but specifically the POWER server
PCI bridges for which we configure a 64-bit window high up in the
address space.

FYI: Basically we setup PCI DMA addresses 0...2G as a 32-bit IOMMU
remapped window and we also configure a "direct map" window higher up
that bypasses the IOMMU and provides a direct linear mapping of RAM for
64-bit capable devices. The problem is that the HW on those server chips
requires that second window to have address bit 59 set. Then based on
the DMA mask, the arch code provide the device with DMA ops that will
either go through the iommu remap window or through the bypass window
based on the DMA mask set by the driver.

Cheers,
Ben.

Takashi Iwai

2014-10-01 08:09:28 UTC

At Wed, 01 Oct 2014 17:41:29 +1000,

The only concern is that this will disable 64bit DMA also on x86 where
it has been working fine. Can we add an ifdef CONFIG_PPC for this?

I don't like that approach because technically the chip doesn't do
64-bit DMA ... it does something like 40 or 48 (might actually depend on
the chip version) and for all I know it will break on future x86 with
more memory or other platforms with similar address encodings as
powerpc...
The right thing might be to get the exact number of bits and do the
appropriate dma_set_mask() like the graphics driver does, but that's a
bit tricky unless we add a DMA mask field in that big array of chips in
there...

I think setting the dma mask explicitly would be a better approach
although it results in a bit bigger change. At least, it would impact
less than forcing 32bit DMA, as most desktop machines have less than
40bit address. How about a patch like below? Only compile tested,
and feel free to add more tags or edit comments appropriately.
(I'm on vacation now, so have little time to type :)

thanks,

Takashi

---
From: Takashi Iwai <***@suse.de>
Subject: [PATCH] ALSA: hda - Limit 40bit DMA for AMD HDMI controllers

AMD/ATI HDMI controller chip models, we already have a filter to lower
to 32bit DMA, but the rest are supposed to be working with 64bit
although the hardware doesn't really work with 64bit but only with 40
or 48bit DMA. In this patch, we take 40bit DMA for safety for the
AMD/ATI controllers as the graphics drivers does.

Signed-off-by: Takashi Iwai <***@suse.de>
---
sound/pci/hda/hda_intel.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index aa302fb03fc5..6410a3edf8ff 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1482,6 +1482,7 @@ static int azx_first_init(struct azx *chip)
struct snd_card *card = chip->card;
int err;
unsigned short gcap;
+ unsigned int dma_bits = 64;

#if BITS_PER_LONG != 64
/* Fix up base address on ULI M5461 */
@@ -1521,6 +1522,8 @@ static int azx_first_init(struct azx *chip)
/* disable SB600 64bit support for safety */
if (chip->pci->vendor == PCI_VENDOR_ID_ATI) {
struct pci_dev *p_smbus;
+ /* AMD devices support 40 or 48bit DMA, take the safe one */
+ dma_bits = 40;
p_smbus = pci_get_device(PCI_VENDOR_ID_ATI,
PCI_DEVICE_ID_ATI_SBX00_SMBUS,
NULL);
@@ -1550,9 +1553,11 @@ static int azx_first_init(struct azx *chip)
}

/* allow 64bit DMA address if supported by H/W */
- if ((gcap & AZX_GCAP_64OK) && !pci_set_dma_mask(pci, DMA_BIT_MASK(64)))
- pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(64));
- else {
+ if (!(gcap & AZX_GCAP_64OK))
+ dma_bits = 32;
+ if (!pci_set_dma_mask(pci, DMA_BIT_MASK(dma_bits))) {
+ pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(dma_bits));
+ } else {
pci_set_dma_mask(pci, DMA_BIT_MASK(32));
pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(32));
}

--
2.1.0

Takashi Iwai

2014-10-01 08:30:53 UTC

At Wed, 01 Oct 2014 10:09:28 +0200,

Post by Takashi Iwai
At Wed, 01 Oct 2014 17:41:29 +1000,

The only concern is that this will disable 64bit DMA also on x86 where
it has been working fine. Can we add an ifdef CONFIG_PPC for this?

I don't like that approach because technically the chip doesn't do
64-bit DMA ... it does something like 40 or 48 (might actually depend on
the chip version) and for all I know it will break on future x86 with
more memory or other platforms with similar address encodings as
powerpc...
The right thing might be to get the exact number of bits and do the
appropriate dma_set_mask() like the graphics driver does, but that's a
bit tricky unless we add a DMA mask field in that big array of chips in
there...

Oops, it obviously doesn't work with AMD ID... Fixed in below.

Takashi

-- 8< --
From: Takashi Iwai <***@suse.de>
Subject: [PATCH v2] ALSA: hda - Limit 40bit DMA for AMD HDMI controllers

AMD/ATI HDMI controller chip models, we already have a filter to lower
to 32bit DMA, but the rest are supposed to be working with 64bit
although the hardware doesn't really work with 63bit but only with 40
or 48bit DMA. In this patch, we take 40bit DMA for safety for the
AMD/ATI controllers as the graphics drivers does.

Signed-off-by: Takashi Iwai <***@suse.de>
---
sound/pci/hda/hda_intel.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index aa302fb03fc5..99b367bd9b1b 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1482,6 +1482,7 @@ static int azx_first_init(struct azx *chip)
struct snd_card *card = chip->card;
int err;
unsigned short gcap;
+ unsigned int dma_bits = 64;

#if BITS_PER_LONG != 64
/* Fix up base address on ULI M5461 */
@@ -1518,9 +1519,14 @@ static int azx_first_init(struct azx *chip)
gcap = azx_readw(chip, GCAP);
dev_dbg(card->dev, "chipset global capabilities = 0x%x\n", gcap);

+ /* AMD devices support 40 or 48bit DMA, take the safe one */
+ if (chip->pci->vendor == PCI_VENDOR_ID_AMD)
+ dma_bits = 40;
+
/* disable SB600 64bit support for safety */
if (chip->pci->vendor == PCI_VENDOR_ID_ATI) {
struct pci_dev *p_smbus;
+ dma_bits = 40;
p_smbus = pci_get_device(PCI_VENDOR_ID_ATI,
PCI_DEVICE_ID_ATI_SBX00_SMBUS,
NULL);
@@ -1550,9 +1556,11 @@ static int azx_first_init(struct azx *chip)
}

/* allow 64bit DMA address if supported by H/W */
- if ((gcap & AZX_GCAP_64OK) && !pci_set_dma_mask(pci, DMA_BIT_MASK(64)))
- pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(64));
- else {
+ if (!(gcap & AZX_GCAP_64OK))
+ dma_bits = 32;
+ if (!pci_set_dma_mask(pci, DMA_BIT_MASK(dma_bits))) {
+ pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(dma_bits));
+ } else {
pci_set_dma_mask(pci, DMA_BIT_MASK(32));
pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(32));
}

--
2.1.0

Alex Deucher

2014-10-01 17:58:13 UTC

Post by Takashi Iwai
At Wed, 01 Oct 2014 10:09:28 +0200,

Post by Takashi Iwai
At Wed, 01 Oct 2014 17:41:29 +1000,

The only concern is that this will disable 64bit DMA also on x86 where
it has been working fine. Can we add an ifdef CONFIG_PPC for this?

I don't like that approach because technically the chip doesn't do
64-bit DMA ... it does something like 40 or 48 (might actually depend on
the chip version) and for all I know it will break on future x86 with
more memory or other platforms with similar address encodings as
powerpc...
The right thing might be to get the exact number of bits and do the
appropriate dma_set_mask() like the graphics driver does, but that's a
bit tricky unless we add a DMA mask field in that big array of chips in
there...

Patch looks good. Audio DMAs are limited to 40 bits, same as the GPU
side. I'm still waiting to hear back on the MSIs for audio, but they
probably follow the GPU side, so I expect they should be fixed on Sea
Islands as well.

Alex

Post by Takashi Iwai
Oops, it obviously doesn't work with AMD ID... Fixed in below.
Takashi
-- 8< --
Subject: [PATCH v2] ALSA: hda - Limit 40bit DMA for AMD HDMI controllers
AMD/ATI HDMI controller chip models, we already have a filter to lower
to 32bit DMA, but the rest are supposed to be working with 64bit
although the hardware doesn't really work with 63bit but only with 40
or 48bit DMA. In this patch, we take 40bit DMA for safety for the
AMD/ATI controllers as the graphics drivers does.
---
sound/pci/hda/hda_intel.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index aa302fb03fc5..99b367bd9b1b 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1482,6 +1482,7 @@ static int azx_first_init(struct azx *chip)
struct snd_card *card = chip->card;
int err;
unsigned short gcap;
+ unsigned int dma_bits = 64;
#if BITS_PER_LONG != 64
/* Fix up base address on ULI M5461 */
@@ -1518,9 +1519,14 @@ static int azx_first_init(struct azx *chip)
gcap = azx_readw(chip, GCAP);
dev_dbg(card->dev, "chipset global capabilities = 0x%x\n", gcap);
+ /* AMD devices support 40 or 48bit DMA, take the safe one */
+ if (chip->pci->vendor == PCI_VENDOR_ID_AMD)
+ dma_bits = 40;
+
/* disable SB600 64bit support for safety */
if (chip->pci->vendor == PCI_VENDOR_ID_ATI) {
struct pci_dev *p_smbus;
+ dma_bits = 40;
p_smbus = pci_get_device(PCI_VENDOR_ID_ATI,
PCI_DEVICE_ID_ATI_SBX00_SMBUS,
NULL);
@@ -1550,9 +1556,11 @@ static int azx_first_init(struct azx *chip)
}
/* allow 64bit DMA address if supported by H/W */
- if ((gcap & AZX_GCAP_64OK) && !pci_set_dma_mask(pci, DMA_BIT_MASK(64)))
- pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(64));
- else {
+ if (!(gcap & AZX_GCAP_64OK))
+ dma_bits = 32;
+ if (!pci_set_dma_mask(pci, DMA_BIT_MASK(dma_bits))) {
+ pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(dma_bits));
+ } else {
pci_set_dma_mask(pci, DMA_BIT_MASK(32));
pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(32));
}
--
2.1.0

Benjamin Herrenschmidt

2014-10-01 22:08:02 UTC

Post by Alex Deucher
Patch looks good. Audio DMAs are limited to 40 bits, same as the GPU
side. I'm still waiting to hear back on the MSIs for audio, but they
probably follow the GPU side, so I expect they should be fixed on Sea
Islands as well.

In the audio driver we don't have the "family names", just a list of
PCI IDs, do you happen to know which ones are unaffected ?

Cheers,
Ben.

Benjamin Herrenschmidt

2014-10-02 00:15:52 UTC

In the audio driver we don't have the "family names", just a list of
PCI IDs, do you happen to know which ones are unaffected ?

So I tried to be more discriminate but in the end, I had to give up, the
most recent ID we have in the driver seems to be Cap Verde/Pitcairn,
which isn't fixed. So either we are missing IDs or the new cards use the
same ID.

I think the consequences of being over-zealous here are nil on what
matters, ie, x86. AFAIK, x86 always uses 32-bit addresses for MSIs
anyway, so it's unaffected and ARM ... well, I doubt it will be. In
general, I don't think we have a big issue here by just flagging them
all.

I'll send an updated series with just the flag name changed as suggested
by Alex and the DMA bits updated as suggested by Takashi.

Cheers,
Ben.

Alex Deucher

2014-10-02 01:19:24 UTC

On Wed, Oct 1, 2014 at 6:08 PM, Benjamin Herrenschmidt