linux-kernel - Re: [PATCH v2 2/2] Support for DW CSI-2 Host IPK

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]

Date:   Thu, 12 Jan 2017 19:06:41 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Ramiro Oliveira <Ramiro.Oliveira@...opsys.com>, robh+dt@...nel.org,
        mark.rutland@....com, mchehab@...nel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org
Cc:     davem@...emloft.net, gregkh@...uxfoundation.org,
        geert+renesas@...der.be, akpm@...ux-foundation.org,
        linux@...ck-us.net, laurent.pinchart+renesas@...asonboard.com,
        arnd@...db.de, sudipm.mukherjee@...il.com,
        tiffany.lin@...iatek.com, minghsiu.tsai@...iatek.com,
        jean-christophe.trotin@...com, andrew-ct.chen@...iatek.com,
        simon.horman@...ronome.com, songjun.wu@...rochip.com,
        bparrot@...com, CARLOS.PALMINHA@...opsys.com,
        Sakari Ailus <sakari.ailus@....fi>
Subject: Re: [PATCH v2 2/2] Support for DW CSI-2 Host IPK

On 01/12/2017 06:43 PM, Ramiro Oliveira wrote:
> Hi Hans,
> 
> Thank you for your feedback.
> 
> On 1/11/2017 11:54 AM, Hans Verkuil wrote:
>> Hi Ramiro,
>>
>> See my review comments below:
>>
>> On 12/12/16 16:00, Ramiro Oliveira wrote:
>>> Add support for the DesignWare CSI-2 Host IP Prototyping Kit
>>>
>>> Signed-off-by: Ramiro Oliveira <roliveir@...opsys.com>
> 
> [snip]

>>> +
>>> +static int vid_dev_subdev_s_power(struct v4l2_subdev *sd, int on)
>>> +{
>>> +    return 0;
>>> +}
>>
>> Just drop this empty function, shouldn't be needed.
>>
> 
> When I start my system I'm hoping all the subdevs have s_power registered. If it
> doesn't exist should I change the way I handle it, or will the core handle it
> for me?

If it isn't provided, then it is just skipped. The general rule is that
you only provide these ops if they do something useful.

> 
>>> +
>>> +static int vid_dev_subdev_registered(struct v4l2_subdev *sd)
>>> +{
>>> +    struct video_device_dev *vid_dev = v4l2_get_subdevdata(sd);
>>> +    struct vb2_queue *q = &vid_dev->vb_queue;
>>> +    struct video_device *vfd = &vid_dev->ve.vdev;
>>> +    int ret;
>>> +
>>> +    memset(vfd, 0, sizeof(*vfd));
>>> +
>>> +    strlcpy(vfd->name, VIDEO_DEVICE_NAME, sizeof(vfd->name));
>>> +
>>> +    vfd->fops = &vid_dev_fops;
>>> +    vfd->ioctl_ops = &vid_dev_ioctl_ops;
>>> +    vfd->v4l2_dev = sd->v4l2_dev;
>>> +    vfd->minor = -1;
>>> +    vfd->release = video_device_release_empty;
>>> +    vfd->queue = q;
>>> +
>>> +    INIT_LIST_HEAD(&vid_dev->vidq.active);
>>> +    init_waitqueue_head(&vid_dev->vidq.wq);
>>> +    memset(q, 0, sizeof(*q));
>>> +    q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +    q->io_modes = VB2_MMAP | VB2_USERPTR;
>>
>> Add VB2_DMABUF and VB2_READ.
>>
> 
> I'll add them, but I'm not using them, is it standard procedure to add them all
> even if they aren't used?

You may not use them, but others might. And it doesn't cost anything to add them.

> 
>>> +    q->ops = &vb2_video_qops;
>>> +    q->mem_ops = &vb2_vmalloc_memops;
>>
>> Why is vmalloc used? Can't you use dma_contig or dma_sg and avoid having to copy
>> the image data? That's a really bad design given the amount of video data that
>> you have to copy.
>>
> 
> When I started development, the arch I was using (ARC) didn't support
> dma_contig, so I was forced to use vmalloc.
> 
> Since then things have changed and I'm already using dma_contig, however it
> wasn't included in this patch. I'll add it to the next patch.

Ah, good. If you are switching to dma_contig, then remove VB2_USERPTR.
VB2_DMABUF should be used instead.

Regards,

	Hans