| Message ID | 1480085841-28276-1-git-send-email-todor.tomov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
A few comments below: On 11/25/2016 03:57 PM, Todor Tomov wrote: > These files handle the video device nodes of the camss driver. > > Signed-off-by: Todor Tomov <todor.tomov@linaro.org> > --- > drivers/media/platform/qcom/camss-8x16/video.c | 597 +++++++++++++++++++++++++ > drivers/media/platform/qcom/camss-8x16/video.h | 67 +++ > 2 files changed, 664 insertions(+) > create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c > create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h > > diff --git a/drivers/media/platform/qcom/camss-8x16/video.c b/drivers/media/platform/qcom/camss-8x16/video.c > new file mode 100644 > index 0000000..0bf8ea9 > --- /dev/null > +++ b/drivers/media/platform/qcom/camss-8x16/video.c > @@ -0,0 +1,597 @@ <snip> > +static int video_start_streaming(struct vb2_queue *q, unsigned int count) > +{ > + struct camss_video *video = vb2_get_drv_priv(q); > + struct video_device *vdev = video->vdev; > + struct media_entity *entity; > + struct media_pad *pad; > + struct v4l2_subdev *subdev; > + int ret; > + > + ret = media_entity_pipeline_start(&vdev->entity, &video->pipe); > + if (ret < 0) > + return ret; > + > + ret = video_check_format(video); > + if (ret < 0) > + goto error; > + > + entity = &vdev->entity; > + while (1) { > + pad = &entity->pads[0]; > + if (!(pad->flags & MEDIA_PAD_FL_SINK)) > + break; > + > + pad = media_entity_remote_pad(pad); > + if (!pad || !is_media_entity_v4l2_subdev(pad->entity)) > + break; > + > + entity = pad->entity; > + subdev = media_entity_to_v4l2_subdev(entity); > + > + ret = v4l2_subdev_call(subdev, video, s_stream, 1); > + if (ret < 0 && ret != -ENOIOCTLCMD) > + goto error; > + } > + > + return 0; > + > +error: > + media_entity_pipeline_stop(&vdev->entity); > + On error all queued buffers must be returned with state VB2_STATE_QUEUED. Basically the same as 'camss_video_call(video, flush_buffers);', just with a different state. > + return ret; > +} <snip> > +static int video_querycap(struct file *file, void *fh, > + struct v4l2_capability *cap) > +{ > + strlcpy(cap->driver, "qcom-camss", sizeof(cap->driver)); > + strlcpy(cap->card, "Qualcomm Camera Subsystem", sizeof(cap->card)); > + strlcpy(cap->bus_info, "platform:qcom-camss", sizeof(cap->bus_info)); > + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | > + V4L2_CAP_DEVICE_CAPS; > + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; Don't set capabilities and device_caps here. Instead fill in the struct video_device device_caps field and the v4l2 core will take care of cap->capabilities and cap->device_caps. > + > + return 0; > +} > + > +static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f) > +{ > + struct camss_video *video = video_drvdata(file); > + struct v4l2_format format; > + int ret; > + > + if (f->type != video->type) > + return -EINVAL; > + > + if (f->index) > + return -EINVAL; > + > + ret = video_get_subdev_format(video, &format); > + if (ret < 0) > + return ret; > + > + f->pixelformat = format.fmt.pix.pixelformat; > + > + return 0; > +} > + > +static int video_g_fmt(struct file *file, void *fh, struct v4l2_format *f) > +{ > + struct camss_video *video = video_drvdata(file); > + > + if (f->type != video->type) > + return -EINVAL; > + > + *f = video->active_fmt; > + > + return 0; > +} > + > +static int video_s_fmt(struct file *file, void *fh, struct v4l2_format *f) > +{ > + struct camss_video *video = video_drvdata(file); > + int ret; > + > + if (f->type != video->type) > + return -EINVAL; > + > + ret = video_get_subdev_format(video, f); > + if (ret < 0) > + return ret; > + > + video->active_fmt = *f; > + > + return 0; > +} > + > +static int video_try_fmt(struct file *file, void *fh, struct v4l2_format *f) > +{ > + struct camss_video *video = video_drvdata(file); > + > + if (f->type != video->type) > + return -EINVAL; > + > + return video_get_subdev_format(video, f); > +} > + > +static int video_enum_input(struct file *file, void *fh, > + struct v4l2_input *input) > +{ > + if (input->index > 0) > + return -EINVAL; > + > + strlcpy(input->name, "camera", sizeof(input->name)); > + input->type = V4L2_INPUT_TYPE_CAMERA; > + > + return 0; > +} > + > +static int video_g_input(struct file *file, void *fh, unsigned int *input) > +{ > + *input = 0; > + > + return 0; > +} > + > +static int video_s_input(struct file *file, void *fh, unsigned int input) > +{ > + return input == 0 ? 0 : -EINVAL; > +} > + > +static const struct v4l2_ioctl_ops msm_vid_ioctl_ops = { > + .vidioc_querycap = video_querycap, > + .vidioc_enum_fmt_vid_cap = video_enum_fmt, > + .vidioc_g_fmt_vid_cap = video_g_fmt, > + .vidioc_s_fmt_vid_cap = video_s_fmt, > + .vidioc_try_fmt_vid_cap = video_try_fmt, > + .vidioc_reqbufs = vb2_ioctl_reqbufs, > + .vidioc_querybuf = vb2_ioctl_querybuf, > + .vidioc_qbuf = vb2_ioctl_qbuf, > + .vidioc_dqbuf = vb2_ioctl_dqbuf, > + .vidioc_create_bufs = vb2_ioctl_create_bufs, > + .vidioc_streamon = vb2_ioctl_streamon, > + .vidioc_streamoff = vb2_ioctl_streamoff, > + .vidioc_enum_input = video_enum_input, > + .vidioc_g_input = video_g_input, > + .vidioc_s_input = video_s_input, > +}; > + > +/* ----------------------------------------------------------------------------- > + * V4L2 file operations > + */ > + > +/* > + * video_init_format - Helper function to initialize format > + * > + * Initialize all pad formats with default values. > + */ > +static int video_init_format(struct file *file, void *fh) > +{ > + struct v4l2_format format; > + > + memset(&format, 0, sizeof(format)); > + format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > + > + return video_s_fmt(file, fh, &format); > +} > + > +static int video_open(struct file *file) > +{ > + struct video_device *vdev = video_devdata(file); > + struct camss_video *video = video_drvdata(file); > + struct camss_video_fh *handle; > + int ret; > + > + handle = kzalloc(sizeof(*handle), GFP_KERNEL); > + if (handle == NULL) > + return -ENOMEM; > + > + v4l2_fh_init(&handle->vfh, video->vdev); > + v4l2_fh_add(&handle->vfh); > + > + handle->video = video; > + file->private_data = &handle->vfh; > + > + ret = v4l2_pipeline_pm_use(&vdev->entity, 1); > + if (ret < 0) { > + dev_err(video->camss->dev, "Failed to power up pipeline\n"); > + goto error_pm_use; > + } > + > + ret = video_init_format(file, &handle->vfh); > + if (ret < 0) { > + dev_err(video->camss->dev, "Failed to init format\n"); > + goto error_init_format; > + } > + > + return 0; > + > +error_init_format: > + v4l2_pipeline_pm_use(&vdev->entity, 0); > + > +error_pm_use: > + v4l2_fh_del(&handle->vfh); You're missing a call to v4l2_fh_exit(). > + kfree(handle); But I would recommend replacing v4l2_fh_del, v4l2_fh_exit and kfree by a single call to v4l2_fh_release(). > + > + return ret; > +} > + > +static int video_release(struct file *file) > +{ > + struct video_device *vdev = video_devdata(file); > + struct camss_video *video = video_drvdata(file); > + struct v4l2_fh *vfh = file->private_data; > + struct camss_video_fh *handle = container_of(vfh, struct camss_video_fh, > + vfh); > + > + vb2_ioctl_streamoff(file, vfh, video->type); Don't call this function, instead call vb2_fop_release(). > + > + v4l2_pipeline_pm_use(&vdev->entity, 0); > + > + v4l2_fh_del(vfh); > + kfree(handle); These two lines can be dropped as well when you use vb2_fop_release. > + file->private_data = NULL; > + > + return 0; > +} > + > +static unsigned int video_poll(struct file *file, > + struct poll_table_struct *wait) > +{ > + struct camss_video *video = video_drvdata(file); > + > + return vb2_poll(&video->vb2_q, file, wait); > +} > + > +static int video_mmap(struct file *file, struct vm_area_struct *vma) > +{ > + struct camss_video *video = video_drvdata(file); > + > + return vb2_mmap(&video->vb2_q, vma); > +} > + > +static const struct v4l2_file_operations msm_vid_fops = { > + .owner = THIS_MODULE, > + .unlocked_ioctl = video_ioctl2, > + .open = video_open, > + .release = video_release, > + .poll = video_poll, > + .mmap = video_mmap, You should be able to use vb2_fop_poll/mmap here instead of rolling your own. I recommend adding vb2_fop_read as well. > +}; > + > +/* ----------------------------------------------------------------------------- > + * CAMSS video core > + */ > + > +int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev, > + const char *name) > +{ > + struct media_pad *pad = &video->pad; > + struct video_device *vdev; > + struct vb2_queue *q; > + int ret; > + > + vdev = video_device_alloc(); > + if (vdev == NULL) { > + dev_err(v4l2_dev->dev, "Failed to allocate video device\n"); > + return -ENOMEM; > + } > + > + video->vdev = vdev; > + > + q = &video->vb2_q; > + q->drv_priv = video; > + q->mem_ops = &vb2_dma_contig_memops; > + q->ops = &msm_video_vb2_q_ops; > + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > + q->io_modes = VB2_MMAP; Add modes VB2_DMABUF and VB2_READ. These are for free, so why not? Especially DMABUF is of course very desirable to have. > + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > + q->buf_struct_size = sizeof(struct camss_buffer); > + q->dev = video->camss->dev; > + ret = vb2_queue_init(q); > + if (ret < 0) { > + dev_err(v4l2_dev->dev, "Failed to init vb2 queue\n"); > + return ret; > + } > + > + pad->flags = MEDIA_PAD_FL_SINK; > + ret = media_entity_pads_init(&vdev->entity, 1, pad); > + if (ret < 0) { > + dev_err(v4l2_dev->dev, "Failed to init video entity\n"); > + goto error_media_init; > + } > + > + vdev->fops = &msm_vid_fops; > + vdev->ioctl_ops = &msm_vid_ioctl_ops; > + vdev->release = video_device_release; > + vdev->v4l2_dev = v4l2_dev; > + vdev->vfl_dir = VFL_DIR_RX; > + vdev->queue = &video->vb2_q; As mentioned in querycap: set vdev->device_caps here. > + strlcpy(vdev->name, name, sizeof(vdev->name)); > + > + ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1); > + if (ret < 0) { > + dev_err(v4l2_dev->dev, "Failed to register video device\n"); > + goto error_video_register; > + } > + > + video_set_drvdata(vdev, video); > + > + return 0; > + > +error_video_register: > + media_entity_cleanup(&vdev->entity); > +error_media_init: > + vb2_queue_release(&video->vb2_q); > + > + return ret; > +} > + > +void msm_video_unregister(struct camss_video *video) > +{ > + video_unregister_device(video->vdev); > + media_entity_cleanup(&video->vdev->entity); > + vb2_queue_release(&video->vb2_q); > +} > diff --git a/drivers/media/platform/qcom/camss-8x16/video.h b/drivers/media/platform/qcom/camss-8x16/video.h > new file mode 100644 > index 0000000..5ab5929d > --- /dev/null > +++ b/drivers/media/platform/qcom/camss-8x16/video.h > @@ -0,0 +1,67 @@ > +/* > + * video.h > + * > + * Qualcomm MSM Camera Subsystem - V4L2 device node > + * > + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2015-2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#ifndef QC_MSM_CAMSS_VIDEO_H > +#define QC_MSM_CAMSS_VIDEO_H > + > +#include <linux/videodev2.h> > +#include <media/media-entity.h> > +#include <media/v4l2-dev.h> > +#include <media/v4l2-device.h> > +#include <media/v4l2-fh.h> > +#include <media/v4l2-mediabus.h> > +#include <media/videobuf2-v4l2.h> > + > +#define camss_video_call(f, op, args...) \ > + (!(f) ? -ENODEV : (((f)->ops && (f)->ops->op) ? \ > + (f)->ops->op((f), ##args) : -ENOIOCTLCMD)) > + > +struct camss_buffer { > + struct vb2_v4l2_buffer vb; > + dma_addr_t addr; > + struct list_head queue; > +}; > + > +struct camss_video; > + > +struct camss_video_ops { > + int (*queue_buffer)(struct camss_video *vid, struct camss_buffer *buf); > + int (*flush_buffers)(struct camss_video *vid); > +}; > + > +struct camss_video { > + struct camss *camss; > + struct vb2_queue vb2_q; > + struct video_device *vdev; > + struct media_pad pad; > + struct v4l2_format active_fmt; > + enum v4l2_buf_type type; > + struct media_pipeline pipe; > + struct camss_video_ops *ops; > +}; > + > +struct camss_video_fh { > + struct v4l2_fh vfh; > + struct camss_video *video; > +}; > + > +int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev, > + const char *name); > + > +void msm_video_unregister(struct camss_video *video); > + > +#endif /* QC_MSM_CAMSS_VIDEO_H */ > Regards, Hans
Hi Hans, On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote: > > +static int video_querycap(struct file *file, void *fh, > > + struct v4l2_capability *cap) > > +{ > > + strlcpy(cap->driver, "qcom-camss", sizeof(cap->driver)); > > + strlcpy(cap->card, "Qualcomm Camera Subsystem", sizeof(cap->card)); > > + strlcpy(cap->bus_info, "platform:qcom-camss", > > sizeof(cap->bus_info)); > > + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | > > + V4L2_CAP_DEVICE_CAPS > > ; > > + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; > > Don't set capabilities and device_caps here. Instead fill in the struct > video_device device_caps field and the v4l2 core will take care of > cap->capabilities and cap->device_caps. Time to add this to checkpatch.pl ? :-) > > + > > + return 0; > > +} -- Regards, Laurent Pinchart
Hi Todor, Thank you for the patch. On Friday 25 Nov 2016 16:57:20 Todor Tomov wrote: > These files handle the video device nodes of the camss driver. camss is a quite generic, I'm a bit concerned about claiming that acronym in the global kernel namespace. Would it be too long if we prefixed symbols with msm_camss instead ? > Signed-off-by: Todor Tomov <todor.tomov@linaro.org> > --- > drivers/media/platform/qcom/camss-8x16/video.c | 597 ++++++++++++++++++++++ > drivers/media/platform/qcom/camss-8x16/video.h | 67 +++ > 2 files changed, 664 insertions(+) > create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c > create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h > > diff --git a/drivers/media/platform/qcom/camss-8x16/video.c > b/drivers/media/platform/qcom/camss-8x16/video.c new file mode 100644 > index 0000000..0bf8ea9 > --- /dev/null > +++ b/drivers/media/platform/qcom/camss-8x16/video.c > @@ -0,0 +1,597 @@ > +/* > + * video.c > + * > + * Qualcomm MSM Camera Subsystem - V4L2 device node > + * > + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2015-2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#include <media/media-entity.h> > +#include <media/v4l2-dev.h> > +#include <media/v4l2-device.h> > +#include <media/v4l2-ioctl.h> > +#include <media/v4l2-mc.h> > +#include <media/videobuf-core.h> > +#include <media/videobuf2-dma-contig.h> > + > +#include "video.h" > +#include "camss.h" > + > +/* > + * struct format_info - ISP media bus format information > + * @code: V4L2 media bus format code > + * @pixelformat: V4L2 pixel format FCC identifier > + * @bpp: Bits per pixel when stored in memory > + */ > +static const struct format_info { > + u32 code; > + u32 pixelformat; > + unsigned int bpp; > +} formats[] = { > + { MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY, 16 }, > + { MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY, 16 }, > + { MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV, 16 }, > + { MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU, 16 }, > + { MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8 }, > + { MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8 }, > + { MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8 }, > + { MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8 }, > + { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P, 10 }, > + { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P, 10 }, > + { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P, 10 }, > + { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P, 10 }, > + { MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 }, > + { MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P, 12 }, > + { MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P, 12 }, > + { MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 } > +}; > + > +/* > --------------------------------------------------------------------------- > -- + * Helper functions > + */ > + > +/* > + * video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format > + * @mbus: v4l2_mbus_framefmt format (input) > + * @pix: v4l2_pix_format format (output) > + * > + * Fill the output pix structure with information from the input mbus > format. > + * > + * Return 0 on success or a negative error code otherwise > + */ > +static unsigned int video_mbus_to_pix(const struct v4l2_mbus_framefmt > *mbus, > + struct v4l2_pix_format *pix) > +{ > + unsigned int i; > + > + memset(pix, 0, sizeof(*pix)); > + pix->width = mbus->width; > + pix->height = mbus->height; > + > + for (i = 0; i < ARRAY_SIZE(formats); ++i) { > + if (formats[i].code == mbus->code) > + break; > + } > + > + if (WARN_ON(i == ARRAY_SIZE(formats))) > + return -EINVAL; > + > + pix->pixelformat = formats[i].pixelformat; > + pix->bytesperline = pix->width * formats[i].bpp / 8; > + pix->bytesperline = ALIGN(pix->bytesperline, 8); Does the hardware support padding at the end of lines ? If so it would be useful to use the maximum of the computed and requested by userspace values (up to the maximum size of the padding supported by the hardware). > + pix->sizeimage = pix->bytesperline * pix->height; Similarly, should we support padding at the end of the image ? > + pix->colorspace = mbus->colorspace; > + pix->field = mbus->field; > + > + return 0; > +} > + > +static struct v4l2_subdev *video_remote_subdev(struct camss_video *video, > + u32 *pad) > +{ > + struct media_pad *remote; > + > + remote = media_entity_remote_pad(&video->pad); > + > + if (!remote || !is_media_entity_v4l2_subdev(remote->entity)) > + return NULL; > + > + if (pad) > + *pad = remote->index; > + > + return media_entity_to_v4l2_subdev(remote->entity); > +} > + > +static int video_get_subdev_format(struct camss_video *video, > + struct v4l2_format *format) > +{ > + struct v4l2_subdev_format fmt; > + struct v4l2_subdev *subdev; > + u32 pad; > + int ret; > + > + subdev = video_remote_subdev(video, &pad); > + if (subdev == NULL) > + return -EINVAL; > + > + fmt.pad = pad; > + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + > + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); > + if (ret) > + return ret; > + > + format->type = video->type; > + return video_mbus_to_pix(&fmt.format, &format->fmt.pix); > +} > + > +/* ------------------------------------------------------------------------ > + * Video queue operations > + */ > + > +static int video_queue_setup(struct vb2_queue *q, > + unsigned int *num_buffers, unsigned int *num_planes, > + unsigned int sizes[], struct device *alloc_devs[]) > +{ > + struct camss_video *video = vb2_get_drv_priv(q); > + > + if (*num_planes) { > + if (*num_planes != 1) > + return -EINVAL; > + > + if (sizes[0] < video->active_fmt.fmt.pix.sizeimage) > + return -EINVAL; > + > + return 0; > + } > + > + *num_planes = 1; > + > + sizes[0] = video->active_fmt.fmt.pix.sizeimage; > + > + return 0; > +} > + > +static int video_buf_prepare(struct vb2_buffer *vb) > +{ > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct camss_video *video = vb2_get_drv_priv(vb->vb2_queue); > + struct camss_buffer *buffer = container_of(vbuf, struct camss_buffer, > + vb); You can define a static inline function to wrap this container_of() as it's used in multiple places in this file. > + > + vb2_set_plane_payload(vb, 0, video->active_fmt.fmt.pix.sizeimage); > + if (vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0)) Wouldn't it be more efficient to compare video->active_fmt.fmt.pix.sizeimage instead of vb2_get_plane_payload(vb, 0) ? The two should be identical as you've just called vb2_set_plane_payload(). I would also move the vb2_set_plane_payload() call after the error check. > + return -EINVAL; > + > + vbuf->field = V4L2_FIELD_NONE; > + > + buffer->addr = vb2_dma_contig_plane_dma_addr(vb, 0); > + > + return 0; > +} > + > +static void video_buf_queue(struct vb2_buffer *vb) > +{ > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct camss_video *video = vb2_get_drv_priv(vb->vb2_queue); > + struct camss_buffer *buffer = container_of(vbuf, struct camss_buffer, > + vb); > + > + camss_video_call(video, queue_buffer, buffer); Should I assume that this abstraction means you'll add support for more than the RDI in the future ? ;-) I'd remove the camss_video_call() macro and use video->ops->queue_buffer(video, buffer); directly as it's easier to follow, and we don't need to test video->ops && video->ops->op as all operations should always be provided. > +} > + > + A single blank line is enough. > +static int video_check_format(struct camss_video *video) > +{ > + struct v4l2_pix_format *pix = &video->active_fmt.fmt.pix; > + struct v4l2_format format; > + int ret; > + > + ret = video_get_subdev_format(video, &format); > + if (ret < 0) > + return ret; > + > + if (pix->pixelformat != format.fmt.pix.pixelformat || > + pix->height != format.fmt.pix.height || > + pix->width != format.fmt.pix.width || > + pix->bytesperline != format.fmt.pix.bytesperline || > + pix->sizeimage != format.fmt.pix.sizeimage || > + pix->field != format.fmt.pix.field) > + return -EINVAL; > + > + return 0; > +} > + > +static int video_start_streaming(struct vb2_queue *q, unsigned int count) > +{ > + struct camss_video *video = vb2_get_drv_priv(q); > + struct video_device *vdev = video->vdev; > + struct media_entity *entity; > + struct media_pad *pad; > + struct v4l2_subdev *subdev; > + int ret; > + > + ret = media_entity_pipeline_start(&vdev->entity, &video->pipe); > + if (ret < 0) > + return ret; > + > + ret = video_check_format(video); > + if (ret < 0) > + goto error; > + > + entity = &vdev->entity; > + while (1) { > + pad = &entity->pads[0]; > + if (!(pad->flags & MEDIA_PAD_FL_SINK)) > + break; > + > + pad = media_entity_remote_pad(pad); > + if (!pad || !is_media_entity_v4l2_subdev(pad->entity)) > + break; > + > + entity = pad->entity; > + subdev = media_entity_to_v4l2_subdev(entity); > + > + ret = v4l2_subdev_call(subdev, video, s_stream, 1); > + if (ret < 0 && ret != -ENOIOCTLCMD) > + goto error; > + } > + > + return 0; > + > +error: > + media_entity_pipeline_stop(&vdev->entity); > + > + return ret; > +} > + > +static void video_stop_streaming(struct vb2_queue *q) > +{ > + struct camss_video *video = vb2_get_drv_priv(q); > + struct video_device *vdev = video->vdev; > + struct media_entity *entity; > + struct media_pad *pad; > + struct v4l2_subdev *subdev; > + struct v4l2_subdev *subdev_vfe; > + > + entity = &vdev->entity; > + while (1) { > + pad = &entity->pads[0]; > + if (!(pad->flags & MEDIA_PAD_FL_SINK)) > + break; > + > + pad = media_entity_remote_pad(pad); > + if (!pad || !is_media_entity_v4l2_subdev(pad->entity)) > + break; > + > + entity = pad->entity; > + subdev = media_entity_to_v4l2_subdev(entity); > + > + if (strstr(subdev->name, "vfe")) { > + subdev_vfe = subdev; > + } else if (strstr(subdev->name, "ispif")) { > + v4l2_subdev_call(subdev, video, s_stream, 0); > + v4l2_subdev_call(subdev_vfe, video, s_stream, 0); > + } else { > + v4l2_subdev_call(subdev, video, s_stream, 0); > + } > + } > + > + media_entity_pipeline_stop(&vdev->entity); > + > + camss_video_call(video, flush_buffers); > +} > + > +static struct vb2_ops msm_video_vb2_q_ops = { You can make this static const. > + .queue_setup = video_queue_setup, > + .buf_prepare = video_buf_prepare, > + .buf_queue = video_buf_queue, > + .start_streaming = video_start_streaming, > + .stop_streaming = video_stop_streaming, > +}; > + > +/* ------------------------------------------------------------------------ > + * V4L2 ioctls > + */ > + > +static int video_querycap(struct file *file, void *fh, > + struct v4l2_capability *cap) > +{ > + strlcpy(cap->driver, "qcom-camss", sizeof(cap->driver)); > + strlcpy(cap->card, "Qualcomm Camera Subsystem", sizeof(cap->card)); > + strlcpy(cap->bus_info, "platform:qcom-camss", sizeof(cap->bus_info)); snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", dev_name(...)); would be better. > + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | > + V4L2_CAP_DEVICE_CAPS; > + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; > + > + return 0; > +} > + > +static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc > *f) > +{ > + struct camss_video *video = video_drvdata(file); > + struct v4l2_format format; > + int ret; > + > + if (f->type != video->type) > + return -EINVAL; > + > + if (f->index) > + return -EINVAL; > + > + ret = video_get_subdev_format(video, &format); > + if (ret < 0) > + return ret; > + > + f->pixelformat = format.fmt.pix.pixelformat; > + > + return 0; > +} > + > +static int video_g_fmt(struct file *file, void *fh, struct v4l2_format *f) > +{ > + struct camss_video *video = video_drvdata(file); > + > + if (f->type != video->type) > + return -EINVAL; > + > + *f = video->active_fmt; > + > + return 0; > +} > + > +static int video_s_fmt(struct file *file, void *fh, struct v4l2_format *f) > +{ > + struct camss_video *video = video_drvdata(file); > + int ret; > + > + if (f->type != video->type) > + return -EINVAL; > + > + ret = video_get_subdev_format(video, f); > + if (ret < 0) > + return ret; > + > + video->active_fmt = *f; > + > + return 0; > +} > + > +static int video_try_fmt(struct file *file, void *fh, struct v4l2_format > *f) +{ > + struct camss_video *video = video_drvdata(file); > + > + if (f->type != video->type) > + return -EINVAL; > + > + return video_get_subdev_format(video, f); > +} > + > +static int video_enum_input(struct file *file, void *fh, > + struct v4l2_input *input) > +{ > + if (input->index > 0) > + return -EINVAL; > + > + strlcpy(input->name, "camera", sizeof(input->name)); > + input->type = V4L2_INPUT_TYPE_CAMERA; > + > + return 0; > +} > + > +static int video_g_input(struct file *file, void *fh, unsigned int *input) > +{ > + *input = 0; > + > + return 0; > +} > + > +static int video_s_input(struct file *file, void *fh, unsigned int input) > +{ > + return input == 0 ? 0 : -EINVAL; > +} > + > +static const struct v4l2_ioctl_ops msm_vid_ioctl_ops = { > + .vidioc_querycap = video_querycap, > + .vidioc_enum_fmt_vid_cap = video_enum_fmt, > + .vidioc_g_fmt_vid_cap = video_g_fmt, > + .vidioc_s_fmt_vid_cap = video_s_fmt, > + .vidioc_try_fmt_vid_cap = video_try_fmt, > + .vidioc_reqbufs = vb2_ioctl_reqbufs, > + .vidioc_querybuf = vb2_ioctl_querybuf, > + .vidioc_qbuf = vb2_ioctl_qbuf, > + .vidioc_dqbuf = vb2_ioctl_dqbuf, > + .vidioc_create_bufs = vb2_ioctl_create_bufs, > + .vidioc_streamon = vb2_ioctl_streamon, > + .vidioc_streamoff = vb2_ioctl_streamoff, > + .vidioc_enum_input = video_enum_input, > + .vidioc_g_input = video_g_input, > + .vidioc_s_input = video_s_input, > +}; > + > +/* ------------------------------------------------------------------------ > + * V4L2 file operations > + */ > + > +/* > + * video_init_format - Helper function to initialize format > + * > + * Initialize all pad formats with default values. > + */ > +static int video_init_format(struct file *file, void *fh) > +{ > + struct v4l2_format format; > + > + memset(&format, 0, sizeof(format)); > + format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > + > + return video_s_fmt(file, fh, &format); This will set the active format every time you open the device node, I don't think that's what you want. > +} > + > +static int video_open(struct file *file) > +{ > + struct video_device *vdev = video_devdata(file); > + struct camss_video *video = video_drvdata(file); > + struct camss_video_fh *handle; > + int ret; > + > + handle = kzalloc(sizeof(*handle), GFP_KERNEL); > + if (handle == NULL) > + return -ENOMEM; > + > + v4l2_fh_init(&handle->vfh, video->vdev); > + v4l2_fh_add(&handle->vfh); > + > + handle->video = video; > + file->private_data = &handle->vfh; > + > + ret = v4l2_pipeline_pm_use(&vdev->entity, 1); > + if (ret < 0) { > + dev_err(video->camss->dev, "Failed to power up pipeline\n"); > + goto error_pm_use; > + } > + > + ret = video_init_format(file, &handle->vfh); > + if (ret < 0) { > + dev_err(video->camss->dev, "Failed to init format\n"); > + goto error_init_format; > + } > + > + return 0; > + > +error_init_format: > + v4l2_pipeline_pm_use(&vdev->entity, 0); > + > +error_pm_use: > + v4l2_fh_del(&handle->vfh); > + kfree(handle); > + > + return ret; > +} > + > +static int video_release(struct file *file) > +{ > + struct video_device *vdev = video_devdata(file); > + struct camss_video *video = video_drvdata(file); > + struct v4l2_fh *vfh = file->private_data; > + struct camss_video_fh *handle = container_of(vfh, struct camss_video_fh, > + vfh); > + > + vb2_ioctl_streamoff(file, vfh, video->type); > + > + v4l2_pipeline_pm_use(&vdev->entity, 0); > + > + v4l2_fh_del(vfh); > + kfree(handle); > + file->private_data = NULL; > + > + return 0; > +} > + > +static unsigned int video_poll(struct file *file, > + struct poll_table_struct *wait) > +{ > + struct camss_video *video = video_drvdata(file); > + > + return vb2_poll(&video->vb2_q, file, wait); > +} > + > +static int video_mmap(struct file *file, struct vm_area_struct *vma) > +{ > + struct camss_video *video = video_drvdata(file); > + > + return vb2_mmap(&video->vb2_q, vma); > +} > + > +static const struct v4l2_file_operations msm_vid_fops = { > + .owner = THIS_MODULE, > + .unlocked_ioctl = video_ioctl2, > + .open = video_open, > + .release = video_release, > + .poll = video_poll, > + .mmap = video_mmap, > +}; > + > +/* ------------------------------------------------------------------------ > + * CAMSS video core > + */ > + > +int msm_video_register(struct camss_video *video, struct v4l2_device > *v4l2_dev, > + const char *name) > +{ > + struct media_pad *pad = &video->pad; > + struct video_device *vdev; > + struct vb2_queue *q; > + int ret; > + > + vdev = video_device_alloc(); > + if (vdev == NULL) { > + dev_err(v4l2_dev->dev, "Failed to allocate video device\n"); > + return -ENOMEM; > + } > + > + video->vdev = vdev; > + > + q = &video->vb2_q; > + q->drv_priv = video; > + q->mem_ops = &vb2_dma_contig_memops; > + q->ops = &msm_video_vb2_q_ops; > + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > + q->io_modes = VB2_MMAP; > + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > + q->buf_struct_size = sizeof(struct camss_buffer); > + q->dev = video->camss->dev; > + ret = vb2_queue_init(q); > + if (ret < 0) { > + dev_err(v4l2_dev->dev, "Failed to init vb2 queue\n"); > + return ret; > + } > + > + pad->flags = MEDIA_PAD_FL_SINK; > + ret = media_entity_pads_init(&vdev->entity, 1, pad); > + if (ret < 0) { > + dev_err(v4l2_dev->dev, "Failed to init video entity\n"); > + goto error_media_init; > + } > + > + vdev->fops = &msm_vid_fops; > + vdev->ioctl_ops = &msm_vid_ioctl_ops; > + vdev->release = video_device_release; This will result in a race condition between device unbind and userspace access to the video node. You should instead refcount the top-level camss data structure, and provide a custom release function here that decrements the refcount. If you do that you can embed your struct video_device in struct camss_video instead of allocating it dynamically. The media_entity_cleanup() call will have to move from msm_video_unregister() to the release operation for your top-level structure. To test this race condition, perform the following steps: - Configure the pipeline - Open the video device node, configure it and start streaming - Unbind the device through the sysfs unbind attribute of the driver - Stop streaming You'll likely get an oops with you current version. There are unfortunately very few drivers implementing this correctly (including drivers I wrote :-/). A good (even if slightly complex) example is uvcvideo. > + vdev->v4l2_dev = v4l2_dev; > + vdev->vfl_dir = VFL_DIR_RX; > + vdev->queue = &video->vb2_q; > + strlcpy(vdev->name, name, sizeof(vdev->name)); > + > + ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1); > + if (ret < 0) { > + dev_err(v4l2_dev->dev, "Failed to register video device\n"); > + goto error_video_register; > + } > + > + video_set_drvdata(vdev, video); > + > + return 0; > + > +error_video_register: > + media_entity_cleanup(&vdev->entity); > +error_media_init: > + vb2_queue_release(&video->vb2_q); > + > + return ret; > +} > + > +void msm_video_unregister(struct camss_video *video) > +{ > + video_unregister_device(video->vdev); > + media_entity_cleanup(&video->vdev->entity); > + vb2_queue_release(&video->vb2_q); If you use vb2_fop_release() as Hans proposed you can remove the vb2_queue_release() call here. > +} > diff --git a/drivers/media/platform/qcom/camss-8x16/video.h > b/drivers/media/platform/qcom/camss-8x16/video.h new file mode 100644 > index 0000000..5ab5929d > --- /dev/null > +++ b/drivers/media/platform/qcom/camss-8x16/video.h > @@ -0,0 +1,67 @@ > +/* > + * video.h > + * > + * Qualcomm MSM Camera Subsystem - V4L2 device node > + * > + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2015-2016 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#ifndef QC_MSM_CAMSS_VIDEO_H > +#define QC_MSM_CAMSS_VIDEO_H > + > +#include <linux/videodev2.h> > +#include <media/media-entity.h> > +#include <media/v4l2-dev.h> > +#include <media/v4l2-device.h> > +#include <media/v4l2-fh.h> > +#include <media/v4l2-mediabus.h> > +#include <media/videobuf2-v4l2.h> > + > +#define camss_video_call(f, op, args...) \ > + (!(f) ? -ENODEV : (((f)->ops && (f)->ops->op) ? \ > + (f)->ops->op((f), ##args) : -ENOIOCTLCMD)) > + > +struct camss_buffer { > + struct vb2_v4l2_buffer vb; > + dma_addr_t addr; > + struct list_head queue; > +}; > + > +struct camss_video; > + > +struct camss_video_ops { > + int (*queue_buffer)(struct camss_video *vid, struct camss_buffer *buf); > + int (*flush_buffers)(struct camss_video *vid); > +}; > + > +struct camss_video { > + struct camss *camss; > + struct vb2_queue vb2_q; > + struct video_device *vdev; > + struct media_pad pad; > + struct v4l2_format active_fmt; > + enum v4l2_buf_type type; > + struct media_pipeline pipe; > + struct camss_video_ops *ops; You can make this const. > +}; > + > +struct camss_video_fh { > + struct v4l2_fh vfh; > + struct camss_video *video; > +}; If there's nothing else to be stored here you don't need a custom struct camss_video_fh. You can use struct v4l2_fh and retrieve the camss_video instance with a container_of on vfh->vdev (provided you embed the video_device instance in struct camss_video as I proposed above). > + > +int msm_video_register(struct camss_video *video, struct v4l2_device > *v4l2_dev, > + const char *name); > + > +void msm_video_unregister(struct camss_video *video); > + > +#endif /* QC_MSM_CAMSS_VIDEO_H */ -- Regards, Laurent Pinchart
Hi Hans, On Monday 05 Dec 2016 16:02:55 Hans Verkuil wrote: > On 12/05/2016 03:45 PM, Laurent Pinchart wrote: > > On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote: > >> On 11/25/2016 03:57 PM, Todor Tomov wrote: > >>> These files handle the video device nodes of the camss driver. > >>> > >>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org> > >>> --- > >>> > >>> drivers/media/platform/qcom/camss-8x16/video.c | 597 +++++++++++++++++ > >>> drivers/media/platform/qcom/camss-8x16/video.h | 67 +++ > >>> 2 files changed, 664 insertions(+) > >>> create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c > >>> create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h > > > > [snip] > > > >>> +int msm_video_register(struct camss_video *video, struct v4l2_device > >>> *v4l2_dev, > >>> + const char *name) > >>> +{ > >>> + struct media_pad *pad = &video->pad; > >>> + struct video_device *vdev; > >>> + struct vb2_queue *q; > >>> + int ret; > >>> + > >>> + vdev = video_device_alloc(); > >>> + if (vdev == NULL) { > >>> + dev_err(v4l2_dev->dev, "Failed to allocate video > >>> device\n"); > >>> + return -ENOMEM; > >>> + } > >>> + > >>> + video->vdev = vdev; > >>> + > >>> + q = &video->vb2_q; > >>> + q->drv_priv = video; > >>> + q->mem_ops = &vb2_dma_contig_memops; > >>> + q->ops = &msm_video_vb2_q_ops; > >>> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > >>> + q->io_modes = VB2_MMAP; > >> > >> Add modes VB2_DMABUF and VB2_READ. These are for free, so why not? > >> Especially DMABUF is of course very desirable to have. > > > > I certainly agree with VB2_DMABUF, but I wouldn't expose VB2_READ. read() > > for this kind of device is inefficient and we should encourage userspace > > application to move away from it (and certainly make it very clear that > > new applications should not use read() with this device). > > VB2_READ is very nice when debugging (no need to program a stream I/O > application first) There's at least v4l2-ctl and yavta that can (and should) be used for debugging ;-) > and useful when you want to pipe the video. Except that you lose frame boundaries. It's really a poor man's solution that should never be used in any "real" application. I'd rather add an option to v4l2-ctl to output the video stream to stdout (and/or stderr). > Nobody uses read() in 'regular' applications since it is obviously > inefficient, but I don't see that as a reason not to offer VB2_READ. > > I don't think this is something anyone will ever abuse, Famous last words ;-) > and it is a nice feature to have (and for free as well). -- Regards, Laurent Pinchart
Hi Hans, Thank you for the extensive review. On 12/05/2016 03:44 PM, Hans Verkuil wrote: > A few comments below: > > On 11/25/2016 03:57 PM, Todor Tomov wrote: >> These files handle the video device nodes of the camss driver. >> >> Signed-off-by: Todor Tomov <todor.tomov@linaro.org> >> --- >> drivers/media/platform/qcom/camss-8x16/video.c | 597 +++++++++++++++++++++++++ >> drivers/media/platform/qcom/camss-8x16/video.h | 67 +++ >> 2 files changed, 664 insertions(+) >> create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c >> create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h >> >> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c b/drivers/media/platform/qcom/camss-8x16/video.c >> new file mode 100644 >> index 0000000..0bf8ea9 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/camss-8x16/video.c >> @@ -0,0 +1,597 @@ > > <snip> > >> +static int video_start_streaming(struct vb2_queue *q, unsigned int count) >> +{ >> + struct camss_video *video = vb2_get_drv_priv(q); >> + struct video_device *vdev = video->vdev; >> + struct media_entity *entity; >> + struct media_pad *pad; >> + struct v4l2_subdev *subdev; >> + int ret; >> + >> + ret = media_entity_pipeline_start(&vdev->entity, &video->pipe); >> + if (ret < 0) >> + return ret; >> + >> + ret = video_check_format(video); >> + if (ret < 0) >> + goto error; >> + >> + entity = &vdev->entity; >> + while (1) { >> + pad = &entity->pads[0]; >> + if (!(pad->flags & MEDIA_PAD_FL_SINK)) >> + break; >> + >> + pad = media_entity_remote_pad(pad); >> + if (!pad || !is_media_entity_v4l2_subdev(pad->entity)) >> + break; >> + >> + entity = pad->entity; >> + subdev = media_entity_to_v4l2_subdev(entity); >> + >> + ret = v4l2_subdev_call(subdev, video, s_stream, 1); >> + if (ret < 0 && ret != -ENOIOCTLCMD) >> + goto error; >> + } >> + >> + return 0; >> + >> +error: >> + media_entity_pipeline_stop(&vdev->entity); >> + > > On error all queued buffers must be returned with state VB2_STATE_QUEUED. > > Basically the same as 'camss_video_call(video, flush_buffers);', just with > a different state. Ok, I'll add this. > >> + return ret; >> +} > > <snip> > >> +static int video_querycap(struct file *file, void *fh, >> + struct v4l2_capability *cap) >> +{ >> + strlcpy(cap->driver, "qcom-camss", sizeof(cap->driver)); >> + strlcpy(cap->card, "Qualcomm Camera Subsystem", sizeof(cap->card)); >> + strlcpy(cap->bus_info, "platform:qcom-camss", sizeof(cap->bus_info)); >> + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | >> + V4L2_CAP_DEVICE_CAPS; >> + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; > > Don't set capabilities and device_caps here. Instead fill in the struct video_device > device_caps field and the v4l2 core will take care of cap->capabilities and > cap->device_caps. Ok. > >> + >> + return 0; >> +} >> + >> +static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f) >> +{ >> + struct camss_video *video = video_drvdata(file); >> + struct v4l2_format format; >> + int ret; >> + >> + if (f->type != video->type) >> + return -EINVAL; >> + >> + if (f->index) >> + return -EINVAL; >> + >> + ret = video_get_subdev_format(video, &format); >> + if (ret < 0) >> + return ret; >> + >> + f->pixelformat = format.fmt.pix.pixelformat; >> + >> + return 0; >> +} >> + >> +static int video_g_fmt(struct file *file, void *fh, struct v4l2_format *f) >> +{ >> + struct camss_video *video = video_drvdata(file); >> + >> + if (f->type != video->type) >> + return -EINVAL; >> + >> + *f = video->active_fmt; >> + >> + return 0; >> +} >> + >> +static int video_s_fmt(struct file *file, void *fh, struct v4l2_format *f) >> +{ >> + struct camss_video *video = video_drvdata(file); >> + int ret; >> + >> + if (f->type != video->type) >> + return -EINVAL; >> + >> + ret = video_get_subdev_format(video, f); >> + if (ret < 0) >> + return ret; >> + >> + video->active_fmt = *f; >> + >> + return 0; >> +} >> + >> +static int video_try_fmt(struct file *file, void *fh, struct v4l2_format *f) >> +{ >> + struct camss_video *video = video_drvdata(file); >> + >> + if (f->type != video->type) >> + return -EINVAL; >> + >> + return video_get_subdev_format(video, f); >> +} >> + >> +static int video_enum_input(struct file *file, void *fh, >> + struct v4l2_input *input) >> +{ >> + if (input->index > 0) >> + return -EINVAL; >> + >> + strlcpy(input->name, "camera", sizeof(input->name)); >> + input->type = V4L2_INPUT_TYPE_CAMERA; >> + >> + return 0; >> +} >> + >> +static int video_g_input(struct file *file, void *fh, unsigned int *input) >> +{ >> + *input = 0; >> + >> + return 0; >> +} >> + >> +static int video_s_input(struct file *file, void *fh, unsigned int input) >> +{ >> + return input == 0 ? 0 : -EINVAL; >> +} >> + >> +static const struct v4l2_ioctl_ops msm_vid_ioctl_ops = { >> + .vidioc_querycap = video_querycap, >> + .vidioc_enum_fmt_vid_cap = video_enum_fmt, >> + .vidioc_g_fmt_vid_cap = video_g_fmt, >> + .vidioc_s_fmt_vid_cap = video_s_fmt, >> + .vidioc_try_fmt_vid_cap = video_try_fmt, >> + .vidioc_reqbufs = vb2_ioctl_reqbufs, >> + .vidioc_querybuf = vb2_ioctl_querybuf, >> + .vidioc_qbuf = vb2_ioctl_qbuf, >> + .vidioc_dqbuf = vb2_ioctl_dqbuf, >> + .vidioc_create_bufs = vb2_ioctl_create_bufs, >> + .vidioc_streamon = vb2_ioctl_streamon, >> + .vidioc_streamoff = vb2_ioctl_streamoff, >> + .vidioc_enum_input = video_enum_input, >> + .vidioc_g_input = video_g_input, >> + .vidioc_s_input = video_s_input, >> +}; >> + >> +/* ----------------------------------------------------------------------------- >> + * V4L2 file operations >> + */ >> + >> +/* >> + * video_init_format - Helper function to initialize format >> + * >> + * Initialize all pad formats with default values. >> + */ >> +static int video_init_format(struct file *file, void *fh) >> +{ >> + struct v4l2_format format; >> + >> + memset(&format, 0, sizeof(format)); >> + format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> + >> + return video_s_fmt(file, fh, &format); >> +} >> + >> +static int video_open(struct file *file) >> +{ >> + struct video_device *vdev = video_devdata(file); >> + struct camss_video *video = video_drvdata(file); >> + struct camss_video_fh *handle; >> + int ret; >> + >> + handle = kzalloc(sizeof(*handle), GFP_KERNEL); >> + if (handle == NULL) >> + return -ENOMEM; >> + >> + v4l2_fh_init(&handle->vfh, video->vdev); >> + v4l2_fh_add(&handle->vfh); >> + >> + handle->video = video; >> + file->private_data = &handle->vfh; >> + >> + ret = v4l2_pipeline_pm_use(&vdev->entity, 1); >> + if (ret < 0) { >> + dev_err(video->camss->dev, "Failed to power up pipeline\n"); >> + goto error_pm_use; >> + } >> + >> + ret = video_init_format(file, &handle->vfh); >> + if (ret < 0) { >> + dev_err(video->camss->dev, "Failed to init format\n"); >> + goto error_init_format; >> + } >> + >> + return 0; >> + >> +error_init_format: >> + v4l2_pipeline_pm_use(&vdev->entity, 0); >> + >> +error_pm_use: >> + v4l2_fh_del(&handle->vfh); > > You're missing a call to v4l2_fh_exit(). > >> + kfree(handle); > > But I would recommend replacing v4l2_fh_del, v4l2_fh_exit and kfree by a single > call to v4l2_fh_release(). Ok, I'll switch to v4l2_fh_release(). > >> + >> + return ret; >> +} >> + >> +static int video_release(struct file *file) >> +{ >> + struct video_device *vdev = video_devdata(file); >> + struct camss_video *video = video_drvdata(file); >> + struct v4l2_fh *vfh = file->private_data; >> + struct camss_video_fh *handle = container_of(vfh, struct camss_video_fh, >> + vfh); >> + >> + vb2_ioctl_streamoff(file, vfh, video->type); > > Don't call this function, instead call vb2_fop_release(). Ok. > >> + >> + v4l2_pipeline_pm_use(&vdev->entity, 0); >> + >> + v4l2_fh_del(vfh); >> + kfree(handle); > > These two lines can be dropped as well when you use vb2_fop_release. Ok. > >> + file->private_data = NULL; >> + >> + return 0; >> +} >> + >> +static unsigned int video_poll(struct file *file, >> + struct poll_table_struct *wait) >> +{ >> + struct camss_video *video = video_drvdata(file); >> + >> + return vb2_poll(&video->vb2_q, file, wait); >> +} >> + >> +static int video_mmap(struct file *file, struct vm_area_struct *vma) >> +{ >> + struct camss_video *video = video_drvdata(file); >> + >> + return vb2_mmap(&video->vb2_q, vma); >> +} >> + >> +static const struct v4l2_file_operations msm_vid_fops = { >> + .owner = THIS_MODULE, >> + .unlocked_ioctl = video_ioctl2, >> + .open = video_open, >> + .release = video_release, >> + .poll = video_poll, >> + .mmap = video_mmap, > > You should be able to use vb2_fop_poll/mmap here instead of rolling your own. > I recommend adding vb2_fop_read as well. Ok. Trying to use vb2_fop_poll makes me realize that I probably have to initialize video_device->lock before video_register_device() and maybe vb2_queue->lock too. I'll try with these. <snip> -- Best regards, Todor Tomov
Hi Laurent, Hans, On 12/05/2016 05:25 PM, Laurent Pinchart wrote: > Hi Hans, > > On Monday 05 Dec 2016 16:02:55 Hans Verkuil wrote: >> On 12/05/2016 03:45 PM, Laurent Pinchart wrote: >>> On Monday 05 Dec 2016 14:44:55 Hans Verkuil wrote: >>>> On 11/25/2016 03:57 PM, Todor Tomov wrote: >>>>> These files handle the video device nodes of the camss driver. >>>>> >>>>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org> >>>>> --- >>>>> >>>>> drivers/media/platform/qcom/camss-8x16/video.c | 597 +++++++++++++++++ >>>>> drivers/media/platform/qcom/camss-8x16/video.h | 67 +++ >>>>> 2 files changed, 664 insertions(+) >>>>> create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c >>>>> create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h >>> >>> [snip] >>> >>>>> +int msm_video_register(struct camss_video *video, struct v4l2_device >>>>> *v4l2_dev, >>>>> + const char *name) >>>>> +{ >>>>> + struct media_pad *pad = &video->pad; >>>>> + struct video_device *vdev; >>>>> + struct vb2_queue *q; >>>>> + int ret; >>>>> + >>>>> + vdev = video_device_alloc(); >>>>> + if (vdev == NULL) { >>>>> + dev_err(v4l2_dev->dev, "Failed to allocate video >>>>> device\n"); >>>>> + return -ENOMEM; >>>>> + } >>>>> + >>>>> + video->vdev = vdev; >>>>> + >>>>> + q = &video->vb2_q; >>>>> + q->drv_priv = video; >>>>> + q->mem_ops = &vb2_dma_contig_memops; >>>>> + q->ops = &msm_video_vb2_q_ops; >>>>> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >>>>> + q->io_modes = VB2_MMAP; >>>> >>>> Add modes VB2_DMABUF and VB2_READ. These are for free, so why not? >>>> Especially DMABUF is of course very desirable to have. >>> >>> I certainly agree with VB2_DMABUF, but I wouldn't expose VB2_READ. read() >>> for this kind of device is inefficient and we should encourage userspace >>> application to move away from it (and certainly make it very clear that >>> new applications should not use read() with this device). >> >> VB2_READ is very nice when debugging (no need to program a stream I/O >> application first) > > There's at least v4l2-ctl and yavta that can (and should) be used for > debugging ;-) > >> and useful when you want to pipe the video. > > Except that you lose frame boundaries. It's really a poor man's solution that > should never be used in any "real" application. I'd rather add an option to > v4l2-ctl to output the video stream to stdout (and/or stderr). > >> Nobody uses read() in 'regular' applications since it is obviously >> inefficient, but I don't see that as a reason not to offer VB2_READ. >> >> I don't think this is something anyone will ever abuse, > > Famous last words ;-) > >> and it is a nice feature to have (and for free as well). > Thank you for the discussion over this. Both of you have reasonable arguments about the read i/o. I'd say that you cannot always save a person from himself. I'll add VB2_READ. -- Best regards, Todor Tomov
Hi Laurent, On 01/19/2017 12:43 PM, Todor Tomov wrote: > Hi Laurent, > > Thank you for the detailed review. > > On 12/05/2016 05:22 PM, Laurent Pinchart wrote: >> Hi Todor, >> >> Thank you for the patch. >> >> On Friday 25 Nov 2016 16:57:20 Todor Tomov wrote: >>> These files handle the video device nodes of the camss driver. >> >> camss is a quite generic, I'm a bit concerned about claiming that acronym in >> the global kernel namespace. Would it be too long if we prefixed symbols with >> msm_camss instead ? > > Ok. Are you concerned about camss_enable_clocks() and camss_disable_clocks() or > you have something else in mind too? Could you please add more details about this? > >> >>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org> >>> --- >>> drivers/media/platform/qcom/camss-8x16/video.c | 597 ++++++++++++++++++++++ >>> drivers/media/platform/qcom/camss-8x16/video.h | 67 +++ >>> 2 files changed, 664 insertions(+) >>> create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c >>> create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h >>> >>> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c >>> b/drivers/media/platform/qcom/camss-8x16/video.c new file mode 100644 >>> index 0000000..0bf8ea9 >>> --- /dev/null >>> +++ b/drivers/media/platform/qcom/camss-8x16/video.c <snip> >>> +/* ------------------------------------------------------------------------ >>> + * V4L2 file operations >>> + */ >>> + >>> +/* >>> + * video_init_format - Helper function to initialize format >>> + * >>> + * Initialize all pad formats with default values. >>> + */ >>> +static int video_init_format(struct file *file, void *fh) >>> +{ >>> + struct v4l2_format format; >>> + >>> + memset(&format, 0, sizeof(format)); >>> + format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >>> + >>> + return video_s_fmt(file, fh, &format); >> >> This will set the active format every time you open the device node, I don't >> think that's what you want. > > Well, actually this is what I wanted. I wanted to keep in sync the pixel format > on the video node and the media bus format on the subdev node (i.e. the pixel > format will be always correct for the current media bus format). For the current > version there is a direct correspondence between the pixel format and the media > format so this will work I think. For the future there might be multiple pixel > formats for one media bus format and a second open of the video node could reset > the pixel format to unwanted value so this will need a change. I'm wondering about > (and still not able to find) a good moment/event when to perform the initialization > of the format on the video node. As it gets the current format from the subdev > node, the moment of the registration will be too early as the media link is still > not created. But after that I couldn't find a suitable callback/event where to do > it. If you can share any idea about this, please do :) I still haven't found a better solution for this. If you have something in mind, please share. > >> >>> +} >>> + >>> +static int video_open(struct file *file) >>> +{ >>> + struct video_device *vdev = video_devdata(file); >>> + struct camss_video *video = video_drvdata(file); >>> + struct camss_video_fh *handle; >>> + int ret; >>> + >>> + handle = kzalloc(sizeof(*handle), GFP_KERNEL); >>> + if (handle == NULL) >>> + return -ENOMEM; >>> + >>> + v4l2_fh_init(&handle->vfh, video->vdev); >>> + v4l2_fh_add(&handle->vfh); >>> + >>> + handle->video = video; >>> + file->private_data = &handle->vfh; >>> + >>> + ret = v4l2_pipeline_pm_use(&vdev->entity, 1); >>> + if (ret < 0) { >>> + dev_err(video->camss->dev, "Failed to power up pipeline\n"); >>> + goto error_pm_use; >>> + } >>> + >>> + ret = video_init_format(file, &handle->vfh); >>> + if (ret < 0) { >>> + dev_err(video->camss->dev, "Failed to init format\n"); >>> + goto error_init_format; >>> + } >>> + >>> + return 0; >>> + >>> +error_init_format: >>> + v4l2_pipeline_pm_use(&vdev->entity, 0); >>> + >>> +error_pm_use: >>> + v4l2_fh_del(&handle->vfh); >>> + kfree(handle); >>> + >>> + return ret; >>> +} -- Best regards, Todor Tomov
diff --git a/MAINTAINERS b/MAINTAINERS index 411e3b8..0740aee 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9971,6 +9971,14 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git S: Supported F: drivers/net/wireless/ath/ath10k/ +QUALCOMM CAMERA SUBSYSTEM DRIVER +M: Todor Tomov <todor.tomov@linaro.org> +L: linux-media@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/media/qcom,camss.txt +F: Documentation/media/v4l-drivers/qcom_camss.rst +F: drivers/media/platform/qcom/camss/ + QUALCOMM EMAC GIGABIT ETHERNET DRIVER M: Timur Tabi <timur@codeaurora.org> L: netdev@vger.kernel.org
Add an entry for Qualcomm Camera subsystem driver. Signed-off-by: Todor Tomov <todor.tomov@linaro.org> --- MAINTAINERS | 8 ++++++++ 1 file changed, 8 insertions(+) -- 1.9.1