New SE401 driver + major pwc driver cleanup

Hi All,

I'm happy to present my latest webcam work to you:

I could not just stand by watching the old v4l1 se401 driver (which has been broken for a long while btw) get removed from the kernel, without writing a replacement, so I'm happy to present a new, actually working, gspca based v4l2 driver for se401 based webcams :)

I've also wanted to do some much needed cleanups to the pwc driver for a long while. When I finally started with this I ended up with just replacing large parts with the new v4l2 framework, so after this patch set pwc now features:
-videobuf2 for buffer management
-ctrls handled by the control framework, including proper
setting inactive of foo controls when autofoo is on, etc.
-new v4l2 controls for pan/tilt on models with pan/tilt
to replace the non standard sysfs interface for this

May I also point your attention to the feature-removal-schedule commit, which adds a whole bunch of custom pwc API's / ioctls for removal, since we can handle this all fine with v4l2. If you think some of these should not be removed speak up now, or hold your silence for ever :)

The following changes since commit 0c2ec360f0228bbc0c0eb6f115839d39fbbd9c61:

[media] v4l2-event.h: add overview documentation to the header (2011-07-01 20:54:03 -0300)

are available in the git repository at:
git://linuxtv.org/hgoede/gspca.git media-for_v3.1

Hans Verkuil (1):
[media] v4l2-ctrls.c: add support for V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK

Hans de Goede (22):
videodev2.h Add SE401 compressed RGB format
gspca: reset image_len to 0 on LAST_PACKET when discarding frame
gspca: Add new se401 camera driver
gspca_sunplus: Fix streaming on logitech quicksmart 420
gspca: s/strncpy/strlcpy/
pwc: better usb disconnect handling
pwc: Remove a bunch of bogus sanity checks / don't return EFAULT wrongly
pwc: remove __cplusplus guards from private header
pwc: Replace private buffer management code with videobuf2
pwc: Fix non CodingStyle compliant 3 space indent in pwc.h
pwc: Get rid of error_status and unplugged variables
pwc: Remove some unused PWC_INT_PIPE left overs
pwc: Make power-saving a per device option
pwc: Move various initialization to driver load and / or stream start
pwc: Allow multiple opens
pwc: properly allocate dma-able memory for ISO buffers
pwc: Replace control code with v4l2-ctrls framework
pwc: Allow dqbuf / read to complete while waiting for controls
pwc: Add v4l2 controls for pan/tilt on Logitech QuickCam Orbit/Sphere
pwc: Add a bunch of pwc custom API to feature-removal-schedule.txt
pwc: Enable power-management by default on tested models
pwc: clean-up header files

 Documentation/DocBook/media/v4l/pixfmt.xml         |    5 +
 .../DocBook/media/v4l/vidioc-subscribe-event.xml   |   36 +-
 Documentation/feature-removal-schedule.txt         |   35 +
 drivers/media/video/gspca/Kconfig                  |   10 +
 drivers/media/video/gspca/Makefile                 |    2 +
 drivers/media/video/gspca/gspca.c                  |   11 +-
 drivers/media/video/gspca/se401.c                  |  774 +++++++++++
 drivers/media/video/gspca/se401.h                  |   90 ++
 drivers/media/video/gspca/sunplus.c                |    3 -
 drivers/media/video/gspca/t613.c                   |    2 +-
 drivers/media/video/pwc/Kconfig                    |    1 +
 drivers/media/video/pwc/pwc-ctrl.c                 |  805 ++----------
 drivers/media/video/pwc/pwc-dec1.c                 |   28 +-
 drivers/media/video/pwc/pwc-dec1.h                 |    8 +-
 drivers/media/video/pwc/pwc-dec23.c                |   22 -
 drivers/media/video/pwc/pwc-dec23.h                |   10 -
 drivers/media/video/pwc/pwc-if.c                   | 1399 ++++++--------------
 drivers/media/video/pwc/pwc-ioctl.h                |  322 -----
 drivers/media/video/pwc/pwc-kiara.c                |    1 -
 drivers/media/video/pwc/pwc-misc.c                 |    4 -
 drivers/media/video/pwc/pwc-uncompress.c           |   17 +-
 drivers/media/video/pwc/pwc-uncompress.h           |   40 -
 drivers/media/video/pwc/pwc-v4l.c                  | 1256 +++++++++++--------
 drivers/media/video/pwc/pwc.h                      |  404 +++---
 drivers/media/video/v4l2-ctrls.c                   |    3 +-
 include/linux/videodev2.h                          |    4 +-
 26 files changed, 2473 insertions(+), 2819 deletions(-)
 create mode 100644 drivers/media/video/gspca/se401.c
 create mode 100644 drivers/media/video/gspca/se401.h
 delete mode 100644 drivers/media/video/pwc/pwc-ioctl.h
 delete mode 100644 drivers/media/video/pwc/pwc-uncompress.h

Regards,

Hans

Hi Hans,

I have some notes:

(2011-07-01 20:54:03 -0300)

V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK

wrongly

Snippet from this patch:

@@ -727,6 +740,9 @@ static int pwc_streamon(struct file *file, void *fh, enum v4l2_buf_type i)

        if (!pdev->udev)
                return -ENODEV;

+ if (pdev->capt_file != file)
+ return -EBUSY;
+

        return vb2_streamon(&pdev->vb_queue, i);

}

This really needs to be codified in vb2. I'll see if I can do some work on this. Drivers need to keep track of this themselves at the moment, which varying degrees of success :-)

The private controls should get their own range in videodev2.h. Something like:

/* Reserve range USER | 0x1000 to USER | 0x1020 for the pwc drivers */ #define V4L2_CID_USER_PWC_BASE (V4L2_CTRL_CLASS_USER | 0x1000)

The control IDs themselves should be either added to videodev2.h or to a public pwc.h header.

I also wonder if most/all of these controls are not better done as camera controls (CLASS_CAMERA).

Regarding auto-whitebalance: this 'overwrites' the standard auto-whitebalance
control: should we perhaps change the standard awb control instead to a menu? Or should we add a separate menu control for the lighting condition?

With respect to the autocluster part: that needs to be revisited. We need to clearly define acceptable behaviors on the part of the driver and codify that in the autocluster support of the framework. You have to do way too much work in the driver right now.

None of this prevents this series from being merged, BTW.

Another thing that needs to be looked at is the use of the priv field in VIDIOC_S_FMT:

        if (f->fmt.pix.priv) {
                compression = (f->fmt.pix.priv & PWC_QLT_MASK) >> 
PWC_QLT_SHIFT;
                snapshot = !!(f->fmt.pix.priv & PWC_FPS_SNAPSHOT);
                fps = (f->fmt.pix.priv & PWC_FPS_FRMASK) >> PWC_FPS_SHIFT;
                if (fps == 0)
                        fps = pdev->vframes;
        }

I think at least the fps part should be done through G/S_PARM. What the others do is not clear to me.

Regards,

Hans

++++++--------------

+++++++++++--------

Hi,

<snip>

I agree that having this in vb2 would be good!

Hmm, note that pwc-ioctl.h already has CID's for these, but they use the old V4L2_CID_PRIVATE_BASE for this, which the ctrl framework does not like, so I looked at the private controls from the vivi driver and mimicked that. If we can somehow make the ctrl framework except these old CID-s, then that would be great as then we can keep using the same CID-s for now.

I agree that it would be better to map these to standardized controls. Proposal:
contour -> This really is just sharpness I believe, map the sharpness
autocontour -> Add a new autosharpness CID, this will be a user class control,
since sharpness itself is too.
noise_reduction -> new dynamic noise reduction cid. I can see this being available
on non cameras too, so I suggest using a user class cid for this.

save_user, restore_user, restore_factory settings buttons, very pwc specific IMHO, best kept as pwc private controls, preferably using the old CID-s.

That could work. I'm not sure how unique the pwc is here, it basically has 3 modes:
1) manual
2) auto
3) use a preset setting

Where 3 is split into 3 presets, outdoor, indoor incandescent, indoor fluorescent Note that in the hardware this is mapped as 0-4 into a single register.

I'm not in favor of that, that only leads to more controls without any added value, a simple menu with:
auto
manual
outdoor preset
indoor incandescent preset
indoor fluorescent preset

Is best IMHO. I'm not sure if we should make the awb control be a menu always and standardize on these, since so far I've only seen them on the pwc.

While on the subject of standardizing various controls, I would like to see the light freq menu be extended with an auto option. Some ov webcams sensors have a mode where they figure out if they need to filter out 50 or 60 hz flicker themselves. Currently this is used in gspca, once gspca moves over to the framework it would be nice if this option would be there so it does not need to override the std menu control.

While on the subject of white-balance, many many webcams don't have red + blue balance, but rather they have red + blue + green gains (as well as a separate over all gain) I would very much like to see 3 new standard controls for this, currently some drivers hack around this emulating 2 balances, others use private cid-s, etc ...

One issue with adding 3 new COLOR_GAIN CID-s is that V4L2_CID_AUTO_WHITE_BALANCE is a user control, so these would have to be user controls too, even though they are more or less camera controls ...

NB quite a few sensors have 2 green gains (one for each green pixel in a 2x2 bayer tile) but I've yet to see any sensor where setting these to 2 different values does anything useful (usually this leads to an ugly checker board effect), so I think1 green gain cid will be fine.

Right, see the other thread,

Once we've defined the acceptable behavior moving parts of this to the framework would be great.

Notice that I did the whole caching of values mostly because of
a shortcoming in the framework which can be addressed before we've defined the acceptable behavior. Currently when a cluster gets set, you can see which values have actually changed with the is_new flag, but when a volatile cluster gets read you cannot see which values are actually being read by the application.

If there were a flag for that the caching could be removed.

Yeah, notice I've added this to the deprecation schedule. The plan is to add G/S_PARM support for FPS, and always choose the lowest compression which will get the desired FPS. Note that the driver already does the last bit, if it cannot do the requested fps at the user / module param selected compression, it automatically ups the compression.

Regards,

Hans