[PATCH 1/2] media: i2c: ov08x40: Implement the selection API

[Pierre Pinon]

ov08x40 implements none of the selection targets: its pad ops carry only
enum_mbus_code, get_fmt, set_fmt and enum_frame_size, and ov08x40_open()
states "No crop or compose" outright.

libcamera requires V4L2_SEL_TGT_CROP_BOUNDS, V4L2_SEL_TGT_CROP_DEFAULT
and V4L2_SEL_TGT_CROP from a sensor driver
(Documentation/sensor_driver_requirements in the libcamera sources), and
warns that support is scheduled to become mandatory. Without them it
falls back to "crop == active area" for every mode:

  'ov08x40 18-0036': Failed to retrieve the sensor crop rectangle
  'ov08x40 18-0036': The sensor kernel driver needs to be fixed

The practical consequence is worse than a warning. Because the crop
rectangle is what distinguishes a binned mode (full-array crop, reduced
output) from a cropped one (reduced crop, 1:1 output), libcamera cannot
tell that 1928x1208 and 1928x1088 are 2x2 binned views of the whole
array. It therefore never selects them, and satisfies small requests by
centre-cropping the full resolution mode instead. On an IPU7 platform
using the CPU software ISP, that path is unusable: the debayer window
desyncs from the real line stride and every non-native resolution
produces a uniformly saturated image.

The mode register lists program only the low bytes of the vertical
window (0x3803, 0x3807) and the output size (0x3808-0x380b). The
horizontal window (0x3800/0x3801, 0x3804/0x3805) and the high bytes of
the vertical window (0x3802, 0x3806) are never written and keep their
power-on values, so they cannot be derived from the driver source. They
were read back over I2C from the sensor on a Dell Pro 14 Premium PA14260
and are identical in every mode:

  0x3800/0x3801 = 0x0000     x_start = 0
  0x3804/0x3805 = 0x0f1f     x_end   = 3871
  0x3802        = 0x00       y_start high byte
  0x3806        = 0x09       y_end high byte

Combining those with the per-mode low bytes gives:

  mode         crop (left, top, w, h)   output       binning
  3856x2416    (0,   0, 3872, 2432)     3856x2416    1:1
  3856x2176    (0, 112, 3872, 2208)     3856x2176    1:1
  1928x1208    (0,   0, 3872, 2432)     1928x1208    2x2
  1928x1088    (0, 120, 3872, 2192)     1928x1088    2x2

The margins are consistent throughout: 16 columns and 16 or 32 rows for
the 1:1 modes, and exactly 8 columns and 8 rows once the binned modes
are scaled down, which supports the reading of the high bytes above.

Report the full readable array as 3872x2432 for V4L2_SEL_TGT_CROP_BOUNDS
and V4L2_SEL_TGT_NATIVE_SIZE, and the largest mode's crop for
V4L2_SEL_TGT_CROP_DEFAULT.

Two caveats, since I have no datasheet for this sensor. First, only the
3856x2176 and 1928x1088 modes could be exercised on this hardware --
libcamera never selects the other three -- so their register values were
read directly and the remaining rows are derived from the same measured
high bytes. Second, this sensor's largest mode reads the whole array, so
CROP_DEFAULT (the active area) and CROP_BOUNDS (the readable area) come
out equal; if OmniVision or Intel can confirm the true active area
differs, CROP_DEFAULT should be narrowed accordingly.

Signed-off-by: Pierre Pinon <pierre@xxxxxxxxx>
---
 drivers/media/i2c/ov08x40.c | 70 +++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/media/i2c/ov08x40.c b/drivers/media/i2c/ov08x40.c
index 5eaf454f4763..3de06e19803c 100644
--- a/drivers/media/i2c/ov08x40.c
+++ b/drivers/media/i2c/ov08x40.c
@@ -17,6 +17,15 @@
 #include <media/v4l2-fwnode.h>
 
 #define OV08X40_REG_VALUE_08BIT		1
+
+/*
+ * Full readable pixel array. The X window registers (0x3800/0x3801,
+ * 0x3804/0x3805) and the high bytes of the Y window (0x3802, 0x3806) are never
+ * programmed by the mode register lists, so they keep their power-on values;
+ * these were read back from the sensor as 0x0000-0x0f1f and 0x00/0x09.
+ */
+#define OV08X40_NATIVE_WIDTH		3872U
+#define OV08X40_NATIVE_HEIGHT		2432U
 #define OV08X40_REG_VALUE_16BIT		2
 #define OV08X40_REG_VALUE_24BIT		3
 
@@ -151,6 +160,9 @@ struct ov08x40_mode {
 	/* Exposure calculation */
 	u16 exposure_margin;
 	u16 exposure_shift;
+
+	/* Analogue crop rectangle, in native pixel array coordinates */
+	struct v4l2_rect crop;
 };
 
 static const struct ov08x40_reg ov08x40_global_regs[] = {
@@ -1232,6 +1244,12 @@ static const struct ov08x40_mode supported_modes[] = {
 			.num_of_regs = ARRAY_SIZE(mode_3856x2416_regs),
 			.regs = mode_3856x2416_regs,
 		},
+		.crop = {
+			.left = 0,
+			.top = 0,
+			.width = 3872,
+			.height = 2432,
+		},
 		.link_freq_index = OV08X40_LINK_FREQ_400MHZ_INDEX,
 		.exposure_shift = 1,
 		.exposure_margin = OV08X40_EXPOSURE_MAX_MARGIN,
@@ -1247,6 +1265,12 @@ static const struct ov08x40_mode supported_modes[] = {
 			.num_of_regs = ARRAY_SIZE(mode_3856x2176_regs_800mbps),
 			.regs = mode_3856x2176_regs_800mbps,
 		},
+		.crop = {
+			.left = 0,
+			.top = 112,
+			.width = 3872,
+			.height = 2208,
+		},
 		.link_freq_index = OV08X40_LINK_FREQ_400MHZ_INDEX,
 		.exposure_shift = 1,
 		.exposure_margin = OV08X40_EXPOSURE_MAX_MARGIN,
@@ -1263,6 +1287,12 @@ static const struct ov08x40_mode supported_modes[] = {
 			.num_of_regs = ARRAY_SIZE(mode_1928x1208_regs),
 			.regs = mode_1928x1208_regs,
 		},
+		.crop = {
+			.left = 0,
+			.top = 0,
+			.width = 3872,
+			.height = 2432,
+		},
 		.link_freq_index = OV08X40_LINK_FREQ_400MHZ_INDEX,
 		.exposure_shift = 0,
 		.exposure_margin = OV08X40_EXPOSURE_BIN_MAX_MARGIN,
@@ -1278,6 +1308,12 @@ static const struct ov08x40_mode supported_modes[] = {
 			.num_of_regs = ARRAY_SIZE(mode_3856x2176_regs_1500mbps),
 			.regs = mode_3856x2176_regs_1500mbps,
 		},
+		.crop = {
+			.left = 0,
+			.top = 112,
+			.width = 3872,
+			.height = 2208,
+		},
 		.link_freq_index = OV08X40_LINK_FREQ_749MHZ_INDEX,
 		.exposure_shift = 1,
 		.exposure_margin = OV08X40_EXPOSURE_MAX_MARGIN,
@@ -1293,6 +1329,12 @@ static const struct ov08x40_mode supported_modes[] = {
 			.num_of_regs = ARRAY_SIZE(mode_1928x1088_regs_1500mbps),
 			.regs = mode_1928x1088_regs_1500mbps,
 		},
+		.crop = {
+			.left = 0,
+			.top = 120,
+			.width = 3872,
+			.height = 2192,
+		},
 		.link_freq_index = OV08X40_LINK_FREQ_749MHZ_INDEX,
 		.exposure_shift = 0,
 		.exposure_margin = OV08X40_EXPOSURE_MAX_MARGIN,
@@ -2055,10 +2097,38 @@ static const struct v4l2_subdev_video_ops ov08x40_video_ops = {
 	.s_stream = ov08x40_set_stream,
 };
 
+static int ov08x40_get_selection(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_selection *sel)
+{
+	struct ov08x40 *ov08x = to_ov08x40(sd);
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		mutex_lock(&ov08x->mutex);
+		sel->r = ov08x->cur_mode->crop;
+		mutex_unlock(&ov08x->mutex);
+		return 0;
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.left = 0;
+		sel->r.top = 0;
+		sel->r.width = OV08X40_NATIVE_WIDTH;
+		sel->r.height = OV08X40_NATIVE_HEIGHT;
+		return 0;
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r = supported_modes[0].crop;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static const struct v4l2_subdev_pad_ops ov08x40_pad_ops = {
 	.enum_mbus_code = ov08x40_enum_mbus_code,
 	.get_fmt = ov08x40_get_pad_format,
 	.set_fmt = ov08x40_set_pad_format,
+	.get_selection = ov08x40_get_selection,
 	.enum_frame_size = ov08x40_enum_frame_size,
 };
 

base-commit: 26cf1fa3d28a98a0cb08ae1929c71f681e5d2dcc
-- 
2.55.0