Hi, Tomasz, Thanks for the review. > -----Original Message----- > From: Tomasz Figa [mailto:tfiga@xxxxxxxxxxxx] > Sent: Monday, June 18, 2018 12:09 AM > To: Zhi, Yong <yong.zhi@xxxxxxxxx> > Cc: Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx>; Sakari Ailus > <sakari.ailus@xxxxxxxxxxxxxxx>; Mani, Rajmohan > <rajmohan.mani@xxxxxxxxx>; Toivonen, Tuukka > <tuukka.toivonen@xxxxxxxxx>; Hu, Jerry W <jerry.w.hu@xxxxxxxxx>; Zheng, > Jian Xu <jian.xu.zheng@xxxxxxxxx> > Subject: Re: [PATCH v6 04/12] intel-ipu3: Implement DMA mapping > functions > > On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi <yong.zhi@xxxxxxxxx> wrote: > > > > From: Tomasz Figa <tfiga@xxxxxxxxxxxx> > > > > 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 <tfiga@xxxxxxxxxxxx> > > Signed-off-by: Yong Zhi <yong.zhi@xxxxxxxxx> > > --- > > 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 000000000000..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 000000000000..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 <tfiga@xxxxxxxxxxxx> > > + * Author: Yong Zhi <yong.zhi@xxxxxxxxx> */ > > + > > +#include <linux/vmalloc.h> > > + > > +#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. > Ack. > > + > > 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? > Agreed in principle to pass struct imgu_device to all ipu3_dmamap_*, will try and evaluate the changes. > > + unsigned long shift = iova_shift(&imgu->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; > > + > > + if (WARN_ON(!dev)) > > + return NULL; > > Isn't this impossible to happen? Indeed, this should not happen. > > > + > > + dev_dbg(dev, "%s: allocating %zu\n", __func__, size); > > + > > + iova = alloc_iova(&imgu->iova_domain, size >> shift, > > + imgu->mmu->aperture_end >> shift, 0); > > + if (!iova) > > + return NULL; > [snip] > > +void ipu3_dmamap_exit(struct device *dev) { > > + struct imgu_device *imgu = dev_get_drvdata(dev); > > + > > + put_iova_domain(&imgu->iova_domain); > > + iova_cache_put(); > > + imgu->mmu = NULL; > > We can't set mmu to NULL here, because ipu3_mmu module is the owner of > it and it will be still dereferenced in ipu3_mmu_exit(). (This might be fixed > in your tree already as per > https://chromium- > review.googlesource.com/c/chromiumos/third_party/kernel/+/1084522) > Ack, thanks for the fix!! > > +} > [snip] > > diff --git a/drivers/media/pci/intel/ipu3/ipu3.h > > b/drivers/media/pci/intel/ipu3/ipu3.h > > new file mode 100644 > > index 000000000000..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 <linux/iova.h> > > +#include <linux/pci.h> > > + > > +#include <media/v4l2-device.h> > > +#include <media/videobuf2-dma-sg.h> > > + > > +#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. > Best regards, > Tomasz
