Re: [New Driver]: usbvideo2 webcam core + pac207 driver using it.

On 22:53 Fri 28 Mar 2008, Hans de Goede wrote:
>  I'm currently posting these as .c files for easy reading and
>  compilation / testing, but I still hope to get a lot of feedback / a
>  thorough review, esp of the core <-> pac207 split version as I hope
>  to submit that as a patch for mainline inclusion soon.

The driver look pretty good.  Comments inline.

> struct pac207_decompress_table_t {
> 	u8 is_abs;
> 	u8 len;
> 	s8 val;
> };

Why add the _t?

> int pac207_read_reg(struct usbvideo2_device* cam, u16 index)
> {
> 	struct usb_device* udev = cam->usbdev;
> 	u8* buff = cam->control_buffer;
> 	int res;
> 
> 	res = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), 0x00,
> 			USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
> 			0x00, index, buff, 1, USBVIDEO2_CTRL_TIMEOUT);
> 	if (res < 0)
> 		DBG(1, "Failed to read a register (index 0x%04X, error %d)",
> 			index, res);
> 
> 	return (res >= 0) ? (int)(*buff) : res;

Why not do the obvious thing and return from the if (res < 0) statement?

> /*****************************************************************************/
> 
> /* auto gain and exposure algorithm based on the knee algorithm described here:
>    http://ytse.tricolour.net/docs/LowLightOptimization.html */

URL is dead.

> #define CLIP(color) (unsigned char)(((color)>0xFF)?0xff:(((color)<0)?0:(color)))

Add a comment about what this is doing?  Could you just do it as a
static function instead?

> static int
> pac207_vidioc_s_ctrl(struct file *file, void *fh, struct v4l2_control *a)
> {
> 	struct usbvideo2_device* cam = fh;
> 	struct pac207_data *data = cam->cam_data;
> 	int new_value = a->value;
> 	int err;
> 
> 	if ((err = pac207_vidioc_g_ctrl(file, fh, a)))
> 		return err;
> 
> 	if (a->value == new_value)
> 		return 0;

This all needs some locking to protect from multi-threaded applications.
Otherwise the hardware and data structures could be in two different
states.

> 	/* don't allow mucking with gain / exposure when using autogain */
> 	if (data->autogain && (a->id == V4L2_CID_GAIN ||
> 			a->id == V4L2_CID_EXPOSURE))
> 		return -EINVAL;
> 
> 	switch (a->id) {
> 		case V4L2_CID_BRIGHTNESS:
> 			data->brightness = new_value;
> 			pac207_write_reg(cam, 0x0008, data->brightness);
> 			/* give brightness change time to take effect before
> 			   doing autogain based on the new brightness */
> 			data->autogain_ignore_frames =
> 						PAC207_AUTOGAIN_IGNORE_FRAMES;
> 			break;
> 
> 		case V4L2_CID_EXPOSURE:
> 			data->exposure = new_value;
> 			pac207_write_reg(cam, 0x0002, data->exposure);
> 			break;
> 
> 		case V4L2_CID_AUTOGAIN:
> 			data->autogain = new_value;
> 			/* when switching to autogain set defaults to make sure
> 			   we are on a valid point of the autogain gain /
> 			   exposure knee graph, and give this change time to
> 			   take effect before doing autogain. */
> 			if (data->autogain) {
> 				data->exposure = PAC207_EXPOSURE_DEFAULT;
> 				data->gain = PAC207_GAIN_DEFAULT;
> 				data->autogain_ignore_frames =
> 						PAC207_AUTOGAIN_IGNORE_FRAMES;
> 				pac207_write_reg(cam, 0x0002, data->exposure);
> 				pac207_write_reg(cam, 0x000e, data->gain);
> 			}
> 			break;
> 
> 		case V4L2_CID_GAIN:
> 			data->gain = new_value;
> 			pac207_write_reg(cam, 0x000e, data->gain);
> 			break;
> 	
> 		/* no default needed already checked in pac207_vidioc_g_ctrl */
> 	}
> 
> 	pac207_write_reg(cam, 0x13, 0x01); /* load registers to sensor */
> 	pac207_write_reg(cam, 0x1c, 0x01); /* not documented */
> 
> 	return 0;
> }

> static void usbvideo2_urb_complete(struct urb *urb)
> {
> 	struct usbvideo2_device* cam = urb->context;
> 	struct usbvideo2_frame_t** f;
> 	int i, ret;
> 
> 	switch (urb->status) {
> 		case 0:
> 			break;
> 		case -ENOENT:		/* usb_kill_urb() called. */
> 		case -ECONNRESET:	/* usb_unlink_urb() called. */
> 		case -ESHUTDOWN:	/* The endpoint is being disabled. */
> 			return;
> 		default:
> 			goto resubmit_urb;
> 	}
> 
> 	f = &cam->frame_current;
> 
> 	if (!(*f)) {
> 		if (list_empty(&cam->inqueue))
> 			goto resubmit_urb;
> 
> 		(*f) = list_entry(cam->inqueue.next, struct usbvideo2_frame_t,
> 					frame);
> 	} 

Don't you want to take a spinlock here?  Most accesses of inqueue seem
to take a spinlock.

> static ssize_t
> usbvideo2_read(struct file* filp, char __user * buf, size_t count, loff_t* f_pos)
> {
> 	struct usbvideo2_device *cam = filp->private_data;
> 	struct usbvideo2_frame_t *f;
> 	unsigned long lock_flags;
> 	long timeout;
> 	int err = 0;
> 
> 
> 	if (cam->disconnected) {
> 		err = -ENODEV;
> 		goto out;
> 	}
> 
> 	if (cam->io == IO_MMAP) {
> 		DBG(2, "Close and open the device again to choose the read "
> 			"method");
> 		err = -EBUSY;
> 		goto out;
> 	}
> 
> static void usbvideo2_vm_close(struct vm_area_struct* vma)
> {
> 	/* NOTE: buffers are not freed here */

Why is that worth noting?

> 	struct usbvideo2_frame_t* f = vma->vm_private_data;
> 	f->vma_use_count--;
> }
> 
> 
> static int
> usbvideo2_vidioc_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
> {
> 	u32 i;
> 	struct usbvideo2_device* cam = fh;
> 
> 	if (b->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> 			b->memory != V4L2_MEMORY_MMAP)
> 		return -EINVAL;
> 
> 	if (cam->io == IO_READ) {
> 		DBG(2, "Close and open the device again to choose the "
> 			"mmap I/O method");
> 		return -EBUSY;
> 	}

Again, why is close then open required?

> static int
> usbvideo2_vidioc_dqbuf(struct file *file, void *fh, struct v4l2_buffer *b)
> {
> 	struct usbvideo2_device* cam = fh;
> 	struct usbvideo2_frame_t *f;
> 	unsigned long lock_flags;
> 	long timeout;
> 
> 	if (b->type != V4L2_BUF_TYPE_VIDEO_CAPTURE || cam->io != IO_MMAP ||
> 			cam->stream == STREAM_OFF)
> 		return -EINVAL;
> 
> 	if (list_empty(&cam->outqueue)) {
> 		if (file->f_flags & O_NONBLOCK)
> 			return -EAGAIN;
> 
> 		timeout = wait_event_interruptible_timeout(cam->wait_frame,
> 				!list_empty(&cam->outqueue) ||
> 				cam->disconnected,
> 				msecs_to_jiffies(USBVIDEO2_FRAME_TIMEOUT) );
> 		if (cam->disconnected)
> 			return -ENODEV;
> 
> 		if (timeout <= 0)
> 			return (timeout < 0)? timeout : -EIO;
> 	}

Where is the locking?  What happens if two threads call dqbuf on this
device at the same time?  

> 	if (cam->funcs->frame_dequeued)
> 		cam->funcs->frame_dequeued(cam);
> 
> 	spin_lock_irqsave(&cam->queue_lock, lock_flags);

You will probably hit LIST_POISON on a second thread because of the list
being empty when it comes through.

> 	f = list_entry(cam->outqueue.next, struct usbvideo2_frame_t, frame);
> 	list_del(cam->outqueue.next);
> 	spin_unlock_irqrestore(&cam->queue_lock, lock_flags);
> 
> 	f->state = F_UNUSED;
> 
> 	memcpy(b, &f->buf, sizeof(*b));
> 	if (f->vma_use_count)
> 		b->flags |= V4L2_BUF_FLAG_MAPPED;
> 
> 	return 0;
> }

> struct usbvideo2_frame_t {
> 	void* bufmem;
> 	struct v4l2_buffer buf;
> 	enum usbvideo2_frame_state state;
> 	struct list_head frame;
> 	unsigned long vma_use_count;
> };

Why the _t on the end?

Thanks,

	Brandon

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list