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 <yong....@intel.com>
> ---
>  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 0000000..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 <linux/module.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <media/v4l2-ioctl.h>
> +
> +#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(&imgu->media_dev, imgu->pci_dev, IMGU_NAME);
> +
> +     /* Set up v4l2 device */
> +     imgu->v4l2_dev.mdev = &imgu->media_dev;
> +     imgu->v4l2_dev.ctrl_handler = imgu->ctrl_handler;
> +     r = v4l2_device_register(&imgu->pci_dev->dev, &imgu->v4l2_dev);
> +     if (r) {
> +             dev_err(&imgu->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(&imgu->subdev.entity, IMGU_NODE_NUM,
> +                                imgu->subdev_pads);
> +     if (r) {
> +             dev_err(&imgu->pci_dev->dev,
> +                     "failed initialize subdev media entity (%d)\n", r);
> +             goto fail_media_entity;
> +     }
> +     imgu->subdev.entity.ops = &ipu3_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(&imgu->subdev, &ipu3_subdev_ops);
> +     imgu->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_STATISTICS;
> +     imgu->subdev.internal_ops = &ipu3_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(&imgu->subdev, imgu);
> +     imgu->subdev.ctrl_handler = imgu->ctrl_handler;
> +     r = v4l2_device_register_subdev(&imgu->v4l2_dev, &imgu->subdev);
> +     if (r) {
> +             dev_err(&imgu->pci_dev->dev,
> +                     "failed initialize subdev (%d)\n", r);
> +             goto fail_subdev;
> +     }
> +     r = v4l2_device_register_subdev_nodes(&imgu->v4l2_dev);
> +     if (r) {
> +             dev_err(&imgu->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;
> +
> +     def_pix_fmt.width = def_bus_fmt.width;
> +     def_pix_fmt.height = def_bus_fmt.height;
> +     def_pix_fmt.field = def_bus_fmt.field;
> +     def_pix_fmt.num_planes = 1;
> +     def_pix_fmt.plane_fmt[0].bytesperline = def_pix_fmt.width * 2;
> +     def_pix_fmt.plane_fmt[0].sizeimage =
> +             def_pix_fmt.height * def_pix_fmt.plane_fmt[0].bytesperline;
> +     def_pix_fmt.flags = 0;
> +     def_pix_fmt.colorspace = def_bus_fmt.colorspace;
> +     def_pix_fmt.ycbcr_enc = def_bus_fmt.ycbcr_enc;
> +     def_pix_fmt.quantization = def_bus_fmt.quantization;
> +     def_pix_fmt.xfer_func = def_bus_fmt.xfer_func;
> +
> +     /* Create video nodes and links */
> +     for (i = 0; i < IMGU_NODE_NUM; i++) {
> +             struct imgu_video_device *node = &imgu->nodes[i];
> +             struct video_device *vdev = &node->vdev;
> +             struct vb2_queue *vbq = &node->vbq;
> +             u32 flags;
> +
> +             /* Initialize miscellaneous variables */
> +             mutex_init(&node->lock);
> +             INIT_LIST_HEAD(&node->buffers);
> +
> +             /* Initialize formats to default values */
> +             node->pad_fmt = def_bus_fmt;
> +             ipu3_node_to_v4l2(i, vdev, &node->vdev_fmt);
> +             if (node->vdev_fmt.type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE ||
> +                 node->vdev_fmt.type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> +                     def_pix_fmt.pixelformat = node->output ?
> +                                             V4L2_PIX_FMT_IPU3_SGRBG10 :
> +                                             V4L2_PIX_FMT_NV12;
> +                     node->vdev_fmt.fmt.pix_mp = def_pix_fmt;
> +             }
> +             /* Initialize media entities */
> +             r = media_entity_pads_init(&vdev->entity, 1, &node->vdev_pad);
> +             if (r) {
> +                     dev_err(&imgu->pci_dev->dev,
> +                             "failed initialize media entity (%d)\n", r);
> +                     goto fail_vdev_media_entity;
> +             }
> +             node->vdev_pad.flags = node->output ?
> +                     MEDIA_PAD_FL_SOURCE : MEDIA_PAD_FL_SINK;
> +             vdev->entity.ops = NULL;
> +
> +             /* Initialize vbq */
> +             vbq->type = node->vdev_fmt.type;
> +             vbq->io_modes = VB2_USERPTR | VB2_MMAP | VB2_DMABUF;
> +             vbq->ops = &ipu3_vb2_ops;
> +             vbq->mem_ops = &vb2_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 = &node->lock;
> +             r = vb2_queue_init(vbq);
> +             if (r) {
> +                     dev_err(&imgu->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 = &ipu3_v4l2_fops;
> +             vdev->lock = &node->lock;
> +             vdev->v4l2_dev = &imgu->v4l2_dev;
> +             vdev->queue = &node->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(&imgu->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(&vdev->entity, 0,
> +                                               &imgu->subdev.entity,
> +                                              i, flags);
> +             } else {
> +                     r = media_create_pad_link(&imgu->subdev.entity,
> +                                               i, &vdev->entity, 0, flags);
> +             }
> +             if (r)
> +                     goto fail_link;
> +     }
> +
> +     r = media_device_register(&imgu->media_dev);
> +     if (r) {
> +             dev_err(&imgu->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(&imgu->nodes[i].vdev);
> +fail_vdev:
> +             media_entity_cleanup(&imgu->nodes[i].vdev.entity);
> +fail_vdev_media_entity:
> +             mutex_destroy(&imgu->nodes[i].lock);
> +     }
> +fail_subdevs:
> +     v4l2_device_unregister_subdev(&imgu->subdev);
> +fail_subdev:
> +     media_entity_cleanup(&imgu->subdev.entity);
> +fail_media_entity:
> +     kfree(imgu->subdev_pads);
> +fail_subdev_pads:
> +     v4l2_device_unregister(&imgu->v4l2_dev);
> +fail_v4l2_dev:
> +     media_device_cleanup(&imgu->media_dev);
> +
> +     return r;
> +}
> +EXPORT_SYMBOL_GPL(ipu3_v4l2_register);
> +
> +int ipu3_v4l2_unregister(struct imgu_device *imgu)
> +{
> +     unsigned int i;
> +
> +     media_device_unregister(&imgu->media_dev);
> +     media_device_cleanup(&imgu->media_dev);

media_device_cleanup() should take place after unregistering the
sub-devices.

> +
> +     for (i = 0; i < IMGU_NODE_NUM; i++) {
> +             video_unregister_device(&imgu->nodes[i].vdev);
> +             media_entity_cleanup(&imgu->nodes[i].vdev.entity);
> +             mutex_destroy(&imgu->nodes[i].lock);
> +     }
> +
> +     v4l2_device_unregister_subdev(&imgu->subdev);
> +     media_entity_cleanup(&imgu->subdev.entity);
> +     kfree(imgu->subdev_pads);
> +     v4l2_device_unregister(&imgu->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(&b->list);
> +     vb2_buffer_done(&b->vbb.vb2_buf, state);
> +}
> +EXPORT_SYMBOL_GPL(ipu3_v4l2_buffer_done);

-- 
Kind regards,

Sakari Ailus
sakari.ai...@linux.intel.com

Reply via email to