Re: [RFC/PATCH] media: Add stk1160 new driver
On Mon, May 28, 2012 at 7:22 AM, Hans Verkuil hverk...@xs4all.nl wrote: In practice it seems that the easiest approach is not to clean up anything in the disconnect, just take the lock, do the bare minimum necessary for the disconnect, unregister the video nodes, unlock and end with v4l2_device_put(v4l2_dev). It's a suggestion only, but experience has shown that it works well. And as I said, when you get multiple device nodes, then this is the only workable approach. I'm convinced: it's both cleaner and more logical to use v4l2_release instead of video_device release to the final cleanup. OK, the general rule is as follows (many drivers do not follow this correctly, BTW, but this is what should happen): - the filehandle that calls REQBUFS owns the buffers and is the only one that can start/stop streaming and queue/dequeue buffers. and read, poll, etc right? This is until REQBUFS with count == 0 is called, or until the filehandle is closed. Okey. But currently videobuf2 doesn't notify the driver when reqbufs with zero count has been called. So, I have to assume it (aka trouble ahead) or capture the zero count case before/after calling vb2_reqbufs (aka ugly). I humbly think that, if we wan't to enforce this behavior (as part of v4l2 driver semantics) then we should have videobuf2 tell the driver when reqbufs has been called with zero count. You can take a look at pwc which only drops owner on filehandle close, or uvc which captures this from vb2_reqbufs. After looking at uvc, now I wonder is it really ugly? or perhaps it's just ok. v4l2_device is a top-level struct, video_device represents a single device node. For cleanup purposes there isn't much difference between the two if you have only one device node. When you have more, then those differences are much more important. Yes, it's cleaner now. Thanks! Ezequiel. -- 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/PATCH] media: Add stk1160 new driver
On Tue 29 May 2012 14:05:08 Ezequiel Garcia wrote: On Mon, May 28, 2012 at 7:22 AM, Hans Verkuil hverk...@xs4all.nl wrote: In practice it seems that the easiest approach is not to clean up anything in the disconnect, just take the lock, do the bare minimum necessary for the disconnect, unregister the video nodes, unlock and end with v4l2_device_put(v4l2_dev). It's a suggestion only, but experience has shown that it works well. And as I said, when you get multiple device nodes, then this is the only workable approach. I'm convinced: it's both cleaner and more logical to use v4l2_release instead of video_device release to the final cleanup. OK, the general rule is as follows (many drivers do not follow this correctly, BTW, but this is what should happen): - the filehandle that calls REQBUFS owns the buffers and is the only one that can start/stop streaming and queue/dequeue buffers. and read, poll, etc right? Read yes, but anyone can poll. This is until REQBUFS with count == 0 is called, or until the filehandle is closed. Okey. But currently videobuf2 doesn't notify the driver when reqbufs with zero count has been called. So, I have to assume it (aka trouble ahead) or capture the zero count case before/after calling vb2_reqbufs (aka ugly). You just check the count after calling vb2_reqbufs. Nothing ugly about it. I humbly think that, if we wan't to enforce this behavior (as part of v4l2 driver semantics) then we should have videobuf2 tell the driver when reqbufs has been called with zero count. You can take a look at pwc which only drops owner on filehandle close, That's a bug. REQBUFS(0) must drop the owner: you've just released all resources related to streaming, so there is no reason to prevent others from using them. But I suspect 90% of all drivers do this wrong. Not to mention that the old videobuf doesn't handle a count of 0 at all, which is a complete violation of the spec. So an application never knows whether a count of 0 will work. Lovely... The qv4l2 test tool has some really ugly workaround for this :-) At some point I'm going to add a test for this to v4l2-compliance... or uvc which captures this from vb2_reqbufs. After looking at uvc, now I wonder is it really ugly? or perhaps it's just ok. It would be nice if this was somehow integrated into videobuf2, but not everyone liked the idea of vb2 using filehandle information from what I remember. But I didn't push for it very hard. v4l2_device is a top-level struct, video_device represents a single device node. For cleanup purposes there isn't much difference between the two if you have only one device node. When you have more, then those differences are much more important. Yes, it's cleaner now. Thanks! Ezequiel. Your welcome! 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/PATCH] media: Add stk1160 new driver
On Sun May 27 2012 23:20:21 Ezequiel Garcia wrote: Hi Hans, On Sat, May 26, 2012 at 2:05 PM, Hans Verkuil hverk...@xs4all.nl wrote: Always test your driver using the v4l2-compliance test tool that it part of v4l-utils! If it passes that, then your code will be in really good shape! You're right. I'll add v4l2-compliance output in the next patch. On Sat May 26 2012 18:41:00 Ezequiel Garcia wrote: This driver adds support for stk1160 usb bridge as used in some video/audio usb capture devices. It is a complete rewrite of staging/media/easycap driver and it's expected as a future replacement. Signed-off-by: Ezequiel Garcia elezegar...@gmail.com --- As of today testing has been performed using both vlc and mplayer on a gentoo machine, including hot unplug and on-the-fly standard change using a device with 1-cvs and 1-audio output. However more testing is underway with another device and/or another distribution. Alsa sound support is a missing feature (work in progress). As this is my first complete driver, the patch is (obviously) intended as RFC only. Any comments/reviews of *any* kind will be greatly appreciated. --- drivers/media/video/Kconfig |2 + drivers/media/video/Makefile|1 + drivers/media/video/stk1160/Kconfig | 11 + drivers/media/video/stk1160/Makefile|6 + drivers/media/video/stk1160/stk1160-core.c | 399 + drivers/media/video/stk1160/stk1160-i2c.c | 304 ++ drivers/media/video/stk1160/stk1160-reg.h | 78 +++ drivers/media/video/stk1160/stk1160-v4l.c | 846 +++ drivers/media/video/stk1160/stk1160-video.c | 506 drivers/media/video/stk1160/stk1160.h | 183 ++ 10 files changed, 2336 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/stk1160/Kconfig create mode 100644 drivers/media/video/stk1160/Makefile create mode 100644 drivers/media/video/stk1160/stk1160-core.c create mode 100644 drivers/media/video/stk1160/stk1160-i2c.c create mode 100644 drivers/media/video/stk1160/stk1160-reg.h create mode 100644 drivers/media/video/stk1160/stk1160-v4l.c create mode 100644 drivers/media/video/stk1160/stk1160-video.c create mode 100644 drivers/media/video/stk1160/stk1160.h + + /* + * We take the lock just before device registration, + * to prevent someone (probably udev) from opening us + * before we finish initialization + */ + mutex_init(dev-mutex); + mutex_lock(dev-mutex); + + rc = stk1160_video_register(dev); It's usually better to register the video node as the very last thing in probe(). That way when the node appears it's ready for udev to use. No need to lock the mutex in that case. Done. + /* + * Wait until all current v4l2 operation are finished + * then deallocate resources + */ + mutex_lock(dev-mutex); + + /* Since saa7115 is no longer present, it's better to unregister it */ + v4l2_device_unregister_subdev(dev-sd_saa7115); + + stk1160_stop_streaming(dev); + + v4l2_device_disconnect(dev-v4l2_dev); + + /* This way current users can detect device is gone */ + dev-udev = NULL; + + mutex_unlock(dev-mutex); + + stk1160_i2c_unregister(dev); + stk1160_video_unregister(dev); I recommend that you use the same approach as I did in media/radio/dsbr100.c: use the v4l2_dev-release callback to handle the final cleanup. That is a safe method that does all the refcounting for you. I'm sorry but I don't really see the difference. Right now I'm having video_device release function to handle the final cleanup. I don't do the refcounting myself either. See my other comment below. First of all, you can make it work reliably with just the video_device release(). But the advantage of using the release of v4l2_device is that it will also work with USB devices with multiple video/vbi/radio nodes and it allows you to put all the code in disconnect() under the lock. The disconnect call can happen at any time, so it can be hard to get it right. In the code above an application can open the video node right after the unlock and before i2c_unregister (which I would move to the release callback anyway). The clean up from i2c_unregister might cause problems for that new open. In practice it seems that the easiest approach is not to clean up anything in the disconnect, just take the lock, do the bare minimum necessary for the disconnect, unregister the video nodes, unlock and end with v4l2_device_put(v4l2_dev). It's a suggestion only, but experience has shown that it works well. And as I said, when you get multiple device nodes, then this is the only workable approach. Weren't there easycap devices with multiple video inputs? Or is that handled by cycling
Re: [RFC/PATCH] media: Add stk1160 new driver
Hi Hans,
On Sat, May 26, 2012 at 2:05 PM, Hans Verkuil hverk...@xs4all.nl wrote:
Always test your driver using the v4l2-compliance test tool that it part of
v4l-utils! If it passes that, then your code will be in really good shape!
You're right. I'll add v4l2-compliance output in the next patch.
On Sat May 26 2012 18:41:00 Ezequiel Garcia wrote:
This driver adds support for stk1160 usb bridge as used in some
video/audio usb capture devices.
It is a complete rewrite of staging/media/easycap driver and
it's expected as a future replacement.
Signed-off-by: Ezequiel Garcia elezegar...@gmail.com
---
As of today testing has been performed using both vlc and mplayer
on a gentoo machine, including hot unplug and on-the-fly standard
change using a device with 1-cvs and 1-audio output.
However more testing is underway with another device and/or another
distribution.
Alsa sound support is a missing feature (work in progress).
As this is my first complete driver, the patch is (obviously) intended as
RFC only.
Any comments/reviews of *any* kind will be greatly appreciated.
---
drivers/media/video/Kconfig | 2 +
drivers/media/video/Makefile | 1 +
drivers/media/video/stk1160/Kconfig | 11 +
drivers/media/video/stk1160/Makefile | 6 +
drivers/media/video/stk1160/stk1160-core.c | 399 +
drivers/media/video/stk1160/stk1160-i2c.c | 304 ++
drivers/media/video/stk1160/stk1160-reg.h | 78 +++
drivers/media/video/stk1160/stk1160-v4l.c | 846
+++
drivers/media/video/stk1160/stk1160-video.c | 506
drivers/media/video/stk1160/stk1160.h | 183 ++
10 files changed, 2336 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/stk1160/Kconfig
create mode 100644 drivers/media/video/stk1160/Makefile
create mode 100644 drivers/media/video/stk1160/stk1160-core.c
create mode 100644 drivers/media/video/stk1160/stk1160-i2c.c
create mode 100644 drivers/media/video/stk1160/stk1160-reg.h
create mode 100644 drivers/media/video/stk1160/stk1160-v4l.c
create mode 100644 drivers/media/video/stk1160/stk1160-video.c
create mode 100644 drivers/media/video/stk1160/stk1160.h
+
+ /*
+ * We take the lock just before device registration,
+ * to prevent someone (probably udev) from opening us
+ * before we finish initialization
+ */
+ mutex_init(dev-mutex);
+ mutex_lock(dev-mutex);
+
+ rc = stk1160_video_register(dev);
It's usually better to register the video node as the very last thing
in probe(). That way when the node appears it's ready for udev to use.
No need to lock the mutex in that case.
Done.
+ /*
+ * Wait until all current v4l2 operation are finished
+ * then deallocate resources
+ */
+ mutex_lock(dev-mutex);
+
+ /* Since saa7115 is no longer present, it's better to unregister it */
+ v4l2_device_unregister_subdev(dev-sd_saa7115);
+
+ stk1160_stop_streaming(dev);
+
+ v4l2_device_disconnect(dev-v4l2_dev);
+
+ /* This way current users can detect device is gone */
+ dev-udev = NULL;
+
+ mutex_unlock(dev-mutex);
+
+ stk1160_i2c_unregister(dev);
+ stk1160_video_unregister(dev);
I recommend that you use the same approach as I did in media/radio/dsbr100.c:
use the v4l2_dev-release callback to handle the final cleanup. That is a safe
method that does all the refcounting for you.
I'm sorry but I don't really see the difference.
Right now I'm having video_device release function to handle the final cleanup.
I don't do the refcounting myself either. See my other comment below.
...
diff --git a/drivers/media/video/stk1160/stk1160-v4l.c
b/drivers/media/video/stk1160/stk1160-v4l.c
new file mode 100644
index 000..a7a012b
--- /dev/null
+++ b/drivers/media/video/stk1160/stk1160-v4l.c
...
+static int stk1160_open(struct file *filp)
+{
+ struct stk1160 *dev = video_drvdata(filp);
+
+ dev-users++;
Why the users field? You shouldn't need it.
Done.
+
+ stk1160_info(opened: users=%d\n, dev-users);
+
+ return v4l2_fh_open(filp);
+}
+
+static int stk1160_close(struct file *file)
+{
+ struct stk1160 *dev = video_drvdata(file);
+
+ dev-users--;
+
+ stk1160_info(closed: users=%d\n, dev-users);
+
+ /*
+ * If this is the last fh remaining open, then we
+ * stop streaming and free/dequeue all buffers.
+ * This prevents device from streaming without
+ * any opened filehandles.
+ */
+ if (v4l2_fh_is_singular_file(file))
+ vb2_queue_release(dev-vb_vidq);
No. You should keep track of which filehandle started streaming (actually
the filehandle that called REQBUFS is the owner of the queue) and release
the queue when that particular filehandle is closed (or it calls REQBUFS
with a count of 0).
Ah. I gave much
Re: [RFC/PATCH] media: Add stk1160 new driver
Hi Sylwester,
On Sat, May 26, 2012 at 4:31 PM, Sylwester Nawrocki snj...@gmail.com wrote:
You can drop this line, it's overwritten with KERNEL_VERSION in v4l2-ioctl.c.
Also I could imagine there might be better names, than dev, for
capabilities.
Yes, indeed. Starting with cap.
+ dev-capabilities =
+ V4L2_CAP_VIDEO_CAPTURE |
+ V4L2_CAP_STREAMING |
+ V4L2_CAP_READWRITE;
+ return 0;
+ f-fmt.pix.width = dev-width;
+ f-fmt.pix.height = dev-height;
+ f-fmt.pix.field = V4L2_FIELD_INTERLACED;
+ f-fmt.pix.pixelformat = dev-fmt-fourcc;
+ f-fmt.pix.bytesperline = dev-width 1;
Probably better to just write: dev-width * 2.
I saw like that in a number of places and I tought it was faster.
Guess, I was being truly naive.
Could be also done as:
*buffers = clamp(*buffers, STK1160_MIN_VIDEO_BUFFERS,
STK1160_MAX_VIDEO_BUFFERS);
Done.
+ /*
+ * videobuf2-vmalloc allocator is context-less so no need to set
+ * alloc_ctxs array.
+ */
+
+ if (v4l_fmt) {
+ stk1160_info(selected format %d (%d)\n,
+ v4l_fmt-fmt.pix.pixelformat,
+ dev-fmt-fourcc);
+ }
This log is not exactly right. You just ignore v4l_fmt, so it is not selected
anywhere. The *v4l_fmt argument is here for ioctls like VIDIOC_CREATE_BUFS,
and in case you wanted to support this ioctl you would need to do something
like:
pix = v4l_fmt-fmt.pix;
sizes[0] = pix-width * pix-height * 2;
Of course with any required sanity checks.
But this driver does not implement vidioc_create_bufs/vidioc_prepare_buf ioctl
callbacks are not, so the code is generally OK.
You're right, that was just legacy code from some early stages.
+static int buffer_prepare(struct vb2_buffer *vb)
+{
+ struct stk1160 *dev = vb2_get_drv_priv(vb-vb2_queue);
+ struct stk1160_buffer *buf =
+ container_of(vb, struct stk1160_buffer, vb);
+
+ /* If the device is disconnected, reject the buffer */
+ if (!dev-udev)
+ return -ENODEV;
+
+ buf-mem = vb2_plane_vaddr(vb, 0);
+ buf-length = vb2_plane_size(vb, 0);
Where do you check if the buffer you get from vb2 has correct parameters
for your hardware (with current settings) to start writing data to it ?
It seems that this driver supports just one pixel format and resolution,
but still would be good to do such checks in buf_prepare().
You mean I should check buf-length?
And initialization of buf-mem, buf-length is better done from
buffer_queue() op. This way there would be no need to take dev-buf_lock
spinlock also in buf_prepare() to protect the driver's buffer (queue),
which isn't done but it should in buffer_prepare() btw.
Yes, I missed this spot.
+ buf-bytesused = 0;
+ buf-pos = 0;
+
+ return 0;
+}
+
+static int buffer_finish(struct vb2_buffer *vb)
+{
+ return 0;
+}
+
+static void buffer_cleanup(struct vb2_buffer *vb)
+{
+}
buf_init, buf_finish, buf_cleanup are optional callbacks, so if you
don't use them they could be removed altogether.
Done.
+
+static void buffer_queue(struct vb2_buffer *vb)
+{
+ unsigned long flags = 0;
+ struct stk1160 *dev = vb2_get_drv_priv(vb-vb2_queue);
+ struct stk1160_buffer *buf =
+ container_of(vb, struct stk1160_buffer, vb);
+
+ spin_lock_irqsave(dev-buf_lock, flags);
+ if (!dev-udev) {
+ /*
+ * If the device is disconnected return the buffer to userspace
+ * directly. The next QBUF call will fail with -ENODEV.
+ */
+ vb2_buffer_done(buf-vb, VB2_BUF_STATE_ERROR);
+ } else {
+ list_add_tail(buf-list,dev-avail_bufs);
+ }
+ spin_unlock_irqrestore(dev-buf_lock, flags);
+}
--
Regards,
Sylwester
Thanks,
Ezequiel.
--
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/PATCH] media: Add stk1160 new driver
Hi Ezequiel,
On 05/27/2012 11:32 PM, Ezequiel Garcia wrote:
+static int buffer_prepare(struct vb2_buffer *vb)
+{
+struct stk1160 *dev = vb2_get_drv_priv(vb-vb2_queue);
+struct stk1160_buffer *buf =
+container_of(vb, struct stk1160_buffer, vb);
+
+/* If the device is disconnected, reject the buffer */
+if (!dev-udev)
+return -ENODEV;
+
+buf-mem = vb2_plane_vaddr(vb, 0);
+buf-length = vb2_plane_size(vb, 0);
Where do you check if the buffer you get from vb2 has correct parameters
for your hardware (with current settings) to start writing data to it ?
It seems that this driver supports just one pixel format and resolution,
but still would be good to do such checks in buf_prepare().
You mean I should check buf-length?
Yeah, you should validate the passed in vb2_buffer. Here is some example:
http://lxr.linux.no/#linux+v3.4/drivers/media/video/mx2_emmaprp.c#L715
IOW, you should compare value returned from vb2_plane_vaddr(vb, 0) during
streaming with value computed from pixel format and resolution -
configured with S_FMT. Rather than accepting any buffer passed to the driver
from user space.
Regards,
Sylwester
--
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/PATCH] media: Add stk1160 new driver
(Ezequiel, your original CC list was mangled, so I'm reposting this)
Thanks!
Here is a quick run through of the code.
Always test your driver using the v4l2-compliance test tool that it part of
v4l-utils! If it passes that, then your code will be in really good shape!
On Sat May 26 2012 18:41:00 Ezequiel Garcia wrote:
This driver adds support for stk1160 usb bridge as used in some
video/audio usb capture devices.
It is a complete rewrite of staging/media/easycap driver and
it's expected as a future replacement.
Signed-off-by: Ezequiel Garcia elezegar...@gmail.com
---
As of today testing has been performed using both vlc and mplayer
on a gentoo machine, including hot unplug and on-the-fly standard
change using a device with 1-cvs and 1-audio output.
However more testing is underway with another device and/or another
distribution.
Alsa sound support is a missing feature (work in progress).
As this is my first complete driver, the patch is (obviously) intended as RFC
only.
Any comments/reviews of *any* kind will be greatly appreciated.
---
drivers/media/video/Kconfig |2 +
drivers/media/video/Makefile|1 +
drivers/media/video/stk1160/Kconfig | 11 +
drivers/media/video/stk1160/Makefile|6 +
drivers/media/video/stk1160/stk1160-core.c | 399 +
drivers/media/video/stk1160/stk1160-i2c.c | 304 ++
drivers/media/video/stk1160/stk1160-reg.h | 78 +++
drivers/media/video/stk1160/stk1160-v4l.c | 846
+++
drivers/media/video/stk1160/stk1160-video.c | 506
drivers/media/video/stk1160/stk1160.h | 183 ++
10 files changed, 2336 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/stk1160/Kconfig
create mode 100644 drivers/media/video/stk1160/Makefile
create mode 100644 drivers/media/video/stk1160/stk1160-core.c
create mode 100644 drivers/media/video/stk1160/stk1160-i2c.c
create mode 100644 drivers/media/video/stk1160/stk1160-reg.h
create mode 100644 drivers/media/video/stk1160/stk1160-v4l.c
create mode 100644 drivers/media/video/stk1160/stk1160-video.c
create mode 100644 drivers/media/video/stk1160/stk1160.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 99937c9..8d94d56 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -661,6 +661,8 @@ source drivers/media/video/hdpvr/Kconfig
source drivers/media/video/em28xx/Kconfig
+source drivers/media/video/stk1160/Kconfig
+
source drivers/media/video/tlg2300/Kconfig
source drivers/media/video/cx231xx/Kconfig
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index d209de0..7698b25 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -125,6 +125,7 @@ obj-$(CONFIG_VIDEO_HEXIUM_ORION) += hexium_orion.o
obj-$(CONFIG_VIDEO_HEXIUM_GEMINI) += hexium_gemini.o
obj-$(CONFIG_STA2X11_VIP) += sta2x11_vip.o
obj-$(CONFIG_VIDEO_TIMBERDALE) += timblogiw.o
+obj-$(CONFIG_VIDEO_STK1160) += stk1160/
obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o
obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o
diff --git a/drivers/media/video/stk1160/Kconfig
b/drivers/media/video/stk1160/Kconfig
new file mode 100644
index 000..d481b8e
--- /dev/null
+++ b/drivers/media/video/stk1160/Kconfig
@@ -0,0 +1,11 @@
+config VIDEO_STK1160
+ tristate STK1160 USB video capture support
+ depends on VIDEO_DEV I2C EASYCAP!=m EASYCAP!=y
+ select VIDEOBUF2_VMALLOC
+ select VIDEO_SAA711X if VIDEO_HELPER_CHIPS_AUTO
+
+ ---help---
+ This is a video4linux driver for STK1160 based video capture devices
+
+ To compile this driver as a module, choose M here: the
+ module will be called stk1160
diff --git a/drivers/media/video/stk1160/Makefile
b/drivers/media/video/stk1160/Makefile
new file mode 100644
index 000..30ca209
--- /dev/null
+++ b/drivers/media/video/stk1160/Makefile
@@ -0,0 +1,6 @@
+stk1160-y := stk1160-core.o stk1160-v4l.o stk1160-video.o stk1160-i2c.o
+
+obj-$(CONFIG_VIDEO_STK1160) += stk1160.o
+
+ccflags-y += -Wall
+ccflags-y += -Idrivers/media/video
diff --git a/drivers/media/video/stk1160/stk1160-core.c
b/drivers/media/video/stk1160/stk1160-core.c
new file mode 100644
index 000..7fdee6b
--- /dev/null
...
+++ b/drivers/media/video/stk1160/stk1160-core.c
+static int stk1160_probe(struct usb_interface *interface,
+ const struct usb_device_id *id)
+{
+ int ifnum;
+ int rc = 0;
+
+ unsigned int *alt_max_pkt_size; /* array of wMaxPacketSize */
+ struct usb_device *udev;
+ struct stk1160 *dev = NULL;
+
+ ifnum = interface-altsetting[0].desc.bInterfaceNumber;
+ udev = interface_to_usbdev(interface);
+
+ /* Don't register audio interfaces */
+ if (interface-altsetting[0].desc.bInterfaceClass == USB_CLASS_AUDIO) {
Re: [RFC/PATCH] media: Add stk1160 new driver
Hi Hans, Thanks for your review (I'm a bit amazed at how fast you went through the code :). I'll address your excellent comments soon. I'm still unsure about a numbre of things. Two of them: 1. It seems to mee tracing is not too nice and I wasn't really sure how to handle it: dev_xxx, pr_xxx, v4l2_xxx. What's the current trend? 2. The original driver allowed to set frame size, but it seemed to me that could be done at userspace. Hence, my implementation says: V4L2_STD_625_50 is 720x756 and V4L2_STD_525_60 is 720x480. (This is related to the way the video decoder saa711x also assuming that sizes.) So userspace is supposed to get frame size, right after changing video standard and handle buffer of appropriate size. What do you think? On Sat, May 26, 2012 at 2:50 PM, Hans Verkuil hverk...@xs4all.nl wrote: (Ezequiel, your original CC list was mangled, so I'm reposting this) Sorry about this :( I'll check my git-send-mail config. Thanks again, Ezequiel. -- 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/PATCH] media: Add stk1160 new driver
Hi Ezequiel,
On 05/26/2012 07:50 PM, Hans Verkuil wrote:
+/*
+ * IOCTL vidioc handling
+ */
+static int vidioc_reqbufs(struct file *file, void *priv,
+ struct v4l2_requestbuffers *p)
+{
+struct stk1160 *dev = video_drvdata(file);
+return vb2_reqbufs(dev-vb_vidq, p);
+}
+
+static int vidioc_querybuf(struct file *file, void *priv, struct
v4l2_buffer *p)
+{
+struct stk1160 *dev = video_drvdata(file);
+return vb2_querybuf(dev-vb_vidq, p);
+}
+
+static int vidioc_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
+{
+struct stk1160 *dev = video_drvdata(file);
+return vb2_qbuf(dev-vb_vidq, p);
+}
+
+static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer
*p)
+{
+struct stk1160 *dev = video_drvdata(file);
+return vb2_dqbuf(dev-vb_vidq, p, file-f_flags O_NONBLOCK);
+}
+
+static int vidioc_streamon(struct file *file, void *priv, enum
v4l2_buf_type i)
+{
+struct stk1160 *dev = video_drvdata(file);
+return vb2_streamon(dev-vb_vidq, i);
+}
+
+static int vidioc_streamoff(struct file *file, void *priv, enum
v4l2_buf_type i)
+{
+struct stk1160 *dev = video_drvdata(file);
+return vb2_streamoff(dev-vb_vidq, i);
+}
+
+static int vidioc_querycap(struct file *file,
+void *priv, struct v4l2_capability *dev)
+{
+strcpy(dev-driver, stk1160);
+strcpy(dev-card, stk1160);
+dev-version = STK1160_VERSION_NUM;
You can drop this line, it's overwritten with KERNEL_VERSION in v4l2-ioctl.c.
Also I could imagine there might be better names, than dev, for capabilities.
+dev-capabilities =
+V4L2_CAP_VIDEO_CAPTURE |
+V4L2_CAP_STREAMING |
+V4L2_CAP_READWRITE;
+return 0;
+}
+
+static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
+struct v4l2_fmtdesc *f)
+{
+if (f-index != 0)
+return -EINVAL;
+
+strlcpy(f-description, format[f-index].name, sizeof(f-description));
+f-pixelformat = format[f-index].fourcc;
+return 0;
+}
+
+static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
+struct v4l2_format *f)
+{
+struct stk1160 *dev = video_drvdata(file);
+
+f-fmt.pix.width = dev-width;
+f-fmt.pix.height = dev-height;
+f-fmt.pix.field = V4L2_FIELD_INTERLACED;
+f-fmt.pix.pixelformat = dev-fmt-fourcc;
+f-fmt.pix.bytesperline = dev-width 1;
Probably better to just write: dev-width * 2.
+f-fmt.pix.sizeimage = dev-height * f-fmt.pix.bytesperline;
+f-fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
+
+return 0;
+}
+
+static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
+struct v4l2_format *f)
+{
+struct stk1160 *dev = video_drvdata(file);
+
+if (f-fmt.pix.pixelformat != format[0].fourcc) {
I would rename format[] to stk1160_formats[], but it might just be me.
+stk1160_err(fourcc format 0x%08x invalid\n,
+f-fmt.pix.pixelformat);
+return -EINVAL;
+}
+
+/*
+ * User can't choose size at his own will,
+ * so we just return him the current size chosen
+ * at standard selection.
+ * TODO: Implement frame scaling?
+ */
+
+f-fmt.pix.width = dev-width;
+f-fmt.pix.height = dev-height;
+f-fmt.pix.field = V4L2_FIELD_INTERLACED;
+f-fmt.pix.bytesperline = dev-width 1;
dev-width * 2;
+f-fmt.pix.sizeimage = dev-height * f-fmt.pix.bytesperline;
+f-fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
+
+return 0;
+}
...
+/*
+ * Videobuf2 operations
+ */
+static int queue_setup(struct vb2_queue *vq, const struct v4l2_format
*v4l_fmt,
+unsigned int *nbuffers, unsigned int *nplanes,
+unsigned int sizes[], void *alloc_ctxs[])
+{
+struct stk1160 *dev = vb2_get_drv_priv(vq);
+unsigned long size;
+
+size = dev-width * dev-height * 2;
+
+/*
+ * Here we can change the number of buffers being requested.
+ * For instance, we could set a minimum and a maximum,
+ * like this:
+ */
+if (*nbuffers STK1160_MIN_VIDEO_BUFFERS)
+*nbuffers = STK1160_MIN_VIDEO_BUFFERS;
+else if (*nbuffers STK1160_MAX_VIDEO_BUFFERS)
+*nbuffers = STK1160_MAX_VIDEO_BUFFERS;
Could be also done as:
*buffers = clamp(*buffers, STK1160_MIN_VIDEO_BUFFERS,
STK1160_MAX_VIDEO_BUFFERS);
+
+/* This means a packed colorformat */
+*nplanes = 1;
+
+sizes[0] = size;
+
+/*
+ * videobuf2-vmalloc allocator is context-less so no need to set
+ * alloc_ctxs array.
+ */
+
+if (v4l_fmt) {
+stk1160_info(selected format %d (%d)\n,
+v4l_fmt-fmt.pix.pixelformat,
+
