RE: [PATCH v5 3/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver user interace

[<yuji2.ishikawa@toshiba.co.jp>]

> -----Original Message-----
> From: Hans Verkuil <hverkuil@xs4all.nl>
> Sent: Thursday, January 26, 2023 5:02 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@toshiba.co.jp>; laurent.pinchart@ideasonboard.com;
> mchehab@kernel.org; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> <nobuhiro1.iwamatsu@toshiba.co.jp>; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; rafael.j.wysocki@intel.com;
> broonie@kernel.org
> Cc: linux-media@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
> 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@toshiba.co.jp wrote:
> > Hello Hans,
> >
> > Thank you for your comments.
> >
> >> -----Original Message-----
> >> From: Hans Verkuil <hverkuil@xs4all.nl>
> >> Sent: Tuesday, January 17, 2023 8:47 PM
> >> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> >> <yuji2.ishikawa@toshiba.co.jp>; Laurent Pinchart
> >> <laurent.pinchart@ideasonboard.com>; Mauro Carvalho Chehab
> >> <mchehab@kernel.org>; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> >> <nobuhiro1.iwamatsu@toshiba.co.jp>; Rob Herring <robh+dt@kernel.org>;
> >> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Rafael J .
> >> Wysocki <rafael.j.wysocki@intel.com>; Mark Brown <broonie@kernel.org>
> >> Cc: linux-media@vger.kernel.org;
> >> linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
> >> 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@toshiba.co.jp>
> >>> ---
> >>> 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