Re: [PATCH v3 03/12] intel-ipu3: Add DMA API implementation
On Fri, Jul 21, 2017 at 7:09 AM, Sakari Ailus wrote: > Hi Arnd, > > On Wed, Jul 19, 2017 at 09:24:41AM +0200, Arnd Bergmann wrote: >> On Wed, Jul 19, 2017 at 5:12 AM, Yong Zhi wrote: >> > From: Tomasz Figa >> > >> > This patch adds support for the IPU3 DMA mapping API. >> > >> > Signed-off-by: Tomasz Figa >> > Signed-off-by: Yong Zhi >> >> This needs some explanation on why you decided to go down the >> route of adding your own dma_map_ops. It's not obvious at all, >> and and I'm still concerned that this complicates things more than >> it helps. > > There are a few considerations here --- they could be documented in the > patch commit message > > - The device has its own MMU. The default x86 DMA ops assume there isn't. > > - As this is an image signal processor device, the buffers are typically > large (often in the range of tens of MB) and they do not need to be > physically contiguous. The current implementation of e.g. > drivers/iommu/intel-iommu.c allocate memory using alloc_pages() which is > unfeasible for such single allocations. Neither CMA is needed. > > Also other IOMMU implementations have their own DMA ops currently. > > I agree it'd be nice to unify these in the long run but I don't think this > stands apart from the rest currently --- except that the MMU is only used > by a single PCI device, the same which it is contained in. On top of what Sakari said, it just perfectly matches what V4L2 videobuf2 framework expects. It does all the buffer mapping and synchronization using DMA mapping and given the x86 DMA ops being useless for this device, it makes everything that videobuf2 does using them useless too. Best regards, Tomasz
Re: [PATCH v3 03/12] intel-ipu3: Add DMA API implementation
Hi Arnd, On Wed, Jul 19, 2017 at 09:24:41AM +0200, Arnd Bergmann wrote: > On Wed, Jul 19, 2017 at 5:12 AM, Yong Zhi wrote: > > From: Tomasz Figa > > > > This patch adds support for the IPU3 DMA mapping API. > > > > Signed-off-by: Tomasz Figa > > Signed-off-by: Yong Zhi > > This needs some explanation on why you decided to go down the > route of adding your own dma_map_ops. It's not obvious at all, > and and I'm still concerned that this complicates things more than > it helps. There are a few considerations here --- they could be documented in the patch commit message - The device has its own MMU. The default x86 DMA ops assume there isn't. - As this is an image signal processor device, the buffers are typically large (often in the range of tens of MB) and they do not need to be physically contiguous. The current implementation of e.g. drivers/iommu/intel-iommu.c allocate memory using alloc_pages() which is unfeasible for such single allocations. Neither CMA is needed. Also other IOMMU implementations have their own DMA ops currently. I agree it'd be nice to unify these in the long run but I don't think this stands apart from the rest currently --- except that the MMU is only used by a single PCI device, the same which it is contained in. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
Re: [PATCH v3 03/12] intel-ipu3: Add DMA API implementation
Hi Yong,
On Tue, Jul 18, 2017 at 10:12:58PM -0500, Yong Zhi wrote:
> From: Tomasz Figa
>
> This patch adds support for the IPU3 DMA mapping API.
>
> Signed-off-by: Tomasz Figa
> Signed-off-by: Yong Zhi
> ---
> drivers/media/pci/intel/ipu3/Kconfig | 8 +
> drivers/media/pci/intel/ipu3/Makefile | 2 +-
> drivers/media/pci/intel/ipu3/ipu3-dmamap.c | 302
> +
> drivers/media/pci/intel/ipu3/ipu3-dmamap.h | 22 +++
> 4 files changed, 333 insertions(+), 1 deletion(-)
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.h
>
> diff --git a/drivers/media/pci/intel/ipu3/Kconfig
> b/drivers/media/pci/intel/ipu3/Kconfig
> index 7bcdfa5..d503806 100644
> --- a/drivers/media/pci/intel/ipu3/Kconfig
> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> @@ -24,3 +24,11 @@ config INTEL_IPU3_MMU
> ---help---
> For IPU3, this option enables its MMU driver to translate its internal
> virtual address to 39 bits wide physical address for 64GBytes space
> access.
> +
> +config INTEL_IPU3_DMAMAP
> + tristate
> + default n
> + select IOMMU_DMA
> + select IOMMU_IOVA
> + ---help---
> + This is IPU3 IOMMU domain specific DMA driver.
> diff --git a/drivers/media/pci/intel/ipu3/Makefile
> b/drivers/media/pci/intel/ipu3/Makefile
> index 91cac9c..6517732 100644
> --- a/drivers/media/pci/intel/ipu3/Makefile
> +++ b/drivers/media/pci/intel/ipu3/Makefile
> @@ -13,4 +13,4 @@
>
> obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
> obj-$(CONFIG_INTEL_IPU3_MMU) += ipu3-mmu.o
> -
> +obj-$(CONFIG_INTEL_IPU3_DMAMAP) += ipu3-dmamap.o
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> b/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> new file mode 100644
> index 000..86a0e15
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> @@ -0,0 +1,302 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation.
> + * Copyright (C) 2017 Google, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "ipu3-mmu.h"
> +
> +/*
> + * Based on arch/arm64/mm/dma-mapping.c, with simplifications possible due
> + * to driver-specific character of this file.
> + */
> +
> +static pgprot_t __get_dma_pgprot(unsigned long attrs, pgprot_t prot)
> +{
> + if (DMA_ATTR_NON_CONSISTENT & attrs)
> + return prot;
> + return pgprot_writecombine(prot);
> +}
> +
> +static void flush_page(struct device *dev, const void *virt, phys_addr_t
> phys)
> +{
> + /*
> + * FIXME: Yes, casting to override the const specifier is ugly.
> + * However, for some reason, this callback is intended to flush cache
> + * for a page pointed to by a const pointer, even though the cach
> + * flush operation by definition does not keep the affected memory
> + * constant...
> + */
> + clflush_cache_range((void *)virt, PAGE_SIZE);
Hmm. Is this needed? The hardware is coherent --- apart from the MMU
tables.
Same for the flushes below.
> +}
> +
> +static void *ipu3_dmamap_alloc(struct device *dev, size_t size,
> +dma_addr_t *handle, gfp_t gfp,
> +unsigned long attrs)
> +{
> + int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, false, attrs);
> + size_t iosize = size;
> + struct page **pages;
> + pgprot_t prot;
> + void *addr;
> +
> + if (WARN(!dev, "cannot create IOMMU mapping for unknown device\n"))
> + return NULL;
> +
> + if (WARN(!gfpflags_allow_blocking(gfp),
> + "atomic allocations not supported\n") ||
> + WARN((DMA_ATTR_FORCE_CONTIGUOUS & attrs),
> + "contiguous allocations not supported\n"))
> + return NULL;
> +
> + size = PAGE_ALIGN(size);
> +
> + dev_dbg(dev, "%s: allocating %zu\n", __func__, size);
> +
> + /*
> + * Some drivers rely on this, and we probably don't want the
> + * possibility of stale kernel data being read by devices anyway.
> + */
> + gfp |= __GFP_ZERO;
> +
> + /*
> + * On x86, __GFP_DMA or __GFP_DMA32 might be added implicitly, based
> + * on device DMA mask. However the mask does not apply to the IOMMU,
> + * which is expected to be able to map any physical page.
> + */
> + gfp &= ~(__GFP_DMA | __GFP_DMA32);
> +
> + pages = iommu_dma_alloc(dev, iosize, gfp, attrs, ioprot,
> + handle,
Re: [PATCH v3 03/12] intel-ipu3: Add DMA API implementation
On 19/07/17 04:12, Yong Zhi wrote:
> From: Tomasz Figa
>
> This patch adds support for the IPU3 DMA mapping API.
>
> Signed-off-by: Tomasz Figa
> Signed-off-by: Yong Zhi
> ---
> drivers/media/pci/intel/ipu3/Kconfig | 8 +
> drivers/media/pci/intel/ipu3/Makefile | 2 +-
> drivers/media/pci/intel/ipu3/ipu3-dmamap.c | 302
> +
> drivers/media/pci/intel/ipu3/ipu3-dmamap.h | 22 +++
> 4 files changed, 333 insertions(+), 1 deletion(-)
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.h
>
> diff --git a/drivers/media/pci/intel/ipu3/Kconfig
> b/drivers/media/pci/intel/ipu3/Kconfig
> index 7bcdfa5..d503806 100644
> --- a/drivers/media/pci/intel/ipu3/Kconfig
> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> @@ -24,3 +24,11 @@ config INTEL_IPU3_MMU
> ---help---
> For IPU3, this option enables its MMU driver to translate its internal
> virtual address to 39 bits wide physical address for 64GBytes space
> access.
> +
> +config INTEL_IPU3_DMAMAP
> + tristate
> + default n
bool again?
> + select IOMMU_DMA
> + select IOMMU_IOVA
> + ---help---
> + This is IPU3 IOMMU domain specific DMA driver.
> diff --git a/drivers/media/pci/intel/ipu3/Makefile
> b/drivers/media/pci/intel/ipu3/Makefile
> index 91cac9c..6517732 100644
> --- a/drivers/media/pci/intel/ipu3/Makefile
> +++ b/drivers/media/pci/intel/ipu3/Makefile
> @@ -13,4 +13,4 @@
>
> obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
> obj-$(CONFIG_INTEL_IPU3_MMU) += ipu3-mmu.o
> -
> +obj-$(CONFIG_INTEL_IPU3_DMAMAP) += ipu3-dmamap.o
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> b/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> new file mode 100644
> index 000..86a0e15
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> @@ -0,0 +1,302 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation.
> + * Copyright (C) 2017 Google, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "ipu3-mmu.h"
> +
> +/*
> + * Based on arch/arm64/mm/dma-mapping.c, with simplifications possible due
> + * to driver-specific character of this file.
> + */
> +
> +static pgprot_t __get_dma_pgprot(unsigned long attrs, pgprot_t prot)
> +{
> + if (DMA_ATTR_NON_CONSISTENT & attrs)
> + return prot;
> + return pgprot_writecombine(prot);
> +}
> +
> +static void flush_page(struct device *dev, const void *virt, phys_addr_t
> phys)
> +{
> + /*
> + * FIXME: Yes, casting to override the const specifier is ugly.
> + * However, for some reason, this callback is intended to flush cache
> + * for a page pointed to by a const pointer, even though the cach
> + * flush operation by definition does not keep the affected memory
> + * constant...
> + */
I don't follow that comment - in terms of the C abstract machine,
flush_page() certainly should not be changing the contents of the buffer
pointed to by virt; stepping down to the architectural level, this is a
clflush of a freshly-dirtied VA, so shouldn't change what a CPU read
from that VA returns either. If the act of making a device see the same
memory contents as the CPU sees changes what the CPU sees, something's
horribly, horribly wrong.
FWIW, it should technically be fine to use clwb here if you really
wanted to, although it looks like that wouldn't help much.
> + clflush_cache_range((void *)virt, PAGE_SIZE);
> +}
> +
> +static void *ipu3_dmamap_alloc(struct device *dev, size_t size,
> +dma_addr_t *handle, gfp_t gfp,
> +unsigned long attrs)
> +{
> + int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, false, attrs);
> + size_t iosize = size;
> + struct page **pages;
> + pgprot_t prot;
> + void *addr;
> +
> + if (WARN(!dev, "cannot create IOMMU mapping for unknown device\n"))
> + return NULL;
Since this is a private set of DMA ops, this would only be
sanity-checking the other parts of the IPU driver, which should be
unnecessary. Also I don't think it's even possible to get here without a
valid dev anyway.
> + if (WARN(!gfpflags_allow_blocking(gfp),
> + "atomic allocations not supported\n") ||
> + WARN((DMA_ATTR_FORCE_CONTIGUOUS & attrs),
> + "contiguous allocations not supported\n"))
The standard practice for unsupported DMA attribute is to si
Re: [PATCH v3 03/12] intel-ipu3: Add DMA API implementation
On Wed, Jul 19, 2017 at 5:12 AM, Yong Zhi wrote: > From: Tomasz Figa > > This patch adds support for the IPU3 DMA mapping API. > > Signed-off-by: Tomasz Figa > Signed-off-by: Yong Zhi This needs some explanation on why you decided to go down the route of adding your own dma_map_ops. It's not obvious at all, and and I'm still concerned that this complicates things more than it helps. Arnd
