[GIT PATCHES FOR 3.1] New SE401 driver + major pwc driver cleanup

[GIT PATCHES FOR 3.1] New SE401 driver + major pwc driver cleanup

[Hans de Goede]

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

Re: [GIT PATCHES FOR 3.1] New SE401 driver + major pwc driver cleanup

[Hans Verkuil]

Hi Hans,

I have some notes:

On Sunday, July 03, 2011 22:00:39 Hans de Goede wrote:
> 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

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 :-)

>        pwc: properly allocate dma-able memory for ISO buffers
>        pwc: Replace control code with v4l2-ctrls framework

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

>        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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [GIT PATCHES FOR 3.1] New SE401 driver + major pwc driver cleanup

[Hans de Goede]

Hi,

On 07/04/2011 11:15 AM, Hans Verkuil wrote:
> Hi Hans,
>
> I have some notes:
>
> On Sunday, July 03, 2011 22:00:39 Hans de Goede wrote:

<snip>

>> 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
>
> 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 :-)

I agree that having this in vb2 would be good!

>>         pwc: properly allocate dma-able memory for ISO buffers
>>         pwc: Replace control code with v4l2-ctrls framework
>
> 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.

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 also wonder if most/all of these controls are not better done as camera
> controls (CLASS_CAMERA).

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.

> Regarding auto-whitebalance: this 'overwrites' the standard auto-whitebalance
> control: should we perhaps change the standard awb control instead to a menu?

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.

> Or should we add a separate menu control for the lighting condition?

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.

> With respect to the autocluster part: that needs to be revisited. We need to
> clearly define acceptable behaviors on the part of the driver

Right, see the other thread,

> and codify that
> in the autocluster support of the framework.

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.

>
> 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.

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