Re: [PATCH v2 1/2] media: davinci: vpif capture: upgrade the driver with v4l offerings
From: Prabhakar Lad
Date: Thu Apr 10 2014 - 09:23:27 EST
Hi Hans,
Thanks for the review.
On Fri, Apr 4, 2014 at 2:47 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> Hi Prabhakar,
>
> Some review comments below. I'm going through the code quite carefully since
> this very nice cleanup is a good opportunity to check for correct behavior in
> this driver.
>
Thank you :)
> On 04/04/2014 07:17 AM, Lad, Prabhakar wrote:
>> From: "Lad, Prabhakar" <prabhakar.csengg@xxxxxxxxx>
>>
>> This patch upgrades the vpif display driver with
>> v4l helpers, this patch does the following,
>>
>> 1: initialize the vb2 queue and context at the time of probe
>> and removes context at remove() callback.
>> 2: uses vb2_ioctl_*() helpers.
>> 3: uses vb2_fop_*() helpers.
>> 4: uses SIMPLE_DEV_PM_OPS.
>> 5: uses vb2_ioctl_*() helpers.
>> 6: vidioc_g/s_priority is now handled by v4l core.
>> 7: removed driver specific fh and now using one provided by v4l.
>> 8: fixes checkpatch warnings.
>>
>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
>> ---
>> drivers/media/platform/davinci/vpif_capture.c | 931 +++++++------------------
>> drivers/media/platform/davinci/vpif_capture.h | 32 +-
>> 2 files changed, 234 insertions(+), 729 deletions(-)
>>
>> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
>> index 8dea0b8..e4046f5 100644
>> --- a/drivers/media/platform/davinci/vpif_capture.c
>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>> @@ -1,5 +1,6 @@
>> /*
>> * Copyright (C) 2009 Texas Instruments Inc
>> + * Copyright (C) 2014 Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
>> *
>> * This program is free software; you can redistribute it and/or modify
>> * it under the terms of the GNU General Public License as published by
>> @@ -37,6 +38,8 @@ MODULE_VERSION(VPIF_CAPTURE_VERSION);
>> #define vpif_dbg(level, debug, fmt, arg...) \
>> v4l2_dbg(level, debug, &vpif_obj.v4l2_dev, fmt, ## arg)
>>
>> +#define VPIF_DRIVER_NAME "vpif_capture"
>> +
>> static int debug = 1;
>> static u32 ch0_numbuffers = 3;
>> static u32 ch1_numbuffers = 3;
>> @@ -65,11 +68,25 @@ static struct vpif_config_params config_params = {
>> .channel_bufsize[1] = 720 * 576 * 2,
>> };
>>
>> +static u8 channel_first_int[VPIF_NUMBER_OF_OBJECTS][2] = { {1, 1} };
>> +
>> /* global variables */
>> static struct vpif_device vpif_obj = { {NULL} };
>> static struct device *vpif_dev;
>> static void vpif_calculate_offsets(struct channel_obj *ch);
>> static void vpif_config_addr(struct channel_obj *ch, int muxmode);
>> +static int vpif_check_format(struct channel_obj *ch,
>> + struct v4l2_pix_format *pixfmt, int update);
>> +
>> +/*
>> + * Is set to 1 in case of SDTV formats, 2 in case of HDTV formats.
>> + */
>> +static int ycmux_mode;
>> +
>> +static inline struct vpif_cap_buffer *to_vpif_buffer(struct vb2_buffer *vb)
>> +{
>> + return container_of(vb, struct vpif_cap_buffer, vb);
>> +}
>>
>> /**
>> * buffer_prepare : callback function for buffer prepare
>> @@ -81,10 +98,8 @@ static void vpif_config_addr(struct channel_obj *ch, int muxmode);
>> */
>> static int vpif_buffer_prepare(struct vb2_buffer *vb)
>> {
>> - /* Get the file handle object and channel object */
>> - struct vpif_fh *fh = vb2_get_drv_priv(vb->vb2_queue);
>> struct vb2_queue *q = vb->vb2_queue;
>> - struct channel_obj *ch = fh->channel;
>> + struct channel_obj *ch = vb2_get_drv_priv(q);
>> struct common_obj *common;
>> unsigned long addr;
>>
>> @@ -100,7 +115,7 @@ static int vpif_buffer_prepare(struct vb2_buffer *vb)
>> goto exit;
>> addr = vb2_dma_contig_plane_dma_addr(vb, 0);
>>
>> - if (q->streaming) {
>> + if (vb2_is_streaming(q)) {
>> if (!IS_ALIGNED((addr + common->ytop_off), 8) ||
>> !IS_ALIGNED((addr + common->ybtm_off), 8) ||
>> !IS_ALIGNED((addr + common->ctop_off), 8) ||
>
> Why would you do this check only when streaming? Usually apps queued all
> buffers before calling S_STREAMON, so vb2_is_streaming(q) will still be
> false.
>
> The problem is that vpif_calculate_offsets() is called in start_streaming,
> but it should be called earlier in queue_setup. After queue_setup is called
> the application is no longer allowed to change the format, so that's a good
> place to do it. And then you can drop the vb2_is_streaming() check here
> since the offsets will always be valid.
>
> Also the 'if (vb->state != VB2_BUF_STATE_ACTIVE && vb->state != VB2_BUF_STATE_PREPARED)'
> can be droppedd. It will never be called in an invalid state.
>
> This check:
>
> if (vb2_plane_vaddr(vb, 0) &&
> vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0))
> goto exit;
>
> can also be improved: drop the vb2_plane_vaddr(vb, 0) since the payload check
> should be done unconditionally. The 'goto exit' should be replaced with a
> proper vpif_dbg since the message printed in the 'exit' is for the alignment
> check.
>
Ok
>> @@ -131,9 +146,7 @@ static int vpif_buffer_queue_setup(struct vb2_queue *vq,
>> unsigned int *nbuffers, unsigned int *nplanes,
>> unsigned int sizes[], void *alloc_ctxs[])
>> {
>> - /* Get the file handle object and channel object */
>> - struct vpif_fh *fh = vb2_get_drv_priv(vq);
>> - struct channel_obj *ch = fh->channel;
>> + struct channel_obj *ch = vb2_get_drv_priv(vq);
>> struct common_obj *common;
>> unsigned long size;
>>
>> @@ -141,8 +154,7 @@ static int vpif_buffer_queue_setup(struct vb2_queue *vq,
>>
>> vpif_dbg(2, debug, "vpif_buffer_setup\n");
>>
>> - /* If memory type is not mmap, return */
>> - if (V4L2_MEMORY_MMAP == common->memory) {
>> + if (vq->memory == V4L2_MEMORY_MMAP) {
>> /* Calculate the size of the buffer */
>> size = config_params.channel_bufsize[ch->channel_id];
>> /*
>
> A few general questions regarding queue_setup (not specific to this patch):
>
> Why do we use 'config_params.channel_bufsize[ch->channel_id]' as the buffer size
> in the MMAP case instead of common->fmt.fmt.pix.sizeimage? Why do we have the
> ch[01]_bufsize module options? How and when is video_limit configured? Why do we
> have ch[01]_numbuffers module options?
>
This module options were for to make sure that driver doesn't
fail to get contiguous memory, I think this can be dropped as well
with CMA in place.
> Since you added support for create_buffers the code in queue_setup needs to
> change a bit as well:
>
> if (*nbuffers < config_params.min_numbuffers)
> *nbuffers = config_params.min_numbuffers;
>
> becomes:
>
> if (vq->num_buffers + *nbuffers < config_params.min_numbuffers)
> *nbuffers = config_params.min_numbuffers - vq->num_buffers;
>
> And when setting the size it should be:
>
> size = fmt ? fmt->fmt.pix.sizeimage : common->fmt.fmt.pix.sizeimage;
>
>> @@ -183,11 +195,8 @@ static int vpif_buffer_queue_setup(struct vb2_queue *vq,
>> */
>> static void vpif_buffer_queue(struct vb2_buffer *vb)
>> {
>> - /* Get the file handle object and channel object */
>> - struct vpif_fh *fh = vb2_get_drv_priv(vb->vb2_queue);
>> - struct channel_obj *ch = fh->channel;
>> - struct vpif_cap_buffer *buf = container_of(vb,
>> - struct vpif_cap_buffer, vb);
>> + struct channel_obj *ch = vb2_get_drv_priv(vb->vb2_queue);
>> + struct vpif_cap_buffer *buf = to_vpif_buffer(vb);
>> struct common_obj *common;
>> unsigned long flags;
>>
>> @@ -210,11 +219,8 @@ static void vpif_buffer_queue(struct vb2_buffer *vb)
>> */
>> static void vpif_buf_cleanup(struct vb2_buffer *vb)
>> {
>> - /* Get the file handle object and channel object */
>> - struct vpif_fh *fh = vb2_get_drv_priv(vb->vb2_queue);
>> - struct vpif_cap_buffer *buf = container_of(vb,
>> - struct vpif_cap_buffer, vb);
>> - struct channel_obj *ch = fh->channel;
>> + struct channel_obj *ch = vb2_get_drv_priv(vb->vb2_queue);
>> + struct vpif_cap_buffer *buf = to_vpif_buffer(vb);
>> struct common_obj *common;
>> unsigned long flags;
>>
>
> buf_cleanup is *never* called if the buffer is in the active state, so you
> can drop the whole function.
>
OK
>> @@ -227,65 +233,26 @@ static void vpif_buf_cleanup(struct vb2_buffer *vb)
>>
>> }
>>
>> -static void vpif_wait_prepare(struct vb2_queue *vq)
>> -{
>> - struct vpif_fh *fh = vb2_get_drv_priv(vq);
>> - struct channel_obj *ch = fh->channel;
>> - struct common_obj *common;
>> -
>> - common = &ch->common[VPIF_VIDEO_INDEX];
>> - mutex_unlock(&common->lock);
>> -}
>> -
>> -static void vpif_wait_finish(struct vb2_queue *vq)
>> -{
>> - struct vpif_fh *fh = vb2_get_drv_priv(vq);
>> - struct channel_obj *ch = fh->channel;
>> - struct common_obj *common;
>> -
>> - common = &ch->common[VPIF_VIDEO_INDEX];
>> - mutex_lock(&common->lock);
>> -}
>> -
>> -static int vpif_buffer_init(struct vb2_buffer *vb)
>> -{
>> - struct vpif_cap_buffer *buf = container_of(vb,
>> - struct vpif_cap_buffer, vb);
>> -
>> - INIT_LIST_HEAD(&buf->list);
>> -
>> - return 0;
>> -}
>> -
>> -static u8 channel_first_int[VPIF_NUMBER_OF_OBJECTS][2] =
>> - { {1, 1} };
>> -
>> static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> {
>> - struct vpif_capture_config *vpif_config_data =
>> - vpif_dev->platform_data;
>> - struct vpif_fh *fh = vb2_get_drv_priv(vq);
>> - struct channel_obj *ch = fh->channel;
>> + struct vpif_capture_config *vpif_config_data;
>> + struct channel_obj *ch = vb2_get_drv_priv(vq);
>> struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>> struct vpif_params *vpif = &ch->vpifparams;
>> - unsigned long addr = 0;
>> - unsigned long flags;
>> + struct vpif_cap_buffer *buf, *tmp;
>> + unsigned long addr, flags;
>> int ret;
>>
>> spin_lock_irqsave(&common->irqlock, flags);
>>
>> - /* Get the next frame from the buffer queue */
>> - common->cur_frm = common->next_frm = list_entry(common->dma_queue.next,
>> - struct vpif_cap_buffer, list);
>> - /* Remove buffer from the buffer queue */
>> - list_del(&common->cur_frm->list);
>> - spin_unlock_irqrestore(&common->irqlock, flags);
>> - /* Mark state of the current frame to active */
>> - common->cur_frm->vb.state = VB2_BUF_STATE_ACTIVE;
>> - /* Initialize field_id and started member */
>> + /* Initialize field_id */
>> ch->field_id = 0;
>> - common->started = 1;
>> - addr = vb2_dma_contig_plane_dma_addr(&common->cur_frm->vb, 0);
>> +
>> + ret = vpif_check_format(ch, &common->fmt.fmt.pix, 0);
>> + if (ret) {
>> + ret = -EINVAL;
>> + goto err;
>> + }
>
> Why do a check_format here? Since that is already checked by try/s_fmt you
> should never get a wrong format here in start_streaming().
>
Ok will drop it.
>>
>> /* Calculate the offset for Y and C data in the buffer */
>> vpif_calculate_offsets(ch);
>
> The same is true for the field checks that are done after the call to
> vpif_calculate_offsets.
>
> The way it should work is that s_std and s_dv_timings reset the current format
> to something that is valid for the new standard/timings. So field can never be
> wrong. See e.g. how the v4l2-pci-skeleton.c example source does it.
>
>> @@ -296,30 +263,50 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> (!vpif->std_info.frm_fmt &&
>> (common->fmt.fmt.pix.field == V4L2_FIELD_NONE))) {
>> vpif_dbg(1, debug, "conflict in field format and std format\n");
>> - return -EINVAL;
>> + ret = -EINVAL;
>> + goto err;
>> }
>>
>> + vpif_config_data = vpif_dev->platform_data;
>> /* configure 1 or 2 channel mode */
>> if (vpif_config_data->setup_input_channel_mode) {
>> ret = vpif_config_data->
>> setup_input_channel_mode(vpif->std_info.ycmux_mode);
>> if (ret < 0) {
>> vpif_dbg(1, debug, "can't set vpif channel mode\n");
>> - return ret;
>> + ret = -EINVAL;
>> + goto err;
>> }
>> }
>>
>> + ret = v4l2_subdev_call(ch->sd, video, s_stream, 1);
>> +
>> + if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV) {
>> + vpif_dbg(1, debug, "stream on failed in subdev\n");
>> + goto err;
>> + }
>> +
>> /* Call vpif_set_params function to set the parameters and addresses */
>> ret = vpif_set_video_params(vpif, ch->channel_id);
>> -
>> if (ret < 0) {
>> vpif_dbg(1, debug, "can't set video params\n");
>> - return ret;
>> + ret = -EINVAL;
>> + goto err;
>> }
>> -
>> - common->started = ret;
>> + ycmux_mode = ret;
>> vpif_config_addr(ch, ret);
>>
>> + /* Get the next frame from the buffer queue */
>> + common->cur_frm = common->next_frm = list_entry(common->dma_queue.next,
>> + struct vpif_cap_buffer, list);
>> + /* Remove buffer from the buffer queue */
>> + list_del(&common->cur_frm->list);
>> + spin_unlock_irqrestore(&common->irqlock, flags);
>> + /* Mark state of the current frame to active */
>> + common->cur_frm->vb.state = VB2_BUF_STATE_ACTIVE;
>> +
>> + addr = vb2_dma_contig_plane_dma_addr(&common->cur_frm->vb, 0);
>> +
>> common->set_addr(addr + common->ytop_off,
>> addr + common->ybtm_off,
>> addr + common->ctop_off,
>> @@ -330,31 +317,35 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> * VPIF register
>> */
>> channel_first_int[VPIF_VIDEO_INDEX][ch->channel_id] = 1;
>> - if ((VPIF_CHANNEL0_VIDEO == ch->channel_id)) {
>> + if (VPIF_CHANNEL0_VIDEO == ch->channel_id) {
>> channel0_intr_assert();
>> channel0_intr_enable(1);
>> enable_channel0(1);
>> }
>> - if ((VPIF_CHANNEL1_VIDEO == ch->channel_id) ||
>> - (common->started == 2)) {
>> + if (VPIF_CHANNEL1_VIDEO == ch->channel_id || ycmux_mode == 2) {
>> channel1_intr_assert();
>> channel1_intr_enable(1);
>> enable_channel1(1);
>> }
>>
>> return 0;
>> +
>> +err:
>> + list_for_each_entry_safe(buf, tmp, &common->dma_queue, list) {
>> + list_del(&buf->list);
>> + vb2_buffer_done(&buf->vb, VB2_BUF_STATE_QUEUED);
>> + }
>> +
>> + return ret;
>> }
>>
>> /* abort streaming and wait for last buffer */
>> static int vpif_stop_streaming(struct vb2_queue *vq)
>> {
>> - struct vpif_fh *fh = vb2_get_drv_priv(vq);
>> - struct channel_obj *ch = fh->channel;
>> + struct channel_obj *ch = vb2_get_drv_priv(vq);
>> struct common_obj *common;
>> unsigned long flags;
>> -
>> - if (!vb2_is_streaming(vq))
>> - return 0;
>> + int ret;
>>
>> common = &ch->common[VPIF_VIDEO_INDEX];
>>
>> @@ -363,12 +354,17 @@ static int vpif_stop_streaming(struct vb2_queue *vq)
>> enable_channel0(0);
>> channel0_intr_enable(0);
>> }
>> - if ((VPIF_CHANNEL1_VIDEO == ch->channel_id) ||
>> - (2 == common->started)) {
>> + if (VPIF_CHANNEL1_VIDEO == ch->channel_id || ycmux_mode == 2) {
>> enable_channel1(0);
>> channel1_intr_enable(0);
>> }
>> - common->started = 0;
>> +
>> + ycmux_mode = 0;
>> +
>> + ret = v4l2_subdev_call(ch->sd, video, s_stream, 0);
>> +
>> + if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV)
>> + vpif_dbg(1, debug, "stream off failed in subdev\n");
>>
>> /* release all active buffers */
>> spin_lock_irqsave(&common->irqlock, flags);
>> @@ -396,9 +392,8 @@ static int vpif_stop_streaming(struct vb2_queue *vq)
>>
>> static struct vb2_ops video_qops = {
>> .queue_setup = vpif_buffer_queue_setup,
>> - .wait_prepare = vpif_wait_prepare,
>> - .wait_finish = vpif_wait_finish,
>> - .buf_init = vpif_buffer_init,
>> + .wait_prepare = vb2_ops_wait_prepare,
>> + .wait_finish = vb2_ops_wait_finish,
>> .buf_prepare = vpif_buffer_prepare,
>> .start_streaming = vpif_start_streaming,
>> .stop_streaming = vpif_stop_streaming,
>> @@ -417,8 +412,7 @@ static struct vb2_ops video_qops = {
>> static void vpif_process_buffer_complete(struct common_obj *common)
>> {
>> v4l2_get_timestamp(&common->cur_frm->vb.v4l2_buf.timestamp);
>> - vb2_buffer_done(&common->cur_frm->vb,
>> - VB2_BUF_STATE_DONE);
>> + vb2_buffer_done(&common->cur_frm->vb, VB2_BUF_STATE_DONE);
>
> common->cur_frm->vb.v4l2_buf.field should also be set.
>
> This should have been caught by v4l2-compliance -s!
>
OK will fix it.
>> /* Make curFrm pointing to nextFrm */
>> common->cur_frm = common->next_frm;
>> }
>> @@ -433,7 +427,7 @@ static void vpif_process_buffer_complete(struct common_obj *common)
>> */
>> static void vpif_schedule_next_buffer(struct common_obj *common)
>> {
>> - unsigned long addr = 0;
>> + unsigned long addr;
>>
>> spin_lock(&common->irqlock);
>> common->next_frm = list_entry(common->dma_queue.next,
>> @@ -479,7 +473,7 @@ static irqreturn_t vpif_channel_isr(int irq, void *dev_id)
>> for (i = 0; i < VPIF_NUMBER_OF_OBJECTS; i++) {
>> common = &ch->common[i];
>> /* skip If streaming is not started in this channel */
>> - if (0 == common->started)
>> + if (!vb2_is_streaming(&common->buffer_queue))
>> continue;
>
> This check can be dropped. If you get here, you are always streaming.
>
OK
>>
>> /* Check the field format */
>> @@ -683,10 +677,6 @@ static void vpif_config_format(struct channel_obj *ch)
>> vpif_dbg(2, debug, "vpif_config_format\n");
>>
>> common->fmt.fmt.pix.field = V4L2_FIELD_ANY;
>
> In general I strongly recommend against using FIELD_ANY internally. It's
> cleaner to only use proper field values that are valid for the current format.
>
> Only in check_fmt would you check for FIELD_ANY and replace it with whatever
> is appropriate for the current format. The reason is that while the app my
> specify FIELD_ANY, the driver should never return it anywhere. To keep that
> from happening and to keep everything sane it is best in my experience to
> just make sure it is never used in internal data structures.
>
Ok will replace it with appropriate fields.
>> - if (config_params.numbuffers[ch->channel_id] == 0)
>> - common->memory = V4L2_MEMORY_USERPTR;
>> - else
>> - common->memory = V4L2_MEMORY_MMAP;
>>
>> common->fmt.fmt.pix.sizeimage
>> = config_params.channel_bufsize[ch->channel_id];
>> @@ -837,415 +827,6 @@ static void vpif_config_addr(struct channel_obj *ch, int muxmode)
>> }
>>
>> /**
>> - * vpif_mmap : It is used to map kernel space buffers into user spaces
>> - * @filep: file pointer
>> - * @vma: ptr to vm_area_struct
>> - */
>> -static int vpif_mmap(struct file *filep, struct vm_area_struct *vma)
>> -{
>> - /* Get the channel object and file handle object */
>> - struct vpif_fh *fh = filep->private_data;
>> - struct channel_obj *ch = fh->channel;
>> - struct common_obj *common = &(ch->common[VPIF_VIDEO_INDEX]);
>> - int ret;
>> -
>> - vpif_dbg(2, debug, "vpif_mmap\n");
>> -
>> - if (mutex_lock_interruptible(&common->lock))
>> - return -ERESTARTSYS;
>> - ret = vb2_mmap(&common->buffer_queue, vma);
>> - mutex_unlock(&common->lock);
>> - return ret;
>> -}
>> -
>> -/**
>> - * vpif_poll: It is used for select/poll system call
>> - * @filep: file pointer
>> - * @wait: poll table to wait
>> - */
>> -static unsigned int vpif_poll(struct file *filep, poll_table * wait)
>> -{
>> - struct vpif_fh *fh = filep->private_data;
>> - struct channel_obj *channel = fh->channel;
>> - struct common_obj *common = &(channel->common[VPIF_VIDEO_INDEX]);
>> - unsigned int res = 0;
>> -
>> - vpif_dbg(2, debug, "vpif_poll\n");
>> -
>> - if (common->started) {
>> - mutex_lock(&common->lock);
>> - res = vb2_poll(&common->buffer_queue, filep, wait);
>> - mutex_unlock(&common->lock);
>> - }
>> - return res;
>> -}
>> -
>> -/**
>> - * vpif_open : vpif open handler
>> - * @filep: file ptr
>> - *
>> - * It creates object of file handle structure and stores it in private_data
>> - * member of filepointer
>> - */
>> -static int vpif_open(struct file *filep)
>> -{
>> - struct video_device *vdev = video_devdata(filep);
>> - struct common_obj *common;
>> - struct video_obj *vid_ch;
>> - struct channel_obj *ch;
>> - struct vpif_fh *fh;
>> -
>> - vpif_dbg(2, debug, "vpif_open\n");
>> -
>> - ch = video_get_drvdata(vdev);
>> -
>> - vid_ch = &ch->video;
>> - common = &ch->common[VPIF_VIDEO_INDEX];
>> -
>> - /* Allocate memory for the file handle object */
>> - fh = kzalloc(sizeof(struct vpif_fh), GFP_KERNEL);
>> - if (NULL == fh) {
>> - vpif_err("unable to allocate memory for file handle object\n");
>> - return -ENOMEM;
>> - }
>> -
>> - if (mutex_lock_interruptible(&common->lock)) {
>> - kfree(fh);
>> - return -ERESTARTSYS;
>> - }
>> - /* store pointer to fh in private_data member of filep */
>> - filep->private_data = fh;
>> - fh->channel = ch;
>> - fh->initialized = 0;
>> - /* If decoder is not initialized. initialize it */
>> - if (!ch->initialized) {
>> - fh->initialized = 1;
>> - ch->initialized = 1;
>> - memset(&(ch->vpifparams), 0, sizeof(struct vpif_params));
>> - }
>> - /* Increment channel usrs counter */
>> - ch->usrs++;
>> - /* Set io_allowed member to false */
>> - fh->io_allowed[VPIF_VIDEO_INDEX] = 0;
>> - /* Initialize priority of this instance to default priority */
>> - fh->prio = V4L2_PRIORITY_UNSET;
>> - v4l2_prio_open(&ch->prio, &fh->prio);
>> - mutex_unlock(&common->lock);
>> - return 0;
>> -}
>> -
>> -/**
>> - * vpif_release : function to clean up file close
>> - * @filep: file pointer
>> - *
>> - * This function deletes buffer queue, frees the buffers and the vpif file
>> - * handle
>> - */
>> -static int vpif_release(struct file *filep)
>> -{
>> - struct vpif_fh *fh = filep->private_data;
>> - struct channel_obj *ch = fh->channel;
>> - struct common_obj *common;
>> -
>> - vpif_dbg(2, debug, "vpif_release\n");
>> -
>> - common = &ch->common[VPIF_VIDEO_INDEX];
>> -
>> - mutex_lock(&common->lock);
>> - /* if this instance is doing IO */
>> - if (fh->io_allowed[VPIF_VIDEO_INDEX]) {
>> - /* Reset io_usrs member of channel object */
>> - common->io_usrs = 0;
>> - /* Free buffers allocated */
>> - vb2_queue_release(&common->buffer_queue);
>> - vb2_dma_contig_cleanup_ctx(common->alloc_ctx);
>> - }
>> -
>> - /* Decrement channel usrs counter */
>> - ch->usrs--;
>> -
>> - /* Close the priority */
>> - v4l2_prio_close(&ch->prio, fh->prio);
>> -
>> - if (fh->initialized)
>> - ch->initialized = 0;
>> -
>> - mutex_unlock(&common->lock);
>> - filep->private_data = NULL;
>> - kfree(fh);
>> - return 0;
>> -}
>> -
>> -/**
>> - * vpif_reqbufs() - request buffer handler
>> - * @file: file ptr
>> - * @priv: file handle
>> - * @reqbuf: request buffer structure ptr
>> - */
>> -static int vpif_reqbufs(struct file *file, void *priv,
>> - struct v4l2_requestbuffers *reqbuf)
>> -{
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> - struct common_obj *common;
>> - u8 index = 0;
>> - struct vb2_queue *q;
>> - int ret;
>> -
>> - vpif_dbg(2, debug, "vpif_reqbufs\n");
>> -
>> - /**
>> - * This file handle has not initialized the channel,
>> - * It is not allowed to do settings
>> - */
>> - if ((VPIF_CHANNEL0_VIDEO == ch->channel_id)
>> - || (VPIF_CHANNEL1_VIDEO == ch->channel_id)) {
>> - if (!fh->initialized) {
>> - vpif_dbg(1, debug, "Channel Busy\n");
>> - return -EBUSY;
>> - }
>> - }
>> -
>> - if (V4L2_BUF_TYPE_VIDEO_CAPTURE != reqbuf->type || !vpif_dev)
>> - return -EINVAL;
>> -
>> - index = VPIF_VIDEO_INDEX;
>> -
>> - common = &ch->common[index];
>> -
>> - if (0 != common->io_usrs)
>> - return -EBUSY;
>> -
>> - /* Initialize videobuf2 queue as per the buffer type */
>> - common->alloc_ctx = vb2_dma_contig_init_ctx(vpif_dev);
>> - if (IS_ERR(common->alloc_ctx)) {
>> - vpif_err("Failed to get the context\n");
>> - return PTR_ERR(common->alloc_ctx);
>> - }
>> - q = &common->buffer_queue;
>> - q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> - q->io_modes = VB2_MMAP | VB2_USERPTR;
>> - q->drv_priv = fh;
>> - q->ops = &video_qops;
>> - q->mem_ops = &vb2_dma_contig_memops;
>> - q->buf_struct_size = sizeof(struct vpif_cap_buffer);
>> - q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> - q->min_buffers_needed = 1;
>> -
>> - ret = vb2_queue_init(q);
>> - if (ret) {
>> - vpif_err("vpif_capture: vb2_queue_init() failed\n");
>> - vb2_dma_contig_cleanup_ctx(common->alloc_ctx);
>> - return ret;
>> - }
>> - /* Set io allowed member of file handle to TRUE */
>> - fh->io_allowed[index] = 1;
>> - /* Increment io usrs member of channel object to 1 */
>> - common->io_usrs = 1;
>> - /* Store type of memory requested in channel object */
>> - common->memory = reqbuf->memory;
>> - INIT_LIST_HEAD(&common->dma_queue);
>> -
>> - /* Allocate buffers */
>> - return vb2_reqbufs(&common->buffer_queue, reqbuf);
>> -}
>> -
>> -/**
>> - * vpif_querybuf() - query buffer handler
>> - * @file: file ptr
>> - * @priv: file handle
>> - * @buf: v4l2 buffer structure ptr
>> - */
>> -static int vpif_querybuf(struct file *file, void *priv,
>> - struct v4l2_buffer *buf)
>> -{
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> - struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>> -
>> - vpif_dbg(2, debug, "vpif_querybuf\n");
>> -
>> - if (common->fmt.type != buf->type)
>> - return -EINVAL;
>> -
>> - if (common->memory != V4L2_MEMORY_MMAP) {
>> - vpif_dbg(1, debug, "Invalid memory\n");
>> - return -EINVAL;
>> - }
>> -
>> - return vb2_querybuf(&common->buffer_queue, buf);
>> -}
>> -
>> -/**
>> - * vpif_qbuf() - query buffer handler
>> - * @file: file ptr
>> - * @priv: file handle
>> - * @buf: v4l2 buffer structure ptr
>> - */
>> -static int vpif_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
>> -{
>> -
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> - struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>> - struct v4l2_buffer tbuf = *buf;
>> -
>> - vpif_dbg(2, debug, "vpif_qbuf\n");
>> -
>> - if (common->fmt.type != tbuf.type) {
>> - vpif_err("invalid buffer type\n");
>> - return -EINVAL;
>> - }
>> -
>> - if (!fh->io_allowed[VPIF_VIDEO_INDEX]) {
>> - vpif_err("fh io not allowed\n");
>> - return -EACCES;
>> - }
>> -
>> - return vb2_qbuf(&common->buffer_queue, buf);
>> -}
>> -
>> -/**
>> - * vpif_dqbuf() - query buffer handler
>> - * @file: file ptr
>> - * @priv: file handle
>> - * @buf: v4l2 buffer structure ptr
>> - */
>> -static int vpif_dqbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
>> -{
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> - struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>> -
>> - vpif_dbg(2, debug, "vpif_dqbuf\n");
>> -
>> - return vb2_dqbuf(&common->buffer_queue, buf,
>> - (file->f_flags & O_NONBLOCK));
>> -}
>> -
>> -/**
>> - * vpif_streamon() - streamon handler
>> - * @file: file ptr
>> - * @priv: file handle
>> - * @buftype: v4l2 buffer type
>> - */
>> -static int vpif_streamon(struct file *file, void *priv,
>> - enum v4l2_buf_type buftype)
>> -{
>> -
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> - struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>> - struct channel_obj *oth_ch = vpif_obj.dev[!ch->channel_id];
>> - struct vpif_params *vpif;
>> - int ret = 0;
>> -
>> - vpif_dbg(2, debug, "vpif_streamon\n");
>> -
>> - vpif = &ch->vpifparams;
>> -
>> - if (buftype != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>> - vpif_dbg(1, debug, "buffer type not supported\n");
>> - return -EINVAL;
>> - }
>> -
>> - /* If file handle is not allowed IO, return error */
>> - if (!fh->io_allowed[VPIF_VIDEO_INDEX]) {
>> - vpif_dbg(1, debug, "io not allowed\n");
>> - return -EACCES;
>> - }
>> -
>> - /* If Streaming is already started, return error */
>> - if (common->started) {
>> - vpif_dbg(1, debug, "channel->started\n");
>> - return -EBUSY;
>> - }
>> -
>> - if ((ch->channel_id == VPIF_CHANNEL0_VIDEO &&
>> - oth_ch->common[VPIF_VIDEO_INDEX].started &&
>> - vpif->std_info.ycmux_mode == 0) ||
>> - ((ch->channel_id == VPIF_CHANNEL1_VIDEO) &&
>> - (2 == oth_ch->common[VPIF_VIDEO_INDEX].started))) {
>> - vpif_dbg(1, debug, "other channel is being used\n");
>> - return -EBUSY;
>> - }
>> -
>> - ret = vpif_check_format(ch, &common->fmt.fmt.pix, 0);
>> - if (ret)
>> - return ret;
>> -
>> - /* Enable streamon on the sub device */
>> - ret = v4l2_subdev_call(ch->sd, video, s_stream, 1);
>> -
>> - if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV) {
>> - vpif_dbg(1, debug, "stream on failed in subdev\n");
>> - return ret;
>> - }
>> -
>> - /* Call vb2_streamon to start streaming in videobuf2 */
>> - ret = vb2_streamon(&common->buffer_queue, buftype);
>> - if (ret) {
>> - vpif_dbg(1, debug, "vb2_streamon\n");
>> - return ret;
>> - }
>> -
>> - return ret;
>> -}
>> -
>> -/**
>> - * vpif_streamoff() - streamoff handler
>> - * @file: file ptr
>> - * @priv: file handle
>> - * @buftype: v4l2 buffer type
>> - */
>> -static int vpif_streamoff(struct file *file, void *priv,
>> - enum v4l2_buf_type buftype)
>> -{
>> -
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> - struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>> - int ret;
>> -
>> - vpif_dbg(2, debug, "vpif_streamoff\n");
>> -
>> - if (buftype != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>> - vpif_dbg(1, debug, "buffer type not supported\n");
>> - return -EINVAL;
>> - }
>> -
>> - /* If io is allowed for this file handle, return error */
>> - if (!fh->io_allowed[VPIF_VIDEO_INDEX]) {
>> - vpif_dbg(1, debug, "io not allowed\n");
>> - return -EACCES;
>> - }
>> -
>> - /* If streaming is not started, return error */
>> - if (!common->started) {
>> - vpif_dbg(1, debug, "channel->started\n");
>> - return -EINVAL;
>> - }
>> -
>> - /* disable channel */
>> - if (VPIF_CHANNEL0_VIDEO == ch->channel_id) {
>> - enable_channel0(0);
>> - channel0_intr_enable(0);
>> - } else {
>> - enable_channel1(0);
>> - channel1_intr_enable(0);
>> - }
>> -
>> - common->started = 0;
>> -
>> - ret = v4l2_subdev_call(ch->sd, video, s_stream, 0);
>> -
>> - if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV)
>> - vpif_dbg(1, debug, "stream off failed in subdev\n");
>> -
>> - return vb2_streamoff(&common->buffer_queue, buftype);
>> -}
>> -
>> -/**
>> * vpif_input_to_subdev() - Maps input to sub device
>> * @vpif_cfg - global config ptr
>> * @chan_cfg - channel config ptr
>> @@ -1348,9 +929,9 @@ static int vpif_set_input(
>> */
>> static int vpif_querystd(struct file *file, void *priv, v4l2_std_id *std_id)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> - int ret = 0;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>> + int ret;
>>
>> vpif_dbg(2, debug, "vpif_querystd\n");
>>
>> @@ -1375,8 +956,8 @@ static int vpif_querystd(struct file *file, void *priv, v4l2_std_id *std_id)
>> */
>> static int vpif_g_std(struct file *file, void *priv, v4l2_std_id *std)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>>
>> vpif_dbg(2, debug, "vpif_g_std\n");
>>
>> @@ -1392,31 +973,15 @@ static int vpif_g_std(struct file *file, void *priv, v4l2_std_id *std)
>> */
>> static int vpif_s_std(struct file *file, void *priv, v4l2_std_id std_id)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>> struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>> - int ret = 0;
>> + int ret;
>>
>> vpif_dbg(2, debug, "vpif_s_std\n");
>>
>> - if (common->started) {
>> - vpif_err("streaming in progress\n");
>> + if (vb2_is_streaming(&common->buffer_queue))
>
> This should be vb2_is_busy(). is_streaming is true when streaming is in progress,
> is_busy is true once reqbufs()/create_buffers() is called since that's the moment
> the format becomes locked and can no longer be changed.
>
OK
>> return -EBUSY;
>> - }
>> -
>> - if ((VPIF_CHANNEL0_VIDEO == ch->channel_id) ||
>> - (VPIF_CHANNEL1_VIDEO == ch->channel_id)) {
>> - if (!fh->initialized) {
>> - vpif_dbg(1, debug, "Channel Busy\n");
>> - return -EBUSY;
>> - }
>> - }
>> -
>> - ret = v4l2_prio_check(&ch->prio, fh->prio);
>> - if (0 != ret)
>> - return ret;
>> -
>> - fh->initialized = 1;
>>
>> /* Call encoder subdevice function to set the standard */
>> ch->video.stdid = std_id;
>> @@ -1452,8 +1017,8 @@ static int vpif_enum_input(struct file *file, void *priv,
>>
>> struct vpif_capture_config *config = vpif_dev->platform_data;
>> struct vpif_capture_chan_config *chan_cfg;
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>>
>> chan_cfg = &config->chan_config[ch->channel_id];
>>
>> @@ -1475,8 +1040,8 @@ static int vpif_enum_input(struct file *file, void *priv,
>> */
>> static int vpif_g_input(struct file *file, void *priv, unsigned int *index)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>>
>> *index = ch->input_idx;
>> return 0;
>> @@ -1492,34 +1057,18 @@ static int vpif_s_input(struct file *file, void *priv, unsigned int index)
>> {
>> struct vpif_capture_config *config = vpif_dev->platform_data;
>> struct vpif_capture_chan_config *chan_cfg;
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>> struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>> - int ret;
>> +
>> + if (vb2_is_streaming(&common->buffer_queue))
>
> vb2_is_busy()
>
>> + return -EBUSY;
>>
>> chan_cfg = &config->chan_config[ch->channel_id];
>>
>> if (index >= chan_cfg->input_count)
>> return -EINVAL;
>>
>> - if (common->started) {
>> - vpif_err("Streaming in progress\n");
>> - return -EBUSY;
>> - }
>> -
>> - if ((VPIF_CHANNEL0_VIDEO == ch->channel_id) ||
>> - (VPIF_CHANNEL1_VIDEO == ch->channel_id)) {
>> - if (!fh->initialized) {
>> - vpif_dbg(1, debug, "Channel Busy\n");
>> - return -EBUSY;
>> - }
>> - }
>> -
>> - ret = v4l2_prio_check(&ch->prio, fh->prio);
>> - if (0 != ret)
>> - return ret;
>> -
>> - fh->initialized = 1;
>> return vpif_set_input(config, ch, index);
>> }
>>
>> @@ -1532,8 +1081,8 @@ static int vpif_s_input(struct file *file, void *priv, unsigned int index)
>> static int vpif_enum_fmt_vid_cap(struct file *file, void *priv,
>> struct v4l2_fmtdesc *fmt)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>>
>> if (fmt->index != 0) {
>> vpif_dbg(1, debug, "Invalid format index\n");
>> @@ -1562,8 +1111,8 @@ static int vpif_enum_fmt_vid_cap(struct file *file, void *priv,
>> static int vpif_try_fmt_vid_cap(struct file *file, void *priv,
>> struct v4l2_format *fmt)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>> struct v4l2_pix_format *pixfmt = &fmt->fmt.pix;
>>
>> return vpif_check_format(ch, pixfmt, 1);
>> @@ -1579,8 +1128,8 @@ static int vpif_try_fmt_vid_cap(struct file *file, void *priv,
>> static int vpif_g_fmt_vid_cap(struct file *file, void *priv,
>> struct v4l2_format *fmt)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>> struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>>
>> /* Check the validity of the buffer type */
>> @@ -1601,8 +1150,8 @@ static int vpif_g_fmt_vid_cap(struct file *file, void *priv,
>> static int vpif_s_fmt_vid_cap(struct file *file, void *priv,
>> struct v4l2_format *fmt)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>> struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>> struct v4l2_pix_format *pixfmt;
>> int ret = 0;
>> @@ -1610,31 +1159,17 @@ static int vpif_s_fmt_vid_cap(struct file *file, void *priv,
>> vpif_dbg(2, debug, "%s\n", __func__);
>>
>> /* If streaming is started, return error */
>> - if (common->started) {
>> + if (vb2_is_streaming(&common->buffer_queue)) {
>
> vb2_is_busy()
>
>> vpif_dbg(1, debug, "Streaming is started\n");
>> return -EBUSY;
>> }
>>
>> - if ((VPIF_CHANNEL0_VIDEO == ch->channel_id) ||
>> - (VPIF_CHANNEL1_VIDEO == ch->channel_id)) {
>> - if (!fh->initialized) {
>> - vpif_dbg(1, debug, "Channel Busy\n");
>> - return -EBUSY;
>> - }
>> - }
>> -
>> - ret = v4l2_prio_check(&ch->prio, fh->prio);
>> - if (0 != ret)
>> - return ret;
>> -
>> - fh->initialized = 1;
>> -
>> pixfmt = &fmt->fmt.pix;
>> /* Check for valid field format */
>> ret = vpif_check_format(ch, pixfmt, 0);
>
> Once check_format is no longer called from start_streaming the check_format code
> can be moved to vpif_try_fmt_vid_cap and vpif_s_fmt_vid_cap can call
> vpif_try_fmt_vid_cap directly instead of check_formats.
>
OK
>> -
>> if (ret)
>> return ret;
>> +
>> /* store the format in the channel object */
>> common->fmt = *fmt;
>> return 0;
>> @@ -1653,7 +1188,7 @@ static int vpif_querycap(struct file *file, void *priv,
>>
>> cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
>
> You can add V4L2_CAP_READWRITE here if you add VB2_READ to q->io_modes and
> add .read = vb2_fop_read to vpif_fops.
>
>> cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
>> - snprintf(cap->driver, sizeof(cap->driver), "%s", dev_name(vpif_dev));
>> + strlcpy(cap->driver, VPIF_DRIVER_NAME, sizeof(cap->driver));
>> snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
>> dev_name(vpif_dev));
>> strlcpy(cap->card, config->card_name, sizeof(cap->card));
>> @@ -1662,37 +1197,6 @@ static int vpif_querycap(struct file *file, void *priv,
>> }
>>
>> /**
>> - * vpif_g_priority() - get priority handler
>> - * @file: file ptr
>> - * @priv: file handle
>> - * @prio: ptr to v4l2_priority structure
>> - */
>> -static int vpif_g_priority(struct file *file, void *priv,
>> - enum v4l2_priority *prio)
>> -{
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> -
>> - *prio = v4l2_prio_max(&ch->prio);
>> -
>> - return 0;
>> -}
>> -
>> -/**
>> - * vpif_s_priority() - set priority handler
>> - * @file: file ptr
>> - * @priv: file handle
>> - * @prio: ptr to v4l2_priority structure
>> - */
>> -static int vpif_s_priority(struct file *file, void *priv, enum v4l2_priority p)
>> -{
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> -
>> - return v4l2_prio_change(&ch->prio, &fh->prio, p);
>> -}
>> -
>> -/**
>> * vpif_cropcap() - cropcap handler
>> * @file: file ptr
>> * @priv: file handle
>> @@ -1701,8 +1205,8 @@ static int vpif_s_priority(struct file *file, void *priv, enum v4l2_priority p)
>> static int vpif_cropcap(struct file *file, void *priv,
>> struct v4l2_cropcap *crop)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>> struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>>
>> if (V4L2_BUF_TYPE_VIDEO_CAPTURE != crop->type)
>
> I would drop cropcap. First of all this driver doesn't support cropping,
> so there is no need for cropcap, secondly it doesn't set pixelaspect
> which is wrong anyway.
>
OK
>> @@ -1726,8 +1230,8 @@ static int
>> vpif_enum_dv_timings(struct file *file, void *priv,
>> struct v4l2_enum_dv_timings *timings)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>> int ret;
>>
>
> A general note for all dv_timings and std ioctls: if the current input or output
> does not support the std or dv_timings ioctls, then -ENODATA should be returned.
>
> So trying to call VIDIOC_G_DV_TIMINGS for an input that only supports SDTV or
> calling G_STD for an input that only supports HDTV should result in -ENODATA.
>
> Special care should be taken with ENUMSTD: that's handled by the v4l2 core and it
> uses the videodev->tvnorms field. So that field should be updated whenever you
> change inputs. See v4l2-pci-skeleton.c which actually implements that.
>
Ok will fix it.
>> ret = v4l2_subdev_call(ch->sd, video, enum_dv_timings, timings);
>> @@ -1746,8 +1250,8 @@ static int
>> vpif_query_dv_timings(struct file *file, void *priv,
>> struct v4l2_dv_timings *timings)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>> int ret;
>>
>> ret = v4l2_subdev_call(ch->sd, video, query_dv_timings, timings);
>> @@ -1765,8 +1269,8 @@ vpif_query_dv_timings(struct file *file, void *priv,
>> static int vpif_s_dv_timings(struct file *file, void *priv,
>> struct v4l2_dv_timings *timings)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>> struct vpif_params *vpifparams = &ch->vpifparams;
>> struct vpif_channel_config_params *std_info = &vpifparams->std_info;
>> struct video_obj *vid_ch = &ch->video;
>> @@ -1853,8 +1357,8 @@ static int vpif_s_dv_timings(struct file *file, void *priv,
>> static int vpif_g_dv_timings(struct file *file, void *priv,
>> struct v4l2_dv_timings *timings)
>> {
>> - struct vpif_fh *fh = priv;
>> - struct channel_obj *ch = fh->channel;
>> + struct video_device *vdev = video_devdata(file);
>> + struct channel_obj *ch = video_get_drvdata(vdev);
>> struct video_obj *vid_ch = &ch->video;
>>
>> *timings = vid_ch->dv_timings;
>> @@ -1879,49 +1383,46 @@ static int vpif_log_status(struct file *filep, void *priv)
>>
>> /* vpif capture ioctl operations */
>> static const struct v4l2_ioctl_ops vpif_ioctl_ops = {
>> - .vidioc_querycap = vpif_querycap,
>> - .vidioc_g_priority = vpif_g_priority,
>> - .vidioc_s_priority = vpif_s_priority,
>> + .vidioc_querycap = vpif_querycap,
>> .vidioc_enum_fmt_vid_cap = vpif_enum_fmt_vid_cap,
>> - .vidioc_g_fmt_vid_cap = vpif_g_fmt_vid_cap,
>> + .vidioc_g_fmt_vid_cap = vpif_g_fmt_vid_cap,
>> .vidioc_s_fmt_vid_cap = vpif_s_fmt_vid_cap,
>> .vidioc_try_fmt_vid_cap = vpif_try_fmt_vid_cap,
>> +
>> .vidioc_enum_input = vpif_enum_input,
>> .vidioc_s_input = vpif_s_input,
>> .vidioc_g_input = vpif_g_input,
>> - .vidioc_reqbufs = vpif_reqbufs,
>> - .vidioc_querybuf = vpif_querybuf,
>> +
>> + .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_streamon = vb2_ioctl_streamon,
>> + .vidioc_streamoff = vb2_ioctl_streamoff,
>
> Add .vidioc_expbuf = vb2_ioctl_expbuf
>
> That allows apps to export dmabuf buffers from this driver.
>
OK
>> +
>> .vidioc_querystd = vpif_querystd,
>> - .vidioc_s_std = vpif_s_std,
>> + .vidioc_s_std = vpif_s_std,
>> .vidioc_g_std = vpif_g_std,
>> - .vidioc_qbuf = vpif_qbuf,
>> - .vidioc_dqbuf = vpif_dqbuf,
>> - .vidioc_streamon = vpif_streamon,
>> - .vidioc_streamoff = vpif_streamoff,
>> - .vidioc_cropcap = vpif_cropcap,
>> - .vidioc_enum_dv_timings = vpif_enum_dv_timings,
>> - .vidioc_query_dv_timings = vpif_query_dv_timings,
>> - .vidioc_s_dv_timings = vpif_s_dv_timings,
>> - .vidioc_g_dv_timings = vpif_g_dv_timings,
>> +
>> + .vidioc_cropcap = vpif_cropcap,
>> +
>> + .vidioc_enum_dv_timings = vpif_enum_dv_timings,
>> + .vidioc_query_dv_timings = vpif_query_dv_timings,
>> + .vidioc_s_dv_timings = vpif_s_dv_timings,
>> + .vidioc_g_dv_timings = vpif_g_dv_timings,
>> +
>> .vidioc_log_status = vpif_log_status,
>> };
>>
>> /* vpif file operations */
>> static struct v4l2_file_operations vpif_fops = {
>> - .owner = THIS_MODULE,
>> - .open = vpif_open,
>> - .release = vpif_release,
>> - .unlocked_ioctl = video_ioctl2,
>> - .mmap = vpif_mmap,
>> - .poll = vpif_poll
>> -};
>> -
>> -/* vpif video template */
>> -static struct video_device vpif_video_template = {
>> - .name = "vpif",
>> - .fops = &vpif_fops,
>> - .minor = -1,
>> - .ioctl_ops = &vpif_ioctl_ops,
>> + .owner = THIS_MODULE,
>> + .open = v4l2_fh_open,
>> + .release = vb2_fop_release,
>> + .unlocked_ioctl = video_ioctl2,
>> + .mmap = vb2_fop_mmap,
>> + .poll = vb2_fop_poll
>> };
>>
>> /**
>> @@ -1999,7 +1500,9 @@ static int vpif_async_bound(struct v4l2_async_notifier *notifier,
>> static int vpif_probe_complete(void)
>> {
>> struct common_obj *common;
>> + struct video_device *vdev;
>> struct channel_obj *ch;
>> + struct vb2_queue *q;
>> int i, j, err, k;
>>
>> for (j = 0; j < VPIF_CAPTURE_MAX_DEVICES; j++) {
>> @@ -2009,16 +1512,55 @@ static int vpif_probe_complete(void)
>> spin_lock_init(&common->irqlock);
>> mutex_init(&common->lock);
>> ch->video_dev->lock = &common->lock;
>> - /* Initialize prio member of channel object */
>> - v4l2_prio_init(&ch->prio);
>> - video_set_drvdata(ch->video_dev, ch);
>>
>> /* select input 0 */
>> err = vpif_set_input(vpif_obj.config, ch, 0);
>> if (err)
>> goto probe_out;
>>
>> - err = video_register_device(ch->video_dev,
>> + /* Initialize vb2 queue */
>> + q = &common->buffer_queue;
>> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> + q->io_modes = VB2_MMAP | VB2_USERPTR;
>
> Add VB2_DMABUF as well to get automatic DMABUF support.
>
OK
>> + q->drv_priv = ch;
>> + q->ops = &video_qops;
>> + q->mem_ops = &vb2_dma_contig_memops;
>> + q->buf_struct_size = sizeof(struct vpif_cap_buffer);
>> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> + q->min_buffers_needed = 3;
>
> This is a bit too high. min_buffers_needed is the minimum number of buffers
> required before you can start the DMA engine. Which for this device is either
> 1 or 2. I can't remember when the davinci DMA engine reads the pointer for
> the next buffer to DMA: does that happen when the DMA engine is at the end
> of the current frame or is that at the start of the current frame? In the
> first case min_buffers_needed should be 1 so the DMA engine will start as
> quickly as possible. In the second case it should be 2 since otherwise it
> would most likely have to repeat the first frame.
>
the DMA engine reads the pointer of next buffer at the end of next frame.
>> + q->lock = &common->lock;
>> + q->gfp_flags = GFP_DMA32;
>
> Does this make sense for a davinci? It doesn't support 64 bit DMA, so I
> would just drop this.
>
Ok will drop.
>> +
>> + err = vb2_queue_init(q);
>> + if (err) {
>> + vpif_err("vpif_display: vb2_queue_init() failed\n");
>> + vb2_dma_contig_cleanup_ctx(common->alloc_ctx);
>> + goto probe_out;
>> + }
>> +
>> + common->alloc_ctx = vb2_dma_contig_init_ctx(vpif_dev);
>> + if (IS_ERR(common->alloc_ctx)) {
>> + vpif_err("Failed to get the context\n");
>> + err = PTR_ERR(common->alloc_ctx);
>> + goto probe_out;
>> + }
>> +
>> + INIT_LIST_HEAD(&common->dma_queue);
>> +
>> + /* Initialize the video_device structure */
>> + vdev = ch->video_dev;
>> + strlcpy(vdev->name, VPIF_DRIVER_NAME, sizeof(vdev->name));
>> + vdev->release = video_device_release;
>> + vdev->fops = &vpif_fops;
>> + vdev->ioctl_ops = &vpif_ioctl_ops;
>> + vdev->lock = &common->lock;
>> + vdev->queue = q;
>> + vdev->v4l2_dev = &vpif_obj.v4l2_dev;
>> + vdev->vfl_dir = VFL_DIR_RX;
>> + set_bit(V4L2_FL_USE_FH_PRIO, &vdev->flags);
>> + video_set_drvdata(vdev, ch);
>> +
>> + err = video_register_device(vdev,
>> VFL_TYPE_GRABBER, (j ? 1 : 0));
>> if (err)
>> goto probe_out;
>> @@ -2031,6 +1573,8 @@ probe_out:
>> for (k = 0; k < j; k++) {
>> /* Get the pointer to the channel object */
>> ch = vpif_obj.dev[k];
>> + common = &ch->common[k];
>> + vb2_dma_contig_cleanup_ctx(common->alloc_ctx);
>> /* Unregister video device */
>> video_unregister_device(ch->video_dev);
>> }
>> @@ -2085,7 +1629,7 @@ static __init int vpif_probe(struct platform_device *pdev)
>>
>> while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) {
>> err = devm_request_irq(&pdev->dev, res->start, vpif_channel_isr,
>> - IRQF_SHARED, "VPIF_Capture",
>> + IRQF_SHARED, VPIF_DRIVER_NAME,
>> (void *)(&vpif_obj.dev[res_idx]->
>> channel_id));
>> if (err) {
>> @@ -2109,13 +1653,6 @@ static __init int vpif_probe(struct platform_device *pdev)
>> goto vpif_unregister;
>> }
>>
>> - /* Initialize field of video device */
>> - *vfd = vpif_video_template;
>> - vfd->v4l2_dev = &vpif_obj.v4l2_dev;
>> - vfd->release = video_device_release;
>> - snprintf(vfd->name, sizeof(vfd->name),
>> - "VPIF_Capture_DRIVER_V%s",
>> - VPIF_CAPTURE_VERSION);
>> /* Set video_dev to the video device */
>> ch->video_dev = vfd;
>> }
>> @@ -2209,6 +1746,7 @@ vpif_unregister:
>> */
>> static int vpif_remove(struct platform_device *device)
>> {
>> + struct common_obj *common;
>> struct channel_obj *ch;
>> int i;
>>
>> @@ -2219,6 +1757,8 @@ static int vpif_remove(struct platform_device *device)
>> for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++) {
>> /* Get the pointer to the channel object */
>> ch = vpif_obj.dev[i];
>> + common = &ch->common[i];
>> + vb2_dma_contig_cleanup_ctx(common->alloc_ctx);
>> /* Unregister video device */
>> video_unregister_device(ch->video_dev);
>> kfree(vpif_obj.dev[i]);
>> @@ -2226,13 +1766,12 @@ static int vpif_remove(struct platform_device *device)
>> return 0;
>> }
>>
>> -#ifdef CONFIG_PM
>> +#ifdef CONFIG_PM_SLEEP
>> /**
>> * vpif_suspend: vpif device suspend
>> */
>> static int vpif_suspend(struct device *dev)
>> {
>> -
>> struct common_obj *common;
>> struct channel_obj *ch;
>> int i;
>> @@ -2241,18 +1780,19 @@ static int vpif_suspend(struct device *dev)
>> /* Get the pointer to the channel object */
>> ch = vpif_obj.dev[i];
>> common = &ch->common[VPIF_VIDEO_INDEX];
>> +
>> + if (!vb2_is_streaming(&common->buffer_queue))
>> + continue;
>> +
>> mutex_lock(&common->lock);
>> - if (ch->usrs && common->io_usrs) {
>> - /* Disable channel */
>> - if (ch->channel_id == VPIF_CHANNEL0_VIDEO) {
>> - enable_channel0(0);
>> - channel0_intr_enable(0);
>> - }
>> - if (ch->channel_id == VPIF_CHANNEL1_VIDEO ||
>> - common->started == 2) {
>> - enable_channel1(0);
>> - channel1_intr_enable(0);
>> - }
>> + /* Disable channel */
>> + if (ch->channel_id == VPIF_CHANNEL0_VIDEO) {
>> + enable_channel0(0);
>> + channel0_intr_enable(0);
>> + }
>> + if (ch->channel_id == VPIF_CHANNEL1_VIDEO || ycmux_mode == 2) {
>> + enable_channel1(0);
>> + channel1_intr_enable(0);
>> }
>> mutex_unlock(&common->lock);
>> }
>> @@ -2273,18 +1813,19 @@ static int vpif_resume(struct device *dev)
>> /* Get the pointer to the channel object */
>> ch = vpif_obj.dev[i];
>> common = &ch->common[VPIF_VIDEO_INDEX];
>> +
>> + if (!vb2_is_streaming(&common->buffer_queue))
>> + continue;
>> +
>> mutex_lock(&common->lock);
>> - if (ch->usrs && common->io_usrs) {
>> - /* Disable channel */
>> - if (ch->channel_id == VPIF_CHANNEL0_VIDEO) {
>> - enable_channel0(1);
>> - channel0_intr_enable(1);
>> - }
>> - if (ch->channel_id == VPIF_CHANNEL1_VIDEO ||
>> - common->started == 2) {
>> - enable_channel1(1);
>> - channel1_intr_enable(1);
>> - }
>> + /* Enable channel */
>> + if (ch->channel_id == VPIF_CHANNEL0_VIDEO) {
>> + enable_channel0(1);
>> + channel0_intr_enable(1);
>> + }
>> + if (ch->channel_id == VPIF_CHANNEL1_VIDEO || ycmux_mode == 2) {
>> + enable_channel1(1);
>> + channel1_intr_enable(1);
>> }
>> mutex_unlock(&common->lock);
>> }
>> @@ -2292,21 +1833,15 @@ static int vpif_resume(struct device *dev)
>> return 0;
>> }
>>
>> -static const struct dev_pm_ops vpif_dev_pm_ops = {
>> - .suspend = vpif_suspend,
>> - .resume = vpif_resume,
>> -};
>> -
>> -#define vpif_pm_ops (&vpif_dev_pm_ops)
>> -#else
>> -#define vpif_pm_ops NULL
>> #endif
>>
>> +static SIMPLE_DEV_PM_OPS(vpif_pm_ops, vpif_suspend, vpif_resume);
>> +
>> static __refdata struct platform_driver vpif_driver = {
>> .driver = {
>> - .name = "vpif_capture",
>> + .name = VPIF_DRIVER_NAME,
>> .owner = THIS_MODULE,
>> - .pm = vpif_pm_ops,
>> + .pm = &vpif_pm_ops,
>> },
>> .probe = vpif_probe,
>> .remove = vpif_remove,
>> diff --git a/drivers/media/platform/davinci/vpif_capture.h b/drivers/media/platform/davinci/vpif_capture.h
>> index 5a29d9a..8af3b33 100644
>> --- a/drivers/media/platform/davinci/vpif_capture.h
>> +++ b/drivers/media/platform/davinci/vpif_capture.h
>> @@ -19,8 +19,6 @@
>> #ifndef VPIF_CAPTURE_H
>> #define VPIF_CAPTURE_H
>>
>> -#ifdef __KERNEL__
>> -
>> /* Header files */
>> #include <media/videobuf2-dma-contig.h>
>> #include <media/v4l2-device.h>
>> @@ -63,11 +61,6 @@ struct common_obj {
>> struct vpif_cap_buffer *cur_frm;
>> /* Pointer pointing to current v4l2_buffer */
>> struct vpif_cap_buffer *next_frm;
>> - /*
>> - * This field keeps track of type of buffer exchange mechanism
>> - * user has selected
>> - */
>> - enum v4l2_memory memory;
>> /* Used to store pixel format */
>> struct v4l2_format fmt;
>> /* Buffer queue used in video-buf */
>> @@ -80,12 +73,8 @@ struct common_obj {
>> spinlock_t irqlock;
>> /* lock used to access this structure */
>> struct mutex lock;
>> - /* number of users performing IO */
>> - u32 io_usrs;
>> - /* Indicates whether streaming started */
>> - u8 started;
>> /* Function pointer to set the addresses */
>> - void (*set_addr) (unsigned long, unsigned long, unsigned long,
>> + void (*set_addr)(unsigned long, unsigned long, unsigned long,
>> unsigned long);
>> /* offset where Y top starts from the starting of the buffer */
>> u32 ytop_off;
>> @@ -104,14 +93,8 @@ struct common_obj {
>> struct channel_obj {
>> /* Identifies video device for this channel */
>> struct video_device *video_dev;
>> - /* Used to keep track of state of the priority */
>> - struct v4l2_prio_state prio;
>> - /* number of open instances of the channel */
>> - int usrs;
>> /* Indicates id of the field which is being displayed */
>> u32 field_id;
>> - /* flag to indicate whether decoder is initialized */
>> - u8 initialized;
>> /* Identifies channel */
>> enum vpif_channel_id channel_id;
>> /* Current input */
>> @@ -126,18 +109,6 @@ struct channel_obj {
>> struct video_obj video;
>> };
>>
>> -/* File handle structure */
>> -struct vpif_fh {
>> - /* pointer to channel object for opened device */
>> - struct channel_obj *channel;
>> - /* Indicates whether this file handle is doing IO */
>> - u8 io_allowed[VPIF_NUMBER_OF_OBJECTS];
>> - /* Used to keep track priority of this instance */
>> - enum v4l2_priority prio;
>> - /* Used to indicate channel is initialize or not */
>> - u8 initialized;
>> -};
>> -
>> struct vpif_device {
>> struct v4l2_device v4l2_dev;
>> struct channel_obj *dev[VPIF_CAPTURE_NUM_CHANNELS];
>> @@ -157,5 +128,4 @@ struct vpif_config_params {
>> u8 max_device_type;
>> };
>>
>> -#endif /* End of __KERNEL__ */
>> #endif /* VPIF_CAPTURE_H */
>>
>
> Note that I'm not yet reviewing the vpif_display code. It's very similar to this
> one, so many of my comments apply there as well.
>
Ok will fix for display patch too.
> Some of my comments are really about bugs/weirdness in the existing code. So
> those should probably be fixed in separate patches.
>
OK
> Also, I would really appreciate it if you add the v4l2-compliance -s output when
> you post the next patch series. I'd like to see that.
>
Ok will post along with v2 series.
Thanks,
--Prabhakar Lad
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/