linux-kernel - Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <16480551.5NNPjmTzBo@avalon>
Date:   Mon, 05 Dec 2016 16:45:26 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     Todor Tomov <todor.tomov@...aro.org>, mchehab@...nel.org,
        laurent.pinchart+renesas@...asonboard.com, hans.verkuil@...co.com,
        javier@....samsung.com, s.nawrocki@...sung.com,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        bjorn.andersson@...aro.org, srinivas.kandagatla@...aro.org
Subject: Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

Hello,

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@...aro.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).

> > +     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;
> > +}

-- 
Regards,

Laurent Pinchart