[PATCH 0/5] media: ov2680: Add all controls required by libcamera

[PATCH 0/5] media: ov2680: Add all controls required by libcamera

[Hans de Goede]

Hi All,

This patch-series adds all controls required by libcamera to the ov2680
driver.

This has been tested together with the atomisp using the simple
pipelinehandler. This should also work in IPU3 based devices with
an ov2680.

Note the vblank control new default value after a mode-set call 
has been chosen so as to preserve the old behavior of always
running at 30 fps, so as to not change behavior for any existing
use-cases.

Regards,

Hans


Hans de Goede (5):
  media: ov2680: Stop sending more data then requested
  media: ov2680: Drop hts, vts ov2680_mode struct members
  media: ov2680: Add vblank control
  media: ov2680: Add hblank control
  media: ov2680: Add camera orientation and sensor rotation controls

 drivers/media/i2c/ov2680.c | 82 +++++++++++++++++++++++++++++---------
 1 file changed, 63 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH 1/5] media: ov2680: Stop sending more data then requested

[Hans de Goede]

There is no reason to send OV2680_END_MARGIN extra columns on top of
the mode width and the same for sending extra lines over the mode height.

This sending of extra lines/columns was inherited from the atomisp
ov2680 driver, it is unclear why this was done and this complicates
adding V4L2_CID_VBLANK support, so remove it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 39d321e2b7f9..5b04c6c0554a 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -86,9 +86,6 @@
 #define OV2680_PIXELS_PER_LINE			1704
 #define OV2680_LINES_PER_FRAME			1294
 
-/* If possible send 16 extra rows / lines to the ISP as padding */
-#define OV2680_END_MARGIN			16
-
 /* Max exposure time is VTS - 8 */
 #define OV2680_INTEGRATION_TIME_MARGIN		8
 
@@ -359,11 +356,9 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
 	sensor->mode.v_start = (sensor->mode.crop.top +
 				(sensor->mode.crop.height - height) / 2) & ~1;
 	sensor->mode.h_end =
-		min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1,
-		    OV2680_NATIVE_WIDTH - 1);
+		min(sensor->mode.h_start + width - 1, OV2680_NATIVE_WIDTH - 1);
 	sensor->mode.v_end =
-		min(sensor->mode.v_start + height + OV2680_END_MARGIN - 1,
-		    OV2680_NATIVE_HEIGHT - 1);
+		min(sensor->mode.v_start + height - 1, OV2680_NATIVE_HEIGHT - 1);
 	sensor->mode.h_output_size = orig_width;
 	sensor->mode.v_output_size = orig_height;
 	sensor->mode.hts = OV2680_PIXELS_PER_LINE;
-- 
2.43.0

[PATCH 2/5] media: ov2680: Drop hts, vts ov2680_mode struct members

[Hans de Goede]

The hts, vts ov2680_mode struct members always contain
OV2680_PIXELS_PER_LINE resp. OV2680_LINES_PER_FRAME,
drop them and simply use these values directly.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 5b04c6c0554a..b4d5936dcd02 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -140,8 +140,6 @@ struct ov2680_mode {
 	u16				v_end;
 	u16				h_output_size;
 	u16				v_output_size;
-	u16				hts;
-	u16				vts;
 };
 
 struct ov2680_dev {
@@ -361,8 +359,6 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
 		min(sensor->mode.v_start + height - 1, OV2680_NATIVE_HEIGHT - 1);
 	sensor->mode.h_output_size = orig_width;
 	sensor->mode.v_output_size = orig_height;
-	sensor->mode.hts = OV2680_PIXELS_PER_LINE;
-	sensor->mode.vts = OV2680_LINES_PER_FRAME;
 }
 
 static int ov2680_set_mode(struct ov2680_dev *sensor)
@@ -397,9 +393,9 @@ static int ov2680_set_mode(struct ov2680_dev *sensor)
 	cci_write(sensor->regmap, OV2680_REG_VERTICAL_OUTPUT_SIZE,
 		  sensor->mode.v_output_size, &ret);
 	cci_write(sensor->regmap, OV2680_REG_TIMING_HTS,
-		  sensor->mode.hts, &ret);
+		  OV2680_PIXELS_PER_LINE, &ret);
 	cci_write(sensor->regmap, OV2680_REG_TIMING_VTS,
-		  sensor->mode.vts, &ret);
+		  OV2680_LINES_PER_FRAME, &ret);
 	cci_write(sensor->regmap, OV2680_REG_ISP_X_WIN, 0, &ret);
 	cci_write(sensor->regmap, OV2680_REG_ISP_Y_WIN, 0, &ret);
 	cci_write(sensor->regmap, OV2680_REG_X_INC, inc, &ret);
-- 
2.43.0

[PATCH 3/5] media: ov2680: Add vblank control

[Hans de Goede]

Add vblank control to allow changing the framerate /
higher exposure values.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 47 ++++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index b4d5936dcd02..4c9db83d876e 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -75,6 +75,8 @@
 #define OV2680_ACTIVE_START_TOP			8
 #define OV2680_MIN_CROP_WIDTH			2
 #define OV2680_MIN_CROP_HEIGHT			2
+#define OV2680_MIN_VBLANK			4
+#define OV2680_MAX_VBLANK			0xffff
 
 /* Fixed pre-div of 1/2 */
 #define OV2680_PLL_PREDIV0			2
@@ -84,7 +86,7 @@
 
 /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */
 #define OV2680_PIXELS_PER_LINE			1704
-#define OV2680_LINES_PER_FRAME			1294
+#define OV2680_LINES_PER_FRAME_30FPS		1294
 
 /* Max exposure time is VTS - 8 */
 #define OV2680_INTEGRATION_TIME_MARGIN		8
@@ -127,6 +129,7 @@ struct ov2680_ctrls {
 	struct v4l2_ctrl *test_pattern;
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_ctrl *vblank;
 };
 
 struct ov2680_mode {
@@ -394,8 +397,7 @@ static int ov2680_set_mode(struct ov2680_dev *sensor)
 		  sensor->mode.v_output_size, &ret);
 	cci_write(sensor->regmap, OV2680_REG_TIMING_HTS,
 		  OV2680_PIXELS_PER_LINE, &ret);
-	cci_write(sensor->regmap, OV2680_REG_TIMING_VTS,
-		  OV2680_LINES_PER_FRAME, &ret);
+	/* VTS gets set by the vblank ctrl */
 	cci_write(sensor->regmap, OV2680_REG_ISP_X_WIN, 0, &ret);
 	cci_write(sensor->regmap, OV2680_REG_ISP_Y_WIN, 0, &ret);
 	cci_write(sensor->regmap, OV2680_REG_X_INC, inc, &ret);
@@ -469,6 +471,15 @@ static int ov2680_exposure_set(struct ov2680_dev *sensor, u32 exp)
 			 NULL);
 }
 
+static void ov2680_exposure_update_range(struct ov2680_dev *sensor)
+{
+	int exp_max = sensor->mode.fmt.height + sensor->ctrls.vblank->val -
+		      OV2680_INTEGRATION_TIME_MARGIN;
+
+	__v4l2_ctrl_modify_range(sensor->ctrls.exposure, 0, exp_max,
+				 1, exp_max);
+}
+
 static int ov2680_stream_enable(struct ov2680_dev *sensor)
 {
 	int ret;
@@ -635,7 +646,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *try_fmt;
 	const struct v4l2_rect *crop;
 	unsigned int width, height;
-	int ret = 0;
+	int def, max, ret = 0;
 
 	crop = __ov2680_get_pad_crop(sensor, sd_state, format->pad,
 				     format->which);
@@ -664,6 +675,15 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	sensor->mode.fmt = format->format;
 	ov2680_calc_mode(sensor);
 
+	/* vblank range is height dependent adjust and reset to default */
+	max = OV2680_MAX_VBLANK - height;
+	def = OV2680_LINES_PER_FRAME_30FPS - height;
+	__v4l2_ctrl_modify_range(sensor->ctrls.vblank, OV2680_MIN_VBLANK, max,
+				 1, def);
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, def);
+	/* exposure range depends on vts which may have changed */
+	ov2680_exposure_update_range(sensor);
+
 unlock:
 	mutex_unlock(&sensor->lock);
 
@@ -833,6 +853,10 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 	int ret;
 
+	/* Update exposure range on vblank changes */
+	if (ctrl->id == V4L2_CID_VBLANK)
+		ov2680_exposure_update_range(sensor);
+
 	/* Only apply changes to the controls if the device is powered up */
 	if (!pm_runtime_get_if_in_use(sensor->sd.dev)) {
 		ov2680_set_bayer_order(sensor, &sensor->mode.fmt);
@@ -855,6 +879,10 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_TEST_PATTERN:
 		ret = ov2680_test_pattern_set(sensor, ctrl->val);
 		break;
+	case V4L2_CID_VBLANK:
+		ret = cci_write(sensor->regmap, OV2680_REG_TIMING_VTS,
+				sensor->mode.fmt.height + ctrl->val, NULL);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -913,8 +941,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 	const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
 	struct ov2680_ctrls *ctrls = &sensor->ctrls;
 	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
-	int exp_max = OV2680_LINES_PER_FRAME - OV2680_INTEGRATION_TIME_MARGIN;
-	int ret = 0;
+	int def, max, ret = 0;
 
 	v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops);
 	sensor->sd.internal_ops = &ov2680_internal_ops;
@@ -939,8 +966,9 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 					ARRAY_SIZE(test_pattern_menu) - 1,
 					0, 0, test_pattern_menu);
 
+	max = OV2680_LINES_PER_FRAME_30FPS - OV2680_INTEGRATION_TIME_MARGIN;
 	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
-					    0, exp_max, 1, exp_max);
+					    0, max, 1, max);
 
 	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
 					0, 1023, 1, 250);
@@ -951,6 +979,11 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 					      0, sensor->pixel_rate,
 					      1, sensor->pixel_rate);
 
+	max = OV2680_MAX_VBLANK - OV2680_DEFAULT_HEIGHT;
+	def = OV2680_LINES_PER_FRAME_30FPS - OV2680_DEFAULT_HEIGHT;
+	ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
+					  OV2680_MIN_VBLANK, max, 1, def);
+
 	if (hdl->error) {
 		ret = hdl->error;
 		goto cleanup_entity;
-- 
2.43.0

[PATCH 4/5] media: ov2680: Add hblank control

[Hans de Goede]

Add hblank control so that the sensor has all the mandatory
controls for libcamera.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 4c9db83d876e..fef9e11ee141 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -130,6 +130,7 @@ struct ov2680_ctrls {
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl *vblank;
+	struct v4l2_ctrl *hblank;
 };
 
 struct ov2680_mode {
@@ -684,6 +685,10 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	/* exposure range depends on vts which may have changed */
 	ov2680_exposure_update_range(sensor);
 
+	/* adjust hblank value for new width */
+	def = OV2680_PIXELS_PER_LINE - width;
+	__v4l2_ctrl_modify_range(sensor->ctrls.hblank, def, def, 1, def);
+
 unlock:
 	mutex_unlock(&sensor->lock);
 
@@ -984,6 +989,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 	ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
 					  OV2680_MIN_VBLANK, max, 1, def);
 
+	def = OV2680_PIXELS_PER_LINE - OV2680_DEFAULT_WIDTH;
+	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
+					  def, def, 1, def);
+	if (ctrls->hblank)
+		ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	if (hdl->error) {
 		ret = hdl->error;
 		goto cleanup_entity;
-- 
2.43.0

[PATCH 5/5] media: ov2680: Add camera orientation and sensor rotation controls

[Hans de Goede]

Add camera orientation and sensor rotation controls using
the v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties()
helpers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index fef9e11ee141..bcd031882a37 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -946,6 +946,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 	const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
 	struct ov2680_ctrls *ctrls = &sensor->ctrls;
 	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
+	struct v4l2_fwnode_device_properties props;
 	int def, max, ret = 0;
 
 	v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops);
@@ -1000,6 +1001,14 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 		goto cleanup_entity;
 	}
 
+	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
+	if (ret)
+		goto cleanup_entity;
+
+	ret = v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
+	if (ret)
+		goto cleanup_entity;
+
 	ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 	ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-- 
2.43.0

Re: [PATCH 1/5] media: ov2680: Stop sending more data then requested

[Kieran Bingham]

Quoting Hans de Goede (2024-02-16 22:32:33)
> There is no reason to send OV2680_END_MARGIN extra columns on top of
> the mode width and the same for sending extra lines over the mode height.
> 
> This sending of extra lines/columns was inherited from the atomisp
> ov2680 driver, it is unclear why this was done and this complicates
> adding V4L2_CID_VBLANK support, so remove it.

Was this some niave way of adding some HBLANK to let the AtomISP keep up
with processing each line?

Or is it an optical black region? or padding? (I hit issues around that
on the IMX283 lately).

Is this a sensor you have and can visually check?

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/ov2680.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index 39d321e2b7f9..5b04c6c0554a 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -86,9 +86,6 @@
>  #define OV2680_PIXELS_PER_LINE                 1704
>  #define OV2680_LINES_PER_FRAME                 1294
>  
> -/* If possible send 16 extra rows / lines to the ISP as padding */
> -#define OV2680_END_MARGIN                      16
> -
>  /* Max exposure time is VTS - 8 */
>  #define OV2680_INTEGRATION_TIME_MARGIN         8
>  
> @@ -359,11 +356,9 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
>         sensor->mode.v_start = (sensor->mode.crop.top +
>                                 (sensor->mode.crop.height - height) / 2) & ~1;
>         sensor->mode.h_end =
> -               min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1,
> -                   OV2680_NATIVE_WIDTH - 1);
> +               min(sensor->mode.h_start + width - 1, OV2680_NATIVE_WIDTH - 1);
>         sensor->mode.v_end =
> -               min(sensor->mode.v_start + height + OV2680_END_MARGIN - 1,
> -                   OV2680_NATIVE_HEIGHT - 1);
> +               min(sensor->mode.v_start + height - 1, OV2680_NATIVE_HEIGHT - 1);
>         sensor->mode.h_output_size = orig_width;
>         sensor->mode.v_output_size = orig_height;
>         sensor->mode.hts = OV2680_PIXELS_PER_LINE;

Especially as seeing hts = OV2680_PIXELS_PER_LINE it does sound like the
margin is superfluous.

> -- 
> 2.43.0
>

Re: [PATCH 2/5] media: ov2680: Drop hts, vts ov2680_mode struct members

[Kieran Bingham]

Quoting Hans de Goede (2024-02-16 22:32:34)
> The hts, vts ov2680_mode struct members always contain
> OV2680_PIXELS_PER_LINE resp. OV2680_LINES_PER_FRAME,
> drop them and simply use these values directly.
> 


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/ov2680.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index 5b04c6c0554a..b4d5936dcd02 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -140,8 +140,6 @@ struct ov2680_mode {
>         u16                             v_end;
>         u16                             h_output_size;
>         u16                             v_output_size;
> -       u16                             hts;
> -       u16                             vts;
>  };
>  
>  struct ov2680_dev {
> @@ -361,8 +359,6 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
>                 min(sensor->mode.v_start + height - 1, OV2680_NATIVE_HEIGHT - 1);
>         sensor->mode.h_output_size = orig_width;
>         sensor->mode.v_output_size = orig_height;
> -       sensor->mode.hts = OV2680_PIXELS_PER_LINE;
> -       sensor->mode.vts = OV2680_LINES_PER_FRAME;
>  }
>  
>  static int ov2680_set_mode(struct ov2680_dev *sensor)
> @@ -397,9 +393,9 @@ static int ov2680_set_mode(struct ov2680_dev *sensor)
>         cci_write(sensor->regmap, OV2680_REG_VERTICAL_OUTPUT_SIZE,
>                   sensor->mode.v_output_size, &ret);
>         cci_write(sensor->regmap, OV2680_REG_TIMING_HTS,
> -                 sensor->mode.hts, &ret);
> +                 OV2680_PIXELS_PER_LINE, &ret);
>         cci_write(sensor->regmap, OV2680_REG_TIMING_VTS,
> -                 sensor->mode.vts, &ret);
> +                 OV2680_LINES_PER_FRAME, &ret);
>         cci_write(sensor->regmap, OV2680_REG_ISP_X_WIN, 0, &ret);
>         cci_write(sensor->regmap, OV2680_REG_ISP_Y_WIN, 0, &ret);
>         cci_write(sensor->regmap, OV2680_REG_X_INC, inc, &ret);
> -- 
> 2.43.0
>

Re: [PATCH 3/5] media: ov2680: Add vblank control

[Kieran Bingham]

Quoting Hans de Goede (2024-02-16 22:32:35)
> Add vblank control to allow changing the framerate /
> higher exposure values.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/ov2680.c | 47 ++++++++++++++++++++++++++++++++------
>  1 file changed, 40 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index b4d5936dcd02..4c9db83d876e 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -75,6 +75,8 @@
>  #define OV2680_ACTIVE_START_TOP                        8
>  #define OV2680_MIN_CROP_WIDTH                  2
>  #define OV2680_MIN_CROP_HEIGHT                 2
> +#define OV2680_MIN_VBLANK                      4
> +#define OV2680_MAX_VBLANK                      0xffff
>  
>  /* Fixed pre-div of 1/2 */
>  #define OV2680_PLL_PREDIV0                     2
> @@ -84,7 +86,7 @@
>  
>  /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */
>  #define OV2680_PIXELS_PER_LINE                 1704
> -#define OV2680_LINES_PER_FRAME                 1294
> +#define OV2680_LINES_PER_FRAME_30FPS           1294

I can definitely foresee some core sensor helpers to handle all the
calculations for things like this as parameters rather than static data
to support when changes and features are extended in drivers like
supporting different link frequencies - but for now ACK here.

>  /* Max exposure time is VTS - 8 */
>  #define OV2680_INTEGRATION_TIME_MARGIN         8
> @@ -127,6 +129,7 @@ struct ov2680_ctrls {
>         struct v4l2_ctrl *test_pattern;
>         struct v4l2_ctrl *link_freq;
>         struct v4l2_ctrl *pixel_rate;
> +       struct v4l2_ctrl *vblank;
>  };
>  
>  struct ov2680_mode {
> @@ -394,8 +397,7 @@ static int ov2680_set_mode(struct ov2680_dev *sensor)
>                   sensor->mode.v_output_size, &ret);
>         cci_write(sensor->regmap, OV2680_REG_TIMING_HTS,
>                   OV2680_PIXELS_PER_LINE, &ret);
> -       cci_write(sensor->regmap, OV2680_REG_TIMING_VTS,
> -                 OV2680_LINES_PER_FRAME, &ret);
> +       /* VTS gets set by the vblank ctrl */
>         cci_write(sensor->regmap, OV2680_REG_ISP_X_WIN, 0, &ret);
>         cci_write(sensor->regmap, OV2680_REG_ISP_Y_WIN, 0, &ret);
>         cci_write(sensor->regmap, OV2680_REG_X_INC, inc, &ret);
> @@ -469,6 +471,15 @@ static int ov2680_exposure_set(struct ov2680_dev *sensor, u32 exp)
>                          NULL);
>  }
>  
> +static void ov2680_exposure_update_range(struct ov2680_dev *sensor)
> +{

Reading here for the first time, I almost would have needed the comment 

  /* Max exposure time is VTS - 8 */

to be here. Fortunately, it was in the context of this patch above so I
could see it.


> +       int exp_max = sensor->mode.fmt.height + sensor->ctrls.vblank->val -
> +                     OV2680_INTEGRATION_TIME_MARGIN;
> +
> +       __v4l2_ctrl_modify_range(sensor->ctrls.exposure, 0, exp_max,
> +                                1, exp_max);
> +}
> +
>  static int ov2680_stream_enable(struct ov2680_dev *sensor)
>  {
>         int ret;
> @@ -635,7 +646,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
>         struct v4l2_mbus_framefmt *try_fmt;
>         const struct v4l2_rect *crop;
>         unsigned int width, height;
> -       int ret = 0;
> +       int def, max, ret = 0;
>  
>         crop = __ov2680_get_pad_crop(sensor, sd_state, format->pad,
>                                      format->which);
> @@ -664,6 +675,15 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
>         sensor->mode.fmt = format->format;
>         ov2680_calc_mode(sensor);
>  
> +       /* vblank range is height dependent adjust and reset to default */
> +       max = OV2680_MAX_VBLANK - height;
> +       def = OV2680_LINES_PER_FRAME_30FPS - height;
> +       __v4l2_ctrl_modify_range(sensor->ctrls.vblank, OV2680_MIN_VBLANK, max,
> +                                1, def);
> +       __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, def);
> +       /* exposure range depends on vts which may have changed */
> +       ov2680_exposure_update_range(sensor);
> +
>  unlock:
>         mutex_unlock(&sensor->lock);
>  
> @@ -833,6 +853,10 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
>         struct ov2680_dev *sensor = to_ov2680_dev(sd);
>         int ret;
>  
> +       /* Update exposure range on vblank changes */
> +       if (ctrl->id == V4L2_CID_VBLANK)
> +               ov2680_exposure_update_range(sensor);
> +
>         /* Only apply changes to the controls if the device is powered up */
>         if (!pm_runtime_get_if_in_use(sensor->sd.dev)) {
>                 ov2680_set_bayer_order(sensor, &sensor->mode.fmt);
> @@ -855,6 +879,10 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
>         case V4L2_CID_TEST_PATTERN:
>                 ret = ov2680_test_pattern_set(sensor, ctrl->val);
>                 break;
> +       case V4L2_CID_VBLANK:
> +               ret = cci_write(sensor->regmap, OV2680_REG_TIMING_VTS,
> +                               sensor->mode.fmt.height + ctrl->val, NULL);
> +               break;
>         default:
>                 ret = -EINVAL;
>                 break;
> @@ -913,8 +941,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>         const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
>         struct ov2680_ctrls *ctrls = &sensor->ctrls;
>         struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> -       int exp_max = OV2680_LINES_PER_FRAME - OV2680_INTEGRATION_TIME_MARGIN;
> -       int ret = 0;
> +       int def, max, ret = 0;
>  
>         v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops);
>         sensor->sd.internal_ops = &ov2680_internal_ops;
> @@ -939,8 +966,9 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>                                         ARRAY_SIZE(test_pattern_menu) - 1,
>                                         0, 0, test_pattern_menu);
>  
> +       max = OV2680_LINES_PER_FRAME_30FPS - OV2680_INTEGRATION_TIME_MARGIN;
>         ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> -                                           0, exp_max, 1, exp_max);
> +                                           0, max, 1, max);
>  
>         ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
>                                         0, 1023, 1, 250);
> @@ -951,6 +979,11 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>                                               0, sensor->pixel_rate,
>                                               1, sensor->pixel_rate);
>  
> +       max = OV2680_MAX_VBLANK - OV2680_DEFAULT_HEIGHT;
> +       def = OV2680_LINES_PER_FRAME_30FPS - OV2680_DEFAULT_HEIGHT;
> +       ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
> +                                         OV2680_MIN_VBLANK, max, 1, def);
> +


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         if (hdl->error) {
>                 ret = hdl->error;
>                 goto cleanup_entity;
> -- 
> 2.43.0
>

Re: [PATCH 4/5] media: ov2680: Add hblank control

[Kieran Bingham]

Quoting Hans de Goede (2024-02-16 22:32:36)
> Add hblank control so that the sensor has all the mandatory
> controls for libcamera.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/ov2680.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index 4c9db83d876e..fef9e11ee141 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -130,6 +130,7 @@ struct ov2680_ctrls {
>         struct v4l2_ctrl *link_freq;
>         struct v4l2_ctrl *pixel_rate;
>         struct v4l2_ctrl *vblank;
> +       struct v4l2_ctrl *hblank;
>  };
>  
>  struct ov2680_mode {
> @@ -684,6 +685,10 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
>         /* exposure range depends on vts which may have changed */
>         ov2680_exposure_update_range(sensor);
>  
> +       /* adjust hblank value for new width */
> +       def = OV2680_PIXELS_PER_LINE - width;

I presume HTS 'could' be modified ... but I think it's reasonable to
keep it fixed.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +       __v4l2_ctrl_modify_range(sensor->ctrls.hblank, def, def, 1, def);
> +
>  unlock:
>         mutex_unlock(&sensor->lock);
>  
> @@ -984,6 +989,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>         ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
>                                           OV2680_MIN_VBLANK, max, 1, def);
>  
> +       def = OV2680_PIXELS_PER_LINE - OV2680_DEFAULT_WIDTH;
> +       ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> +                                         def, def, 1, def);
> +       if (ctrls->hblank)
> +               ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
>         if (hdl->error) {
>                 ret = hdl->error;
>                 goto cleanup_entity;
> -- 
> 2.43.0
>

Re: [PATCH 5/5] media: ov2680: Add camera orientation and sensor rotation controls

[Kieran Bingham]

Quoting Hans de Goede (2024-02-16 22:32:37)
> Add camera orientation and sensor rotation controls using
> the v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties()
> helpers.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  drivers/media/i2c/ov2680.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index fef9e11ee141..bcd031882a37 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -946,6 +946,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>         const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
>         struct ov2680_ctrls *ctrls = &sensor->ctrls;
>         struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> +       struct v4l2_fwnode_device_properties props;
>         int def, max, ret = 0;
>  
>         v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops);
> @@ -1000,6 +1001,14 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>                 goto cleanup_entity;
>         }
>  
> +       ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> +       if (ret)
> +               goto cleanup_entity;
> +
> +       ret = v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> +       if (ret)
> +               goto cleanup_entity;
> +
>         ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>         ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>         ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> -- 
> 2.43.0
>

Re: [PATCH 1/5] media: ov2680: Stop sending more data then requested

[Hans de Goede]

Hi,

Sorry for being very slow with replying to this.

On 2/17/24 4:38 PM, Kieran Bingham wrote:
> Quoting Hans de Goede (2024-02-16 22:32:33)
>> There is no reason to send OV2680_END_MARGIN extra columns on top of
>> the mode width and the same for sending extra lines over the mode height.
>>
>> This sending of extra lines/columns was inherited from the atomisp
>> ov2680 driver, it is unclear why this was done and this complicates
>> adding V4L2_CID_VBLANK support, so remove it.
> 
> Was this some niave way of adding some HBLANK to let the AtomISP keep up
> with processing each line?

The total amount of pixels per line and of lines per frame are fixed:

#define OV2680_PIXELS_PER_LINE                 1704
#define OV2680_LINES_PER_FRAME                 1294

This patch only changes the h_end and v_end registers which
before AFAIK configure when to stop sending actual pixel
data (and move over to sending blanking data). So this was
actually requesting for more pixels to be send then there are.

> Or is it an optical black region? or padding? (I hit issues around that
> on the IMX283 lately).

AFAICT (from the datasheet) there is no optical black region, so
this really just seemed some weirdness in the original Android
kernel driver where this is derived from.

The datasheet says:

"2.4 pixel array addresses
The addressable pixel array of the OV2680 sensor is 1616 x 1216. The addressed region of the pixel array is controlled
by the horizontal_start, vertical_start, horizontal_end and vertical_end registers. The start and end addresses are limited
to even and odd numbers, respectively, to ensure that there is always an even number of pixels read out in x and y."

> Is this a sensor you have and can visually check?

Yes this is a sensor which I have, not sure what you mean with
visually check. The atomisp driver does not allow getting the full
resolution as the ISP needs some padding. So the max I can get
is 1600x1200. I think the original Android code just added
the 16 extra pixels needed by the ISP to h_end and v_end
twice. Starting with the extra 16 pixels which are actually
there on the sensor and then adding an extra 16 which are
fully made up by the driver author and somehow this still
works (I guess the sensor just sends all 0 data for these).


> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/i2c/ov2680.c | 9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
>> index 39d321e2b7f9..5b04c6c0554a 100644
>> --- a/drivers/media/i2c/ov2680.c
>> +++ b/drivers/media/i2c/ov2680.c
>> @@ -86,9 +86,6 @@
>>  #define OV2680_PIXELS_PER_LINE                 1704
>>  #define OV2680_LINES_PER_FRAME                 1294
>>  
>> -/* If possible send 16 extra rows / lines to the ISP as padding */
>> -#define OV2680_END_MARGIN                      16
>> -
>>  /* Max exposure time is VTS - 8 */
>>  #define OV2680_INTEGRATION_TIME_MARGIN         8
>>  
>> @@ -359,11 +356,9 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
>>         sensor->mode.v_start = (sensor->mode.crop.top +
>>                                 (sensor->mode.crop.height - height) / 2) & ~1;
>>         sensor->mode.h_end =
>> -               min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1,
>> -                   OV2680_NATIVE_WIDTH - 1);
>> +               min(sensor->mode.h_start + width - 1, OV2680_NATIVE_WIDTH - 1);
>>         sensor->mode.v_end =
>> -               min(sensor->mode.v_start + height + OV2680_END_MARGIN - 1,
>> -                   OV2680_NATIVE_HEIGHT - 1);
>> +               min(sensor->mode.v_start + height - 1, OV2680_NATIVE_HEIGHT - 1);
>>         sensor->mode.h_output_size = orig_width;
>>         sensor->mode.v_output_size = orig_height;
>>         sensor->mode.hts = OV2680_PIXELS_PER_LINE;
> 
> Especially as seeing hts = OV2680_PIXELS_PER_LINE it does sound like the
> margin is superfluous.

Right.

Regards,

Hans

Re: [PATCH 1/5] media: ov2680: Stop sending more data then requested

[Kieran Bingham]

Quoting Hans de Goede (2024-04-10 12:27:03)
> Hi,
> 
> Sorry for being very slow with replying to this.
> 

No worries,


> On 2/17/24 4:38 PM, Kieran Bingham wrote:
> > Quoting Hans de Goede (2024-02-16 22:32:33)
> >> There is no reason to send OV2680_END_MARGIN extra columns on top of
> >> the mode width and the same for sending extra lines over the mode height.
> >>
> >> This sending of extra lines/columns was inherited from the atomisp
> >> ov2680 driver, it is unclear why this was done and this complicates
> >> adding V4L2_CID_VBLANK support, so remove it.
> > 
> > Was this some niave way of adding some HBLANK to let the AtomISP keep up
> > with processing each line?
> 
> The total amount of pixels per line and of lines per frame are fixed:
> 
> #define OV2680_PIXELS_PER_LINE                 1704
> #define OV2680_LINES_PER_FRAME                 1294
> 
> This patch only changes the h_end and v_end registers which
> before AFAIK configure when to stop sending actual pixel
> data (and move over to sending blanking data). So this was
> actually requesting for more pixels to be send then there are.
> 
> > Or is it an optical black region? or padding? (I hit issues around that
> > on the IMX283 lately).
> 
> AFAICT (from the datasheet) there is no optical black region, so
> this really just seemed some weirdness in the original Android
> kernel driver where this is derived from.
> 
> The datasheet says:
> 
> "2.4 pixel array addresses
> The addressable pixel array of the OV2680 sensor is 1616 x 1216. The addressed region of the pixel array is controlled
> by the horizontal_start, vertical_start, horizontal_end and vertical_end registers. The start and end addresses are limited
> to even and odd numbers, respectively, to ensure that there is always an even number of pixels read out in x and y."
> 
> > Is this a sensor you have and can visually check?
> 
> Yes this is a sensor which I have, not sure what you mean with
> visually check. The atomisp driver does not allow getting the full

Me neither at this point. I was probably worried about the impact of
changing these values causing visible issues in the stride/line widths
or such. But if you're testing and capturing images successfully I'm not
worried now.


> resolution as the ISP needs some padding. So the max I can get
> is 1600x1200. I think the original Android code just added
> the 16 extra pixels needed by the ISP to h_end and v_end
> twice. Starting with the extra 16 pixels which are actually
> there on the sensor and then adding an extra 16 which are
> fully made up by the driver author and somehow this still
> works (I guess the sensor just sends all 0 data for these).

Or maybe that was the part to check visually ;-) But I guess it's not
easy to capture the full raw images for these ?

> > 
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/media/i2c/ov2680.c | 9 ++-------
> >>  1 file changed, 2 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> >> index 39d321e2b7f9..5b04c6c0554a 100644
> >> --- a/drivers/media/i2c/ov2680.c
> >> +++ b/drivers/media/i2c/ov2680.c
> >> @@ -86,9 +86,6 @@
> >>  #define OV2680_PIXELS_PER_LINE                 1704
> >>  #define OV2680_LINES_PER_FRAME                 1294
> >>  
> >> -/* If possible send 16 extra rows / lines to the ISP as padding */
> >> -#define OV2680_END_MARGIN                      16
> >> -
> >>  /* Max exposure time is VTS - 8 */
> >>  #define OV2680_INTEGRATION_TIME_MARGIN         8
> >>  
> >> @@ -359,11 +356,9 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
> >>         sensor->mode.v_start = (sensor->mode.crop.top +
> >>                                 (sensor->mode.crop.height - height) / 2) & ~1;
> >>         sensor->mode.h_end =
> >> -               min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1,
> >> -                   OV2680_NATIVE_WIDTH - 1);
> >> +               min(sensor->mode.h_start + width - 1, OV2680_NATIVE_WIDTH - 1);
> >>         sensor->mode.v_end =
> >> -               min(sensor->mode.v_start + height + OV2680_END_MARGIN - 1,
> >> -                   OV2680_NATIVE_HEIGHT - 1);
> >> +               min(sensor->mode.v_start + height - 1, OV2680_NATIVE_HEIGHT - 1);
> >>         sensor->mode.h_output_size = orig_width;
> >>         sensor->mode.v_output_size = orig_height;
> >>         sensor->mode.hts = OV2680_PIXELS_PER_LINE;
> > 
> > Especially as seeing hts = OV2680_PIXELS_PER_LINE it does sound like the
> > margin is superfluous.
> 
> Right.
> 
> Regards,
> 
> Hans
> 
>

Re: [PATCH 1/5] media: ov2680: Stop sending more data then requested

[Hans de Goede]

Hi,

On 4/10/24 11:24 PM, Kieran Bingham wrote:
> Quoting Hans de Goede (2024-04-10 12:27:03)
>> Hi,
>>
>> Sorry for being very slow with replying to this.
>>
> 
> No worries,
> 
> 
>> On 2/17/24 4:38 PM, Kieran Bingham wrote:
>>> Quoting Hans de Goede (2024-02-16 22:32:33)
>>>> There is no reason to send OV2680_END_MARGIN extra columns on top of
>>>> the mode width and the same for sending extra lines over the mode height.
>>>>
>>>> This sending of extra lines/columns was inherited from the atomisp
>>>> ov2680 driver, it is unclear why this was done and this complicates
>>>> adding V4L2_CID_VBLANK support, so remove it.
>>>
>>> Was this some niave way of adding some HBLANK to let the AtomISP keep up
>>> with processing each line?
>>
>> The total amount of pixels per line and of lines per frame are fixed:
>>
>> #define OV2680_PIXELS_PER_LINE                 1704
>> #define OV2680_LINES_PER_FRAME                 1294
>>
>> This patch only changes the h_end and v_end registers which
>> before AFAIK configure when to stop sending actual pixel
>> data (and move over to sending blanking data). So this was
>> actually requesting for more pixels to be send then there are.
>>
>>> Or is it an optical black region? or padding? (I hit issues around that
>>> on the IMX283 lately).
>>
>> AFAICT (from the datasheet) there is no optical black region, so
>> this really just seemed some weirdness in the original Android
>> kernel driver where this is derived from.
>>
>> The datasheet says:
>>
>> "2.4 pixel array addresses
>> The addressable pixel array of the OV2680 sensor is 1616 x 1216. The addressed region of the pixel array is controlled
>> by the horizontal_start, vertical_start, horizontal_end and vertical_end registers. The start and end addresses are limited
>> to even and odd numbers, respectively, to ensure that there is always an even number of pixels read out in x and y."
>>
>>> Is this a sensor you have and can visually check?
>>
>> Yes this is a sensor which I have, not sure what you mean with
>> visually check. The atomisp driver does not allow getting the full
> 
> Me neither at this point. I was probably worried about the impact of
> changing these values causing visible issues in the stride/line widths
> or such. But if you're testing and capturing images successfully I'm not
> worried now.
> 
> 
>> resolution as the ISP needs some padding. So the max I can get
>> is 1600x1200. I think the original Android code just added
>> the 16 extra pixels needed by the ISP to h_end and v_end
>> twice. Starting with the extra 16 pixels which are actually
>> there on the sensor and then adding an extra 16 which are
>> fully made up by the driver author and somehow this still
>> works (I guess the sensor just sends all 0 data for these).
> 
> Or maybe that was the part to check visually ;-) But I guess it's not
> easy to capture the full raw images for these ?

Besides a bunch of devices with the atomisp I also have 1 IPU3
based device with an ov2680 sensor. So I could capture full
raw resolution there. But unless I modify the driver full
raw resolution is 1616x1216 where as before this patch the driver
sets v_end to 1631 so more pixels then the full size, which is
the weirdness this patch corrects.

Regards,

Hans




> 
>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/media/i2c/ov2680.c | 9 ++-------
>>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
>>>> index 39d321e2b7f9..5b04c6c0554a 100644
>>>> --- a/drivers/media/i2c/ov2680.c
>>>> +++ b/drivers/media/i2c/ov2680.c
>>>> @@ -86,9 +86,6 @@
>>>>  #define OV2680_PIXELS_PER_LINE                 1704
>>>>  #define OV2680_LINES_PER_FRAME                 1294
>>>>  
>>>> -/* If possible send 16 extra rows / lines to the ISP as padding */
>>>> -#define OV2680_END_MARGIN                      16
>>>> -
>>>>  /* Max exposure time is VTS - 8 */
>>>>  #define OV2680_INTEGRATION_TIME_MARGIN         8
>>>>  
>>>> @@ -359,11 +356,9 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
>>>>         sensor->mode.v_start = (sensor->mode.crop.top +
>>>>                                 (sensor->mode.crop.height - height) / 2) & ~1;
>>>>         sensor->mode.h_end =
>>>> -               min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1,
>>>> -                   OV2680_NATIVE_WIDTH - 1);
>>>> +               min(sensor->mode.h_start + width - 1, OV2680_NATIVE_WIDTH - 1);
>>>>         sensor->mode.v_end =
>>>> -               min(sensor->mode.v_start + height + OV2680_END_MARGIN - 1,
>>>> -                   OV2680_NATIVE_HEIGHT - 1);
>>>> +               min(sensor->mode.v_start + height - 1, OV2680_NATIVE_HEIGHT - 1);
>>>>         sensor->mode.h_output_size = orig_width;
>>>>         sensor->mode.v_output_size = orig_height;
>>>>         sensor->mode.hts = OV2680_PIXELS_PER_LINE;
>>>
>>> Especially as seeing hts = OV2680_PIXELS_PER_LINE it does sound like the
>>> margin is superfluous.
>>
>> Right.
>>
>> Regards,
>>
>> Hans
>>
>>
> 

Re: [PATCH 1/5] media: ov2680: Stop sending more data then requested

[Kieran Bingham]

Hi Hans,

Quoting Hans de Goede (2024-04-11 13:19:15)
> Hi,
> 
> On 4/10/24 11:24 PM, Kieran Bingham wrote:
> > Quoting Hans de Goede (2024-04-10 12:27:03)
> >> Hi,
> >>
> >> Sorry for being very slow with replying to this.
> >>
> > 
> > No worries,
> > 
> > 
> >> On 2/17/24 4:38 PM, Kieran Bingham wrote:
> >>> Quoting Hans de Goede (2024-02-16 22:32:33)
> >>>> There is no reason to send OV2680_END_MARGIN extra columns on top of
> >>>> the mode width and the same for sending extra lines over the mode height.
> >>>>
> >>>> This sending of extra lines/columns was inherited from the atomisp
> >>>> ov2680 driver, it is unclear why this was done and this complicates
> >>>> adding V4L2_CID_VBLANK support, so remove it.
> >>>
> >>> Was this some niave way of adding some HBLANK to let the AtomISP keep up
> >>> with processing each line?
> >>
> >> The total amount of pixels per line and of lines per frame are fixed:
> >>
> >> #define OV2680_PIXELS_PER_LINE                 1704
> >> #define OV2680_LINES_PER_FRAME                 1294
> >>
> >> This patch only changes the h_end and v_end registers which
> >> before AFAIK configure when to stop sending actual pixel
> >> data (and move over to sending blanking data). So this was
> >> actually requesting for more pixels to be send then there are.
> >>
> >>> Or is it an optical black region? or padding? (I hit issues around that
> >>> on the IMX283 lately).
> >>
> >> AFAICT (from the datasheet) there is no optical black region, so
> >> this really just seemed some weirdness in the original Android
> >> kernel driver where this is derived from.
> >>
> >> The datasheet says:
> >>
> >> "2.4 pixel array addresses
> >> The addressable pixel array of the OV2680 sensor is 1616 x 1216. The addressed region of the pixel array is controlled
> >> by the horizontal_start, vertical_start, horizontal_end and vertical_end registers. The start and end addresses are limited
> >> to even and odd numbers, respectively, to ensure that there is always an even number of pixels read out in x and y."
> >>
> >>> Is this a sensor you have and can visually check?
> >>
> >> Yes this is a sensor which I have, not sure what you mean with
> >> visually check. The atomisp driver does not allow getting the full
> > 
> > Me neither at this point. I was probably worried about the impact of
> > changing these values causing visible issues in the stride/line widths
> > or such. But if you're testing and capturing images successfully I'm not
> > worried now.
> > 
> > 
> >> resolution as the ISP needs some padding. So the max I can get
> >> is 1600x1200. I think the original Android code just added
> >> the 16 extra pixels needed by the ISP to h_end and v_end
> >> twice. Starting with the extra 16 pixels which are actually
> >> there on the sensor and then adding an extra 16 which are
> >> fully made up by the driver author and somehow this still
> >> works (I guess the sensor just sends all 0 data for these).
> > 
> > Or maybe that was the part to check visually ;-) But I guess it's not
> > easy to capture the full raw images for these ?
> 
> Besides a bunch of devices with the atomisp I also have 1 IPU3
> based device with an ov2680 sensor. So I could capture full
> raw resolution there. But unless I modify the driver full
> raw resolution is 1616x1216 where as before this patch the driver
> sets v_end to 1631 so more pixels then the full size, which is
> the weirdness this patch corrects.

That sounds reasonable to me, thank you for all the explanations.

I don't object to this patch, but I don't have a way to test, nor
further verify this one myself specifically.

But, the commit does reflect what the commit message states ... so I
guess that's a ...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> Regards,
> 
> Hans
> 
> 
> 
> 
> > 
> >>>
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>> ---
> >>>>  drivers/media/i2c/ov2680.c | 9 ++-------
> >>>>  1 file changed, 2 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> >>>> index 39d321e2b7f9..5b04c6c0554a 100644
> >>>> --- a/drivers/media/i2c/ov2680.c
> >>>> +++ b/drivers/media/i2c/ov2680.c
> >>>> @@ -86,9 +86,6 @@
> >>>>  #define OV2680_PIXELS_PER_LINE                 1704
> >>>>  #define OV2680_LINES_PER_FRAME                 1294
> >>>>  
> >>>> -/* If possible send 16 extra rows / lines to the ISP as padding */
> >>>> -#define OV2680_END_MARGIN                      16
> >>>> -
> >>>>  /* Max exposure time is VTS - 8 */
> >>>>  #define OV2680_INTEGRATION_TIME_MARGIN         8
> >>>>  
> >>>> @@ -359,11 +356,9 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
> >>>>         sensor->mode.v_start = (sensor->mode.crop.top +
> >>>>                                 (sensor->mode.crop.height - height) / 2) & ~1;
> >>>>         sensor->mode.h_end =
> >>>> -               min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1,
> >>>> -                   OV2680_NATIVE_WIDTH - 1);
> >>>> +               min(sensor->mode.h_start + width - 1, OV2680_NATIVE_WIDTH - 1);
> >>>>         sensor->mode.v_end =
> >>>> -               min(sensor->mode.v_start + height + OV2680_END_MARGIN - 1,
> >>>> -                   OV2680_NATIVE_HEIGHT - 1);
> >>>> +               min(sensor->mode.v_start + height - 1, OV2680_NATIVE_HEIGHT - 1);
> >>>>         sensor->mode.h_output_size = orig_width;
> >>>>         sensor->mode.v_output_size = orig_height;
> >>>>         sensor->mode.hts = OV2680_PIXELS_PER_LINE;
> >>>
> >>> Especially as seeing hts = OV2680_PIXELS_PER_LINE it does sound like the
> >>> margin is superfluous.
> >>
> >> Right.
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> > 
>

Re: [PATCH 1/5] media: ov2680: Stop sending more data then requested

[Hans de Goede]

Hi Kieran,

On 4/15/24 12:03 AM, Kieran Bingham wrote:
> Hi Hans,
> 
> Quoting Hans de Goede (2024-04-11 13:19:15)
>> Hi,
>>
>> On 4/10/24 11:24 PM, Kieran Bingham wrote:
>>> Quoting Hans de Goede (2024-04-10 12:27:03)
>>>> Hi,
>>>>
>>>> Sorry for being very slow with replying to this.
>>>>
>>>
>>> No worries,
>>>
>>>
>>>> On 2/17/24 4:38 PM, Kieran Bingham wrote:
>>>>> Quoting Hans de Goede (2024-02-16 22:32:33)
>>>>>> There is no reason to send OV2680_END_MARGIN extra columns on top of
>>>>>> the mode width and the same for sending extra lines over the mode height.
>>>>>>
>>>>>> This sending of extra lines/columns was inherited from the atomisp
>>>>>> ov2680 driver, it is unclear why this was done and this complicates
>>>>>> adding V4L2_CID_VBLANK support, so remove it.
>>>>>
>>>>> Was this some niave way of adding some HBLANK to let the AtomISP keep up
>>>>> with processing each line?
>>>>
>>>> The total amount of pixels per line and of lines per frame are fixed:
>>>>
>>>> #define OV2680_PIXELS_PER_LINE                 1704
>>>> #define OV2680_LINES_PER_FRAME                 1294
>>>>
>>>> This patch only changes the h_end and v_end registers which
>>>> before AFAIK configure when to stop sending actual pixel
>>>> data (and move over to sending blanking data). So this was
>>>> actually requesting for more pixels to be send then there are.
>>>>
>>>>> Or is it an optical black region? or padding? (I hit issues around that
>>>>> on the IMX283 lately).
>>>>
>>>> AFAICT (from the datasheet) there is no optical black region, so
>>>> this really just seemed some weirdness in the original Android
>>>> kernel driver where this is derived from.
>>>>
>>>> The datasheet says:
>>>>
>>>> "2.4 pixel array addresses
>>>> The addressable pixel array of the OV2680 sensor is 1616 x 1216. The addressed region of the pixel array is controlled
>>>> by the horizontal_start, vertical_start, horizontal_end and vertical_end registers. The start and end addresses are limited
>>>> to even and odd numbers, respectively, to ensure that there is always an even number of pixels read out in x and y."
>>>>
>>>>> Is this a sensor you have and can visually check?
>>>>
>>>> Yes this is a sensor which I have, not sure what you mean with
>>>> visually check. The atomisp driver does not allow getting the full
>>>
>>> Me neither at this point. I was probably worried about the impact of
>>> changing these values causing visible issues in the stride/line widths
>>> or such. But if you're testing and capturing images successfully I'm not
>>> worried now.
>>>
>>>
>>>> resolution as the ISP needs some padding. So the max I can get
>>>> is 1600x1200. I think the original Android code just added
>>>> the 16 extra pixels needed by the ISP to h_end and v_end
>>>> twice. Starting with the extra 16 pixels which are actually
>>>> there on the sensor and then adding an extra 16 which are
>>>> fully made up by the driver author and somehow this still
>>>> works (I guess the sensor just sends all 0 data for these).
>>>
>>> Or maybe that was the part to check visually ;-) But I guess it's not
>>> easy to capture the full raw images for these ?
>>
>> Besides a bunch of devices with the atomisp I also have 1 IPU3
>> based device with an ov2680 sensor. So I could capture full
>> raw resolution there. But unless I modify the driver full
>> raw resolution is 1616x1216 where as before this patch the driver
>> sets v_end to 1631 so more pixels then the full size, which is
>> the weirdness this patch corrects.
> 
> That sounds reasonable to me, thank you for all the explanations.
> 
> I don't object to this patch, but I don't have a way to test, nor
> further verify this one myself specifically.
> 
> But, the commit does reflect what the commit message states ... so I
> guess that's a ...
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Thank you.

Since this patch set is quite old by now I'll go and prepare a
resend with all the collected tags soon.

Regards,

Hans



>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>>  drivers/media/i2c/ov2680.c | 9 ++-------
>>>>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
>>>>>> index 39d321e2b7f9..5b04c6c0554a 100644
>>>>>> --- a/drivers/media/i2c/ov2680.c
>>>>>> +++ b/drivers/media/i2c/ov2680.c
>>>>>> @@ -86,9 +86,6 @@
>>>>>>  #define OV2680_PIXELS_PER_LINE                 1704
>>>>>>  #define OV2680_LINES_PER_FRAME                 1294
>>>>>>  
>>>>>> -/* If possible send 16 extra rows / lines to the ISP as padding */
>>>>>> -#define OV2680_END_MARGIN                      16
>>>>>> -
>>>>>>  /* Max exposure time is VTS - 8 */
>>>>>>  #define OV2680_INTEGRATION_TIME_MARGIN         8
>>>>>>  
>>>>>> @@ -359,11 +356,9 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
>>>>>>         sensor->mode.v_start = (sensor->mode.crop.top +
>>>>>>                                 (sensor->mode.crop.height - height) / 2) & ~1;
>>>>>>         sensor->mode.h_end =
>>>>>> -               min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1,
>>>>>> -                   OV2680_NATIVE_WIDTH - 1);
>>>>>> +               min(sensor->mode.h_start + width - 1, OV2680_NATIVE_WIDTH - 1);
>>>>>>         sensor->mode.v_end =
>>>>>> -               min(sensor->mode.v_start + height + OV2680_END_MARGIN - 1,
>>>>>> -                   OV2680_NATIVE_HEIGHT - 1);
>>>>>> +               min(sensor->mode.v_start + height - 1, OV2680_NATIVE_HEIGHT - 1);
>>>>>>         sensor->mode.h_output_size = orig_width;
>>>>>>         sensor->mode.v_output_size = orig_height;
>>>>>>         sensor->mode.hts = OV2680_PIXELS_PER_LINE;
>>>>>
>>>>> Especially as seeing hts = OV2680_PIXELS_PER_LINE it does sound like the
>>>>> margin is superfluous.
>>>>
>>>> Right.
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>
>>
>