[PATCH 1/2] Add a V4L2 control to allow configuring BLC from userspace.

[PATCH 1/2] Add a V4L2 control to allow configuring BLC from userspace.

[Arec Kao]

Trigger BLC update when analog gain change in specific range.

Signed-off-by: Arec Kao <arec.kao@intel.com>
---
 drivers/media/i2c/ov5675.c                | 41 ++++++++++++++++++++++-
 drivers/media/v4l2-core/v4l2-ctrls-defs.c |  1 +
 include/uapi/linux/v4l2-controls.h        |  1 +
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index 82ba9f56baec..39a0a7a06249 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -74,6 +74,13 @@
 #define OV5675_REG_FORMAT1		0x3820
 #define OV5675_REG_FORMAT2		0x373d
 
+/* BLC Control */
+#define OV5675_REG_BLC_CTRL10		0x4010
+#define OV5675_BLC_ENABLE		BIT(6) /* Gain change BLC trigger enable */
+
+#define OV5675_REG_BLC_CTRL11		0x4011
+#define OV5675_BLC_MULTI_FRAME_ENABLE	BIT(4) /* Gain change BLC trigger multi-frame enable */
+
 #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
 
 enum {
@@ -684,6 +691,34 @@ static int ov5675_set_ctrl_vflip(struct ov5675 *ov5675, u8 ctrl_val)
 				ctrl_val ? val | BIT(1) : val & ~BIT(1));
 }
 
+static int ov5675_update_blc(struct ov5675 *ov5675, u8 ctrl_val)
+{
+	int ret;
+	u32 val;
+
+	ret = ov5675_read_reg(ov5675, OV5675_REG_BLC_CTRL10,
+			      OV5675_REG_VALUE_08BIT, &val);
+	if (ret)
+		return ret;
+
+	ret = ov5675_write_reg(ov5675, OV5675_REG_BLC_CTRL10,
+			       OV5675_REG_VALUE_08BIT,
+			       ctrl_val ? val | OV5675_BLC_ENABLE :
+			       val & ~OV5675_BLC_ENABLE);
+	if (ret)
+		return ret;
+
+	ret = ov5675_read_reg(ov5675, OV5675_REG_BLC_CTRL11,
+			      OV5675_REG_VALUE_08BIT, &val);
+	if (ret)
+		return ret;
+
+	return ov5675_write_reg(ov5675, OV5675_REG_BLC_CTRL11,
+				OV5675_REG_VALUE_08BIT,
+				ctrl_val ? val | OV5675_BLC_MULTI_FRAME_ENABLE :
+				val & ~OV5675_BLC_MULTI_FRAME_ENABLE);
+}
+
 static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct ov5675 *ov5675 = container_of(ctrl->handler,
@@ -748,6 +783,9 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl)
 		ov5675_set_ctrl_vflip(ov5675, ctrl->val);
 		break;
 
+	case V4L2_CID_BLC:
+		ret = ov5675_update_blc(ov5675, ctrl->val);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -819,7 +857,8 @@ static int ov5675_init_controls(struct ov5675 *ov5675)
 			  V4L2_CID_HFLIP, 0, 1, 1, 0);
 	v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops,
 			  V4L2_CID_VFLIP, 0, 1, 1, 0);
-
+	v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops,
+			  V4L2_CID_BLC, 0, 1, 1, 1);
 	if (ctrl_hdlr->error)
 		return ctrl_hdlr->error;
 
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 54ca4e6b820b..2b0b295fc047 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1110,6 +1110,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_TEST_PATTERN_BLUE:	return "Blue Pixel Value";
 	case V4L2_CID_TEST_PATTERN_GREENB:	return "Green (Blue) Pixel Value";
 	case V4L2_CID_NOTIFY_GAINS:		return "Notify Gains";
+	case V4L2_CID_BLC:			return "Black Level Calibration";
 
 	/* Image processing controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index c8e0f84d204d..0a0fb1283124 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1126,6 +1126,7 @@ enum v4l2_jpeg_chroma_subsampling {
 #define V4L2_CID_TEST_PATTERN_GREENB		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
 #define V4L2_CID_UNIT_CELL_SIZE			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
 #define V4L2_CID_NOTIFY_GAINS			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 9)
+#define V4L2_CID_BLC				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 10)
 
 
 /* Image processing controls */
-- 
2.17.1

[PATCH 2/2] Re-run BLC when gain change

[Arec Kao]

Changing the gain affects the black-level through the device;
the gain should therefore not be changed during a BLC procedure.
If the gain changes, then the BLC routine should be re-run
in some scenarios.

Signed-off-by: Arec Kao <arec.kao@intel.com>
---
 .../userspace-api/media/v4l/ext-ctrls-image-source.rst       | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
index 71f23f131f97..5ee53ba76d46 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
@@ -59,6 +59,11 @@ Image Source Control IDs
     non-sensitive.
     This control is required for automatic calibration of sensors/cameras.
 
+``V4L2_CID_BLC (integer)``
+    Changing the gain affects the black-level through the device; the gain
+    should therefore not be changed during a BLC procedure. If the gain
+    changes, the BLC routine should be re-run in some scenarios.
+
 .. c:type:: v4l2_area
 
 .. flat-table:: struct v4l2_area
-- 
2.17.1

Re: [PATCH 1/2] Add a V4L2 control to allow configuring BLC from userspace.

[Laurent Pinchart]

Hi Arec,

Thank you for the patch.

On Tue, Mar 08, 2022 at 11:38:38AM +0800, Arec Kao wrote:
> Trigger BLC update when analog gain change in specific range.
> 
> Signed-off-by: Arec Kao <arec.kao@intel.com>
> ---
>  drivers/media/i2c/ov5675.c                | 41 ++++++++++++++++++++++-
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c |  1 +
>  include/uapi/linux/v4l2-controls.h        |  1 +

This should be split in two patches, with the changes to
drivers/media/v4l2-core/v4l2-ctrls-defs.c and
include/uapi/linux/v4l2-controls.h bundled with the documentation change
from patch 2/2. A second patch should then implement support for this
control in the ov5675 driver.

>  3 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 82ba9f56baec..39a0a7a06249 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -74,6 +74,13 @@
>  #define OV5675_REG_FORMAT1		0x3820
>  #define OV5675_REG_FORMAT2		0x373d
>  
> +/* BLC Control */
> +#define OV5675_REG_BLC_CTRL10		0x4010
> +#define OV5675_BLC_ENABLE		BIT(6) /* Gain change BLC trigger enable */
> +
> +#define OV5675_REG_BLC_CTRL11		0x4011
> +#define OV5675_BLC_MULTI_FRAME_ENABLE	BIT(4) /* Gain change BLC trigger multi-frame enable */
> +
>  #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
>  
>  enum {
> @@ -684,6 +691,34 @@ static int ov5675_set_ctrl_vflip(struct ov5675 *ov5675, u8 ctrl_val)
>  				ctrl_val ? val | BIT(1) : val & ~BIT(1));
>  }
>  
> +static int ov5675_update_blc(struct ov5675 *ov5675, u8 ctrl_val)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = ov5675_read_reg(ov5675, OV5675_REG_BLC_CTRL10,
> +			      OV5675_REG_VALUE_08BIT, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov5675_write_reg(ov5675, OV5675_REG_BLC_CTRL10,
> +			       OV5675_REG_VALUE_08BIT,
> +			       ctrl_val ? val | OV5675_BLC_ENABLE :
> +			       val & ~OV5675_BLC_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov5675_read_reg(ov5675, OV5675_REG_BLC_CTRL11,
> +			      OV5675_REG_VALUE_08BIT, &val);
> +	if (ret)
> +		return ret;
> +
> +	return ov5675_write_reg(ov5675, OV5675_REG_BLC_CTRL11,
> +				OV5675_REG_VALUE_08BIT,
> +				ctrl_val ? val | OV5675_BLC_MULTI_FRAME_ENABLE :
> +				val & ~OV5675_BLC_MULTI_FRAME_ENABLE);
> +}
> +
>  static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct ov5675 *ov5675 = container_of(ctrl->handler,
> @@ -748,6 +783,9 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl)
>  		ov5675_set_ctrl_vflip(ov5675, ctrl->val);
>  		break;
>  
> +	case V4L2_CID_BLC:
> +		ret = ov5675_update_blc(ov5675, ctrl->val);
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -819,7 +857,8 @@ static int ov5675_init_controls(struct ov5675 *ov5675)
>  			  V4L2_CID_HFLIP, 0, 1, 1, 0);
>  	v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops,
>  			  V4L2_CID_VFLIP, 0, 1, 1, 0);
> -
> +	v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops,
> +			  V4L2_CID_BLC, 0, 1, 1, 1);
>  	if (ctrl_hdlr->error)
>  		return ctrl_hdlr->error;
>  
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 54ca4e6b820b..2b0b295fc047 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1110,6 +1110,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_TEST_PATTERN_BLUE:	return "Blue Pixel Value";
>  	case V4L2_CID_TEST_PATTERN_GREENB:	return "Green (Blue) Pixel Value";
>  	case V4L2_CID_NOTIFY_GAINS:		return "Notify Gains";
> +	case V4L2_CID_BLC:			return "Black Level Calibration";
>  
>  	/* Image processing controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index c8e0f84d204d..0a0fb1283124 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1126,6 +1126,7 @@ enum v4l2_jpeg_chroma_subsampling {
>  #define V4L2_CID_TEST_PATTERN_GREENB		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
>  #define V4L2_CID_UNIT_CELL_SIZE			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
>  #define V4L2_CID_NOTIFY_GAINS			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 9)
> +#define V4L2_CID_BLC				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 10)
>  
>  
>  /* Image processing controls */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] Re-run BLC when gain change

[Laurent Pinchart]

Hi Arec,

Thank you for the patch.

On Tue, Mar 08, 2022 at 11:38:39AM +0800, Arec Kao wrote:
> Changing the gain affects the black-level through the device;
> the gain should therefore not be changed during a BLC procedure.
> If the gain changes, then the BLC routine should be re-run
> in some scenarios.
> 
> Signed-off-by: Arec Kao <arec.kao@intel.com>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-image-source.rst       | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> index 71f23f131f97..5ee53ba76d46 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> @@ -59,6 +59,11 @@ Image Source Control IDs
>      non-sensitive.
>      This control is required for automatic calibration of sensors/cameras.
>  
> +``V4L2_CID_BLC (integer)``
> +    Changing the gain affects the black-level through the device; the gain
> +    should therefore not be changed during a BLC procedure. If the gain
> +    changes, the BLC routine should be re-run in some scenarios.

This doesn't tell me what the control does.

> +
>  .. c:type:: v4l2_area
>  
>  .. flat-table:: struct v4l2_area

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] Add a V4L2 control to allow configuring BLC from userspace.

[Hans Verkuil]

On 3/8/22 04:38, Arec Kao wrote:
> Trigger BLC update when analog gain change in specific range.
> 
> Signed-off-by: Arec Kao <arec.kao@intel.com>
> ---
>  drivers/media/i2c/ov5675.c                | 41 ++++++++++++++++++++++-
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c |  1 +
>  include/uapi/linux/v4l2-controls.h        |  1 +
>  3 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 82ba9f56baec..39a0a7a06249 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -74,6 +74,13 @@
>  #define OV5675_REG_FORMAT1		0x3820
>  #define OV5675_REG_FORMAT2		0x373d
>  
> +/* BLC Control */
> +#define OV5675_REG_BLC_CTRL10		0x4010
> +#define OV5675_BLC_ENABLE		BIT(6) /* Gain change BLC trigger enable */
> +
> +#define OV5675_REG_BLC_CTRL11		0x4011
> +#define OV5675_BLC_MULTI_FRAME_ENABLE	BIT(4) /* Gain change BLC trigger multi-frame enable */
> +
>  #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
>  
>  enum {
> @@ -684,6 +691,34 @@ static int ov5675_set_ctrl_vflip(struct ov5675 *ov5675, u8 ctrl_val)
>  				ctrl_val ? val | BIT(1) : val & ~BIT(1));
>  }
>  
> +static int ov5675_update_blc(struct ov5675 *ov5675, u8 ctrl_val)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = ov5675_read_reg(ov5675, OV5675_REG_BLC_CTRL10,
> +			      OV5675_REG_VALUE_08BIT, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov5675_write_reg(ov5675, OV5675_REG_BLC_CTRL10,
> +			       OV5675_REG_VALUE_08BIT,
> +			       ctrl_val ? val | OV5675_BLC_ENABLE :
> +			       val & ~OV5675_BLC_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov5675_read_reg(ov5675, OV5675_REG_BLC_CTRL11,
> +			      OV5675_REG_VALUE_08BIT, &val);
> +	if (ret)
> +		return ret;
> +
> +	return ov5675_write_reg(ov5675, OV5675_REG_BLC_CTRL11,
> +				OV5675_REG_VALUE_08BIT,
> +				ctrl_val ? val | OV5675_BLC_MULTI_FRAME_ENABLE :
> +				val & ~OV5675_BLC_MULTI_FRAME_ENABLE);
> +}
> +
>  static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct ov5675 *ov5675 = container_of(ctrl->handler,
> @@ -748,6 +783,9 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl)
>  		ov5675_set_ctrl_vflip(ov5675, ctrl->val);
>  		break;
>  
> +	case V4L2_CID_BLC:
> +		ret = ov5675_update_blc(ov5675, ctrl->val);
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -819,7 +857,8 @@ static int ov5675_init_controls(struct ov5675 *ov5675)
>  			  V4L2_CID_HFLIP, 0, 1, 1, 0);
>  	v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops,
>  			  V4L2_CID_VFLIP, 0, 1, 1, 0);
> -
> +	v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops,
> +			  V4L2_CID_BLC, 0, 1, 1, 1);

It's an integer control, but used as a bool. So shouldn't it be a bool control?
Without the documentation of what it does exactly it is hard to tell.

>  	if (ctrl_hdlr->error)
>  		return ctrl_hdlr->error;
>  
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 54ca4e6b820b..2b0b295fc047 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1110,6 +1110,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_TEST_PATTERN_BLUE:	return "Blue Pixel Value";
>  	case V4L2_CID_TEST_PATTERN_GREENB:	return "Green (Blue) Pixel Value";
>  	case V4L2_CID_NOTIFY_GAINS:		return "Notify Gains";
> +	case V4L2_CID_BLC:			return "Black Level Calibration";
>  
>  	/* Image processing controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index c8e0f84d204d..0a0fb1283124 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1126,6 +1126,7 @@ enum v4l2_jpeg_chroma_subsampling {
>  #define V4L2_CID_TEST_PATTERN_GREENB		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
>  #define V4L2_CID_UNIT_CELL_SIZE			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
>  #define V4L2_CID_NOTIFY_GAINS			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 9)
> +#define V4L2_CID_BLC				(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 10)

Please rename this to V4L2_CID_BLACK_LEVEL_CALIB or _CALIBRATION.

Control names should be descriptive, and I had no idea what BLC meant.

I'm leaning to writing _CALIBRATION in full, but Laurent might prefer something
shorter. _CALIB is also understandable, I think.

Regards,

	Hans

>  
>  
>  /* Image processing controls */

Re: [PATCH 2/2] Re-run BLC when gain change

[Sakari Ailus]

Hi Arec,

Thanks for the patch.

On Tue, Mar 08, 2022 at 11:38:39AM +0800, Arec Kao wrote:
> Changing the gain affects the black-level through the device;
> the gain should therefore not be changed during a BLC procedure.
> If the gain changes, then the BLC routine should be re-run
> in some scenarios.

Could you also explain what are the scenarios the BLC routine should be
re-run and how does the user space know this should be done? Could this be
done in the driver instead without involving the user space?

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/2] Re-run BLC when gain change

[Tomasz Figa]

On Tue, Mar 8, 2022 at 9:48 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Arec,
>
> Thanks for the patch.
>
> On Tue, Mar 08, 2022 at 11:38:39AM +0800, Arec Kao wrote:
> > Changing the gain affects the black-level through the device;
> > the gain should therefore not be changed during a BLC procedure.
> > If the gain changes, then the BLC routine should be re-run
> > in some scenarios.
>
> Could you also explain what are the scenarios the BLC routine should be
> re-run and how does the user space know this should be done? Could this be
> done in the driver instead without involving the user space?

FWIW, this turned out to be a camera module design issue (the
investigation was running in parallel to the potential software fix),
so this patch series isn't needed anymore.

Best regards,
Tomasz