RE: [PATCH v7 15/16] intel-ipu3: Add imgu top level pci device driver

2018-11-12 Thread Zhi, Yong 
Hi, Sakari,

Thanks again for the code review.

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Friday, November 9, 2018 6:54 AM
> To: Zhi, Yong 
> Cc: linux-media@vger.kernel.org; tf...@chromium.org;
> mche...@kernel.org; hans.verk...@cisco.com;
> laurent.pinch...@ideasonboard.com; Mani, Rajmohan
> ; Zheng, Jian Xu ; Hu,
> Jerry W ; Toivonen, Tuukka
> ; Qiu, Tian Shu ; Cao,
> Bingbu 
> Subject: Re: [PATCH v7 15/16] intel-ipu3: Add imgu top level pci device driver
> 
> Hi Yong,
> 
> On Mon, Oct 29, 2018 at 03:23:09PM -0700, Yong Zhi wrote:
> > This patch adds support for the Intel IPU v3 as found on Skylake and
> > Kaby Lake SoCs.
> >
> > The driver glues v4l2, css(camera sub system) and other pieces
> > together to perform its functions, it also loads the IPU3 firmware
> > binary as part of its initialization.
> >
> > Signed-off-by: Yong Zhi 
> > Signed-off-by: Tomasz Figa 
> > ---
> >  drivers/media/pci/intel/ipu3/Kconfig  |  16 +
> > drivers/media/pci/intel/ipu3/Makefile |  12 +
> >  drivers/media/pci/intel/ipu3/ipu3.c   | 844
> ++
> >  drivers/media/pci/intel/ipu3/ipu3.h   | 153 ++
> >  4 files changed, 1025 insertions(+)
> >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3.c
> >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3.h
> >
> > diff --git a/drivers/media/pci/intel/ipu3/Kconfig
> > b/drivers/media/pci/intel/ipu3/Kconfig
> > index 715f776..44ebcbb 100644
> > --- a/drivers/media/pci/intel/ipu3/Kconfig
> > +++ b/drivers/media/pci/intel/ipu3/Kconfig
> > @@ -15,3 +15,19 @@ config VIDEO_IPU3_CIO2
> >   Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
> >   connected camera.
> >   The module will be called ipu3-cio2.
> > +
> > +config VIDEO_IPU3_IMGU
> > +   tristate "Intel ipu3-imgu driver"
> > +   depends on PCI && VIDEO_V4L2
> > +   depends on MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
> > +   depends on X86
> > +   select IOMMU_IOVA
> > +   select VIDEOBUF2_DMA_SG
> > +
> 
> Extra newline.
> 

Ack.

> > +   ---help---
> > + This is the video4linux2 driver for Intel IPU3 image processing
> > +unit,
> 
> "Video4Linux2"
> 

Ack.

> > + found in Intel Skylake and Kaby Lake SoCs and used for processing
> > + images and video.
> > +
> > + Say Y or M here if you have a Skylake/Kaby Lake SoC with a MIPI
> > + camera.   The module will be called ipu3-imgu.
> 
> The latter tab should be a space only.
> 

Ack.

> > diff --git a/drivers/media/pci/intel/ipu3/Makefile
> > b/drivers/media/pci/intel/ipu3/Makefile
> > index 20186e3..60bd5db 100644
> > --- a/drivers/media/pci/intel/ipu3/Makefile
> > +++ b/drivers/media/pci/intel/ipu3/Makefile
> > @@ -1 +1,13 @@
> > +#
> > +# Makefile for the IPU3 cio2 and ImgU drivers #
> > +
> >  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
> > +
> > +ipu3-imgu-objs += \
> > +   ipu3-mmu.o ipu3-dmamap.o \
> > +   ipu3-tables.o ipu3-css-pool.o \
> > +   ipu3-css-fw.o ipu3-css-params.o \
> > +   ipu3-css.o ipu3-v4l2.o ipu3.o
> > +
> > +obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3.c
> > b/drivers/media/pci/intel/ipu3/ipu3.c
> > new file mode 100644
> > index 000..eda7299
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3.c
> > @@ -0,0 +1,844 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017 Intel Corporation
> > + * Copyright 2017 Google LLC
> > + *
> > + * Based on Intel IPU4 driver.
> > + *
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "ipu3.h"
> > +#include "ipu3-dmamap.h"
> > +#include "ipu3-mmu.h"
> > +
> > +#define IMGU_PCI_ID0x1919
> > +#define IMGU_PCI_BAR   0
> > +#define IMGU_DMA_MASK  DMA_BIT_MASK(39)
> > +#define IMGU_MAX_QUEUE_DEPTH   (2 + 2)
> > +
> > +/*
> > + * pre-allocated buffer size for IMGU dummy buffers. Those
> > + * values should be tuned to big enough to avoid buffer
> > + * re-allocation when streaming to lower streaming latency.
> > + */
> > +#define CSS_QUEUE_IN_BUF_SIZE  0
> > +#define CSS_QUEUE_PARAMS_BUF_SI

Re: [PATCH v7 15/16] intel-ipu3: Add imgu top level pci device driver

2018-11-09 Thread Sakari Ailus 
Hi Yong,

On Mon, Oct 29, 2018 at 03:23:09PM -0700, Yong Zhi wrote:
> This patch adds support for the Intel IPU v3 as found
> on Skylake and Kaby Lake SoCs.
> 
> The driver glues v4l2, css(camera sub system) and other
> pieces together to perform its functions, it also loads
> the IPU3 firmware binary as part of its initialization.
> 
> Signed-off-by: Yong Zhi 
> Signed-off-by: Tomasz Figa 
> ---
>  drivers/media/pci/intel/ipu3/Kconfig  |  16 +
>  drivers/media/pci/intel/ipu3/Makefile |  12 +
>  drivers/media/pci/intel/ipu3/ipu3.c   | 844 
> ++
>  drivers/media/pci/intel/ipu3/ipu3.h   | 153 ++
>  4 files changed, 1025 insertions(+)
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3.c
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3.h
> 
> diff --git a/drivers/media/pci/intel/ipu3/Kconfig 
> b/drivers/media/pci/intel/ipu3/Kconfig
> index 715f776..44ebcbb 100644
> --- a/drivers/media/pci/intel/ipu3/Kconfig
> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> @@ -15,3 +15,19 @@ config VIDEO_IPU3_CIO2
> Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
> connected camera.
> The module will be called ipu3-cio2.
> +
> +config VIDEO_IPU3_IMGU
> + tristate "Intel ipu3-imgu driver"
> + depends on PCI && VIDEO_V4L2
> + depends on MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
> + depends on X86
> + select IOMMU_IOVA
> + select VIDEOBUF2_DMA_SG
> +

Extra newline.

> + ---help---
> +   This is the video4linux2 driver for Intel IPU3 image processing unit,

"Video4Linux2"

> +   found in Intel Skylake and Kaby Lake SoCs and used for processing
> +   images and video.
> +
> +   Say Y or M here if you have a Skylake/Kaby Lake SoC with a MIPI
> +   camera.   The module will be called ipu3-imgu.

The latter tab should be a space only.

> diff --git a/drivers/media/pci/intel/ipu3/Makefile 
> b/drivers/media/pci/intel/ipu3/Makefile
> index 20186e3..60bd5db 100644
> --- a/drivers/media/pci/intel/ipu3/Makefile
> +++ b/drivers/media/pci/intel/ipu3/Makefile
> @@ -1 +1,13 @@
> +#
> +# Makefile for the IPU3 cio2 and ImgU drivers
> +#
> +
>  obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
> +
> +ipu3-imgu-objs += \
> + ipu3-mmu.o ipu3-dmamap.o \
> + ipu3-tables.o ipu3-css-pool.o \
> + ipu3-css-fw.o ipu3-css-params.o \
> + ipu3-css.o ipu3-v4l2.o ipu3.o
> +
> +obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o
> diff --git a/drivers/media/pci/intel/ipu3/ipu3.c 
> b/drivers/media/pci/intel/ipu3/ipu3.c
> new file mode 100644
> index 000..eda7299
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3.c
> @@ -0,0 +1,844 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 Intel Corporation
> + * Copyright 2017 Google LLC
> + *
> + * Based on Intel IPU4 driver.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "ipu3.h"
> +#include "ipu3-dmamap.h"
> +#include "ipu3-mmu.h"
> +
> +#define IMGU_PCI_ID  0x1919
> +#define IMGU_PCI_BAR 0
> +#define IMGU_DMA_MASKDMA_BIT_MASK(39)
> +#define IMGU_MAX_QUEUE_DEPTH (2 + 2)
> +
> +/*
> + * pre-allocated buffer size for IMGU dummy buffers. Those
> + * values should be tuned to big enough to avoid buffer
> + * re-allocation when streaming to lower streaming latency.
> + */
> +#define CSS_QUEUE_IN_BUF_SIZE0
> +#define CSS_QUEUE_PARAMS_BUF_SIZE0
> +#define CSS_QUEUE_OUT_BUF_SIZE   (4160 * 3120 * 12 / 8)
> +#define CSS_QUEUE_VF_BUF_SIZE(1920 * 1080 * 12 / 8)
> +#define CSS_QUEUE_STAT_3A_BUF_SIZE   125664

Could you use sizeof(struct ipu3_uapi_stats_3a) instead?

That said, it might not be a bad idea to add a sanity check on the size:

BUILD_BUG_ON(sizeof(struct ipu3_uapi_stats_3a) !=
 CSS_QUEUE_STAT_3A_BUF_SIZE);

...

> diff --git a/drivers/media/pci/intel/ipu3/ipu3.h 
> b/drivers/media/pci/intel/ipu3/ipu3.h
> new file mode 100644
> index 000..5c2b420
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3.h
> @@ -0,0 +1,153 @@
> +/* 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_MASTERIPU3_CSS_QUEUE_IN
> +#define IMGU_QUEUE_FIRST_INPUT   IPU3_CSS_QUEUE_OUT
> +#define IMGU_MAX_QUEUE_DEPTH (2 + 2)
> +
> +#define IMGU_NODE_IN 0 /* Inpu

[PATCH v7 15/16] intel-ipu3: Add imgu top level pci device driver

2018-10-29 Thread Yong Zhi 
This patch adds support for the Intel IPU v3 as found
on Skylake and Kaby Lake SoCs.

The driver glues v4l2, css(camera sub system) and other
pieces together to perform its functions, it also loads
the IPU3 firmware binary as part of its initialization.

Signed-off-by: Yong Zhi 
Signed-off-by: Tomasz Figa 
---
 drivers/media/pci/intel/ipu3/Kconfig  |  16 +
 drivers/media/pci/intel/ipu3/Makefile |  12 +
 drivers/media/pci/intel/ipu3/ipu3.c   | 844 ++
 drivers/media/pci/intel/ipu3/ipu3.h   | 153 ++
 4 files changed, 1025 insertions(+)
 create mode 100644 drivers/media/pci/intel/ipu3/ipu3.c
 create mode 100644 drivers/media/pci/intel/ipu3/ipu3.h

diff --git a/drivers/media/pci/intel/ipu3/Kconfig 
b/drivers/media/pci/intel/ipu3/Kconfig
index 715f776..44ebcbb 100644
--- a/drivers/media/pci/intel/ipu3/Kconfig
+++ b/drivers/media/pci/intel/ipu3/Kconfig
@@ -15,3 +15,19 @@ config VIDEO_IPU3_CIO2
  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
  connected camera.
  The module will be called ipu3-cio2.
+
+config VIDEO_IPU3_IMGU
+   tristate "Intel ipu3-imgu driver"
+   depends on PCI && VIDEO_V4L2
+   depends on MEDIA_CONTROLLER && VIDEO_V4L2_SUBDEV_API
+   depends on X86
+   select IOMMU_IOVA
+   select VIDEOBUF2_DMA_SG
+
+   ---help---
+ This is the video4linux2 driver for Intel IPU3 image processing unit,
+ found in Intel Skylake and Kaby Lake SoCs and used for processing
+ images and video.
+
+ Say Y or M here if you have a Skylake/Kaby Lake SoC with a MIPI
+ camera.   The module will be called ipu3-imgu.
diff --git a/drivers/media/pci/intel/ipu3/Makefile 
b/drivers/media/pci/intel/ipu3/Makefile
index 20186e3..60bd5db 100644
--- a/drivers/media/pci/intel/ipu3/Makefile
+++ b/drivers/media/pci/intel/ipu3/Makefile
@@ -1 +1,13 @@
+#
+# Makefile for the IPU3 cio2 and ImgU drivers
+#
+
 obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
+
+ipu3-imgu-objs += \
+   ipu3-mmu.o ipu3-dmamap.o \
+   ipu3-tables.o ipu3-css-pool.o \
+   ipu3-css-fw.o ipu3-css-params.o \
+   ipu3-css.o ipu3-v4l2.o ipu3.o
+
+obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o
diff --git a/drivers/media/pci/intel/ipu3/ipu3.c 
b/drivers/media/pci/intel/ipu3/ipu3.c
new file mode 100644
index 000..eda7299
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/ipu3.c
@@ -0,0 +1,844 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 Intel Corporation
+ * Copyright 2017 Google LLC
+ *
+ * Based on Intel IPU4 driver.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include "ipu3.h"
+#include "ipu3-dmamap.h"
+#include "ipu3-mmu.h"
+
+#define IMGU_PCI_ID0x1919
+#define IMGU_PCI_BAR   0
+#define IMGU_DMA_MASK  DMA_BIT_MASK(39)
+#define IMGU_MAX_QUEUE_DEPTH   (2 + 2)
+
+/*
+ * pre-allocated buffer size for IMGU dummy buffers. Those
+ * values should be tuned to big enough to avoid buffer
+ * re-allocation when streaming to lower streaming latency.
+ */
+#define CSS_QUEUE_IN_BUF_SIZE  0
+#define CSS_QUEUE_PARAMS_BUF_SIZE  0
+#define CSS_QUEUE_OUT_BUF_SIZE (4160 * 3120 * 12 / 8)
+#define CSS_QUEUE_VF_BUF_SIZE  (1920 * 1080 * 12 / 8)
+#define CSS_QUEUE_STAT_3A_BUF_SIZE 125664
+
+static const size_t css_queue_buf_size_map[IPU3_CSS_QUEUES] = {
+   [IPU3_CSS_QUEUE_IN] = CSS_QUEUE_IN_BUF_SIZE,
+   [IPU3_CSS_QUEUE_PARAMS] = CSS_QUEUE_PARAMS_BUF_SIZE,
+   [IPU3_CSS_QUEUE_OUT] = CSS_QUEUE_OUT_BUF_SIZE,
+   [IPU3_CSS_QUEUE_VF] = CSS_QUEUE_VF_BUF_SIZE,
+   [IPU3_CSS_QUEUE_STAT_3A] = CSS_QUEUE_STAT_3A_BUF_SIZE,
+};
+
+static const struct imgu_node_mapping imgu_node_map[IMGU_NODE_NUM] = {
+   [IMGU_NODE_IN] = {IPU3_CSS_QUEUE_IN, "input"},
+   [IMGU_NODE_PARAMS] = {IPU3_CSS_QUEUE_PARAMS, "parameters"},
+   [IMGU_NODE_OUT] = {IPU3_CSS_QUEUE_OUT, "output"},
+   [IMGU_NODE_VF] = {IPU3_CSS_QUEUE_VF, "viewfinder"},
+   [IMGU_NODE_PV] = {IPU3_CSS_QUEUE_VF, "postview"},
+   [IMGU_NODE_STAT_3A] = {IPU3_CSS_QUEUE_STAT_3A, "3a stat"},
+};
+
+unsigned int imgu_node_to_queue(unsigned int node)
+{
+   return imgu_node_map[node].css_queue;
+}
+
+unsigned int imgu_map_node(struct imgu_device *imgu, unsigned int css_queue)
+{
+   unsigned int i;
+
+   if (css_queue == IPU3_CSS_QUEUE_VF)
+   return imgu->nodes[IMGU_NODE_VF].enabled ?
+   IMGU_NODE_VF : IMGU_NODE_PV;
+
+   for (i = 0; i < IMGU_NODE_NUM; i++)
+   if (imgu_node_map[i].css_queue == css_queue)
+   break;
+
+   return i;
+}
+
+/ Dummy buffers /
+
+static void imgu_dummybufs_cleanup(struct imgu_device *imgu)
+{
+   unsigned int i;
+
+   for (i = 0; i < IPU3_CSS_QUEUES; i++)
+   ipu3_dmamap_free(imgu, &imgu->queues[i].dmap);
+}
+
+static int imgu_dummybu