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@xxxxxxxxxxxx>
>
> [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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
