Hi, Sakari, Thanks again for the code review. > -----Original Message----- > From: Sakari Ailus [mailto:sakari.ailus@xxxxxxxxxxxxxxx] > Sent: Friday, November 9, 2018 6:54 AM > To: Zhi, Yong <yong.zhi@xxxxxxxxx> > Cc: linux-media@xxxxxxxxxxxxxxx; tfiga@xxxxxxxxxxxx; > mchehab@xxxxxxxxxx; hans.verkuil@xxxxxxxxx; > laurent.pinchart@xxxxxxxxxxxxxxxx; Mani, Rajmohan > <rajmohan.mani@xxxxxxxxx>; Zheng, Jian Xu <jian.xu.zheng@xxxxxxxxx>; Hu, > Jerry W <jerry.w.hu@xxxxxxxxx>; Toivonen, Tuukka > <tuukka.toivonen@xxxxxxxxx>; Qiu, Tian Shu <tian.shu.qiu@xxxxxxxxx>; Cao, > Bingbu <bingbu.cao@xxxxxxxxx> > 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 <yong.zhi@xxxxxxxxx> > > Signed-off-by: Tomasz Figa <tfiga@xxxxxxxxxxxx> > > --- > > 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 0000000..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 <linux/delay.h> > > +#include <linux/interrupt.h> > > +#include <linux/module.h> > > +#include <linux/pm_runtime.h> > > + > > +#include "ipu3.h" > > +#include "ipu3-dmamap.h" > > +#include "ipu3-mmu.h" > > + > > +#define IMGU_PCI_ID 0x1919 > > +#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 > > 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); > Ack. > ... > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3.h > > b/drivers/media/pci/intel/ipu3/ipu3.h > > new file mode 100644 > > index 0000000..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 <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 > > + > > +#define file_to_intel_ipu3_node(__file) \ > > + container_of(video_devdata(__file), struct imgu_video_device, vdev) > > + > > +#define IPU3_INPUT_MIN_WIDTH 0U > > +#define IPU3_INPUT_MIN_HEIGHT 0U > > +#define IPU3_INPUT_MAX_WIDTH 5120U > > +#define IPU3_INPUT_MAX_HEIGHT 38404U > > +#define IPU3_OUTPUT_MIN_WIDTH 2U > > +#define IPU3_OUTPUT_MIN_HEIGHT 2U > > +#define IPU3_OUTPUT_MAX_WIDTH 4480U > > +#define IPU3_OUTPUT_MAX_HEIGHT 34004U > > + > > +struct ipu3_vb2_buffer { > > + /* Public fields */ > > + struct vb2_v4l2_buffer vbb; /* Must be the first field */ > > + > > + /* Private fields */ > > + struct list_head list; > > +}; > > + > > +struct imgu_buffer { > > + struct ipu3_vb2_buffer vid_buf; /* Must be the first field */ > > + struct ipu3_css_buffer css_buf; > > + struct ipu3_css_map map; > > +}; > > + > > +struct imgu_node_mapping { > > + unsigned int css_queue; > > + const char *name; > > +}; > > + > > +/** > > + * struct imgu_video_device > > + * each node registers as video device and maintains its > > + * own vb2_queue. > > + */ > > +struct imgu_video_device { > > + const char *name; > > + bool output; /* Frames to the driver? */ > > + bool immutable; /* Can not be enabled/disabled */ > > + bool enabled; > > + int queued; /* Buffers already queued */ > > The queued field is unused. > Ack, thanks for catching this. Best regards, Yong > > + struct v4l2_format vdev_fmt; /* Currently set format */ > > + > > + /* Private fields */ > > + struct video_device vdev; > > + struct media_pad vdev_pad; > > + struct v4l2_mbus_framefmt pad_fmt; > > + struct vb2_queue vbq; > > + struct list_head buffers; > > + /* Protect vb2_queue and vdev structs*/ > > + struct mutex lock; > > + atomic_t sequence; > > +}; > > + > > -- > Kind regards, > > Sakari Ailus > sakari.ailus@xxxxxxxxxxxxxxx
