Hi, Hans, Thanks for the review. > -----Original Message----- > From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx] > Sent: Thursday, November 15, 2018 6:51 AM > To: Zhi, Yong <yong.zhi@xxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx; > sakari.ailus@xxxxxxxxxxxxxxx > Cc: tfiga@xxxxxxxxxxxx; mchehab@xxxxxxxxxx; hans.verkuil@xxxxxxxxx; > laurent.pinchart@xxxxxxxxxxxxxxxx; Mani, Rajmohan > <rajmohan.mani@xxxxxxxxx>; Zheng, Jian Xu <jian.xu.zheng@xxxxxxxxx>; Hu, > Jerry W <jerry.w.hu@xxxxxxxxx>; Toivonen, Tuukka > <tuukka.toivonen@xxxxxxxxx>; Qiu, Tian Shu <tian.shu.qiu@xxxxxxxxx>; Cao, > Bingbu <bingbu.cao@xxxxxxxxx> > 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 <yong.zhi@xxxxxxxxx> > > --- > > 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 > > <snip> > > > +int ipu3_v4l2_register(struct imgu_device *imgu) { > > <snip> > > > + /* 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 = &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); > > + > > + 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); > > > > Regards, > > Hans
