We shouldn't assume CPU physical address we get from page_to_phys()
is same as DMA address we get from dma_alloc_coherent(). On x86_64,
we won't run into any problem with the assumption when dma_ops is
nommu_dma_ops. However, DMA address is IOVA when IOMMU is enabled.
And it's most likely different from CPU physical address when AMD
IOMMU is not in passthrough mode.
Signed-off-by: Yu Zhao <[email protected]>
---
sound/soc/amd/acp-pcm-dma.c | 15 +++++----------
sound/soc/amd/acp.h | 2 +-
2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index cdebab2f8ce5..fd3db4c37882 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -303,11 +303,10 @@ static void set_acp_to_i2s_dma_descriptors(void __iomem *acp_mmio, u32 size,
}
/* Create page table entries in ACP SRAM for the allocated memory */
-static void acp_pte_config(void __iomem *acp_mmio, struct page *pg,
+static void acp_pte_config(void __iomem *acp_mmio, dma_addr_t addr,
u16 num_of_pages, u32 pte_offset)
{
u16 page_idx;
- u64 addr;
u32 low;
u32 high;
u32 offset;
@@ -317,7 +316,6 @@ static void acp_pte_config(void __iomem *acp_mmio, struct page *pg,
/* Load the low address of page int ACP SRAM through SRBM */
acp_reg_write((offset + (page_idx * 8)),
acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
- addr = page_to_phys(pg);
low = lower_32_bits(addr);
high = upper_32_bits(addr);
@@ -333,7 +331,7 @@ static void acp_pte_config(void __iomem *acp_mmio, struct page *pg,
acp_reg_write(high, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
/* Move to next physically contiguos page */
- pg++;
+ addr += PAGE_SIZE;
}
}
@@ -343,7 +341,7 @@ static void config_acp_dma(void __iomem *acp_mmio,
{
u16 ch_acp_sysmem, ch_acp_i2s;
- acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
+ acp_pte_config(acp_mmio, rtd->dma_addr, rtd->num_of_pages,
rtd->pte_offset);
if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {
@@ -850,7 +848,6 @@ static int acp_dma_hw_params(struct snd_pcm_substream *substream,
int status;
uint64_t size;
u32 val = 0;
- struct page *pg;
struct snd_pcm_runtime *runtime;
struct audio_substream_data *rtd;
struct snd_soc_pcm_runtime *prtd = substream->private_data;
@@ -986,16 +983,14 @@ static int acp_dma_hw_params(struct snd_pcm_substream *substream,
return status;
memset(substream->runtime->dma_area, 0, params_buffer_bytes(params));
- pg = virt_to_page(substream->dma_buffer.area);
- if (pg) {
+ if (substream->dma_buffer.area) {
acp_set_sram_bank_state(rtd->acp_mmio, 0, true);
/* Save for runtime private data */
- rtd->pg = pg;
+ rtd->dma_addr = substream->dma_buffer.addr;
rtd->order = get_order(size);
/* Fill the page table entries in ACP SRAM */
- rtd->pg = pg;
rtd->size = size;
rtd->num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
rtd->direction = substream->stream;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index dbbb1a85638d..e5ab6c6040a6 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -123,7 +123,7 @@ enum acp_dma_priority_level {
};
struct audio_substream_data {
- struct page *pg;
+ dma_addr_t dma_addr;
unsigned int order;
u16 num_of_pages;
u16 i2s_instance;
--
2.20.0.rc1.387.gf8505762e3-goog
AMD platform device acp_audio_dma can only be created by parent PCI
device driver (drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c). Pass struct
device of the parent to snd_pcm_lib_preallocate_pages() so
dma_alloc_coherent() can use correct dma_ops. Otherwise, it will
use default dma_ops which is nommu_dma_ops on x86_64 even when
IOMMU is enabled and set to non passthrough mode.
Though platform device inherits some dma related fields during its
creation in mfd_add_device(), we can't simply pass its struct device
to snd_pcm_lib_preallocate_pages() because dma_ops is not among the
inherited fields. Even it were, drivers/iommu/amd_iommu.c would
ignore it because get_device_id() doesn't handle platform device.
This change shouldn't give us any trouble even struct device of the
parent becomes null or represents some non PCI device in the future,
because get_dma_ops() correctly handles null struct device or uses
the default dma_ops if struct device doesn't have it set.
Signed-off-by: Yu Zhao <[email protected]>
---
sound/soc/amd/acp-pcm-dma.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index fd3db4c37882..f4011bebc7ec 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -1146,18 +1146,21 @@ static int acp_dma_new(struct snd_soc_pcm_runtime *rtd)
struct snd_soc_component *component = snd_soc_rtdcom_lookup(rtd,
DRV_NAME);
struct audio_drv_data *adata = dev_get_drvdata(component->dev);
+ struct device *parent = component->dev->parent;
switch (adata->asic_type) {
case CHIP_STONEY:
ret = snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
SNDRV_DMA_TYPE_DEV,
- NULL, ST_MIN_BUFFER,
+ parent,
+ ST_MIN_BUFFER,
ST_MAX_BUFFER);
break;
default:
ret = snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
SNDRV_DMA_TYPE_DEV,
- NULL, MIN_BUFFER,
+ parent,
+ MIN_BUFFER,
MAX_BUFFER);
break;
}
--
2.20.0.rc1.387.gf8505762e3-goog
On 05/12/18 4:12 AM, Yu Zhao wrote:
> We shouldn't assume CPU physical address we get from page_to_phys()
> is same as DMA address we get from dma_alloc_coherent(). On x86_64,
> we won't run into any problem with the assumption when dma_ops is
> nommu_dma_ops. However, DMA address is IOVA when IOMMU is enabled.
> And it's most likely different from CPU physical address when AMD
> IOMMU is not in passthrough mode.
This is really a good fix. We will apply this patch changes for Raven
platform also.
Thanks,
Vijendar
>
> Signed-off-by: Yu Zhao <[email protected]>
> ---
> sound/soc/amd/acp-pcm-dma.c | 15 +++++----------
> sound/soc/amd/acp.h | 2 +-
> 2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
> index cdebab2f8ce5..fd3db4c37882 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -303,11 +303,10 @@ static void set_acp_to_i2s_dma_descriptors(void __iomem *acp_mmio, u32 size,
> }
>
> /* Create page table entries in ACP SRAM for the allocated memory */
> -static void acp_pte_config(void __iomem *acp_mmio, struct page *pg,
> +static void acp_pte_config(void __iomem *acp_mmio, dma_addr_t addr,
> u16 num_of_pages, u32 pte_offset)
> {
> u16 page_idx;
> - u64 addr;
> u32 low;
> u32 high;
> u32 offset;
> @@ -317,7 +316,6 @@ static void acp_pte_config(void __iomem *acp_mmio, struct page *pg,
> /* Load the low address of page int ACP SRAM through SRBM */
> acp_reg_write((offset + (page_idx * 8)),
> acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
> - addr = page_to_phys(pg);
>
> low = lower_32_bits(addr);
> high = upper_32_bits(addr);
> @@ -333,7 +331,7 @@ static void acp_pte_config(void __iomem *acp_mmio, struct page *pg,
> acp_reg_write(high, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
>
> /* Move to next physically contiguos page */
> - pg++;
> + addr += PAGE_SIZE;
> }
> }
>
> @@ -343,7 +341,7 @@ static void config_acp_dma(void __iomem *acp_mmio,
> {
> u16 ch_acp_sysmem, ch_acp_i2s;
>
> - acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
> + acp_pte_config(acp_mmio, rtd->dma_addr, rtd->num_of_pages,
> rtd->pte_offset);
>
> if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {
> @@ -850,7 +848,6 @@ static int acp_dma_hw_params(struct snd_pcm_substream *substream,
> int status;
> uint64_t size;
> u32 val = 0;
> - struct page *pg;
> struct snd_pcm_runtime *runtime;
> struct audio_substream_data *rtd;
> struct snd_soc_pcm_runtime *prtd = substream->private_data;
> @@ -986,16 +983,14 @@ static int acp_dma_hw_params(struct snd_pcm_substream *substream,
> return status;
>
> memset(substream->runtime->dma_area, 0, params_buffer_bytes(params));
> - pg = virt_to_page(substream->dma_buffer.area);
>
> - if (pg) {
> + if (substream->dma_buffer.area) {
> acp_set_sram_bank_state(rtd->acp_mmio, 0, true);
> /* Save for runtime private data */
> - rtd->pg = pg;
> + rtd->dma_addr = substream->dma_buffer.addr;
> rtd->order = get_order(size);
>
> /* Fill the page table entries in ACP SRAM */
> - rtd->pg = pg;
> rtd->size = size;
> rtd->num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> rtd->direction = substream->stream;
> diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
> index dbbb1a85638d..e5ab6c6040a6 100644
> --- a/sound/soc/amd/acp.h
> +++ b/sound/soc/amd/acp.h
> @@ -123,7 +123,7 @@ enum acp_dma_priority_level {
> };
>
> struct audio_substream_data {
> - struct page *pg;
> + dma_addr_t dma_addr;
> unsigned int order;
> u16 num_of_pages;
> u16 i2s_instance;
>
On 05/12/18 4:12 AM, Yu Zhao wrote:
> AMD platform device acp_audio_dma can only be created by parent PCI
> device driver (drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c). Pass struct
> device of the parent to snd_pcm_lib_preallocate_pages() so
> dma_alloc_coherent() can use correct dma_ops. Otherwise, it will
> use default dma_ops which is nommu_dma_ops on x86_64 even when
> IOMMU is enabled and set to non passthrough mode.
>
> Though platform device inherits some dma related fields during its
> creation in mfd_add_device(), we can't simply pass its struct device
> to snd_pcm_lib_preallocate_pages() because dma_ops is not among the
> inherited fields. Even it were, drivers/iommu/amd_iommu.c would
> ignore it because get_device_id() doesn't handle platform device.
>
> This change shouldn't give us any trouble even struct device of the
> parent becomes null or represents some non PCI device in the future,
> because get_dma_ops() correctly handles null struct device or uses
> the default dma_ops if struct device doesn't have it set.
>
This is really a good fix. We will also apply this fix for Raven platform.
Thanks,
Vijendar
> Signed-off-by: Yu Zhao <[email protected]>
> ---
> sound/soc/amd/acp-pcm-dma.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
> index fd3db4c37882..f4011bebc7ec 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -1146,18 +1146,21 @@ static int acp_dma_new(struct snd_soc_pcm_runtime *rtd)
> struct snd_soc_component *component = snd_soc_rtdcom_lookup(rtd,
> DRV_NAME);
> struct audio_drv_data *adata = dev_get_drvdata(component->dev);
> + struct device *parent = component->dev->parent;
>
> switch (adata->asic_type) {
> case CHIP_STONEY:
> ret = snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
> SNDRV_DMA_TYPE_DEV,
> - NULL, ST_MIN_BUFFER,
> + parent,
> + ST_MIN_BUFFER,
> ST_MAX_BUFFER);
> break;
> default:
> ret = snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
> SNDRV_DMA_TYPE_DEV,
> - NULL, MIN_BUFFER,
> + parent,
> + MIN_BUFFER,
> MAX_BUFFER);
> break;
> }
>
The patch
ASoC: use dma_ops of parent device for acp_audio_dma
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
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
From 23aa128bb28d9da69bb1bdb2b70e50128857884a Mon Sep 17 00:00:00 2001
From: Yu Zhao <[email protected]>
Date: Tue, 4 Dec 2018 15:42:53 -0700
Subject: [PATCH] ASoC: use dma_ops of parent device for acp_audio_dma
AMD platform device acp_audio_dma can only be created by parent PCI
device driver (drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c). Pass struct
device of the parent to snd_pcm_lib_preallocate_pages() so
dma_alloc_coherent() can use correct dma_ops. Otherwise, it will
use default dma_ops which is nommu_dma_ops on x86_64 even when
IOMMU is enabled and set to non passthrough mode.
Though platform device inherits some dma related fields during its
creation in mfd_add_device(), we can't simply pass its struct device
to snd_pcm_lib_preallocate_pages() because dma_ops is not among the
inherited fields. Even it were, drivers/iommu/amd_iommu.c would
ignore it because get_device_id() doesn't handle platform device.
This change shouldn't give us any trouble even struct device of the
parent becomes null or represents some non PCI device in the future,
because get_dma_ops() correctly handles null struct device or uses
the default dma_ops if struct device doesn't have it set.
Signed-off-by: Yu Zhao <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
sound/soc/amd/acp-pcm-dma.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index fd3db4c37882..f4011bebc7ec 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -1146,18 +1146,21 @@ static int acp_dma_new(struct snd_soc_pcm_runtime *rtd)
struct snd_soc_component *component = snd_soc_rtdcom_lookup(rtd,
DRV_NAME);
struct audio_drv_data *adata = dev_get_drvdata(component->dev);
+ struct device *parent = component->dev->parent;
switch (adata->asic_type) {
case CHIP_STONEY:
ret = snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
SNDRV_DMA_TYPE_DEV,
- NULL, ST_MIN_BUFFER,
+ parent,
+ ST_MIN_BUFFER,
ST_MAX_BUFFER);
break;
default:
ret = snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
SNDRV_DMA_TYPE_DEV,
- NULL, MIN_BUFFER,
+ parent,
+ MIN_BUFFER,
MAX_BUFFER);
break;
}
--
2.19.0.rc2
The patch
ASoC: use DMA addr rather than CPU pa for acp_audio_dma
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
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
From d6d08273996b3363178b920ccfa74acabaf67963 Mon Sep 17 00:00:00 2001
From: Yu Zhao <[email protected]>
Date: Tue, 4 Dec 2018 15:42:52 -0700
Subject: [PATCH] ASoC: use DMA addr rather than CPU pa for acp_audio_dma
We shouldn't assume CPU physical address we get from page_to_phys()
is same as DMA address we get from dma_alloc_coherent(). On x86_64,
we won't run into any problem with the assumption when dma_ops is
nommu_dma_ops. However, DMA address is IOVA when IOMMU is enabled.
And it's most likely different from CPU physical address when AMD
IOMMU is not in passthrough mode.
Signed-off-by: Yu Zhao <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
sound/soc/amd/acp-pcm-dma.c | 15 +++++----------
sound/soc/amd/acp.h | 2 +-
2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index cdebab2f8ce5..fd3db4c37882 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -303,11 +303,10 @@ static void set_acp_to_i2s_dma_descriptors(void __iomem *acp_mmio, u32 size,
}
/* Create page table entries in ACP SRAM for the allocated memory */
-static void acp_pte_config(void __iomem *acp_mmio, struct page *pg,
+static void acp_pte_config(void __iomem *acp_mmio, dma_addr_t addr,
u16 num_of_pages, u32 pte_offset)
{
u16 page_idx;
- u64 addr;
u32 low;
u32 high;
u32 offset;
@@ -317,7 +316,6 @@ static void acp_pte_config(void __iomem *acp_mmio, struct page *pg,
/* Load the low address of page int ACP SRAM through SRBM */
acp_reg_write((offset + (page_idx * 8)),
acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
- addr = page_to_phys(pg);
low = lower_32_bits(addr);
high = upper_32_bits(addr);
@@ -333,7 +331,7 @@ static void acp_pte_config(void __iomem *acp_mmio, struct page *pg,
acp_reg_write(high, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
/* Move to next physically contiguos page */
- pg++;
+ addr += PAGE_SIZE;
}
}
@@ -343,7 +341,7 @@ static void config_acp_dma(void __iomem *acp_mmio,
{
u16 ch_acp_sysmem, ch_acp_i2s;
- acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
+ acp_pte_config(acp_mmio, rtd->dma_addr, rtd->num_of_pages,
rtd->pte_offset);
if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {
@@ -850,7 +848,6 @@ static int acp_dma_hw_params(struct snd_pcm_substream *substream,
int status;
uint64_t size;
u32 val = 0;
- struct page *pg;
struct snd_pcm_runtime *runtime;
struct audio_substream_data *rtd;
struct snd_soc_pcm_runtime *prtd = substream->private_data;
@@ -986,16 +983,14 @@ static int acp_dma_hw_params(struct snd_pcm_substream *substream,
return status;
memset(substream->runtime->dma_area, 0, params_buffer_bytes(params));
- pg = virt_to_page(substream->dma_buffer.area);
- if (pg) {
+ if (substream->dma_buffer.area) {
acp_set_sram_bank_state(rtd->acp_mmio, 0, true);
/* Save for runtime private data */
- rtd->pg = pg;
+ rtd->dma_addr = substream->dma_buffer.addr;
rtd->order = get_order(size);
/* Fill the page table entries in ACP SRAM */
- rtd->pg = pg;
rtd->size = size;
rtd->num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
rtd->direction = substream->stream;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index dbbb1a85638d..e5ab6c6040a6 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -123,7 +123,7 @@ enum acp_dma_priority_level {
};
struct audio_substream_data {
- struct page *pg;
+ dma_addr_t dma_addr;
unsigned int order;
u16 num_of_pages;
u16 i2s_instance;
--
2.19.0.rc2