Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans de Goede <hdegoede@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

Hi,

Oops, forgot the attachment it is here now...

Regards,

Hans


On 05/05/2012 05:05 PM, Hans de Goede wrote:
> Hi,
>
> On 05/05/2012 04:44 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 05/05/2012 11:14 AM, Hans Verkuil wrote:
>>> So you get:
>>>
>>> vidioc_foo()
>>> lock(mylock)
>>> v4l2_ctrl_s_ctrl(ctrl, val)
>>> s_ctrl(ctrl, val)
>>> lock(mylock)
>>
>> Easy solution here, remove the first lock(mylock), since we are not using v4l2-dev's
>> locking, we are the one doing the first lock, and if we are going to call v4l2_ctrl_s_ctrl
>> we should simply not do that!
>>
>> Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in gspca.c, so
>> we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb locking if
>> gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted simply remove
>> it. Actually I'm thinking about making the jpegqual control part of the gspca_dev struct
>> itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and into
>> the core.
>
> Here is an updated version of this patch implementing this approach for
> vidioc_g/s_jpegcomp. We may need to do something similar in other places, although I cannot
> think of any such places atm,
>
> Regards,
>
> Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: 0001-gspca-allow-subdrivers-to-use-the-control-framework.patch --]
[-- Type: text/x-patch, Size: 4264 bytes --]

>From eb7eb7c63156c1c040a7fddaeddcf1b1891f0fb7 Mon Sep 17 00:00:00 2001
From: Hans Verkuil <hans.verkuil@cisco.com>
Date: Sat, 28 Apr 2012 17:09:50 +0200
Subject: [PATCH 1/8] gspca: allow subdrivers to use the control framework.

Make the necessary changes to allow subdrivers to use the control framework.
This does not add control event support, that needs more work.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/gspca/gspca.c |   47 ++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index ca5a2b1..dfe2e8a 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -38,6 +38,7 @@
 #include <linux/uaccess.h>
 #include <linux/ktime.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-ctrls.h>
 
 #include "gspca.h"
 
@@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
 
 	/* set the current control values to their default values
 	 * which may have changed in sd_init() */
+	/* does nothing if ctrl_handler == NULL */
+	v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
 	ctrl = gspca_dev->cam.ctrls;
 	if (ctrl != NULL) {
 		for (i = 0;
@@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
 	PDEBUG(D_PROBE, "%s released",
 		video_device_node_name(&gspca_dev->vdev));
 
+	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
 	kfree(gspca_dev->usb_buf);
 	kfree(gspca_dev);
 }
@@ -1771,14 +1775,21 @@ static int vidioc_g_jpegcomp(struct file *file, void *priv,
 
 	if (!gspca_dev->sd_desc->get_jcomp)
 		return -EINVAL;
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
+
+	/* Don't take the usb_lock when using the v4l2-ctrls framework */
+	if (gspca_dev->vdev.ctrl_handler == NULL)
+		if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+			return -ERESTARTSYS;
+
 	gspca_dev->usb_err = 0;
 	if (gspca_dev->present)
 		ret = gspca_dev->sd_desc->get_jcomp(gspca_dev, jpegcomp);
 	else
 		ret = -ENODEV;
-	mutex_unlock(&gspca_dev->usb_lock);
+
+	if (gspca_dev->vdev.ctrl_handler == NULL)
+		mutex_unlock(&gspca_dev->usb_lock);
+
 	return ret;
 }
 
@@ -1790,14 +1801,21 @@ static int vidioc_s_jpegcomp(struct file *file, void *priv,
 
 	if (!gspca_dev->sd_desc->set_jcomp)
 		return -EINVAL;
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
+
+	/* Don't take the usb_lock when using the v4l2-ctrls framework */
+	if (gspca_dev->vdev.ctrl_handler == NULL)
+		if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+			return -ERESTARTSYS;
+
 	gspca_dev->usb_err = 0;
 	if (gspca_dev->present)
 		ret = gspca_dev->sd_desc->set_jcomp(gspca_dev, jpegcomp);
 	else
 		ret = -ENODEV;
-	mutex_unlock(&gspca_dev->usb_lock);
+
+	if (gspca_dev->vdev.ctrl_handler == NULL)
+		mutex_unlock(&gspca_dev->usb_lock);
+
 	return ret;
 }
 
@@ -2347,6 +2365,14 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	gspca_dev->sd_desc = sd_desc;
 	gspca_dev->nbufread = 2;
 	gspca_dev->empty_packet = -1;	/* don't check the empty packets */
+	gspca_dev->vdev = gspca_template;
+	gspca_dev->vdev.parent = &intf->dev;
+	gspca_dev->module = module;
+	gspca_dev->present = 1;
+
+	mutex_init(&gspca_dev->usb_lock);
+	mutex_init(&gspca_dev->queue_lock);
+	init_waitqueue_head(&gspca_dev->wq);
 
 	/* configure the subdriver and initialize the USB device */
 	ret = sd_desc->config(gspca_dev, id);
@@ -2363,15 +2389,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	if (ret)
 		goto out;
 
-	mutex_init(&gspca_dev->usb_lock);
-	mutex_init(&gspca_dev->queue_lock);
-	init_waitqueue_head(&gspca_dev->wq);
-
 	/* init video stuff */
-	memcpy(&gspca_dev->vdev, &gspca_template, sizeof gspca_template);
-	gspca_dev->vdev.parent = &intf->dev;
-	gspca_dev->module = module;
-	gspca_dev->present = 1;
 	ret = video_register_device(&gspca_dev->vdev,
 				  VFL_TYPE_GRABBER,
 				  -1);
@@ -2391,6 +2409,7 @@ out:
 	if (gspca_dev->input_dev)
 		input_unregister_device(gspca_dev->input_dev);
 #endif
+	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
 	kfree(gspca_dev->usb_buf);
 	kfree(gspca_dev);
 	return ret;
-- 
1.7.10