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
>
>>> + /* 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.
>>> + 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
