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-owner@xxxxxxxxxxxxxxx [mailto:linux-media- > owner@xxxxxxxxxxxxxxx] On Behalf Of Tomasz Figa > Sent: Monday, June 12, 2017 2:59 AM > To: Zhi, Yong <yong.zhi@xxxxxxxxx> > Cc: linux-media@xxxxxxxxxxxxxxx; Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>; > Zheng, Jian Xu <jian.xu.zheng@xxxxxxxxx>; Mani, Rajmohan > <rajmohan.mani@xxxxxxxxx>; Toivonen, Tuukka > <tuukka.toivonen@xxxxxxxxx>; Hans Verkuil <hverkuil@xxxxxxxxx>; Yang, > Hyungwoo <hyungwoo.yang@xxxxxxxxx> > 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.zhi@xxxxxxxxx> wrote: > > This patch adds CIO2 CSI-2 device driver for Intel's IPU3 camera > > sub-system support. > > > > Signed-off-by: Yong Zhi <yong.zhi@xxxxxxxxx> > > --- > > 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 0000000..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 0000000..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 0000000..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 > timing, if possible. Ack Will disable interrupts for the duration of this buffer queueing. > [snip] > > +static int cio2_v4l2_querycap(struct file *file, void *fh, > > + struct v4l2_capability *cap) { > > + struct cio2_device *cio2 = video_drvdata(file); > > + > > + strlcpy(cap->driver, CIO2_NAME, sizeof(cap->driver)); > > + strlcpy(cap->card, CIO2_DEVICE_NAME, sizeof(cap->card)); > > + snprintf(cap->bus_info, sizeof(cap->bus_info), > > + "PCI:%s", pci_name(cio2->pci_dev)); > > + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | > > + V4L2_CAP_STREAMING; > > Hmm, I thought single plane queue type was deprecated these days and > _MPLANE recommended for all new drivers. I'll defer this to other reviewers, > though. Will switch to MPLANE support in v5. > > > + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS; > > + > > + return 0; > > +} > [snip] > > +static int cio2_v4l2_try_fmt(struct file *file, void *fh, struct > > +v4l2_format *f) { > > + u32 pixelformat = f->fmt.pix.pixelformat; > > + unsigned int i; > > + > > + cio2_v4l2_g_fmt(file, fh, f); > > + > > + for (i = 0; i < ARRAY_SIZE(cio2_csi2_fmts); i++) { > > + if (pixelformat == cio2_csi2_fmts[i]) > > + break; > > + } > > + > > + /* Use SRGGB10 as default if not found */ > > + if (i >= ARRAY_SIZE(cio2_csi2_fmts)) > > + pixelformat = V4L2_PIX_FMT_IPU3_SRGGB10; > > + > > + f->fmt.pix.pixelformat = pixelformat; > > + f->fmt.pix.bytesperline = cio2_bytesperline(f->fmt.pix.width); > > + f->fmt.pix.sizeimage = f->fmt.pix.bytesperline * > > + f->fmt.pix.height; > > Shouldn't you use f->fmt.pix_mp instead? > Agreed, will update here together with MPLANE support. > [snip] > > + > > + /* Initialize vbq */ > > + vbq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > > + vbq->io_modes = VB2_USERPTR | VB2_MMAP; > > > Best regards, > Tomasz
