RE: [PATCH v7 01/16] v4l: Add Intel IPU3 meta buffer formats
Hi, Laurent,
Thanks for the review.
> -Original Message-
> From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com]
> Sent: Thursday, November 29, 2018 1:17 PM
> To: Zhi, Yong
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com;
> tf...@chromium.org; mche...@kernel.org; hans.verk...@cisco.com; Mani,
> Rajmohan ; Zheng, Jian Xu
> ; Hu, Jerry W ; Toivonen,
> Tuukka ; Qiu, Tian Shu
> ; Cao, Bingbu
> Subject: Re: [PATCH v7 01/16] v4l: Add Intel IPU3 meta buffer formats
>
> Hello Yong,
>
> Thank you for the patch.
>
> On Tuesday, 30 October 2018 00:22:55 EET Yong Zhi wrote:
> > Add IPU3-specific meta formats for parameter processing and 3A, DVS
> > statistics:
>
> Unless I'm mistaken DVS support has been removed. You can write this as
>
> Add IPU3-specific meta formats for processing parameters and 3A statistics.
>
Ack.
> >
> > V4L2_META_FMT_IPU3_PARAMS
> > V4L2_META_FMT_IPU3_STAT_3A
> >
> > Signed-off-by: Yong Zhi
> > ---
> > drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++
> > include/uapi/linux/videodev2.h | 4
> > 2 files changed, 6 insertions(+)
>
> I would squash this with patch 03/16.
>
OK, will squash patch 01 and 03 into single patch for v8.
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> > b/drivers/media/v4l2-core/v4l2-ioctl.c index 6489f25..abff64b 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1299,6 +1299,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc
> *fmt)
> > case V4L2_META_FMT_VSP1_HGO:descr = "R-Car VSP1 1-D Histogram";
> break;
> > case V4L2_META_FMT_VSP1_HGT:descr = "R-Car VSP1 2-D Histogram";
> break;
> > case V4L2_META_FMT_UVC: descr = "UVC payload header
> metadata"; break;
> > + case V4L2_META_FMT_IPU3_PARAMS: descr = "IPU3 processing
> parameters";
> > break;
> > + case V4L2_META_FMT_IPU3_STAT_3A:descr = "IPU3 3A statistics";
> break;
> >
> > default:
> > /* Compressed formats */
> > diff --git a/include/uapi/linux/videodev2.h
> > b/include/uapi/linux/videodev2.h index f0a968a..bdccd7a 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -718,6 +718,10 @@ struct v4l2_pix_format {
> > #define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* UVC
> > Payload Header metadata */ #define V4L2_META_FMT_D4XX
> > v4l2_fourcc('D', '4', 'X', 'X') /* D4XX Payload Header metadata */
> >
> > +/* Vendor specific - used for IPU3 camera sub-system */
> > +#define V4L2_META_FMT_IPU3_PARAMS v4l2_fourcc('i', 'p', '3', 'p') /*
> IPU3
> > params */
>
> Maybe "IPU3 processing parameters" in full ?
>
Ack, thanks!!
> Apart from that the patch looks good to me.
>
> Reviewed-by: Laurent Pinchart
>
> > +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 's') /*
> IPU3
> > 3A statistics */
> > +
> > /* priv field value to indicates that subsequent fields are valid. */
> > #define V4L2_PIX_FMT_PRIV_MAGIC0xfeedcafe
>
>
> --
> Regards,
>
> Laurent Pinchart
>
>
RE: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
Hi, Sakari,
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Thursday, November 29, 2018 4:46 PM
> 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 ; Li, Chao C
> Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
>
> Hi Yong,
>
> On Fri, Nov 16, 2018 at 10:37:00PM +, Zhi, Yong wrote:
> ...
> > > > +/**
> > > > + * struct ipu3_uapi_shd_grid_config - Bayer shading(darkening)
> > > > +correction
> > > > + *
> > > > + * @width: Grid horizontal dimensions, u8, [8, 128], default 73
> > > > + * @height:Grid vertical dimensions, u8, [8, 128], default 56
> > > > + * @block_width_log2: Log2 of the width of the grid cell in pixel
> > > count
> > > > + * u4, [0, 15], default value 5.
> > > > + * @__reserved0: reserved
> > > > + * @block_height_log2: Log2 of the height of the grid cell in pixel
> > > count
> > > > + * u4, [0, 15], default value 6.
> > > > + * @__reserved1: reserved
> > > > + * @grid_height_per_slice: SHD_MAX_CELLS_PER_SET/width.
> > > > + * (with SHD_MAX_CELLS_PER_SET =
> 146).
> > > > + * @x_start: X value of top left corner of sensor relative to ROI
> > > > + * u12, [-4096, 0]. default 0, only negative values.
> > > > + * @y_start: Y value of top left corner of sensor relative to ROI
> > > > + * u12, [-4096, 0]. default 0, only negative values.
> > >
> > > I suppose u12 is incorrect here, if the value is signed --- and
> > > negative (sign bit) if not 0?
> > >
> >
> > The value will be written to 13 bit register, should use s12.0.
>
> If you have s12, that means the most significant bit is the sign bit. So if
> the
> smallest value is -4096, you'd need s13.
>
> But where is the sign bit, i.e. is this either s13 or s16?
>
The notation of s12.0 means 13 bit with fraction bit as 0 right?
> >
> > > > + */
> > > > +struct ipu3_uapi_shd_grid_config {
> > > > + /* reg 0 */
> > > > + __u8 width;
> > > > + __u8 height;
> > > > + __u8 block_width_log2:3;
> > > > + __u8 __reserved0:1;
> > > > + __u8 block_height_log2:3;
> > > > + __u8 __reserved1:1;
> > > > + __u8 grid_height_per_slice;
> > > > + /* reg 1 */
> > > > + __s16 x_start;
> > > > + __s16 y_start;
> > > > +} __packed;
>
> ...
>
> > > > +/**
> > > > + * struct ipu3_uapi_iefd_cux2_1 - Calculate power of non-directed
> denoise
> > > > + * element apply.
> > > > + * @x0: X0 point of Config Unit, u9.0, default 0.
> > > > + * @x1: X1 point of Config Unit, u9.0, default 0.
> > > > + * @a01: Slope A of Config Unit, s4.4, default 0.
> > >
> > > The field is marked unsigned below. Which one is correct?
> > >
> >
> > They are both correct, however, s4.4 is the internal representation
> > used by CU, the inputs are unsigned, I will add a note in v8, same
> > applies to the few other places as you commented.
>
> I still find this rather confusing. Is there a sign bit or is there not?
>
It's unsigned number from driver perspective, all CU inputs are unsigned,
however, they will be "converted" to signed for FW/HW to use. I have to consult
FW expert if more clarification is needed.
> >
> > > > + * @__reserved1: reserved
> > > > + * @b01: offset B0 of Config Unit, u7.0, default 0.
> > > > + * @__reserved2: reserved
> > > > + */
> > > > +struct ipu3_uapi_iefd_cux2_1 {
> > > > + __u32 x0:9;
> > > > + __u32 x1:9;
> > > > + __u32 a01:9;
> > > > + __u32 __reserved1:5;
> > > > +
> > > > + __u32 b01:8;
> > > > + __u32 __reserved2:24;
> > > > +} __packed;
> > > > +
> > > > +/**
> > > > + * struct ipu3_uapi_iefd_cux4 - Calculate power of non-directed
> > > sharpening
> > > > + * element.
> > > > + *
> > &g
RE: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
Hi, Sakari,
Thanks for the thorough review.
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Friday, November 2, 2018 8:03 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 ; Li, Chao C
> Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
>
> Hi Yong,
>
> Thanks for the update! I went through this again... a few comments below
> but I'd say they're mostly pretty minor issues.
>
> On Mon, Oct 29, 2018 at 03:22:57PM -0700, Yong Zhi wrote:
> > These meta formats are used on Intel IPU3 ImgU video queues
> > to carry 3A statistics and ISP pipeline parameters.
> >
> > V4L2_META_FMT_IPU3_3A
> > V4L2_META_FMT_IPU3_PARAMS
> >
> > Signed-off-by: Yong Zhi
> > Signed-off-by: Chao C Li
> > Signed-off-by: Rajmohan Mani
> > ---
> > Documentation/media/uapi/v4l/meta-formats.rst |1 +
> > .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst | 181 ++
> > include/uapi/linux/intel-ipu3.h| 2819
> >
> > 3 files changed, 3001 insertions(+)
> > create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-intel-
> ipu3.rst
> > create mode 100644 include/uapi/linux/intel-ipu3.h
> >
> > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
> b/Documentation/media/uapi/v4l/meta-formats.rst
> > index cf971d5..eafc534 100644
> > --- a/Documentation/media/uapi/v4l/meta-formats.rst
> > +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> > @@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata`
> interface only.
> > .. toctree::
> > :maxdepth: 1
> >
> > +pixfmt-meta-intel-ipu3
> > pixfmt-meta-d4xx
> > pixfmt-meta-uvc
> > pixfmt-meta-vsp1-hgo
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > new file mode 100644
> > index 000..23b945b
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > @@ -0,0 +1,181 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _intel-ipu3:
>
> Instead, to avoid a warning from Sphinx, replace the line with these:
>
> .. _v4l2-meta-fmt-ipu3-params:
> .. _v4l2-meta-fmt-ipu3-stat-3a:
>
Ack.
> > +
> >
> +***
> ***
> > +V4L2_META_FMT_IPU3_PARAMS ('ip3p'), V4L2_META_FMT_IPU3_3A
> ('ip3s')
> >
> +***
> ***
> > +
> > +.. c:type:: ipu3_uapi_stats_3a
> > +
> > +3A statistics
> > +=
> > +
> > +For IPU3 ImgU, the 3A statistics accelerators collect different statistics
> > over
> > +an input bayer frame. Those statistics, defined in data struct
> > +:c:type:`ipu3_uapi_stats_3a`, are meta output obtained from "ipu3-imgu
> 3a stat"
> > +video node, which are then passed to user space for statistics analysis
> > +using :c:type:`v4l2_meta_format` interface.
> > +
> > +The statistics collected are AWB (Auto-white balance) RGBS (Red, Green,
> Blue and
>
> Extra whitespace at the end of the line.
>
Ack.
> > +Saturation measure) cells, AWB filter response, AF (Auto-focus) filter
> response,
> > +and AE (Auto-exposure) histogram.
> > +
> > +struct :c:type:`ipu3_uapi_4a_config` saves configurable parameters for all
> above.
> > +
> > +
> > +.. code-block:: c
> > +
> > +
> > + struct ipu3_uapi_stats_3a {
> > + struct ipu3_uapi_awb_raw_buffer awb_raw_buffer
> > +__attribute__((aligned(32)));
> > + struct ipu3_uapi_ae_raw_buffer_aligned
> > + ae_raw_buffer[IPU3_UAPI_MAX_STRIPES];
> > + struct ipu3_uapi_af_raw_buffer af_raw_buffer;
> > + struct ipu3_uapi_awb_fr_raw_buffer awb_fr_raw_buffer;
> > + struct ipu3_uapi_4a_config stats_4a_config;
> > + __u32 ae_join_buffers;
> > + __u8 padding[28];
> > + struct ipu3_uapi_stats_3a_bubble_info_per_stripe
> > + stats_3a_bubble_per_stripe;
>
> I think you could just unwrap these, even if it causes them to be over 80
> characters per line. They display better in a web browser that way. Or
> alternatively align the wrapped lines to the same column.
>
Ack.
> > + struct ipu3_u
RE: [PATCH v7 14/16] intel-ipu3: Add v4l2 driver based on media framework
Hi, Hans,
Thanks for the review.
> -Original Message-
> From: Hans Verkuil [mailto:hverk...@xs4all.nl]
> Sent: Thursday, November 15, 2018 6:51 AM
> To: Zhi, Yong ; linux-media@vger.kernel.org;
> sakari.ai...@linux.intel.com
> Cc: 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 14/16] intel-ipu3: Add v4l2 driver based on media
> framework
>
> On 10/29/18 23:23, Yong Zhi wrote:
> > Implement video driver that utilizes v4l2, vb2 queue support and media
> > controller APIs. The driver exposes single subdevice and six nodes.
> >
> > Signed-off-by: Yong Zhi
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-v4l2.c | 1091
> > ++
> > 1 file changed, 1091 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-v4l2.c
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-v4l2.c
> > b/drivers/media/pci/intel/ipu3/ipu3-v4l2.c
>
>
>
> > +int ipu3_v4l2_register(struct imgu_device *imgu) {
>
>
>
> > + /* Initialize vbq */
> > + vbq->type = node->vdev_fmt.type;
> > + vbq->io_modes = VB2_USERPTR | VB2_MMAP |
> VB2_DMABUF;
>
> Are you sure USERPTR works? If you have alignment requirements that the
> buffer starts at a multiple of more than (I think) 8 bytes, then USERPTR won't
> work.
USRPTR was used at the beginning of project, we then switched to dma buffer
mainly for performance reason:
https://chromium-review.googlesource.com/c/chromiumos/platform/arc-camera/+/620252
>
> > + vbq->ops = _vb2_ops;
> > + vbq->mem_ops = _dma_sg_memops;
> > + if (imgu->buf_struct_size <= 0)
> > + imgu->buf_struct_size = sizeof(struct
> ipu3_vb2_buffer);
> > + vbq->buf_struct_size = imgu->buf_struct_size;
> > + vbq->timestamp_flags =
> V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > + vbq->min_buffers_needed = 0;/* Can streamon w/o buffers
> */
> > + vbq->drv_priv = imgu;
> > + vbq->lock = >lock;
> > + r = vb2_queue_init(vbq);
> > + if (r) {
> > + dev_err(>pci_dev->dev,
> > + "failed to initialize video queue (%d)\n", r);
> > + goto fail_vdev;
> > + }
> > +
> > + /* Initialize vdev */
> > + snprintf(vdev->name, sizeof(vdev->name), "%s %s",
> > +IMGU_NAME, node->name);
> > + vdev->release = video_device_release_empty;
> > + vdev->fops = _v4l2_fops;
> > + vdev->lock = >lock;
> > + vdev->v4l2_dev = >v4l2_dev;
> > + vdev->queue = >vbq;
> > + vdev->vfl_dir = node->output ? VFL_DIR_TX : VFL_DIR_RX;
> > + video_set_drvdata(vdev, imgu);
> > + r = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> > + if (r) {
> > + dev_err(>pci_dev->dev,
> > + "failed to register video device (%d)\n", r);
> > + goto fail_vdev;
> > + }
> > +
> > + /* Create link between video node and the subdev pad */
> > + flags = 0;
> > + if (node->enabled)
> > + flags |= MEDIA_LNK_FL_ENABLED;
> > + if (node->immutable)
> > + flags |= MEDIA_LNK_FL_IMMUTABLE;
> > + if (node->output) {
> > + r = media_create_pad_link(>entity, 0,
> > + >subdev.entity,
> > +i, flags);
> > + } else {
> > + r = media_create_pad_link(>subdev.entity,
> > + i, >entity, 0, flags);
> > + }
> > + if (r)
> > + goto fail_link;
> > + }
> > +
> > + r = media_device_register(>media_dev);
> > + if (r) {
> > + dev_err(>pci_dev->dev,
> > + "failed to register media device (%d)\n", r);
> > + i--;
> > + goto fail_link;
> > + }
> > +
> > + return 0;
> > +
> > + for (; i >
RE: [PATCH v7 15/16] intel-ipu3: Add imgu top level pci device driver
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 14/16] intel-ipu3: Add v4l2 driver based on media framework
Hi, Sakari,
Thanks for the feedback.
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Friday, November 9, 2018 6:37 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 14/16] intel-ipu3: Add v4l2 driver based on media
> framework
>
> Hi Yong,
>
> On Mon, Oct 29, 2018 at 03:23:08PM -0700, Yong Zhi wrote:
> > Implement video driver that utilizes v4l2, vb2 queue support and media
> > controller APIs. The driver exposes single subdevice and six nodes.
> >
> > Signed-off-by: Yong Zhi
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-v4l2.c | 1091
> > ++
> > 1 file changed, 1091 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-v4l2.c
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-v4l2.c
> > b/drivers/media/pci/intel/ipu3/ipu3-v4l2.c
> > new file mode 100644
> > index 000..31a3514
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-v4l2.c
> > @@ -0,0 +1,1091 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2018 Intel Corporation
> > +
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +#include "ipu3.h"
> > +#include "ipu3-dmamap.h"
> > +
> > +/ v4l2_subdev_ops /
> > +
> > +static int ipu3_subdev_open(struct v4l2_subdev *sd, struct
> > +v4l2_subdev_fh *fh) {
> > + struct imgu_device *imgu = container_of(sd, struct imgu_device,
> subdev);
> > + struct v4l2_rect try_crop = {
> > + .top = 0,
> > + .left = 0,
> > + .height = imgu-
> >nodes[IMGU_NODE_IN].vdev_fmt.fmt.pix_mp.height,
> > + .width = imgu-
> >nodes[IMGU_NODE_IN].vdev_fmt.fmt.pix_mp.width,
> > + };
> > + unsigned int i;
> > +
> > + /* Initialize try_fmt */
> > + for (i = 0; i < IMGU_NODE_NUM; i++)
> > + *v4l2_subdev_get_try_format(sd, fh->pad, i) =
> > + imgu->nodes[i].pad_fmt;
>
> The try formats should reflect the defaults, not the current device state.
>
Ack, will fix in next update.
> > +
> > + *v4l2_subdev_get_try_crop(sd, fh->pad, IMGU_NODE_IN) = try_crop;
>
> Same for the crop. How about the compose rectangle?
>
Ok, will check both crop and compose rectangle.
> > +
> > + return 0;
> > +}
>
> ...
>
> > +int ipu3_v4l2_register(struct imgu_device *imgu) {
> > + struct v4l2_mbus_framefmt def_bus_fmt = { 0 };
> > + struct v4l2_pix_format_mplane def_pix_fmt = { 0 };
> > +
>
> Extra newline.
Ack.
>
> > + int i, r;
> > +
> > + /* Initialize miscellaneous variables */
> > + imgu->streaming = false;
> > +
> > + /* Init media device */
> > + media_device_pci_init(>media_dev, imgu->pci_dev,
> IMGU_NAME);
> > +
> > + /* Set up v4l2 device */
> > + imgu->v4l2_dev.mdev = >media_dev;
> > + imgu->v4l2_dev.ctrl_handler = imgu->ctrl_handler;
> > + r = v4l2_device_register(>pci_dev->dev, >v4l2_dev);
> > + if (r) {
> > + dev_err(>pci_dev->dev,
> > + "failed to register V4L2 device (%d)\n", r);
> > + goto fail_v4l2_dev;
> > + }
> > +
> > + /* Initialize subdev media entity */
> > + imgu->subdev_pads = kzalloc(sizeof(*imgu->subdev_pads) *
> > + IMGU_NODE_NUM, GFP_KERNEL);
>
> As the number of pads is static, could you instead put the array directly to
> the struct, instead of using a pointer? Remember to remove to
> corresponding kfree, too.
Good point, kzalloc does not serve its purpose here.
>
> > + if (!imgu->subdev_pads) {
> > + r = -ENOMEM;
> > + goto fail_subdev_pads;
> > + }
> > + r = media_entity_pads_init(>subdev.entity,
> IMGU_NODE_NUM,
> > + imgu->subdev_pads);
> > + if (r) {
> > + dev_err(>pci_dev->dev,
> > + "failed initialize subdev media entity (%d)\n", r);
> > + goto fail_media_entity;
> > + }
> > + imgu->subdev.entity.ops = _media_ops;
> > + for (i = 0; i < IMGU_NODE_NUM; i++) {
>
RE: [PATCH v7 08/16] intel-ipu3: css: Add dma buff pool utility functions
Hi, Sakari,
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Thursday, November 8, 2018 9:36 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 08/16] intel-ipu3: css: Add dma buff pool utility
> functions
>
> Hi Yong,
>
> On Mon, Oct 29, 2018 at 03:23:02PM -0700, Yong Zhi wrote:
> > The pools are used to store previous parameters set by user with the
> > parameter queue. Due to pipelining, there needs to be multiple sets
> > (up to four) of parameters which are queued in a host-to-sp queue.
> >
> > Signed-off-by: Yong Zhi
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-css-pool.c | 136
> > +++
> > drivers/media/pci/intel/ipu3/ipu3-css-pool.h | 56 +++
> > 2 files changed, 192 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > new file mode 100644
> > index 000..eab41c3
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > @@ -0,0 +1,136 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2018 Intel Corporation
> > +
> > +#include
> > +
> > +#include "ipu3.h"
> > +#include "ipu3-css-pool.h"
> > +#include "ipu3-dmamap.h"
> > +
> > +int ipu3_css_dma_buffer_resize(struct imgu_device *imgu,
> > + struct ipu3_css_map *map, size_t size) {
> > + if (map->size < size && map->vaddr) {
> > + dev_warn(>pci_dev->dev, "dma buf resized from %zu
> to %zu",
> > +map->size, size);
> > +
> > + ipu3_dmamap_free(imgu, map);
> > + if (!ipu3_dmamap_alloc(imgu, map, size))
> > + return -ENOMEM;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +void ipu3_css_pool_cleanup(struct imgu_device *imgu, struct
> > +ipu3_css_pool *pool) {
> > + unsigned int i;
> > +
> > + for (i = 0; i < IPU3_CSS_POOL_SIZE; i++)
> > + ipu3_dmamap_free(imgu, >entry[i].param); }
> > +
> > +int ipu3_css_pool_init(struct imgu_device *imgu, struct ipu3_css_pool
> *pool,
> > + size_t size)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < IPU3_CSS_POOL_SIZE; i++) {
> > + /*
> > +* entry[i].framenum is initialized to INT_MIN so that
> > +* ipu3_css_pool_check() can treat it as usesable slot.
> > +*/
> > + pool->entry[i].framenum = INT_MIN;
> > +
> > + if (size == 0) {
> > + pool->entry[i].param.vaddr = NULL;
> > + continue;
> > + }
> > +
> > + if (!ipu3_dmamap_alloc(imgu, >entry[i].param, size))
> > + goto fail;
> > + }
> > +
> > + pool->last = IPU3_CSS_POOL_SIZE;
> > +
> > + return 0;
> > +
> > +fail:
> > + ipu3_css_pool_cleanup(imgu, pool);
> > + return -ENOMEM;
> > +}
> > +
> > +/*
> > + * Check that the following call to pool_get succeeds.
> > + * Return negative on error.
> > + */
> > +static int ipu3_css_pool_check(struct ipu3_css_pool *pool, long
> > +framenum) {
> > + /* Get the oldest entry */
> > + int n = (pool->last + 1) % IPU3_CSS_POOL_SIZE;
> > + long diff = framenum - pool->entry[n].framenum;
> > +
> > + /* if framenum wraps around and becomes smaller than entry n */
> > + if (diff < 0)
> > + diff += LONG_MAX;
>
> Have you tested the wrap-around? As a result, the value of the diff is
> between -1 and LONG_MAX - 1 (without considering more than just the two
> lines above). Is that intended?
>
Yes, I simulated wrap-around using a smaller limit in v5.
> You seem to be using different types for the frame number; sometimes int,
> sometimes long. Could you align that, preferrably to an unsigned type? u32
> would probably be a sound choice.
>
Will use u32 at places except entry.framenum, which is initialized to INT_MIN.
This is because the frame i
RE: [PATCH v7 00/16] Intel IPU3 ImgU patchset
Hi, Sakari, Thanks for your review and comments. Bingbu has replied to some of your questions, so I will continue with the rest. > -Original Message- > From: Bing Bu Cao [mailto:bingbu@linux.intel.com] > Sent: Tuesday, November 6, 2018 10:17 PM > To: Sakari Ailus ; 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 00/16] Intel IPU3 ImgU patchset > > > On 11/01/2018 08:03 PM, Sakari Ailus wrote: > > Hi Yong, > > > > Thanks for the update! > > > > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote: > >> Hi, > >> > >> This series adds support for the Intel IPU3 (Image Processing Unit) > >> ImgU which is essentially a modern memory-to-memory ISP. It > >> implements raw Bayer to YUV image format conversion as well as a > >> large number of other pixel processing algorithms for improving the image > quality. > >> > >> Meta data formats are defined for image statistics (3A, i.e. > >> automatic white balance, exposure and focus, histogram and local area > >> contrast > >> enhancement) as well as for the pixel processing algorithm parameters. > >> The documentation for these formats is currently not included in the > >> patchset but will be added in a future version of this set. > >> > >> The algorithm parameters need to be considered specific to a given > >> frame and typically a large number of these parameters change on > >> frame to frame basis. Additionally, the parameters are highly > >> structured (and not a flat space of independent configuration > >> primitives). They also reflect the data structures used by the > >> firmware and the hardware. On top of that, the algorithms require > >> highly specialized user space to make meaningful use of them. For > >> these reasons it has been chosen video buffers to pass the parameters to > the device. > >> > >> On individual patches: > >> > >> The heart of ImgU is the CSS, or Camera Subsystem, which contains the > >> image processors and HW accelerators. > >> > >> The 3A statistics and other firmware parameter computation related > >> functions are implemented in patch 11. > >> > >> All IPU3 pipeline default settings can be found in patch 10. > >> > >> To access DDR via ImgU's own memory space, IPU3 is also equipped with > >> its own MMU unit, the driver is implemented in patch 6. > >> > >> Patch 7 uses above driver for DMA mapping operation. > >> > >> The communication between IPU3 firmware and driver is implemented > >> with circular queues in patch 8. > >> > >> Patch 9 provide some utility functions and manage IPU3 fw download > >> and install. > >> > >> The firmware which is called ipu3-fw.bin can be downloaded from: > >> > >> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware > >> .git (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f) > >> > >> Firmware ABI is defined in patches 4 and 5. > >> > >> Patches 12 and 13 are of the same file, the former contains all h/w > >> programming related code, the latter implements interface functions > >> for access fw & hw capabilities. > >> > >> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT > work: > >> > >> https://patchwork.kernel.org/patch/9976295/> > > I've pushed the latest set here: > > > > https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output> > > > > You can just say the entire set depends on those going forward; the > > documentation is needed, too. > > Ack. > >> Patch 15 represents the top level that glues all of the other > >> components together, passing arguments between the components. > >> > >> Patch 16 is a recent effort to extend v6 for advanced camera features > >> like Continuous View Finder (CVF) and Snapshot During Video(SDV) > support. > >> > >> Link to user space implementation: > >> > >> git clone > >> https://chromium.googlesource.com/chromiumos/platform/arc-camera > >> > >> ImgU media topology print: > >> > >> # media-ctl -d /dev/media0 -p > >> Media controller API version 4.19.0 > >> > >> Media d
RE: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
Hi, Mauro,
Thanks for your review.
> -Original Message-
> From: Mauro Carvalho Chehab [mailto:mchehab+sams...@kernel.org]
> Sent: Friday, November 2, 2018 6:49 AM
> To: Zhi, Yong
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com;
> tf...@chromium.org; hans.verk...@cisco.com;
> laurent.pinch...@ideasonboard.com; Mani, Rajmohan
> ; Zheng, Jian Xu ; Hu,
> Jerry W ; Toivonen, Tuukka
> ; Qiu, Tian Shu ; Cao,
> Bingbu ; Li, Chao C
> Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
>
> Em Mon, 29 Oct 2018 15:22:57 -0700
> Yong Zhi escreveu:
>
> > These meta formats are used on Intel IPU3 ImgU video queues
> > to carry 3A statistics and ISP pipeline parameters.
>
> Just minor things. See below.
>
> >
> > V4L2_META_FMT_IPU3_3A
> > V4L2_META_FMT_IPU3_PARAMS
> >
> > Signed-off-by: Yong Zhi
> > Signed-off-by: Chao C Li
> > Signed-off-by: Rajmohan Mani
> > ---
> > Documentation/media/uapi/v4l/meta-formats.rst |1 +
> > .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst | 181 ++
>
> I would actually prefer to have those two changes merged together with
> patch 1, as it makes easier for review.
>
> > include/uapi/linux/intel-ipu3.h| 2819
> >
>
> This one makes sense to have a separate patch.
>
Ack, will re-group the three files as suggested.
> > 3 files changed, 3001 insertions(+)
> > create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-intel-
> ipu3.rst
> > create mode 100644 include/uapi/linux/intel-ipu3.h
> >
> > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
> b/Documentation/media/uapi/v4l/meta-formats.rst
> > index cf971d5..eafc534 100644
> > --- a/Documentation/media/uapi/v4l/meta-formats.rst
> > +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> > @@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata`
> interface only.
> > .. toctree::
> > :maxdepth: 1
> >
> > +pixfmt-meta-intel-ipu3
> > pixfmt-meta-d4xx
> > pixfmt-meta-uvc
> > pixfmt-meta-vsp1-hgo
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > new file mode 100644
> > index 000..23b945b
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > @@ -0,0 +1,181 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _intel-ipu3:
> > +
> >
> +***
> ***
> > +V4L2_META_FMT_IPU3_PARAMS ('ip3p'), V4L2_META_FMT_IPU3_3A
> ('ip3s')
> >
> +***
> ***
> > +
> > +.. c:type:: ipu3_uapi_stats_3a
> > +
> > +3A statistics
> > +=
> > +
> > +For IPU3 ImgU, the 3A statistics accelerators collect different statistics
> > over
> > +an input bayer frame. Those statistics, defined in data struct
> > +:c:type:`ipu3_uapi_stats_3a`, are meta output obtained from "ipu3-imgu
> 3a stat"
> > +video node, which are then passed to user space for statistics analysis
> > +using :c:type:`v4l2_meta_format` interface.
> > +
> > +The statistics collected are AWB (Auto-white balance) RGBS (Red, Green,
> Blue and
> > +Saturation measure) cells, AWB filter response, AF (Auto-focus) filter
> response,
> > +and AE (Auto-exposure) histogram.
> > +
> > +struct :c:type:`ipu3_uapi_4a_config` saves configurable parameters for all
> above.
> > +
> > +
> > +.. code-block:: c
> > +
> > +
> > + struct ipu3_uapi_stats_3a {
> > + struct ipu3_uapi_awb_raw_buffer awb_raw_buffer
> > +__attribute__((aligned(32)));
> > + struct ipu3_uapi_ae_raw_buffer_aligned
> > + ae_raw_buffer[IPU3_UAPI_MAX_STRIPES];
> > + struct ipu3_uapi_af_raw_buffer af_raw_buffer;
> > + struct ipu3_uapi_awb_fr_raw_buffer awb_fr_raw_buffer;
> > + struct ipu3_uapi_4a_config stats_4a_config;
> > + __u32 ae_join_buffers;
> > + __u8 padding[28];
> > + struct ipu3_uapi_stats_3a_bubble_info_per_stripe
> > + stats_3a_bubble_per_stripe;
> > + struct ipu3_uapi_ff_status stats_3a_status;
> > + } __packed;
> > +
> > +
> > +.. c:type:: ipu3_uapi_params
> > +
> > +Pipeline parameters
> > +===
> > +
> > +IPU3 pip
RE: [PATCH v7 06/16] intel-ipu3: mmu: Implement driver
Hi, Sakari,
Thanks for the feedback.
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Monday, November 5, 2018 3:55 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 06/16] intel-ipu3: mmu: Implement driver
>
> Hi Yong,
>
> On Mon, Oct 29, 2018 at 03:23:00PM -0700, Yong Zhi wrote:
> > From: Tomasz Figa
> >
> > This driver translates IO virtual address to physical address based on
> > two levels page tables.
> >
> > Signed-off-by: Tomasz Figa
> > Signed-off-by: Yong Zhi
> > ---
>
> ...
>
> > +static void call_if_ipu3_is_powered(struct ipu3_mmu *mmu,
> > + void (*func)(struct ipu3_mmu *mmu)) {
> > + pm_runtime_get_noresume(mmu->dev);
> > + if (pm_runtime_active(mmu->dev))
> > + func(mmu);
> > + pm_runtime_put(mmu->dev);
>
> How about:
>
> if (!pm_runtime_get_if_in_use(mmu->dev))
> return;
>
> func(mmu);
> pm_runtime_put(mmu->dev);
>
Ack, unless Tomasz has different opinion.
>
> > +}
>
> --
> Sakari Ailus
> sakari.ai...@linux.intel.com
RE: [PATCH v6 12/12] intel-ipu3: Add imgu top level pci device driver
Hi, Tomasz,
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Tomasz Figa
> Sent: Tuesday, September 18, 2018 10:23 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 12/12] intel-ipu3: Add imgu top level pci device
> driver
>
> On Mon, Sep 17, 2018 at 5:20 AM Zhi, Yong wrote:
> >
> > Hi, Tomasz,
> >
> > Thanks for the code review.
> >
> > > -Original Message-
> > > From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> > > ow...@vger.kernel.org] On Behalf Of Tomasz Figa
> > > Sent: Monday, July 2, 2018 3:08 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 12/12] intel-ipu3: Add imgu top level pci
> > > device driver
> > >
> > > Hi Yong,
> > >
> > > On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi
> wrote:
> > > > +/*
> > > > + * Queue as many buffers to CSS as possible. If all buffers don't
> > > > +fit into
> > > > + * CSS buffer queues, they remain unqueued and will be queued later.
> > > > + */
> > > > +int imgu_queue_buffers(struct imgu_device *imgu, bool initial) {
> > > > + unsigned int node;
> > > > + int r = 0;
> > > > + struct imgu_buffer *ibuf;
> > > > +
> > > > + if (!ipu3_css_is_streaming(>css))
> > > > + return 0;
> > > > +
> > > > + mutex_lock(>lock);
> > > > +
> > > > + /* Buffer set is queued to FW only when input buffer is ready */
> > > > + if (!imgu_queue_getbuf(imgu, IMGU_NODE_IN)) {
> > > > + mutex_unlock(>lock);
> > > > + return 0;
> > > > + }
> > > > + for (node = IMGU_NODE_IN + 1; 1; node = (node + 1) %
> > > > + IMGU_NODE_NUM) {
> > >
> > > Shouldn't we make (node != IMGU_NODE_IN ||
> imgu_queue_getbuf(imgu,
> > > IMGU_NODE_IN)) the condition here, rather than 1?
> > >
> > > This would also let us remove the explicit call to
> > > imgu_queue_getbuf() above the loop.
> > >
> >
> > Ack, will make the suggested changes regarding the loop condition
> evaluation.
>
> Just to make sure, the suggestion also includes starting from
> IMGU_NODE_IN (not + 1), i.e.
>
> for (node = IMGU_NODE_IN;
> node != IMGU_NODE_IN || imgu_queue_getbuf(imgu, IMGU_NODE_IN);
> node = (node + 1) % IMGU_NODE_NUM) {
> // ...
> }
>
Thanks for the clarification.
> > > > +static int __maybe_unused imgu_suspend(struct device *dev) {
> > > > + struct pci_dev *pci_dev = to_pci_dev(dev);
> > > > + struct imgu_device *imgu = pci_get_drvdata(pci_dev);
> > > > + unsigned long expire;
> > > > +
> > > > + dev_dbg(dev, "enter %s\n", __func__);
> > > > + imgu->suspend_in_stream = ipu3_css_is_streaming(
> >css);
> > > > + if (!imgu->suspend_in_stream)
> > > > + goto out;
> > > > + /* Block new buffers to be queued to CSS. */
> > > > + atomic_set(>qbuf_barrier, 1);
> > > > + /*
> > > > +* Wait for currently running irq handler to be done so that
> > > > +* no new buffers will be queued to fw later.
> > > > +*/
> > > > + synchronize_irq(pci_dev->irq);
> > > > + /* Wait until all buffers in CSS are done. */
> > > > + expire = jiffies + msecs_to_jiffies(1000);
> > > > + while (!ipu3_css_queue_empty(>css)) {
> > > > + if (time_is_before_jiffies(expire)) {
> > > > + dev_err(dev, "wait buffer drain timeout.\n");
> > > > + break;
> > > > + }
> > > > + }
> > >
> > > Uhm. We struggle to save some power by suspending the device only to
> > > end up with an ugly busy wait that could take even a second here.
> > > This doesn't make any sense.
> > >
> > > We had a working solution using a wait queue in previous revision [1].
> > > What happened to it?
> > >
> > > [1] https://chromium-
> > >
> review.googlesource.com/c/chromiumos/third_party/kernel/+/1029594/2
> > > /drivers/media/pci/intel/ipu3/ipu3.c#b913
> > > (see the left side)
> > >
> >
> > The code here was based on an old version of patch "ipu3-imgu: Avoid
> might sleep operations in suspend callback" at submission, so it did have
> buf_drain_wq, sorry for the confusion.
> >
>
> I guess that means that v7 is going to have the workqueue back? :)
>
Yes, that's the plan.
> Best regards,
> Tomasz
RE: [PATCH v6 06/12] intel-ipu3: css: Add support for firmware management
Hi, Sakari,
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Friday, September 21, 2018 6:52 AM
> To: Zhi, Yong
> Cc: Tomasz Figa ; Linux Media Mailing List me...@vger.kernel.org>; Mani, Rajmohan ;
> Toivonen, Tuukka ; Hu, Jerry W
> ; Zheng, Jian Xu
> Subject: Re: [PATCH v6 06/12] intel-ipu3: css: Add support for firmware
> management
>
> Hi Yong,
>
> On Wed, Sep 19, 2018 at 10:57:55PM +, Zhi, Yong wrote:
> ...
> > > > +struct imgu_abi_osys_frame_params {
> > > > + /* Output pins */
> > > > + __u32 enable;
> > > > + __u32 format; /* enum imgu_abi_osys_format */
> > > > + __u32 flip;
> > > > + __u32 mirror;
> > > > + __u32 tiling; /* enum imgu_abi_osys_tiling */
> > > > + __u32 width;
> > > > + __u32 height;
> > > > + __u32 stride;
> > > > + __u32 scaled;
> > > > +} __packed;
> > > [snip]
> > > > +/* Defect pixel correction */
> > > > +
> > > > +struct imgu_abi_dpc_config {
> > > > + __u8 __reserved[240832];
> > > > +} __packed;
> > >
> > > Do we need this structure? One could just add a reserved field in
> > > the parent structure. Also, just to confirm, is 240832 really the right
> value here?
> > > Where does it come from? Please create a macro for it, possibly
> > > further breaking it down into the values used to compute this number.
> > >
> >
> > We can add a reserved field in the parent structure, the size is based
> > on original definition of dpc config which was removed since it's not
> > enabled/used.
>
> What's your plan with the DPC? If you don't plan to add it now, you could
> as well drop the configuration for that block. If there's a need to add it
> later
> on, you can still do it by defining a new struct for the buffer. Or simply
> adding it at the end of the existing struct while allowing the use of the old
> size without the DPC configuration.
>
> There would be a little extra work to do there by that time when DPC
> support would be added, but OTOH it seems silly to have quarter of a
> megabyte of extra stuff to pass around in a struct that's never used.
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ai...@linux.intel.com
It's a very good point, but as I was informed, there is no plan to update the
abi between the driver and firmware, so the size of imgu_abi_acc_param has not
changed since v1 to maintain the compatibility.
RE: [PATCH v6 06/12] intel-ipu3: css: Add support for firmware management
Hi, Tomasz,
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Tomasz Figa
> Sent: Monday, July 2, 2018 2:05 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 06/12] intel-ipu3: css: Add support for firmware
> management
>
> Hi Yong,
>
> Continuing my review. Sorry for the delay.
>
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi wrote:
> >
> > Introduce functions to load and install ImgU FW blobs.
> >
> > Signed-off-by: Yong Zhi
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-abi.h| 1888
>
> > drivers/media/pci/intel/ipu3/ipu3-css-fw.c | 261
> > drivers/media/pci/intel/ipu3/ipu3-css-fw.h | 198 +++
> > 3 files changed, 2347 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-abi.h
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.c
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.h
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-abi.h
> > b/drivers/media/pci/intel/ipu3/ipu3-abi.h
> > new file mode 100644
> > index ..24102647a89e
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-abi.h
> [snip]
> > +/* SYSTEM_REQ_0_5_0_IMGHMMADR */
> > +#define IMGU_REG_SYSTEM_REQ0x18
> > +#define IMGU_SYSTEM_REQ_FREQ_MASK 0x3f
> > +#define IMGU_SYSTEM_REQ_FREQ_DIVIDER 25
> > +#define IMGU_REG_INT_STATUS0x30
> > +#define IMGU_REG_INT_ENABLE0x34
> > +#define IMGU_REG_INT_CSS_IRQ (1 << 31)
>
> BIT(31)
>
Ack.
> [snip]
> > + IMGU_ABI_FRAME_FORMAT_CSI_MIPI_LEGACY_YUV420_8, /*
> Legacy YUV420.
> > +* UY odd line;
> > +* VY even line
> > +*/
> > + IMGU_ABI_FRAME_FORMAT_CSI_MIPI_YUV420_10,/* 10 bit per
> Y/U/V. Y odd
> > + * line; UYVY interleaved
> > + * even line
> > + */
> > + IMGU_ABI_FRAME_FORMAT_YCgCo444_16, /* Internal format for
> > + ISP2.7,
>
> Macros and enums should be uppercase.
>
Ack.
> [snip]
> > +struct imgu_abi_shd_intra_frame_operations_data {
> > + struct imgu_abi_acc_operation
> > + operation_list[IMGU_ABI_SHD_MAX_OPERATIONS]
> IPU3_ALIGN;
> > + struct imgu_abi_acc_process_lines_cmd_data
> > + process_lines_data[IMGU_ABI_SHD_MAX_PROCESS_LINES]
> IPU3_ALIGN;
> > + struct imgu_abi_shd_transfer_luts_set_data
> > + transfer_data[IMGU_ABI_SHD_MAX_TRANSFERS] IPU3_ALIGN;
> > +} __packed;
> > +
> > +struct imgu_abi_shd_config {
> > + struct ipu3_uapi_shd_config_static shd IMGU_ABI_PAD;
> > + struct imgu_abi_shd_intra_frame_operations_data shd_ops
> IMGU_ABI_PAD;
> > + struct ipu3_uapi_shd_lut shd_lut IMGU_ABI_PAD;
>
> Definitions of both IPU3_ALIGN and IMGU_ABI_PAD seem to be equivalent.
> Could we remove one and use the other everywhere?
>
Agree, will remove IMGU_ABI_PAD.
> [snip]
> > +struct imgu_abi_osys_scaler_params {
> > + __u32 inp_buf_y_st_addr;
> > + __u32 inp_buf_y_line_stride;
> > + __u32 inp_buf_y_buffer_stride;
> > + __u32 inp_buf_u_st_addr;
> > + __u32 inp_buf_v_st_addr;
> > + __u32 inp_buf_uv_line_stride;
> > + __u32 inp_buf_uv_buffer_stride;
> > + __u32 inp_buf_chunk_width;
> > + __u32 inp_buf_nr_buffers;
> > + /* Output buffers */
> > + __u32 out_buf_y_st_addr;
> > + __u32 out_buf_y_line_stride;
> > + __u32 out_buf_y_buffer_stride;
> > + __u32 out_buf_u_st_addr;
> > + __u32 out_buf_v_st_addr;
> > + __u32 out_buf_uv_line_stride;
> > + __u32 out_buf_uv_buffer_stride;
> > + __u32 out_buf_nr_buffers;
> > + /* Intermediate buffers */
> > + __u32 int_buf_y_st_addr;
> > + __u32 int_buf_y_line_stride;
> > + __u32 int_buf_u_st_addr;
> > + __u32 int_buf_v_st_addr;
> > + __u32 int_buf_uv_line_stride;
> > + __u32 int_buf_height;
> > + __u32 int_buf_chunk_width;
> &
RE: [PATCH v6 12/12] intel-ipu3: Add imgu top level pci device driver
Hi, Tomasz,
Thanks for the code review.
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Tomasz Figa
> Sent: Monday, July 2, 2018 3:08 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 12/12] intel-ipu3: Add imgu top level pci device
> driver
>
> Hi Yong,
>
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi wrote:
> > +/*
> > + * Queue as many buffers to CSS as possible. If all buffers don't fit
> > +into
> > + * CSS buffer queues, they remain unqueued and will be queued later.
> > + */
> > +int imgu_queue_buffers(struct imgu_device *imgu, bool initial) {
> > + unsigned int node;
> > + int r = 0;
> > + struct imgu_buffer *ibuf;
> > +
> > + if (!ipu3_css_is_streaming(>css))
> > + return 0;
> > +
> > + mutex_lock(>lock);
> > +
> > + /* Buffer set is queued to FW only when input buffer is ready */
> > + if (!imgu_queue_getbuf(imgu, IMGU_NODE_IN)) {
> > + mutex_unlock(>lock);
> > + return 0;
> > + }
> > + for (node = IMGU_NODE_IN + 1; 1; node = (node + 1) %
> > + IMGU_NODE_NUM) {
>
> Shouldn't we make (node != IMGU_NODE_IN || imgu_queue_getbuf(imgu,
> IMGU_NODE_IN)) the condition here, rather than 1?
>
> This would also let us remove the explicit call to imgu_queue_getbuf()
> above the loop.
>
Ack, will make the suggested changes regarding the loop condition evaluation.
> > + if (node == IMGU_NODE_VF &&
> > + (imgu->css.pipe_id == IPU3_CSS_PIPE_ID_CAPTURE ||
> > +!imgu->nodes[IMGU_NODE_VF].enabled)) {
> > + continue;
> > + } else if (node == IMGU_NODE_PV &&
> > + (imgu->css.pipe_id == IPU3_CSS_PIPE_ID_VIDEO ||
> > + !imgu->nodes[IMGU_NODE_PV].enabled)) {
> > + continue;
> > + } else if (imgu->queue_enabled[node]) {
> > + struct ipu3_css_buffer *buf =
> > + imgu_queue_getbuf(imgu, node);
> > + int dummy;
> > +
> > + if (!buf)
> > + break;
> > +
> > + r = ipu3_css_buf_queue(>css, buf);
> > + if (r)
> > + break;
> > + dummy = imgu_dummybufs_check(imgu, buf);
> > + if (!dummy)
> > + ibuf = container_of(buf, struct imgu_buffer,
> > + css_buf);
> > + dev_dbg(>pci_dev->dev,
> > + "queue %s %s buffer %d to css da: 0x%08x\n",
> > + dummy ? "dummy" : "user",
> > + imgu_node_map[node].name,
> > + dummy ? 0 : ibuf->vid_buf.vbb.vb2_buf.index,
> > + (u32)buf->daddr);
> > + }
> > + if (node == IMGU_NODE_IN &&
> > + !imgu_queue_getbuf(imgu, IMGU_NODE_IN))
> > + break;
>
> My suggestion to the for loop condition is based on this.
>
Got it.
> > + }
> > + mutex_unlock(>lock);
> > +
> > + if (r && r != -EBUSY)
> > + goto failed;
> > +
> > + return 0;
> > +
> > +failed:
> > + /*
> > +* On error, mark all buffers as failed which are not
> > +* yet queued to CSS
> > +*/
> > + dev_err(>pci_dev->dev,
> > + "failed to queue buffer to CSS on queue %i (%d)\n",
> > + node, r);
> > +
> > + if (initial)
> > + /* If we were called from streamon(), no need to finish
> > bufs */
> > + return r;
> > +
> > + for (node = 0; node < IMGU_NODE_NUM; node++) {
> > + struct imgu_buffer *buf, *buf0;
> > +
> > + if (!imgu->queue_enabled[node])
> > + continue; /* Skip disabled queues */
> > +
> > + mutex_lock(>
RE: [PATCH v6 11/12] intel-ipu3: Add v4l2 driver based on media framework
Hi, Tomasz,
Sorry for the delay in responding to your review.
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Tomasz Figa
> Sent: Monday, July 2, 2018 2:50 AM
> To: Zhi, Yong
> Cc: Linux Media Mailing List ; Sakari Ailus
> ; Mani, Rajmohan
> ; Toivonen, Tuukka
> ; Hu, Jerry W ; Zheng,
> Jian Xu ; Vijaykumar, Ramya
>
> Subject: Re: [PATCH v6 11/12] intel-ipu3: Add v4l2 driver based on media
> framework
>
> Hi Yong,
>
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi wrote:
> [snip]
> > +static int ipu3_vidioc_enum_input(struct file *file, void *fh,
> > + struct v4l2_input *input) {
> > + if (input->index > 0)
> > + return -EINVAL;
> > + strlcpy(input->name, "camera", sizeof(input->name));
> > + input->type = V4L2_INPUT_TYPE_CAMERA;
> > +
> > + return 0;
> > +}
> > +
> > +static int ipu3_vidioc_g_input(struct file *file, void *fh, unsigned
> > +int *input) {
> > + *input = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int ipu3_vidioc_s_input(struct file *file, void *fh, unsigned
> > +int input) {
> > + return input == 0 ? 0 : -EINVAL; }
> > +
> > +static int ipu3_vidioc_enum_output(struct file *file, void *fh,
> > + struct v4l2_output *output) {
> > + if (output->index > 0)
> > + return -EINVAL;
> > + strlcpy(output->name, "camera", sizeof(output->name));
> > + output->type = V4L2_INPUT_TYPE_CAMERA;
> > +
> > + return 0;
> > +}
> > +
> > +static int ipu3_vidioc_g_output(struct file *file, void *fh,
> > + unsigned int *output) {
> > + *output = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int ipu3_vidioc_s_output(struct file *file, void *fh,
> > + unsigned int output) {
> > + return output == 0 ? 0 : -EINVAL; }
>
> Do we really need to implement the 6 functions above? They don't seem to
> be doing anything useful.
>
They are here to pass v4l2-compliance test. I can add a note in next update for
their purpose. We can remove them in the future when defaults callbacks are
available for those ops.
> [snip]
>
> > +int ipu3_v4l2_register(struct imgu_device *imgu) {
> > + struct v4l2_mbus_framefmt def_bus_fmt = { 0 };
> > + struct v4l2_pix_format_mplane def_pix_fmt = { 0 };
> > +
> > + int i, r;
> > +
> > + /* Initialize miscellaneous variables */
> > + imgu->streaming = false;
> > +
> > + /* Set up media device */
> > + imgu->media_dev.dev = >pci_dev->dev;
> > + strlcpy(imgu->media_dev.model, IMGU_NAME,
> > + sizeof(imgu->media_dev.model));
> > + snprintf(imgu->media_dev.bus_info, sizeof(imgu-
> >media_dev.bus_info),
> > +"%s", dev_name(>pci_dev->dev));
> > + imgu->media_dev.hw_revision = 0;
> > + media_device_init(>media_dev);
> > + r = media_device_register(>media_dev);
> > + if (r) {
> > + dev_err(>pci_dev->dev,
> > + "failed to register media device (%d)\n", r);
> > + return r;
> > + }
>
> Shouldn't we register the media device at the end, after all video nodes are
> registered below? Otherwise, since media_device_register() exposes the
> media node to userspace, we risk a race, when userspace opens the media
> device before all the entities are created and linked.
>
Make sense, will change the call order in v7.
> [snip]
>
> > +int ipu3_v4l2_unregister(struct imgu_device *imgu) {
> > + unsigned int i;
> > +
> > + for (i = 0; i < IMGU_NODE_NUM; i++) {
> > + video_unregister_device(>nodes[i].vdev);
> > + media_entity_cleanup(>nodes[i].vdev.entity);
> > + mutex_destroy(>nodes[i].lock);
> > + }
> > +
> > + v4l2_device_unregister_subdev(>subdev);
> > + media_entity_cleanup(>subdev.entity);
> > + kfree(imgu->subdev_pads);
> > + v4l2_device_unregister(>v4l2_dev);
> > + media_device_unregister(>media_dev);
>
> Should unregister media device at the beginning, so that it cannot be used
> when we continue to clean up the entities.
>
Agree, thanks for the review.
> > + media_device_cleanup(>media_dev);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(ipu3_v4l2_unregister);
>
> Best regards,
> Tomasz
RE: [PATCH v6 02/12] intel-ipu3: Add user space API definitions
Hi, Tomasz,
Thank you for the time spent to review this long file.
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Sunday, June 17, 2018 11:09 PM
> To: Zhi, Yong
> Cc: Linux Media Mailing List ; Sakari Ailus
> ; Mani, Rajmohan
> ; Toivonen, Tuukka
> ; Hu, Jerry W ; Zheng,
> Jian Xu
> Subject: Re: [PATCH v6 02/12] intel-ipu3: Add user space API definitions
>
> Hi Yong,
>
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi wrote:
> >
> > Define the structures and macros to be used by public.
> >
> > Signed-off-by: Yong Zhi
> > Signed-off-by: Rajmohan Mani
> > ---
> > include/uapi/linux/intel-ipu3.h | 1403
> > +++
> > 1 file changed, 1403 insertions(+)
> > create mode 100644 include/uapi/linux/intel-ipu3.h
> >
>
> Since we'll need 1 more resend with latest fixes from Chromium tree and
> recently posted documentation anyway, let me do some more nitpicking
> inline, so we can end up with slightly cleaner code. :)
>
> > diff --git a/include/uapi/linux/intel-ipu3.h
> > b/include/uapi/linux/intel-ipu3.h new file mode 100644 index
> > ..694ef0c8d7a7
> > --- /dev/null
> > +++ b/include/uapi/linux/intel-ipu3.h
> > @@ -0,0 +1,1403 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (C) 2018 Intel Corporation */
> > +
> > +#ifndef __IPU3_UAPI_H
> > +#define __IPU3_UAPI_H
> > +
> > +#include
> > +
> > +#define IPU3_UAPI_ISP_VEC_ELEMS64
> > +
> > +#define IMGU_ABI_PAD __aligned(IPU3_UAPI_ISP_WORD_BYTES)
>
> This seems unused.
Ack, will remove.
>
> > +#define IPU3_ALIGN
> __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES)))
>
> Any reason to mix both __aligned() and __attribute__((aligned()))?
>
> > +
> > +#define IPU3_UAPI_ISP_WORD_BYTES 32
>
> It would make sense to define this above IPU3_ALIGN(), which references it.
>
Sure.
> > +#define IPU3_UAPI_MAX_STRIPES 2
> > +
> > +/*** ipu3_uapi_stats_3a ***/
> > +
> > +#define IPU3_UAPI_MAX_BUBBLE_SIZE 10
> > +
> > +#define IPU3_UAPI_AE_COLORS4
> > +#define IPU3_UAPI_AE_BINS 256
> > +
> > +#define IPU3_UAPI_AWB_MD_ITEM_SIZE 8
> > +#define IPU3_UAPI_AWB_MAX_SETS 60
> > +#define IPU3_UAPI_AWB_SET_SIZE 0x500
>
> Why not just decimal 1280?
Ok, will change above and similar places to decimal expression.
>
> > +#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
> > + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> > +IPU3_UAPI_AWB_MD_ITEM_SIZE)
> > +#define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
> > + (IPU3_UAPI_AWB_MAX_SETS * \
> > +(IPU3_UAPI_AWB_SET_SIZE +
> IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
> > +
> > +#define IPU3_UAPI_AF_MAX_SETS 24
> > +#define IPU3_UAPI_AF_MD_ITEM_SIZE 4
> > +#define IPU3_UAPI_AF_SPARE_FOR_BUBBLES \
> > + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> > +IPU3_UAPI_AF_MD_ITEM_SIZE)
> > +#define IPU3_UAPI_AF_Y_TABLE_SET_SIZE 0x80
>
> Why not just decimal 128?
Ack.
>
> > +#define IPU3_UAPI_AF_Y_TABLE_MAX_SIZE \
> > + (IPU3_UAPI_AF_MAX_SETS * \
> > +(IPU3_UAPI_AF_Y_TABLE_SET_SIZE +
> IPU3_UAPI_AF_SPARE_FOR_BUBBLES) * \
> > +IPU3_UAPI_MAX_STRIPES)
> > +
> > +#define IPU3_UAPI_AWB_FR_MAX_SETS 24
> > +#define IPU3_UAPI_AWB_FR_MD_ITEM_SIZE 8
> > +#define IPU3_UAPI_AWB_FR_BAYER_TBL_SIZE0x100
>
> Why not just decimal 256?
Ack.
>
> > +#define IPU3_UAPI_AWB_FR_SPARE_FOR_BUBBLES \
> > + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> > +IPU3_UAPI_AWB_FR_MD_ITEM_SIZE) #define
> > +IPU3_UAPI_AWB_FR_BAYER_TABLE_MAX_SIZE \
> > + (IPU3_UAPI_AWB_FR_MAX_SETS * \
> > + (IPU3_UAPI_AWB_FR_BAYER_TBL_SIZE + \
> > +IPU3_UAPI_AWB_FR_SPARE_FOR_BUBBLES) *
> IPU3_UAPI_MAX_STRIPES)
> [snip]
> > +struct ipu3_uapi_af_filter_config {
> > + struct {
> > + __u8 a1;
> > + __u8 a2;
> > + __u8 a3;
> > + __u8 a4;
> > + } y1_coeff_0;
> > + struct {
> > + __u8 a5;
> > + __u8 a6;
> &g
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 03/12] intel-ipu3: mmu: Implement driver
Hi, Tomasz,
Thanks for the code review.
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Sunday, June 17, 2018 11:46 PM
> To: Zhi, Yong
> Cc: Linux Media Mailing List ; Sakari Ailus
> ; Mani, Rajmohan
> ; Toivonen, Tuukka
> ; Hu, Jerry W ; Zheng,
> Jian Xu
> Subject: Re: [PATCH v6 03/12] intel-ipu3: mmu: Implement driver
>
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi wrote:
> >
> > From: Tomasz Figa
> >
> > This driver translates IO virtual address to physical address based on
> > two levels page tables.
> >
> > Signed-off-by: Tomasz Figa
> > Signed-off-by: Yong Zhi
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-mmu.c | 560
> >
> > drivers/media/pci/intel/ipu3/ipu3-mmu.h | 28 ++
> > 2 files changed, 588 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-mmu.c
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-mmu.h
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-mmu.c
> > b/drivers/media/pci/intel/ipu3/ipu3-mmu.c
> > new file mode 100644
> > index ..a4b3e1680bbb
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-mmu.c
> > @@ -0,0 +1,560 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 Intel Corporation.
> > + * Copyright (C) 2018 Google, Inc.
>
> I followed wrong guide when adding this one. Could you fix it up to the
> following?
>
> Copyright 2018 Google LLC.
>
Sure, will do.
> [snip]
> > +/**
> > + * ipu3_mmu_exit() - clean up IPU3 MMU block
> > + * @mmu: IPU3 MMU private data
> > + */
> > +void ipu3_mmu_exit(struct ipu3_mmu_info *info) {
> > + struct ipu3_mmu *mmu = to_ipu3_mmu(info);
> > +
> > + /* We are going to free our page tables, no more memory access. */
> > + ipu3_mmu_set_halt(mmu, true);
> > + ipu3_mmu_tlb_invalidate(mmu);
> > +
> > + ipu3_mmu_free_page_table(mmu->l1pt);
> > + vfree(mmu->l2pts);
> > + ipu3_mmu_free_page_table(mmu->dummy_l2pt);
> > + kfree(mmu->dummy_page);
>
> Should be free_page(). (Might be already included in your tree as per
> https://chromium-
> review.googlesource.com/c/chromiumos/third_party/kernel/+/1084522)
>
Yes, will add above fix to next upstream version.
> > + kfree(mmu);
> > +}
> > +
> > +void ipu3_mmu_suspend(struct ipu3_mmu_info *info) {
> > + struct ipu3_mmu *mmu = to_ipu3_mmu(info);
> > +
> > + ipu3_mmu_set_halt(mmu, true);
> > +}
> > +
> > +void ipu3_mmu_resume(struct ipu3_mmu_info *info) {
> > + struct ipu3_mmu *mmu = to_ipu3_mmu(info);
> > + u32 pteval;
> > +
> > + ipu3_mmu_set_halt(mmu, true);
> > +
> > + pteval = IPU3_ADDR2PTE(virt_to_phys(mmu->l1pt));
> > + writel(pteval, mmu->base + REG_L1_PHYS);
> > +
> > + ipu3_mmu_tlb_invalidate(mmu);
> > + ipu3_mmu_set_halt(mmu, false); }
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-mmu.h
> > b/drivers/media/pci/intel/ipu3/ipu3-mmu.h
> > new file mode 100644
> > index ..4976187c18f6
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-mmu.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (C) 2018 Intel Corporation */
> > +/* Copyright (C) 2018 Google, Inc. */
> > +
> > +#ifndef __IPU3_MMU_H
> > +#define __IPU3_MMU_H
> > +
> > +struct ipu3_mmu_info {
> > + dma_addr_t aperture_start; /* First address that can be mapped
> */
> > + dma_addr_t aperture_end; /* Last address that can be mapped
> */
> > + unsigned long pgsize_bitmap;/* Bitmap of page sizes in use */
>
> If documenting the fields, why not use a kerneldoc comment above the
> struct instead?
>
Ack.
> Best regards,
> Tomasz
RE: [PATCH v6 12/12] intel-ipu3: Add imgu top level pci device driver
Hi, Tomasz,
Thanks for the review again.
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Thursday, April 26, 2018 12:15 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus
> <sakari.ai...@linux.intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>; Zheng,
> Jian Xu <jian.xu.zh...@intel.com>
> Subject: Re: [PATCH v6 12/12] intel-ipu3: Add imgu top level pci device
> driver
>
> Hi Yong,
>
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi <yong@intel.com> wrote:
> [snip]
> > +static int imgu_video_nodes_init(struct imgu_device *imgu) {
> > + struct v4l2_pix_format_mplane *fmts[IPU3_CSS_QUEUES] = { NULL };
> > + struct v4l2_rect *rects[IPU3_CSS_RECTS] = { NULL };
> > + unsigned int i;
> > + int r;
> > +
> > + imgu->buf_struct_size = sizeof(struct imgu_buffer);
> > +
> > + for (i = 0; i < IMGU_NODE_NUM; i++) {
> > + imgu->nodes[i].name = imgu_node_map[i].name;
> > + imgu->nodes[i].output = i < IMGU_QUEUE_FIRST_INPUT;
> > + imgu->nodes[i].immutable = false;
> > + imgu->nodes[i].enabled = false;
> > +
> > + if (i != IMGU_NODE_PARAMS && i != IMGU_NODE_STAT_3A)
> > + fmts[imgu_node_map[i].css_queue] =
> > + >nodes[i].vdev_fmt.fmt.pix_mp;
> > + atomic_set(>nodes[i].sequence, 0);
> > + }
> > +
> > + /* Master queue is always enabled */
> > + imgu->nodes[IMGU_QUEUE_MASTER].immutable = true;
> > + imgu->nodes[IMGU_QUEUE_MASTER].enabled = true;
> > +
> > + r = ipu3_v4l2_register(imgu);
> > + if (r)
> > + return r;
> > +
> > + /* Set initial formats and initialize formats of video nodes */
> > + rects[IPU3_CSS_RECT_EFFECTIVE] = >rect.eff;
> > + rects[IPU3_CSS_RECT_BDS] = >rect.bds;
> > + ipu3_css_fmt_set(>css, fmts, rects);
> > +
> > + /* Pre-allocate dummy buffers */
> > + r = imgu_dummybufs_preallocate(imgu);
> > + if (r) {
> > + dev_err(>pci_dev->dev,
> > + "failed to pre-allocate dummy buffers (%d)", r);
> > + imgu_dummybufs_cleanup(imgu);
>
> No need to call ipu3_v4l2_unregister() here?
>
> (That's why I keep suggesting use of single return error path with labels
> named after the first cleanup step that needs to be done, as it makes it
> easier to spot such mistakes.)
>
Good catch, suggestion taken :)
Maybe I should move the imgu_dummybufs_preallocate() out from
imgu_video_nodes_init().
> > + return r;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void imgu_video_nodes_exit(struct imgu_device *imgu) {
> > + imgu_dummybufs_cleanup(imgu);
> > + ipu3_v4l2_unregister(imgu);
> > +}
>
> Best regards,
> Tomasz
RE: [PATCH v6 10/12] intel-ipu3: Add css pipeline programming
Hi, Tomasz,
Thanks for the code review.
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Thursday, April 26, 2018 12:12 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus
> <sakari.ai...@linux.intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>; Zheng,
> Jian Xu <jian.xu.zh...@intel.com>
> Subject: Re: [PATCH v6 10/12] intel-ipu3: Add css pipeline programming
>
> Hi Yong,
>
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi <yong@intel.com> wrote:
> [snip]
> > +int ipu3_css_init(struct device *dev, struct ipu3_css *css,
> > + void __iomem *base, int length) {
> > + int r, p, q, i;
> > +
> > + /* Initialize main data structure */
> > + css->dev = dev;
> > + css->base = base;
> > + css->iomem_length = length;
> > + css->current_binary = IPU3_CSS_DEFAULT_BINARY;
> > + css->pipe_id = IPU3_CSS_PIPE_ID_NUM;
> > + css->vf_output_en = IPU3_NODE_VF_DISABLED;
> > + spin_lock_init(>qlock);
> > +
> > + for (q = 0; q < IPU3_CSS_QUEUES; q++) {
> > + r = ipu3_css_queue_init(>queue[q], NULL, 0);
> > + if (r)
> > + return r;
> > + }
> > +
> > + r = ipu3_css_fw_init(css);
> > + if (r)
> > + return r;
> > +
> > + /* Allocate and map common structures with imgu hardware */
> > +
> > + for (p = 0; p < IPU3_CSS_PIPE_ID_NUM; p++)
> > + for (i = 0; i < IMGU_ABI_MAX_STAGES; i++) {
> > + if (!ipu3_dmamap_alloc(dev,
> > +
> >xmem_sp_stage_ptrs[p][i],
> > + sizeof(struct
> imgu_abi_sp_stage)))
>
> checkpatch reports line over 80 characters here.
Ack, I opted for alignment over line limit here , will fix in v7.
>
> > + goto error_no_memory;
> > + if (!ipu3_dmamap_alloc(dev,
> > +
> >xmem_isp_stage_ptrs[p][i],
> > + sizeof(struct
> imgu_abi_isp_stage)))
>
> Ditto.
Sure, thanks!!
>
> > + goto error_no_memory;
> > + }
>
> Best regards,
> Tomasz
RE: [PATCH] media: intel-ipu3: cio2: Use SPDX license headers
Hi, Sakari, Sorry for the late response, somehow, I missed this email in my Inbox. > -Original Message- > From: linux-media-ow...@vger.kernel.org [mailto:linux-media- > ow...@vger.kernel.org] On Behalf Of Sakari Ailus > Sent: Wednesday, February 7, 2018 2:36 PM > To: Zhi, Yong <yong@intel.com> > Cc: linux-media@vger.kernel.org; Zheng, Jian Xu <jian.xu.zh...@intel.com>; > Mani, Rajmohan <rajmohan.m...@intel.com> > Subject: Re: [PATCH] media: intel-ipu3: cio2: Use SPDX license headers > > Hi Yong, > > Thanks for the patch. > > On Mon, Feb 05, 2018 at 08:19:53PM -0800, Yong Zhi wrote: > > Adopt SPDX license headers and update year to 2018. > > > > Signed-off-by: Yong Zhi <yong@intel.com> > > --- > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 12 ++-- > > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 14 ++ > > 2 files changed, 4 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > index 6cb..725973f 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > @@ -1,14 +1,6 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > /* > > - * Copyright (c) 2017 Intel Corporation. > > - * > > - * 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. > > + * Copyright (C) 2018 Intel Corporation > > * > > * Based partially on Intel IPU4 driver written by > > * Sakari Ailus <sakari.ai...@linux.intel.com> diff --git > > a/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > index 78a5799..6a11051 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > @@ -1,15 +1,5 @@ > > -/* > > - * Copyright (c) 2017 Intel Corporation. > > - * > > - * 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. > > - */ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* Copyright (C) 2018 Intel Corporation */ > > Should this be: > > /* Copyright (C) 2017 -- 2018 Intel Corporation */ > > ? > > Same for the one above. > Sure, will send an update. Thanks!! > > > > #ifndef __IPU3_CIO2_H > > #define __IPU3_CIO2_H > > -- > > 1.9.1 > > > > -- > Sakari Ailus > sakari.ai...@linux.intel.com
RE: [PATCH] media: intel-ipu3: cio2: Synchronize irqs at stop_streaming
Hi, Sakari, > -Original Message- > From: Sakari Ailus [mailto:sakari.ai...@iki.fi] > Sent: Wednesday, February 7, 2018 11:38 PM > To: Zhi, Yong <yong@intel.com> > Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; > tf...@chromium.org; Qiu, Tian Shu <tian.shu@intel.com>; Zheng, Jian > Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan > <rajmohan.m...@intel.com> > Subject: Re: [PATCH] media: intel-ipu3: cio2: Synchronize irqs at > stop_streaming > > Hi Yong, > > On Wed, Feb 07, 2018 at 02:47:50PM -0800, Yong Zhi wrote: > > This is to avoid pending interrupts to be handled during stream off, > > in which case, the ready buffer will be removed from buffer list, thus > > not all buffers can be returned to VB2 as expected. Disable CIO2 irq > > at cio2_hw_exit() so no new interrupts are generated. > > > > Signed-off-by: Yong Zhi <yong@intel.com> > > Signed-off-by: Tianshu Qiu <tian.shu@intel.com> > > --- > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > index 725973f..8d75146 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > @@ -518,6 +518,8 @@ static void cio2_hw_exit(struct cio2_device *cio2, > struct cio2_queue *q) > > unsigned int i, maxloops = 1000; > > > > /* Disable CSI receiver and MIPI backend devices */ > > + writel(0, q->csi_rx_base + CIO2_REG_IRQCTRL_MASK); > > + writel(0, q->csi_rx_base + CIO2_REG_IRQCTRL_ENABLE); > > writel(0, q->csi_rx_base + CIO2_REG_CSIRX_ENABLE); > > writel(0, q->csi_rx_base + CIO2_REG_MIPIBE_ENABLE); > > > > @@ -1027,6 +1029,7 @@ static void cio2_vb2_stop_streaming(struct > vb2_queue *vq) > > "failed to stop sensor streaming\n"); > > > > cio2_hw_exit(cio2, q); > > + synchronize_irq(cio2->pci_dev->irq); > > Shouldn't this be put in cio2_hw_exit(), which is called from multiple > locations? Presumably the same issue exists there, too. > Thanks for catching this, cio2_hw_exit() is used at two other places, and only one of them is subject to racing, so I will add synchronize_irq there in next update if it's OK. > > cio2_vb2_return_all_buffers(q, VB2_BUF_STATE_ERROR); > > media_pipeline_stop(>vdev.entity); > > pm_runtime_put(>pci_dev->dev); > > -- > Regards, > > Sakari Ailus > e-mail: sakari.ai...@iki.fi
RE: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings
Hi, Tomasz,
Thanks for reviewing the patch.
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Friday, January 12, 2018 12:19 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus
> <sakari.ai...@linux.intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Cao, Bingbu <bingbu@intel.com>
> Subject: Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state
> warnings
>
> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi <yong@intel.com> wrote:
> > cio2 driver should release buffer with QUEUED state when start_stream
> > op failed, wrong buffer state will cause vb2 core throw a warning.
> >
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > Signed-off-by: Cao Bing Bu <bingbu@intel.com>
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 +
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > index 949f43d206ad..106d04306372 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > @@ -785,7 +785,8 @@ static irqreturn_t cio2_irq(int irq, void
> > *cio2_ptr)
> >
> > / Videobuf2 interface /
> >
> > -static void cio2_vb2_return_all_buffers(struct cio2_queue *q)
> > +static void cio2_vb2_return_all_buffers(struct cio2_queue *q,
> > + enum vb2_buffer_state state)
> > {
> > unsigned int i;
> >
> > @@ -793,7 +794,7 @@ static void cio2_vb2_return_all_buffers(struct
> cio2_queue *q)
> > if (q->bufs[i]) {
> > atomic_dec(>bufs_queued);
> > vb2_buffer_done(>bufs[i]->vbb.vb2_buf,
> > - VB2_BUF_STATE_ERROR);
> > + state);
>
> nit: Does it really exceed 80 characters after folding into previous line?
>
Thanks for catching this, seems this patch was merged, may I fix it in future
patch?
> With the nit fixed:
> Reviewed-by: Tomasz Figa <tf...@chromium.org>
>
> Best regards,
> Tomasz
RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access
Hi, Tomasz,
Thanks for the patch review.
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Friday, January 12, 2018 12:17 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus
> <sakari.ai...@linux.intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Cao, Bingbu <bingbu@intel.com>
> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-
> bounds access
>
> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi <yong@intel.com> wrote:
> > When dmabuf is used for BLOB type frame, the frame buffers allocated
> > by gralloc will hold more pages than the valid frame data due to
> > height alignment.
> >
> > In this case, the page numbers in sg list could exceed the FBPT upper
> > limit value - max_lops(8)*1024 to cause crash.
> >
> > Limit the LOP access to the valid data length to avoid FBPT
> > sub-entries overflow.
> >
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > Signed-off-by: Cao Bing Bu <bingbu@intel.com>
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > index 941caa987dab..949f43d206ad 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
> > container_of(vb, struct cio2_buffer, vbb.vb2_buf);
> > static const unsigned int entries_per_page =
> > CIO2_PAGE_SIZE / sizeof(u32);
> > - unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> CIO2_PAGE_SIZE);
> > - unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
> > + unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> > + CIO2_PAGE_SIZE) + 1;
>
> Why + 1? This would still overflow the buffer, wouldn't it?
The "pages" variable is used to calculate lops which has one extra page at the
end that points to dummy page.
>
> > + unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
> > struct sg_table *sg;
> > struct sg_page_iter sg_iter;
> > int i, j;
> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer
> > *vb)
> >
> > i = j = 0;
> > for_each_sg_page(sg->sgl, _iter, sg->nents, 0) {
> > + if (!pages--)
> > + break;
>
> Or perhaps we should check here for (pages > 1)?
This is so that the end of lop is set to the dummy_page.
>
> Best regards,
> Tomasz
RE: [RESEND GIT PULL for 4.16] Intel IPU3 CIO2 CSI-2 receiver driver
Hi, Mauro and Sakari, Sorry for the late response, the fix of the 2 warnings is attached. Best regards, Yong From: Zhi, Yong Sent: Friday, December 08, 2017 3:32 PM To: Mauro Carvalho Chehab; Sakari Ailus Cc: linux-media@vger.kernel.org; Mani, Rajmohan Subject: RE: [RESEND GIT PULL for 4.16] Intel IPU3 CIO2 CSI-2 receiver driver Hi, Mauro, > -Original Message- > From: Mauro Carvalho Chehab [mailto:mche...@s-opensource.com] > Sent: Friday, December 8, 2017 7:00 AM > To: Sakari Ailus <sakari.ai...@iki.fi> > Cc: linux-media@vger.kernel.org; Mani, Rajmohan > <rajmohan.m...@intel.com>; Zhi, Yong <yong@intel.com> > Subject: Re: [RESEND GIT PULL for 4.16] Intel IPU3 CIO2 CSI-2 receiver driver > > Em Fri, 1 Dec 2017 16:31:36 +0200 > Sakari Ailus <sakari.ai...@iki.fi> escreveu: > > > Hi Mauro, > > > > Here's the Intel IPU3 CIO2 CSI-2 receiver driver, with the > > accompanying format definitions. > > This patch generates two warnings: > > drivers/media/pci/intel/ipu3/ipu3-cio2.c:1899:16: warning: Variable length > array is used. > drivers/media/pci/intel/ipu3/ipu3-cio2.c: In function 'cio2_pci_probe': > drivers/media/pci/intel/ipu3/ipu3-cio2.c:1726:14: warning: variable 'phys' > set but not used [-Wunused-but-set-variable] > phys_addr_t phys; > ^~~~ > > We should never use variable-length array on Kernel, as Linux stack is very > limited, and we have static analyzers to check for it at compilation time. > > Also, the logic should check if pci_resource_start() succeeded, instead of > just ignoring it. > > Please fix. > Thanks for catching the warnings, for the variable size array, from the code context, the size is limited to 128 bytes, maybe this language feature itself is not recommended, we will send a patch to address above soon. > > > > > Please pull. > > > > > > The following changes since commit > be9b53c83792e3898755dce90f8c632d40e7c83e: > > > > media: dvb-frontends: complete kernel-doc markups (2017-11-30 > > 04:19:05 -0500) > > > > are available in the git repository at: > > > > ssh://linuxtv.org/git/sailus/media_tree.git ipu3 > > > > for you to fetch changes up to > f178207daa68e817ab6fd702d81ed7c8637ab72c: > > > > intel-ipu3: cio2: add new MIPI-CSI2 driver (2017-11-30 14:19:47 > > +0200) > > > > > > Yong Zhi (3): > > videodev2.h, v4l2-ioctl: add IPU3 raw10 color format > > doc-rst: add IPU3 raw10 bayer pixel format definitions > > intel-ipu3: cio2: add new MIPI-CSI2 driver > > > > Documentation/media/uapi/v4l/pixfmt-rgb.rst|1 + > > .../media/uapi/v4l/pixfmt-srggb10-ipu3.rst | 335 > > MAINTAINERS|8 + > > drivers/media/pci/Kconfig |2 + > > drivers/media/pci/Makefile |3 +- > > drivers/media/pci/intel/Makefile |5 + > > drivers/media/pci/intel/ipu3/Kconfig | 19 + > > drivers/media/pci/intel/ipu3/Makefile |1 + > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 2052 > > > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 449 + > > drivers/media/v4l2-core/v4l2-ioctl.c |4 + > > include/uapi/linux/videodev2.h |6 + > > 12 files changed, 2884 insertions(+), 1 deletion(-) create mode > > 100644 Documentation/media/uapi/v4l/pixfmt-srggb10-ipu3.rst > > create mode 100644 drivers/media/pci/intel/Makefile create mode > > 100644 drivers/media/pci/intel/ipu3/Kconfig > > create mode 100644 drivers/media/pci/intel/ipu3/Makefile > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h > > > > > > Thanks, > Mauro From d7e9baf13b1a8c8308906a7ed213f75d12756c24 Mon Sep 17 00:00:00 2001 From: Yong Zhi <yong@intel.com> Date: Tue, 12 Dec 2017 10:16:31 -0600 Subject: [PATCH] [PATCH v8] intel-ipu3: cio2: fix two warnings in the code Fix two warnings reported by Mauro Carvalho Chehab: ipu3-cio2.c:1899:16: warning: Variable length array is used. In function 'cio2_pci_probe': ipu3-cio2.c:1726:14: warning: variable 'phys' set but not used [-Wunused-but-set-variable] Hi, Sakari, can you squash the patch to your tree? Signed-off-by: Yong Zhi <yong@intel.com> --- drivers/media/pci/intel/ipu3/ipu3-cio2.c | 16 ++-- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git
RE: [RESEND GIT PULL for 4.16] Intel IPU3 CIO2 CSI-2 receiver driver
Hi, Mauro, > -Original Message- > From: Mauro Carvalho Chehab [mailto:mche...@s-opensource.com] > Sent: Friday, December 8, 2017 7:00 AM > To: Sakari Ailus <sakari.ai...@iki.fi> > Cc: linux-media@vger.kernel.org; Mani, Rajmohan > <rajmohan.m...@intel.com>; Zhi, Yong <yong@intel.com> > Subject: Re: [RESEND GIT PULL for 4.16] Intel IPU3 CIO2 CSI-2 receiver driver > > Em Fri, 1 Dec 2017 16:31:36 +0200 > Sakari Ailus <sakari.ai...@iki.fi> escreveu: > > > Hi Mauro, > > > > Here's the Intel IPU3 CIO2 CSI-2 receiver driver, with the > > accompanying format definitions. > > This patch generates two warnings: > > drivers/media/pci/intel/ipu3/ipu3-cio2.c:1899:16: warning: Variable length > array is used. > drivers/media/pci/intel/ipu3/ipu3-cio2.c: In function 'cio2_pci_probe': > drivers/media/pci/intel/ipu3/ipu3-cio2.c:1726:14: warning: variable 'phys' > set but not used [-Wunused-but-set-variable] > phys_addr_t phys; > ^~~~ > > We should never use variable-length array on Kernel, as Linux stack is very > limited, and we have static analyzers to check for it at compilation time. > > Also, the logic should check if pci_resource_start() succeeded, instead of > just ignoring it. > > Please fix. > Thanks for catching the warnings, for the variable size array, from the code context, the size is limited to 128 bytes, maybe this language feature itself is not recommended, we will send a patch to address above soon. > > > > > Please pull. > > > > > > The following changes since commit > be9b53c83792e3898755dce90f8c632d40e7c83e: > > > > media: dvb-frontends: complete kernel-doc markups (2017-11-30 > > 04:19:05 -0500) > > > > are available in the git repository at: > > > > ssh://linuxtv.org/git/sailus/media_tree.git ipu3 > > > > for you to fetch changes up to > f178207daa68e817ab6fd702d81ed7c8637ab72c: > > > > intel-ipu3: cio2: add new MIPI-CSI2 driver (2017-11-30 14:19:47 > > +0200) > > > > > > Yong Zhi (3): > > videodev2.h, v4l2-ioctl: add IPU3 raw10 color format > > doc-rst: add IPU3 raw10 bayer pixel format definitions > > intel-ipu3: cio2: add new MIPI-CSI2 driver > > > > Documentation/media/uapi/v4l/pixfmt-rgb.rst|1 + > > .../media/uapi/v4l/pixfmt-srggb10-ipu3.rst | 335 > > MAINTAINERS|8 + > > drivers/media/pci/Kconfig |2 + > > drivers/media/pci/Makefile |3 +- > > drivers/media/pci/intel/Makefile |5 + > > drivers/media/pci/intel/ipu3/Kconfig | 19 + > > drivers/media/pci/intel/ipu3/Makefile |1 + > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 2052 > > > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 449 + > > drivers/media/v4l2-core/v4l2-ioctl.c |4 + > > include/uapi/linux/videodev2.h |6 + > > 12 files changed, 2884 insertions(+), 1 deletion(-) create mode > > 100644 Documentation/media/uapi/v4l/pixfmt-srggb10-ipu3.rst > > create mode 100644 drivers/media/pci/intel/Makefile create mode > > 100644 drivers/media/pci/intel/ipu3/Kconfig > > create mode 100644 drivers/media/pci/intel/ipu3/Makefile > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h > > > > > > Thanks, > Mauro
RE: [PATCH v4 10/12] intel-ipu3: css pipeline
Hi, Sakari,
Thanks for the review.
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Sakari Ailus
> Sent: Wednesday, November 1, 2017 11:57 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian
> Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>
> Subject: Re: [PATCH v4 10/12] intel-ipu3: css pipeline
>
> Hi Yong,
>
> Apologies for the late reply. Please find my (few) comments below.
>
> On Tue, Oct 17, 2017 at 10:54:55PM -0500, Yong Zhi wrote:
> > Add css pipeline and v4l code.
> >
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-css.c | 1761
> ++-
> > drivers/media/pci/intel/ipu3/ipu3-css.h | 89 ++
> > 2 files changed, 1849 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-css.c
> b/drivers/media/pci/intel/ipu3/ipu3-css.c
> > index 6e615bf9378a..11f7ad3514c3 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-css.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-css.c
> > @@ -13,9 +13,16 @@
(snip)
> > +static bool ipu3_css_queue_enabled(struct ipu3_css_queue *q)
> > +{
> > + return !!q->css_fmt;
>
> No need for !!.
>
The original code "return q->css_fmt !=NULL;" is more explicit, I changed to
the current form to silent checkpatch.pl CHECK for null comparison.
> > +}
> > +
> > /*** css hw ***/
> >
(snip)
> > + /* Configure SP group */
> > +
> > + sp_group = css->xmem_sp_group_ptrs.vaddr;
> > + memset(sp_group, 0, sizeof(*sp_group));
> > +
> > + sp_group->pipe[thread].num_stages = 1;
> > + sp_group->pipe[thread].pipe_id = PIPE_ID;
> > + sp_group->pipe[thread].thread_id = thread;
> > + sp_group->pipe[thread].pipe_num = pipe;
> > + sp_group->pipe[thread].num_execs = -1;
> > + sp_group->pipe[thread].pipe_qos_config = -1;
> > + sp_group->pipe[thread].required_bds_factor = 0;
> > + sp_group->pipe[thread].dvs_frame_delay = IPU3_CSS_AUX_FRAMES
> - 1;
> > + sp_group->pipe[thread].inout_port_config =
> > + IMGU_ABI_PORT_CONFIG_TYPE_INPUT_HOST |
> > + IMGU_ABI_PORT_CONFIG_TYPE_OUTPUT_HOST;
>
> Indentation. Most of this seems to have been fixed but some remains in this
> patch at least. Could you address that for the next version, please?
Yes, will fix all indentation in next version.
>
> > + sp_group->pipe[thread].scaler_pp_lut = 0;
> > + sp_group-
> >pipe[thread].shading.internal_frame_origin_x_bqs_on_sctbl = 0;
> > + sp_group-
> >pipe[thread].shading.internal_frame_origin_y_bqs_on_sctbl = 0;
> > + sp_group->pipe[thread].sp_stage_addr[stage] =
> > + css->xmem_sp_stage_ptrs[pipe][stage].daddr;
> > + sp_group->pipe[thread].pipe_config =
> > + bi->info.isp.sp.enable.params ? (1 << thread) : 0;
(snip)
> > +int ipu3_css_set_parameters(struct ipu3_css *css,
> > + struct ipu3_uapi_params *set_params,
> > + struct ipu3_uapi_gdc_warp_param *set_gdc,
> > + unsigned int gdc_bytes,
> > + struct ipu3_uapi_obgrid_param *set_obgrid,
> > + unsigned int obgrid_bytes)
> > +{
> > + static const unsigned int queue_id = IMGU_ABI_QUEUE_A_ID;
> > + const int stage = 0, thread = 0;
> > + const struct imgu_fw_info *bi;
> > + int obgrid_size;
> > + unsigned int stripes;
> > + struct ipu3_uapi_flags *use = set_params ? _params->use : NULL;
> > +
> > + /* Destination buffers which are filled here */
> > + struct imgu_abi_parameter_set_info *param_set;
> > + struct ipu3_uapi_acc_param *acc = NULL;
> > + struct ipu3_uapi_gdc_warp_param *gdc = NULL;
> > + struct ipu3_uapi_obgrid_param *obgrid = NULL;
> > + const struct ipu3_css_map *map;
> > + void *vmem0 = NULL;
> > + void *dmem0 = NULL;
> > +
> > + enum imgu_abi_memories m;
> > + int r = -EBUSY;
> > + int s;
> > +
> > + if (!css->streaming)
> > + return -EPROTO;
> > +
> > + bi = >fwp->binary_header[css->current_binary];
> > + obgrid_size = ipu3_css_fw_obgrid_size(bi);
> > + stripes = bi->info.isp.sp.iterator.num_st
RE: [PATCH v4 03/12] intel-ipu3: Add IOMMU based dmamap support
Hi, Sakari,
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Friday, October 20, 2017 2:20 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian
> Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>;
> a...@arndb.de; h...@lst.de; robin.mur...@arm.com; iommu@lists.linux-
> foundation.org; Tomasz Figa <tf...@chromium.org>
> Subject: Re: [PATCH v4 03/12] intel-ipu3: Add IOMMU based dmamap
> support
>
> Hi Yong,
>
> On Tue, Oct 17, 2017 at 10:48:59PM -0500, Yong Zhi wrote:
> > From: Tomasz Figa <tf...@chromium.org>
> >
> > This patch adds driver to support IPU3-specific MMU-aware memory
> > alloc/free and sg mapping functions.
> >
> > Signed-off-by: Tomasz Figa <tf...@chromium.org>
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > ---
> > drivers/media/pci/intel/ipu3/Kconfig | 7 +
> > drivers/media/pci/intel/ipu3/Makefile | 2 +-
> > drivers/media/pci/intel/ipu3/ipu3-dmamap.c | 342
> > +
> > drivers/media/pci/intel/ipu3/ipu3-dmamap.h | 33 +++
> > 4 files changed, 383 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 46ff138f3e50..d7dab52dc881 100644
> > --- a/drivers/media/pci/intel/ipu3/Kconfig
> > +++ b/drivers/media/pci/intel/ipu3/Kconfig
> > @@ -26,3 +26,10 @@ 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_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 91cac9cb7401..651773231496 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 ..e54bd9dfa302
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-dmamap.c
> > @@ -0,0 +1,342 @@
> > +/*
> > + * 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
>
> Do you need this for something?
>
Ouch, will remove the un-needed headers.
> > +#include
> > +
> > +#include "ipu3-css-pool.h"
> > +#include "ipu3.h"
> > +
> > +/*
> > + * Based on arch/arm64/mm/dma-mapping.c, with simplifications
> > +possible due
> > + * to driver-specific character of this file.
> > + */
> > +
> > +static int dma_direction_to_prot(enum dma_data_direction dir, bool
> > +coherent) {
> > + int prot = coherent ? IOMMU_CACHE : 0;
> > +
> > + switch (dir) {
> > + case DMA_BIDIRECTIONAL:
> > + return prot | IOMMU_READ | IOMMU_WRITE;
> > + case DMA_TO_DEVICE:
> > + return prot | IOMMU_READ;
> > + case DMA_FROM_DEVICE:
> > + return prot | IOMMU_WRITE;
> > + default:
> > + return 0;
> > + }
> > +}
> > +
RE: [PATCH v5 2/3] doc-rst: add IPU3 raw10 bayer pixel format definitions
Hi, Sakari,
Thanks for the feedback.
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Tuesday, October 10, 2017 1:33 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com;
> hans.verk...@cisco.com; Zheng, Jian Xu <jian.xu.zh...@intel.com>;
> tf...@chromium.org; Mani, Rajmohan <rajmohan.m...@intel.com>;
> Toivonen, Tuukka <tuukka.toivo...@intel.com>; Yang, Hyungwoo
> <hyungwoo.y...@intel.com>; Vijaykumar, Ramya
> <ramya.vijayku...@intel.com>; Rapolu, Chiranjeevi
> <chiranjeevi.rap...@intel.com>
> Subject: Re: [PATCH v5 2/3] doc-rst: add IPU3 raw10 bayer pixel format
> definitions
>
> Hi Yong,
>
> On Fri, Oct 06, 2017 at 06:39:00PM -0500, Yong Zhi wrote:
> > The formats added by this patch are:
> >
> > V4L2_PIX_FMT_IPU3_SBGGR10
> > V4L2_PIX_FMT_IPU3_SGBRG10
> > V4L2_PIX_FMT_IPU3_SGRBG10
> > V4L2_PIX_FMT_IPU3_SRGGB10
> >
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > Signed-off-by: Hyungwoo Yang <hyungwoo.y...@intel.com>
> > ---
> > Documentation/media/uapi/v4l/pixfmt-rgb.rst| 1 +
> > .../media/uapi/v4l/pixfmt-srggb10-ipu3.rst | 166
> +
> > 2 files changed, 167 insertions(+)
> > create mode 100644
> > Documentation/media/uapi/v4l/pixfmt-srggb10-ipu3.rst
> >
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-rgb.rst
> > b/Documentation/media/uapi/v4l/pixfmt-rgb.rst
> > index 4cc27195dc79..cf2ef7df9616 100644
> > --- a/Documentation/media/uapi/v4l/pixfmt-rgb.rst
> > +++ b/Documentation/media/uapi/v4l/pixfmt-rgb.rst
> > @@ -16,6 +16,7 @@ RGB Formats
> > pixfmt-srggb10p
> > pixfmt-srggb10alaw8
> > pixfmt-srggb10dpcm8
> > +pixfmt-srggb10-ipu3
> > pixfmt-srggb12
> > pixfmt-srggb12p
> > pixfmt-srggb16
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-srggb10-ipu3.rst
> > b/Documentation/media/uapi/v4l/pixfmt-srggb10-ipu3.rst
> > new file mode 100644
> > index ..50292186a8b4
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-srggb10-ipu3.rst
> > @@ -0,0 +1,166 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _V4L2_PIX_FMT_IPU3_SBGGR10:
> > +.. _V4L2_PIX_FMT_IPU3_SGBRG10:
> > +.. _V4L2_PIX_FMT_IPU3_SGRBG10:
> > +.. _V4L2_PIX_FMT_IPU3_SRGGB10:
> > +
> >
> +***
> **
> >
> +***
> **
> > +
> > +V4L2_PIX_FMT_IPU3_SBGGR10 ('ip3b'), V4L2_PIX_FMT_IPU3_SGBRG10
> > +('ip3g'), V4L2_PIX_FMT_IPU3_SGRBG10 ('ip3G'),
> > +V4L2_PIX_FMT_IPU3_SRGGB10 ('ip3r')
> >
> +***
> **
> >
> +***
> **
> > +
> > +
> > +10-bit Bayer formats
> > +
> > +Description
> > +===
> > +
> > +These four pixel formats are used by Intel IPU3 driver, they are raw
> > +sRGB / Bayer formats with 10 bits per sample with every 25 pixels
> > +packed to 32 bytes leaving 6 most significant bits padding in the last
> > byte.
> > +The format is little endian.
> > +
> > +In other respects this format is similar to :ref:`V4L2-PIX-FMT-SRGGB10`.
>
> You could add:
>
> Below is an example of a small image in V4L2_PIX_FMT_IPU3_SBGGR10
> format.
>
Ack.
> > +
> > +**Byte Order.**
> > +Each cell is one byte.
> > +
> > +.. raw:: latex
> > +
> > +\newline\newline\begin{adjustbox}{width=\columnwidth}
> > +
> > +.. tabularcolumns::
> >
> +|p{1.3cm}|p{1.0cm}|p{10.9cm}|p{10.9cm}|p{10.9cm}|p{1.0cm}|p{1.0cm}|p
> {
> >
> +10.9cm}|p{10.9cm}|p{10.9cm}|p{1.0cm}|p{1.0cm}|p{10.9cm}|p{10.9cm}|p{
> 1
> >
> +0.9cm}|p{1.0cm}|p{1.0cm}|p{10.9cm}|p{10.9cm}|p{10.9cm}|p{1.0cm}|p{1.
> 0
> >
> +cm}|p{10.9cm}|p{10.9cm}|p{10.9cm}|p{1.0cm}|p{1.0cm}|p{10.9cm}|p{10.9
> c
> > +m}|p{10.9cm}|p{1.0cm}|p{1.0cm}|p{10.9cm}|
>
> The width of this table is over one metre. Could you use fewer columns in it,
> say, four or eight?
>
Sure, will do four columns in next update.
(snip)
>
> --
> Kind regards,
>
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi
RE: [PATCH v4 12/12] intel-ipu3: imgu top level pci device
Hi, Sakari,
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Sakari Ailus
> Sent: Friday, October 20, 2017 4:15 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian
> Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>;
> a...@arndb.de; h...@lst.de; robin.mur...@arm.com; iommu@lists.linux-
> foundation.org; Tomasz Figa <tf...@chromium.org>
> Subject: Re: [PATCH v4 12/12] intel-ipu3: imgu top level pci device
>
> On Tue, Oct 17, 2017 at 10:55:59PM -0500, Yong Zhi wrote:
> > This patch adds support for the Intel IPU v3 as found on Skylake and
> > Kaby Lake SoCs. The driver has a dependency on the firmware binary to
> > function properly.
> >
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > Signed-off-by: Tomasz Figa <tf...@chromium.org>
> > ---
> > drivers/media/pci/intel/ipu3/Kconfig | 17 +
> > drivers/media/pci/intel/ipu3/Makefile | 6 +
> > drivers/media/pci/intel/ipu3/ipu3.c | 882
> ++
> > drivers/media/pci/intel/ipu3/ipu3.h | 186 +++
> > 4 files changed, 1091 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3.c
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3.h
> >
(snip)
> > +/*
> > + * imgu_mem2mem2_ops - used by v4l2 and vb2 */ static const struct
> > +ipu3_mem2mem2_ops imgu_mem2mem2_ops = {
> > + .s_stream = imgu_mem2mem2_s_stream,
>
> You have a single instance of this. How about just using
> imgu_mem2mem2_s_stream instead?
>
Yes, we can remove another level of indirectness.
(snip)
> > +++ b/drivers/media/pci/intel/ipu3/ipu3.h
> > @@ -0,0 +1,186 @@
> > +/*
> > + * Copyright (c) 2017 Intel Corporation.
> > + *
> > + * 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.
> > + *
> > + */
> > +
> > +#ifndef __IPU3_H
> > +#define __IPU3_H
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include "ipu3-css.h"
> > +
> > +/*
> > + * 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_STAT_DVS 6 /* DVS statistics */
> > +#define IMGU_NODE_STAT_LACE7 /* Lace statistics */
> > +#define IMGU_NODE_NUM 8
> > +
> > +#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_mem2mem2_buffer {
> > + /* Public fields */
> > + struct vb2_v4l2_buffer vbb; /* Must be the first field */
>
RE: [PATCH v4 11/12] intel-ipu3: Add imgu v4l2 driver
Hi, Sakari,
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Sakari Ailus
> Sent: Friday, October 20, 2017 4:30 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian
> Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>;
> Vijaykumar, Ramya <ramya.vijayku...@intel.com>
> Subject: Re: [PATCH v4 11/12] intel-ipu3: Add imgu v4l2 driver
>
> Hi Yong,
>
> On Tue, Oct 17, 2017 at 10:54:56PM -0500, Yong Zhi wrote:
> > ipu3 imgu video device based on v4l2, vb2 and media controller
> > framework.
> >
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > Signed-off-by: Ramya Vijaykumar <ramya.vijayku...@intel.com>
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-v4l2.c | 1150
> > ++
> > 1 file changed, 1150 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-v4l2.c
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-v4l2.c
> > b/drivers/media/pci/intel/ipu3/ipu3-v4l2.c
> > new file mode 100644
> > index ..4618880b8675
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-v4l2.c
> > @@ -0,0 +1,1150 @@
(snip)
> > +static int mem2mem2_g_selection(struct ipu3_mem2mem2_device
> *m2m2_dev,
> > + int node, struct v4l2_selection *s) {
> > + struct imgu_device *const imgu =
> > + container_of(m2m2_dev, struct imgu_device, mem2mem2);
> > +
> > + if (node != IPU3_CSS_QUEUE_IN)
> > + return -ENOIOCTLCMD;
> > +
> > + switch (s->target) {
> > + case V4L2_SEL_TGT_CROP:
> > + s->r = imgu->rect.eff;
> > + break;
> > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > + break;
> > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > + break;
> > + case V4L2_SEL_TGT_COMPOSE_PADDED:
> > + break;
> > + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > + break;
> > + case V4L2_SEL_TGT_COMPOSE:
> > + s->r = imgu->rect.bds;
> > + break;
> > + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > + break;
> > +
>
> As the driver uses the V4L2 sub-device interface, the selection API belongsis
> implemented in the sub-device node, not the video nodes.
>
Ok, will study how to make necessary changes to use sub-dev interface for above.
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ipu3_try_fmt(struct file *file, void *fh, struct
> > +v4l2_format *f) {
> > + struct v4l2_pix_format_mplane *pixm = >fmt.pix_mp;
> > + const struct ipu3_fmt *fmt;
> > +
> > + if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> > + fmt = find_format(f, M2M_CAPTURE);
> > + else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> > + fmt = find_format(f, M2M_OUTPUT);
> > + else
> > + return -EINVAL;
> > +
> > + pixm->pixelformat = fmt->fourcc;
> > +
> > + memset(pixm->plane_fmt[0].reserved, 0,
> > + sizeof(pixm->plane_fmt[0].reserved));
>
> No need for the memset here, the framework handles this.
>
> Are there limits on the image size?
>
The memset is added to fix v4l2-compliance failure here.
The image size limit is checked in ipu3-css.c/ipu3_css_queue_init().
(snip)
> > +int ipu3_v4l2_register(struct imgu_device *dev) {
> > + struct ipu3_mem2mem2_device *m2m2 = >mem2mem2;
> > + struct v4l2_mbus_framefmt def_bus_fmt;
> > + struct v4l2_pix_format_mplane def_pix_fmt;
> > +
> > + int i, r;
> > +
> > + /* Initialize miscellaneous variables */
> > + m2m2->streaming = false;
> > + m2m2->v4l2_file_ops = ipu3_v4l2_fops;
> > +
> > + /* Set up media device */
> > + m2m2->media_dev.dev = m2m2->dev;
> > + strlcpy(m2m2->media_dev.model, m2m2->model,
> > + sizeof(m2m2->media_dev.model));
> > + snprintf(m2m2->media_dev.bus_info, sizeof(m2m2-
> >media_dev.bus_info),
> > +"%s", dev_name(m2m2->dev));
> > + m2m2->media_dev.hw_revision = 0;
> > + media_device_init(>media_dev);
> > + r = media_device_register(>media_dev);
> > + if (r) {
> > + dev_err(m2m2-&g
RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset
Hi, Sakari, > -Original Message- > From: linux-media-ow...@vger.kernel.org [mailto:linux-media- > ow...@vger.kernel.org] On Behalf Of Sakari Ailus > Sent: Friday, October 20, 2017 2:10 AM > To: Zhi, Yong <yong@intel.com> > Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian > Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan > <rajmohan.m...@intel.com>; Toivonen, Tuukka > <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>; > a...@arndb.de; h...@lst.de; robin.mur...@arm.com; iommu@lists.linux- > foundation.org > Subject: Re: [PATCH v4 00/12] Intel IPU3 ImgU patchset > > Hi Yong, > > On Tue, Oct 17, 2017 at 10:46:48PM -0500, Yong Zhi wrote: > > This patchset adds support for the Intel IPU3 (Image Processing Unit) > > ImgU which is essentially a modern memory-to-memory ISP. It implements > > raw Bayer to YUV image format conversion as well as a large number of > > other pixel processing algorithms for improving the image quality. > > > > Meta data formats are defined for image statistics (3A, i.e. automatic > > white balance, exposure and focus, histogram and local area contrast > > enhancement) as well as for the pixel processing algorithm parameters. > > The documentation for these formats is currently not included in the > > patchset but will be added in a future version of this set. > > > > The algorithm parameters need to be considered specific to a given > > frame and typically a large number of these parameters change on frame > > to frame basis. Additionally, the parameters are highly structured > > (and not a flat space of independent configuration primitives). They > > also reflect the data structures used by the firmware and the > > hardware. On top of that, the algorithms require highly specialized > > user space to make meaningful use of them. For these reasons it has > > been chosen video buffers to pass > > Do you have a to-do list for this patchset? I think it would be useful to > maintain one, in case not all the comments have been addressed. > Sure, will add in next update. > -- > Sakari Ailus > e-mail: sakari.ai...@iki.fi
RE: [PATCH v4 06/12] intel-ipu3: css: imgu dma buff pool
Hi, Sakari,
Thanks for your review.
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Friday, October 20, 2017 3:38 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian
> Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>; Tomasz
> Figa <tf...@chromium.org>
> Subject: Re: [PATCH v4 06/12] intel-ipu3: css: imgu dma buff pool
>
> On Tue, Oct 17, 2017 at 10:54:51PM -0500, Yong Zhi wrote:
> > The pools are used to store previous parameters set by user with the
> > parameter queue. Due to pipelining, there needs to be multiple sets
> > (up to four) of parameters which are queued in a host-to-sp queue.
> >
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > Signed-off-by: Tomasz Figa <tf...@chromium.org>
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-css-pool.c | 132
> > +++
> > drivers/media/pci/intel/ipu3/ipu3-css-pool.h | 54 +++
> > 2 files changed, 186 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > new file mode 100644
> > index ..d08e2a8b68ed
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > @@ -0,0 +1,132 @@
> > +/*
> > + * Copyright (c) 2017 Intel Corporation.
> > + *
> > + * 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 "ipu3-css-pool.h"
> > +#include "ipu3-dmamap.h"
> > +
> > +int ipu3_css_dma_alloc(struct device *dev,
> > + struct ipu3_css_map *map, size_t size) {
> > + struct imgu_device *imgu = dev_get_drvdata(dev);
> > +
> > + if (size == 0) {
> > + map->vaddr = NULL;
> > + return 0;
> > + }
> > +
> > + if (!ipu3_dmamap_alloc(imgu, map, size))
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +
> > +void ipu3_css_dma_free(struct device *dev, struct ipu3_css_map *map)
> > +{
> > + struct imgu_device *imgu = dev_get_drvdata(dev);
> > +
> > + ipu3_dmamap_free(imgu, map);
> > +}
> > +
> > +void ipu3_css_pool_cleanup(struct device *dev, struct ipu3_css_pool
> > +*pool) {
> > + int i;
> > +
> > + for (i = 0; i < IPU3_CSS_POOL_SIZE; i++)
> > + ipu3_css_dma_free(dev, >entry[i].param); }
> > +
> > +int ipu3_css_pool_init(struct device *dev, struct ipu3_css_pool *pool,
> > + int size)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < IPU3_CSS_POOL_SIZE; i++) {
> > + pool->entry[i].framenum = INT_MIN;
> > + if (ipu3_css_dma_alloc(dev, >entry[i].param, size))
> > + goto fail;
> > + }
> > +
> > + pool->last = IPU3_CSS_POOL_SIZE;
> > +
> > + return 0;
> > +
> > +fail:
> > + ipu3_css_pool_cleanup(dev, pool);
> > + return -ENOMEM;
> > +}
> > +
> > +/*
> > + * Check that the following call to pool_get succeeds.
> > + * Return negative on error.
> > + */
> > +static int ipu3_css_pool_check(struct ipu3_css_pool *pool, long
> > +framenum) {
> > + /* Get the oldest entry */
> > + int n = (pool->last + 1) % IPU3_CSS_POOL_SIZE;
> > +
> > + /*
> > +* pool->entry[n].framenum stores the frame number where that
> > +* entry was allocated. If that was allocated more than POOL_SIZE
> > +* frames back, it is old enough that we know it is no more in
> > +* use by firmware.
> > +*/
> > + if (pool->entry[n].framenum + IPU3_CSS_POOL_SIZE > framenum)
>
> This will wrap aro
RE: [PATCH v3 08/12] intel-ipu3: params: compute and program ccs
Hi, Sakari,
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Friday, October 20, 2017 2:58 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian
> Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Yang, Hyungwoo
> <hyungwoo.y...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>
> Subject: Re: [PATCH v3 08/12] intel-ipu3: params: compute and program ccs
>
> Hi Yong,
>
> On Tue, Jul 18, 2017 at 10:13:40PM -0500, Yong Zhi wrote:
> > A collection of routines that are mainly responsible to calculate the
> > acc parameters.
> >
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-css-params.c | 3114
> > drivers/media/pci/intel/ipu3/ipu3-css-
> params.h | 105 +
> > drivers/media/pci/intel/ipu3/ipu3-css.h| 92 +
> > 3 files changed, 3311 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-params.c
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-params.h
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-params.c
> > b/drivers/media/pci/intel/ipu3/ipu3-css-params.c
> > new file mode 100644
> > index 000..4b600bc
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-css-params.c
> > @@ -0,0 +1,3114 @@
> > +/*
> > + * Copyright (c) 2017 Intel Corporation.
> > + *
> > + * 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 "ipu3-abi.h"
> > +#include "ipu3-css.h"
> > +#include "ipu3-css-fw.h"
> > +#include "ipu3-css-params.h"
> > +#include "ipu3-tables.h"
> > +
> > +static unsigned int ipu3_css_scaler_get_exp(unsigned int counter,
> > + unsigned int divider)
> > +{
> > + int i = fls(divider) - fls(counter);
> > +
> > + if (i <= 0)
> > + return 0;
> > +
> > + if (divider >> i < counter)
> > + i = i - 1;
>
> i--;
>
> > +
> > + return i;
> > +}
> > +
> > +/* Set up the CSS scaler look up table */ static void
> > +ipu3_css_scaler_setup_lut(unsigned int taps,
> > + unsigned int input_width,
> > + unsigned int output_width,
> > + int phase_step_correction,
> > + const int *coeffs,
> > + unsigned int coeffs_size,
> > + s8 coeff_lut[IMGU_SCALER_PHASES *
> > +
> IMGU_SCALER_FILTER_TAPS],
> > + struct ipu3_css_scaler_info *info) {
> > + int tap;
> > + int phase;
> > + int exponent = ipu3_css_scaler_get_exp(output_width, input_width);
> > + int mantissa = (1 << exponent) * output_width;
> > + unsigned int phase_step = 0;
> > + int phase_sum_left = 0;
> > + int phase_sum_right = 0;
> > +
> > + for (phase = 0; phase < IMGU_SCALER_PHASES; phase++) {
> > + for (tap = 0; tap < taps; tap++) {
> > + /* flip table to for convolution reverse indexing */
> > + s64 coeff = coeffs[coeffs_size -
> > + ((tap * (coeffs_size / taps)) +
> > + phase) - 1];
> > + coeff *= mantissa;
> > + coeff /= input_width;
>
> Please use do_div() so this will compile on 32-bit machines.
>
Thanks, above was implemented in v4.
> > +
> > + /* Add +"0.5" */
> > + coeff += 1 << (IMGU_SCALER_COEFF_BITS - 1);
> > + coeff >>= IMGU_SCALER_COEFF_BITS;
> > +
> > + coeff_lut[phase * IMGU_SCALER_FILTER_TAPS + tap]
> =
> > + coeff;
> > +
RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset
Hi, Christoph, Thanks for the message. > -Original Message- > From: Christoph Hellwig [mailto:h...@lst.de] > Sent: Tuesday, October 17, 2017 11:23 PM > To: Zhi, Yong <yong@intel.com> > Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian > Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan > <rajmohan.m...@intel.com>; Toivonen, Tuukka > <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>; > a...@arndb.de; h...@lst.de; robin.mur...@arm.com; iommu@lists.linux- > foundation.org > Subject: Re: [PATCH v4 00/12] Intel IPU3 ImgU patchset > > Please keep everyone on CC for all the patches, othervise they are complete > unreviable and will be ignored. Last time, Tomasz instructed me to cc you and other iommu experts only for mmu/dmamap patches(3 of 12), should I resend the 12 patches to both Linux-media and iommu list? or to Linux-media and cc everyone? Or just send the missing patches to iommu list and you folks this time? Yong
RE: [PATCH 0/2] Add V4L2_BUF_TYPE_META_OUTPUT buffer type
Hi, Sakari, > -Original Message- > From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com] > Sent: Friday, August 18, 2017 2:31 PM > To: linux-media@vger.kernel.org > Cc: linux-...@vger.kernel.org; tf...@chromium.org; Zhi, Yong > <yong@intel.com> > Subject: [PATCH 0/2] Add V4L2_BUF_TYPE_META_OUTPUT buffer type > > Hi folks, > > Here's a non-RFC version of the META_OUTPUT buffer type patches. > > The V4L2_BUF_TYPE_META_OUTPUT buffer type complements the metadata > buffer types support for OUTPUT buffers, capture being already supported. > This is intended for similar cases than V4L2_BUF_TYPE_META_CAPTURE but > for output buffers, e.g. device parameters that may be complex and highly > hierarchical data structure. Statistics are a current use case for metadata > capture buffers. > > Yong: could you take these to your IPU3 ImgU patchset, please? As that > would be the first user, the patches would be merged with the driver itself. > We implemented the meta format support in IPU3, the changes will be in ImgU v4, thanks!! > since RFC: > > - Fix make htmldocs build. > > - Fix CAPTURE -> OUTPUT in buffer.rst. > > - Added " for specifying how the device processes images" in the > documentation. > > Sakari Ailus (2): > v4l: Add support for V4L2_BUF_TYPE_META_OUTPUT > docs-rst: v4l: Document V4L2_BUF_TYPE_META_OUTPUT interface > > Documentation/media/uapi/v4l/buffer.rst | 3 +++ > Documentation/media/uapi/v4l/dev-meta.rst| 33 ++-- > > Documentation/media/uapi/v4l/vidioc-querycap.rst | 3 +++ > Documentation/media/videodev2.h.rst.exceptions | 2 ++ > drivers/media/v4l2-core/v4l2-compat-ioctl32.c| 2 ++ > drivers/media/v4l2-core/v4l2-ioctl.c | 25 ++ > drivers/media/v4l2-core/videobuf2-v4l2.c | 1 + > include/media/v4l2-ioctl.h | 17 > include/uapi/linux/videodev2.h | 2 ++ > 9 files changed, 75 insertions(+), 13 deletions(-) > > -- > 2.7.4
RE: [PATCH v5 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
Hi, Sakari,
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Wednesday, October 11, 2017 11:20 PM
> To: Zhi, Yong <yong@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com;
> hans.verk...@cisco.com; Zheng, Jian Xu <jian.xu.zh...@intel.com>;
> tf...@chromium.org; Mani, Rajmohan <rajmohan.m...@intel.com>;
> Toivonen, Tuukka <tuukka.toivo...@intel.com>; Yang, Hyungwoo
> <hyungwoo.y...@intel.com>; Vijaykumar, Ramya
> <ramya.vijayku...@intel.com>; Rapolu, Chiranjeevi
> <chiranjeevi.rap...@intel.com>
> Subject: Re: [PATCH v5 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
>
> Hi Yong,
>
> One more comment below...
>
> On Thu, Oct 12, 2017 at 01:02:54AM +, Zhi, Yong wrote:
> ...
> > > > +/*** V4L2 sub-device asynchronous registration
> > > callbacks***/
> > > > +
> > > > +struct sensor_async_subdev {
> > > > + struct v4l2_async_subdev asd;
> > > > + struct csi2_bus_info csi2;
> > > > +};
> > > > +
> > > > +static struct cio2_queue *cio2_find_queue_by_sensor_node(struct
> > > cio2_queue *q,
> > > > + struct fwnode_handle
> > > *fwnode)
> > > > +{
> > > > + unsigned int i;
> > > > +
> > > > + for (i = 0; i < CIO2_QUEUES; i++) {
> > > > + if (q[i].sensor->fwnode == fwnode)
> > > > + return [i];
> > > > + }
> > > > +
> > > > + return NULL;
> > > > +}
> > > > +
> > > > +/* The .bound() notifier callback when a match is found */ static
> > > > +int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
> > > > + struct v4l2_subdev *sd,
> > > > + struct v4l2_async_subdev *asd) {
> > > > + struct cio2_device *cio2 = container_of(notifier,
> > > > + struct cio2_device, notifier);
> > > > + struct sensor_async_subdev *s_asd = container_of(asd,
> > > > + struct sensor_async_subdev,
> > > > asd);
> > > > + struct cio2_queue *q;
> > > > + unsigned int i;
> > > > +
> > > > +
> > > > + /* Find first free slot for the subdev */
> > > > + for (i = 0; i < CIO2_QUEUES; i++)
> > > > + if (!cio2->queue[i].sensor)
> > > > + break;
>
> The queues are related to sub-devices with the same number in the name,
> whereas the number of the CSI-2 receiver is q->csi2.port. The problem here
> is that the CSI-2 receiver that the sensor appears to be connected is a
> incrementing number from zero onwards, depending on the order in which
> the devices are bound rather than the real number of the receiver.
>
> The easiest way to address this would be to create 1:1 mapping between the
> queues and CSI-2 receivers.
>
Sure, will fix according your suggestion.
> > > > +
> > > > + if (i >= CIO2_QUEUES) {
> > > > + dev_err(>pci_dev->dev, "too many subdevs\n");
> > > > + return -ENOSPC;
> > > > + }
> > > > + q = >queue[i];
> > > > +
> > > > + q->csi2 = s_asd->csi2;
> > > > + q->sensor = sd;
> > > > + q->csi_rx_base = cio2->base + CIO2_REG_PIPE_BASE(q->csi2.port);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/* The .unbind callback */
> > > > +static void cio2_notifier_unbind(struct v4l2_async_notifier *notifier,
> > > > +struct v4l2_subdev *sd,
> > > > +struct v4l2_async_subdev *asd) {
> > > > + struct cio2_device *cio2 = container_of(notifier,
> > > > + struct cio2_device,
> > > > notifier);
> > > > + unsigned int i;
> > > > +
> > > > + /* Note: sd may here point to unallocated memory. Do not access.
> > > > +*/
> > >
> > > That may be the case but the patchset that this driver depends on
> > > changes it. :-) So you can remove the comment.
> > &
RE: [PATCH v5 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
Hi, Sakari, Thanks for your quick review!! > -Original Message- > From: linux-media-ow...@vger.kernel.org [mailto:linux-media- > ow...@vger.kernel.org] On Behalf Of Sakari Ailus > Sent: Tuesday, October 10, 2017 12:46 AM > To: Zhi, Yong <yong@intel.com> > Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; > hans.verk...@cisco.com; Zheng, Jian Xu <jian.xu.zh...@intel.com>; > tf...@chromium.org; Mani, Rajmohan <rajmohan.m...@intel.com>; > Toivonen, Tuukka <tuukka.toivo...@intel.com>; Yang, Hyungwoo > <hyungwoo.y...@intel.com>; Vijaykumar, Ramya > <ramya.vijayku...@intel.com>; Rapolu, Chiranjeevi > <chiranjeevi.rap...@intel.com> > Subject: Re: [PATCH v5 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver > > Hi Yong, > > Thanks for the update! Please see my comments below. > > On Fri, Oct 06, 2017 at 06:39:01PM -0500, Yong Zhi wrote: > > This patch adds CIO2 CSI-2 device driver for > > Intel's IPU3 camera sub-system support. > > > > Signed-off-by: Yong Zhi <yong@intel.com> > > Signed-off-by: Hyungwoo Yang <hyungwoo.y...@intel.com> > > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com> > > Signed-off-by: Vijaykumar Ramya <ramya.vijayku...@intel.com> > > --- > > drivers/media/pci/Kconfig|2 + > > drivers/media/pci/Makefile |3 +- > > drivers/media/pci/intel/Makefile |5 + > > drivers/media/pci/intel/ipu3/Kconfig | 19 + > > drivers/media/pci/intel/ipu3/Makefile|1 + > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 2052 > ++ > > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 452 +++ > > 7 files changed, 2533 insertions(+), 1 deletion(-) > > create mode 100644 drivers/media/pci/intel/Makefile > > create mode 100644 drivers/media/pci/intel/ipu3/Kconfig > > create mode 100644 drivers/media/pci/intel/ipu3/Makefile > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h > > > > diff --git a/drivers/media/pci/Kconfig b/drivers/media/pci/Kconfig > > index da28e68c87d8..5932e225f9c0 100644 > > --- a/drivers/media/pci/Kconfig > > +++ b/drivers/media/pci/Kconfig > > @@ -54,5 +54,7 @@ source "drivers/media/pci/smipcie/Kconfig" > > source "drivers/media/pci/netup_unidvb/Kconfig" > > endif > > > > +source "drivers/media/pci/intel/ipu3/Kconfig" > > + > > endif #MEDIA_PCI_SUPPORT > > endif #PCI > > diff --git a/drivers/media/pci/Makefile b/drivers/media/pci/Makefile > > index a7e8af0f64a7..d8f98434fb8c 100644 > > --- a/drivers/media/pci/Makefile > > +++ b/drivers/media/pci/Makefile > > @@ -13,7 +13,8 @@ obj-y+= ttpci/ \ > > ddbridge/ \ > > saa7146/\ > > smipcie/\ > > - netup_unidvb/ > > + netup_unidvb/ \ > > + intel/ > > > > obj-$(CONFIG_VIDEO_IVTV) += ivtv/ > > obj-$(CONFIG_VIDEO_ZORAN) += zoran/ > > diff --git a/drivers/media/pci/intel/Makefile > b/drivers/media/pci/intel/Makefile > > new file mode 100644 > > index ..745c8b2a7819 > > --- /dev/null > > +++ b/drivers/media/pci/intel/Makefile > > @@ -0,0 +1,5 @@ > > +# > > +# Makefile for the IPU3 cio2 and ImGU drivers > > +# > > + > > +obj-y += ipu3/ > > diff --git a/drivers/media/pci/intel/ipu3/Kconfig > b/drivers/media/pci/intel/ipu3/Kconfig > > new file mode 100644 > > index ..0861077a4dae > > --- /dev/null > > +++ b/drivers/media/pci/intel/ipu3/Kconfig > > @@ -0,0 +1,19 @@ > > +config VIDEO_IPU3_CIO2 > > + tristate "Intel ipu3-cio2 driver" > > + depends on VIDEO_V4L2 && PCI > > + depends on VIDEO_V4L2_SUBDEV_API > > + depends on MEDIA_CONTROLLER > > + depends on HAS_DMA > > + depends on ACPI > > + depends on X86_64 || COMPILE_TEST > > Why X86_64? We encountered some issue to build with 64bit previously, will fix in next update. > > > + select V4L2_FWNODE > > + select VIDEOBUF2_DMA_SG > > + > > + ---help--- > > + This is the Intel IPU3 CIO2 CSI-2 receiver unit, found in Intel > > + Skylake and Kaby Lake SoCs and used for capturing images and > > + video from a camera sensor. > > + > > + Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2 > > + connected camera. > > + The module will
RE: [PATCH v2 08/12] intel-ipu3: params: compute and program ccs
Hi, Andy,
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Andy Shevchenko
> Sent: Friday, June 16, 2017 3:53 PM
> To: Zhi, Yong <yong@intel.com>
> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>;
> sakari.ai...@linux.intel.com; Zheng, Jian Xu <jian.xu.zh...@intel.com>;
> tf...@chromium.org; Mani, Rajmohan <rajmohan.m...@intel.com>;
> Toivonen, Tuukka <tuukka.toivo...@intel.com>
> Subject: Re: [PATCH v2 08/12] intel-ipu3: params: compute and program ccs
>
> On Thu, Jun 15, 2017 at 1:19 AM, Yong Zhi <yong@intel.com> wrote:
> > A collection of routines that are mainly responsible to calculate the
> > acc parameters.
>
> > +static unsigned int ipu3_css_scaler_get_exp(unsigned int counter,
> > + unsigned int divider) {
> > + unsigned int i = 0;
> > +
> > + while (counter <= divider / 2) {
> > + divider /= 2;
> > + i++;
> > + }
> > +
> > + return i;
>
> We have a lot of different helpers including one you may use instead of this
> function.
>
> It's *highly* recommended you learn what we have under lib/ (and not only
> there) in kernel bewfore submitting a new version.
>
Tried to identify more places that could be re-implemented with lib helpers or
more generic method, but we failed to spot any obvious candidate thus far.
> --
> With Best Regards,
> Andy Shevchenko
RE: [PATCH v2 09/12] intel-ipu3: css hardware setup
Hi, Andy,
Thanks for the review.
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Andy Shevchenko
> Sent: Saturday, June 17, 2017 12:07 PM
> To: Sakari Ailus <sakari.ai...@iki.fi>
> Cc: Zhi, Yong <yong@intel.com>; Linux Media Mailing List me...@vger.kernel.org>; sakari.ai...@linux.intel.com; Zheng, Jian Xu
> <jian.xu.zh...@intel.com>; Tomasz Figa <tf...@chromium.org>; Mani,
> Rajmohan <rajmohan.m...@intel.com>; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>
> Subject: Re: [PATCH v2 09/12] intel-ipu3: css hardware setup
>
> On Sat, Jun 17, 2017 at 9:43 PM, Sakari Ailus <sakari.ai...@iki.fi> wrote:
> > On Sat, Jun 17, 2017 at 01:54:51AM +0300, Andy Shevchenko wrote:
> >> On Thu, Jun 15, 2017 at 1:19 AM, Yong Zhi <yong@intel.com> wrote:
>
> >> > +static void writes(void *mem, ssize_t len, void __iomem *reg) {
> >> > + while (len >= 4) {
> >> > + writel(*(u32 *)mem, reg);
> >> > + mem += 4;
> >> > + reg += 4;
> >> > + len -= 4;
> >> > + }
> >> > +}
> >>
> >> Again, I just looked into patches and first what I see is reinventing the
> wheel.
> >>
> >> memcpy_toio()
>
> > That doesn't quite work: the hardware only supports 32-bit access.
> >
> > So the answer is writesl().
>
> Makes sense!
>
We are not able to use writesl() in the past because it's not defined for x86
platform, but now the helper function is available in 4.14, we will use it in
next update.
> --
> With Best Regards,
> Andy Shevchenko
RE: [PATCH v2 3/3] [media] intel-ipu3: cio2: Add new MIPI-CSI2 driver
Hi, Tomasz, Sorry for the late reply. I will omit the points that have been fixed in v4 or discussed earlier by either Tuukka or Sakari (https://patchwork.linuxtv.org/patch/41665) > -Original Message- > From: linux-media-ow...@vger.kernel.org [mailto:linux-media- > ow...@vger.kernel.org] On Behalf Of Tomasz Figa > Sent: Monday, June 12, 2017 2:59 AM > To: Zhi, Yong <yong@intel.com> > Cc: linux-media@vger.kernel.org; Sakari Ailus <sakari.ai...@linux.intel.com>; > Zheng, Jian Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan > <rajmohan.m...@intel.com>; Toivonen, Tuukka > <tuukka.toivo...@intel.com>; Hans Verkuil <hverk...@xs4all.nl>; Yang, > Hyungwoo <hyungwoo.y...@intel.com> > Subject: Re: [PATCH v2 3/3] [media] intel-ipu3: cio2: Add new MIPI-CSI2 > driver > > Hi Yong, > > Please see my comments inline. > > On Wed, Jun 7, 2017 at 10:34 AM, Yong Zhi <yong@intel.com> wrote: > > This patch adds CIO2 CSI-2 device driver for Intel's IPU3 camera > > sub-system support. > > > > Signed-off-by: Yong Zhi <yong@intel.com> > > --- > > drivers/media/pci/Kconfig|2 + > > drivers/media/pci/Makefile |3 +- > > drivers/media/pci/intel/Makefile |5 + > > drivers/media/pci/intel/ipu3/Kconfig | 17 + > > drivers/media/pci/intel/ipu3/Makefile|1 + > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1788 > > ++ > > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 424 +++ > > 7 files changed, 2239 insertions(+), 1 deletion(-) create mode > > 100644 drivers/media/pci/intel/Makefile create mode 100644 > > drivers/media/pci/intel/ipu3/Kconfig > > create mode 100644 drivers/media/pci/intel/ipu3/Makefile > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h > [snip] > > diff --git a/drivers/media/pci/intel/ipu3/Kconfig > > b/drivers/media/pci/intel/ipu3/Kconfig > > new file mode 100644 > > index 000..2a895d6 > > --- /dev/null > > +++ b/drivers/media/pci/intel/ipu3/Kconfig > > @@ -0,0 +1,17 @@ > > +config VIDEO_IPU3_CIO2 > > + tristate "Intel ipu3-cio2 driver" > > + depends on VIDEO_V4L2 && PCI > > + depends on MEDIA_CONTROLLER > > + depends on HAS_DMA > > + depends on ACPI > > I wonder if it wouldn't make sense to make this depend on X86 (|| > COMPILE_TEST) as well. Are we expecting a standalone PCI(e) card with this > device in the future? Will add depends on (X86 || COMPILE_TEST) && 64BIT > > > + select V4L2_FWNODE > > + select VIDEOBUF2_DMA_SG > > + > > + ---help--- > > + This is the Intel IPU3 CIO2 CSI-2 receiver unit, found in Intel > > + Skylake and Kaby Lake SoCs and used for capturing images and > > + video from a camera sensor. > > + > > + 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. > > diff --git a/drivers/media/pci/intel/ipu3/Makefile > > b/drivers/media/pci/intel/ipu3/Makefile > > new file mode 100644 > > index 000..20186e3 > > --- /dev/null > > +++ b/drivers/media/pci/intel/ipu3/Makefile > > @@ -0,0 +1 @@ > > +obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > new file mode 100644 > > index 000..69c47fc > > --- /dev/null > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > [snip] > > > + u32 fbpt_rp = > > + (readl(cio2->base + CIO2_REG_CDMARI(CIO2_DMA_CHAN)) > > +>> CIO2_CDMARI_FBPT_RP_SHIFT) > > + & CIO2_CDMARI_FBPT_RP_MASK; > > + > > + /* > > +* fbpt_rp is the fbpt entry that the dma is currently > > working > > +* on, but since it could jump to next entry at any time, > > +* assume that we might already be there. > > +*/ > > + fbpt_rp = (fbpt_rp + 1) % CIO2_MAX_BUFFERS; > > Hmm, this is really racy. This code can be pre-empted and not execute for > quite long time, depending on system load, resuming after the hardware > goes even further. Technically you could prevent this using > *_irq_save()/_irq_restore(), but I'd try to find a way that doesn't rely on > the
RE: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
Hi, Sakari, Thanks for the review. > -Original Message- > From: linux-media-ow...@vger.kernel.org [mailto:linux-media- > ow...@vger.kernel.org] On Behalf Of Sakari Ailus > Sent: Tuesday, July 11, 2017 3:34 AM > To: Zhi, Yong <yong@intel.com> > Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; > hans.verk...@cisco.com; Zheng, Jian Xu <jian.xu.zh...@intel.com>; > tf...@chromium.org; Mani, Rajmohan <rajmohan.m...@intel.com>; > Toivonen, Tuukka <tuukka.toivo...@intel.com>; Yang, Hyungwoo > <hyungwoo.y...@intel.com>; Vijaykumar, Ramya > <ramya.vijayku...@intel.com> > Subject: Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver > > Hi Yong, > > Thanks for the update! This looks pretty good in general, still some more > comments below. > > Do you happen to have a todo list for the changes that are still planned? > AFAIR we should have two items in the list at least --- > > 1. Extend the format example to include the DMA word boundary and > > 2. Separate formats on the sub-device and on the video node. > Will include above in v5 update. > On Mon, Jul 10, 2017 at 06:43:34PM -0500, Yong Zhi wrote: > > This patch adds CIO2 CSI-2 device driver for > > Intel's IPU3 camera sub-system support. > > > > Signed-off-by: Yong Zhi <yong@intel.com> > > Signed-off-by: Hyungwoo Yang <hyungwoo.y...@intel.com> > > Signed-off-by: Ramya Vijaykumar <ramya.vijayku...@intel.com> > > --- > > drivers/media/pci/Kconfig|2 + > > drivers/media/pci/Makefile |3 +- > > drivers/media/pci/intel/Makefile |5 + > > drivers/media/pci/intel/ipu3/Kconfig | 17 + > > drivers/media/pci/intel/ipu3/Makefile|1 + > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1873 > ++ > > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 442 +++ > > 7 files changed, 2342 insertions(+), 1 deletion(-) > > create mode 100644 drivers/media/pci/intel/Makefile > > create mode 100644 drivers/media/pci/intel/ipu3/Kconfig > > create mode 100644 drivers/media/pci/intel/ipu3/Makefile > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h > > > > diff --git a/drivers/media/pci/Kconfig b/drivers/media/pci/Kconfig > > index da28e68..5932e22 100644 > > --- a/drivers/media/pci/Kconfig > > +++ b/drivers/media/pci/Kconfig > > @@ -54,5 +54,7 @@ source "drivers/media/pci/smipcie/Kconfig" > > source "drivers/media/pci/netup_unidvb/Kconfig" > > endif > > > > +source "drivers/media/pci/intel/ipu3/Kconfig" > > + > > endif #MEDIA_PCI_SUPPORT > > endif #PCI > > diff --git a/drivers/media/pci/Makefile b/drivers/media/pci/Makefile > > index a7e8af0..d8f9843 100644 > > --- a/drivers/media/pci/Makefile > > +++ b/drivers/media/pci/Makefile > > @@ -13,7 +13,8 @@ obj-y+= ttpci/ \ > > ddbridge/ \ > > saa7146/\ > > smipcie/\ > > - netup_unidvb/ > > + netup_unidvb/ \ > > + intel/ > > > > obj-$(CONFIG_VIDEO_IVTV) += ivtv/ > > obj-$(CONFIG_VIDEO_ZORAN) += zoran/ > > diff --git a/drivers/media/pci/intel/Makefile > b/drivers/media/pci/intel/Makefile > > new file mode 100644 > > index 000..745c8b2 > > --- /dev/null > > +++ b/drivers/media/pci/intel/Makefile > > @@ -0,0 +1,5 @@ > > +# > > +# Makefile for the IPU3 cio2 and ImGU drivers > > +# > > + > > +obj-y += ipu3/ > > diff --git a/drivers/media/pci/intel/ipu3/Kconfig > b/drivers/media/pci/intel/ipu3/Kconfig > > new file mode 100644 > > index 000..2a895d6 > > --- /dev/null > > +++ b/drivers/media/pci/intel/ipu3/Kconfig > > @@ -0,0 +1,17 @@ > > +config VIDEO_IPU3_CIO2 > > + tristate "Intel ipu3-cio2 driver" > > + depends on VIDEO_V4L2 && PCI > > + depends on MEDIA_CONTROLLER > > Could you also add VIDEO_V4L2_SUBDEV_API, please? Ack. > > > + depends on HAS_DMA > > + depends on ACPI > > + select V4L2_FWNODE > > + select VIDEOBUF2_DMA_SG > > + > > + ---help--- > > + This is the Intel IPU3 CIO2 CSI-2 receiver unit, found in Intel > > + Skylake and Kaby Lake SoCs and used for capturing images and > > + video from a camera sensor. > > + > > + Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2 > > + connected came
RE: [PATCH 04/12] intel-ipu3: Add user space ABI definitions
Hi, Sakari and Hans, Sorry for the late response to this header file. > -Original Message- > From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com] > Sent: Wednesday, June 7, 2017 3:23 PM > To: Hans Verkuil <hverk...@xs4all.nl> > Cc: Zhi, Yong <yong@intel.com>; linux-media@vger.kernel.org; Zheng, > Jian Xu <jian.xu.zh...@intel.com>; tf...@chromium.org; Mani, Rajmohan > <rajmohan.m...@intel.com>; Toivonen, Tuukka > <tuukka.toivo...@intel.com> > Subject: Re: [PATCH 04/12] intel-ipu3: Add user space ABI definitions > > Hi Hans, > > On Tue, Jun 06, 2017 at 10:28:26AM +0200, Hans Verkuil wrote: > > On 05/06/17 22:39, Yong Zhi wrote: > > > > Commit message missing. > > > > > Signed-off-by: Yong Zhi <yong@intel.com> > > > --- > > > include/uapi/linux/intel-ipu3.h | 2182 > > > +++ > > > 1 file changed, 2182 insertions(+) > > > create mode 100644 include/uapi/linux/intel-ipu3.h > > > > > > diff --git a/include/uapi/linux/intel-ipu3.h > > > b/include/uapi/linux/intel-ipu3.h new file mode 100644 index > > > 000..9e90aec > > > --- /dev/null > > > +++ b/include/uapi/linux/intel-ipu3.h > > > @@ -0,0 +1,2182 @@ > > > +/* > > > + * Copyright (c) 2017 Intel Corporation. > > > + * > > > + * 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. > > > + */ > > > + > > > +#ifndef __IPU3_UAPI_H > > > +#define __IPU3_UAPI_H > > > + > > > +#include > > > + > > > +#define IPU3_UAPI_ISP_VEC_ELEMS 64 > > > + > > > +#define IMGU_ABI_PAD __aligned(IPU3_UAPI_ISP_WORD_BYTES) > > > +#define IPU3_ALIGN > __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES))) > > > + > > > +#define IPU3_UAPI_ISP_WORD_BYTES 32 > > > +#define IPU3_UAPI_MAX_STRIPES2 > > > + > > > +/*** ipu3_uapi_stats_3a ***/ > > > + > > > +#define IPU3_UAPI_MAX_BUBBLE_SIZE10 > > > + > > > +#define IPU3_UAPI_AE_COLORS 4 > > > +#define IPU3_UAPI_AE_BINS256 > > > + > > > +#define IPU3_UAPI_AWB_MD_ITEM_SIZE 8 > > > +#define IPU3_UAPI_AWB_MAX_SETS 60 > > > +#define IPU3_UAPI_AWB_SET_SIZE 0x500 > > > +#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \ > > > + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * > \ > > > + IPU3_UAPI_AWB_MD_ITEM_SIZE) > > > +#define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \ > > > + (IPU3_UAPI_AWB_MAX_SETS * \ > > > + (IPU3_UAPI_AWB_SET_SIZE + > IPU3_UAPI_AWB_SPARE_FOR_BUBBLES)) > > > + > > > +#define IPU3_UAPI_AF_MAX_SETS24 > > > +#define IPU3_UAPI_AF_MD_ITEM_SIZE4 > > > +#define IPU3_UAPI_AF_SPARE_FOR_BUBBLES \ > > > + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * > \ > > > + IPU3_UAPI_AF_MD_ITEM_SIZE) > > > +#define IPU3_UAPI_AF_Y_TABLE_SET_SIZE0x80 > > > +#define IPU3_UAPI_AF_Y_TABLE_MAX_SIZE \ > > > + (IPU3_UAPI_AF_MAX_SETS * \ > > > + (IPU3_UAPI_AF_Y_TABLE_SET_SIZE + > IPU3_UAPI_AF_SPARE_FOR_BUBBLES) * \ > > > + IPU3_UAPI_MAX_STRIPES) > > > + > > > +#define IPU3_UAPI_AWB_FR_MAX_SETS24 > > > +#define IPU3_UAPI_AWB_FR_MD_ITEM_SIZE8 > > > +#define IPU3_UAPI_AWB_FR_BAYER_TBL_SIZE 0x100 > > > +#define IPU3_UAPI_AWB_FR_SPARE_FOR_BUBBLES \ > > > + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * > \ > > > + IPU3_UAPI_AWB_FR_MD_ITEM_SIZE) > > > +#define IPU3_UAPI_AWB_FR_BAYER_TABLE_MAX_SIZE \ > > > + (IPU3_UAPI_AWB_FR_MAX_SETS * \ > > > + (IPU3_UAPI_AWB_FR_BAYER_TBL_SIZE + \ > > > + IPU3_UAPI_A
RE: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
Hi, Sakari,
Thanks for the time spent on code review, acks to all the comments, except two
places:
> +/* .complete() is called after all subdevices have been located */
> +static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct cio2_device *cio2 = container_of(notifier, struct cio2_device,
> + notifier);
> + struct sensor_async_subdev *s_asd;
> + struct fwnode_handle *fwn_remote, *fwn_endpt, *fwn_remote_endpt;
> + struct cio2_queue *q;
> + struct fwnode_endpoint remote_endpt;
> + unsigned int i, pad;
> + int ret;
> +
> + for (i = 0; i < notifier->num_subdevs; i++) {
> + s_asd = container_of(cio2->notifier.subdevs[i],
> + struct sensor_async_subdev,
> + asd);
> +
> + fwn_remote = s_asd->asd.match.fwnode.fwnode;
> + fwn_endpt = (struct fwnode_handle *)
> + s_asd->vfwn_endpt.base.local_fwnode;
Why do you need a cast?
[YZ] With a cast results in compilation warning:
drivers/media/pci/ipu3/ipu3-cio2.c:1298:13: warning: assignment discards
‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
fwn_endpt = /*(struct fwnode_handle *)*/
> + ret = v4l2_async_notifier_register(>v4l2_dev, >notifier);
> + if (ret) {
> + cio2->notifier.num_subdevs = 0;
No need to assign num_subdevs as 0.
[YZ] _notifier_exit() will call _unregister() if this is not 0.
Regards,
Yong
From: linux-media-ow...@vger.kernel.org [linux-media-ow...@vger.kernel.org] on
behalf of Sakari Ailus [sakari.ai...@iki.fi]
Sent: Tuesday, July 11, 2017 3:33 AM
To: Zhi, Yong
Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com;
hans.verk...@cisco.com; Zheng, Jian Xu; tf...@chromium.org; Mani, Rajmohan;
Toivonen, Tuukka; Yang, Hyungwoo; Vijaykumar, Ramya
Subject: Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
Hi Yong,
Thanks for the update! This looks pretty good in general, still some more
comments below.
Do you happen to have a todo list for the changes that are still planned?
AFAIR we should have two items in the list at least ---
1. Extend the format example to include the DMA word boundary and
2. Separate formats on the sub-device and on the video node.
On Mon, Jul 10, 2017 at 06:43:34PM -0500, Yong Zhi wrote:
> This patch adds CIO2 CSI-2 device driver for
> Intel's IPU3 camera sub-system support.
>
> Signed-off-by: Yong Zhi <yong@intel.com>
> Signed-off-by: Hyungwoo Yang <hyungwoo.y...@intel.com>
> Signed-off-by: Ramya Vijaykumar <ramya.vijayku...@intel.com>
> ---
> drivers/media/pci/Kconfig|2 +
> drivers/media/pci/Makefile |3 +-
> drivers/media/pci/intel/Makefile |5 +
> drivers/media/pci/intel/ipu3/Kconfig | 17 +
> drivers/media/pci/intel/ipu3/Makefile|1 +
> drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1873
> ++
> drivers/media/pci/intel/ipu3/ipu3-cio2.h | 442 +++
> 7 files changed, 2342 insertions(+), 1 deletion(-)
> create mode 100644 drivers/media/pci/intel/Makefile
> create mode 100644 drivers/media/pci/intel/ipu3/Kconfig
> create mode 100644 drivers/media/pci/intel/ipu3/Makefile
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h
>
> diff --git a/drivers/media/pci/Kconfig b/drivers/media/pci/Kconfig
> index da28e68..5932e22 100644
> --- a/drivers/media/pci/Kconfig
> +++ b/drivers/media/pci/Kconfig
> @@ -54,5 +54,7 @@ source "drivers/media/pci/smipcie/Kconfig"
> source "drivers/media/pci/netup_unidvb/Kconfig"
> endif
>
> +source "drivers/media/pci/intel/ipu3/Kconfig"
> +
> endif #MEDIA_PCI_SUPPORT
> endif #PCI
> diff --git a/drivers/media/pci/Makefile b/drivers/media/pci/Makefile
> index a7e8af0..d8f9843 100644
> --- a/drivers/media/pci/Makefile
> +++ b/drivers/media/pci/Makefile
> @@ -13,7 +13,8 @@ obj-y+= ttpci/ \
> ddbridge/ \
> saa7146/\
> smipcie/\
> - netup_unidvb/
> + netup_unidvb/ \
> + intel/
>
> obj-$(CONFIG_VIDEO_IVTV) += ivtv/
> obj-$(CONFIG_VIDEO_ZORAN) += zoran/
> diff --git a/drivers/media/pci/intel/Makefile
> b/drivers/media/pci/intel/Makefile
> new file mode 100644
> index 000..745c8b2
> --- /dev/null
> +++ b/drivers/media/pci/intel/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for the IPU3 cio2 and ImGU drivers
> +#
> +
> +obj-y
RE: [v2,07/12] intel-ipu3: css: firmware management
Thanks Kaehlcke for reviewing the code. > -Original Message- > From: Matthias Kaehlcke [mailto:m...@chromium.org] > Sent: Wednesday, July 12, 2017 11:33 AM > To: Zhi, Yong <yong@intel.com> > Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian > Xu <jian.xu.zh...@intel.com>; tf...@chromium.org; Mani, Rajmohan > <rajmohan.m...@intel.com>; Toivonen, Tuukka > <tuukka.toivo...@intel.com> > Subject: Re: [v2,07/12] intel-ipu3: css: firmware management > > El Wed, Jun 14, 2017 at 05:19:22PM -0500 Yong Zhi ha dit: > > > Functions to load and install imgu FW blobs > > > > Signed-off-by: Yong Zhi <yong@intel.com> > > --- > > drivers/media/pci/intel/ipu3/ipu3-abi.h| 1573 > > > drivers/media/pci/intel/ipu3/ipu3-css-fw.c | 272 + > > drivers/media/pci/intel/ipu3/ipu3-css-fw.h | 219 > > drivers/media/pci/intel/ipu3/ipu3-css.h| 54 + > > 4 files changed, 2118 insertions(+) > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-abi.h > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.c > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.h > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css.h > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-abi.h > b/drivers/media/pci/intel/ipu3/ipu3-abi.h > > new file mode 100644 > > index 000..5107b35 > > --- /dev/null > > +++ b/drivers/media/pci/intel/ipu3/ipu3-abi.h > > @@ -0,0 +1,1573 @@ > > +/* > > + * Copyright (c) 2017 Intel Corporation. > > + * > > + * 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. > > + */ > > + > > +#ifndef __IPU3_ABI_H > > +#define __IPU3_ABI_H > > + > > +#include > > + > > +/*** IMGU Hardware information > ***/ > > + > > +#define IMGU_ISP_VMEM_ALIGN128 > > +#define IMGU_DVS_BLOCK_W 64 > > +#define IMGU_DVS_BLOCK_H 32 > > +#define IMGU_GDC_BUF_X (2 * > IMGU_DVS_BLOCK_W) > > +#define IMGU_GDC_BUF_Y IMGU_DVS_BLOCK_H > > +/* n = 0..1 */ > > +#define IMGU_SP_PMEM_BASE(n) (0x2 + (n) * > 0x4000) > > +#define IMGU_MAX_BQ_GRID_WIDTH 80 > > +#define IMGU_MAX_BQ_GRID_HEIGHT60 > > +#define IMGU_OBGRID_TILE_SIZE 16 > > +#define IMGU_PIXELS_PER_WORD 50 > > +#define IMGU_BYTES_PER_WORD64 > > +#define IMGU_STRIPE_FIXED_HALF_OVERLAP 2 > > +#define IMGU_SHD_SETS 3 > > +#define IMGU_BDS_MIN_CLIP_VAL 0 > > +#define IMGU_BDS_MAX_CLIP_VAL 2 > > +#define IPU3_ABI_AWB_MAX_CELLS_PER_SET 160 > > +#define IPU3_ABI_AF_MAX_CELLS_PER_SET 32 > > +#define IPU3_ABI_AWB_FR_MAX_CELLS_PER_SET 32 > > + > > +#define IMGU_ABI_ACC_OP_IDLE 0 > > +#define IMGU_ABI_ACC_OP_END_OF_ACK 1 > > +#define IMGU_ABI_ACC_OP_END_OF_OPS 2 > > +#define IMGU_ABI_ACC_OP_NO_OPS 3 > > + > > +#define IMGU_ABI_ACC_OPTYPE_PROCESS_LINES 0 > > +#define IMGU_ABI_ACC_OPTYPE_TRANSFER_DATA 1 > > + > > +#define IMGU_MMU_PADDR_SHIFT 12 > > + > > +/* Register definitions */ > > + > > +/* PM_CTRL_0_5_0_IMGHMMADR */ > > +#define IMGU_REG_PM_CTRL 0x0 > > +#define IMGU_PM_CTRL_START BIT(0) > > +#define IMGU_PM_CTRL_CFG_DONE BIT(1) > > +#define IMGU_PM_CTRL_RACE_TO_HALT BIT(2) > > +#define IMGU_PM_CTRL_NACK_ALL BIT(3) > > +#define IMGU_PM_CTRL_CSS_PWRDN BIT(4) > > +#define IMGU_PM_CTRL_RST_AT_EOFBIT(5) > > +#define IMGU_PM_CTRL_FORCE_HALTBIT(6) > > +#define IMGU_PM_CTRL_FORCE_UNHALT BIT(7) > > +#define IMGU_PM_CTRL_FORCE_PWRDN BIT(8) > > +#define IMGU_PM_CTRL_FORCE_R
RE: [PATCH v2 12/12] intel-ipu3: imgu top level pci device
Hi, Andy,
> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Friday, June 16, 2017 3:59 PM
> To: Zhi, Yong <yong@intel.com>
> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>;
> sakari.ai...@linux.intel.com; Zheng, Jian Xu <jian.xu.zh...@intel.com>;
> tf...@chromium.org; Mani, Rajmohan <rajmohan.m...@intel.com>;
> Toivonen, Tuukka <tuukka.toivo...@intel.com>
> Subject: Re: [PATCH v2 12/12] intel-ipu3: imgu top level pci device
>
> On Thu, Jun 15, 2017 at 1:19 AM, Yong Zhi <yong@intel.com> wrote:
>
> Commit message.
>
> > Signed-off-by: Yong Zhi <yong@intel.com>
>
> > + /* Set Power */
> > + r = pm_runtime_get_sync(dev);
> > + if (r < 0) {
> > + dev_err(dev, "failed to set imgu power\n");
>
> > + pm_runtime_put(dev);
>
> I'm not sure it's a right thing to do.
> How did you test runtime PM counters in this case?
>
> > + return r;
> > + }
>
Actually I have not tested the error case, what the right way to do in your
opinion? there is no checking of this function return in lot of the driver
code, or simply returning the error code, I also saw examples to call either
pm_runtime_put() or pm_runtime_put_noidle() in this case.
> --
> With Best Regards,
> Andy Shevchenko
RE: [PATCH 11/12] intel-ipu3: Add imgu v4l2 driver
Hi, Hans and Sakari, Thanks a lot for the code review, I attached the topology print FYI. Regards, Yong From: Sakari Ailus [sakari.ai...@iki.fi] Sent: Friday, June 09, 2017 2:20 AM To: Hans Verkuil Cc: Zhi, Yong; linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian Xu; tf...@chromium.org; Mani, Rajmohan; Toivonen, Tuukka Subject: Re: [PATCH 11/12] intel-ipu3: Add imgu v4l2 driver Hi Hans, On Tue, Jun 06, 2017 at 11:08:07AM +0200, Hans Verkuil wrote: > > + /* Initialize vdev */ > > + strlcpy(vdev->name, node->name, sizeof(vdev->name)); > > + vdev->release = video_device_release_empty; > > + vdev->fops = >v4l2_file_ops; > > + vdev->ioctl_ops = _v4l2_ioctl_ops; > > + vdev->lock = >lock; > > + vdev->v4l2_dev = >v4l2_dev; > > + vdev->queue = >vbq; > > + vdev->vfl_dir = node->output ? VFL_DIR_TX : VFL_DIR_RX; > > Why have two video nodes (one tx, one rx) instead of a single m2m device > node? > > I'm not saying this is wrong, I just like to know the rationale for this > design. There are a bunch of outputs from the same input stream. Also the parameters are needed to process the frame, I think there are two OUTPUT devices and five CAPTURE devices. Yong: could you send the media graph of the device, please? -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk Media controller API version 0.1.0 Media device information driver ipu3-imgu model ipu3-imgu serial bus info:00:05.0 hw revision 0x0 driver version 4.12.0 Device topology - entity 1: ipu3-imgu:0 (8 pads, 8 links) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev0 pad0:Sink stream:0[fmt:UYVY2X8/352x288] link: <- "input":0 [ENABLED,IMMUTABLE] pad1:Sink stream:0[fmt:UYVY2X8/352x288] link: <- "parameters":0 [] pad2:Source stream:0[fmt:UYVY2X8/352x288] link: -> "output":0 [] pad3:Source stream:0[fmt:UYVY2X8/352x288] link: -> "viewfinder":0 [] pad4:Source stream:0[fmt:UYVY2X8/352x288] link: -> "postview":0 [] pad5:Source stream:0[fmt:UYVY2X8/352x288] link: -> "3a stat":0 [] pad6:Source stream:0[fmt:UYVY2X8/352x288] link: -> "dvs stat":0 [] pad7:Source stream:0[fmt:UYVY2X8/352x288] link: -> "lace stat":0 [] - entity 2: input (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video0 pad0:Source link: -> "ipu3-imgu:0":0 [ENABLED,IMMUTABLE] - entity 3: parameters (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video1 pad0:Source link: -> "ipu3-imgu:0":1 [] - entity 4: output (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video2 pad0:Sink link: <- "ipu3-imgu:0":2 [] - entity 5: viewfinder (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video3 pad0:Sink link: <- "ipu3-imgu:0":3 [] - entity 6: postview (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video4 pad0:Sink link: <- "ipu3-imgu:0":4 [] - entity 7: 3a stat (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video5 pad0:Sink link: <- "ipu3-imgu:0":5 [] - entity 8: dvs stat (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video6 pad0:Sink link: <- "ipu3-imgu:0":6 [] - entity 9: lace stat (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video7 pad0:Sink link: <- "ipu3-imgu:0":7 [] imgu-topology.dot Description: imgu-topology.dot
RE: [PATCH 07/12] intel-ipu3: css: firmware management
> -Original Message- > From: linux-media-ow...@vger.kernel.org [mailto:linux-media- > ow...@vger.kernel.org] On Behalf Of Hans Verkuil > Sent: Tuesday, June 6, 2017 1:39 AM > To: Zhi, Yong <yong@intel.com>; linux-media@vger.kernel.org; > sakari.ai...@linux.intel.com > Cc: Zheng, Jian Xu <jian.xu.zh...@intel.com>; tf...@chromium.org; Mani, > Rajmohan <rajmohan.m...@intel.com>; Toivonen, Tuukka > <tuukka.toivo...@intel.com> > Subject: Re: [PATCH 07/12] intel-ipu3: css: firmware management > > On 05/06/17 22:39, Yong Zhi wrote: > > Functions to load and install imgu FW blobs > > > > Signed-off-by: Yong Zhi <yong@intel.com> > > --- > > drivers/media/pci/intel/ipu3/ipu3-abi.h| 1572 > > > drivers/media/pci/intel/ipu3/ipu3-css-fw.c | 272 + > > drivers/media/pci/intel/ipu3/ipu3-css-fw.h | 215 > > drivers/media/pci/intel/ipu3/ipu3-css.h| 54 + > > 4 files changed, 2113 insertions(+) > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-abi.h > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.c > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.h > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css.h > > Has this been tested for both i686 and x86_64 modes? > > Regards, > > Hans Sorry for the late response, the above code has been tested for x86_64 mode only.
RE: [PATCH v3 1/3] videodev2.h, v4l2-ioctl: add IPU3 raw10 color format
> -Original Message- > From: Alan Cox [mailto:gno...@lxorguk.ukuu.org.uk] > Sent: Wednesday, June 14, 2017 6:49 AM > To: Zhi, Yong <yong@intel.com> > Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian > Xu <jian.xu.zh...@intel.com>; tf...@chromium.org; Mani, Rajmohan > <rajmohan.m...@intel.com>; Toivonen, Tuukka > <tuukka.toivo...@intel.com>; Yang, Hyungwoo > <hyungwoo.y...@intel.com>; Mohandass, Divagar > <divagar.mohand...@intel.com> > Subject: Re: [PATCH v3 1/3] videodev2.h, v4l2-ioctl: add IPU3 raw10 color > format > > On Tue, 13 Jun 2017 15:17:14 -0500 > Yong Zhi <yong@intel.com> wrote: > > > Add IPU3 specific formats: > > > > V4L2_PIX_FMT_IPU3_SBGGR10 > > V4L2_PIX_FMT_IPU3_SGBRG10 > > V4L2_PIX_FMT_IPU3_SGRBG10 > > V4L2_PIX_FMT_IPU3_SRGGB10 > > As I said before these are just more bitpacked bayer formats with no reason > to encode them as IPUv3 specific names. > > Alan Ack, will update for next version.
RE: [PATCH 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
Hi, Tomasz, Thanks for your code review, still need more time to study and test the solution for the rest of comments, going forward, I will respond to your review first before submitting new version. Thanks, Yong > -Original Message- > From: linux-media-ow...@vger.kernel.org [mailto:linux-media- > ow...@vger.kernel.org] On Behalf Of Tomasz Figa > Sent: Tuesday, June 13, 2017 5:01 PM > To: Zhi, Yong <yong@intel.com> > Cc: linux-media@vger.kernel.org; Sakari Ailus <sakari.ai...@linux.intel.com>; > Zheng, Jian Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan > <rajmohan.m...@intel.com>; Toivonen, Tuukka > <tuukka.toivo...@intel.com>; Yang, Hyungwoo > <hyungwoo.y...@intel.com>; Mohandass, Divagar > <divagar.mohand...@intel.com> > Subject: Re: [PATCH 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver > > Hi Yong, > > On Wed, Jun 14, 2017 at 5:17 AM, Yong Zhi <yong@intel.com> wrote: > > This patch adds CIO2 CSI-2 device driver for Intel's IPU3 camera > > sub-system support. > > > > Signed-off-by: Yong Zhi <yong@intel.com> > > --- > > drivers/media/pci/Kconfig|2 + > > drivers/media/pci/Makefile |3 +- > > drivers/media/pci/intel/Makefile |5 + > > drivers/media/pci/intel/ipu3/Kconfig | 17 + > > drivers/media/pci/intel/ipu3/Makefile|1 + > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1779 > > ++ > > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 434 > > 7 files changed, 2240 insertions(+), 1 deletion(-) create mode > > 100644 drivers/media/pci/intel/Makefile create mode 100644 > > drivers/media/pci/intel/ipu3/Kconfig > > create mode 100644 drivers/media/pci/intel/ipu3/Makefile > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h > > > > I quickly checked the code and it doesn't seem to have most of my comments > from v2 addressed. It's not a very good practice to send new version without > addressing or at least replying to all the comments - it's the best way to > lose > track of necessary changes. Please make sure that all the comments are > taken care of. > > Best regards, > Tomasz
