[PATCH 1/4] gspca_pac7302: correct register documentation

linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 1/4] gspca_pac7302: correct register documentation
@ 2012-09-16 16:00 Frank Schäfer
  2012-09-16 16:00 ` [PATCH 2/4] gspca_pac7302: use registers 0x01 and 0x03 for red and blue balance controls Frank Schäfer
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Frank Schäfer @ 2012-09-16 16:00 UTC (permalink / raw)
  To: hdegoede; +Cc: linux-media, Frank Schäfer

R,G,B balance registers are 0x01-0x03 instead of 0x02-0x04,
which lead to the wrong conclusion that values are inverted.
Exposure is controlled via page 3 registers and this is already documented.
Also fix a whitespace issue.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/gspca/pac7302.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/gspca/pac7302.c b/drivers/media/usb/gspca/pac7302.c
index 2d5c6d83..4894ac1 100644
--- a/drivers/media/usb/gspca/pac7302.c
+++ b/drivers/media/usb/gspca/pac7302.c
@@ -29,14 +29,13 @@
  * Register page 0:
  *
  * Address	Description
- * 0x02		Red balance control
- * 0x03		Green balance control
- * 0x04 	Blue balance control
- *		     Valus are inverted (0=max, 255=min).
+ * 0x01		Red balance control
+ * 0x02		Green balance control
+ * 0x03		Blue balance control
  *		     The Windows driver uses a quadratic approach to map
  *		     the settable values (0-200) on register values:
- *		     min=0x80, default=0x40, max=0x20
- * 0x0f-0x20	Colors, saturation and exposure control
+ *		     min=0x20, default=0x40, max=0x80
+ * 0x0f-0x20	Color and saturation control
  * 0xa2-0xab	Brightness, contrast and gamma control
  * 0xb6		Sharpness control (bits 0-4)
  *
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] gspca_pac7302: use registers 0x01 and 0x03 for red and blue balance controls
  2012-09-16 16:00 [PATCH 1/4] gspca_pac7302: correct register documentation Frank Schäfer
@ 2012-09-16 16:00 ` Frank Schäfer
  2012-09-16 16:00 ` [PATCH 3/4] v4l2-ctrl: add a control for green balance Frank Schäfer
  2012-09-16 16:00 ` [PATCH 4/4] gspca_pac7302: add support for green balance adjustment Frank Schäfer
  2 siblings, 0 replies; 13+ messages in thread
From: Frank Schäfer @ 2012-09-16 16:00 UTC (permalink / raw)
  To: hdegoede; +Cc: linux-media, Frank Schäfer

Currently used registers 0xc5 and 0xc7 provide only a very coarse
adjustment possibility within a very small value range (0-3).
With registers 0x01 and 0x03, a fine grained adjustment with
255 steps is possible. This is also what the Windows driver does.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/gspca/pac7302.c |   51 +++++++++++++++++++++++++++++--------
 1 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/drivers/media/usb/gspca/pac7302.c b/drivers/media/usb/gspca/pac7302.c
index 4894ac1..8a0f4d6 100644
--- a/drivers/media/usb/gspca/pac7302.c
+++ b/drivers/media/usb/gspca/pac7302.c
@@ -77,12 +77,12 @@
  *
  * Page | Register   | Function
  * -----+------------+---------------------------------------------------
+ *  0   | 0x01       | setredbalance()
+ *  0   | 0x03       | setbluebalance()
  *  0   | 0x0f..0x20 | setcolors()
  *  0   | 0xa2..0xab | setbrightcont()
  *  0   | 0xb6       | setsharpness()
- *  0   | 0xc5       | setredbalance()
  *  0   | 0xc6       | setwhitebalance()
- *  0   | 0xc7       | setbluebalance()
  *  0   | 0xdc       | setbrightcont(), setcolors()
  *  3   | 0x02       | setexposure()
  *  3   | 0x10, 0x12 | setgain()
@@ -98,10 +98,13 @@
 /* Include pac common sof detection functions */
 #include "pac_common.h"
 
-#define PAC7302_GAIN_DEFAULT      15
-#define PAC7302_GAIN_KNEE         42
-#define PAC7302_EXPOSURE_DEFAULT  66 /* 33 ms / 30 fps */
-#define PAC7302_EXPOSURE_KNEE    133 /* 66 ms / 15 fps */
+#define PAC7302_RGB_BALANCE_MIN		  0
+#define PAC7302_RGB_BALANCE_MAX		200
+#define PAC7302_RGB_BALANCE_DEFAULT	100
+#define PAC7302_GAIN_DEFAULT		 15
+#define PAC7302_GAIN_KNEE 		 42
+#define PAC7302_EXPOSURE_DEFAULT	 66 /* 33 ms / 30 fps */
+#define PAC7302_EXPOSURE_KNEE		133 /* 66 ms / 15 fps */
 
 MODULE_AUTHOR("Jean-Francois Moine <http://moinejf.free.fr>, "
 		"Thomas Kaiser thomas@kaiser-linux.li");
@@ -438,12 +441,31 @@ static void setwhitebalance(struct gspca_dev *gspca_dev)
 	reg_w(gspca_dev, 0xdc, 0x01);
 }
 
+static u8 rgbbalance_ctrl_to_reg_value(s32 rgb_ctrl_val)
+{
+	const unsigned int k = 1000;	/* precision factor */
+	unsigned int norm;
+
+	/* Normed value [0...k] */
+	norm = k * (rgb_ctrl_val - PAC7302_RGB_BALANCE_MIN)
+		    / (PAC7302_RGB_BALANCE_MAX - PAC7302_RGB_BALANCE_MIN);
+	/* Qudratic apporach improves control at small (register) values: */
+	return 64 * norm * norm / (k*k)  +  32 * norm / k  +  32;
+	/* Y = 64*X*X + 32*X + 32
+	 * => register values 0x20-0x80; Windows driver uses these limits */
+
+	/* NOTE: for full value range (0x00-0xff) use
+	 *         Y = 254*X*X + X
+	 *         => 254 * norm * norm / (k*k)  +  1 * norm / k	*/
+}
+
 static void setredbalance(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 
-	reg_w(gspca_dev, 0xff, 0x00);		/* page 0 */
-	reg_w(gspca_dev, 0xc5, sd->red_balance->val);
+	reg_w(gspca_dev, 0xff, 0x00);			/* page 0 */
+	reg_w(gspca_dev, 0x01,
+	      rgbbalance_ctrl_to_reg_value(sd->red_balance->val));
 
 	reg_w(gspca_dev, 0xdc, 0x01);
 }
@@ -453,7 +475,8 @@ static void setbluebalance(struct gspca_dev *gspca_dev)
 	struct sd *sd = (struct sd *) gspca_dev;
 
 	reg_w(gspca_dev, 0xff, 0x00);			/* page 0 */
-	reg_w(gspca_dev, 0xc7, sd->blue_balance->val);
+	reg_w(gspca_dev, 0x03,
+	      rgbbalance_ctrl_to_reg_value(sd->blue_balance->val));
 
 	reg_w(gspca_dev, 0xdc, 0x01);
 }
@@ -642,9 +665,15 @@ static int sd_init_controls(struct gspca_dev *gspca_dev)
 					V4L2_CID_WHITE_BALANCE_TEMPERATURE,
 					0, 255, 1, 55);
 	sd->red_balance = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
-					V4L2_CID_RED_BALANCE, 0, 3, 1, 1);
+					V4L2_CID_RED_BALANCE,
+					PAC7302_RGB_BALANCE_MIN,
+					PAC7302_RGB_BALANCE_MAX,
+					1, PAC7302_RGB_BALANCE_DEFAULT);
 	sd->blue_balance = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
-					V4L2_CID_BLUE_BALANCE, 0, 3, 1, 1);
+					V4L2_CID_BLUE_BALANCE,
+					PAC7302_RGB_BALANCE_MIN,
+					PAC7302_RGB_BALANCE_MAX,
+					1, PAC7302_RGB_BALANCE_DEFAULT);
 
 	gspca_dev->autogain = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
 					V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] v4l2-ctrl: add a control for green balance
  2012-09-16 16:00 [PATCH 1/4] gspca_pac7302: correct register documentation Frank Schäfer
  2012-09-16 16:00 ` [PATCH 2/4] gspca_pac7302: use registers 0x01 and 0x03 for red and blue balance controls Frank Schäfer
@ 2012-09-16 16:00 ` Frank Schäfer
  2012-09-16 16:00 ` [PATCH 4/4] gspca_pac7302: add support for green balance adjustment Frank Schäfer
  2 siblings, 0 replies; 13+ messages in thread
From: Frank Schäfer @ 2012-09-16 16:00 UTC (permalink / raw)
  To: hdegoede; +Cc: linux-media, Frank Schäfer

We already support the red balance (V4L2_CID_RED_BALANCE) and
blue balance (V4L2_CID_BLUE_BALANCE) controls and lots of hardware
provides a possibility to adjust the green balance, too.
Several drivers already support this as custom controls, other just
don't do that due to the lack of a V4L2 standard control.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 Documentation/DocBook/media/v4l/controls.xml |    5 +++++
 drivers/media/v4l2-core/v4l2-ctrls.c         |    2 ++
 include/linux/videodev2.h                    |    4 +++-
 3 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index 272a5f7..dbb3b61 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -162,6 +162,11 @@ activated, keeps adjusting the white balance.</entry>
 	    <entry>Red chroma balance.</entry>
 	  </row>
 	  <row>
+	    <entry><constant>V4L2_CID_GREEN_BALANCE</constant></entry>
+	    <entry>integer</entry>
+	    <entry>Green chroma balance.</entry>
+	  </row>
+	  <row>
 	    <entry><constant>V4L2_CID_BLUE_BALANCE</constant></entry>
 	    <entry>integer</entry>
 	    <entry>Blue chroma balance.</entry>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index ab287f2..39b9bb8 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -575,6 +575,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:	return "Min Number of Output Buffers";
 	case V4L2_CID_ALPHA_COMPONENT:		return "Alpha Component";
 	case V4L2_CID_COLORFX_CBCR:		return "Color Effects, CbCr";
+	case V4L2_CID_GREEN_BALANCE:		return "Green Balance";
 
 	/* MPEG controls */
 	/* Keep the order of the 'case's the same as in videodev2.h! */
@@ -941,6 +942,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_SATURATION:
 	case V4L2_CID_HUE:
 	case V4L2_CID_RED_BALANCE:
+	case V4L2_CID_GREEN_BALANCE:
 	case V4L2_CID_BLUE_BALANCE:
 	case V4L2_CID_GAMMA:
 	case V4L2_CID_SHARPNESS:
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 4862165..72354ad 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1390,8 +1390,10 @@ enum v4l2_colorfx {
 #define V4L2_CID_ALPHA_COMPONENT		(V4L2_CID_BASE+41)
 #define V4L2_CID_COLORFX_CBCR			(V4L2_CID_BASE+42)
 
+#define V4L2_CID_GREEN_BALANCE			(V4L2_CID_BASE+43)
+
 /* last CID + 1 */
-#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+43)
+#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
 
 /*  MPEG-class control IDs defined by V4L2 */
 #define V4L2_CID_MPEG_BASE 			(V4L2_CTRL_CLASS_MPEG | 0x900)
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] gspca_pac7302: add support for green balance adjustment
  2012-09-16 16:00 [PATCH 1/4] gspca_pac7302: correct register documentation Frank Schäfer
  2012-09-16 16:00 ` [PATCH 2/4] gspca_pac7302: use registers 0x01 and 0x03 for red and blue balance controls Frank Schäfer
  2012-09-16 16:00 ` [PATCH 3/4] v4l2-ctrl: add a control for green balance Frank Schäfer
@ 2012-09-16 16:00 ` Frank Schäfer
  2012-09-19 17:25   ` Frank Schäfer
  2 siblings, 1 reply; 13+ messages in thread
From: Frank Schäfer @ 2012-09-16 16:00 UTC (permalink / raw)
  To: hdegoede; +Cc: linux-media, Frank Schäfer

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/gspca/pac7302.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/drivers/media/usb/gspca/pac7302.c b/drivers/media/usb/gspca/pac7302.c
index 8a0f4d6..9b62b74 100644
--- a/drivers/media/usb/gspca/pac7302.c
+++ b/drivers/media/usb/gspca/pac7302.c
@@ -78,6 +78,7 @@
  * Page | Register   | Function
  * -----+------------+---------------------------------------------------
  *  0   | 0x01       | setredbalance()
+ *  0   | 0x02       | setgreenbalance()
  *  0   | 0x03       | setbluebalance()
  *  0   | 0x0f..0x20 | setcolors()
  *  0   | 0xa2..0xab | setbrightcont()
@@ -121,6 +122,7 @@ struct sd {
 	struct v4l2_ctrl *saturation;
 	struct v4l2_ctrl *white_balance;
 	struct v4l2_ctrl *red_balance;
+	struct v4l2_ctrl *green_balance;
 	struct v4l2_ctrl *blue_balance;
 	struct { /* flip cluster */
 		struct v4l2_ctrl *hflip;
@@ -470,6 +472,17 @@ static void setredbalance(struct gspca_dev *gspca_dev)
 	reg_w(gspca_dev, 0xdc, 0x01);
 }
 
+static void setgreenbalance(struct gspca_dev *gspca_dev)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+
+	reg_w(gspca_dev, 0xff, 0x00);			/* page 0 */
+	reg_w(gspca_dev, 0x02,
+	      rgbbalance_ctrl_to_reg_value(sd->green_balance->val));
+
+	reg_w(gspca_dev, 0xdc, 0x01);
+}
+
 static void setbluebalance(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
@@ -620,6 +633,9 @@ static int sd_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_RED_BALANCE:
 		setredbalance(gspca_dev);
 		break;
+	case V4L2_CID_GREEN_BALANCE:
+		setgreenbalance(gspca_dev);
+		break;
 	case V4L2_CID_BLUE_BALANCE:
 		setbluebalance(gspca_dev);
 		break;
@@ -652,7 +668,7 @@ static int sd_init_controls(struct gspca_dev *gspca_dev)
 	struct v4l2_ctrl_handler *hdl = &gspca_dev->ctrl_handler;
 
 	gspca_dev->vdev.ctrl_handler = hdl;
-	v4l2_ctrl_handler_init(hdl, 12);
+	v4l2_ctrl_handler_init(hdl, 13);
 
 	sd->brightness = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
 					V4L2_CID_BRIGHTNESS, 0, 32, 1, 16);
@@ -669,6 +685,11 @@ static int sd_init_controls(struct gspca_dev *gspca_dev)
 					PAC7302_RGB_BALANCE_MIN,
 					PAC7302_RGB_BALANCE_MAX,
 					1, PAC7302_RGB_BALANCE_DEFAULT);
+	sd->green_balance = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+					V4L2_CID_GREEN_BALANCE,
+					PAC7302_RGB_BALANCE_MIN,
+					PAC7302_RGB_BALANCE_MAX,
+					1, PAC7302_RGB_BALANCE_DEFAULT);
 	sd->blue_balance = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
 					V4L2_CID_BLUE_BALANCE,
 					PAC7302_RGB_BALANCE_MIN,
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] gspca_pac7302: add support for green balance adjustment
  2012-09-16 16:00 ` [PATCH 4/4] gspca_pac7302: add support for green balance adjustment Frank Schäfer
@ 2012-09-19 17:25   ` Frank Schäfer
  2012-09-19 20:34     ` Hans de Goede
  0 siblings, 1 reply; 13+ messages in thread
From: Frank Schäfer @ 2012-09-19 17:25 UTC (permalink / raw)
  To: Hans de Goede; +Cc: linux-media

Am 16.09.2012 18:00, schrieb Frank Schäfer:
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/usb/gspca/pac7302.c |   23 ++++++++++++++++++++++-
>  1 files changed, 22 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/usb/gspca/pac7302.c b/drivers/media/usb/gspca/pac7302.c
> index 8a0f4d6..9b62b74 100644
> --- a/drivers/media/usb/gspca/pac7302.c
> +++ b/drivers/media/usb/gspca/pac7302.c
> @@ -78,6 +78,7 @@
>   * Page | Register   | Function
>   * -----+------------+---------------------------------------------------
>   *  0   | 0x01       | setredbalance()
> + *  0   | 0x02       | setgreenbalance()
>   *  0   | 0x03       | setbluebalance()
>   *  0   | 0x0f..0x20 | setcolors()
>   *  0   | 0xa2..0xab | setbrightcont()
> @@ -121,6 +122,7 @@ struct sd {
>  	struct v4l2_ctrl *saturation;
>  	struct v4l2_ctrl *white_balance;
>  	struct v4l2_ctrl *red_balance;
> +	struct v4l2_ctrl *green_balance;
>  	struct v4l2_ctrl *blue_balance;
>  	struct { /* flip cluster */
>  		struct v4l2_ctrl *hflip;
> @@ -470,6 +472,17 @@ static void setredbalance(struct gspca_dev *gspca_dev)
>  	reg_w(gspca_dev, 0xdc, 0x01);
>  }
>  
> +static void setgreenbalance(struct gspca_dev *gspca_dev)
> +{
> +	struct sd *sd = (struct sd *) gspca_dev;
> +
> +	reg_w(gspca_dev, 0xff, 0x00);			/* page 0 */
> +	reg_w(gspca_dev, 0x02,
> +	      rgbbalance_ctrl_to_reg_value(sd->green_balance->val));
> +
> +	reg_w(gspca_dev, 0xdc, 0x01);
> +}
> +
>  static void setbluebalance(struct gspca_dev *gspca_dev)
>  {
>  	struct sd *sd = (struct sd *) gspca_dev;
> @@ -620,6 +633,9 @@ static int sd_s_ctrl(struct v4l2_ctrl *ctrl)
>  	case V4L2_CID_RED_BALANCE:
>  		setredbalance(gspca_dev);
>  		break;
> +	case V4L2_CID_GREEN_BALANCE:
> +		setgreenbalance(gspca_dev);
> +		break;
>  	case V4L2_CID_BLUE_BALANCE:
>  		setbluebalance(gspca_dev);
>  		break;
> @@ -652,7 +668,7 @@ static int sd_init_controls(struct gspca_dev *gspca_dev)
>  	struct v4l2_ctrl_handler *hdl = &gspca_dev->ctrl_handler;
>  
>  	gspca_dev->vdev.ctrl_handler = hdl;
> -	v4l2_ctrl_handler_init(hdl, 12);
> +	v4l2_ctrl_handler_init(hdl, 13);
>  
>  	sd->brightness = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
>  					V4L2_CID_BRIGHTNESS, 0, 32, 1, 16);
> @@ -669,6 +685,11 @@ static int sd_init_controls(struct gspca_dev *gspca_dev)
>  					PAC7302_RGB_BALANCE_MIN,
>  					PAC7302_RGB_BALANCE_MAX,
>  					1, PAC7302_RGB_BALANCE_DEFAULT);
> +	sd->green_balance = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
> +					V4L2_CID_GREEN_BALANCE,
> +					PAC7302_RGB_BALANCE_MIN,
> +					PAC7302_RGB_BALANCE_MAX,
> +					1, PAC7302_RGB_BALANCE_DEFAULT);
>  	sd->blue_balance = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
>  					V4L2_CID_BLUE_BALANCE,
>  					PAC7302_RGB_BALANCE_MIN,

Hans, it seems like you didn't pick up these patches up yet...
Is there anything wrong with them ?

Regards,
Frank




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] gspca_pac7302: add support for green balance adjustment
  2012-09-19 17:25   ` Frank Schäfer
@ 2012-09-19 20:34     ` Hans de Goede
  2012-09-19 20:54       ` Frank Schäfer
  0 siblings, 1 reply; 13+ messages in thread
From: Hans de Goede @ 2012-09-19 20:34 UTC (permalink / raw)
  To: Frank Schäfer; +Cc: linux-media

Hi,

On 09/19/2012 07:25 PM, Frank Schäfer wrote:
> Am 16.09.2012 18:00, schrieb Frank Schäfer:
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>>   drivers/media/usb/gspca/pac7302.c |   23 ++++++++++++++++++++++-
>>   1 files changed, 22 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/media/usb/gspca/pac7302.c b/drivers/media/usb/gspca/pac7302.c
>> index 8a0f4d6..9b62b74 100644
>> --- a/drivers/media/usb/gspca/pac7302.c
>> +++ b/drivers/media/usb/gspca/pac7302.c
>> @@ -78,6 +78,7 @@
>>    * Page | Register   | Function
>>    * -----+------------+---------------------------------------------------
>>    *  0   | 0x01       | setredbalance()
>> + *  0   | 0x02       | setgreenbalance()
>>    *  0   | 0x03       | setbluebalance()
>>    *  0   | 0x0f..0x20 | setcolors()
>>    *  0   | 0xa2..0xab | setbrightcont()
>> @@ -121,6 +122,7 @@ struct sd {
>>   	struct v4l2_ctrl *saturation;
>>   	struct v4l2_ctrl *white_balance;
>>   	struct v4l2_ctrl *red_balance;
>> +	struct v4l2_ctrl *green_balance;
>>   	struct v4l2_ctrl *blue_balance;
>>   	struct { /* flip cluster */
>>   		struct v4l2_ctrl *hflip;
>> @@ -470,6 +472,17 @@ static void setredbalance(struct gspca_dev *gspca_dev)
>>   	reg_w(gspca_dev, 0xdc, 0x01);
>>   }
>>
>> +static void setgreenbalance(struct gspca_dev *gspca_dev)
>> +{
>> +	struct sd *sd = (struct sd *) gspca_dev;
>> +
>> +	reg_w(gspca_dev, 0xff, 0x00);			/* page 0 */
>> +	reg_w(gspca_dev, 0x02,
>> +	      rgbbalance_ctrl_to_reg_value(sd->green_balance->val));
>> +
>> +	reg_w(gspca_dev, 0xdc, 0x01);
>> +}
>> +
>>   static void setbluebalance(struct gspca_dev *gspca_dev)
>>   {
>>   	struct sd *sd = (struct sd *) gspca_dev;
>> @@ -620,6 +633,9 @@ static int sd_s_ctrl(struct v4l2_ctrl *ctrl)
>>   	case V4L2_CID_RED_BALANCE:
>>   		setredbalance(gspca_dev);
>>   		break;
>> +	case V4L2_CID_GREEN_BALANCE:
>> +		setgreenbalance(gspca_dev);
>> +		break;
>>   	case V4L2_CID_BLUE_BALANCE:
>>   		setbluebalance(gspca_dev);
>>   		break;
>> @@ -652,7 +668,7 @@ static int sd_init_controls(struct gspca_dev *gspca_dev)
>>   	struct v4l2_ctrl_handler *hdl = &gspca_dev->ctrl_handler;
>>
>>   	gspca_dev->vdev.ctrl_handler = hdl;
>> -	v4l2_ctrl_handler_init(hdl, 12);
>> +	v4l2_ctrl_handler_init(hdl, 13);
>>
>>   	sd->brightness = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
>>   					V4L2_CID_BRIGHTNESS, 0, 32, 1, 16);
>> @@ -669,6 +685,11 @@ static int sd_init_controls(struct gspca_dev *gspca_dev)
>>   					PAC7302_RGB_BALANCE_MIN,
>>   					PAC7302_RGB_BALANCE_MAX,
>>   					1, PAC7302_RGB_BALANCE_DEFAULT);
>> +	sd->green_balance = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
>> +					V4L2_CID_GREEN_BALANCE,
>> +					PAC7302_RGB_BALANCE_MIN,
>> +					PAC7302_RGB_BALANCE_MAX,
>> +					1, PAC7302_RGB_BALANCE_DEFAULT);
>>   	sd->blue_balance = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
>>   					V4L2_CID_BLUE_BALANCE,
>>   					PAC7302_RGB_BALANCE_MIN,
>
> Hans, it seems like you didn't pick up these patches up yet...
> Is there anything wrong with them ?

I've somehow completely missed them. Can you resend the entire set please?

Thanks,

Hans

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] gspca_pac7302: add support for green balance adjustment
  2012-09-19 20:34     ` Hans de Goede
@ 2012-09-19 20:54       ` Frank Schäfer
  2012-09-20  9:08         ` Hans de Goede
  0 siblings, 1 reply; 13+ messages in thread
From: Frank Schäfer @ 2012-09-19 20:54 UTC (permalink / raw)
  To: Hans de Goede; +Cc: linux-media

Am 19.09.2012 22:34, schrieb Hans de Goede:
> Hi,
>
> On 09/19/2012 07:25 PM, Frank Schäfer wrote:
>> Am 16.09.2012 18:00, schrieb Frank Schäfer:
>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>> ---
>>>   drivers/media/usb/gspca/pac7302.c |   23 ++++++++++++++++++++++-
>>>   1 files changed, 22 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/gspca/pac7302.c
>>> b/drivers/media/usb/gspca/pac7302.c
>>> index 8a0f4d6..9b62b74 100644
>>> --- a/drivers/media/usb/gspca/pac7302.c
>>> +++ b/drivers/media/usb/gspca/pac7302.c
>>> @@ -78,6 +78,7 @@
>>>    * Page | Register   | Function
>>>    *
>>> -----+------------+---------------------------------------------------
>>>    *  0   | 0x01       | setredbalance()
>>> + *  0   | 0x02       | setgreenbalance()
>>>    *  0   | 0x03       | setbluebalance()
>>>    *  0   | 0x0f..0x20 | setcolors()
>>>    *  0   | 0xa2..0xab | setbrightcont()
>>> @@ -121,6 +122,7 @@ struct sd {
>>>       struct v4l2_ctrl *saturation;
>>>       struct v4l2_ctrl *white_balance;
>>>       struct v4l2_ctrl *red_balance;
>>> +    struct v4l2_ctrl *green_balance;
>>>       struct v4l2_ctrl *blue_balance;
>>>       struct { /* flip cluster */
>>>           struct v4l2_ctrl *hflip;
>>> @@ -470,6 +472,17 @@ static void setredbalance(struct gspca_dev
>>> *gspca_dev)
>>>       reg_w(gspca_dev, 0xdc, 0x01);
>>>   }
>>>
>>> +static void setgreenbalance(struct gspca_dev *gspca_dev)
>>> +{
>>> +    struct sd *sd = (struct sd *) gspca_dev;
>>> +
>>> +    reg_w(gspca_dev, 0xff, 0x00);            /* page 0 */
>>> +    reg_w(gspca_dev, 0x02,
>>> +          rgbbalance_ctrl_to_reg_value(sd->green_balance->val));
>>> +
>>> +    reg_w(gspca_dev, 0xdc, 0x01);
>>> +}
>>> +
>>>   static void setbluebalance(struct gspca_dev *gspca_dev)
>>>   {
>>>       struct sd *sd = (struct sd *) gspca_dev;
>>> @@ -620,6 +633,9 @@ static int sd_s_ctrl(struct v4l2_ctrl *ctrl)
>>>       case V4L2_CID_RED_BALANCE:
>>>           setredbalance(gspca_dev);
>>>           break;
>>> +    case V4L2_CID_GREEN_BALANCE:
>>> +        setgreenbalance(gspca_dev);
>>> +        break;
>>>       case V4L2_CID_BLUE_BALANCE:
>>>           setbluebalance(gspca_dev);
>>>           break;
>>> @@ -652,7 +668,7 @@ static int sd_init_controls(struct gspca_dev
>>> *gspca_dev)
>>>       struct v4l2_ctrl_handler *hdl = &gspca_dev->ctrl_handler;
>>>
>>>       gspca_dev->vdev.ctrl_handler = hdl;
>>> -    v4l2_ctrl_handler_init(hdl, 12);
>>> +    v4l2_ctrl_handler_init(hdl, 13);
>>>
>>>       sd->brightness = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
>>>                       V4L2_CID_BRIGHTNESS, 0, 32, 1, 16);
>>> @@ -669,6 +685,11 @@ static int sd_init_controls(struct gspca_dev
>>> *gspca_dev)
>>>                       PAC7302_RGB_BALANCE_MIN,
>>>                       PAC7302_RGB_BALANCE_MAX,
>>>                       1, PAC7302_RGB_BALANCE_DEFAULT);
>>> +    sd->green_balance = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
>>> +                    V4L2_CID_GREEN_BALANCE,
>>> +                    PAC7302_RGB_BALANCE_MIN,
>>> +                    PAC7302_RGB_BALANCE_MAX,
>>> +                    1, PAC7302_RGB_BALANCE_DEFAULT);
>>>       sd->blue_balance = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
>>>                       V4L2_CID_BLUE_BALANCE,
>>>                       PAC7302_RGB_BALANCE_MIN,
>>
>> Hans, it seems like you didn't pick up these patches up yet...
>> Is there anything wrong with them ?
>
> I've somehow completely missed them. Can you resend the entire set
> please?

No problem, but I can't do that before weekend (I'm currently not at home).
I've sent these 4 patches on last Sunday (16. Sept) evening.
Maybe you can pick them up from patchwork ?
http://patchwork.linuxtv.org/patch/14433/

Regards,
Frank

>
> Thanks,
>
> Hans


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] gspca_pac7302: add support for green balance adjustment
  2012-09-19 20:54       ` Frank Schäfer
@ 2012-09-20  9:08         ` Hans de Goede
  2012-09-20 11:54           ` Frank Schäfer
  0 siblings, 1 reply; 13+ messages in thread
From: Hans de Goede @ 2012-09-20  9:08 UTC (permalink / raw)
  To: Frank Schäfer; +Cc: linux-media

Hi,

On 09/19/2012 10:54 PM, Frank Schäfer wrote:
> Am 19.09.2012 22:34, schrieb Hans de Goede:

<snip>

>>> Hans, it seems like you didn't pick up these patches up yet...
>>> Is there anything wrong with them ?
>>
>> I've somehow completely missed them. Can you resend the entire set
>> please?
>
> No problem, but I can't do that before weekend (I'm currently not at home).
> I've sent these 4 patches on last Sunday (16. Sept) evening.
> Maybe you can pick them up from patchwork ?
> http://patchwork.linuxtv.org/patch/14433/

Ah yes, patchwork, that will work. Unfortunately that only solves the
me having missed the patches problem.

First of all thanks for working on this! I'm afraid you've managed to find
one of the weak spots in the v4l2 API, namely how we deal with RGB gains.

Many webcams have RGB gains, but we don't have standard CID-s for these,
instead we've Blue and Red Balance. This has grown historically because of
the bttv cards which actually have Blue and Red balance controls in hardware,
rather then the usual RGB gain triplet. Various gspca drivers cope with this
in different ways.

If you look at the pac7302 driver before your latest 4 patches it has
a Red and Blue balance control controlling the Red and Blue gain, and a
Whitebalance control, which is not White balance at all, but simply
controls the green gain...

And as said other drivers have similar (albeit usually different) hacks.

At a minimum I would like you to rework your patches to:
1) Not add the new Green balance, and instead modify the existing whitebalance
to control the new green gain you've found. Keeping things as broken as
they are, but not worse; and
2) Try to use both the page 0 reg 0x01 - 0x03 and page 0 reg 0xc5 - 0xc7
at the same time to get a wider ranged control. Maybe 0xc5 - 0xc7 are
simply the most significant bits of a wider ranged gain ?
Note that if you cannot control them both from a single control in such
a way that you get a smooth control range, then lets just fix
0xc5 - 0xc7 at a value of 2 for all 3 and be done with it, but at least
we should try :)

As said the above is the minimum, what I would really like is a discussion
on linux-media about adding proper RGB gain controls for all the cameras
which have these.

Note this brings with itself the question should we export such lowlevel
controls at all ? In some drivers the per color gains are simply all
kept at the same level and controlled as part of the master gain control,
giving it a wider and/or more fine grained range, leading to better autogain
behavior. Given how our sw autowhitebalance works (and that that works
reasonable well), their is not much added value in exporting them separately,
while they do tend to improve the standard gain control when used as part
of it ...

So what we really need is a plan how to deal with these controls, and then
send an RFC for this to the list.

Regards,

Hans

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] gspca_pac7302: add support for green balance adjustment
  2012-09-20  9:08         ` Hans de Goede
@ 2012-09-20 11:54           ` Frank Schäfer
  2012-09-23 13:27             ` Frank Schäfer
  2012-09-24 10:47             ` Hans de Goede
  0 siblings, 2 replies; 13+ messages in thread
From: Frank Schäfer @ 2012-09-20 11:54 UTC (permalink / raw)
  Cc: Hans de Goede, linux-media

Hi,

Am 20.09.2012 11:08, schrieb Hans de Goede:
> Hi,
>
>>>> Hans, it seems like you didn't pick up these patches up yet...
>>>> Is there anything wrong with them ?
>>>
>>> I've somehow completely missed them. Can you resend the entire set
>>> please?
>>
>> No problem, but I can't do that before weekend (I'm currently not at
>> home).
>> I've sent these 4 patches on last Sunday (16. Sept) evening.
>> Maybe you can pick them up from patchwork ?
>> http://patchwork.linuxtv.org/patch/14433/
>
> Ah yes, patchwork, that will work. Unfortunately that only solves the
> me having missed the patches problem.
>
> First of all thanks for working on this! I'm afraid you've managed to
> find
> one of the weak spots in the v4l2 API, namely how we deal with RGB gains.

It seems like this is one of my talents... :(

>
> Many webcams have RGB gains, but we don't have standard CID-s for these,
> instead we've Blue and Red Balance. This has grown historically
> because of
> the bttv cards which actually have Blue and Red balance controls in
> hardware,
> rather then the usual RGB gain triplet. Various gspca drivers cope
> with this
> in different ways.
>
> If you look at the pac7302 driver before your latest 4 patches it has
> a Red and Blue balance control controlling the Red and Blue gain, and a
> Whitebalance control, which is not White balance at all, but simply
> controls the green gain...

Ok, so if I understand you right, red+green+blue balance = white balance.
And because we already have defined red, blue and whitebalance controls
for historical reasons, we don't need green balance ?
Maybe that matches physics, but I don't think it's a sane approach for a
user interface...


>
> And as said other drivers have similar (albeit usually different) hacks.
>
> At a minimum I would like you to rework your patches to:
> 1) Not add the new Green balance, and instead modify the existing
> whitebalance
> to control the new green gain you've found. Keeping things as broken as
> they are, but not worse; and

I prefer waiting for the results of the discussion you are proposing
further down.

> 2) Try to use both the page 0 reg 0x01 - 0x03 and page 0 reg 0xc5 - 0xc7
> at the same time to get a wider ranged control. Maybe 0xc5 - 0xc7 are
> simply the most significant bits of a wider ranged gain ?

I don't think so. The windows driver does not use them.
It even doesn't use the full range of registers 0x01-0x03.
Of course, I have expermiented with the full range and it works, but it
doesn't make much sense to use it.

Experimenting with the device to determine the meaing of unknown
registerts, you will notice, that there are several registert which
affect RGB.
But that doesn't mean that they are suitable for a user control...

> Note that if you cannot control them both from a single control in such
> a way that you get a smooth control range, then lets just fix
> 0xc5 - 0xc7 at a value of 2 for all 3 and be done with it, but at least
> we should try :)

There is no need to fix registers 0xc5 and 0xc7.
The Windows driver sets them to 1, which is exactly the value we are
currently using as default value with the blue and red value controls.

>
> As said the above is the minimum, what I would really like is a
> discussion
> on linux-media about adding proper RGB gain controls for all the cameras
> which have these.
>
> Note this brings with itself the question should we export such lowlevel
> controls at all ? In some drivers the per color gains are simply all
> kept at the same level and controlled as part of the master gain control,
> giving it a wider and/or more fine grained range, leading to better
> autogain
> behavior. Given how our sw autowhitebalance works (and that that works
> reasonable well), their is not much added value in exporting them
> separately,
> while they do tend to improve the standard gain control when used as part
> of it ...

I would say, let the drivers decide how to do things. It also depends on
the hardware design.

Regards,
Frank

>
> So what we really need is a plan how to deal with these controls, and
> then
> send an RFC for this to the list.
>
> Regards,
>
> Hans


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] gspca_pac7302: add support for green balance adjustment
  2012-09-20 11:54           ` Frank Schäfer
@ 2012-09-23 13:27             ` Frank Schäfer
  2012-09-24 10:47             ` Hans de Goede
  1 sibling, 0 replies; 13+ messages in thread
From: Frank Schäfer @ 2012-09-23 13:27 UTC (permalink / raw)
  To: Hans de Goede, linux-media

Hi,

Am 20.09.2012 13:54, schrieb Frank Schäfer:
> Hi,
>
> Am 20.09.2012 11:08, schrieb Hans de Goede:
>> Hi,
>>
>>>>> Hans, it seems like you didn't pick up these patches up yet...
>>>>> Is there anything wrong with them ?
>>>> I've somehow completely missed them. Can you resend the entire set
>>>> please?
>>> No problem, but I can't do that before weekend (I'm currently not at
>>> home).
>>> I've sent these 4 patches on last Sunday (16. Sept) evening.
>>> Maybe you can pick them up from patchwork ?
>>> http://patchwork.linuxtv.org/patch/14433/
>> Ah yes, patchwork, that will work. Unfortunately that only solves the
>> me having missed the patches problem.
>>
>> First of all thanks for working on this! I'm afraid you've managed to
>> find
>> one of the weak spots in the v4l2 API, namely how we deal with RGB gains.
> It seems like this is one of my talents... :(
>
>> Many webcams have RGB gains, but we don't have standard CID-s for these,
>> instead we've Blue and Red Balance. This has grown historically
>> because of
>> the bttv cards which actually have Blue and Red balance controls in
>> hardware,
>> rather then the usual RGB gain triplet. Various gspca drivers cope
>> with this
>> in different ways.
>>
>> If you look at the pac7302 driver before your latest 4 patches it has
>> a Red and Blue balance control controlling the Red and Blue gain, and a
>> Whitebalance control, which is not White balance at all, but simply
>> controls the green gain...
> Ok, so if I understand you right, red+green+blue balance = white balance.
> And because we already have defined red, blue and whitebalance controls
> for historical reasons, we don't need green balance ?
> Maybe that matches physics, but I don't think it's a sane approach for a
> user interface...
>
>
>> And as said other drivers have similar (albeit usually different) hacks.
>>
>> At a minimum I would like you to rework your patches to:
>> 1) Not add the new Green balance, and instead modify the existing
>> whitebalance
>> to control the new green gain you've found. Keeping things as broken as
>> they are, but not worse; and
> I prefer waiting for the results of the discussion you are proposing
> further down.
>
>> 2) Try to use both the page 0 reg 0x01 - 0x03 and page 0 reg 0xc5 - 0xc7
>> at the same time to get a wider ranged control. Maybe 0xc5 - 0xc7 are
>> simply the most significant bits of a wider ranged gain ?
> I don't think so. The windows driver does not use them.
> It even doesn't use the full range of registers 0x01-0x03.
> Of course, I have expermiented with the full range and it works, but it
> doesn't make much sense to use it.
>
> Experimenting with the device to determine the meaing of unknown
> registerts, you will notice, that there are several registert which
> affect RGB.
> But that doesn't mean that they are suitable for a user control...
>
>> Note that if you cannot control them both from a single control in such
>> a way that you get a smooth control range, then lets just fix
>> 0xc5 - 0xc7 at a value of 2 for all 3 and be done with it, but at least
>> we should try :)
> There is no need to fix registers 0xc5 and 0xc7.
> The Windows driver sets them to 1, which is exactly the value we are
> currently using as default value with the blue and red value controls.
>
>> As said the above is the minimum, what I would really like is a
>> discussion
>> on linux-media about adding proper RGB gain controls for all the cameras
>> which have these.
>>
>> Note this brings with itself the question should we export such lowlevel
>> controls at all ? In some drivers the per color gains are simply all
>> kept at the same level and controlled as part of the master gain control,
>> giving it a wider and/or more fine grained range, leading to better
>> autogain
>> behavior. Given how our sw autowhitebalance works (and that that works
>> reasonable well), their is not much added value in exporting them
>> separately,
>> while they do tend to improve the standard gain control when used as part
>> of it ...
> I would say, let the drivers decide how to do things. It also depends on
> the hardware design.
>
> Regards,
> Frank
>
>> So what we really need is a plan how to deal with these controls, and
>> then
>> send an RFC for this to the list.
>>
>> Regards,
>>
>> Hans
>>

Hans, I don't have the bandwidth to go through a long discussion in
which probably nobody is really interested.
So please just drop the last two patches which are related to the
V4L2_CID_GREEN_BALANCE issue.
I documented the registers and behavior of the Windows driver, so if you
one day come to the conclusion that such a control would be usefull, it
can be added easily at any time later.

But I would really like to see the first two patches getting applied for
3.7.
The first one is a documentation fix, the second one clearly improves
the behavior of the red and blue balance control (as explained above).
I will resend both patches.


Regards,
Frank

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] gspca_pac7302: add support for green balance adjustment
  2012-09-20 11:54           ` Frank Schäfer
  2012-09-23 13:27             ` Frank Schäfer
@ 2012-09-24 10:47             ` Hans de Goede
  2012-09-24 13:36               ` Frank Schäfer
  1 sibling, 1 reply; 13+ messages in thread
From: Hans de Goede @ 2012-09-24 10:47 UTC (permalink / raw)
  To: Frank Schäfer; +Cc: linux-media

Hi,

Sorry for the slow response ...

On 09/20/2012 01:54 PM, Frank Schäfer wrote:
> Hi,
>
> Am 20.09.2012 11:08, schrieb Hans de Goede:

<snip>

>> Many webcams have RGB gains, but we don't have standard CID-s for these,
>> instead we've Blue and Red Balance. This has grown historically
>> because of
>> the bttv cards which actually have Blue and Red balance controls in
>> hardware,
>> rather then the usual RGB gain triplet. Various gspca drivers cope
>> with this
>> in different ways.
>>
>> If you look at the pac7302 driver before your latest 4 patches it has
>> a Red and Blue balance control controlling the Red and Blue gain, and a
>> Whitebalance control, which is not White balance at all, but simply
>> controls the green gain...
>
> Ok, so if I understand you right, red+green+blue balance = white balance.
> And because we already have defined red, blue and whitebalance controls
> for historical reasons, we don't need green balance ?
> Maybe that matches physics, but I don't think it's a sane approach for a
> user interface...

No what I was trying to say is that the balance controls are for hardware
which actually has balance controls and not per color gains (such as the
bt87x chips), but they are being abused by many drivers to add support for
per color gains. And then you miss one control. And in the case of the pac7302
driver the "original" route was taken of using whitebalance to control
the green gain. Which is wrong IMHO, but it is what the driver does know.

A proper fix would be to introduce new controls for all 3 gains, and use
those instead of using the balance controls + adding a 3th balance control
being discussed in the thread titled:
"Gain controls in v4l2-ctrl framework".

>> And as said other drivers have similar (albeit usually different) hacks.
>>
>> At a minimum I would like you to rework your patches to:
>> 1) Not add the new Green balance, and instead modify the existing
>> whitebalance
>> to control the new green gain you've found. Keeping things as broken as
>> they are, but not worse; and
>
> I prefer waiting for the results of the discussion you are proposing
> further down.
>

I see in your next mail that you've changed your mind. So I would like to
move forward with this by adding your 2 patches + 1 more patch to also
make the whitebalance control (which really is the green gain control)
use 0x02 rather then 0xc6. To do this we must make sure that 0xc6 has
a proper fixed / default setting. So what does the windows driver use
for this? 1 like with 0xc5 and 0xc7 ?

And can you do a 3th patch to make the whitebalance control control
0x02 rather then 0xc6 like you did for red and blue balance?

Then later on when the "Gain controls in v4l2-ctrl framework" discussion
is done we can change these controls to the new controls.

>> 2) Try to use both the page 0 reg 0x01 - 0x03 and page 0 reg 0xc5 - 0xc7
>> at the same time to get a wider ranged control. Maybe 0xc5 - 0xc7 are
>> simply the most significant bits of a wider ranged gain ?
>
> I don't think so. The windows driver does not use them.
> It even doesn't use the full range of registers 0x01-0x03.
> Of course, I have expermiented with the full range and it works, but it
> doesn't make much sense to use it.
>

Ok.

Regards,

Hans

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] gspca_pac7302: add support for green balance adjustment
  2012-09-24 10:47             ` Hans de Goede
@ 2012-09-24 13:36               ` Frank Schäfer
  2012-09-24 14:07                 ` Hans de Goede
  0 siblings, 1 reply; 13+ messages in thread
From: Frank Schäfer @ 2012-09-24 13:36 UTC (permalink / raw)
  To: Hans de Goede; +Cc: linux-media

Hi Hans,

Am 24.09.2012 12:47, schrieb Hans de Goede:
> Hi,
>
> Sorry for the slow response ...
>
> On 09/20/2012 01:54 PM, Frank Schäfer wrote:
>> Hi,
>>
>> Am 20.09.2012 11:08, schrieb Hans de Goede:
>
> <snip>
>
>>> Many webcams have RGB gains, but we don't have standard CID-s for
>>> these,
>>> instead we've Blue and Red Balance. This has grown historically
>>> because of
>>> the bttv cards which actually have Blue and Red balance controls in
>>> hardware,
>>> rather then the usual RGB gain triplet. Various gspca drivers cope
>>> with this
>>> in different ways.
>>>
>>> If you look at the pac7302 driver before your latest 4 patches it has
>>> a Red and Blue balance control controlling the Red and Blue gain, and a
>>> Whitebalance control, which is not White balance at all, but simply
>>> controls the green gain...
>>
>> Ok, so if I understand you right, red+green+blue balance = white
>> balance.
>> And because we already have defined red, blue and whitebalance controls
>> for historical reasons, we don't need green balance ?
>> Maybe that matches physics, but I don't think it's a sane approach for a
>> user interface...
>
> No what I was trying to say is that the balance controls are for hardware
> which actually has balance controls and not per color gains (such as the
> bt87x chips), but they are being abused by many drivers to add support
> for
> per color gains. And then you miss one control. And in the case of the
> pac7302
> driver the "original" route was taken of using whitebalance to control
> the green gain. Which is wrong IMHO, but it is what the driver does know.
>
> A proper fix would be to introduce new controls for all 3 gains, and use
> those instead of using the balance controls + adding a 3th balance
> control
> being discussed in the thread titled:
> "Gain controls in v4l2-ctrl framework".

Ok, it seems I'm misunderstanding the meaning of color "gain" and
"balance"...

Anyway, I would say that it makes sense to have per color AND global
controls, so a V4L2_CID_GREEN_BALANCE would be missing.
Weather it makes sense to use them at the same time is a different question.

And why do you think the controls in question are gain controls instead
of balance controls ?
The Windows driver calls them balance controls, too (which could of
course also be a Windows driver or API issue...).


>
>>> And as said other drivers have similar (albeit usually different)
>>> hacks.
>>>
>>> At a minimum I would like you to rework your patches to:
>>> 1) Not add the new Green balance, and instead modify the existing
>>> whitebalance
>>> to control the new green gain you've found. Keeping things as broken as
>>> they are, but not worse; and
>>
>> I prefer waiting for the results of the discussion you are proposing
>> further down.
>>
>
> I see in your next mail that you've changed your mind. So I would like to
> move forward with this by adding your 2 patches + 1 more patch to also
> make the whitebalance control (which really is the green gain control)
> use 0x02 rather then 0xc6. To do this we must make sure that 0xc6 has
> a proper fixed / default setting. So what does the windows driver use
> for this? 1 like with 0xc5 and 0xc7 ?
>
> And can you do a 3th patch to make the whitebalance control control
> 0x02 rather then 0xc6 like you did for red and blue balance?

No, we shouldn't do that.
Reg 0xc6 (currently called "white balance temperature") definitely works
different compared to register 0x02.
Whatever it does exactly, it's not green gain or balance adjustment.
Will try to figure out next time.
The Windows driver doesn't use this register for an (user settable)
image control. It just sets its value to 55, which I fixed with one of
the patches from my previous patch series, so we have the correct
default value now.
0xc6 is also different from regs 0xc5 and 0xc7: settable values are
0-255 compared to 0-3.

So let's not touch 0xc6 / "white balance" until we know its real meaning
and just apply the first 2 patches.

Regards,
Frank


>
> Then later on when the "Gain controls in v4l2-ctrl framework" discussion
> is done we can change these controls to the new controls.
>
>>> 2) Try to use both the page 0 reg 0x01 - 0x03 and page 0 reg 0xc5 -
>>> 0xc7
>>> at the same time to get a wider ranged control. Maybe 0xc5 - 0xc7 are
>>> simply the most significant bits of a wider ranged gain ?
>>
>> I don't think so. The windows driver does not use them.
>> It even doesn't use the full range of registers 0x01-0x03.
>> Of course, I have expermiented with the full range and it works, but it
>> doesn't make much sense to use it.
>>
>
> Ok.
>
>
> Regards,
>
> Hans


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] gspca_pac7302: add support for green balance adjustment
  2012-09-24 13:36               ` Frank Schäfer
@ 2012-09-24 14:07                 ` Hans de Goede
  0 siblings, 0 replies; 13+ messages in thread
From: Hans de Goede @ 2012-09-24 14:07 UTC (permalink / raw)
  To: Frank Schäfer; +Cc: linux-media

Hi,

On 09/24/2012 03:36 PM, Frank Schäfer wrote:
> Hi Hans,
>
> Am 24.09.2012 12:47, schrieb Hans de Goede:
>> Hi,
>>
>> Sorry for the slow response ...
>>
>> On 09/20/2012 01:54 PM, Frank Schäfer wrote:
>>> Hi,
>>>
>>> Am 20.09.2012 11:08, schrieb Hans de Goede:
>>
>> <snip>
>>
>>>> Many webcams have RGB gains, but we don't have standard CID-s for
>>>> these,
>>>> instead we've Blue and Red Balance. This has grown historically
>>>> because of
>>>> the bttv cards which actually have Blue and Red balance controls in
>>>> hardware,
>>>> rather then the usual RGB gain triplet. Various gspca drivers cope
>>>> with this
>>>> in different ways.
>>>>
>>>> If you look at the pac7302 driver before your latest 4 patches it has
>>>> a Red and Blue balance control controlling the Red and Blue gain, and a
>>>> Whitebalance control, which is not White balance at all, but simply
>>>> controls the green gain...
>>>
>>> Ok, so if I understand you right, red+green+blue balance = white
>>> balance.
>>> And because we already have defined red, blue and whitebalance controls
>>> for historical reasons, we don't need green balance ?
>>> Maybe that matches physics, but I don't think it's a sane approach for a
>>> user interface...
>>
>> No what I was trying to say is that the balance controls are for hardware
>> which actually has balance controls and not per color gains (such as the
>> bt87x chips), but they are being abused by many drivers to add support
>> for
>> per color gains. And then you miss one control. And in the case of the
>> pac7302
>> driver the "original" route was taken of using whitebalance to control
>> the green gain. Which is wrong IMHO, but it is what the driver does know.
>>
>> A proper fix would be to introduce new controls for all 3 gains, and use
>> those instead of using the balance controls + adding a 3th balance
>> control
>> being discussed in the thread titled:
>> "Gain controls in v4l2-ctrl framework".
>
> Ok, it seems I'm misunderstanding the meaning of color "gain" and
> "balance"...
>
> Anyway, I would say that it makes sense to have per color AND global
> controls, so a V4L2_CID_GREEN_BALANCE would be missing.
> Weather it makes sense to use them at the same time is a different question.
>
> And why do you think the controls in question are gain controls instead
> of balance controls ?

"Balance" suggest balancing it against some fixed value, where as once
there are 3 for all of r, g and b, there is no fixed value, then the registers
are just controlling an amplification factor (which could be less then 1.0)
and that is normally called a gain.

If you look in most sensor datasheets they talk about color gains not
balance controls ...

Anyways this is mostly just semantics. We are working on defining standard
controls for per color gains, and once we have those we can map them to
reg 0x01 - 0x03.

> The Windows driver calls them balance controls, too (which could of
> course also be a Windows driver or API issue...).
>
>
>>
>>>> And as said other drivers have similar (albeit usually different)
>>>> hacks.
>>>>
>>>> At a minimum I would like you to rework your patches to:
>>>> 1) Not add the new Green balance, and instead modify the existing
>>>> whitebalance
>>>> to control the new green gain you've found. Keeping things as broken as
>>>> they are, but not worse; and
>>>
>>> I prefer waiting for the results of the discussion you are proposing
>>> further down.
>>>
>>
>> I see in your next mail that you've changed your mind. So I would like to
>> move forward with this by adding your 2 patches + 1 more patch to also
>> make the whitebalance control (which really is the green gain control)
>> use 0x02 rather then 0xc6. To do this we must make sure that 0xc6 has
>> a proper fixed / default setting. So what does the windows driver use
>> for this? 1 like with 0xc5 and 0xc7 ?
>>
>> And can you do a 3th patch to make the whitebalance control control
>> 0x02 rather then 0xc6 like you did for red and blue balance?
>
> No, we shouldn't do that.
> Reg 0xc6 (currently called "white balance temperature") definitely works
> different compared to register 0x02.
> Whatever it does exactly, it's not green gain or balance adjustment.
> Will try to figure out next time.
> The Windows driver doesn't use this register for an (user settable)
> image control. It just sets its value to 55, which I fixed with one of
> the patches from my previous patch series, so we have the correct
> default value now.

Ah, interesting!

> 0xc6 is also different from regs 0xc5 and 0xc7: settable values are
> 0-255 compared to 0-3.

True, I should have noticed that.

> So let's not touch 0xc6 / "white balance" until we know its real meaning
> and just apply the first 2 patches.

OK, will do.

Regards,

Hans

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-09-24 14:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-16 16:00 [PATCH 1/4] gspca_pac7302: correct register documentation Frank Schäfer
2012-09-16 16:00 ` [PATCH 2/4] gspca_pac7302: use registers 0x01 and 0x03 for red and blue balance controls Frank Schäfer
2012-09-16 16:00 ` [PATCH 3/4] v4l2-ctrl: add a control for green balance Frank Schäfer
2012-09-16 16:00 ` [PATCH 4/4] gspca_pac7302: add support for green balance adjustment Frank Schäfer
2012-09-19 17:25   ` Frank Schäfer
2012-09-19 20:34     ` Hans de Goede
2012-09-19 20:54       ` Frank Schäfer
2012-09-20  9:08         ` Hans de Goede
2012-09-20 11:54           ` Frank Schäfer
2012-09-23 13:27             ` Frank Schäfer
2012-09-24 10:47             ` Hans de Goede
2012-09-24 13:36               ` Frank Schäfer
2012-09-24 14:07                 ` Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).