[PATCH v2] [media] omap3isp: queue: fail QBUF if user buffer is too small
Add buffer length check to sanity checks for USERPTR QBUF
Signed-off-by: Michael Jones michael.jo...@matrix-vision.de
---
Changes for v2:
- only check when V4L2_MEMORY_USERPTR
drivers/media/video/omap3isp/ispqueue.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/omap3isp/ispqueue.c
b/drivers/media/video/omap3isp/ispqueue.c
index 9c31714..9bebb1e 100644
--- a/drivers/media/video/omap3isp/ispqueue.c
+++ b/drivers/media/video/omap3isp/ispqueue.c
@@ -868,6 +868,10 @@ int omap3isp_video_queue_qbuf(struct isp_video_queue
*queue,
goto done;
if (vbuf-memory == V4L2_MEMORY_USERPTR
+ vbuf-length buf-vbuf.length)
+ goto done;
+
+ if (vbuf-memory == V4L2_MEMORY_USERPTR
vbuf-m.userptr != buf-vbuf.m.userptr) {
isp_video_buffer_cleanup(buf);
buf-vbuf.m.userptr = vbuf-m.userptr;
--
1.7.6
MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6 v4] V4L: add two new ioctl()s for multi-size videobuffer management
On Tuesday, August 09, 2011 00:06:10 Laurent Pinchart wrote:
Hi Hans,
On Monday 08 August 2011 14:40:27 Hans Verkuil wrote:
On Monday, August 08, 2011 13:40:23 Laurent Pinchart wrote:
On Monday 08 August 2011 11:16:41 Hans Verkuil wrote:
On Friday, August 05, 2011 09:47:13 Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different
sizes in V4L2 is required for an efficient implementation of
asnapshot mode. This patch adds two new ioctl()s: VIDIOC_CREATE_BUFS
and
VIDIOC_PREPARE_BUF and defines respective data structures.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
v4:
1. CREATE_BUFS now takes an array of plane sizes and a fourcc code in
its argument, instead of a frame format specification, including
documentation update
2. documentation improvements, as suggested by Hans
3. increased reserved fields to 18, as suggested by Sakari
Documentation/DocBook/media/v4l/io.xml | 17 ++
Documentation/DocBook/media/v4l/v4l2.xml |2 +
.../DocBook/media/v4l/vidioc-create-bufs.xml | 161
.../DocBook/media/v4l/vidioc-prepare-buf.xml | 96
drivers/media/video/v4l2-compat-ioctl32.c |
6 + drivers/media/video/v4l2-ioctl.c | 26 +++
include/linux/videodev2.h | 18 +++
include/media/v4l2-ioctl.h |2 + 8 files
changed, 328 insertions(+), 0 deletions(-)
create mode 100644
Documentation/DocBook/media/v4l/vidioc-create-bufs.xml create mode
100644 Documentation/DocBook/media/v4l/vidioc-prepare-buf.xml
snip
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index fca24cc..3cd0cb3 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -653,6 +653,9 @@ struct v4l2_buffer {
#define V4L2_BUF_FLAG_ERROR 0x0040
#define V4L2_BUF_FLAG_TIMECODE 0x0100 /* timecode field is
valid */
#define V4L2_BUF_FLAG_INPUT 0x0200 /* input field is valid */
+/* Cache handling flags */
+#define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE0x0400
+#define V4L2_BUF_FLAG_NO_CACHE_CLEAN 0x0800
/*
* O V E R L A Y P R E V I E W
@@ -2092,6 +2095,18 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers index...index + count - 1
have been
created */
+ __u32 count;
+ __u32 type;
+ __u32 memory;
+ __u32 fourcc;
+ __u32 num_planes;
+ __u32 sizes[VIDEO_MAX_PLANES];
+ __u32 reserved[18];
+};
I know you are going to hate me for this, but I've changed my mind: I
think this should use a struct v4l2_format after all.
This change of heart came out of discussions during the V4L2 brainstorm
meeting last week. The only way to be sure the buffers are allocated
optimally is if the driver has all the information. The easiest way to
do that is by passing struct v4l2_format. This is also consistent with
REQBUFS since that uses the information from the currently selected
format (i.e. what you get back from VIDIOC_G_FMT).
There can be subtle behaviors such as allocating from different memory
back based on the fourcc and the size of the image.
One reason why I liked passing sizes directly is that it allows the
caller to ask for more memory than is strictly necessary.
However, while brainstorming last week the suggestion was made that
there is no reason why the user can't set the sizeimage field in
v4l2_pix_format(_mplane) to something higher. The S/TRY_FMT spec
explicitly mentions that the sizeimage field is set by the driver, but
for the new CREATEBUFS ioctl no such limitation has to be placed. The
only thing necessary is to ensure that sizeimage is not too small (and
you probably want some sanity check against crazy values as well).
We need to decide on a policy here. What should be the maximum allowable
size for MMAP buffers ? How do we restrict the requested image size so
that application won't be allowed to starve the system by requesting
memory for 1GP images ?
Either just a arbitrary cap like 1 GB (mainly to prevent any weird
calculation problems around the 2 GB (signedness) and 4 GB (wrap-around)
boundaries), or something like 3 or 4 times the minimum buffer size.
I'm in favor of enforcing a 1 GB cap in vb2 and letting drivers enforce a
policy of their own if that makes
Re: [Workshop-2011] Media Subsystem Workshop 2011
Hi, On 08/08/2011 07:39 PM, Theodore Kilgore wrote: On Mon, 8 Aug 2011, Mauro Carvalho Chehab wrote: snip Mauro, In fact none of the currently known and supported cameras are using PTP. All of them are proprietary. They have a rather intimidating set of differences in functionality, too. Namely, some of them have an isochronous endpoint, and some of them rely exclusively upon bulk transport. Some of them have a well developed set of internal capabilities as far as handling still photos are concerned. I mean, such things as the ability to download a single photo, selected at random from the set of photos on the camera, and some do not, requiring that the ability to do this is emulated in software -- by first downloading all previously listed photos and sending the data to /dev/null, then downloading the desired photo and saving it. Some of them permit deletion of individual photos, or all photos, and some do not. For some of them it is even true, as I have previously mentioned, that the USB command string which will delete all photos is the same command used for starting the camera in streaming mode. But the point here is that these cameras are all different from one another, depending upon chipset and even, sometimes, upon firmware or chipset version. The still camera abilities and limitations of all of them are pretty much worked out in libgphoto2. My suggestion would be that the libgphoto2 support libraries for these cameras ought to be left the hell alone, except for some changes in, for example, how the camera is accessed in the first place (through libusb or through a kernel device) in order to address adequately the need to support both modes. I know what is in those libgphoto2 drivers because I wrote them. I can definitely promise that to move all of that functionality over into kernel modules would be a nightmare and would moreover greatly contribute to kernel bloat. You really don't want to go there. I strongly disagree with this. The libgphoto2 camlibs (drivers) for these cameras handle a number of different tasks: 1) Talking to the camera getting binary blobs out of them (be it a PAT or some data) 2) Interpreting said blobs 3) Converting the data parts to pictures doing post processing, etc. I'm not suggesting to move all of this to the kernel driver, we just need to move part 1. to the kernel driver. This is not rocket science. We currently have a really bad situation were drivers are fighting for the same device. The problem here is that these devices are not only one device on the physical level, but also one device on the logical level (IOW they have only 1 usb interface). It is time to quit thinking in band-aides and solve this properly, 1 logical device means it gets 1 driver. This may be an approach which means some more work then others, but I believe in the end that doing it right is worth the effort. As for Mauro's resource locking patches, these won't work because the assume both drivers are active at the same time, which is simply not true. Only 1 driver can be bound to the interface at a time, and when switching from the gspca driver to the usbfs driver, gspca will see an unplug which is indistinguishable from a real device unplug. More over a kernel only solution without libgphoto changes won't solve the problem of a libgphoto app keeping the device open locking out streaming. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Workshop-2011] Media Subsystem Workshop 2011
Hi, snip OK, another example. The cameras supported in camlibs/jl2005c do not have webcam ability, but someone could at any time design and market a dualmode which has in stillcam mode the same severe limitation. What limitation? Well, the entire memory of the camera must be dumped, or else the camera jams itself. You can stop dumping in the middle of the operation, but you must continue after that. Suppose that you had ten pictures on the camera and you only wanted to download the first one. Then you can do that and temporarily stop downloading the rest. But while exiting you have to check whether the rest are downloaded or not. And if they are not, then it has to be done, with the data simply thrown in the trash, and then the camera's memory pointer reset before the camera is released. How, one might ask, did anyone produce something so primitive? Well, it is done. Perhaps the money saved thereby was at least in part devoted to producing better optics for the camera. At least, one can hope so. But people did produce those cameras, and people have bought them. But does anyone want to reproduce the code to support this kind of crap in the kernel? And go through all of the hoops required in order to fake the behavior which one woulld expect from a real still camera? It has already been done in camlibs/jl2005c and isn't that enough? This actually is an example where doing a kernel driver would be easier, a kernel driver never exits. So it can simply remember where it was reading (and cache the data it has read sofar). If an app requests picture 10, we read 1-10, cache them and return picture 10 to the app, then the same or another app asks for picture 4, get it from cache, asks for picture 20 read 11-20, etc. Having written code for various small digital picture frames (the keychain models) I know where you are coming from. Trust me I do. Recently I had an interesting bug report, with a corrupt PAT (picture allocation table) turns out that when deleting a picture through the menu inside the frame a different marker gets written to the PAT then when deleting it with the windows software, Fun huh? So yeah duplicating this code is no fun, but it is the only realistic solution which will get us a 100% reliable and robust user experience. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] [media] omap3isp: queue: fail QBUF if user buffer is too small
Hi Michael,
On Tuesday 09 August 2011 08:42:20 Michael Jones wrote:
Add buffer length check to sanity checks for USERPTR QBUF
Signed-off-by: Michael Jones michael.jo...@matrix-vision.de
Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
I'll push the patch to v3.2.
---
Changes for v2:
- only check when V4L2_MEMORY_USERPTR
drivers/media/video/omap3isp/ispqueue.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/omap3isp/ispqueue.c
b/drivers/media/video/omap3isp/ispqueue.c index 9c31714..9bebb1e 100644
--- a/drivers/media/video/omap3isp/ispqueue.c
+++ b/drivers/media/video/omap3isp/ispqueue.c
@@ -868,6 +868,10 @@ int omap3isp_video_queue_qbuf(struct isp_video_queue
*queue, goto done;
if (vbuf-memory == V4L2_MEMORY_USERPTR
+ vbuf-length buf-vbuf.length)
+ goto done;
+
+ if (vbuf-memory == V4L2_MEMORY_USERPTR
vbuf-m.userptr != buf-vbuf.m.userptr) {
isp_video_buffer_cleanup(buf);
buf-vbuf.m.userptr = vbuf-m.userptr;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
Hi, On 08/08/2011 07:58 PM, Sarah Sharp wrote: On Fri, Aug 05, 2011 at 09:45:56AM +0200, Hans de Goede wrote: Hi, On 08/05/2011 12:56 AM, Greg KH wrote: On Thu, Aug 04, 2011 at 07:21:47PM -0300, Mauro Carvalho Chehab wrote: I think it is important to separate oranges from apples here, there are at least 3 different problem classes which all seem to have gotten thrown onto a pile here: 1) The reason Mauro suggested having some discussion on this at the USB summit is because of a discussion about dual mode cameras on the linux media list. ... 3) Re-direction of usb devices to virtual machines. This works by using the userspace usbfs interface from qemu / vmware / virtualbox / whatever. The basics of this work fine, but it lacks proper locking / safeguards for when a vm takes over a usb device from the in kernel driver. Hi Hans and Mauro, We have do room in the schedule for the USB mini-summit for this discussion, since the schedule is still pretty flexible. The preliminary schedule is up here: http://userweb.kernel.org/~sarah/linuxcon-usb-minisummit.html I think it's best to discuss the VM redirection in the afternoon when some of the KVM folks join us after Hans' talk on USB redirection over the network. That seems best to me too. I'm available at both proposed time slots, and I would like to join both discussions. It sounds like we need a separate topic for the dual mode cameras and TV tuners. Mauro, do you want to lead that discussion in the early morning (in a 9:30 to 10:30 slot) or in the late afternoon (in a 15:30 to 16:30 slot)? I want to be sure we have all the video/media developers who are interested in this topic present, and I don't know if they will be going to the KVM forum. I would really like to see the dual mode camera and TV tuner discussion separated. They are 2 different issues AFAIK. 1) Dual mode cameras: In the case of the dual mode camera we have 1 single device (both at the hardware level and at the logical block level), which can do 2 things, but not at the same time. It can stream live video data from a sensor, or it can retrieve earlier taken pictures from some picture memory. Unfortunately even though these 2 functions live in a single logical block, historically we've developed 2 drivers for them. This leads to fighting over device ownership (surprise surprise), and to me the solution is very clear, 1 logical block == 1 driver. 2) Tv tuners: Here we have a bunch of logical blocks, each with their own driver (and not 2 drivers for one block), which together form not 1 but 2 video pipelines, typically one pipeline for analog TV, and one for DVB. The problem here is some blocks are shared between the 2 pipelines. The solution here, at least to me, is clear too, we need some way of claiming/locking the pipeline blocks. If pipe1 starts streaming video data it locks all the blocks it uses. If some app then tries to use pipe2 (and thus for example tune the tuner to a different frequency), it will fail with -EBUSY when pipe2 tries to take the tuner lock and gets told it is in use. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] The clock dependencies between sensor subdevs and the host interface drivers
Hi Laurent, On 08/08/2011 05:44 PM, Laurent Pinchart wrote: Hi Sylwester, On Monday 08 August 2011 17:36:32 Sylwester Nawrocki wrote: Hi everyone, Nowadays many of the V4L2 camera device drivers heavily depend on the board code to set up voltage supplies, clocks, and some control signals, like 'reset' and 'standby' signals for the sensors. Those things are often being done by means of the driver specific platform data callbacks. There has been recently quite a lot effort on ARM towards migration to the device tree. Unfortunately the custom platform data callbacks effectively prevent the boards to be booted and configured through the device tree bindings. The following is usually handled in the board files: 1) sensor/frontend power supply 2) sensor's master clock (provided by the host device) 3) reset and standby signals (GPIO) 4) other signals applied by the host processor to the sensor device, e.g. I2C level shifter enable, etc. For 1), the regulator API should possibly be used. It should be applicable for most, if not all cases. 3) and 4) are a bit hard as there might be huge differences across boards as how many GPIOs are used, what are the required delays between changes of their states, etc. Still we could try to find a common description of the behaviour and pass such information to the drivers. For 2) I'd like to propose adding a callback to struct v4l2_device, for instance as in the below patch. The host driver would implement such an operation and the sensor subdev driver would use it in its s_power op. What about using a struct clk object ? There has been lots of work in the ARM tree to make struct clk generic. I'm not sure if all patches have been pushed to mainline yet, but I think that's the direction we should follow. But is the 'struct clk' tried to be unified across all archs, not only ARM ? I'm afraid it's not the case. By using a struct clk object do you also mean implementing some/all ops of this object by the driver which exports it ? I suppose we can't rely only on the clock controller functionality exposed through the clock API. Some devices may need to be brought to an active state before the clock can be used outside. Some may have internal frequency dividers which need to be handled in addition to the clock path in the clock controller. For instance, on Exynos4 the FIMC devices belong to a power domain that needs to be enabled so the clock is not blocked, and this is done through the runtime PM calls. Normally the host device driver runtime resumes the device when /dev/video* is opened. But we might want to use the clock before it happens, when only a /dev/v4l-subdev* is opened, to play with the sensor device only. In this situation the host device needs to be runtime resumed first. Thus the driver would need to (re)implement the clock ops to also handle the details which are not covered by the clock controller driver. I also wonder how could we support the boards which choose to use some extra external oscillator to provide clock to the sensors, rather than the one derived from the host. Thanks, -- Sylwester Nawrocki Samsung Poland RD Center -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Possible issue in videobuf2 with buffer length check at QBUF time
Hello, On Monday, August 08, 2011 12:11 PM Laurent Pinchart wrote: On Friday 05 August 2011 17:01:09 Pawel Osciak wrote: On Fri, Aug 5, 2011 at 01:55, Laurent Pinchart wrote: Hi Marek and Pawel, While reviewing an OMAP3 ISP patch, I noticed that videobuf2 doesn't verify the buffer length field value when a new USERPTR buffer is queued. That's a good catch. We should definitely do something about it. The length given by userspace is copied to the internal buffer length field. It seems to be up to drivers to verify that the value is equal to or higher than the minimum required to store the image, but that's not clearly documented. Should the buf_init operation be made mandatory for drivers that support USERPTR, and documented as required to implement a length check ? Technically, drivers can do the length checks on buf_prepare if they don't allow format changes after REQBUFS. On the other hand though, if a driver supports USERPTR, the buffers queued from userspace have to be verified on qbuf and the only place to do that would be buf_init. So every driver that supports USERPTR would have to implement buf_init, as you said. Alternatively the check could be performed in videobuf2-core, which would probably make sense as the check is required. videobuf2 doesn't store the minimum size for USERPTR buffers though (but that can of course be changed). Let's say we make vb2 save minimum buffer size. This would have to be done on queue_setup I imagine. We could add a new field to vb2_buffer for that. One problem is that if the driver actually supports changing format after REQBUFS, we would need a new function in vb2 to change minimum buffer size. Actually, this has to be minimum plane sizes. So the alternatives are: 1. Make buf_init required for drivers that support USERPTR; or 2. Add minimum plane sizes to vb2_buffer,add a new return array argument to queue_setup to return minimum plane sizes that would be stored in vb2. Make vb2 verify sizes on qbuf of USERPTR. Add a new vb2 function for drivers to call when minimum sizes have to be changed. The first solution is way simpler for drivers that require this. The second solution is maybe a bit simpler for drivers that do not, as they would only have to return the sizes in queue_setup, but implementing buf_init instead wouldn't be a big of a difference I think. So I'm leaning towards the second solution. Any comments, did I miss something? Thanks for the analysis. I would go for the second solution as well. Do we have any driver that supports changing the format after REQBUFS ? If not we can delay adding the new vb2 function until we get such a driver. I really wonder if we should support changing the format which results in buffer/plane size change after REQBUFS. Please notice that it causes additional problems with mmap-style buffers, which are allocated once during the REQBUFS call. Also none driver support it right now and I doubt that changing buffer size after REQBUFS can be implemented without breaking anything other (there are a lot of things that depends on buffer size, both in vb2 and the drivers). I would just simplify solution number 2 - imho vb2 should just store the minimal buffer/plane size acquired in queue_setup and check if any buffers that have been queued are large enough. If one wants to change format to the one that requires other buffer size, then he should just call STREAM_OFF, REQBUFS(0), S_FMT, REQBUFS(n) and STREAM_ON. The other solution will be to use separately created buffers, which already have format/size information attached (Guennadi's proposal). Best regards -- Marek Szyprowski Samsung Poland RD Center -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANN] Meeting minutes of the Cambourne meeting
Hi Sakari, I have a point with the pixel clock. During discussion we found that pixel clock get/set is required for user space to do fine control over the frame-rate etc. What if the user sets the pixel array clock which is above the system/if bus clock? Suppose we are setting the pixel clock (which user space sets) to higher rate at sensor array, but for some reason the bus cannot handle that rate (either low speed or loaded) or lower PCLK at say CSI2 interface is being set. Are we not going to loose data due to this? Also, there would be data validation overhead in driver on what is acceptable PCLK values for a particular sensor on an interface etc. I am still not favoring user space controlling this, and wish driver decides this for a given frame-rate requested by the user space :) Frame-rate resolution HSYNC VSYNC PCLK(array) PCLK (i/f bus) ... Let user space control only first two, and driver decide rest (PCLK can be different at different ISP h/w units though) Regards, Subash Pixel clock and blanking Preliminary conclusions: - Pixel clock(s) and blanking will be exported through controls on subdev nodes. - The pixel array pixel clock is needed by userspace. - Hosts/bridges/ISPs need pixel clock and blanking information to validate pipelines. Actions: - CSI2 and CCP2 bus frequencies will be selectable use integer menu controls. (Sakari) - Add an integer menu control type, replacing the name with a 64-bit integer. (Sakari, Hans) - Research which pixel clock(s) to expose based on the SMIA sensor. (Sakari) - Add two new internal subdev pad operations to get and set clocks and blanking. (Laurent, Sakari) -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv1 PATCH for v3.1] v4l2-ioctl: fix ENOTTY handling.
On Sunday, July 31, 2011 14:43:44 Mauro Carvalho Chehab wrote:
Em 29-07-2011 09:10, Hans Verkuil escreveu:
Hi all,
While converting v4l2-compliance to correctly handle ENOTTY errors I found
several regressions in v4l2-ioctl.c:
1) VIDIOC_ENUM/G/S/TRY_FMT would return -ENOTTY if the op for the particular
format type was not set, even though the op for other types might have
been
present. In such a case -EINVAL should have been returned.
2) The priority check could cause -EBUSY or -EINVAL to be returned instead
of
-ENOTTY if the corresponding ioctl was unsupported.
3) Certain ioctls that have an internal implementation (ENUMSTD, G_STD,
S_STD,
G_PARM and the extended control ioctls) could return -EINVAL when -ENOTTY
should have been returned or vice versa.
I first tried to fix this by adding extra code for each affected ioctl, but
that made the code rather ugly.
So I ended up with this code that first checks whether a certain ioctl is
supported or not and returns -ENOTTY if not.
Comments?
This patch adds an extra cost of double-parsing the ioctl just because the
errors. The proper way is to check at the error path.
See the enclosed patch.
Your patch fixes some but not all of the problems that my patch fixes.
I'm trying to create a new patch on top of yours that actually fixes all the
issues, but I'm having a hard time with that.
It is getting very difficult to follow the error path, which is exactly why
I didn't want to do that in the first place. I've never understood the fixation
on performance *without doing any measurements*. As the old saying goes:
Premature optimization is the root of all evil.
Code such as the likely/unlikely macros just obfuscate the code and should not
be added IMHO unless you can prove that it makes a difference. See for example
the discussion whether prefetch is useful or not: http://lwn.net/Articles/444336
Code complexity is by far the biggest problem with all V4L code. I am tempted
to completely reorganize v4l2-ioctl.c, but I can't do that for v3.1.
I'll try to come up with another approach instead.
Regards,
Hans
From: Mauro Carvalho Chehab mche...@redhat.com
Date: Sun, 31 Jul 2011 09:37:56 -0300
[PATCH] v4l2-ioctl: properly return -EINVAL when parameters are wrong
When an ioctl is implemented, but the parameters are invalid,
the error code should be -EINVAL. However, if the ioctl is
not defined, it should return -ENOTTY instead.
While here, adds a gcc hint that having the ioctl enabled is more
likely, as userspace should know what the driver supports due to QUERYCAP
call.
Reported-by: Hans Verkuil hverk...@xs4all.nl
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c
index 002ce13..9f80e9d 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -55,6 +55,14 @@
memset((u8 *)(p) + offsetof(typeof(*(p)), field) + sizeof((p)-field), \
0, sizeof(*(p)) - offsetof(typeof(*(p)), field) - sizeof((p)-field))
+#define no_ioctl_err(foo) ( (
\
+ ops-vidioc_##foo##_fmt_vid_cap || \
+ ops-vidioc_##foo##_fmt_vid_out || \
+ ops-vidioc_##foo##_fmt_vid_cap_mplane || \
+ ops-vidioc_##foo##_fmt_vid_out_mplane || \
+ ops-vidioc_##foo##_fmt_vid_overlay || \
+ ops-vidioc_##foo##_fmt_type_private) ? -EINVAL : -ENOTTY)
+
struct std_descr {
v4l2_std_id std;
const char *descr;
@@ -591,7 +599,7 @@ static long __video_do_ioctl(struct file *file,
ret = v4l2_prio_check(vfd-prio, vfh-prio);
if (ret)
goto exit_prio;
- ret = -EINVAL;
+ ret = -ENOTTY;
break;
}
}
@@ -638,7 +646,7 @@ static long __video_do_ioctl(struct file *file,
enum v4l2_priority *p = arg;
if (!ops-vidioc_s_priority !use_fh_prio)
- break;
+ break;
dbgarg(cmd, setting priority to %d\n, *p);
if (ops-vidioc_s_priority)
ret = ops-vidioc_s_priority(file, fh, *p);
@@ -654,37 +662,37 @@ static long __video_do_ioctl(struct file *file,
switch (f-type) {
case V4L2_BUF_TYPE_VIDEO_CAPTURE:
- if (ops-vidioc_enum_fmt_vid_cap)
+ if (likely(ops-vidioc_enum_fmt_vid_cap))
ret = ops-vidioc_enum_fmt_vid_cap(file, fh, f);
break;
case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
- if (ops-vidioc_enum_fmt_vid_cap_mplane)
+
[PATCH v2] CXD2820R: Replace i2c message translation with repeater gate control
This patch implements an i2c_gate_ctrl op for the cxd2820r. Thanks to Robert
Schlabbach for identifying the register address and field to set.
The old i2c intercept code that prefixed messages with a passthrough byte has
been removed and the PCTV nanoStick T2 290e entry in em28xx-dvb has been
updated appropriately.
Tested for DVB-T2 use; I would appreciate it if somebody with DVB-C capabilities
could test it as well - from inspection I cannot see any problems.
This is patch v2. It fixes some schoolboy style errors and removes superfluous
i2c entries in cxd2820r.h.
Signed-off-by: Steve Kerrison st...@stevekerrison.com
---
drivers/media/dvb/frontends/cxd2820r.h |9 ---
drivers/media/dvb/frontends/cxd2820r_c.c|1 -
drivers/media/dvb/frontends/cxd2820r_core.c | 76 +++
drivers/media/dvb/frontends/cxd2820r_priv.h |1 -
drivers/media/dvb/frontends/cxd2820r_t.c|1 -
drivers/media/dvb/frontends/cxd2820r_t2.c |1 -
drivers/media/video/em28xx/em28xx-dvb.c |7 +--
7 files changed, 11 insertions(+), 85 deletions(-)
diff --git a/drivers/media/dvb/frontends/cxd2820r.h
b/drivers/media/dvb/frontends/cxd2820r.h
index 2906582..03cab7b 100644
--- a/drivers/media/dvb/frontends/cxd2820r.h
+++ b/drivers/media/dvb/frontends/cxd2820r.h
@@ -93,9 +93,6 @@ extern struct dvb_frontend *cxd2820r_attach(
struct i2c_adapter *i2c,
struct dvb_frontend *fe
);
-extern struct i2c_adapter *cxd2820r_get_tuner_i2c_adapter(
- struct dvb_frontend *fe
-);
#else
static inline struct dvb_frontend *cxd2820r_attach(
const struct cxd2820r_config *config,
@@ -106,12 +103,6 @@ static inline struct dvb_frontend *cxd2820r_attach(
printk(KERN_WARNING %s: driver disabled by Kconfig\n, __func__);
return NULL;
}
-static inline struct i2c_adapter *cxd2820r_get_tuner_i2c_adapter(
- struct dvb_frontend *fe
-)
-{
- return NULL;
-}
#endif
diff --git a/drivers/media/dvb/frontends/cxd2820r_c.c
b/drivers/media/dvb/frontends/cxd2820r_c.c
index 3c07d40..b85f501 100644
--- a/drivers/media/dvb/frontends/cxd2820r_c.c
+++ b/drivers/media/dvb/frontends/cxd2820r_c.c
@@ -335,4 +335,3 @@ int cxd2820r_get_tune_settings_c(struct dvb_frontend *fe,
return 0;
}
-
diff --git a/drivers/media/dvb/frontends/cxd2820r_core.c
b/drivers/media/dvb/frontends/cxd2820r_core.c
index d416e85..0151267 100644
--- a/drivers/media/dvb/frontends/cxd2820r_core.c
+++ b/drivers/media/dvb/frontends/cxd2820r_core.c
@@ -727,70 +727,20 @@ static void cxd2820r_release(struct dvb_frontend *fe)
struct cxd2820r_priv *priv = fe-demodulator_priv;
dbg(%s, __func__);
- if (fe-ops.info.type == FE_OFDM) {
- i2c_del_adapter(priv-tuner_i2c_adapter);
+ if (fe-ops.info.type == FE_OFDM)
kfree(priv);
- }
return;
}
-static u32 cxd2820r_tuner_i2c_func(struct i2c_adapter *adapter)
-{
- return I2C_FUNC_I2C;
-}
-
-static int cxd2820r_tuner_i2c_xfer(struct i2c_adapter *i2c_adap,
- struct i2c_msg msg[], int num)
-{
- struct cxd2820r_priv *priv = i2c_get_adapdata(i2c_adap);
- int ret;
- u8 *obuf = kmalloc(msg[0].len + 2, GFP_KERNEL);
- struct i2c_msg msg2[2] = {
- {
- .addr = priv-cfg.i2c_address,
- .flags = 0,
- .len = msg[0].len + 2,
- .buf = obuf,
- }, {
- .addr = priv-cfg.i2c_address,
- .flags = I2C_M_RD,
- .len = msg[1].len,
- .buf = msg[1].buf,
- }
- };
-
- if (!obuf)
- return -ENOMEM;
-
- obuf[0] = 0x09;
- obuf[1] = (msg[0].addr 1);
- if (num == 2) { /* I2C read */
- obuf[1] = (msg[0].addr 1) | I2C_M_RD; /* I2C RD flag */
- msg2[0].len = msg[0].len + 2 - 1; /* '-1' maybe HW bug ? */
- }
- memcpy(obuf[2], msg[0].buf, msg[0].len);
-
- ret = i2c_transfer(priv-i2c, msg2, num);
- if (ret 0)
- warn(tuner i2c failed ret:%d, ret);
-
- kfree(obuf);
-
- return ret;
-}
-
-static struct i2c_algorithm cxd2820r_tuner_i2c_algo = {
- .master_xfer = cxd2820r_tuner_i2c_xfer,
- .functionality = cxd2820r_tuner_i2c_func,
-};
-
-struct i2c_adapter *cxd2820r_get_tuner_i2c_adapter(struct dvb_frontend *fe)
+static int cxd2820r_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
{
struct cxd2820r_priv *priv = fe-demodulator_priv;
- return priv-tuner_i2c_adapter;
+ dbg(%s: %d, __func__, enable);
+
+ /* Bit 0 of reg 0xdb in bank 0x00 controls I2C repeater */
+ return cxd2820r_wr_reg_mask(priv, 0xdb, enable ? 1 : 0, 0x1);
}
-EXPORT_SYMBOL(cxd2820r_get_tuner_i2c_adapter);
static struct dvb_frontend_ops cxd2820r_ops[2];
@@ -831,18 +781,6 @@ struct dvb_frontend *cxd2820r_attach(const struct
Re: [PATCH v2] CXD2820R: Replace i2c message translation with repeater gate control
Acked-by: Antti Palosaari cr...@iki.fi Tested-by: Antti Palosaari cr...@iki.fi On 08/09/2011 01:16 PM, Steve Kerrison wrote: This patch implements an i2c_gate_ctrl op for the cxd2820r. Thanks to Robert Schlabbach for identifying the register address and field to set. The old i2c intercept code that prefixed messages with a passthrough byte has been removed and the PCTV nanoStick T2 290e entry in em28xx-dvb has been updated appropriately. Tested for DVB-T2 use; I would appreciate it if somebody with DVB-C capabilities could test it as well - from inspection I cannot see any problems. This is patch v2. It fixes some schoolboy style errors and removes superfluous i2c entries in cxd2820r.h. Signed-off-by: Steve Kerrison st...@stevekerrison.com -- http://palosaari.fi/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Anyone tested the DVB-T2 dual tuner TBS6280?
Hi Harald, Currently the controller chip on the BlackGold card is unsupported - I don't know if there's any development work going on for that in this arena or if perhaps BlackGold are working on something themselves. Perhaps somebody else here knows the full story. This TBS card has only just been brought to my attention. I cannot tell what PCIe chip it uses and if it's supported. The alleged Linux driver download for it doesn't have the cxd2820r code in it, so I can't see that having much chance of working. Perhaps ask TBS directly what the status on this one is? I don't know of anybody who has used it yet (even in Windows). Regards, -- Steve Kerrison MEng Hons. http://www.stevekerrison.com/ On Tue, 2011-08-09 at 12:35 +0200, Harald Gustafsson wrote: Hi, I searched for a dual tuner PCI-e DVB-T2 card with Linux support and found this TBS6280 card: http://tbsdvb.blog.com/2011/07/22/tbs-6280-freeview-hd-twin-tuner-card/ http://www.buydvb.net/tbs6280-pcie-dvbt2t-dual-tuner-card_p38.html http://www.tbsdtv.com/english/Download.html Previously I have only found the blackgold product that state they will have Linux support but have not seen any drivers yet. But when searching the mythtv lists and the linux dvb list I could not find anyone using it. Do anyone have any info about this card, does it work well with terrestrial DVB-T2 reception, is linux support working, does it work with mythtv, etc. /Harald -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Anyone tested the DVB-T2 dual tuner TBS6280?
On 08/09/2011 01:57 PM, Steve Kerrison wrote: This TBS card has only just been brought to my attention. I cannot tell what PCIe chip it uses and if it's supported. The alleged Linux driver download for it doesn't have the cxd2820r code in it, so I can't see that having much chance of working. FE I2C addresses used are those CXD2820R ones so it is CXD2820R. Frontend driver is binary and strings stripped out. Antti -- http://palosaari.fi/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Anyone tested the DVB-T2 dual tuner TBS6280?
Steve Kerrison st...@stevekerrison.com writes:
This TBS card has only just been brought to my attention. I cannot tell
what PCIe chip it uses and if it's supported. The alleged Linux driver
download for it doesn't have the cxd2820r code in it, so I can't see
that having much chance of working.
There's a binary-only tbs62x0fe_driver for x86_{32,64} in the archive,
so it _might_ work. But I don't think anyone here will recommend
something like that... You may just as well run Windows instead, and at
least get a somewhat tested binary driver.
Bjørn
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv1 PATCH for v3.1] v4l2-ioctl: fix ENOTTY handling.
Em 09-08-2011 07:10, Hans Verkuil escreveu:
On Sunday, July 31, 2011 14:43:44 Mauro Carvalho Chehab wrote:
Em 29-07-2011 09:10, Hans Verkuil escreveu:
Hi all,
While converting v4l2-compliance to correctly handle ENOTTY errors I found
several regressions in v4l2-ioctl.c:
1) VIDIOC_ENUM/G/S/TRY_FMT would return -ENOTTY if the op for the particular
format type was not set, even though the op for other types might have
been
present. In such a case -EINVAL should have been returned.
2) The priority check could cause -EBUSY or -EINVAL to be returned instead
of
-ENOTTY if the corresponding ioctl was unsupported.
3) Certain ioctls that have an internal implementation (ENUMSTD, G_STD,
S_STD,
G_PARM and the extended control ioctls) could return -EINVAL when -ENOTTY
should have been returned or vice versa.
I first tried to fix this by adding extra code for each affected ioctl, but
that made the code rather ugly.
So I ended up with this code that first checks whether a certain ioctl is
supported or not and returns -ENOTTY if not.
Comments?
This patch adds an extra cost of double-parsing the ioctl just because the
errors. The proper way is to check at the error path.
See the enclosed patch.
Your patch fixes some but not all of the problems that my patch fixes.
What was left?
I'm trying to create a new patch on top of yours that actually fixes all the
issues, but I'm having a hard time with that.
It is getting very difficult to follow the error path, which is exactly why
I didn't want to do that in the first place. I've never understood the
fixation
on performance *without doing any measurements*. As the old saying goes:
Premature optimization is the root of all evil.
Code such as the likely/unlikely macros just obfuscate the code and should not
be added IMHO unless you can prove that it makes a difference. See for example
the discussion whether prefetch is useful or not:
http://lwn.net/Articles/444336
I politely disagree that likely/unlikely macros obfuscate the code.
If it actually make some difference or not would require a study using several
different processors with different types of CPU pipelines, and/or checking the
generated assembler for the supported processor families.
If someone comes to a conclusion that it doesn't make any difference, we can
simply discard them.
Code complexity is by far the biggest problem with all V4L code. I am tempted
to completely reorganize v4l2-ioctl.c, but I can't do that for v3.1.
A single code block like:
switch (ioctl) {
case VIDIOC_foo:
/* handle foo */
break;
...
}
Has less complexity than two blocks:
switch (ioctl) {
case VIDIOC_foo:
/* Check for errors on foo */
break;
...
}
switch (ioctl) {
case VIDIOC_foo:
/* handle foo */
break;
...
}
Also, it allows optimizing the error handling logic to only run when there's
an error, and not before.
In the past, the ioctl handling were a big mess, as there were several switch
loops to handle ioctl's, internally to each V4L driver. This is harder to
review, and may lead into mistakes. A single switch improved a lot code
readability, as now everything is there together.
The priority patches messed it somehow, by adding an extra switch. Let's not
add more complexity to it.
I'll try to come up with another approach instead.
What do you have in mind?
Regards,
Hans
From: Mauro Carvalho Chehab mche...@redhat.com
Date: Sun, 31 Jul 2011 09:37:56 -0300
[PATCH] v4l2-ioctl: properly return -EINVAL when parameters are wrong
When an ioctl is implemented, but the parameters are invalid,
the error code should be -EINVAL. However, if the ioctl is
not defined, it should return -ENOTTY instead.
While here, adds a gcc hint that having the ioctl enabled is more
likely, as userspace should know what the driver supports due to QUERYCAP
call.
Reported-by: Hans Verkuil hverk...@xs4all.nl
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c
index 002ce13..9f80e9d 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -55,6 +55,14 @@
memset((u8 *)(p) + offsetof(typeof(*(p)), field) + sizeof((p)-field), \
0, sizeof(*(p)) - offsetof(typeof(*(p)), field) - sizeof((p)-field))
+#define no_ioctl_err(foo) ( (
\
+ops-vidioc_##foo##_fmt_vid_cap || \
+ops-vidioc_##foo##_fmt_vid_out || \
+ops-vidioc_##foo##_fmt_vid_cap_mplane || \
+ops-vidioc_##foo##_fmt_vid_out_mplane || \
+ops-vidioc_##foo##_fmt_vid_overlay || \
+ops-vidioc_##foo##_fmt_type_private) ? -EINVAL : -ENOTTY)
+
struct std_descr {
v4l2_std_id std;
const char *descr;
@@ -591,7 +599,7
Re: [RFCv1 PATCH for v3.1] v4l2-ioctl: fix ENOTTY handling.
On Tuesday, August 09, 2011 13:22:55 Mauro Carvalho Chehab wrote:
Em 09-08-2011 07:10, Hans Verkuil escreveu:
On Sunday, July 31, 2011 14:43:44 Mauro Carvalho Chehab wrote:
Em 29-07-2011 09:10, Hans Verkuil escreveu:
Hi all,
While converting v4l2-compliance to correctly handle ENOTTY errors I found
several regressions in v4l2-ioctl.c:
1) VIDIOC_ENUM/G/S/TRY_FMT would return -ENOTTY if the op for the
particular
format type was not set, even though the op for other types might have
been
present. In such a case -EINVAL should have been returned.
2) The priority check could cause -EBUSY or -EINVAL to be returned
instead of
-ENOTTY if the corresponding ioctl was unsupported.
3) Certain ioctls that have an internal implementation (ENUMSTD, G_STD,
S_STD,
G_PARM and the extended control ioctls) could return -EINVAL when
-ENOTTY
should have been returned or vice versa.
I first tried to fix this by adding extra code for each affected ioctl,
but
that made the code rather ugly.
So I ended up with this code that first checks whether a certain ioctl is
supported or not and returns -ENOTTY if not.
Comments?
This patch adds an extra cost of double-parsing the ioctl just because the
errors. The proper way is to check at the error path.
See the enclosed patch.
Your patch fixes some but not all of the problems that my patch fixes.
What was left?
A number of things: the priority check has to be done after the 'ENOTTY' check,
the no_ioctl_err() macros had copy and paste errors ('g' was used for both set
and try), and was incomplete (enum_fmt has a subset of the possible ops compared
to get/set/try).
Several error paths were also not handled correctly (returning ENOTTY instead of
EINVAL).
I'm trying to create a new patch on top of yours that actually fixes all the
issues, but I'm having a hard time with that.
It is getting very difficult to follow the error path, which is exactly why
I didn't want to do that in the first place. I've never understood the
fixation
on performance *without doing any measurements*. As the old saying goes:
Premature optimization is the root of all evil.
Code such as the likely/unlikely macros just obfuscate the code and should
not
be added IMHO unless you can prove that it makes a difference. See for
example
the discussion whether prefetch is useful or not:
http://lwn.net/Articles/444336
I politely disagree that likely/unlikely macros obfuscate the code.
If it actually make some difference or not would require a study using
several
different processors with different types of CPU pipelines, and/or checking
the
generated assembler for the supported processor families.
If someone comes to a conclusion that it doesn't make any difference, we can
simply discard them.
That's the wrong way around IMHO. Code must serve a purpose. I have no problem
with likely/unlikely in tight loops or often executed code. Neither of which is
the case here.
Code complexity is by far the biggest problem with all V4L code. I am
tempted
to completely reorganize v4l2-ioctl.c, but I can't do that for v3.1.
A single code block like:
switch (ioctl) {
case VIDIOC_foo:
/* handle foo */
break;
...
}
Has less complexity than two blocks:
switch (ioctl) {
case VIDIOC_foo:
/* Check for errors on foo */
break;
...
}
switch (ioctl) {
case VIDIOC_foo:
/* handle foo */
break;
...
}
Also, it allows optimizing the error handling logic to only run when there's
an error, and not before.
In the past, the ioctl handling were a big mess, as there were several switch
loops to handle ioctl's, internally to each V4L driver. This is harder to
review, and may lead into mistakes. A single switch improved a lot code
readability, as now everything is there together.
The priority patches messed it somehow, by adding an extra switch. Let's not
add more complexity to it.
I'll try to come up with another approach instead.
What do you have in mind?
I'll post a new patch today. The vivi and ivtv drivers now pass the
v4l2-compliance
test again.
Regards,
Hans
Regards,
Hans
From: Mauro Carvalho Chehab mche...@redhat.com
Date: Sun, 31 Jul 2011 09:37:56 -0300
[PATCH] v4l2-ioctl: properly return -EINVAL when parameters are wrong
When an ioctl is implemented, but the parameters are invalid,
the error code should be -EINVAL. However, if the ioctl is
not defined, it should return -ENOTTY instead.
While here, adds a gcc hint that having the ioctl enabled is more
likely, as userspace should know what the driver supports due to QUERYCAP
call.
Reported-by: Hans Verkuil hverk...@xs4all.nl
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c
index 002ce13..9f80e9d
Re: [RFC] The clock dependencies between sensor subdevs and the host interface drivers
Hi Sylwester, On Tuesday 09 August 2011 10:10:40 Sylwester Nawrocki wrote: On 08/08/2011 05:44 PM, Laurent Pinchart wrote: On Monday 08 August 2011 17:36:32 Sylwester Nawrocki wrote: Hi everyone, Nowadays many of the V4L2 camera device drivers heavily depend on the board code to set up voltage supplies, clocks, and some control signals, like 'reset' and 'standby' signals for the sensors. Those things are often being done by means of the driver specific platform data callbacks. There has been recently quite a lot effort on ARM towards migration to the device tree. Unfortunately the custom platform data callbacks effectively prevent the boards to be booted and configured through the device tree bindings. The following is usually handled in the board files: 1) sensor/frontend power supply 2) sensor's master clock (provided by the host device) 3) reset and standby signals (GPIO) 4) other signals applied by the host processor to the sensor device, e.g. I2C level shifter enable, etc. For 1), the regulator API should possibly be used. It should be applicable for most, if not all cases. 3) and 4) are a bit hard as there might be huge differences across boards as how many GPIOs are used, what are the required delays between changes of their states, etc. Still we could try to find a common description of the behaviour and pass such information to the drivers. For 2) I'd like to propose adding a callback to struct v4l2_device, for instance as in the below patch. The host driver would implement such an operation and the sensor subdev driver would use it in its s_power op. What about using a struct clk object ? There has been lots of work in the ARM tree to make struct clk generic. I'm not sure if all patches have been pushed to mainline yet, but I think that's the direction we should follow. But is the 'struct clk' tried to be unified across all archs, not only ARM ? I'm afraid it's not the case. If the goals haven't changed since https://lkml.org/lkml/2011/5/20/85, the new struct clk will be unified across all architectures. By using a struct clk object do you also mean implementing some/all ops of this object by the driver which exports it ? That's correct. I suppose we can't rely only on the clock controller functionality exposed through the clock API. Some devices may need to be brought to an active state before the clock can be used outside. Some may have internal frequency dividers which need to be handled in addition to the clock path in the clock controller. For instance, on Exynos4 the FIMC devices belong to a power domain that needs to be enabled so the clock is not blocked, and this is done through the runtime PM calls. Normally the host device driver runtime resumes the device when /dev/video* is opened. But we might want to use the clock before it happens, when only a /dev/v4l-subdev* is opened, to play with the sensor device only. In this situation the host device needs to be runtime resumed first. Thus the driver would need to (re)implement the clock ops to also handle the details which are not covered by the clock controller driver. The subdev driver would call clk_get(), which would end up being implemented by the driver for whatever hardware block provides the clock. The driver would then runtime_pm_resume() the hardware to start the clock. I also wonder how could we support the boards which choose to use some extra external oscillator to provide clock to the sensors, rather than the one derived from the host. In that case the clock is always running. I'm not sure if we should create a dummy clk object, or just pass a NULL clock and a fixed frequency to the sensor driver. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Anyone tested the DVB-T2 dual tuner TBS6280?
Thanks all for your quick answers.
On Tue, Aug 9, 2011 at 1:10 PM, Bjørn Mork bj...@mork.no wrote:
There's a binary-only tbs62x0fe_driver for x86_{32,64} in the archive,
so it _might_ work. But I don't think anyone here will recommend
something like that...
Yes, this is what I was afraid of. That it was binary drivers, and I
agree that this usually only leads to problems when integrating it
with kernels that was not thought of by the developers.
Since I unfortunately don't have time to reverse engineer this binary
driver, I think I wait for another dual tuner DVB-T2 card shows up.
Thanks,
Harald
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Possible issue in videobuf2 with buffer length check at QBUF time
Hi Marek, On Tuesday 09 August 2011 11:14:43 Marek Szyprowski wrote: On Monday, August 08, 2011 12:11 PM Laurent Pinchart wrote: On Friday 05 August 2011 17:01:09 Pawel Osciak wrote: On Fri, Aug 5, 2011 at 01:55, Laurent Pinchart wrote: Hi Marek and Pawel, While reviewing an OMAP3 ISP patch, I noticed that videobuf2 doesn't verify the buffer length field value when a new USERPTR buffer is queued. That's a good catch. We should definitely do something about it. The length given by userspace is copied to the internal buffer length field. It seems to be up to drivers to verify that the value is equal to or higher than the minimum required to store the image, but that's not clearly documented. Should the buf_init operation be made mandatory for drivers that support USERPTR, and documented as required to implement a length check ? Technically, drivers can do the length checks on buf_prepare if they don't allow format changes after REQBUFS. On the other hand though, if a driver supports USERPTR, the buffers queued from userspace have to be verified on qbuf and the only place to do that would be buf_init. So every driver that supports USERPTR would have to implement buf_init, as you said. Alternatively the check could be performed in videobuf2-core, which would probably make sense as the check is required. videobuf2 doesn't store the minimum size for USERPTR buffers though (but that can of course be changed). Let's say we make vb2 save minimum buffer size. This would have to be done on queue_setup I imagine. We could add a new field to vb2_buffer for that. One problem is that if the driver actually supports changing format after REQBUFS, we would need a new function in vb2 to change minimum buffer size. Actually, this has to be minimum plane sizes. So the alternatives are: 1. Make buf_init required for drivers that support USERPTR; or 2. Add minimum plane sizes to vb2_buffer,add a new return array argument to queue_setup to return minimum plane sizes that would be stored in vb2. Make vb2 verify sizes on qbuf of USERPTR. Add a new vb2 function for drivers to call when minimum sizes have to be changed. The first solution is way simpler for drivers that require this. The second solution is maybe a bit simpler for drivers that do not, as they would only have to return the sizes in queue_setup, but implementing buf_init instead wouldn't be a big of a difference I think. So I'm leaning towards the second solution. Any comments, did I miss something? Thanks for the analysis. I would go for the second solution as well. Do we have any driver that supports changing the format after REQBUFS ? If not we can delay adding the new vb2 function until we get such a driver. I really wonder if we should support changing the format which results in buffer/plane size change after REQBUFS. Please notice that it causes additional problems with mmap-style buffers, which are allocated once during the REQBUFS call. Also none driver support it right now and I doubt that changing buffer size after REQBUFS can be implemented without breaking anything other (there are a lot of things that depends on buffer size, both in vb2 and the drivers). I wasn't awake enough when I sent my previous reply. We probably have no driver that supports this feature at the moment, but changing the format after REQBUFS is the whole point of the CREATE_BUFS and PREPARE_BUF ioctls, so I think we definitely need to support that :-) I would just simplify solution number 2 - imho vb2 should just store the minimal buffer/plane size acquired in queue_setup and check if any buffers that have been queued are large enough. If one wants to change format to the one that requires other buffer size, then he should just call STREAM_OFF, REQBUFS(0), S_FMT, REQBUFS(n) and STREAM_ON. The other solution will be to use separately created buffers, which already have format/size information attached (Guennadi's proposal). -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Possible issue in videobuf2 with buffer length check at QBUF time
Hello, On Tuesday, August 09, 2011 1:52 PM Laurent Pinchart wrote: On Tuesday 09 August 2011 11:14:43 Marek Szyprowski wrote: On Monday, August 08, 2011 12:11 PM Laurent Pinchart wrote: On Friday 05 August 2011 17:01:09 Pawel Osciak wrote: On Fri, Aug 5, 2011 at 01:55, Laurent Pinchart wrote: Hi Marek and Pawel, While reviewing an OMAP3 ISP patch, I noticed that videobuf2 doesn't verify the buffer length field value when a new USERPTR buffer is queued. That's a good catch. We should definitely do something about it. The length given by userspace is copied to the internal buffer length field. It seems to be up to drivers to verify that the value is equal to or higher than the minimum required to store the image, but that's not clearly documented. Should the buf_init operation be made mandatory for drivers that support USERPTR, and documented as required to implement a length check ? Technically, drivers can do the length checks on buf_prepare if they don't allow format changes after REQBUFS. On the other hand though, if a driver supports USERPTR, the buffers queued from userspace have to be verified on qbuf and the only place to do that would be buf_init. So every driver that supports USERPTR would have to implement buf_init, as you said. Alternatively the check could be performed in videobuf2-core, which would probably make sense as the check is required. videobuf2 doesn't store the minimum size for USERPTR buffers though (but that can of course be changed). Let's say we make vb2 save minimum buffer size. This would have to be done on queue_setup I imagine. We could add a new field to vb2_buffer for that. One problem is that if the driver actually supports changing format after REQBUFS, we would need a new function in vb2 to change minimum buffer size. Actually, this has to be minimum plane sizes. So the alternatives are: 1. Make buf_init required for drivers that support USERPTR; or 2. Add minimum plane sizes to vb2_buffer,add a new return array argument to queue_setup to return minimum plane sizes that would be stored in vb2. Make vb2 verify sizes on qbuf of USERPTR. Add a new vb2 function for drivers to call when minimum sizes have to be changed. The first solution is way simpler for drivers that require this. The second solution is maybe a bit simpler for drivers that do not, as they would only have to return the sizes in queue_setup, but implementing buf_init instead wouldn't be a big of a difference I think. So I'm leaning towards the second solution. Any comments, did I miss something? Thanks for the analysis. I would go for the second solution as well. Do we have any driver that supports changing the format after REQBUFS ? If not we can delay adding the new vb2 function until we get such a driver. I really wonder if we should support changing the format which results in buffer/plane size change after REQBUFS. Please notice that it causes additional problems with mmap-style buffers, which are allocated once during the REQBUFS call. Also none driver support it right now and I doubt that changing buffer size after REQBUFS can be implemented without breaking anything other (there are a lot of things that depends on buffer size, both in vb2 and the drivers). I wasn't awake enough when I sent my previous reply. We probably have no driver that supports this feature at the moment, but changing the format after REQBUFS is the whole point of the CREATE_BUFS and PREPARE_BUF ioctls, so I think we definitely need to support that :-) Right, but this is an extension that will come with CRATE_BUFS/PREPARE_BUF calls. Until that, to handle buffers correctly we only need to add min_size entry to the queue and check if queued buffers are large enough. Best regards -- Marek Szyprowski Samsung Poland RD Center -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Possible issue in videobuf2 with buffer length check at QBUF time
Hi Marek, On Tuesday 09 August 2011 14:07:12 Marek Szyprowski wrote: On Tuesday, August 09, 2011 1:52 PM Laurent Pinchart wrote: On Tuesday 09 August 2011 11:14:43 Marek Szyprowski wrote: On Monday, August 08, 2011 12:11 PM Laurent Pinchart wrote: On Friday 05 August 2011 17:01:09 Pawel Osciak wrote: On Fri, Aug 5, 2011 at 01:55, Laurent Pinchart wrote: Hi Marek and Pawel, While reviewing an OMAP3 ISP patch, I noticed that videobuf2 doesn't verify the buffer length field value when a new USERPTR buffer is queued. That's a good catch. We should definitely do something about it. The length given by userspace is copied to the internal buffer length field. It seems to be up to drivers to verify that the value is equal to or higher than the minimum required to store the image, but that's not clearly documented. Should the buf_init operation be made mandatory for drivers that support USERPTR, and documented as required to implement a length check ? Technically, drivers can do the length checks on buf_prepare if they don't allow format changes after REQBUFS. On the other hand though, if a driver supports USERPTR, the buffers queued from userspace have to be verified on qbuf and the only place to do that would be buf_init. So every driver that supports USERPTR would have to implement buf_init, as you said. Alternatively the check could be performed in videobuf2-core, which would probably make sense as the check is required. videobuf2 doesn't store the minimum size for USERPTR buffers though (but that can of course be changed). Let's say we make vb2 save minimum buffer size. This would have to be done on queue_setup I imagine. We could add a new field to vb2_buffer for that. One problem is that if the driver actually supports changing format after REQBUFS, we would need a new function in vb2 to change minimum buffer size. Actually, this has to be minimum plane sizes. So the alternatives are: 1. Make buf_init required for drivers that support USERPTR; or 2. Add minimum plane sizes to vb2_buffer,add a new return array argument to queue_setup to return minimum plane sizes that would be stored in vb2. Make vb2 verify sizes on qbuf of USERPTR. Add a new vb2 function for drivers to call when minimum sizes have to be changed. The first solution is way simpler for drivers that require this. The second solution is maybe a bit simpler for drivers that do not, as they would only have to return the sizes in queue_setup, but implementing buf_init instead wouldn't be a big of a difference I think. So I'm leaning towards the second solution. Any comments, did I miss something? Thanks for the analysis. I would go for the second solution as well. Do we have any driver that supports changing the format after REQBUFS ? If not we can delay adding the new vb2 function until we get such a driver. I really wonder if we should support changing the format which results in buffer/plane size change after REQBUFS. Please notice that it causes additional problems with mmap-style buffers, which are allocated once during the REQBUFS call. Also none driver support it right now and I doubt that changing buffer size after REQBUFS can be implemented without breaking anything other (there are a lot of things that depends on buffer size, both in vb2 and the drivers). I wasn't awake enough when I sent my previous reply. We probably have no driver that supports this feature at the moment, but changing the format after REQBUFS is the whole point of the CREATE_BUFS and PREPARE_BUF ioctls, so I think we definitely need to support that :-) Right, but this is an extension that will come with CRATE_BUFS/PREPARE_BUF calls. Until that, to handle buffers correctly we only need to add min_size entry to the queue and check if queued buffers are large enough. I'm fine with that, but that's not a reason to ignore the bigger picture, especially as it will come back to bite us pretty soon :-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH] More ENOTTY fixes for v3.1
This patch fixes the remaining -ENOTTY fallout:
- The ENUM_FMT ops form a subset of the ops used with G/S/TRY_FMT, so list those
explicitly instead of using a macro.
- Extend the G/S/TRY_FMT ops test to the full set.
- Move all prio tests down to the correct switch cases.
- Fix some 'returns ENOTTY instead of EINVAL' issues with the extended controls
ioctls and the ENUMSTD, S_STD and G_PARM ioctls.
This patch + two other vivi and ivtv driver fixes can be found here:
http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/enottyv2
Comments welcome,
Hans
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 9f80e9d..a0089bf 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -55,13 +55,18 @@
memset((u8 *)(p) + offsetof(typeof(*(p)), field) + sizeof((p)-field), \
0, sizeof(*(p)) - offsetof(typeof(*(p)), field) - sizeof((p)-field))
-#define no_ioctl_err(foo) ( ( \
+#define have_fmt_ops(foo) (\
ops-vidioc_##foo##_fmt_vid_cap || \
ops-vidioc_##foo##_fmt_vid_out || \
ops-vidioc_##foo##_fmt_vid_cap_mplane || \
ops-vidioc_##foo##_fmt_vid_out_mplane || \
ops-vidioc_##foo##_fmt_vid_overlay || \
- ops-vidioc_##foo##_fmt_type_private) ? -EINVAL : -ENOTTY)
+ ops-vidioc_##foo##_fmt_vbi_cap || \
+ ops-vidioc_##foo##_fmt_vid_out_overlay || \
+ ops-vidioc_##foo##_fmt_vbi_out || \
+ ops-vidioc_##foo##_fmt_sliced_vbi_cap || \
+ ops-vidioc_##foo##_fmt_sliced_vbi_out || \
+ ops-vidioc_##foo##_fmt_type_private)
struct std_descr {
v4l2_std_id std;
@@ -551,6 +556,7 @@ static long __video_do_ioctl(struct file *file,
struct v4l2_fh *vfh = NULL;
struct v4l2_format f_copy;
int use_fh_prio = 0;
+ long ret_prio = 0;
long ret = -ENOTTY;
if (ops == NULL) {
@@ -570,39 +576,8 @@ static long __video_do_ioctl(struct file *file,
use_fh_prio = test_bit(V4L2_FL_USE_FH_PRIO, vfd-flags);
}
- if (use_fh_prio) {
- switch (cmd) {
- case VIDIOC_S_CTRL:
- case VIDIOC_S_STD:
- case VIDIOC_S_INPUT:
- case VIDIOC_S_OUTPUT:
- case VIDIOC_S_TUNER:
- case VIDIOC_S_FREQUENCY:
- case VIDIOC_S_FMT:
- case VIDIOC_S_CROP:
- case VIDIOC_S_AUDIO:
- case VIDIOC_S_AUDOUT:
- case VIDIOC_S_EXT_CTRLS:
- case VIDIOC_S_FBUF:
- case VIDIOC_S_PRIORITY:
- case VIDIOC_S_DV_PRESET:
- case VIDIOC_S_DV_TIMINGS:
- case VIDIOC_S_JPEGCOMP:
- case VIDIOC_S_MODULATOR:
- case VIDIOC_S_PARM:
- case VIDIOC_S_HW_FREQ_SEEK:
- case VIDIOC_ENCODER_CMD:
- case VIDIOC_OVERLAY:
- case VIDIOC_REQBUFS:
- case VIDIOC_STREAMON:
- case VIDIOC_STREAMOFF:
- ret = v4l2_prio_check(vfd-prio, vfh-prio);
- if (ret)
- goto exit_prio;
- ret = -ENOTTY;
- break;
- }
- }
+ if (use_fh_prio)
+ ret_prio = v4l2_prio_check(vfd-prio, vfh-prio);
switch (cmd) {
@@ -651,7 +626,9 @@ static long __video_do_ioctl(struct file *file,
if (ops-vidioc_s_priority)
ret = ops-vidioc_s_priority(file, fh, *p);
else
- ret = v4l2_prio_change(vfd-v4l2_dev-prio,
vfh-prio, *p);
+ ret = ret_prio ? ret_prio :
+ v4l2_prio_change(vfd-v4l2_dev-prio,
+ vfh-prio, *p);
break;
}
@@ -701,8 +678,14 @@ static long __video_do_ioctl(struct file *file,
(f-pixelformat 16) 0xff,
(f-pixelformat 24) 0xff,
f-description);
- else if (ret == -ENOTTY)
- ret = no_ioctl_err(enum);
+ else if (ret == -ENOTTY
+(ops-vidioc_enum_fmt_vid_cap ||
+ ops-vidioc_enum_fmt_vid_out ||
+ ops-vidioc_enum_fmt_vid_cap_mplane ||
+ ops-vidioc_enum_fmt_vid_out_mplane ||
+ ops-vidioc_enum_fmt_vid_overlay ||
+ ops-vidioc_enum_fmt_type_private))
+
Re: [RFC] The clock dependencies between sensor subdevs and the host interface drivers
Hi Laurent, On 08/09/2011 01:49 PM, Laurent Pinchart wrote: ... The following is usually handled in the board files: 1) sensor/frontend power supply 2) sensor's master clock (provided by the host device) ... For 2) I'd like to propose adding a callback to struct v4l2_device, for instance as in the below patch. The host driver would implement such an operation and the sensor subdev driver would use it in its s_power op. What about using a struct clk object ? There has been lots of work in the ARM tree to make struct clk generic. I'm not sure if all patches have been pushed to mainline yet, but I think that's the direction we should follow. But is the 'struct clk' tried to be unified across all archs, not only ARM ? I'm afraid it's not the case. If the goals haven't changed since https://lkml.org/lkml/2011/5/20/85, the new struct clk will be unified across all architectures. OK, it sounds good then. Except the uncertainty of when actually the agreement is achieved and the platforms are finally converted. Looks like there might still be much time needed auntil the clk API based approach is applicable. However I'm not denying we should be adopting it. By using a struct clk object do you also mean implementing some/all ops of this object by the driver which exports it ? That's correct. I suppose we can't rely only on the clock controller functionality exposed through the clock API. Some devices may need to be brought to an active state before the clock can be used outside. Some may have internal frequency dividers which need to be handled in addition to the clock path in the clock controller. For instance, on Exynos4 the FIMC devices belong to a power domain that needs to be enabled so the clock is not blocked, and this is done through the runtime PM calls. Normally the host device driver runtime resumes the device when /dev/video* is opened. But we might want to use the clock before it happens, when only a /dev/v4l-subdev* is opened, to play with the sensor device only. In this situation the host device needs to be runtime resumed first. Thus the driver would need to (re)implement the clock ops to also handle the details which are not covered by the clock controller driver. The subdev driver would call clk_get(), which would end up being implemented by the driver for whatever hardware block provides the clock. The driver would then runtime_pm_resume() the hardware to start the clock. Yup, makes sense. I also wonder how could we support the boards which choose to use some extra external oscillator to provide clock to the sensors, rather than the one derived from the host. In that case the clock is always running. I'm not sure if we should create a dummy clk object, or just pass a NULL clock and a fixed frequency to the sensor driver. Good point, a dummy (or not really) clock might be a good idea. Especially that it might need to be created anyway. I think, what we need, is a way to describe which clock goes where in terms of DT bindings. Of course the clock names are not now being passed in DT. -- Regards, Sylwester Nawrocki -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RTL2831U driver updates
On 8 August 2011 20:50, Antti Palosaari cr...@iki.fi wrote:
On 08/06/2011 08:13 AM, Alistair Buxton wrote:
On 6 August 2011 04:56, Alistair Buxton a.j.bux...@gmail.com wrote:
With the latest driver my card never gets a signal lock, not even
once. As before there are no error messages. It does always probe
correctly now though.
I tracked this down to:
http://git.linuxtv.org/anttip/media_tree.git/commit/e5d3e4f27f0cf71c29d12ce39752195d8994dea3
and to this specific change:
@@ -459,21 +563,14 @@ static int rtl28xxu_power_ctrl(struct
dvb_usb_device *d, int onoff)
sys0 = sys0 0x0f;
sys0 |= 0xe0;
} else {
-
-#if 0 /* keep */
/*
* FIXME: Use .fe_ioctl_override() to prevent demod
- * IOCTLs in case of device is powered off.
- *
- * For now we cannot power off device because most FE IOCTLs
- * can be performed only when device is powered.
- * Using IOCTLs when device is powered off will result errors
- * because register access to demod fails.
+ * IOCTLs in case of device is powered off. Or change
+ * RTL2830 demod not perform requestesd IOCTL IO when
sleep.
*/
gpio = (~0x01); /* GPIO0 = 0 */
gpio |= 0x10; /* GPIO4 = 1 */
sys0 = sys0 (~0xc0);
-#endif
}
deb_info(%s: WR SYS0=%02x GPIO_OUT_VAL=%02x\n, __func__, sys0,
gpio);
Commenting those three lines makes the driver work again. Don't know
yet if it will keep working for longer than a couple of days.
I suspect it is tuner GPIO. It is not clear which GPIO pins are used for
tuner reset. I should find this out soon. Most likely MXL5005S tuner
reset have connected to different GPIO than MT2060 design I have.
Could you test commenting out only gpio change to see if it helps?
After a couple of days the card locked up again just like before. Are
there more gpio bits I need to comment? The LED GPIO is definitely
right. Also, is there a way to check the status of the GPIO from
userspace, without recompiling the module? Like maybe with i2c-dev or
something?
--
Alistair Buxton
a.j.bux...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
On Tue, 9 Aug 2011, Hans de Goede wrote: I would really like to see the dual mode camera and TV tuner discussion separated. They are 2 different issues AFAIK. 1) Dual mode cameras: In the case of the dual mode camera we have 1 single device (both at the hardware level and at the logical block level), which can do 2 things, but not at the same time. It can stream live video data from a sensor, or it can retrieve earlier taken pictures from some picture memory. Unfortunately even though these 2 functions live in a single logical block, historically we've developed 2 drivers for them. This leads to fighting over device ownership (surprise surprise), and to me the solution is very clear, 1 logical block == 1 driver. According to Theodore, we have developed 5 drivers for them because the stillcam modes in different devices use four different vendor-specific drivers. Does it really make sense to combine 5 drivers into one? I think some sort of sharing protocol would work out better. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
Hi I've been thinking about the Kernel driver side. Mauro and others emailed requirements on Jun or July. I'm sorry for this spam: maybe you have thought this already. A linked list of read/write locks as the solution for these protections could be a base for the general solution. Locks could be accessed either by name string name or by an integer identifier. The bridge driver would be the container of the lock list. Lock take / release handles would be changeable by Bridge driver at driver init. This way bridge could tune the lock taking actions. Sub devices would not have to know the details of the bridge device. When implemented as a library .ko module, this could be used by all related kernel drivers. The locking code would be very general. Maybe adding a lock list into each PCI bus device would solve the device export problem to KVM too. If some module wouldn't handle the proper locking yet, it would not deliver the protection, but it would work as before: no regressions. The library could also be called so that a driver would ask three locks at the same time. If the driver would get all three locks, it would return success. If the driver would not get all three locks, it would not lock any of them (with _trylock case). I don't have time to implement this feature. Happy meeting for all of you, Marko Ristola -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANN] Meeting minutes of the Cambourne meeting
Subash Patel wrote: Hi Sakari, Hi Subash, I have a point with the pixel clock. During discussion we found that pixel clock get/set is required for user space to do fine control over the frame-rate etc. What if the user sets the pixel array clock which is above the system/if bus clock? Suppose we are setting the pixel clock The pixel array clock should be calculated by the driver based on CSI-2 bus frequency (user specified), lanes (from board data), binning, skipping and crop. This is since there are typically limitations for its value, and the sensor driver can come up with a best value for it, based on the information above. Exactly how, that depends on the sensor driver and the sensor. So the pixel array clock is read-only for the user, and the frame rate can then be chosen using the blanking configuration. (which user space sets) to higher rate at sensor array, but for some reason the bus cannot handle that rate (either low speed or loaded) or lower PCLK at say CSI2 interface is being set. Are we not going to loose data due to this? Also, there would be data validation overhead in driver on what is acceptable PCLK values for a particular sensor on an interface etc. This is something that must be handled independently of the way the sensor pixel clock is configured. Typically the limitation is on either the bus frequency or the pixel rate on the bus. This actually can be better avoided when the user has a chance to choose the bus frequency explicitly rather than receive just something the driver happens to produce based on frame rate and resolution settings. I am still not favoring user space controlling this, and wish driver decides this for a given frame-rate requested by the user space :) Frame-rate resolution HSYNC VSYNC PCLK(array) PCLK (i/f bus) ... You can still do that, but it comes with limitations. Any fixed set of the above parameters is very hardware and use case dependent. Let user space control only first two, and driver decide rest (PCLK can be different at different ISP h/w units though) I'm definitely not against this. We do have drivers which use this kind of interface already and some vendors do not even provide enough information to write a driver for their sensor offering any other kind of interface. Cheers, -- Sakari Ailus sakari.ai...@maxwell.research.nokia.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] [media] ddbridge: fix ddb_ioctl()
There were a several problems in this function:
1) Potential integer overflow in the comparison:
if (fio.write_len + fio.read_len 1028) {
2) If the user gave bogus values for write_len and read_len then
returning -EINVAL is more appropriate than returning -ENOMEM.
3) wbuf was set to the address of an array and could never be NULL
so I removed the pointless NULL check.
4) The call to vfree(wbuf) was improper. That array is part of a
larger struct and isn't allocated by itself.
5) flashio() can't actually fail, but we may as well add error
handling in case this changes later.
6) In the default case where an ioctl is not implemented then
returning -ENOTTY is more appropriate than returning -EFAULT.
Signed-off-by: Dan Carpenter erro...@gmail.com
diff --git a/drivers/media/dvb/ddbridge/ddbridge-core.c
b/drivers/media/dvb/ddbridge/ddbridge-core.c
index 573d540..fe56703 100644
--- a/drivers/media/dvb/ddbridge/ddbridge-core.c
+++ b/drivers/media/dvb/ddbridge/ddbridge-core.c
@@ -1438,7 +1438,7 @@ static long ddb_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
{
struct ddb *dev = file-private_data;
void *parg = (void *)arg;
- int res = -EFAULT;
+ int res;
switch (cmd) {
case IOCTL_DDB_FLASHIO:
@@ -1447,29 +1447,29 @@ static long ddb_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
u8 *rbuf, *wbuf;
if (copy_from_user(fio, parg, sizeof(fio)))
- break;
- if (fio.write_len + fio.read_len 1028) {
- printk(KERN_ERR IOBUF too small\n);
- return -ENOMEM;
- }
+ return -EFAULT;
+
+ if (fio.write_len 1028 || fio.read_len 1028)
+ return -EINVAL;
+ if (fio.write_len + fio.read_len 1028)
+ return -EINVAL;
+
wbuf = dev-iobuf[0];
- if (!wbuf)
- return -ENOMEM;
rbuf = wbuf + fio.write_len;
- if (copy_from_user(wbuf, fio.write_buf, fio.write_len)) {
- vfree(wbuf);
- break;
- }
- res = flashio(dev, wbuf, fio.write_len,
- rbuf, fio.read_len);
+
+ if (copy_from_user(wbuf, fio.write_buf, fio.write_len))
+ return -EFAULT;
+ res = flashio(dev, wbuf, fio.write_len, rbuf, fio.read_len);
+ if (res)
+ return res;
if (copy_to_user(fio.read_buf, rbuf, fio.read_len))
- res = -EFAULT;
+ return -EFAULT;
break;
}
default:
- break;
+ return -ENOTTY;
}
- return res;
+ return 0;
}
static const struct file_operations ddb_fops = {
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANN] Meeting minutes of the Cambourne meeting
nitesh moundekar wrote: Hi all, Hi Nitesh, I am worried about direction v4l2 is taking. It looks against the basic principle of driver i.e. hardware abstraction. So i think giving out pixel clock, binning, skipping, bayer pattern, etc device varying features to user space questionable. We can try to remain generic and proprietary or internal device information can be exposed at subdev level or via sysfs. Welcome to the world of embedded devices... What this would provide you is a way to configure sensors in a generic way at low level without enforcing policies or putting artificial limitations in place while being able to better gain information on the capabilities of the devices in user space. This level of control is essential when implementing digital cameras, be they high end or low end in terms of hardware. If you're not doing that, then this interface might not be relevant to you. Also, this is not meant by any means to replace existing interfaces used by applications. -- Sakari Ailus sakari.ai...@maxwell.research.nokia.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH] Modify volatile auto cluster handling as per earlier discussions
This patch modifies the way autoclusters work when the 'foo' controls are
volatile if autofoo is on.
E.g.: if autogain is true, then gain returns the gain set by the autogain
circuitry.
This patch makes the following changes:
1) The V4L2_CTRL_FLAG_VOLATILE flag is added to let userspace know that a
certain
control is volatile. Currently this is internal information only.
2) If v4l2_ctrl_auto_cluster() is called with the last 'set_volatile' argument
set
to true, then the cluster has the following behavior:
- when in manual mode you can set the manual values normally.
- when in auto mode any new manual values will be ignored. When you
read the manual values you will get those as determined by the auto mode.
- when switching from auto to manual mode the manual values from the auto
mode are obtained through g_volatile_ctrl first. Any manual values
explicitly
set by the application will replace those obtained from the automode and
the
final set of values is sent to the driver with s_ctrl.
- when in auto mode the V4L2_CTRL_FLAG_VOLATILE and INACTIVE flags are set
for
the 'foo' controls. These flags are cleared when in manual mode.
This patch modifies existing users of is_volatile and simplifies the pwc driver
that required this behavior.
The only thing missing is the documentation update and some code comments.
I have to admit that it works quite well.
Hans, can you verify that this does what you wanted it to do?
Regards,
Hans
diff --git a/drivers/media/radio/radio-wl1273.c
b/drivers/media/radio/radio-wl1273.c
index 46cacf8..6d1e4e7 100644
--- a/drivers/media/radio/radio-wl1273.c
+++ b/drivers/media/radio/radio-wl1273.c
@@ -2109,7 +2109,7 @@ static int __devinit wl1273_fm_radio_probe(struct
platform_device *pdev)
V4L2_CID_TUNE_ANTENNA_CAPACITOR,
0, 255, 1, 255);
if (ctrl)
- ctrl-is_volatile = 1;
+ ctrl-flags |= V4L2_CTRL_FLAG_VOLATILE;
if (radio-ctrl_handler.error) {
r = radio-ctrl_handler.error;
diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c
b/drivers/media/radio/wl128x/fmdrv_v4l2.c
index 8c0e192..54b34e5 100644
--- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
+++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
@@ -557,7 +557,7 @@ int fm_v4l2_init_video_device(struct fmdev *fmdev, int
radio_nr)
255, 1, 255);
if (ctrl)
- ctrl-is_volatile = 1;
+ ctrl-flags |= V4L2_CTRL_FLAG_VOLATILE;
return 0;
}
diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
index 279d75d..d0e8ac1 100644
--- a/drivers/media/video/adp1653.c
+++ b/drivers/media/video/adp1653.c
@@ -258,7 +258,7 @@ static int adp1653_init_controls(struct adp1653_flash
*flash)
if (flash-ctrls.error)
return flash-ctrls.error;
- fault-is_volatile = 1;
+ fault-flags |= V4L2_CTRL_FLAG_VOLATILE;
flash-subdev.ctrl_handler = flash-ctrls;
return 0;
diff --git a/drivers/media/video/pwc/pwc-v4l.c
b/drivers/media/video/pwc/pwc-v4l.c
index e9a0e94..4ce00bf 100644
--- a/drivers/media/video/pwc/pwc-v4l.c
+++ b/drivers/media/video/pwc/pwc-v4l.c
@@ -83,6 +83,7 @@ static const struct v4l2_ctrl_config pwc_contour_cfg = {
.id = PWC_CID_CUSTOM(contour),
.type = V4L2_CTRL_TYPE_INTEGER,
.name = Contour,
+ .flags = V4L2_CTRL_FLAG_SLIDER,
.min= 0,
.max= 63,
.step = 1,
@@ -206,8 +207,7 @@ int pwc_init_controls(struct pwc_device *pdev)
pdev-blue_balance = v4l2_ctrl_new_std(hdl, pwc_ctrl_ops,
V4L2_CID_BLUE_BALANCE, 0, 255, 1, def);
- v4l2_ctrl_auto_cluster(3, pdev-auto_white_balance, awb_manual,
- pdev-auto_white_balance-cur.val == awb_auto);
+ v4l2_ctrl_auto_cluster(3, pdev-auto_white_balance, awb_manual, true);
/* autogain, gain */
r = pwc_get_u8_ctrl(pdev, GET_LUM_CTL, AGC_MODE_FORMATTER, def);
@@ -331,12 +331,12 @@ int pwc_init_controls(struct pwc_device *pdev)
pdev-restore_user = v4l2_ctrl_new_custom(hdl, pwc_restore_user_cfg,
NULL);
if (pdev-restore_user)
- pdev-restore_user-flags = V4L2_CTRL_FLAG_UPDATE;
+ pdev-restore_user-flags |= V4L2_CTRL_FLAG_UPDATE;
pdev-restore_factory = v4l2_ctrl_new_custom(hdl,
pwc_restore_factory_cfg,
NULL);
if (pdev-restore_factory)
- pdev-restore_factory-flags = V4L2_CTRL_FLAG_UPDATE;
+ pdev-restore_factory-flags |= V4L2_CTRL_FLAG_UPDATE;
if (!pdev-features FEATURE_MOTOR_PANTILT)
return hdl-error;
@@ -563,8 +563,10 @@ static int pwc_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
omap3isp driver
Hi, Does the omap3isp driver implementation support rgb888 format? Regards, sriram -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Workshop-2011] Media Subsystem Workshop 2011
On Tue, 9 Aug 2011, Hans de Goede wrote: Hi, On 08/08/2011 07:39 PM, Theodore Kilgore wrote: On Mon, 8 Aug 2011, Mauro Carvalho Chehab wrote: snip Mauro, In fact none of the currently known and supported cameras are using PTP. All of them are proprietary. They have a rather intimidating set of differences in functionality, too. Namely, some of them have an isochronous endpoint, and some of them rely exclusively upon bulk transport. Some of them have a well developed set of internal capabilities as far as handling still photos are concerned. I mean, such things as the ability to download a single photo, selected at random from the set of photos on the camera, and some do not, requiring that the ability to do this is emulated in software -- by first downloading all previously listed photos and sending the data to /dev/null, then downloading the desired photo and saving it. Some of them permit deletion of individual photos, or all photos, and some do not. For some of them it is even true, as I have previously mentioned, that the USB command string which will delete all photos is the same command used for starting the camera in streaming mode. But the point here is that these cameras are all different from one another, depending upon chipset and even, sometimes, upon firmware or chipset version. The still camera abilities and limitations of all of them are pretty much worked out in libgphoto2. My suggestion would be that the libgphoto2 support libraries for these cameras ought to be left the hell alone, except for some changes in, for example, how the camera is accessed in the first place (through libusb or through a kernel device) in order to address adequately the need to support both modes. I know what is in those libgphoto2 drivers because I wrote them. I can definitely promise that to move all of that functionality over into kernel modules would be a nightmare and would moreover greatly contribute to kernel bloat. You really don't want to go there. I strongly disagree with this. The libgphoto2 camlibs (drivers) for these cameras handle a number of different tasks: 1) Talking to the camera getting binary blobs out of them (be it a PAT or some data) 2) Interpreting said blobs 3) Converting the data parts to pictures doing post processing, etc. I'm not suggesting to move all of this to the kernel driver, we just need to move part 1. to the kernel driver. I did not assume otherwise. This is not rocket science. No, but both Adam and I realized, approximately at the same time yesterday afternoon, something which is rather important here. Gphoto is not developed exclusively for Linux. Furthermore, it has a significant user base both on Windows and on MacOS, not to mention BSD. It really isn't nice to be screwing around too much with the way it works. We currently have a really bad situation were drivers are fighting for the same device. The problem here is that these devices are not only one device on the physical level, but also one device on the logical level (IOW they have only 1 usb interface). All true. Which is why I brought the topic up for discussion in the first place and why it now gets on the program of the USB Summit. It is time to quit thinking in band-aides and solve this properly, 1 logical device means it gets 1 driver. This may be an approach which means some more work then others, but I believe in the end that doing it right is worth the effort. Clearly, we agree about doing it right is worth the effort. The whole discussion right now is about what is right. As for Mauro's resource locking patches, these won't work because the assume both drivers are active at the same time, which is simply not true. Only 1 driver can be bound to the interface at a time, and when switching from the gspca driver to the usbfs driver, gspca will see an unplug which is indistinguishable from a real device unplug. Things would not have to happen so, of course. Things did not used to happen so. Presence of kernel support for streaming used to block stillcam access through libusb. Period. End of discussion. The code change in libusb which changes that default behavior is quite recent. It was done because the kernel was *not* addressing the problem at all. That change could presumably be reversed if it were decided that the kernel is going to do the work instead. A POV could be defended, that this behavior of libusb was put in as a stopgap measure because the kernel was not doing its job. In which case the right thing to do is to put the missing functionality into the kernel drivers and take out from libusb the attempt to provide it, when libusb really can't do the job completely. More over a kernel only solution without libgphoto changes won't solve the problem of a libgphoto app keeping the device open locking out streaming. Eh? You really lose me with this
Re: USB mini-summit at LinuxCon Vancouver
On Tue, Aug 09, 2011 at 09:52:44AM +0200, Hans de Goede wrote: Hi, On 08/08/2011 07:58 PM, Sarah Sharp wrote: On Fri, Aug 05, 2011 at 09:45:56AM +0200, Hans de Goede wrote: Hi, On 08/05/2011 12:56 AM, Greg KH wrote: On Thu, Aug 04, 2011 at 07:21:47PM -0300, Mauro Carvalho Chehab wrote: I think it is important to separate oranges from apples here, there are at least 3 different problem classes which all seem to have gotten thrown onto a pile here: 1) The reason Mauro suggested having some discussion on this at the USB summit is because of a discussion about dual mode cameras on the linux media list. ... 3) Re-direction of usb devices to virtual machines. This works by using the userspace usbfs interface from qemu / vmware / virtualbox / whatever. The basics of this work fine, but it lacks proper locking / safeguards for when a vm takes over a usb device from the in kernel driver. Hi Hans and Mauro, We have do room in the schedule for the USB mini-summit for this discussion, since the schedule is still pretty flexible. The preliminary schedule is up here: http://userweb.kernel.org/~sarah/linuxcon-usb-minisummit.html I think it's best to discuss the VM redirection in the afternoon when some of the KVM folks join us after Hans' talk on USB redirection over the network. That seems best to me too. I'm available at both proposed time slots, and I would like to join both discussions. It sounds like we need a separate topic for the dual mode cameras and TV tuners. Mauro, do you want to lead that discussion in the early morning (in a 9:30 to 10:30 slot) or in the late afternoon (in a 15:30 to 16:30 slot)? I want to be sure we have all the video/media developers who are interested in this topic present, and I don't know if they will be going to the KVM forum. I would really like to see the dual mode camera and TV tuner discussion separated. They are 2 different issues AFAIK. Ok, great, I've put both topics in the morning, in separate slots. Sarah Sharp -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Workshop-2011] Media Subsystem Workshop 2011
On Tue, 9 Aug 2011, Hans de Goede wrote: Hi, snip OK, another example. The cameras supported in camlibs/jl2005c do not have webcam ability, but someone could at any time design and market a dualmode which has in stillcam mode the same severe limitation. What limitation? Well, the entire memory of the camera must be dumped, or else the camera jams itself. You can stop dumping in the middle of the operation, but you must continue after that. Suppose that you had ten pictures on the camera and you only wanted to download the first one. Then you can do that and temporarily stop downloading the rest. But while exiting you have to check whether the rest are downloaded or not. And if they are not, then it has to be done, with the data simply thrown in the trash, and then the camera's memory pointer reset before the camera is released. How, one might ask, did anyone produce something so primitive? Well, it is done. Perhaps the money saved thereby was at least in part devoted to producing better optics for the camera. At least, one can hope so. But people did produce those cameras, and people have bought them. But does anyone want to reproduce the code to support this kind of crap in the kernel? And go through all of the hoops required in order to fake the behavior which one woulld expect from a real still camera? It has already been done in camlibs/jl2005c and isn't that enough? This actually is an example where doing a kernel driver would be easier, a kernel driver never exits. So it can simply remember where it was reading (and cache the data it has read sofar). If an app requests picture 10, we read 1-10, cache them and return picture 10 to the app, then the same or another app asks for picture 4, get it from cache, asks for picture 20 read 11-20, etc. This, in fact, is the way that the OEM software for most of these cheap cameras works. The camera is dumped, and then raw files for the pictures are created in C:\TEMP. Then the raw files are all processed immediately into viewable pictures, after which thumbnails (which did not previously exist as separate entities) can be created for use in the GUI app. Then, if the user chooses to save certain of the photos, the chosen photos are merely copied to a more permanent location. And when the camera-accessing app is exited, the temporary files are all deleted. Clearly, the OEM approach recommends itself for simplicity. Nevertheless, there is an obvious disadvantage. Namely, *all* of the raw data from the camera needs to be fetched and, as you say, kept in cache. That cache is either going to use RAM, or is going to be based in swap. And not every piece of hardware is a big, honking system with plenty of gigabytes in the RAM slots, and moreover there exist systems with low memory where it is also considered not a good idea to use swap. Precisely because of these realities, the design of libgphoto2 has consciously rejected the approach used in the OEM drivers. Rather, it is a priority to lower the memory footprint by dealing with the data piece by piece. This means, essentially, handling the photos on the camera one at a time. It is worth considering that some of the aforementioned low-powered systems with low quantities of RAM on board, and with no allocated swap space are running Linux these days. Having written code for various small digital picture frames (the keychain models) I know where you are coming from. Trust me I do. Not to worry. I know where you are coming from, too. Trust me I do. Recently I had an interesting bug report, with a corrupt PAT (picture allocation table) turns out that when deleting a picture through the menu inside the frame a different marker gets written to the PAT then when deleting it with the windows software, Fun huh? Yes, of course it is fun. We should not have signed up to do this kind of work if we can't take a joke, right? But, more seriously, there may be some reason why that different character is used -- or OTOH maybe not, and somebody was just being silly. Unfortunately, experience tells me it is probably necessary to figure out which of the two possibilities it is. Theodore Kilgore -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[cron job] v4l-dvb daily build: ERRORS
This message is generated daily by a cron job that builds v4l-dvb for the kernels and architectures in the list below. Results of the daily build of v4l-dvb: date:Tue Aug 9 19:00:33 CEST 2011 git hash:9bed77ee2fb46b74782d0d9d14b92e9d07f3df6e gcc version: i686-linux-gcc (GCC) 4.5.1 host hardware:x86_64 host os: 2.6.32.5 linux-git-armv5: WARNINGS linux-git-armv5-davinci: WARNINGS linux-git-armv5-ixp: WARNINGS linux-git-armv5-omap2: WARNINGS linux-git-i686: WARNINGS linux-git-m32r: OK linux-git-mips: WARNINGS linux-git-powerpc64: WARNINGS linux-git-x86_64: WARNINGS linux-2.6.31.12-i686: ERRORS linux-2.6.32.6-i686: WARNINGS linux-2.6.33-i686: WARNINGS linux-2.6.34-i686: WARNINGS linux-2.6.35.3-i686: WARNINGS linux-2.6.36-i686: WARNINGS linux-2.6.37-i686: WARNINGS linux-2.6.38.2-i686: WARNINGS linux-2.6.39.1-i686: WARNINGS linux-3.0-i686: WARNINGS linux-3.1-rc1-i686: WARNINGS linux-2.6.31.12-x86_64: ERRORS linux-2.6.32.6-x86_64: WARNINGS linux-2.6.33-x86_64: WARNINGS linux-2.6.34-x86_64: WARNINGS linux-2.6.35.3-x86_64: WARNINGS linux-2.6.36-x86_64: WARNINGS linux-2.6.37-x86_64: WARNINGS linux-2.6.38.2-x86_64: WARNINGS linux-2.6.39.1-x86_64: WARNINGS linux-3.0-x86_64: WARNINGS linux-3.1-rc1-x86_64: WARNINGS spec-git: ERRORS sparse: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.tar.bz2 The V4L-DVB specification from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add support for the dvb-t part of CT-3650 v3
On Lunes, 8 de Agosto de 2011 23:44:43 Antti Palosaari escribió:
Reviewed-by: Antti Palosaari cr...@iki.fi
It looks just fine.
regards
Antti
Forgot the Signed-off-by
Signed-off-by: Jose Alberto Reguero jaregu...@telefonica.net
Jose Alberto
On 08/08/2011 01:35 PM, Jose Alberto Reguero wrote:
On Martes, 2 de Agosto de 2011 21:21:13 Jose Alberto Reguero escribió:
On Jueves, 28 de Julio de 2011 21:25:01 Jose Alberto Reguero escribió:
On Miércoles, 27 de Julio de 2011 21:22:26 Antti Palosaari escribió:
On 07/24/2011 12:45 AM, Jose Alberto Reguero wrote:
Read without write work as with write. Attached updated patch.
ttusb2-6.diff
- read = i+1 num (msg[i+1].flags I2C_M_RD);
+ write_read = i+1 num (msg[i+1].flags I2C_M_RD);
+ read = msg[i].flags I2C_M_RD;
ttusb2 I2C-adapter seems to be fine for my eyes now. No more writing
any random bytes in case of single read. Good!
+ .dtv6_if_freq_khz = TDA10048_IF_4000,
+ .dtv7_if_freq_khz = TDA10048_IF_4500,
+ .dtv8_if_freq_khz = TDA10048_IF_5000,
+ .clk_freq_khz = TDA10048_CLK_16000,
+static int ct3650_i2c_gate_ctrl(struct dvb_frontend* fe, int enable)
cosmetic issue rename to ttusb2_ct3650_i2c_gate_ctrl
{ TDA10048_CLK_16000, TDA10048_IF_4000, 10, 3, 0 },
+ { TDA10048_CLK_16000, TDA10048_IF_4500, 5, 3, 0 },
+ { TDA10048_CLK_16000, TDA10048_IF_5000, 5, 3, 0 },
+ /* Set the pll registers */
+ tda10048_writereg(state, TDA10048_CONF_PLL1, 0x0f);
+ tda10048_writereg(state, TDA10048_CONF_PLL2, (u8)(state-
pll_mfactor));
+ tda10048_writereg(state, TDA10048_CONF_PLL3,
tda10048_readreg(state, TDA10048_CONF_PLL3) |
((u8)(state-pll_nfactor) | 0x40));
This if only issue can have effect to functionality and I want double
check. I see few things.
1) clock (and PLL) settings should be done generally only once at
.init() and probably .sleep() in case of needed for sleep. Something
like start clock in init, stop clock in sleep. It is usually very
first thing to set before other. Now it is in wrong place -
.set_frontend().
2) Those clock settings seem somehow weird. As you set different PLL M
divider for 6 MHz bandwidth than others. Have you looked those are
really correct? I suspect there could be some other Xtal than 16MHz
and thus those are wrong. Which Xtals there is inside device used?
There is most likely 3 Xtals, one for each chip. It is metal box
nearest to chip.
I left 6MHz like it was before in the driver. I try to do other way,
allowing to put different PLL in config that the default ones of the
driver and initialize it in init.
Jose Alberto
Attached new version of the patch. Adding tda827x lna and doing tda10048
pll in other way.
Jose Alberto
Another version, without tda827x lna. It don't improve anything.
Jose Alberto
Ran checkpatch.pl also to find out style issues before send patch.
I have send new version, hopefully final, of MFE. It changes array
name from adap-mfe to adap-fe. You should also update that.
regards
Antti
--
To unsubscribe from this list: send the line unsubscribe linux-media
in the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
Hi, On 08/09/2011 04:19 PM, Alan Stern wrote: On Tue, 9 Aug 2011, Hans de Goede wrote: I would really like to see the dual mode camera and TV tuner discussion separated. They are 2 different issues AFAIK. 1) Dual mode cameras: In the case of the dual mode camera we have 1 single device (both at the hardware level and at the logical block level), which can do 2 things, but not at the same time. It can stream live video data from a sensor, or it can retrieve earlier taken pictures from some picture memory. Unfortunately even though these 2 functions live in a single logical block, historically we've developed 2 drivers for them. This leads to fighting over device ownership (surprise surprise), and to me the solution is very clear, 1 logical block == 1 driver. According to Theodore, we have developed 5 drivers for them because the stillcam modes in different devices use four different vendor-specific drivers. Yes, but so the the webcam modes of the different devices, so for the 5 (not sure if that is the right number) dual-cam mode chipsets we support there will be 5 drivers, each supporting both the webcam and the access to pictures stored in memory of the chipset they support. So 5 chipsets - 5 drivers each supporting 1 chipset, and both functions of the single logical device that chipset represents. Does it really make sense to combine 5 drivers into one? Right, that is not the plan. The plan is to simply stop having 2 drivers for 1 logical (and physical) block. So we go from 10 drivers, 5 stillcam + 5 webcam, to just 5 drivers. We will also likely be able to share code between the code for the 2 functionalities for things like generic set / get register functions, initialization, etc. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PATCHES FOR 3.1] s5p-fimc and noon010pc30 driver updates
Em 29-07-2011 05:36, Laurent Pinchart escreveu: Hi Mauro, On Friday 29 July 2011 06:02:54 Mauro Carvalho Chehab wrote: Em 28-07-2011 19:57, Sylwester Nawrocki escreveu: On 07/28/2011 03:20 PM, Mauro Carvalho Chehab wrote: Accumulating sub-dev controls at the video node is the right thing to do. An MC-aware application will need to handle with that, but that doesn't sound to be hard. All such application would need to do is to first probe the subdev controls, and, when parsing the videodev controls, not register controls with duplicated ID's, or to mark them with some special attribute. IMHO it's not a big issue in general. Still, both subdev and the host device may support same control id. And then even though the control ids are same on the subdev and the host they could mean physically different controls (since when registering a subdev at the host driver the host's controls take precedence and doubling subdev controls are skipped). True, but, except for specific usecases, the host control is enough. Not for embedded devices. In many case the control won't even be implemented by the host. If your system has two sensors connected to the same host, they will both expose an exposure time control. Which one do you configure through the video node ? The two sensors will likely have different bounds for the same control, how do you report that ? If the device has two sensors that are mutually exclusive, they should be mapped as two different inputs. The showed control should be the one used by the currently active input. If the sensors aren't mutually exclusive, then two different video nodes will be shown in userspace. Those controls are also quite useless for generic V4L2 applications, which will want auto-exposure anyway. This needs to be implemented in userspace in libv4l. Several webcams export exposure controls. Why shouldn't those controls be exposed to userspace anymore? Ok, if the hardware won't support 3A algorithm, libv4l will implement it, eventually using an extra hardware-aware code to get the best performance for that specific device, but this doesn't mean that the user should always use it. Btw, the 3A algorithm is one of the things I don't like on my cell phone: while it works most of the time, sometimes I want to disable it and manually adjust, as it produces dark images, when there's a very bright light somewhere on the image background. Manually adjusting the exposure time and aperture is something relevant for some users. Also there might be some preference at user space, at which stage of the pipeline to apply some controls. This is where the subdev API helps, and plain video node API does not. Again, this is for specific usecases. On such cases, what is expected is that the more generic control will be exported via V4L2 API. Thus it's a bit hard to imagine that we could do something like optionally not to inherit controls as the subdev/MC API is optional. :) This was actually implemented. There are some cases at ivtv/cx18 driver where both the bridge and a subdev provides the same control (audio volume, for example). The idea is to allow the bridge driver to touch at the subdev control without exposing it to userspace, since the desire was that the bridge driver itself would expose such control, using a logic that combines changing the subdev and the bridge registers for volume. This seem like hard coding a policy in the driver;) Then there is no way (it might not be worth the effort though) to play with volume level at both devices, e.g. to obtain optimal S/N ratio. In general, playing with just one control is enough. Andy had a different opinion when this issue were discussed, and he thinks that playing with both is better. At the end, this is a developers decision, depending on how much information (and bug reports) he had. ivtv/cx18 is a completely different use case, where hardware configurations are known, and experiments possible to find out which control(s) to use and how. In this case you can't know in advance what the sensor and host drivers will be used for. Why not? I never saw an embedded hardware that allows physically changing the sensor. Even if you did, fine image quality tuning requires accessing pretty much all controls individually anyway. The same is also true for non-embedded hardware. The only situation where V4L2 API is not enough is when there are two controls of the same type active. For example, 2 active volume controls, one at the audio demod, and another at the bridge. There may have some cases where you can do the same thing at the sensor or at a DSP block. This is where MC API gives an improvement, by allowing changing both, instead of just one of the controls. This is a hack...sorry, just joking ;-) Seriously, I think the situation with the userspace subdevs is a bit different. Because with one API we directly expose some functionality for applications, with
Re: [Workshop-2011] Media Subsystem Workshop 2011
Hi, On 08/09/2011 07:10 PM, Theodore Kilgore wrote: On Tue, 9 Aug 2011, Hans de Goede wrote: snip No, but both Adam and I realized, approximately at the same time yesterday afternoon, something which is rather important here. Gphoto is not developed exclusively for Linux. Furthermore, it has a significant user base both on Windows and on MacOS, not to mention BSD. It really isn't nice to be screwing around too much with the way it works. Right, so my plan is not to rip out the existing camlibs from libgphoto2, but to instead add a new camlib which talks to /dev/video# nodes which support the new to be defined v4l2 API for this. This camlib will then take precedence over the old libusb based ones when running on a system which has a new enough kernel. On systems without the new enough kernel the matching portdriver won't find any ports, so the camlib will be effectively disabled. On BSD the port driver for this new /dev/video# API and the camlib won't even get compiled. snip It is time to quit thinking in band-aides and solve this properly, 1 logical device means it gets 1 driver. This may be an approach which means some more work then others, but I believe in the end that doing it right is worth the effort. Clearly, we agree about doing it right is worth the effort. The whole discussion right now is about what is right. I'm sorry but I don't get the feeling that the discussion currently is focusing on what is right. To me too much attention is being spend on not throwing away the effort put in the current libgphoto2 camlibs, which I don't like for 2 reasons: 1) It distracts from doing what is right 2) It ignores the fact that a lot has been learned in doing those camlibs, really really a lot. and all that can be re-used in a kernel driver. Let me try to phrase it in a way I think you'll understand. If we agree on doing it right over all other things (such as the fact that doing it right may take a considerable effort). Then this could be an interesting assignment for some of the computer science students I used to be a lecturer for. This assignment could read something like Given the existing situation and knowledge describe all that here, do a re-design for the driverstack for these dual mode cameras, assuming a completely fresh start. Now if I were to give this assignment to a group of students, and they would keep coming back with the but re-doing the camlibs in kernelspace is such a large effort, and we already have them in userspace argument against using one unified driver for these devices, I would give them an F, because they are clearly missing the assuming a completely fresh start part of the assignment. I'm sorry if this sounds a bit harsh, but this is the way how the current discussion feels to me. If we agree on aiming for doing it right then with that comes to me doing a software design from scratch, so without taking into account what is already there. There are of course limits to the from scratch part, in the end we want this to slot into the existing Linux practices for webcams and stillcams, which means: 1) offering a v4l2 /dev/video# node for streaming; and 2) access to the pictures stored on the camera through libgphoto Taking these 2 constrictions into account, and combining that with my firm believe that the solution to all the device sharing problems is handling both functions in a single driver, I end up with only 1 option: Have a kernel driver which provides both functions of the device, with the streaming exported as a standard v4l2 device, and the stillcam function exported with some to be defined API. Combined with a libgphoto2 portlib and camlib for this new API, so that existing libgphoto2 apps can still access the pictures as if nothing was changed. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
On Tuesday 09 August 2011, Hans de Goede wrote: Hi, On 08/09/2011 04:19 PM, Alan Stern wrote: Does it really make sense to combine 5 drivers into one? Right, that is not the plan. The plan is to simply stop having 2 drivers for 1 logical (and physical) block. So we go from 10 drivers, 5 stillcam + 5 webcam, to just 5 drivers. We will also likely be able to share code between the code for the 2 functionalities for things like generic set / get register functions, initialization, etc. Unfortunately as Theodore recently pointed out you don't go from 10 to 5, you go from 10 to 10 where 5 of the new 10 are only used on Win32, FreeBSD and OSX (but they aren't any simpler because they still rely on libusb) and the other 5 that are only used on Linux become significantly more complicated than they currently are. It has also just occured to me that it might be possible to solve the issues we are facing just in the kernel. At the moment when the kernel performs a USBDEVFS_DISCONNECT it keeps the kernel driver locked out until userspace performs a USBDEVFS_CONNECT. If the kernel reattached the kernel driver when the device file was closed then, as gvfs doesn't keep the file open the biggest current issue would be solved instantly. If a mechanism could be found to prevent USBDEVFS_DISCONNECT from succeeding when the corresponding /dev/videox file was open then that would seem to be a reasonable solution. Hans had expressed the opinion that merely having the device open to control the camera not to stream shouldn't prevent stillcam operation - I disagree because if you are setting up the controls you are probably already streaming so you can see what you are doing and if not you are probably about to. Of course changing the behaviour of USBDEVFS_DISCONNECT is not something to be done lightly. I don't know how many other users there are for it and if the current behaviour is actually correct for any of them. Cleaning up on file close does have the useful side effect though that applications no longer need to worry about the fact that even if they clean up properly on a normal exit, if they crash they leave the kernel driver permanently disabled Regards Adam -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
Hi, On 08/09/2011 10:31 PM, Adam Baker wrote: On Tuesday 09 August 2011, Hans de Goede wrote: snip It has also just occured to me that it might be possible to solve the issues we are facing just in the kernel. At the moment when the kernel performs a USBDEVFS_DISCONNECT it keeps the kernel driver locked out until userspace performs a USBDEVFS_CONNECT. If the kernel reattached the kernel driver when the device file was closed then, as gvfs doesn't keep the file open the biggest current issue would be solved instantly. If a mechanism could be found to prevent USBDEVFS_DISCONNECT from succeeding when the corresponding /dev/videox file was open then that would seem to be a reasonable solution. sigh This has been discussed over and over and over again, playing clever tricks with USBDEVFS_[DIS]CONNECT like adding a new USBDEVFS_TRYDISCONNECT which the v4l2 driver could intercept won't cut it. We need some central manager of the device doing multiplexing between the 2 functions, and you can *not* assume that either side will be nice wrt closing file descriptors. Examples: 1) You are wrong wrt gvfs, it does keep the libgphoto2 context open all the time, and through that the usbfs device nodes. 2) Lets say a user starts a photo managing app like f-spot, and that opens the device through libgphoto2 on startup, then the user switches to another virtual desktop and forgets all about having f-spot open. Notice that if the user now tries to stream he will not get a busy error, but the app trying to do the streaming will simply not see the camera at all (kernel driver unbound /dev/video# node is gone). 3) Notice that little speaker icon in your panel on your average Linux desktop, that keeps the mixer of the audio device open *all the time* it is quite easy to imagine a similar applet for v4l2 device controls (see for example gtk-v4l) doing the same. Or a user could simply start up a v4l2 control panel app like gtk-v4l, qv4l2 or v4l2ucp, and leave it running minimized ... 4) Some laptops have a Fn + F## key which enables / disables the builtin webcam by turning its power on / off. Effectively plugging it into / out of a usb port. We would like to have an on screen notification of this one day like we have now for brightness and volume controls, based on udev events. But the current dual mode cam stuff causes udev events for a *new* video device being added / an existing one being removed each time libgphoto2 releases / takes control of the camera. 5) More in general, more and more software is dynamically monitoring the addition / removal of (usb) devices using udev, our current solution suggests to this software the /dev/video device is being unplugged / re-plugged all the time, not pretty. All in all what we've today is a kludge, and if we want to provide a seamless user experience we need to fix it. Don't get me wrong usbfs is a really nice solution for driving usb devices from userspace, like scanners and all other sorts of devices. But what all these devices have in common is that they have no kernel driver. Having a userspace based driver and a kernel driver fight it out just does not work well. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
On Tue, 9 Aug 2011, Hans de Goede wrote:
Hi,
On 08/09/2011 04:19 PM, Alan Stern wrote:
On Tue, 9 Aug 2011, Hans de Goede wrote:
I would really like to see the dual mode camera and TV tuner discussion
separated. They are 2 different issues AFAIK.
1) Dual mode cameras:
In the case of the dual mode camera we have 1 single device (both at
the hardware level and at the logical block level), which can do 2 things,
but not at the same time. It can stream live video data from a sensor,
or it can retrieve earlier taken pictures from some picture memory.
Unfortunately even though these 2 functions live in a single logical
block,
historically we've developed 2 drivers for them. This leads to fighting
over device ownership (surprise surprise), and to me the solution is
very clear, 1 logical block == 1 driver.
According to Theodore, we have developed 5 drivers for them because the
stillcam modes in different devices use four different vendor-specific
drivers.
Yes, but so the the webcam modes of the different devices, so for
the 5 (not sure if that is the right number) dual-cam mode chipsets
we support there will be 5 drivers, each supporting both the
webcam and the access to pictures stored in memory of the chipset
they support. So 5 chipsets - 5 drivers each supporting 1 chipset,
and both functions of the single logical device that chipset
represents.
Does it really make sense to combine 5 drivers into one?
Right, that is not the plan. The plan is to simply stop having 2 drivers
for 1 logical (and physical) block. So we go from 10 drivers, 5 stillcam
+ 5 webcam, to just 5 drivers. We will also likely be able to share
code between the code for the 2 functionalities for things like generic
set / get register functions, initialization, etc.
Probably this is a good time for some inventory to be done.
sure of the exact number of drivers. The jeilinj driver does not count
here. It is a standard mass storage device as a still camera, and if it is
going to be hooked up as a webcam then it has to have some buttons pushed
just so, prior to hookup -- after which it comes up with a different USB
Product number as a proprietary webcam device, supported by the jeilinj
driver. Thus, this is not one of the droids we are looking for.
Specifically, the currently affected drivers are sq905, sq905c, mr97310a,
and sn9c2028.
Fuji Finepix seems to produce both still cameras (PTP protocol) and
webcams. If there are currently any dual-mode cameras, it is not obvious.
Perhaps any of these which can be dual mode are like the jeilinj cameras
and come up under different Product numbers in the different modes, which
is not a problem.
There do seem to exist stv06xx dual-mode cameras:
stv0680/stv0680.c: { STM:USB Dual-mode camera, 0x0553, 0x0202, 1
},
is found in camlibs/stv0680, for example, along with a whole bunch of
other cameras with the same Vendor:Product number. It seems, too, that the
same Vendor:Product number is listed in gspca/stv0680.c
So I guess we found another affected driver.
The intersection seems to be void between the lists of the spca50x cameras
and webcams, though one can not be certain about the future. I do know of
another dualmode camera, powered by the JL2005A chip. I wrote a kernel
module for it some months ago. Said module has not been sent upward
because of mysterious issues with one of the three cameras used for
testing it.
We also find among the gspca driver files this
* GSPCA sub driver for W996[78]CF JPEG USB Dual Mode Camera Chip.
*
* Copyright (C) 2009 Hans de Goede hdego...@redhat.com
but I am not sure about which cameras are using this.
Theodore Kilgore
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PATCHES FOR 3.1] s5p-fimc and noon010pc30 driver updates
Em 09-08-2011 20:18, Sakari Ailus escreveu: Hi Mauro, On Tue, Aug 09, 2011 at 05:05:47PM -0300, Mauro Carvalho Chehab wrote: Em 29-07-2011 05:36, Laurent Pinchart escreveu: Hi Mauro, On Friday 29 July 2011 06:02:54 Mauro Carvalho Chehab wrote: Em 28-07-2011 19:57, Sylwester Nawrocki escreveu: On 07/28/2011 03:20 PM, Mauro Carvalho Chehab wrote: Accumulating sub-dev controls at the video node is the right thing to do. An MC-aware application will need to handle with that, but that doesn't sound to be hard. All such application would need to do is to first probe the subdev controls, and, when parsing the videodev controls, not register controls with duplicated ID's, or to mark them with some special attribute. IMHO it's not a big issue in general. Still, both subdev and the host device may support same control id. And then even though the control ids are same on the subdev and the host they could mean physically different controls (since when registering a subdev at the host driver the host's controls take precedence and doubling subdev controls are skipped). True, but, except for specific usecases, the host control is enough. Not for embedded devices. In many case the control won't even be implemented by the host. If your system has two sensors connected to the same host, they will both expose an exposure time control. Which one do you configure through the video node ? The two sensors will likely have different bounds for the same control, how do you report that ? If the device has two sensors that are mutually exclusive, they should be mapped as two different inputs. The showed control should be the one used by the currently active input. Video nodes should represent a dma engine rather than an image source. True. Image sources are represented, on V4L2, by inputs. You could use the same hardware to process data from memory to memory while streaming video from a sensor to memory at the same time. This is a quite typical use in embedded systems. Usually boards where two sensors are connected to an isp the dependencies are not as straightforward as the sensors being fully independent or require exclusive access. Typically some of the hardware blocks are shared between the two and can be used by only one sensor at a time, so instead you may not get full functionality from both at the same time. And you need to be able to choose which one uses that hardware block. This is exactly what Media controller interface models perfectly. I see. See FIMC Media controller graph AND Sylwester's explanation on it; a few links are actually missing from the grapg. URL:http://www.spinics.net/lists/linux-media/msg35504.html URL:http://wstaw.org/m/2011/05/26/fimc_graph__.png Cc Hans. If the sensors aren't mutually exclusive, then two different video nodes will be shown in userspace. Those controls are also quite useless for generic V4L2 applications, which will want auto-exposure anyway. This needs to be implemented in userspace in libv4l. Several webcams export exposure controls. Why shouldn't those controls be exposed to userspace anymore? This is not a webcam, I know, but the analogy is still valid. it is a software controlled high end digital camera on a mobile device. The difference is that the functionality offered by the hardware is at much lower level compared to a webcam; the result is that more detailed control ia required but also much flexibility and performance is gained. I see. What I failed to see is why to remove control from userspace. If the hardware is more powerful, I expect to see more controls exported, and not removing the V4L2 functionality from the driver. Ok, if the hardware won't support 3A algorithm, libv4l will implement it, eventually using an extra hardware-aware code to get the best performance for that specific device, but this doesn't mean that the user should always use it. Why not? What would be the alternative? User may want or need to disable the 3A algo and control some hardware parameters hardware directly, for example, to take an over-exposed picture, to create some neat effects, or to get some image whose exposure aperture/time is out of the 3A range. Btw, the 3A algorithm is one of the things I don't like on my cell phone: while it works most of the time, sometimes I want to disable it and manually adjust, as it produces dark images, when there's a very bright light somewhere on the image background. Manually adjusting the exposure time and aperture is something relevant for some users. You do want the 3A algorithms even if you use manual white balance. What the automatic white balance algorithm produces is (among other things) gamma tables, rgb-to-rgb conversion matrices and lens shading correction tables. I doubt any end user, even if it was you, would like to fiddle with such large tables directly when capturing photos. There are some hacks for
Re: [Workshop-2011] Media Subsystem Workshop 2011
On Tue, 9 Aug 2011, Hans de Goede wrote: Hi, On 08/09/2011 07:10 PM, Theodore Kilgore wrote: On Tue, 9 Aug 2011, Hans de Goede wrote: snip No, but both Adam and I realized, approximately at the same time yesterday afternoon, something which is rather important here. Gphoto is not developed exclusively for Linux. Furthermore, it has a significant user base both on Windows and on MacOS, not to mention BSD. It really isn't nice to be screwing around too much with the way it works. Right, so my plan is not to rip out the existing camlibs from libgphoto2, but to instead add a new camlib which talks to /dev/video# nodes which support the new to be defined v4l2 API for this. This camlib will then take precedence over the old libusb based ones when running on a system which has a new enough kernel. On systems without the new enough kernel the matching portdriver won't find any ports, so the camlib will be effectively disabled. And then, I assume you mean, the old camlib will still work. On BSD the port driver for this new /dev/video# API and the camlib won't even get compiled. snip It is time to quit thinking in band-aides and solve this properly, 1 logical device means it gets 1 driver. This may be an approach which means some more work then others, but I believe in the end that doing it right is worth the effort. Clearly, we agree about doing it right is worth the effort. The whole discussion right now is about what is right. I'm sorry but I don't get the feeling that the discussion currently is focusing on what is right. You are very impatient. To me too much attention is being spend on not throwing away the effort put in the current libgphoto2 camlibs, which I don't like for 2 reasons: 1) It distracts from doing what is right 2) It ignores the fact that a lot has been learned in doing those camlibs, really really a lot. and all that can be re-used in a kernel driver. Note that your two items can contradict or cancel each other out if one is not careful? Let me try to phrase it in a way I think you'll understand. If we agree on doing it right over all other things (such as the fact that doing it right may take a considerable effort). Then this could be an interesting assignment for some of the computer science students I used to be a lecturer for. This assignment could read something like Given the existing situation and knowledge describe all that here, do a re-design for the driverstack for these dual mode cameras, assuming a completely fresh start. Now if I were to give this assignment to a group of students, and they would keep coming back with the but re-doing the camlibs in kernelspace is such a large effort, and we already have them in userspace argument against using one unified driver for these devices, I would give them an F, because they are clearly missing the assuming a completely fresh start part of the assignment. Well, for one thing, Hans, we do not have here any instructor who is giving us an assignment. And nobody is in the position to specify that the assignment says assuming a completely fresh start -- unless Linus happens to be reading this thread and chimes in. Otherwise, unless there is a convincing demonstration that assuming a completely fresh start is an absolute and unavoidable necessity, someone is probably going to disagree. I'm sorry if this sounds a bit harsh, Yes, I am sorry about that, too. but this is the way how the current discussion feels to me. If we agree on aiming for doing it right then with that comes to me doing a software design from scratch, so without taking into account what is already there. Here, a counter-argument is to point out, as I did in a mail earlier this afternoon, that without taking account what is already there might possibly let one overlook something important. And, no, I am not referring to the userspace-kernelspace problem with this. I am referring to the fact that simply to dump the entire contents of the camera into cache (and to keep it there for quite a while) might not necessarily be a good idea and it had been quite consciously rejected to do that in the design of libgphoto2. Not because it is in userspace, but because to do that eats up and ties up RAM of which one cannot assume there is a surplus. Do not misunderstand, though. I am not even going so far as to say that libgphoto2 made the right decision. It certainly has its drawbacks, in that it places severe requirements on someone programming a driver for a really stupid camera. But what I *am* saying is that the issue was anticipated, the issue was faced, and a conscious decision was made. This is the opposite of not anticipating, not facing an issue, and not making any conscious decision. Oh, another example of such lack of deep thought has produced the current crisis, too. I am referring to the amazing decision of some user interface
Re: USB mini-summit at LinuxCon Vancouver
On Wed, Aug 10, 2011 at 4:57 AM, Hans de Goede hdego...@redhat.com wrote: This has been discussed over and over and over again, playing clever tricks with USBDEVFS_[DIS]CONNECT like adding a new USBDEVFS_TRYDISCONNECT which the v4l2 driver could intercept won't cut it. We need some central manager of the device doing multiplexing between the 2 functions, and you can *not* assume that either side will be nice wrt closing file descriptors. So it seems to be both good and bad for Linux's ability to do multiplexing between the two drivers (one purpose-built kernel driver and one generic usbfs driver to be used libusb). The good thing is that it is quite useful in many use cases. The ability to detach the existing kernel driver and use usbfs is the base for many libusb based program under Linux and it beats Windows and Mac OS X handsomely in this aspect. On the other hand, based on the discussions, it seems to adds quite some issues/complexities in some other use cases. Windows typically does not have such an issue. You can use only one driver. In order to use libusb-win32 or libusb-1.0 Windows backend, you have to replace the existing kernel driver with libusb-win32 kernel driver (libusb0.sys) or libusb-1.0 Windows backend driver (currently winusb.sys). The exception is the libusb0.sys filter driver. In that case, you use libusb0.sys as the upper filter driver on top of the existing kernel driver. On the other hand, libusb-win32 filter driver is not really widely used due to issues with older versions. All in all what we've today is a kludge, and if we want to provide a seamless user experience we need to fix it. Don't get me wrong usbfs is a really nice solution for driving usb devices from userspace, like scanners and all other sorts of devices. But what all these devices have in common is that they have no kernel driver. Having a userspace based driver and a kernel driver fight it out just does not work well. All in all, I think this is a good summary. There are cases where a kernel driver and a usbfs are both used, an example is FTDI device where ftdi-sio driver and the user space libftdi (based on libusb) are used, but there are problems with that as well with regard to switching between the two drivers. There are a few discussions here. http://libusb.6.n5.nabble.com/Patch-libusb-os-linux-usbfs-c-Distingush-between-usbfs-and-other-kernel-mode-drivers-td3199947.html http://libusb.6.n5.nabble.com/open-device-exclusively-td4524397.html And there are no good solutions to a simple issue as the above mentioned. Not so sure if this is a good suggestion or not: just wondering how the other side (eg Windows) deal with these dual mode cameras? Does Windows use single driver or does Windows use two drivers for these dual mode cameras? Since they are customized device, then there must be a vendor driver for them. And since they will function as a video cam and a still cam device, I assume there will be standard Windows drivers on top of the vendor drivers. So there will be more than one drivers associated with them. -- Xiaofan -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
