au0828-vbi.c

REVIEW Hans Verkuil 2 participants 2 messages Dec 12, 2014 Media (V4L/DVB)

lore

Share & tools

lore

Hans Verkuil

author

2014-12-12 09:42 UTC #0 lore raw

Hi Shuah,

This is my review for au0828-vbi.c with your patch applied.

Quoted 25 lines

/*
au0828-vbi.c - VBI driver for au0828
Copyright (C) 2010 Devin Heitmueller <[email protected]>
This work was sponsored by GetWellNetwork Inc.
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 the Free Software Foundation; either version 2 of the License, or (at your option) any later version.
This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details.
You should have received a copy of the GNU General Public License along with this program; if not, write to the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
#include "au0828.h"

Quoted 8 lines

#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/slab.h>
static unsigned int vbibufs = 5; module_param(vbibufs, int, 0644); MODULE_PARM_DESC(vbibufs, "number of vbi buffers, range 2-32");

Drop this vbibufs argument. It's bogus.

Quoted 4 lines

/* ------------------------------------------------------------------ */
static int vbi_queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt,

Quoted 8 lines

			   unsigned int *nbuffers, unsigned int *nplanes,
			   unsigned int sizes[], void *alloc_ctxs[])
{
	struct au0828_dev *dev = vb2_get_drv_priv(vq);
	unsigned long size;

	if (fmt)
		size = fmt->fmt.pix.sizeimage;

It's a VBI format, so the size is

	fmt->fmt.vbi.samples_per_line *
                   (fmt->fmt.vbi.count[0] + fmt->fmt.vbi.count[1]);

Quoted 9 lines

	else
		size = dev->vbi_width * dev->vbi_height * 2;

	if (0 == *nbuffers)
		*nbuffers = 32;
	if (*nbuffers < 2)
		*nbuffers = 2;
	if (*nbuffers > 32)
		*nbuffers = 32;

Remove these checks, they are not needed.

But if a fmt is passed in, then you *do* need to check if the new format size is enough to store the vbi data. If not, return -EINVAL.

Test with v4l2-compliance -V0 -s.

Quoted 42 lines

	*nplanes = 1;
	sizes[0] = size;

	return 0;
}

static int vbi_buffer_prepare(struct vb2_buffer *vb)

{
	struct au0828_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
	struct au0828_buffer *buf = container_of(vb, struct au0828_buffer, vb);
	unsigned long size;

	size = dev->vbi_width * dev->vbi_height * 2;

	if (vb2_plane_size(vb, 0) < size) {
		pr_err("%s data will not fit into plane (%lu < %lu)\n",
			__func__, vb2_plane_size(vb, 0), size);
		return -EINVAL;
	}
	vb2_set_plane_payload(&buf->vb, 0, size);

	return 0;
}

static void
vbi_buffer_queue(struct vb2_buffer *vb)

{
	struct au0828_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
	struct au0828_buffer *buf = container_of(vb, struct au0828_buffer, vb);
	struct au0828_dmaqueue *vbiq = &dev->vbiq;
	unsigned long flags = 0;

	buf->mem = vb2_plane_vaddr(vb, 0);
	buf->length = vb2_plane_size(vb, 0);

	spin_lock_irqsave(&dev->slock, flags);
	list_add_tail(&buf->list, &vbiq->active);
	spin_unlock_irqrestore(&dev->slock, flags);
}

struct vb2_ops au0828_vbi_qops = {

Quoted 8 lines

	.queue_setup     = vbi_queue_setup,
	.buf_prepare     = vbi_buffer_prepare,
	.buf_queue       = vbi_buffer_queue,
.start_streaming = au0828_start_analog_streaming, .stop_streaming = au0828_stop_vbi_streaming,
.wait_prepare = vb2_ops_wait_prepare,
.wait_finish = vb2_ops_wait_finish,
};

Regards,

Hans

Shuah Khan

2014-12-12 14:54 UTC #1 lore raw

Quoted 3 lines · Hans Verkuil

Hi Shuah,
This is my review for au0828-vbi.c with your patch applied.

Thanks. Will fix all of these.

-- Shuah

Quoted 128 lines

Quoted 26 lines
/*
au0828-vbi.c - VBI driver for au0828
Copyright (C) 2010 Devin Heitmueller <[email protected]>
This work was sponsored by GetWellNetwork Inc.
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 the Free Software Foundation; either version 2 of the License, or (at your option) any later version.
This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details.
You should have received a copy of the GNU General Public License along with this program; if not, write to the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
#include "au0828.h"
Quoted 8 lines
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/slab.h>
static unsigned int vbibufs = 5; module_param(vbibufs, int, 0644); MODULE_PARM_DESC(vbibufs, "number of vbi buffers, range 2-32");
Drop this vbibufs argument. It's bogus.
Quoted 4 lines
/* ------------------------------------------------------------------ */
static int vbi_queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt,
Quoted 8 lines
			   unsigned int *nbuffers, unsigned int *nplanes,
			   unsigned int sizes[], void *alloc_ctxs[])
{
	struct au0828_dev *dev = vb2_get_drv_priv(vq);
	unsigned long size;

	if (fmt)
		size = fmt->fmt.pix.sizeimage;
It's a VBI format, so the size is
	fmt->fmt.vbi.samples_per_line *
                   (fmt->fmt.vbi.count[0] + fmt->fmt.vbi.count[1]);
Quoted 9 lines
	else
		size = dev->vbi_width * dev->vbi_height * 2;

	if (0 == *nbuffers)
		*nbuffers = 32;
	if (*nbuffers < 2)
		*nbuffers = 2;
	if (*nbuffers > 32)
		*nbuffers = 32;
Remove these checks, they are not needed.
But if a fmt is passed in, then you *do* need to check if the new format size is enough to store the vbi data. If not, return -EINVAL.
Test with v4l2-compliance -V0 -s.
Quoted 42 lines
	*nplanes = 1;
	sizes[0] = size;

	return 0;
}
static int vbi_buffer_prepare(struct vb2_buffer *vb)
{
	struct au0828_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
	struct au0828_buffer *buf = container_of(vb, struct au0828_buffer, vb);
	unsigned long size;

	size = dev->vbi_width * dev->vbi_height * 2;

	if (vb2_plane_size(vb, 0) < size) {
		pr_err("%s data will not fit into plane (%lu < %lu)\n",
			__func__, vb2_plane_size(vb, 0), size);
		return -EINVAL;
	}
	vb2_set_plane_payload(&buf->vb, 0, size);

	return 0;
}
static void
vbi_buffer_queue(struct vb2_buffer *vb)
{
	struct au0828_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
	struct au0828_buffer *buf = container_of(vb, struct au0828_buffer, vb);
	struct au0828_dmaqueue *vbiq = &dev->vbiq;
	unsigned long flags = 0;

	buf->mem = vb2_plane_vaddr(vb, 0);
	buf->length = vb2_plane_size(vb, 0);

	spin_lock_irqsave(&dev->slock, flags);
	list_add_tail(&buf->list, &vbiq->active);
	spin_unlock_irqrestore(&dev->slock, flags);
}
struct vb2_ops au0828_vbi_qops = {
Quoted 8 lines
	.queue_setup     = vbi_queue_setup,
	.buf_prepare     = vbi_buffer_prepare,
	.buf_queue       = vbi_buffer_queue,
.start_streaming = au0828_start_analog_streaming, .stop_streaming = au0828_stop_vbi_streaming,
.wait_prepare = vb2_ops_wait_prepare,
.wait_finish = vb2_ops_wait_finish,
};
Regards,
Hans

Signature · 5 lines

Shuah Khan
Sr. Linux Kernel Developer
Samsung Open Source Group
Samsung Research America (Silicon Valley)
[email protected] | (970) 217-8978