Add support for OmniVision OV534 based USB cameras.

Adds suport for OmniVision OV534 based cameras:
- Hercules Blog Webcam
- Hercules Dualpix HD Webcam
- Sony HD PS3 Eye (SLEH 00201)

Currently only supports 640x480 YUYV non-interlaced output.

Signed-off-by Mark Ferrell <[email protected]>

Quoted 6 lines · [email protected] wrote:

Adds suport for OmniVision OV534 based cameras:
- Hercules Blog Webcam
- Hercules Dualpix HD Webcam
- Sony HD PS3 Eye (SLEH 00201)
Currently only supports 640x480 YUYV non-interlaced output.

Signed-off-by Mark Ferrell <[email protected]>

Hi Mark,

Have you taken a look at the ov519 driver which is currently in gspca, which is in 2.6.27rc1 and more general (latest version) available here:
http://linuxtv.org/hg/~jfrancois/gspca/

That driver does do jpeg, maybe it can give some clues. gspca is a webcam driver framework. Would you consider porting your driver to gspca, I (we ?) really want to see all usb webcam drivers start using the gspca framework to share as much code as possible.

Review

Quoted 5 lines

(quoted diff) +2 -0

+The ov534 outputs frames in YUYV format, non-interlaced, at 640x480. This
+format does not yet have wide support among user-land applications.

Though at

(quoted diff) +2 -0

+the time of this writing xawtv was known to work correctly.
+

This (custom cam formats) was a big problem with gspca too, for this I've written libv4l, which is a library which does format conversion from many cam specific formats to more general formats in userspace. A joined effort between Debian, Suse and Fedora is currently working on making all v4l apps use libv4l, patches have already been written for gstreamer (cheese), pwlib (ekiga) and xawtv.

For more on libv4l see:
http://hansdegoede.livejournal.com/3636.html
http://linuxtv.org/v4lwiki/index.php/Libv4l_Progress

Maybe you can write a patch to add YUYV input support to libv4l, if you do that please base your work on the latest version which is available here:
http://linuxtv.org/hg/~hgoede/v4l-dvb

Quoted 9 lines · Hans de Goede

Quoted 6 lines · [email protected] wrote:

Adds suport for OmniVision OV534 based cameras:
- Hercules Blog Webcam
- Hercules Dualpix HD Webcam
- Sony HD PS3 Eye (SLEH 00201)
Currently only supports 640x480 YUYV non-interlaced output.

Signed-off-by Mark Ferrell <[email protected]>

Hi Mark,

Quoted 8 lines

Have you taken a look at the ov519 driver which is currently in gspca, which is in 2.6.27rc1 and more general (latest version) available here:
http://linuxtv.org/hg/~jfrancois/gspca/

That driver does do jpeg, maybe it can give some clues. gspca is a webcam driver framework. Would you consider porting your driver to gspca, I (we ?) really want to see all usb webcam drivers start using the gspca framework to share as much code as possible.

I would definitely be willing to merge the code into an existing driver, though I was under the impression that the gspca core was for ISOC based USB devices. The ov534's imagine end-point is bulk transfer, with the audio endpoints being isoc.

Quoted 16 lines

Review

Quoted 5 lines

(quoted diff) +2 -0

+The ov534 outputs frames in YUYV format, non-interlaced, at 640x480. This
+format does not yet have wide support among user-land applications.

Though at

(quoted diff) +2 -0

+the time of this writing xawtv was known to work correctly.
+

This (custom cam formats) was a big problem with gspca too, for this I've written libv4l, which is a library which does format conversion from many cam specific formats to more general formats in userspace. A joined effort between Debian, Suse and Fedora is currently working on making all v4l apps use libv4l, patches have already been written for gstreamer (cheese), pwlib (ekiga) and xawtv.

For more on libv4l see:
http://hansdegoede.livejournal.com/3636.html
http://linuxtv.org/v4lwiki/index.php/Libv4l_Progress

Quoted 3 lines

Maybe you can write a patch to add YUYV input support to libv4l, if you do that please base your work on the latest version which is available here:
http://linuxtv.org/hg/~hgoede/v4l-dvb

Thanks, will take a look.

Quoted 5 lines

I would definitely be willing to merge the code into an existing driver, though I was under the impression that the gspca core was for ISOC based USB devices. The ov534's imagine end-point is bulk transfer, with the audio endpoints being isoc.

Ah yes it is I didn't know non isoc cams existed, so thats why your driver is so small I already was sorta missing the isoc setup stufff :)

In that case its fine as is. Mauro as this is a new driver and looks clean (and uses videobuf) any chance this can get merged for 2.6.27 ?

On Sat, 16 Aug 2008 10:03:13 +0200

Quoted 8 lines

Quoted 5 lines

I would definitely be willing to merge the code into an existing driver, though I was under the impression that the gspca core was for ISOC based USB devices. The ov534's imagine end-point is bulk transfer, with the audio endpoints being isoc.

Ah yes it is I didn't know non isoc cams existed, so thats why your driver is so small I already was sorta missing the isoc setup stufff :)

The usage of videobuf simplifies a lot the driver logic. That's why it is so small :)

I think it would be good if gspca could also use videobuf in the future.

linux/drivers/media/video/ov534.c new file +10 -0

+	/* Updates stream time */
+
+	dev->ms += jiffies_to_msecs(jiffies-dev->jiffies);
+	dev->jiffies = jiffies;
+	if (dev->ms >= 1000) {
+		dev->ms -= 1000;
+		dev->s++;
+		if (dev->s >= 60) {
+			dev->s -= 60;
+			dev->m++;

Hi Mark,

First of all, please review your patch with checkpatch.pl. I've seen a few non-compliances.

If you're using -hg tree, this is as simple as doing: make checkpatch. Otherwise, you can use v4l/scripts/checkpatch.pl <my patch>

I have also a few other comments:

Quoted 1 line

+ switch (data & 0xFF) {

The better is that all hexadecimal values to be on lowercase.

linux/drivers/media/video/ov534.c line 318

Quoted 7 lines

linux/drivers/media/video/ov534.c new file +7 -0

+static void ov534_setup(struct usb_device *udev)
+{
+     ov534_reg_verify_write(udev, 0xe7, 0x3a);
+
+     ov534_reg_write(udev, OV534_REG_ADDRESS, 0x60);
+     ov534_reg_write(udev, OV534_REG_ADDRESS, 0x60);
+     ov534_reg_write(udev, OV534_REG_ADDRESS, 0x60);

Hmm... three times the same write?

linux/drivers/media/video/ov534.c line 321

Quoted 3 lines

linux/drivers/media/video/ov534.c new file +3 -0

+     ov534_reg_write(udev, OV534_REG_ADDRESS, 0x42);
+
+     ov534_reg_verify_write(udev, 0xc2,0x0c);

       ...

Quoted 1 line

+ ov534_reg_verify_write(udev, 0xe7,0x3e);

It is generally better and smaller to have a table instead of those long init sequences.

linux/drivers/media/video/ov534.c line 503

Quoted 6 lines

linux/drivers/media/video/ov534.c new file +5 -0

+static void ov534_fillbuff(struct ov534_dev *dev, struct ov534_buffer *buf)
+{
 
+     tmpbuf = kmalloc(size + 4, GFP_ATOMIC);
+     if (!tmpbuf)
+             return;

It would be better if you allocate the buffer at videobuf prep routines, instead of doing this for every buffer fill. Is there any reason why you just don't use vbuf instead of doing double buffering?

Quoted 10 lines

linux/drivers/media/video/ov534.c new file +10 -0

+     /* Updates stream time */
+
+     dev->ms += jiffies_to_msecs(jiffies-dev->jiffies);
+     dev->jiffies = jiffies;
+     if (dev->ms >= 1000) {
+             dev->ms -= 1000;
+             dev->s++;
+             if (dev->s >= 60) {
+                     dev->s -= 60;
+                     dev->m++;

linux/drivers/media/video/ov534.c line 539

Quoted 10 lines

linux/drivers/media/video/ov534.c new file +10 -0

+                     if (dev->m > 60) {
+                             dev->m -= 60;
+                             dev->h++;
+                             if (dev->h > 24)
+                                     dev->h -= 24;
+                     }
+             }
+     }
+     sprintf(dev->timestr, "%02d:%02d:%02d:%03d",
+                     dev->h, dev->m, dev->s, dev->ms);

You don't need the above. This is at vivi just because we want to print a timestamp at the video. Just remove it and timestr from dev.

I also didn't see any code to provide S1/S3 hibernation. This is probably not needed at vivi (since there's no hardware registers to be saved/restored - yet
- I never tried to do suspend/resume with vivi working), but for sure this is needed for real devices ;) I suggest you to take a look, for example, at cafe_ccic, for its implementation (it is inside #ifdef CONFIG_PM).

Done

Quoted 1 line · On Sat, 16 Aug 2008 07:13:37 -0500 "Mark Ferrell" <[email protected]> wrote:

Done

Mark,

Any news?

Cheers,
Mauro

Actually I've started a port of this driver to gspca as an exercise. You can find a rough preview here:
http://shell.studenti.unina.it/~ospite/tmp/gspca_ov534-20081111-1733.tar.bz2

I tried to (ab)use gpsca infrastructure for bulk transfers, the driver is quite essential but it works acceptably well, for now, even if I still loose fames because of some bug.

The driver needs the attached changes (or any better equivalent) to gspca bulk transfers code.

Quoted 1 line · On Tue, 11 Nov 2008 18:42:00 +0100 Antonio Ospite <[email protected]>…

Actually I've started a port of this driver to gspca as an exercise.

And here's the direct link to the file, sorry for the tarball:
http://shell.studenti.unina.it/~ospite/tmp/gspca_ov534-20081111-1733/ov534.c

Quoted 4 lines

I tried to (ab)use gpsca infrastructure for bulk transfers, the driver is quite essential but it works acceptably well, for now, even if I still loose fames because of some bug.

Quoted 2 lines

The driver needs the attached changes (or any better equivalent) to gspca bulk transfers code.

I forgot to say that the changes are against linux-2.6.28-rc4 from linus git tree.

Quoted 3 lines

Quoted 1 line · Antonio Ospite

Actually I've started a port of this driver to gspca as an exercise.

Thanks, good work! Did this reduce the driver in size, iow do you think it makes writing usb webcam driver easy, any improvements we could make?

Quoted 3 lines

I forgot to say that the changes are against linux-2.6.28-rc4 from linus git tree.

If you do gspca based drivers, you will want to use jfmoine's tree as a base:
http://linuxtv.org/hg/~jfrancois/gspca/

Use "hg clone http://linuxtv.org/hg/~jfrancois/gspca/" to get it. hint if you don't have the hg command it is part of mercurial.

Regards,

Hans

Well, with my hacks to gspca.c the ov534 driver has become really trivial. The source has shrunk from 33K to 13K. But these hacks could not be accepted though :) But, yes, the opinion on gspca is positive.

The improvement that I always dream to see is to have bridge and sensor
drivers split, so sensor drivers can be shared, a-la soc_camera, I mean.

Bringing that idea to the extreme, one could think even to share sensor
drivers with the soc_camera framework itself, but I only have this abstract suggestion, no idea at all about how it can be done, sorry. Could it be a GSoC project for next summer?

Quoted 3 lines · Antonio Ospite

Well, with my hacks to gspca.c the ov534 driver has become really trivial. The source has shrunk from 33K to 13K. But these hacks could not be accepted though :) But, yes, the opinion on gspca is positive.

Hello Antonio,

Thank you for your opinion.

I looked again at your subdriver, and it seems good to me. So forget about mine which is too buggy.

About your hacks to gspca, there are good and bad ideas. The good idea is to have the bulk_nurbs parameter. The bad idea is to force it to one when no set. To preserve the compatibility, the bulk_nurbs may be set to some value for webcams which accept permanent bulk read, the submit being done by gspca. For the other webcams, as those in finepix, a null bulk_nurbs will indicate that the bulk read requests are done by the subdriver. Is it OK for you?

Also, I saw a little problem in your subdriver: in pkt_scan, you use a static variable (count). This does not work with many active webcams and also after stop / restart streaming. Instead, you may know the current byte count using the frame values data and data_end.

So (cam->bulk_nurbs == 0) would mean that the subdriver takes care of usb tranfers, right? That would also imply that bulk_irq() is not set for these drivers and sd_pkt_scan() is never called which looks fair to me.

Quoted 4 lines · Antonio Ospite

So (cam->bulk_nurbs == 0) would mean that the subdriver takes care of usb tranfers, right? That would also imply that bulk_irq() is not set for these drivers and sd_pkt_scan() is never called which looks fair to me.

	[snip]

Yes. In the finepix subdriver, the 'complete' function of the URB is changed to a local function which does the packet analysis and restarts the next transfer after a delay.

I attached the patch of the main driver.

Cheers.

Signature · 7 lines

Ken ar c'hentañ |             ** Breizh ha Linux atav! **
Jef             |               http://moinejf.free.fr/



[gspca_bulk.patch  text/x-patch (1,9KB)]
diff -r 417024f56f55 linux/drivers/media/video/gspca/gspca.c

linux/drivers/media/video/gspca/gspca.c +1 -0

@@ -213,6 +213,7 @@
 {
 	struct gspca_dev *gspca_dev = (struct gspca_dev *) urb->context;
 	struct gspca_frame *frame;
+	int st;
 
 	PDEBUG(D_PACK, "bulk irq");
 	if (!gspca_dev->streaming)

linux/drivers/media/video/gspca/gspca.c +3 -0

+		st = usb_submit_urb(urb, GFP_ATOMIC);
+		if (st < 0)
+			PDEBUG(D_ERR|D_PACK, "usb_submit_urb() ret %d", st);
 	}
 }
 

@@ -533,11 +541,14 @@
 		nurbs = DEF_NURBS;

linux/drivers/media/video/gspca/gspca.c +4 -2

+		else
+			nurbs = 1;
 	}
 
 	gspca_dev->nurbs = nurbs;

@@ -625,8 +636,8 @@
 		gspca_dev->streaming = 1;
 		atomic_set(&gspca_dev->nevent, 0);
 
-		/* bulk transfers are started by the subdriver */
-		if (gspca_dev->alt == 0)
+		/* some bulk transfers are started by the subdriver */
+		if (gspca_dev->alt == 0 && gspca_dev->cam.bulk_nurbs == 0)
 			break;
 
 		/* submit the URBs */

diff -r 417024f56f55 linux/drivers/media/video/gspca/gspca.h

linux/drivers/media/video/gspca/gspca.h +4 -0

@@ -58,6 +58,10 @@
 	int bulk_size;		/* buffer size when image transfer by bulk */
 	struct v4l2_pix_format *cam_mode;	/* size nmodes */
 	char nmodes;
+	__u8 bulk_nurbs;	/* number of URBs in bulk mode
+				 * - cannot be > MAX_NURBS
+				 * - when 0 and bulk_size != 0 means
+				 *   1 URB and submit done by subdriver */
 	__u8 epaddr;
 };
 

Quoted 1 line

I attached the patch of the main driver.

Many thanks Jean-Francois, any ETA of this change in mainline?

linux/drivers/media/video/gspca/gspca.c line 242

Quoted 2 lines

linux/drivers/media/video/gspca/gspca.c +2 -0

+	/* resubmit the URB */
+	if (gspca_dev->cam.bulk_nurbs != 0) {

Just to be sure: this check is still needed because bulk_irq() can be still called even for drivers like finepix, right?

I am not sure I've fully understood the code path here, maybe I should look better at what the finepix driver does, sorry, does it still uses the urb initialized in create_urbs(), even if it drives the submission manually?

linux/drivers/media/video/gspca/gspca.c line 549

Quoted 2 lines

linux/drivers/media/video/gspca/gspca.c +2 -0

+		if (gspca_dev->cam.bulk_nurbs != 0)
+			nurbs = gspca_dev->cam.bulk_nurbs;

should we set this to min(gspca_dev->cam.bulk_nurbs, MAX_NURBS) ?

Quoted 3 lines · Antonio Ospite

Quoted 1 line

I attached the patch of the main driver.

Many thanks Jean-Francois, any ETA of this change in mainline?

Sorry, what do you mean by ETA? I uploaded the patch to my repository with you as the author.

Quoted 4 lines

linux/drivers/media/video/gspca/gspca.c line 549

Quoted 2 lines

linux/drivers/media/video/gspca/gspca.c +2 -0

+		if (gspca_dev->cam.bulk_nurbs != 0)
+			nurbs = gspca_dev->cam.bulk_nurbs;

should we set this to min(gspca_dev->cam.bulk_nurbs, MAX_NURBS) ?

Yes, we should, but, if a greater value is set, the subdriver will crash at the first development test...