Re: [PATCH v6 04/12] intel-ipu3: Implement DMA mapping functions
On Tue, Jun 19, 2018 at 12:42 AM Zhi, Yong wrote: > > Hi, Tomasz, > > Thanks for the review. > > > -Original Message- > > From: Tomasz Figa [mailto:tf...@chromium.org] > > Sent: Monday, June 18, 2018 12:09 AM > > To: Zhi, Yong > > Cc: Linux Media Mailing List ; Sakari Ailus > > ; Mani, Rajmohan > > ; Toivonen, Tuukka > > ; Hu, Jerry W ; Zheng, > > Jian Xu > > Subject: Re: [PATCH v6 04/12] intel-ipu3: Implement DMA mapping > > functions > > > > On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi wrote: [snip] > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3.h > > > b/drivers/media/pci/intel/ipu3/ipu3.h > > > new file mode 100644 > > > index ..2ba6fa58e41c > > > --- /dev/null > > > +++ b/drivers/media/pci/intel/ipu3/ipu3.h > > > @@ -0,0 +1,151 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +/* Copyright (C) 2018 Intel Corporation */ > > > + > > > +#ifndef __IPU3_H > > > +#define __IPU3_H > > > + > > > +#include > > > +#include > > > + > > > +#include > > > +#include > > > + > > > +#include "ipu3-css.h" > > > + > > > +#define IMGU_NAME "ipu3-imgu" > > > + > > > +/* > > > + * The semantics of the driver is that whenever there is a buffer > > > +available in > > > + * master queue, the driver queues a buffer also to all other active > > nodes. > > > + * If user space hasn't provided a buffer to all other video nodes > > > +first, > > > + * the driver gets an internal dummy buffer and queues it. > > > + */ > > > +#define IMGU_QUEUE_MASTER IPU3_CSS_QUEUE_IN > > > +#define IMGU_QUEUE_FIRST_INPUT IPU3_CSS_QUEUE_OUT > > > +#define IMGU_MAX_QUEUE_DEPTH (2 + 2) > > > + > > > +#define IMGU_NODE_IN 0 /* Input RAW image */ > > > +#define IMGU_NODE_PARAMS 1 /* Input parameters */ > > > +#define IMGU_NODE_OUT 2 /* Main output for still or > > > video > > */ > > > +#define IMGU_NODE_VF 3 /* Preview */ > > > +#define IMGU_NODE_PV 4 /* Postview for still capture */ > > > +#define IMGU_NODE_STAT_3A 5 /* 3A statistics */ > > > +#define IMGU_NODE_NUM 6 > > > > Does this file really belong to this patch? > > > > Included because ipu3-dmamap uses struct defined in this header. It sounds like we should either move this patch later in the series or have just the necessary minimum or only forward declarations added in this patch. Best regards, Tomasz
RE: [PATCH v6 04/12] intel-ipu3: Implement DMA mapping functions
Hi, Tomasz,
Thanks for the review.
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Monday, June 18, 2018 12:09 AM
> To: Zhi, Yong
> Cc: Linux Media Mailing List ; Sakari Ailus
> ; Mani, Rajmohan
> ; Toivonen, Tuukka
> ; Hu, Jerry W ; Zheng,
> Jian Xu
> Subject: Re: [PATCH v6 04/12] intel-ipu3: Implement DMA mapping
> functions
>
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi wrote:
> >
> > From: Tomasz Figa
> >
> > This driver uses IOVA space for buffer mapping through IPU3 MMU to
> > transfer data between imaging pipelines and system DDR.
> >
> > Signed-off-by: Tomasz Figa
> > Signed-off-by: Yong Zhi
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-css-pool.h | 36
> > drivers/media/pci/intel/ipu3/ipu3-dmamap.c | 280
> +++
> > drivers/media/pci/intel/ipu3/ipu3-dmamap.h | 22 +++
> > drivers/media/pci/intel/ipu3/ipu3.h | 151 +++
> > 4 files changed, 489 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.h
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3.h
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> > b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> > new file mode 100644
> > index ..4b22e0856232
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> > @@ -0,0 +1,36 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (C) 2018 Intel Corporation */
> > +
> > +#ifndef __IPU3_UTIL_H
> > +#define __IPU3_UTIL_H
> > +
> > +struct device;
> > +
> > +#define IPU3_CSS_POOL_SIZE 4
> > +
> > +struct ipu3_css_map {
> > + size_t size;
> > + void *vaddr;
> > + dma_addr_t daddr;
> > + struct vm_struct *vma;
> > +};
> > +
> > +struct ipu3_css_pool {
> > + struct {
> > + struct ipu3_css_map param;
> > + long framenum;
> > + } entry[IPU3_CSS_POOL_SIZE];
> > + unsigned int last; /* Latest entry */
>
> It's not clear what "Latest entry" means here. Since these structs are a part
> of the interface exposed by this header, could you write proper kerneldoc
> comments for all fields in both of them?
>
Sure.
> > +};
> > +
> > +int ipu3_css_dma_buffer_resize(struct device *dev, struct ipu3_css_map
> *map,
> > + size_t size); void
> > +ipu3_css_pool_cleanup(struct device *dev, struct ipu3_css_pool
> > +*pool); int ipu3_css_pool_init(struct device *dev, struct ipu3_css_pool
> *pool,
> > + size_t size);
> > +int ipu3_css_pool_get(struct ipu3_css_pool *pool, long framenum);
> > +void ipu3_css_pool_put(struct ipu3_css_pool *pool); const struct
> > +ipu3_css_map *ipu3_css_pool_last(struct ipu3_css_pool *pool,
> > + unsigned int last);
> > +
> > +#endif
> > 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 ..b2bc5d00debc
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> > @@ -0,0 +1,280 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2018 Intel Corporation
> > + * Copyright (C) 2018 Google, Inc.
>
> Would you mind changing as below?
>
> Copyright 2018 Google LLC.
>
Ack.
> > + *
> > + * Author: Tomasz Figa
> > + * Author: Yong Zhi */
> > +
> > +#include
> > +
> > +#include "ipu3.h"
> > +#include "ipu3-css-pool.h"
> > +#include "ipu3-mmu.h"
> > +
> > +/*
> > + * Free a buffer allocated by ipu3_dmamap_alloc_buffer() */ static
> > +void ipu3_dmamap_free_buffer(struct page **pages,
> > + size_t size) {
> > + int count = size >> PAGE_SHIFT;
> > +
> > + while (count--)
> > + __free_page(pages[count]);
> > + kvfree(pages);
> > +}
> > +
> > +/*
> > + * Based on the implementation of __iommu_dma_alloc_pages()
> > + * defined in drivers/iommu/dma-iommu.c */ static struct page
> > +**ipu3_dmamap_alloc_buffer(size_t size,
> > + unsigned long order_mask,
> > +
Re: [PATCH v6 04/12] intel-ipu3: Implement DMA mapping functions
On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi wrote:
>
> From: Tomasz Figa
>
> This driver uses IOVA space for buffer mapping through IPU3 MMU
> to transfer data between imaging pipelines and system DDR.
>
> Signed-off-by: Tomasz Figa
> Signed-off-by: Yong Zhi
> ---
> drivers/media/pci/intel/ipu3/ipu3-css-pool.h | 36
> drivers/media/pci/intel/ipu3/ipu3-dmamap.c | 280
> +++
> drivers/media/pci/intel/ipu3/ipu3-dmamap.h | 22 +++
> drivers/media/pci/intel/ipu3/ipu3.h | 151 +++
> 4 files changed, 489 insertions(+)
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-dmamap.h
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3.h
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> new file mode 100644
> index ..4b22e0856232
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2018 Intel Corporation */
> +
> +#ifndef __IPU3_UTIL_H
> +#define __IPU3_UTIL_H
> +
> +struct device;
> +
> +#define IPU3_CSS_POOL_SIZE 4
> +
> +struct ipu3_css_map {
> + size_t size;
> + void *vaddr;
> + dma_addr_t daddr;
> + struct vm_struct *vma;
> +};
> +
> +struct ipu3_css_pool {
> + struct {
> + struct ipu3_css_map param;
> + long framenum;
> + } entry[IPU3_CSS_POOL_SIZE];
> + unsigned int last; /* Latest entry */
It's not clear what "Latest entry" means here. Since these structs are
a part of the interface exposed by this header, could you write proper
kerneldoc comments for all fields in both of them?
> +};
> +
> +int ipu3_css_dma_buffer_resize(struct device *dev, struct ipu3_css_map *map,
> + size_t size);
> +void ipu3_css_pool_cleanup(struct device *dev, struct ipu3_css_pool *pool);
> +int ipu3_css_pool_init(struct device *dev, struct ipu3_css_pool *pool,
> + size_t size);
> +int ipu3_css_pool_get(struct ipu3_css_pool *pool, long framenum);
> +void ipu3_css_pool_put(struct ipu3_css_pool *pool);
> +const struct ipu3_css_map *ipu3_css_pool_last(struct ipu3_css_pool *pool,
> + unsigned int last);
> +
> +#endif
> 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 ..b2bc5d00debc
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Intel Corporation
> + * Copyright (C) 2018 Google, Inc.
Would you mind changing as below?
Copyright 2018 Google LLC.
> + *
> + * Author: Tomasz Figa
> + * Author: Yong Zhi
> + */
> +
> +#include
> +
> +#include "ipu3.h"
> +#include "ipu3-css-pool.h"
> +#include "ipu3-mmu.h"
> +
> +/*
> + * Free a buffer allocated by ipu3_dmamap_alloc_buffer()
> + */
> +static void ipu3_dmamap_free_buffer(struct page **pages,
> + size_t size)
> +{
> + int count = size >> PAGE_SHIFT;
> +
> + while (count--)
> + __free_page(pages[count]);
> + kvfree(pages);
> +}
> +
> +/*
> + * Based on the implementation of __iommu_dma_alloc_pages()
> + * defined in drivers/iommu/dma-iommu.c
> + */
> +static struct page **ipu3_dmamap_alloc_buffer(size_t size,
> + unsigned long order_mask,
> + gfp_t gfp)
> +{
> + struct page **pages;
> + unsigned int i = 0, count = size >> PAGE_SHIFT;
> + const gfp_t high_order_gfp = __GFP_NOWARN | __GFP_NORETRY;
> +
> + /* Allocate mem for array of page ptrs */
> + pages = kvmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
sizeof(*pages) to ensure that the right type is used regardless of declaration.
> +
No need for this blank line.
> + if (!pages)
> + return NULL;
[snip]
> +/**
> + * ipu3_dmamap_alloc - allocate and map a buffer into KVA
> + * @dev: struct device pointer
> + * @map: struct to store mapping variables
> + * @len: size required
> + *
> + * Return KVA on success or NULL on failure
> + */
> +void *ipu3_dmamap_alloc(struct device *dev, struct ipu3_css_map *map,
> + size_t len)
> +{
> + struct imgu_device *imgu = dev_get_drvdata(dev);
Wouldn't it make more sense to just pass struct imgu_device pointer to
all the functions in this file directly?
> + unsigned long shift = iova_shift(>iova_domain);
> + unsigned int alloc_sizes = imgu->mmu->pgsize_bitmap;
> + size_t size = PAGE_ALIGN(len);
> + struct page **pages;
> + dma_addr_t iovaddr;
> + struct iova *iova;
> + int i, rval;
> +
