> -----Original Message-----
> From: Hans Verkuil <hverkuil@xxxxxxxxx>
> Sent: Thursday, January 26, 2023 5:02 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@xxxxxxxxxxxxx>; laurent.pinchart@xxxxxxxxxxxxxxxx;
> mchehab@xxxxxxxxxx; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; rafael.j.wysocki@xxxxxxxxx;
> broonie@xxxxxxxxxx
> Cc: linux-media@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 3/6] media: platform: visconti: Add Toshiba Visconti
> Video Input Interface driver user interace
>
> On 26/01/2023 02:25, yuji2.ishikawa@xxxxxxxxxxxxx wrote:
> > Hello Hans,
> >
> > Thank you for your comments.
> >
> >> -----Original Message-----
> >> From: Hans Verkuil <hverkuil@xxxxxxxxx>
> >> Sent: Tuesday, January 17, 2023 8:47 PM
> >> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> >> <yuji2.ishikawa@xxxxxxxxxxxxx>; Laurent Pinchart
> >> <laurent.pinchart@xxxxxxxxxxxxxxxx>; Mauro Carvalho Chehab
> >> <mchehab@xxxxxxxxxx>; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> >> <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>;
> >> Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Rafael J .
> >> Wysocki <rafael.j.wysocki@xxxxxxxxx>; Mark Brown <broonie@xxxxxxxxxx>
> >> Cc: linux-media@xxxxxxxxxxxxxxx;
> >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> >> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH v5 3/6] media: platform: visconti: Add Toshiba
> >> Visconti Video Input Interface driver user interace
> >>
> >> More comments below:
> >>
> >> On 11/01/2023 03:24, Yuji Ishikawa wrote:
> >>> Add support to Video Input Interface on Toshiba Visconti ARM SoCs.
> >>> The interface device includes CSI2 Receiver, frame grabber, video
> >>> DMAC and image signal processor.
> >>> This patch provides the user interface layer.
> >>>
> >>> A driver instance provides three /dev/videoX device files; one for
> >>> RGB image capture, another one for optional RGB capture with
> >>> different parameters and the last one for RAW capture.
> >>>
> >>> Through the device files, the driver provides streaming (DMA-BUF)
> interface.
> >>> A userland application should feed DMA-BUF instances for capture
> buffers.
> >>>
> >>> The driver is based on media controller framework.
> >>> Its operations are roughly mapped to two subdrivers; one for ISP and
> >>> CSI2 receiver (yields 1 instance), the other for capture (yields 3
> >>> instances for each capture mode).
> >>>
> >>> Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@xxxxxxxxxxxxx>
> >>> ---
> >>> Changelog v2:
> >>> - Resend v1 because a patch exceeds size limit.
> >>>
> >>> Changelog v3:
> >>> - Adapted to media control framework
> >>> - Introduced ISP subdevice, capture device
> >>> - Remove private IOCTLs and add vendor specific V4L2 controls
> >>> - Change function name avoiding camelcase and uppercase letters
> >>>
> >>> Changelog v4:
> >>> - Split patches because the v3 patch exceeds size limit
> >>> - Stop using ID number to identify driver instance:
> >>> - Use dynamically allocated structure to hold HW specific context,
> >>> instead of static one.
> >>> - Call HW layer functions with the context structure instead of ID
> >>> number
> >>> - Use pm_runtime to trigger initialization of HW
> >>> along with open/close of device files.
> >>>
> >>> Changelog v5:
> >>> - Fix coding style problems in viif.c
> >>> ---
> >>> drivers/media/platform/visconti/Makefile | 1 +
> >>> drivers/media/platform/visconti/viif.c | 545 ++++++++
> >>> drivers/media/platform/visconti/viif.h | 203 +++
> >>> .../media/platform/visconti/viif_capture.c | 1201
> >> +++++++++++++++++
> >>> drivers/media/platform/visconti/viif_isp.c | 846 ++++++++++++
> >>> 5 files changed, 2796 insertions(+) create mode 100644
> >>> drivers/media/platform/visconti/viif.c
> >>> create mode 100644 drivers/media/platform/visconti/viif.h
> >>> create mode 100644 drivers/media/platform/visconti/viif_capture.c
> >>> create mode 100644 drivers/media/platform/visconti/viif_isp.c
> >>>
>
> <snip>
>
> >>> +static int viif_g_selection(struct file *file, void *priv, struct
> >>> +v4l2_selection *s) {
> >>> + struct cap_dev *cap_dev = video_drvdata_to_capdev(file);
> >>> + struct viif_device *viif_dev = cap_dev->viif_dev;
> >>> + struct v4l2_subdev_selection sel = {
> >>> + .target = V4L2_SEL_TGT_CROP,
> >>> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >>> + };
> >>> + int ret;
> >>> +
> >>
> >> This is missing validation checks for s->type and s->target.
> >
> > I'll add validation code.
> >
> >> I've pretty sure you didn't run the v4l2-compliance utility: that
> >> would have failed on this.
> >
> > Sorry I didn't know the utility.
> > I'll apply it.
>
> FYI: v4l2-compliance is part of the v4l-utils git repo.
>
> Build it from scratch:
>
> git clone git://linuxtv.org/v4l-utils.git cd v4l-utils ./bootstrap.sh ./configure
> make sudo make install
>
Thank you for information.
I'll run v4l2-compliance tool.
> >
> >>> + /* check path id */
> >>> + if (cap_dev->pathid == CAPTURE_PATH_MAIN_POST0)
> >>> + sel.pad = VIIF_ISP_PAD_SRC_PATH0;
> >>> + else if (cap_dev->pathid == CAPTURE_PATH_MAIN_POST1)
> >>> + sel.pad = VIIF_ISP_PAD_SRC_PATH1;
> >>> + else
> >>> + return -EINVAL;
> >>> +
> >>> + ret = v4l2_subdev_call(&viif_dev->isp_subdev.sd, pad,
> >>> +get_selection,
> >> NULL, &sel);
> >>> + s->r = sel.r;
> >>> +
> >>> + return ret;
> >>> +}
>
> <snip>
>
> >>> +/* ----- register/remove capture device node ----- */ static int
> >>> +visconti_viif_capture_register_node(struct cap_dev *cap_dev) {
> >>> + struct viif_device *viif_dev = cap_dev->viif_dev;
> >>> + struct v4l2_device *v4l2_dev = &viif_dev->v4l2_dev;
> >>> + struct video_device *vdev = &cap_dev->vdev;
> >>> + struct vb2_queue *q = &cap_dev->vb2_vq;
> >>> + static const char *const node_name[] = {
> >>> + "viif_capture_post0",
> >>> + "viif_capture_post1",
> >>> + "viif_capture_sub",
> >>> + };
> >>> + int ret;
> >>> +
> >>> + INIT_LIST_HEAD(&cap_dev->buf_queue);
> >>> +
> >>> + mutex_init(&cap_dev->vlock);
> >>> +
> >>> + /* Initialize vb2 queue. */
> >>> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> >>> + q->io_modes = VB2_DMABUF;
> >>
> >> Why is there no VB2_MMAP?
> >
> > The hardware requests physically contiguous memory for frame buffer.
> > I'm not sure if memory allocation in VB2_MMAP mode satisfies this restriction.
>
> Yes, it does.
>
> >
> >>> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> >>> + q->ops = &viif_vb2_ops;
> >>> + q->mem_ops = &vb2_dma_contig_memops;
>
> The use of vb2_dma_contig_memops is what enforces physically contiguous
> memory.
> It works fine with VB2_MMAP.
>
Thank you. I will add VB2_MMAP support.
> >>> + q->drv_priv = cap_dev;
> >>> + q->buf_struct_size = sizeof(struct viif_buffer);
> >>> + q->min_buffers_needed = 2;
> >>> + q->lock = &cap_dev->vlock;
> >>> + q->dev = viif_dev->v4l2_dev.dev;
> >>> +
> >>> + ret = vb2_queue_init(q);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + /* Register the video device. */
> >>> + strscpy(vdev->name, node_name[cap_dev->pathid],
> >> sizeof(vdev->name));
> >>> + vdev->v4l2_dev = v4l2_dev;
> >>> + vdev->lock = &cap_dev->vlock;
> >>> + vdev->queue = &cap_dev->vb2_vq;
> >>> + vdev->ctrl_handler = NULL;
> >>> + vdev->fops = &viif_fops;
> >>> + vdev->ioctl_ops = &viif_ioctl_ops;
> >>> + vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> >> V4L2_CAP_STREAMING;
> >>> + vdev->device_caps |= V4L2_CAP_IO_MC;
> >>> + vdev->entity.ops = &viif_media_ops;
> >>> + vdev->release = video_device_release_empty;
> >>> + video_set_drvdata(vdev, cap_dev);
> >>> + vdev->vfl_dir = VFL_DIR_RX;
> >>> + cap_dev->capture_pad.flags = MEDIA_PAD_FL_SINK;
> >>> +
> >>> + ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> >>> + if (ret < 0) {
> >>> + dev_err(v4l2_dev->dev, "video_register_device failed: %d\n",
> >> ret);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + ret = media_entity_pads_init(&vdev->entity, 1,
> >> &cap_dev->capture_pad);
> >>> + if (ret) {
> >>> + video_unregister_device(vdev);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + ret = v4l2_ctrl_handler_init(&cap_dev->ctrl_handler, 30);
> >>> + if (ret)
> >>> + return -ENOMEM;
> >>> +
> >>> + cap_dev->vdev.ctrl_handler = &cap_dev->ctrl_handler;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +int visconti_viif_capture_register(struct viif_device *viif_dev) {
> >>> + int ret;
> >>> +
> >>> + /* register MAIN POST0 (primary RGB output)*/
> >>> + viif_dev->cap_dev0.pathid = CAPTURE_PATH_MAIN_POST0;
> >>> + viif_dev->cap_dev0.viif_dev = viif_dev;
> >>> + ret = visconti_viif_capture_register_node(&viif_dev->cap_dev0);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + /* register MAIN POST1 (additional RGB output)*/
> >>> + viif_dev->cap_dev1.pathid = CAPTURE_PATH_MAIN_POST1;
> >>> + viif_dev->cap_dev1.viif_dev = viif_dev;
> >>> + ret = visconti_viif_capture_register_node(&viif_dev->cap_dev1);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + /* register SUB (RAW output) */
> >>> + viif_dev->cap_dev2.pathid = CAPTURE_PATH_SUB;
> >>> + viif_dev->cap_dev2.viif_dev = viif_dev;
> >>> + ret = visconti_viif_capture_register_node(&viif_dev->cap_dev2);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static void visconti_viif_capture_unregister_node(struct cap_dev
> >>> +*cap_dev) {
> >>> + media_entity_cleanup(&cap_dev->vdev.entity);
> >>> + v4l2_ctrl_handler_free(&cap_dev->ctrl_handler);
> >>> + vb2_video_unregister_device(&cap_dev->vdev);
> >>> + mutex_destroy(&cap_dev->vlock);
> >>> +}
> >>> +
> >>> +void visconti_viif_capture_unregister(struct viif_device *viif_dev)
> >>> +{
> >>> + visconti_viif_capture_unregister_node(&viif_dev->cap_dev0);
> >>> + visconti_viif_capture_unregister_node(&viif_dev->cap_dev1);
> >>> + visconti_viif_capture_unregister_node(&viif_dev->cap_dev2);
> >>> +}
>
> Regards,
>
> Hans
Regards,
Yuji Ishikawa
