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 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.
> + 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 >= 0; i--) {
> +fail_link:
> + video_unregister_device(>nodes[i].vdev);
> +fail_vdev:
> + media_entity_cleanup(>nodes[i].vdev.entity);
> +fail_vdev_media_entity:
> + mutex_destroy(>nodes[i].lock);
> + }
> +fail_subdevs:
> + v4l2_device_unregister_subdev(>subdev);
> +fail_subdev:
> + media_entity_cleanup(>subdev.entity);
> +fail_media_entity:
> + kfree(imgu->subdev_pads);
> +fail_subdev_pads:
> + v4l2_device_unregister(>v4l2_dev);
> +fail_v4l2_dev:
> + media_device_cleanup(>media_dev);
> +
> + return r;
> +}
> +EXPORT_SYMBOL_GPL(ipu3_v4l2_register);
> +
> +int ipu3_v4l2_unregister(struct imgu_device *imgu)
> +{
> + unsigned int i;
> +
> + media_device_unregister(>media_dev);
> + media_device_cleanup(>media_dev);
> +
> + 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);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ipu3_v4l2_unregister);
> +
> +void ipu3_v4l2_buffer_done(struct vb2_buffer *vb,
> +enum vb2_buffer_state state)
> +{
> + struct ipu3_vb2_buffer *b =
> + container_of(vb, struct ipu3_vb2_buffer, vbb.vb2_buf);
> +
> + list_del(>list);
> +
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 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.
> +
> + *v4l2_subdev_get_try_crop(sd, fh->pad, IMGU_NODE_IN) = try_crop;
Same for the crop. How about the 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.
> + 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.
> + 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++) {
> + imgu->subdev_pads[i].flags = imgu->nodes[i].output ?
> + MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
> + }
> +
> + /* Initialize subdev */
> + v4l2_subdev_init(>subdev, _subdev_ops);
> + imgu->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_STATISTICS;
> + imgu->subdev.internal_ops = _subdev_internal_ops;
> + imgu->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> + strlcpy(imgu->subdev.name, IMGU_NAME, sizeof(imgu->subdev.name));
strscpy(), please. Same elsewhere in this and also on the 13th.
> + v4l2_set_subdevdata(>subdev, imgu);
> + imgu->subdev.ctrl_handler = imgu->ctrl_handler;
> + r = v4l2_device_register_subdev(>v4l2_dev, >subdev);
> + if (r) {
> + dev_err(>pci_dev->dev,
> + "failed initialize subdev (%d)\n", r);
> + goto fail_subdev;
> + }
> + r = v4l2_device_register_subdev_nodes(>v4l2_dev);
> + if (r) {
> + dev_err(>pci_dev->dev,
> + "failed to register subdevs (%d)\n", r);
> + goto fail_subdevs;
> + }
> +
> + /* Initialize formats to default values */
> + def_bus_fmt.width = 1920;
> + def_bus_fmt.height = 1080;
> + def_bus_fmt.code = MEDIA_BUS_FMT_UYVY8_2X8;
> + def_bus_fmt.field = V4L2_FIELD_NONE;
> + def_bus_fmt.colorspace = V4L2_COLORSPACE_RAW;
> + def_bus_fmt.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + def_bus_fmt.quantization = V4L2_QUANTIZATION_DEFAULT;
> + def_bus_fmt.xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +
>
