Hi Helen,
Please see my comments below.
Helen Koike wrote:
> On 2017-01-25 11:03 AM, Sakari Ailus wrote:
...
>>> + * the videobuf2 framework will allocate this struct based on
>>> + * buf_struct_size and use the first sizeof(struct vb2_buffer)
>>> bytes of
>>> + * memory as a vb2_buffer
>>> + */
>>> + struct vb2_v4l2_buffer vb2;
>>> + struct list_head list;
>>> +};
>>> +
>>> +static int vimc_cap_querycap(struct file *file, void *priv,
>>> + struct v4l2_capability *cap)
>>> +{
>>> + struct vimc_cap_device *vcap = video_drvdata(file);
>>> +
>>> + strlcpy(cap->driver, KBUILD_MODNAME, sizeof(cap->driver));
>>> + strlcpy(cap->card, KBUILD_MODNAME, sizeof(cap->card));
>>> + snprintf(cap->bus_info, sizeof(cap->bus_info),
>>> + "platform:%s", vcap->v4l2_dev->name);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int vimc_cap_enum_input(struct file *file, void *priv,
>>> + struct v4l2_input *i)
>>> +{
>>> + /* We only have one input */
>>> + if (i->index > 0)
>>> + return -EINVAL;
>>> +
>>> + i->type = V4L2_INPUT_TYPE_CAMERA;
>>> + strlcpy(i->name, "VIMC capture", sizeof(i->name));
>>
>> Isn't this (*INPUT IOCTLs) something that should be handled in a
>> sub-device
>> driver, such as a TV tuner?
>
>
> Can the ioctl VIDIOC_ENUMINPUT enumerate no inputs at all? Can I just
> return -EINVAL here in G_INPUT and S_INPUT as well?
> I thought I had to enumerate at least one input, and between
> V4L2_INPUT_TYPE_TUNER and V4L2_INPUT_TYPE_CAMERA, this last
> one seems more appropriated
I don't think other drivers that provide MC interface do this on video
nodes either. The VIMC driver could know what's connected to it, but
generally that's not the case.
>
>
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int vimc_cap_g_input(struct file *file, void *priv, unsigned
>>> int *i)
>>> +{
>>> + /* We only have one input */
>>> + *i = 0;
>>> + return 0;
>>> +}
>>> +
>>> +static int vimc_cap_s_input(struct file *file, void *priv, unsigned
>>> int i)
>>> +{
>>> + /* We only have one input */
>>> + return i ? -EINVAL : 0;
>>> +}
>>> +
>>> +static int vimc_cap_fmt_vid_cap(struct file *file, void *priv,
>>> + struct v4l2_format *f)
>>> +{
>>> + struct vimc_cap_device *vcap = video_drvdata(file);
>>> +
>>> + f->fmt.pix = vcap->format;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
>>> + struct v4l2_fmtdesc *f)
>>> +{
>>> + struct vimc_cap_device *vcap = video_drvdata(file);
>>> +
>>> + if (f->index > 0)
>>> + return -EINVAL;
>>> +
>>> + /* We only support one format for now */
>>> + f->pixelformat = vcap->format.pixelformat;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct v4l2_file_operations vimc_cap_fops = {
>>> + .owner = THIS_MODULE,
>>> + .open = v4l2_fh_open,
>>> + .release = vb2_fop_release,
>>> + .read = vb2_fop_read,
>>> + .poll = vb2_fop_poll,
>>> + .unlocked_ioctl = video_ioctl2,
>>> + .mmap = vb2_fop_mmap,
>>> +};
>>> +
>>> +static const struct v4l2_ioctl_ops vimc_cap_ioctl_ops = {
>>> + .vidioc_querycap = vimc_cap_querycap,
>>> +
>>> + .vidioc_enum_input = vimc_cap_enum_input,
>>> + .vidioc_g_input = vimc_cap_g_input,
>>> + .vidioc_s_input = vimc_cap_s_input,
>>> +
>>> + .vidioc_g_fmt_vid_cap = vimc_cap_fmt_vid_cap,
>>> + .vidioc_s_fmt_vid_cap = vimc_cap_fmt_vid_cap,
>>> + .vidioc_try_fmt_vid_cap = vimc_cap_fmt_vid_cap,
>>> + .vidioc_enum_fmt_vid_cap = vimc_cap_enum_fmt_vid_cap,
>>> +
>>> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
>>> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
>>> + .vidioc_querybuf = vb2_ioctl_querybuf,
>>> + .vidioc_qbuf = vb2_ioctl_qbuf,
>>> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
>>> + .vidioc_expbuf = vb2_ioctl_expbuf,
>>> + .vidioc_streamon = vb2_ioctl_streamon,
>>> + .vidioc_streamoff = vb2_ioctl_streamoff,
>>> +};
>>> +
>>> +static void vimc_cap_return_all_buffers(struct vimc_cap_device *vcap,
>>> + enum vb2_buffer_state state)
>>> +{
>>> + struct vimc_cap_buffer *vbuf, *node;
>>> +
>>> + spin_lock(&vcap->qlock);
>>> +
>>> + list_for_each_entry_safe(vbuf, node, &vcap->buf_list, list) {
>>> + vb2_buffer_done(&vbuf->vb2.vb2_buf, state);
>>> + list_del(&vbuf->list);
>>> + }
>>> +
>>> + spin_unlock(&vcap->qlock);
>>> +}
>>> +
>>> +static int vimc_cap_pipeline_s_stream(struct vimc_cap_device *vcap,
>>> int enable)
>>> +{
>>> + int ret;
>>> + struct media_pad *pad;
>>> + struct media_entity *entity;
>>> + struct v4l2_subdev *sd;
>>> +
>>> + /* Start the stream in the subdevice direct connected */
>>> + entity = &vcap->vdev.entity;
>>> + pad = media_entity_remote_pad(&entity->pads[0]);
>>
>> You could use vcap->vdev.entity.pads here, without assigning to
>> entity. Then
>> entity would only be used to refer to the remove entity at the other
>> end of
>> the link. Up to you.
>>
>>> +
>>> + /* If we are not connected to any subdev node, it means there is
>>> nothing
>>
>> /*
>> * Multi line
>> * comment.
>> */
>>
>>> + * to activate on the pipe (e.g. we can be connected with an input
>>> + * device or we are not connected at all)
>>> + */
>>> + if (pad == NULL || !is_media_entity_v4l2_subdev(pad->entity))
>>> + return 0;
>>> +
>>> + entity = pad->entity;
>>> + sd = media_entity_to_v4l2_subdev(entity);
>>
>> And if you used pad->entity here, you could remove the entity variable
>> altogether.
>>
>>> +
>>> + ret = v4l2_subdev_call(sd, video, s_stream, enable);
>>> + if (ret && ret != -ENOIOCTLCMD)
>>> + return ret;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int vimc_cap_start_streaming(struct vb2_queue *vq, unsigned
>>> int count)
>>> +{
>>> + struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
>>> + struct media_entity *entity;
>>> + int ret;
>>> +
>>> + vcap->sequence = 0;
>>> +
>>> + /* Start the media pipeline */
>>> + entity = &vcap->vdev.entity;
>>> + ret = media_entity_pipeline_start(entity, &vcap->pipe);
>>> + if (ret) {
>>> + vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
>>> + return ret;
>>> + }
>>> +
>>> + /* Enable streaming from the pipe */
>>> + ret = vimc_cap_pipeline_s_stream(vcap, 1);
>>> + if (ret) {
>>> + vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/*
>>> + * Stop the stream engine. Any remaining buffers in the stream queue
>>> are
>>> + * dequeued and passed on to the vb2 framework marked as STATE_ERROR.
>>> + */
>>> +static void vimc_cap_stop_streaming(struct vb2_queue *vq)
>>> +{
>>> + struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
>>> +
>>> + /* Disable streaming from the pipe */
>>> + vimc_cap_pipeline_s_stream(vcap, 0);
>>> +
>>> + /* Stop the media pipeline */
>>> + media_entity_pipeline_stop(&vcap->vdev.entity);
>>> +
>>> + /* Release all active buffers */
>>> + vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_ERROR);
>>> +}
>>> +
>>> +static void vimc_cap_buf_queue(struct vb2_buffer *vb2_buf)
>>> +{
>>> + struct vimc_cap_device *vcap =
>>> vb2_get_drv_priv(vb2_buf->vb2_queue);
>>> + struct vimc_cap_buffer *buf = container_of(vb2_buf,
>>> + struct vimc_cap_buffer,
>>> + vb2.vb2_buf);
>>> +
>>> + spin_lock(&vcap->qlock);
>>> + list_add_tail(&buf->list, &vcap->buf_list);
>>> + spin_unlock(&vcap->qlock);
>>> +}
>>> +
>>> +static int vimc_cap_queue_setup(struct vb2_queue *vq, unsigned int
>>> *nbuffers,
>>> + unsigned int *nplanes, unsigned int sizes[],
>>> + struct device *alloc_devs[])
>>> +{
>>> + struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
>>> +
>>> + if (*nplanes)
>>> + return sizes[0] < vcap->format.sizeimage ? -EINVAL : 0;
>>
>> Why? The user could later reconfigure the device to use with a buffer of
>> this size. This might not be a concern for vimc, but the code from
>> example
>> drivers tends to get copied around.
>
>
> This first version only support a fixed sizeimage, this will be changed
> in the next patch series where
> I add support for multiple sizes
Sounds good to me. Thanks.
--
Kind regards,
Sakari Ailus
sakari.ailus@xxxxxx
