* [GIT PATCHES FOR 3.1] New SE401 driver + major pwc driver cleanup
@ 2011-07-03 20:00 Hans de Goede
2011-07-04 9:15 ` Hans Verkuil
0 siblings, 1 reply; 3+ messages in thread
From: Hans de Goede @ 2011-07-03 20:00 UTC (permalink / raw)
To: Linux Media Mailing List
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [GIT PATCHES FOR 3.1] New SE401 driver + major pwc driver cleanup
2011-07-03 20:00 [GIT PATCHES FOR 3.1] New SE401 driver + major pwc driver cleanup Hans de Goede
@ 2011-07-04 9:15 ` Hans Verkuil
2011-07-12 13:51 ` Hans de Goede
0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil @ 2011-07-04 9:15 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List
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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [GIT PATCHES FOR 3.1] New SE401 driver + major pwc driver cleanup
2011-07-04 9:15 ` Hans Verkuil
@ 2011-07-12 13:51 ` Hans de Goede
0 siblings, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2011-07-12 13:51 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-07-12 13:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-03 20:00 [GIT PATCHES FOR 3.1] New SE401 driver + major pwc driver cleanup Hans de Goede
2011-07-04 9:15 ` Hans Verkuil
2011-07-12 13:51 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.