[PATCH v2 00/16] Updates to ov9282 sensor driver

[PATCH v2 00/16] Updates to ov9282 sensor driver

[Dave Stevenson]

This series adds to the functionality of the Omnivision OV9282 driver to make it
usable with libcamera.

Tested on a Raspberry Pi with OV9281 sensor (same as OV9282 but with alternate
CRA on the optics)

Changes in v2
Collected the relevant R-b tags from Jacopo (many thanks)
Patch 5 - replaced macro OV9282_NUM_MODES with direct use of ARRAY_SIZE (Sakari)
Patch 6 - reworded commit text (Jacopo)
Patch 7 - correct typo (250 instead of 151) (Jacopo)
Patch 8 - moved setting CSI2 clock mode from streamon to power on (Sakari)
Patch 10 - double empty line removed (Jacopo)

No response on patch 11 with regard policy on whether V4L2_CID_VFLIP & HFLIP
should be with regard sensor native orientation or initially submitted driver,
so I've left it as is.

No response on patch 16 as to whether all sensor drivers with controls should
have V4L2_SUBDEV_FL_HAS_EVENTS and subscribe_event/unsubscribe_event handlers,
so again it's unchanged. It fixes a v4l2-compliance failure and therefore would
appear to be valid.

Dave Stevenson (16):
  media: i2c: ov9282: Remove duplication of registers
  media: i2c: ov9282: Split registers into common and mode specific
  media: i2c: ov9282: Remove format code from the mode
  media: i2c: ov9282: Remove pixel rate from mode definition
  media: i2c: ov9281: Support more than 1 mode.
  media: i2c: ov9282: Correct HTS register for configured pixel rate
  media: i2c: ov9282: Reduce vblank_min values based on testing
  media: i2c: ov9282: Add selection for CSI2 clock mode
  media: i2c: ov9282: Add the properties from fwnode
  media: i2c: ov9282: Action CID_VBLANK when set.
  media: i2c: ov9282: Add HFLIP and VFLIP support
  media: i2c: ov9282: Make V4L2_CID_HBLANK r/w
  media: i2c: ov9282: Add selection API calls for cropping info
  media: i2c: ov9282: Add support for 1280x800 and 640x400 modes
  media: i2c: ov9282: Add support for 8bit readout
  media: i2c: ov9282: Support event handlers

 drivers/media/i2c/ov9282.c | 563 ++++++++++++++++++++++++++++++-------
 1 file changed, 456 insertions(+), 107 deletions(-)

-- 
2.34.1

[PATCH v2 01/16] media: i2c: ov9282: Remove duplication of registers

[Dave Stevenson]

TIMING_VTS (registers 0x380e/f), EXPOSURE (registers
0x3500/1/2), and GAIN (0x3509) are all set from
ov9282_update_exp_gain as part of the control handler,
therefore they do not need to be in the main list of
registers.

Remove them.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov9282.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index df144a2f6eda..0c604050b4e5 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -163,14 +163,10 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3030, 0x10},
 	{0x3039, 0x32},
 	{0x303a, 0x00},
-	{0x3500, 0x00},
-	{0x3501, 0x5f},
-	{0x3502, 0x1e},
 	{0x3503, 0x08},
 	{0x3505, 0x8c},
 	{0x3507, 0x03},
 	{0x3508, 0x00},
-	{0x3509, 0x10},
 	{0x3610, 0x80},
 	{0x3611, 0xa0},
 	{0x3620, 0x6e},
@@ -204,8 +200,6 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x380b, 0xd0},
 	{0x380c, 0x05},
 	{0x380d, 0xfa},
-	{0x380e, 0x06},
-	{0x380f, 0xce},
 	{0x3810, 0x00},
 	{0x3811, 0x08},
 	{0x3812, 0x00},
-- 
2.34.1

[PATCH v2 02/16] media: i2c: ov9282: Split registers into common and mode specific

[Dave Stevenson]

Currently only one mode is supported, so all registers were
dropped in one list.
In preparation for adding more modes, split out the common registers
from those which configure the mode.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov9282.c | 77 +++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 30 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 0c604050b4e5..6999ce869a1b 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -147,8 +147,8 @@ static const s64 link_freq[] = {
 	OV9282_LINK_FREQ,
 };
 
-/* Sensor mode registers */
-static const struct ov9282_reg mode_1280x720_regs[] = {
+/* Common registers */
+static const struct ov9282_reg common_regs[] = {
 	{0x0302, 0x32},
 	{0x030d, 0x50},
 	{0x030e, 0x02},
@@ -179,13 +179,49 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x372d, 0x22},
 	{0x3731, 0x80},
 	{0x3732, 0x30},
-	{0x3778, 0x00},
 	{0x377d, 0x22},
 	{0x3788, 0x02},
 	{0x3789, 0xa4},
 	{0x378a, 0x00},
 	{0x378b, 0x4a},
 	{0x3799, 0x20},
+	{0x3881, 0x42},
+	{0x38a8, 0x02},
+	{0x38a9, 0x80},
+	{0x38b1, 0x00},
+	{0x38c4, 0x00},
+	{0x38c5, 0xc0},
+	{0x38c6, 0x04},
+	{0x38c7, 0x80},
+	{0x3920, 0xff},
+	{0x4010, 0x40},
+	{0x4043, 0x40},
+	{0x4307, 0x30},
+	{0x4317, 0x00},
+	{0x4501, 0x00},
+	{0x450a, 0x08},
+	{0x4601, 0x04},
+	{0x470f, 0x00},
+	{0x4f07, 0x00},
+	{0x4800, 0x20},
+	{0x5000, 0x9f},
+	{0x5001, 0x00},
+	{0x5e00, 0x00},
+	{0x5d00, 0x07},
+	{0x5d01, 0x00},
+	{0x0101, 0x01},
+	{0x1000, 0x03},
+	{0x5a08, 0x84},
+};
+
+struct ov9282_reg_list common_regs_list = {
+	.num_of_regs = ARRAY_SIZE(common_regs),
+	.regs = common_regs,
+};
+
+/* Sensor mode registers */
+static const struct ov9282_reg mode_1280x720_regs[] = {
+	{0x3778, 0x00},
 	{0x3800, 0x00},
 	{0x3801, 0x00},
 	{0x3802, 0x00},
@@ -208,40 +244,13 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3815, 0x11},
 	{0x3820, 0x3c},
 	{0x3821, 0x84},
-	{0x3881, 0x42},
-	{0x38a8, 0x02},
-	{0x38a9, 0x80},
-	{0x38b1, 0x00},
-	{0x38c4, 0x00},
-	{0x38c5, 0xc0},
-	{0x38c6, 0x04},
-	{0x38c7, 0x80},
-	{0x3920, 0xff},
 	{0x4003, 0x40},
 	{0x4008, 0x02},
 	{0x4009, 0x05},
 	{0x400c, 0x00},
 	{0x400d, 0x03},
-	{0x4010, 0x40},
-	{0x4043, 0x40},
-	{0x4307, 0x30},
-	{0x4317, 0x00},
-	{0x4501, 0x00},
 	{0x4507, 0x00},
 	{0x4509, 0x80},
-	{0x450a, 0x08},
-	{0x4601, 0x04},
-	{0x470f, 0x00},
-	{0x4f07, 0x00},
-	{0x4800, 0x20},
-	{0x5000, 0x9f},
-	{0x5001, 0x00},
-	{0x5e00, 0x00},
-	{0x5d00, 0x07},
-	{0x5d01, 0x00},
-	{0x0101, 0x01},
-	{0x1000, 0x03},
-	{0x5a08, 0x84},
 };
 
 /* Supported sensor mode configurations */
@@ -653,6 +662,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
 	const struct ov9282_reg_list *reg_list;
 	int ret;
 
+	/* Write common registers */
+	ret = ov9282_write_regs(ov9282, common_regs_list.regs,
+				common_regs_list.num_of_regs);
+	if (ret) {
+		dev_err(ov9282->dev, "fail to write common registers");
+		return ret;
+	}
+
 	/* Write sensor mode registers */
 	reg_list = &ov9282->cur_mode->reg_list;
 	ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
-- 
2.34.1

[PATCH v2 03/16] media: i2c: ov9282: Remove format code from the mode

[Dave Stevenson]

The format code is independent of mode, and each mode could
support both Y10 and Y8, so disassociate the code from the
mode.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov9282.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 6999ce869a1b..ead3a4f22ef8 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -79,7 +79,6 @@ struct ov9282_reg_list {
  * struct ov9282_mode - ov9282 sensor mode structure
  * @width: Frame width
  * @height: Frame height
- * @code: Format code
  * @hblank: Horizontal blanking in lines
  * @vblank: Vertical blanking in lines
  * @vblank_min: Minimum vertical blanking in lines
@@ -91,7 +90,6 @@ struct ov9282_reg_list {
 struct ov9282_mode {
 	u32 width;
 	u32 height;
-	u32 code;
 	u32 hblank;
 	u32 vblank;
 	u32 vblank_min;
@@ -263,7 +261,6 @@ static const struct ov9282_mode supported_mode = {
 	.vblank_max = 51540,
 	.pclk = 160000000,
 	.link_freq_idx = 0,
-	.code = MEDIA_BUS_FMT_Y10_1X10,
 	.reg_list = {
 		.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
 		.regs = mode_1280x720_regs,
@@ -513,7 +510,7 @@ static int ov9282_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index > 0)
 		return -EINVAL;
 
-	code->code = supported_mode.code;
+	code->code = MEDIA_BUS_FMT_Y10_1X10;
 
 	return 0;
 }
@@ -533,7 +530,7 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
 	if (fsize->index > 0)
 		return -EINVAL;
 
-	if (fsize->code != supported_mode.code)
+	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
 		return -EINVAL;
 
 	fsize->min_width = supported_mode.width;
@@ -557,7 +554,7 @@ static void ov9282_fill_pad_format(struct ov9282 *ov9282,
 {
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
-	fmt->format.code = mode->code;
+	fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
 	fmt->format.field = V4L2_FIELD_NONE;
 	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
 	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
-- 
2.34.1

[PATCH v2 04/16] media: i2c: ov9282: Remove pixel rate from mode definition

[Dave Stevenson]

The pixel rate is determined by the PLL setup in the common
registers, not by the mode specific registers, therefore
remove it from the mode definition and define it for all modes.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov9282.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index ead3a4f22ef8..123aa20951b7 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -52,6 +52,10 @@
 #define OV9282_LINK_FREQ	400000000
 #define OV9282_NUM_DATA_LANES	2
 
+/* Pixel rate */
+#define OV9282_PIXEL_RATE	(OV9282_LINK_FREQ * 2 * \
+				 OV9282_NUM_DATA_LANES / 10)
+
 #define OV9282_REG_MIN		0x00
 #define OV9282_REG_MAX		0xfffff
 
@@ -83,7 +87,6 @@ struct ov9282_reg_list {
  * @vblank: Vertical blanking in lines
  * @vblank_min: Minimum vertical blanking in lines
  * @vblank_max: Maximum vertical blanking in lines
- * @pclk: Sensor pixel clock
  * @link_freq_idx: Link frequency index
  * @reg_list: Register list for sensor mode
  */
@@ -94,7 +97,6 @@ struct ov9282_mode {
 	u32 vblank;
 	u32 vblank_min;
 	u32 vblank_max;
-	u64 pclk;
 	u32 link_freq_idx;
 	struct ov9282_reg_list reg_list;
 };
@@ -109,7 +111,6 @@ struct ov9282_mode {
  * @inclk: Sensor input clock
  * @ctrl_handler: V4L2 control handler
  * @link_freq_ctrl: Pointer to link frequency control
- * @pclk_ctrl: Pointer to pixel clock control
  * @hblank_ctrl: Pointer to horizontal blanking control
  * @vblank_ctrl: Pointer to vertical blanking control
  * @exp_ctrl: Pointer to exposure control
@@ -128,7 +129,6 @@ struct ov9282 {
 	struct clk *inclk;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_ctrl *link_freq_ctrl;
-	struct v4l2_ctrl *pclk_ctrl;
 	struct v4l2_ctrl *hblank_ctrl;
 	struct v4l2_ctrl *vblank_ctrl;
 	struct {
@@ -259,7 +259,6 @@ static const struct ov9282_mode supported_mode = {
 	.vblank = 1022,
 	.vblank_min = 151,
 	.vblank_max = 51540,
-	.pclk = 160000000,
 	.link_freq_idx = 0,
 	.reg_list = {
 		.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
@@ -968,11 +967,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 						1, mode->vblank);
 
 	/* Read only controls */
-	ov9282->pclk_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
-					      &ov9282_ctrl_ops,
-					      V4L2_CID_PIXEL_RATE,
-					      mode->pclk, mode->pclk,
-					      1, mode->pclk);
+	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
+			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
+			  OV9282_PIXEL_RATE);
 
 	ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
 							&ov9282_ctrl_ops,
-- 
2.34.1

[PATCH v2 05/16] media: i2c: ov9281: Support more than 1 mode.

[Dave Stevenson]

The driver currently has multiple assumptions that there is
only one supported mode.

Convert supported_mode to an array, and fix up all references
to correctly look at that array.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 46 +++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 123aa20951b7..1524189cf3e5 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -217,6 +217,10 @@ struct ov9282_reg_list common_regs_list = {
 	.regs = common_regs,
 };
 
+#define MODE_1280_720		0
+
+#define DEFAULT_MODE		MODE_1280_720
+
 /* Sensor mode registers */
 static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3778, 0x00},
@@ -252,17 +256,19 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 };
 
 /* Supported sensor mode configurations */
-static const struct ov9282_mode supported_mode = {
-	.width = 1280,
-	.height = 720,
-	.hblank = 250,
-	.vblank = 1022,
-	.vblank_min = 151,
-	.vblank_max = 51540,
-	.link_freq_idx = 0,
-	.reg_list = {
-		.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
-		.regs = mode_1280x720_regs,
+static const struct ov9282_mode supported_modes[] = {
+	[MODE_1280_720] = {
+		.width = 1280,
+		.height = 720,
+		.hblank = 250,
+		.vblank = 1022,
+		.vblank_min = 151,
+		.vblank_max = 51540,
+		.link_freq_idx = 0,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
+			.regs = mode_1280x720_regs,
+		},
 	},
 };
 
@@ -526,15 +532,15 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_frame_size_enum *fsize)
 {
-	if (fsize->index > 0)
+	if (fsize->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
 
 	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
 		return -EINVAL;
 
-	fsize->min_width = supported_mode.width;
+	fsize->min_width = supported_modes[fsize->index].width;
 	fsize->max_width = fsize->min_width;
-	fsize->min_height = supported_mode.height;
+	fsize->min_height = supported_modes[fsize->index].height;
 	fsize->max_height = fsize->min_height;
 
 	return 0;
@@ -609,7 +615,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
 
 	mutex_lock(&ov9282->mutex);
 
-	mode = &supported_mode;
+	mode = v4l2_find_nearest_size(supported_modes,
+				      ARRAY_SIZE(supported_modes),
+				      width, height,
+				      fmt->format.width,
+				      fmt->format.height);
 	ov9282_fill_pad_format(ov9282, mode, fmt);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
@@ -642,7 +652,7 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
 	struct v4l2_subdev_format fmt = { 0 };
 
 	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	ov9282_fill_pad_format(ov9282, &supported_mode, &fmt);
+	ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE], &fmt);
 
 	return ov9282_set_pad_format(sd, sd_state, &fmt);
 }
@@ -1043,8 +1053,8 @@ static int ov9282_probe(struct i2c_client *client)
 		goto error_power_off;
 	}
 
-	/* Set default mode to max resolution */
-	ov9282->cur_mode = &supported_mode;
+	/* Set default mode to first mode */
+	ov9282->cur_mode = &supported_modes[DEFAULT_MODE];
 	ov9282->vblank = ov9282->cur_mode->vblank;
 
 	ret = ov9282_init_controls(ov9282);
-- 
2.34.1

Re: [PATCH v2 05/16] media: i2c: ov9281: Support more than 1 mode.

[Jacopo Mondi]

Hi Dave

On Fri, Oct 28, 2022 at 05:08:51PM +0100, Dave Stevenson wrote:
> The driver currently has multiple assumptions that there is
> only one supported mode.
>
> Convert supported_mode to an array, and fix up all references
> to correctly look at that array.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 46 +++++++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 123aa20951b7..1524189cf3e5 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -217,6 +217,10 @@ struct ov9282_reg_list common_regs_list = {
>  	.regs = common_regs,
>  };
>
> +#define MODE_1280_720		0
> +
> +#define DEFAULT_MODE		MODE_1280_720
> +

I don't mind this considering there will be multiple modes

>  /* Sensor mode registers */
>  static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x3778, 0x00},
> @@ -252,17 +256,19 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  };
>
>  /* Supported sensor mode configurations */
> -static const struct ov9282_mode supported_mode = {
> -	.width = 1280,
> -	.height = 720,
> -	.hblank = 250,
> -	.vblank = 1022,
> -	.vblank_min = 151,
> -	.vblank_max = 51540,
> -	.link_freq_idx = 0,
> -	.reg_list = {
> -		.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> -		.regs = mode_1280x720_regs,
> +static const struct ov9282_mode supported_modes[] = {
> +	[MODE_1280_720] = {
> +		.width = 1280,
> +		.height = 720,
> +		.hblank = 250,
> +		.vblank = 1022,
> +		.vblank_min = 151,
> +		.vblank_max = 51540,
> +		.link_freq_idx = 0,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> +			.regs = mode_1280x720_regs,
> +		},
>  	},
>  };
>
> @@ -526,15 +532,15 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_state *sd_state,
>  				  struct v4l2_subdev_frame_size_enum *fsize)
>  {
> -	if (fsize->index > 0)
> +	if (fsize->index >= ARRAY_SIZE(supported_modes))
>  		return -EINVAL;
>
>  	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
>  		return -EINVAL;
>
> -	fsize->min_width = supported_mode.width;
> +	fsize->min_width = supported_modes[fsize->index].width;
>  	fsize->max_width = fsize->min_width;
> -	fsize->min_height = supported_mode.height;
> +	fsize->min_height = supported_modes[fsize->index].height;
>  	fsize->max_height = fsize->min_height;
>
>  	return 0;
> @@ -609,7 +615,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
>
>  	mutex_lock(&ov9282->mutex);
>
> -	mode = &supported_mode;
> +	mode = v4l2_find_nearest_size(supported_modes,
> +				      ARRAY_SIZE(supported_modes),
> +				      width, height,
> +				      fmt->format.width,
> +				      fmt->format.height);
>  	ov9282_fill_pad_format(ov9282, mode, fmt);
>
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> @@ -642,7 +652,7 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_subdev_format fmt = { 0 };
>
>  	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> -	ov9282_fill_pad_format(ov9282, &supported_mode, &fmt);
> +	ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE], &fmt);
>
>  	return ov9282_set_pad_format(sd, sd_state, &fmt);
>  }
> @@ -1043,8 +1053,8 @@ static int ov9282_probe(struct i2c_client *client)
>  		goto error_power_off;
>  	}
>
> -	/* Set default mode to max resolution */
> -	ov9282->cur_mode = &supported_mode;
> +	/* Set default mode to first mode */
> +	ov9282->cur_mode = &supported_modes[DEFAULT_MODE];

Looks good
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


>  	ov9282->vblank = ov9282->cur_mode->vblank;
>
>  	ret = ov9282_init_controls(ov9282);
> --
> 2.34.1
>

Re: [PATCH v2 05/16] media: i2c: ov9281: Support more than 1 mode.

[Sakari Ailus]

On Fri, Oct 28, 2022 at 05:08:51PM +0100, Dave Stevenson wrote:
> The driver currently has multiple assumptions that there is
> only one supported mode.
> 
> Convert supported_mode to an array, and fix up all references
> to correctly look at that array.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

This probably should have been ov9282 and not ov9281? I'll fix that when
applying it.

-- 
Sakari Ailus

Re: [PATCH v2 05/16] media: i2c: ov9281: Support more than 1 mode.

[Dave Stevenson]

Hi Sakari

On Tue, 1 Nov 2022 at 10:12, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> On Fri, Oct 28, 2022 at 05:08:51PM +0100, Dave Stevenson wrote:
> > The driver currently has multiple assumptions that there is
> > only one supported mode.
> >
> > Convert supported_mode to an array, and fix up all references
> > to correctly look at that array.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> This probably should have been ov9282 and not ov9281? I'll fix that when
> applying it.

Yes, thanks.
As discussed in the patch set at [1], ov9281 and ov9282 are the same
except for CRA in the optical path.
Largely we see OV9281 modules, hence the slip of the keys.

  Dave

[1] https://www.spinics.net/lists/devicetree/msg518160.html

> --
> Sakari Ailus

[PATCH v2 06/16] media: i2c: ov9282: Correct HTS register for configured pixel rate

[Dave Stevenson]

The calculations from pixel rate, width+hblank, and height+vblank
do not give the correct framerate - it's half the speed it should
be.

The datasheet lists the default for the TIMING_HTS registers (0x380c/d)
as being 0x2d8 (728) which is less than the width of the image, so
the units clearly can't be pixels.
If TIMING_HTS is considered to be units of 2-pixels, then the
resulting value of 0x5b0 (1456) makes all the calculations correct.

This driver is reporting an HBLANK value of 250, with an image width
of 1280, so TIMING_HTS is 1530 (0x5fa) pixels. However it was also
setting the register to 0x5fa, thereby not taking into account it
being units of 2-pixels.

Correct the register value to 0x2fd so that all the timing calculations
give the correct results.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 1524189cf3e5..7e0b12b89655 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -236,8 +236,8 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3809, 0x00},
 	{0x380a, 0x02},
 	{0x380b, 0xd0},
-	{0x380c, 0x05},
-	{0x380d, 0xfa},
+	{0x380c, 0x02},
+	{0x380d, 0xfd},
 	{0x3810, 0x00},
 	{0x3811, 0x08},
 	{0x3812, 0x00},
-- 
2.34.1

Re: [PATCH v2 06/16] media: i2c: ov9282: Correct HTS register for configured pixel rate

[Jacopo Mondi]

Hi Dave

On Fri, Oct 28, 2022 at 05:08:52PM +0100, Dave Stevenson wrote:
> The calculations from pixel rate, width+hblank, and height+vblank
> do not give the correct framerate - it's half the speed it should
> be.
>
> The datasheet lists the default for the TIMING_HTS registers (0x380c/d)
> as being 0x2d8 (728) which is less than the width of the image, so
> the units clearly can't be pixels.
> If TIMING_HTS is considered to be units of 2-pixels, then the
> resulting value of 0x5b0 (1456) makes all the calculations correct.
>
> This driver is reporting an HBLANK value of 250, with an image width
> of 1280, so TIMING_HTS is 1530 (0x5fa) pixels. However it was also
> setting the register to 0x5fa, thereby not taking into account it
> being units of 2-pixels.
>
> Correct the register value to 0x2fd so that all the timing calculations
> give the correct results.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Thanks, the commit message is now very clear

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  drivers/media/i2c/ov9282.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 1524189cf3e5..7e0b12b89655 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -236,8 +236,8 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x3809, 0x00},
>  	{0x380a, 0x02},
>  	{0x380b, 0xd0},
> -	{0x380c, 0x05},
> -	{0x380d, 0xfa},
> +	{0x380c, 0x02},
> +	{0x380d, 0xfd},
>  	{0x3810, 0x00},
>  	{0x3811, 0x08},
>  	{0x3812, 0x00},
> --
> 2.34.1
>

[PATCH v2 07/16] media: i2c: ov9282: Reduce vblank_min values based on testing

[Dave Stevenson]

The configured vblank_min setting of 151 (meaning VTS of
720 + 151 = 871) is far higher than the setting that works on
the sensor, and there are no obvious restrictions stated in the
datasheet.

Reduce the vblank_min to allow for faster frame rates.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 7e0b12b89655..35bc2b0438bc 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -262,7 +262,7 @@ static const struct ov9282_mode supported_modes[] = {
 		.height = 720,
 		.hblank = 250,
 		.vblank = 1022,
-		.vblank_min = 151,
+		.vblank_min = 41,
 		.vblank_max = 51540,
 		.link_freq_idx = 0,
 		.reg_list = {
-- 
2.34.1

Re: [PATCH v2 07/16] media: i2c: ov9282: Reduce vblank_min values based on testing

[Jacopo Mondi]

Hi Dave

On Fri, Oct 28, 2022 at 05:08:53PM +0100, Dave Stevenson wrote:
> The configured vblank_min setting of 151 (meaning VTS of
> 720 + 151 = 871) is far higher than the setting that works on
> the sensor, and there are no obvious restrictions stated in the
> datasheet.
>
> Reduce the vblank_min to allow for faster frame rates.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Let's hope this work on all platforms... I wonder if when something is
not documented but experimentally confirmed we should not record it
with a comment.

That's a general issue, not on this patch specifically

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> ---
>  drivers/media/i2c/ov9282.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 7e0b12b89655..35bc2b0438bc 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -262,7 +262,7 @@ static const struct ov9282_mode supported_modes[] = {
>  		.height = 720,
>  		.hblank = 250,
>  		.vblank = 1022,
> -		.vblank_min = 151,
> +		.vblank_min = 41,
>  		.vblank_max = 51540,
>  		.link_freq_idx = 0,
>  		.reg_list = {
> --
> 2.34.1
>

[PATCH v2 08/16] media: i2c: ov9282: Add selection for CSI2 clock mode

[Dave Stevenson]

The sensor supports either having the CSI2 clock lane free
running, or gated when there is no packet to transmit.
The driver only selected gated (non-continuous) clock mode.

Add code to allow fwnode to configure whether the clock is
gated or free running.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov9282.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 35bc2b0438bc..1637cf1177c5 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -45,6 +45,9 @@
 /* Group hold register */
 #define OV9282_REG_HOLD		0x3308
 
+#define OV9282_REG_MIPI_CTRL00	0x4800
+#define OV9282_GATED_CLOCK	BIT(5)
+
 /* Input clock rate */
 #define OV9282_INCLK_RATE	24000000
 
@@ -136,6 +139,7 @@ struct ov9282 {
 		struct v4l2_ctrl *again_ctrl;
 	};
 	u32 vblank;
+	bool noncontinuous_clock;
 	const struct ov9282_mode *cur_mode;
 	struct mutex mutex;
 	bool streaming;
@@ -145,7 +149,13 @@ static const s64 link_freq[] = {
 	OV9282_LINK_FREQ,
 };
 
-/* Common registers */
+/*
+ * Common registers
+ *
+ * Note: Do NOT include a software reset (0x0103, 0x01) in any of these
+ * register arrays as some settings are written as part of ov9282_power_on,
+ * and the reset will clear them.
+ */
 static const struct ov9282_reg common_regs[] = {
 	{0x0302, 0x32},
 	{0x030d, 0x50},
@@ -201,7 +211,6 @@ static const struct ov9282_reg common_regs[] = {
 	{0x4601, 0x04},
 	{0x470f, 0x00},
 	{0x4f07, 0x00},
-	{0x4800, 0x20},
 	{0x5000, 0x9f},
 	{0x5001, 0x00},
 	{0x5e00, 0x00},
@@ -835,6 +844,9 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
 	if (ret)
 		return ret;
 
+	ov9282->noncontinuous_clock =
+		bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
+
 	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV9282_NUM_DATA_LANES) {
 		dev_err(ov9282->dev,
 			"number of CSI2 data lanes %d is not supported",
@@ -903,6 +915,14 @@ static int ov9282_power_on(struct device *dev)
 
 	usleep_range(400, 600);
 
+	ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1,
+			       ov9282->noncontinuous_clock ?
+					OV9282_GATED_CLOCK : 0);
+	if (ret) {
+		dev_err(ov9282->dev, "fail to write MIPI_CTRL00");
+		return ret;
+	}
+
 	return 0;
 
 error_reset:
-- 
2.34.1

[PATCH v2 09/16] media: i2c: ov9282: Add the properties from fwnode

[Dave Stevenson]

Use v4l2_ctrl_new_fwnode_properties to add V4L2_CID_CAMERA_ORIENTATION
and V4L2_CID_CAMERA_SENSOR_ROTATION.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov9282.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 1637cf1177c5..889db9d105a2 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -959,10 +959,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 {
 	struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
 	const struct ov9282_mode *mode = ov9282->cur_mode;
+	struct v4l2_fwnode_device_properties props;
 	u32 lpfr;
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 6);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
 	if (ret)
 		return ret;
 
@@ -1020,7 +1021,14 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	if (ov9282->hblank_ctrl)
 		ov9282->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
-	if (ctrl_hdlr->error) {
+	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
+	if (!ret) {
+		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
+		v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov9282_ctrl_ops,
+						&props);
+	}
+
+	if (ctrl_hdlr->error || ret) {
 		dev_err(ov9282->dev, "control init failed: %d",
 			ctrl_hdlr->error);
 		v4l2_ctrl_handler_free(ctrl_hdlr);
-- 
2.34.1

[PATCH v2 10/16] media: i2c: ov9282: Action CID_VBLANK when set.

[Dave Stevenson]

Programming the sensor with TIMING_VTS (aka LPFR) was done
when triggered by a change in exposure or gain, but not
when V4L2_CID_VBLANK was changed. Dynamic frame rate
changes could therefore not be achieved.

Separate out programming TIMING_VTS so that it is triggered
by set_ctrl(V4L2_CID_VBLANK)

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov9282.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 889db9d105a2..e964461ff1d3 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -417,22 +417,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
  */
 static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
 {
-	u32 lpfr;
 	int ret;
 
-	lpfr = ov9282->vblank + ov9282->cur_mode->height;
-
-	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u, lpfr %u",
-		exposure, gain, lpfr);
+	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
+		exposure, gain);
 
 	ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
 	if (ret)
 		return ret;
 
-	ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
-	if (ret)
-		goto error_release_group_hold;
-
 	ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4);
 	if (ret)
 		goto error_release_group_hold;
@@ -463,6 +456,7 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		container_of(ctrl->handler, struct ov9282, ctrl_handler);
 	u32 analog_gain;
 	u32 exposure;
+	u32 lpfr;
 	int ret;
 
 	switch (ctrl->id) {
@@ -480,11 +474,14 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 					       OV9282_EXPOSURE_OFFSET,
 					       1, OV9282_EXPOSURE_DEFAULT);
 		break;
-	case V4L2_CID_EXPOSURE:
-		/* Set controls only if sensor is in power on state */
-		if (!pm_runtime_get_if_in_use(ov9282->dev))
-			return 0;
+	}
+
+	/* Set controls only if sensor is in power on state */
+	if (!pm_runtime_get_if_in_use(ov9282->dev))
+		return 0;
 
+	switch (ctrl->id) {
+	case V4L2_CID_EXPOSURE:
 		exposure = ctrl->val;
 		analog_gain = ov9282->again_ctrl->val;
 
@@ -492,15 +489,18 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 			exposure, analog_gain);
 
 		ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
-
-		pm_runtime_put(ov9282->dev);
-
+		break;
+	case V4L2_CID_VBLANK:
+		lpfr = ov9282->vblank + ov9282->cur_mode->height;
+		ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
 		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
 	}
 
+	pm_runtime_put(ov9282->dev);
+
 	return ret;
 }
 
-- 
2.34.1

[PATCH v2 11/16] media: i2c: ov9282: Add HFLIP and VFLIP support

[Dave Stevenson]

Adds support for V4L2_CID_HFLIP and V4L2_CID_VFLIP to allow
flipping the image.

The driver previously enabled H & V flips in the register table,
therefore the controls default to the same settings to avoid
changing the behaviour.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 52 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index e964461ff1d3..cfb6e72d8931 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -45,6 +45,10 @@
 /* Group hold register */
 #define OV9282_REG_HOLD		0x3308
 
+#define OV9282_REG_TIMING_FORMAT_1	0x3820
+#define OV9282_REG_TIMING_FORMAT_2	0x3821
+#define OV9282_FLIP_BIT			BIT(2)
+
 #define OV9282_REG_MIPI_CTRL00	0x4800
 #define OV9282_GATED_CLOCK	BIT(5)
 
@@ -438,6 +442,40 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
 	return ret;
 }
 
+static int ov9282_set_ctrl_hflip(struct ov9282 *ov9282, int value)
+{
+	u32 current_val;
+	int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
+				  &current_val);
+	if (ret)
+		return ret;
+
+	if (value)
+		current_val |= OV9282_FLIP_BIT;
+	else
+		current_val &= ~OV9282_FLIP_BIT;
+
+	return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
+				current_val);
+}
+
+static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
+{
+	u32 current_val;
+	int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
+				  &current_val);
+	if (ret)
+		return ret;
+
+	if (value)
+		current_val |= OV9282_FLIP_BIT;
+	else
+		current_val &= ~OV9282_FLIP_BIT;
+
+	return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
+				current_val);
+}
+
 /**
  * ov9282_set_ctrl() - Set subdevice control
  * @ctrl: pointer to v4l2_ctrl structure
@@ -494,6 +532,12 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		lpfr = ov9282->vblank + ov9282->cur_mode->height;
 		ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
 		break;
+	case V4L2_CID_HFLIP:
+		ret = ov9282_set_ctrl_hflip(ov9282, ctrl->val);
+		break;
+	case V4L2_CID_VFLIP:
+		ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
+		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
@@ -963,7 +1007,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	u32 lpfr;
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
 	if (ret)
 		return ret;
 
@@ -997,6 +1041,12 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 						mode->vblank_max,
 						1, mode->vblank);
 
+	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_VFLIP,
+			  0, 1, 1, 1);
+
+	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_HFLIP,
+			  0, 1, 1, 1);
+
 	/* Read only controls */
 	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
 			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
-- 
2.34.1

Re: [PATCH v2 11/16] media: i2c: ov9282: Add HFLIP and VFLIP support

[Jacopo Mondi]

On Fri, Oct 28, 2022 at 05:08:57PM +0100, Dave Stevenson wrote:
> Adds support for V4L2_CID_HFLIP and V4L2_CID_VFLIP to allow
> flipping the image.
>
> The driver previously enabled H & V flips in the register table,
> therefore the controls default to the same settings to avoid
> changing the behaviour.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 52 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index e964461ff1d3..cfb6e72d8931 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -45,6 +45,10 @@
>  /* Group hold register */
>  #define OV9282_REG_HOLD		0x3308
>
> +#define OV9282_REG_TIMING_FORMAT_1	0x3820
> +#define OV9282_REG_TIMING_FORMAT_2	0x3821
> +#define OV9282_FLIP_BIT			BIT(2)
> +
>  #define OV9282_REG_MIPI_CTRL00	0x4800
>  #define OV9282_GATED_CLOCK	BIT(5)
>
> @@ -438,6 +442,40 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
>  	return ret;
>  }
>
> +static int ov9282_set_ctrl_hflip(struct ov9282 *ov9282, int value)
> +{
> +	u32 current_val;
> +	int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
> +				  &current_val);
> +	if (ret)
> +		return ret;
> +
> +	if (value)
> +		current_val |= OV9282_FLIP_BIT;
> +	else
> +		current_val &= ~OV9282_FLIP_BIT;
> +
> +	return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
> +				current_val);
> +}
> +
> +static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
> +{
> +	u32 current_val;
> +	int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
> +				  &current_val);
> +	if (ret)
> +		return ret;
> +
> +	if (value)
> +		current_val |= OV9282_FLIP_BIT;
> +	else
> +		current_val &= ~OV9282_FLIP_BIT;
> +
> +	return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
> +				current_val);
> +}
> +
>  /**
>   * ov9282_set_ctrl() - Set subdevice control
>   * @ctrl: pointer to v4l2_ctrl structure
> @@ -494,6 +532,12 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>  		lpfr = ov9282->vblank + ov9282->cur_mode->height;
>  		ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
>  		break;
> +	case V4L2_CID_HFLIP:
> +		ret = ov9282_set_ctrl_hflip(ov9282, ctrl->val);
> +		break;
> +	case V4L2_CID_VFLIP:
> +		ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
> +		break;
>  	default:
>  		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
>  		ret = -EINVAL;
> @@ -963,7 +1007,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  	u32 lpfr;
>  	int ret;
>
> -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
>  	if (ret)
>  		return ret;
>
> @@ -997,6 +1041,12 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  						mode->vblank_max,
>  						1, mode->vblank);
>
> +	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_VFLIP,
> +			  0, 1, 1, 1);
> +
> +	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_HFLIP,
> +			  0, 1, 1, 1);
> +

I guess, based on the reply on the previous version, that flip by
default is correct in this case

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

>  	/* Read only controls */
>  	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
>  			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> --
> 2.34.1
>

[PATCH v2 12/16] media: i2c: ov9282: Make V4L2_CID_HBLANK r/w

[Dave Stevenson]

There's no reason why HBLANK has to be read-only as it
only changes the TIMING_HTS register in the sensor.

Remove the READ_ONLY flag, and add the relevant handling
for it.

The minimum value also varies based on whether continuous clock
mode is being used or not, so allow hblank_min to depend on
that.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov9282.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index cfb6e72d8931..2313d5e717f3 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -21,6 +21,9 @@
 #define OV9282_MODE_STANDBY	0x00
 #define OV9282_MODE_STREAMING	0x01
 
+#define OV9282_REG_TIMING_HTS	0x380c
+#define OV9282_TIMING_HTS_MAX	0x7fff
+
 /* Lines per frame */
 #define OV9282_REG_LPFR		0x380e
 
@@ -90,7 +93,8 @@ struct ov9282_reg_list {
  * struct ov9282_mode - ov9282 sensor mode structure
  * @width: Frame width
  * @height: Frame height
- * @hblank: Horizontal blanking in lines
+ * @hblank_min: Minimum horizontal blanking in lines for non-continuous[0] and
+ *		continuous[1] clock modes
  * @vblank: Vertical blanking in lines
  * @vblank_min: Minimum vertical blanking in lines
  * @vblank_max: Maximum vertical blanking in lines
@@ -100,7 +104,7 @@ struct ov9282_reg_list {
 struct ov9282_mode {
 	u32 width;
 	u32 height;
-	u32 hblank;
+	u32 hblank_min[2];
 	u32 vblank;
 	u32 vblank_min;
 	u32 vblank_max;
@@ -249,8 +253,6 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3809, 0x00},
 	{0x380a, 0x02},
 	{0x380b, 0xd0},
-	{0x380c, 0x02},
-	{0x380d, 0xfd},
 	{0x3810, 0x00},
 	{0x3811, 0x08},
 	{0x3812, 0x00},
@@ -273,7 +275,7 @@ static const struct ov9282_mode supported_modes[] = {
 	[MODE_1280_720] = {
 		.width = 1280,
 		.height = 720,
-		.hblank = 250,
+		.hblank_min = { 250, 176 },
 		.vblank = 1022,
 		.vblank_min = 41,
 		.vblank_max = 51540,
@@ -397,13 +399,17 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
 static int ov9282_update_controls(struct ov9282 *ov9282,
 				  const struct ov9282_mode *mode)
 {
+	u32 hblank_min;
 	int ret;
 
 	ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
 	if (ret)
 		return ret;
 
-	ret = __v4l2_ctrl_s_ctrl(ov9282->hblank_ctrl, mode->hblank);
+	hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
+	ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
+					OV9282_TIMING_HTS_MAX - mode->width, 1,
+					hblank_min);
 	if (ret)
 		return ret;
 
@@ -538,6 +544,10 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_VFLIP:
 		ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
 		break;
+	case V4L2_CID_HBLANK:
+		ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
+				       (ctrl->val + ov9282->cur_mode->width) >> 1);
+		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
@@ -1004,6 +1014,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
 	const struct ov9282_mode *mode = ov9282->cur_mode;
 	struct v4l2_fwnode_device_properties props;
+	u32 hblank_min;
 	u32 lpfr;
 	int ret;
 
@@ -1062,14 +1073,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	if (ov9282->link_freq_ctrl)
 		ov9282->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
+	hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
 	ov9282->hblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
 						&ov9282_ctrl_ops,
 						V4L2_CID_HBLANK,
-						OV9282_REG_MIN,
-						OV9282_REG_MAX,
-						1, mode->hblank);
-	if (ov9282->hblank_ctrl)
-		ov9282->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+						hblank_min,
+						OV9282_TIMING_HTS_MAX - mode->width,
+						1, hblank_min);
 
 	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
 	if (!ret) {
-- 
2.34.1

[PATCH v2 13/16] media: i2c: ov9282: Add selection API calls for cropping info

[Dave Stevenson]

As required by libcamera, add the relevant cropping targets
to report which portion of the sensor is being read out in
any mode.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov9282.c | 75 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 2313d5e717f3..a520d9fef0cb 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -66,6 +66,17 @@
 #define OV9282_PIXEL_RATE	(OV9282_LINK_FREQ * 2 * \
 				 OV9282_NUM_DATA_LANES / 10)
 
+/*
+ * OV9282 native and active pixel array size.
+ * 8 dummy rows/columns on each edge of a 1280x800 active array
+ */
+#define OV9282_NATIVE_WIDTH		1296U
+#define OV9282_NATIVE_HEIGHT		816U
+#define OV9282_PIXEL_ARRAY_LEFT		8U
+#define OV9282_PIXEL_ARRAY_TOP		8U
+#define OV9282_PIXEL_ARRAY_WIDTH	1280U
+#define OV9282_PIXEL_ARRAY_HEIGHT	800U
+
 #define OV9282_REG_MIN		0x00
 #define OV9282_REG_MAX		0xfffff
 
@@ -109,6 +120,7 @@ struct ov9282_mode {
 	u32 vblank_min;
 	u32 vblank_max;
 	u32 link_freq_idx;
+	struct v4l2_rect crop;
 	struct ov9282_reg_list reg_list;
 };
 
@@ -280,6 +292,16 @@ static const struct ov9282_mode supported_modes[] = {
 		.vblank_min = 41,
 		.vblank_max = 51540,
 		.link_freq_idx = 0,
+		.crop = {
+			/*
+			 * Note that this mode takes the top 720 lines from the
+			 * 800 of the sensor. It does not take a middle crop.
+			 */
+			.left = OV9282_PIXEL_ARRAY_LEFT,
+			.top = OV9282_PIXEL_ARRAY_TOP,
+			.width = 1280,
+			.height = 720
+		},
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
 			.regs = mode_1280x720_regs,
@@ -720,6 +742,58 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
 	return ov9282_set_pad_format(sd, sd_state, &fmt);
 }
 
+static const struct v4l2_rect *
+__ov9282_get_pad_crop(struct ov9282 *ov9282,
+		      struct v4l2_subdev_state *sd_state,
+		      unsigned int pad, enum v4l2_subdev_format_whence which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_crop(&ov9282->sd, sd_state, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &ov9282->cur_mode->crop;
+	}
+
+	return NULL;
+}
+
+static int ov9282_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_selection *sel)
+{
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP: {
+		struct ov9282 *ov9282 = to_ov9282(sd);
+
+		mutex_lock(&ov9282->mutex);
+		sel->r = *__ov9282_get_pad_crop(ov9282, sd_state, sel->pad,
+						sel->which);
+		mutex_unlock(&ov9282->mutex);
+
+		return 0;
+	}
+
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = OV9282_NATIVE_WIDTH;
+		sel->r.height = OV9282_NATIVE_HEIGHT;
+
+		return 0;
+
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = OV9282_PIXEL_ARRAY_TOP;
+		sel->r.left = OV9282_PIXEL_ARRAY_LEFT;
+		sel->r.width = OV9282_PIXEL_ARRAY_WIDTH;
+		sel->r.height = OV9282_PIXEL_ARRAY_HEIGHT;
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 /**
  * ov9282_start_streaming() - Start sensor stream
  * @ov9282: pointer to ov9282 device
@@ -938,6 +1012,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
 	.enum_frame_size = ov9282_enum_frame_size,
 	.get_fmt = ov9282_get_pad_format,
 	.set_fmt = ov9282_set_pad_format,
+	.get_selection = ov9282_get_selection,
 };
 
 static const struct v4l2_subdev_ops ov9282_subdev_ops = {
-- 
2.34.1

[PATCH v2 14/16] media: i2c: ov9282: Add support for 1280x800 and 640x400 modes

[Dave Stevenson]

Adds register settings for additional modes.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 103 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index a520d9fef0cb..c169b532ec8b 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -246,11 +246,44 @@ struct ov9282_reg_list common_regs_list = {
 	.regs = common_regs,
 };
 
-#define MODE_1280_720		0
+#define MODE_1280_800		0
+#define MODE_1280_720		1
+#define MODE_640_400		2
 
 #define DEFAULT_MODE		MODE_1280_720
 
 /* Sensor mode registers */
+static const struct ov9282_reg mode_1280x800_regs[] = {
+	{0x3778, 0x00},
+	{0x3800, 0x00},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x05},
+	{0x3805, 0x0f},
+	{0x3806, 0x03},
+	{0x3807, 0x2f},
+	{0x3808, 0x05},
+	{0x3809, 0x00},
+	{0x380a, 0x03},
+	{0x380b, 0x20},
+	{0x3810, 0x00},
+	{0x3811, 0x08},
+	{0x3812, 0x00},
+	{0x3813, 0x08},
+	{0x3814, 0x11},
+	{0x3815, 0x11},
+	{0x3820, 0x40},
+	{0x3821, 0x00},
+	{0x4003, 0x40},
+	{0x4008, 0x04},
+	{0x4009, 0x0b},
+	{0x400c, 0x00},
+	{0x400d, 0x07},
+	{0x4507, 0x00},
+	{0x4509, 0x00},
+};
+
 static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3778, 0x00},
 	{0x3800, 0x00},
@@ -282,8 +315,57 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x4509, 0x80},
 };
 
+static const struct ov9282_reg mode_640x400_regs[] = {
+	{0x3778, 0x10},
+	{0x3800, 0x00},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x05},
+	{0x3805, 0x0f},
+	{0x3806, 0x03},
+	{0x3807, 0x2f},
+	{0x3808, 0x02},
+	{0x3809, 0x80},
+	{0x380a, 0x01},
+	{0x380b, 0x90},
+	{0x3810, 0x00},
+	{0x3811, 0x04},
+	{0x3812, 0x00},
+	{0x3813, 0x04},
+	{0x3814, 0x31},
+	{0x3815, 0x22},
+	{0x3820, 0x60},
+	{0x3821, 0x01},
+	{0x4008, 0x02},
+	{0x4009, 0x05},
+	{0x400c, 0x00},
+	{0x400d, 0x03},
+	{0x4507, 0x03},
+	{0x4509, 0x80},
+};
+
 /* Supported sensor mode configurations */
 static const struct ov9282_mode supported_modes[] = {
+	[MODE_1280_800] = {
+		.width = 1280,
+		.height = 800,
+		.hblank_min = { 250, 176 },
+		.vblank = 1022,
+		.vblank_min = 110,
+		.vblank_max = 51540,
+		.link_freq_idx = 0,
+		.crop = {
+			.left = OV9282_PIXEL_ARRAY_LEFT,
+			.top = OV9282_PIXEL_ARRAY_TOP,
+			.width = 1280,
+			.height = 800
+		},
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1280x800_regs),
+			.regs = mode_1280x800_regs,
+		},
+	},
 	[MODE_1280_720] = {
 		.width = 1280,
 		.height = 720,
@@ -307,6 +389,25 @@ static const struct ov9282_mode supported_modes[] = {
 			.regs = mode_1280x720_regs,
 		},
 	},
+	[MODE_640_400] = {
+		.width = 640,
+		.height = 400,
+		.hblank_min = { 890, 816 },
+		.vblank = 1022,
+		.vblank_min = 22,
+		.vblank_max = 51540,
+		.link_freq_idx = 0,
+		.crop = {
+			.left = OV9282_PIXEL_ARRAY_LEFT,
+			.top = OV9282_PIXEL_ARRAY_TOP,
+			.width = 1280,
+			.height = 800
+		},
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_640x400_regs),
+			.regs = mode_640x400_regs,
+		},
+	},
 };
 
 /**
-- 
2.34.1

Re: [PATCH v2 14/16] media: i2c: ov9282: Add support for 1280x800 and 640x400 modes

[Jacopo Mondi]

Hi Dave


On Fri, Oct 28, 2022 at 05:09:00PM +0100, Dave Stevenson wrote:
> Adds register settings for additional modes.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 103 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 102 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index a520d9fef0cb..c169b532ec8b 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -246,11 +246,44 @@ struct ov9282_reg_list common_regs_list = {
>  	.regs = common_regs,
>  };
>
> -#define MODE_1280_720		0
> +#define MODE_1280_800		0
> +#define MODE_1280_720		1
> +#define MODE_640_400		2
>
>  #define DEFAULT_MODE		MODE_1280_720
>
>  /* Sensor mode registers */
> +static const struct ov9282_reg mode_1280x800_regs[] = {
> +	{0x3778, 0x00},
> +	{0x3800, 0x00},
> +	{0x3801, 0x00},
> +	{0x3802, 0x00},
> +	{0x3803, 0x00},
> +	{0x3804, 0x05},
> +	{0x3805, 0x0f},
> +	{0x3806, 0x03},
> +	{0x3807, 0x2f},
> +	{0x3808, 0x05},
> +	{0x3809, 0x00},
> +	{0x380a, 0x03},
> +	{0x380b, 0x20},
> +	{0x3810, 0x00},
> +	{0x3811, 0x08},
> +	{0x3812, 0x00},
> +	{0x3813, 0x08},
> +	{0x3814, 0x11},
> +	{0x3815, 0x11},
> +	{0x3820, 0x40},
> +	{0x3821, 0x00},
> +	{0x4003, 0x40},
> +	{0x4008, 0x04},
> +	{0x4009, 0x0b},
> +	{0x400c, 0x00},
> +	{0x400d, 0x07},
> +	{0x4507, 0x00},
> +	{0x4509, 0x00},
> +};
> +
>  static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x3778, 0x00},
>  	{0x3800, 0x00},
> @@ -282,8 +315,57 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x4509, 0x80},
>  };
>
> +static const struct ov9282_reg mode_640x400_regs[] = {
> +	{0x3778, 0x10},
> +	{0x3800, 0x00},
> +	{0x3801, 0x00},
> +	{0x3802, 0x00},
> +	{0x3803, 0x00},
> +	{0x3804, 0x05},
> +	{0x3805, 0x0f},
> +	{0x3806, 0x03},
> +	{0x3807, 0x2f},
> +	{0x3808, 0x02},
> +	{0x3809, 0x80},
> +	{0x380a, 0x01},
> +	{0x380b, 0x90},
> +	{0x3810, 0x00},
> +	{0x3811, 0x04},
> +	{0x3812, 0x00},
> +	{0x3813, 0x04},
> +	{0x3814, 0x31},
> +	{0x3815, 0x22},
> +	{0x3820, 0x60},
> +	{0x3821, 0x01},
> +	{0x4008, 0x02},
> +	{0x4009, 0x05},
> +	{0x400c, 0x00},
> +	{0x400d, 0x03},
> +	{0x4507, 0x03},
> +	{0x4509, 0x80},
> +};
> +
>  /* Supported sensor mode configurations */
>  static const struct ov9282_mode supported_modes[] = {
> +	[MODE_1280_800] = {
> +		.width = 1280,
> +		.height = 800,
> +		.hblank_min = { 250, 176 },
> +		.vblank = 1022,
> +		.vblank_min = 110,
> +		.vblank_max = 51540,
> +		.link_freq_idx = 0,
> +		.crop = {
> +			.left = OV9282_PIXEL_ARRAY_LEFT,
> +			.top = OV9282_PIXEL_ARRAY_TOP,
> +			.width = 1280,
> +			.height = 800
> +		},
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1280x800_regs),
> +			.regs = mode_1280x800_regs,
> +		},
> +	},
>  	[MODE_1280_720] = {
>  		.width = 1280,
>  		.height = 720,
> @@ -307,6 +389,25 @@ static const struct ov9282_mode supported_modes[] = {
>  			.regs = mode_1280x720_regs,
>  		},
>  	},
> +	[MODE_640_400] = {
> +		.width = 640,
> +		.height = 400,
> +		.hblank_min = { 890, 816 },
> +		.vblank = 1022,
> +		.vblank_min = 22,
> +		.vblank_max = 51540,

While hblank_min is adapated to match the limits for full resolution
mode (1280 + 250 - 640 = 890; same for the 816 non-continuous version)
vblank_min is shrinked, giving a min frame length of (400 + 22)
compared to the full-res min frame length of (800 + 110). Is this
intentional ?

> +		.link_freq_idx = 0,
> +		.crop = {
> +			.left = OV9282_PIXEL_ARRAY_LEFT,
> +			.top = OV9282_PIXEL_ARRAY_TOP,
> +			.width = 1280,
> +			.height = 800
> +		},
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_640x400_regs),
> +			.regs = mode_640x400_regs,
> +		},
> +	},
>  };
>
>  /**
> --
> 2.34.1
>

Re: [PATCH v2 14/16] media: i2c: ov9282: Add support for 1280x800 and 640x400 modes

[Dave Stevenson]

Hi Jacopo

On Mon, 31 Oct 2022 at 10:28, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave
>
>
> On Fri, Oct 28, 2022 at 05:09:00PM +0100, Dave Stevenson wrote:
> > Adds register settings for additional modes.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 103 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 102 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index a520d9fef0cb..c169b532ec8b 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -246,11 +246,44 @@ struct ov9282_reg_list common_regs_list = {
> >       .regs = common_regs,
> >  };
> >
> > -#define MODE_1280_720                0
> > +#define MODE_1280_800                0
> > +#define MODE_1280_720                1
> > +#define MODE_640_400         2
> >
> >  #define DEFAULT_MODE         MODE_1280_720
> >
> >  /* Sensor mode registers */
> > +static const struct ov9282_reg mode_1280x800_regs[] = {
> > +     {0x3778, 0x00},
> > +     {0x3800, 0x00},
> > +     {0x3801, 0x00},
> > +     {0x3802, 0x00},
> > +     {0x3803, 0x00},
> > +     {0x3804, 0x05},
> > +     {0x3805, 0x0f},
> > +     {0x3806, 0x03},
> > +     {0x3807, 0x2f},
> > +     {0x3808, 0x05},
> > +     {0x3809, 0x00},
> > +     {0x380a, 0x03},
> > +     {0x380b, 0x20},
> > +     {0x3810, 0x00},
> > +     {0x3811, 0x08},
> > +     {0x3812, 0x00},
> > +     {0x3813, 0x08},
> > +     {0x3814, 0x11},
> > +     {0x3815, 0x11},
> > +     {0x3820, 0x40},
> > +     {0x3821, 0x00},
> > +     {0x4003, 0x40},
> > +     {0x4008, 0x04},
> > +     {0x4009, 0x0b},
> > +     {0x400c, 0x00},
> > +     {0x400d, 0x07},
> > +     {0x4507, 0x00},
> > +     {0x4509, 0x00},
> > +};
> > +
> >  static const struct ov9282_reg mode_1280x720_regs[] = {
> >       {0x3778, 0x00},
> >       {0x3800, 0x00},
> > @@ -282,8 +315,57 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
> >       {0x4509, 0x80},
> >  };
> >
> > +static const struct ov9282_reg mode_640x400_regs[] = {
> > +     {0x3778, 0x10},
> > +     {0x3800, 0x00},
> > +     {0x3801, 0x00},
> > +     {0x3802, 0x00},
> > +     {0x3803, 0x00},
> > +     {0x3804, 0x05},
> > +     {0x3805, 0x0f},
> > +     {0x3806, 0x03},
> > +     {0x3807, 0x2f},
> > +     {0x3808, 0x02},
> > +     {0x3809, 0x80},
> > +     {0x380a, 0x01},
> > +     {0x380b, 0x90},
> > +     {0x3810, 0x00},
> > +     {0x3811, 0x04},
> > +     {0x3812, 0x00},
> > +     {0x3813, 0x04},
> > +     {0x3814, 0x31},
> > +     {0x3815, 0x22},
> > +     {0x3820, 0x60},
> > +     {0x3821, 0x01},
> > +     {0x4008, 0x02},
> > +     {0x4009, 0x05},
> > +     {0x400c, 0x00},
> > +     {0x400d, 0x03},
> > +     {0x4507, 0x03},
> > +     {0x4509, 0x80},
> > +};
> > +
> >  /* Supported sensor mode configurations */
> >  static const struct ov9282_mode supported_modes[] = {
> > +     [MODE_1280_800] = {
> > +             .width = 1280,
> > +             .height = 800,
> > +             .hblank_min = { 250, 176 },
> > +             .vblank = 1022,
> > +             .vblank_min = 110,
> > +             .vblank_max = 51540,
> > +             .link_freq_idx = 0,
> > +             .crop = {
> > +                     .left = OV9282_PIXEL_ARRAY_LEFT,
> > +                     .top = OV9282_PIXEL_ARRAY_TOP,
> > +                     .width = 1280,
> > +                     .height = 800
> > +             },
> > +             .reg_list = {
> > +                     .num_of_regs = ARRAY_SIZE(mode_1280x800_regs),
> > +                     .regs = mode_1280x800_regs,
> > +             },
> > +     },
> >       [MODE_1280_720] = {
> >               .width = 1280,
> >               .height = 720,
> > @@ -307,6 +389,25 @@ static const struct ov9282_mode supported_modes[] = {
> >                       .regs = mode_1280x720_regs,
> >               },
> >       },
> > +     [MODE_640_400] = {
> > +             .width = 640,
> > +             .height = 400,
> > +             .hblank_min = { 890, 816 },
> > +             .vblank = 1022,
> > +             .vblank_min = 22,
> > +             .vblank_max = 51540,
>
> While hblank_min is adapated to match the limits for full resolution
> mode (1280 + 250 - 640 = 890; same for the 816 non-continuous version)
> vblank_min is shrinked, giving a min frame length of (400 + 22)
> compared to the full-res min frame length of (800 + 110). Is this
> intentional ?

I adapted the Rockchip driver [1] ages ago and we had been using that
with extensions in our vendor kernel. With Alexander posting the
patches to this ov9282 driver to add ov9281 support, I looked at
porting the extra functionality I had there.

I added the 640x400 mode to the vendor driver back in Nov 2020 [2]
with a min/default vts of 421. This was then corrected in July 2022
with [3] as VTS 421 actually gave 130fps instead of the expected
~261fps.

The datasheet doesn't give a minimum height for the VBLANK period,
therefore empirical testing is the best we can do in this case.

It may be possible to reduce vblank_min for the other modes, but I
haven't verified that. The datasheet lists the default for registers
0x380E/F as 0x38e or 910, giving VBLANK as 110, and resulting in
120fps. As the sensor is advertised as having a maximum transfer rate
of 1280 x 800: 120fps, exceeding that would probably be foolish.

  Dave

[1] https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/media/i2c/ov9281.c
[2] https://github.com/raspberrypi/linux/pull/3968
[3] https://github.com/raspberrypi/linux/pull/5082

> > +             .link_freq_idx = 0,
> > +             .crop = {
> > +                     .left = OV9282_PIXEL_ARRAY_LEFT,
> > +                     .top = OV9282_PIXEL_ARRAY_TOP,
> > +                     .width = 1280,
> > +                     .height = 800
> > +             },
> > +             .reg_list = {
> > +                     .num_of_regs = ARRAY_SIZE(mode_640x400_regs),
> > +                     .regs = mode_640x400_regs,
> > +             },
> > +     },
> >  };
> >
> >  /**
> > --
> > 2.34.1
> >

Re: [PATCH v2 14/16] media: i2c: ov9282: Add support for 1280x800 and 640x400 modes

[Jacopo Mondi]

Hi Dave,

On Mon, Oct 31, 2022 at 12:09:46PM +0000, Dave Stevenson wrote:
> Hi Jacopo
>
> On Mon, 31 Oct 2022 at 10:28, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Dave
> >
> >
> > On Fri, Oct 28, 2022 at 05:09:00PM +0100, Dave Stevenson wrote:
> > > Adds register settings for additional modes.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/i2c/ov9282.c | 103 ++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 102 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index a520d9fef0cb..c169b532ec8b 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -246,11 +246,44 @@ struct ov9282_reg_list common_regs_list = {
> > >       .regs = common_regs,
> > >  };
> > >
> > > -#define MODE_1280_720                0
> > > +#define MODE_1280_800                0
> > > +#define MODE_1280_720                1
> > > +#define MODE_640_400         2
> > >
> > >  #define DEFAULT_MODE         MODE_1280_720
> > >
> > >  /* Sensor mode registers */
> > > +static const struct ov9282_reg mode_1280x800_regs[] = {
> > > +     {0x3778, 0x00},
> > > +     {0x3800, 0x00},
> > > +     {0x3801, 0x00},
> > > +     {0x3802, 0x00},
> > > +     {0x3803, 0x00},
> > > +     {0x3804, 0x05},
> > > +     {0x3805, 0x0f},
> > > +     {0x3806, 0x03},
> > > +     {0x3807, 0x2f},
> > > +     {0x3808, 0x05},
> > > +     {0x3809, 0x00},
> > > +     {0x380a, 0x03},
> > > +     {0x380b, 0x20},
> > > +     {0x3810, 0x00},
> > > +     {0x3811, 0x08},
> > > +     {0x3812, 0x00},
> > > +     {0x3813, 0x08},
> > > +     {0x3814, 0x11},
> > > +     {0x3815, 0x11},
> > > +     {0x3820, 0x40},
> > > +     {0x3821, 0x00},
> > > +     {0x4003, 0x40},
> > > +     {0x4008, 0x04},
> > > +     {0x4009, 0x0b},
> > > +     {0x400c, 0x00},
> > > +     {0x400d, 0x07},
> > > +     {0x4507, 0x00},
> > > +     {0x4509, 0x00},
> > > +};
> > > +
> > >  static const struct ov9282_reg mode_1280x720_regs[] = {
> > >       {0x3778, 0x00},
> > >       {0x3800, 0x00},
> > > @@ -282,8 +315,57 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
> > >       {0x4509, 0x80},
> > >  };
> > >
> > > +static const struct ov9282_reg mode_640x400_regs[] = {
> > > +     {0x3778, 0x10},
> > > +     {0x3800, 0x00},
> > > +     {0x3801, 0x00},
> > > +     {0x3802, 0x00},
> > > +     {0x3803, 0x00},
> > > +     {0x3804, 0x05},
> > > +     {0x3805, 0x0f},
> > > +     {0x3806, 0x03},
> > > +     {0x3807, 0x2f},
> > > +     {0x3808, 0x02},
> > > +     {0x3809, 0x80},
> > > +     {0x380a, 0x01},
> > > +     {0x380b, 0x90},
> > > +     {0x3810, 0x00},
> > > +     {0x3811, 0x04},
> > > +     {0x3812, 0x00},
> > > +     {0x3813, 0x04},
> > > +     {0x3814, 0x31},
> > > +     {0x3815, 0x22},
> > > +     {0x3820, 0x60},
> > > +     {0x3821, 0x01},
> > > +     {0x4008, 0x02},
> > > +     {0x4009, 0x05},
> > > +     {0x400c, 0x00},
> > > +     {0x400d, 0x03},
> > > +     {0x4507, 0x03},
> > > +     {0x4509, 0x80},
> > > +};
> > > +
> > >  /* Supported sensor mode configurations */
> > >  static const struct ov9282_mode supported_modes[] = {
> > > +     [MODE_1280_800] = {
> > > +             .width = 1280,
> > > +             .height = 800,
> > > +             .hblank_min = { 250, 176 },
> > > +             .vblank = 1022,
> > > +             .vblank_min = 110,
> > > +             .vblank_max = 51540,
> > > +             .link_freq_idx = 0,
> > > +             .crop = {
> > > +                     .left = OV9282_PIXEL_ARRAY_LEFT,
> > > +                     .top = OV9282_PIXEL_ARRAY_TOP,
> > > +                     .width = 1280,
> > > +                     .height = 800
> > > +             },
> > > +             .reg_list = {
> > > +                     .num_of_regs = ARRAY_SIZE(mode_1280x800_regs),
> > > +                     .regs = mode_1280x800_regs,
> > > +             },
> > > +     },
> > >       [MODE_1280_720] = {
> > >               .width = 1280,
> > >               .height = 720,
> > > @@ -307,6 +389,25 @@ static const struct ov9282_mode supported_modes[] = {
> > >                       .regs = mode_1280x720_regs,
> > >               },
> > >       },
> > > +     [MODE_640_400] = {
> > > +             .width = 640,
> > > +             .height = 400,
> > > +             .hblank_min = { 890, 816 },
> > > +             .vblank = 1022,
> > > +             .vblank_min = 22,
> > > +             .vblank_max = 51540,
> >
> > While hblank_min is adapated to match the limits for full resolution
> > mode (1280 + 250 - 640 = 890; same for the 816 non-continuous version)
> > vblank_min is shrinked, giving a min frame length of (400 + 22)
> > compared to the full-res min frame length of (800 + 110). Is this
> > intentional ?
>
> I adapted the Rockchip driver [1] ages ago and we had been using that
> with extensions in our vendor kernel. With Alexander posting the
> patches to this ov9282 driver to add ov9281 support, I looked at
> porting the extra functionality I had there.
>
> I added the 640x400 mode to the vendor driver back in Nov 2020 [2]
> with a min/default vts of 421. This was then corrected in July 2022
> with [3] as VTS 421 actually gave 130fps instead of the expected
> ~261fps.
>
> The datasheet doesn't give a minimum height for the VBLANK period,
> therefore empirical testing is the best we can do in this case.
>
> It may be possible to reduce vblank_min for the other modes, but I
> haven't verified that. The datasheet lists the default for registers
> 0x380E/F as 0x38e or 910, giving VBLANK as 110, and resulting in
> 120fps. As the sensor is advertised as having a maximum transfer rate
> of 1280 x 800: 120fps, exceeding that would probably be foolish.
>

Ok, so vblank_min = 21 "breaks" streaming by halving the framerate,
while vblank_min = 22 works as expected.

It would be great to record that 22 is obtained by sperimental results
in the commit message, or in a comment here and not by documentation ?

Anyway, the series is tagged and Sakari is about to collect it, so no
need to resend, but if you have to...

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


>   Dave
>
> [1] https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/media/i2c/ov9281.c
> [2] https://github.com/raspberrypi/linux/pull/3968
> [3] https://github.com/raspberrypi/linux/pull/5082
>
> > > +             .link_freq_idx = 0,
> > > +             .crop = {
> > > +                     .left = OV9282_PIXEL_ARRAY_LEFT,
> > > +                     .top = OV9282_PIXEL_ARRAY_TOP,
> > > +                     .width = 1280,
> > > +                     .height = 800
> > > +             },
> > > +             .reg_list = {
> > > +                     .num_of_regs = ARRAY_SIZE(mode_640x400_regs),
> > > +                     .regs = mode_640x400_regs,
> > > +             },
> > > +     },
> > >  };
> > >
> > >  /**
> > > --
> > > 2.34.1
> > >

[PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Dave Stevenson]

The sensor supports 8 or 10 bit readout modes, but the
driver only supported 10 bit.

Add 8 bit readout.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 95 +++++++++++++++++++++++++++++++-------
 1 file changed, 78 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c169b532ec8b..e2a98f480b58 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -21,6 +21,10 @@
 #define OV9282_MODE_STANDBY	0x00
 #define OV9282_MODE_STREAMING	0x01
 
+#define OV9282_REG_PLL_CTRL_0D	0x030d
+#define OV9282_PLL_CTRL_0D_RAW8		0x60
+#define OV9282_PLL_CTRL_0D_RAW10	0x50
+
 #define OV9282_REG_TIMING_HTS	0x380c
 #define OV9282_TIMING_HTS_MAX	0x7fff
 
@@ -48,6 +52,10 @@
 /* Group hold register */
 #define OV9282_REG_HOLD		0x3308
 
+#define OV9282_REG_ANA_CORE_2	0x3662
+#define OV9282_ANA_CORE2_RAW8	0x07
+#define OV9282_ANA_CORE2_RAW10	0x05
+
 #define OV9282_REG_TIMING_FORMAT_1	0x3820
 #define OV9282_REG_TIMING_FORMAT_2	0x3821
 #define OV9282_FLIP_BIT			BIT(2)
@@ -63,8 +71,10 @@
 #define OV9282_NUM_DATA_LANES	2
 
 /* Pixel rate */
-#define OV9282_PIXEL_RATE	(OV9282_LINK_FREQ * 2 * \
-				 OV9282_NUM_DATA_LANES / 10)
+#define OV9282_PIXEL_RATE_10BIT		(OV9282_LINK_FREQ * 2 * \
+					 OV9282_NUM_DATA_LANES / 10)
+#define OV9282_PIXEL_RATE_8BIT		(OV9282_LINK_FREQ * 2 * \
+					 OV9282_NUM_DATA_LANES / 8)
 
 /*
  * OV9282 native and active pixel array size.
@@ -140,6 +150,7 @@ struct ov9282_mode {
  * @again_ctrl: Pointer to analog gain control
  * @vblank: Vertical blanking in lines
  * @cur_mode: Pointer to current selected sensor mode
+ * @code: Mbus code currently selected
  * @mutex: Mutex for serializing sensor controls
  * @streaming: Flag indicating streaming state
  */
@@ -158,9 +169,11 @@ struct ov9282 {
 		struct v4l2_ctrl *exp_ctrl;
 		struct v4l2_ctrl *again_ctrl;
 	};
+	struct v4l2_ctrl *pixel_rate;
 	u32 vblank;
 	bool noncontinuous_clock;
 	const struct ov9282_mode *cur_mode;
+	u32 code;
 	struct mutex mutex;
 	bool streaming;
 };
@@ -178,7 +191,6 @@ static const s64 link_freq[] = {
  */
 static const struct ov9282_reg common_regs[] = {
 	{0x0302, 0x32},
-	{0x030d, 0x50},
 	{0x030e, 0x02},
 	{0x3001, 0x00},
 	{0x3004, 0x00},
@@ -516,19 +528,29 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
  * ov9282_update_controls() - Update control ranges based on streaming mode
  * @ov9282: pointer to ov9282 device
  * @mode: pointer to ov9282_mode sensor mode
+ * @fmt: pointer to the requested mode
  *
  * Return: 0 if successful, error code otherwise.
  */
 static int ov9282_update_controls(struct ov9282 *ov9282,
-				  const struct ov9282_mode *mode)
+				  const struct ov9282_mode *mode,
+				  const struct v4l2_subdev_format *fmt)
 {
 	u32 hblank_min;
+	s64 pixel_rate;
 	int ret;
 
 	ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
 	if (ret)
 		return ret;
 
+	pixel_rate = (fmt->format.code == MEDIA_BUS_FMT_Y10_1X10) ?
+		OV9282_PIXEL_RATE_10BIT : OV9282_PIXEL_RATE_8BIT;
+	ret = __v4l2_ctrl_modify_range(ov9282->pixel_rate, pixel_rate,
+				       pixel_rate, 1, pixel_rate);
+	if (ret)
+		return ret;
+
 	hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
 	ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
 					OV9282_TIMING_HTS_MAX - mode->width, 1,
@@ -698,10 +720,16 @@ static int ov9282_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
 {
-	if (code->index > 0)
+	switch (code->index) {
+	case 0:
+		code->code = MEDIA_BUS_FMT_Y10_1X10;
+		break;
+	case 1:
+		code->code = MEDIA_BUS_FMT_Y8_1X8;
+		break;
+	default:
 		return -EINVAL;
-
-	code->code = MEDIA_BUS_FMT_Y10_1X10;
+	}
 
 	return 0;
 }
@@ -721,7 +749,8 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
 	if (fsize->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
 
-	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
+	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10 &&
+	    fsize->code != MEDIA_BUS_FMT_Y8_1X8)
 		return -EINVAL;
 
 	fsize->min_width = supported_modes[fsize->index].width;
@@ -737,15 +766,17 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
  *                            from selected sensor mode
  * @ov9282: pointer to ov9282 device
  * @mode: pointer to ov9282_mode sensor mode
+ * @code: mbus code to be stored
  * @fmt: V4L2 sub-device format need to be filled
  */
 static void ov9282_fill_pad_format(struct ov9282 *ov9282,
 				   const struct ov9282_mode *mode,
+				   u32 code,
 				   struct v4l2_subdev_format *fmt)
 {
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
-	fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
+	fmt->format.code = code;
 	fmt->format.field = V4L2_FIELD_NONE;
 	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
 	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
@@ -775,7 +806,8 @@ static int ov9282_get_pad_format(struct v4l2_subdev *sd,
 		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
 		fmt->format = *framefmt;
 	} else {
-		ov9282_fill_pad_format(ov9282, ov9282->cur_mode, fmt);
+		ov9282_fill_pad_format(ov9282, ov9282->cur_mode, ov9282->code,
+				       fmt);
 	}
 
 	mutex_unlock(&ov9282->mutex);
@@ -797,6 +829,7 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
 {
 	struct ov9282 *ov9282 = to_ov9282(sd);
 	const struct ov9282_mode *mode;
+	u32 code;
 	int ret = 0;
 
 	mutex_lock(&ov9282->mutex);
@@ -806,7 +839,12 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
 				      width, height,
 				      fmt->format.width,
 				      fmt->format.height);
-	ov9282_fill_pad_format(ov9282, mode, fmt);
+	if (fmt->format.code == MEDIA_BUS_FMT_Y8_1X8)
+		code = MEDIA_BUS_FMT_Y8_1X8;
+	else
+		code = MEDIA_BUS_FMT_Y10_1X10;
+
+	ov9282_fill_pad_format(ov9282, mode, code, fmt);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
 		struct v4l2_mbus_framefmt *framefmt;
@@ -814,9 +852,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
 		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
 		*framefmt = fmt->format;
 	} else {
-		ret = ov9282_update_controls(ov9282, mode);
-		if (!ret)
+		ret = ov9282_update_controls(ov9282, mode, fmt);
+		if (!ret) {
 			ov9282->cur_mode = mode;
+			ov9282->code = code;
+		}
 	}
 
 	mutex_unlock(&ov9282->mutex);
@@ -838,7 +878,8 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
 	struct v4l2_subdev_format fmt = { 0 };
 
 	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE], &fmt);
+	ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE],
+			       ov9282->code, &fmt);
 
 	return ov9282_set_pad_format(sd, sd_state, &fmt);
 }
@@ -903,7 +944,17 @@ static int ov9282_get_selection(struct v4l2_subdev *sd,
  */
 static int ov9282_start_streaming(struct ov9282 *ov9282)
 {
+	const struct ov9282_reg bitdepth_regs[2][2] = {
+		{
+			{OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW10},
+			{OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10},
+		}, {
+			{OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW8},
+			{OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW8},
+		}
+	};
 	const struct ov9282_reg_list *reg_list;
+	int bitdepth_index;
 	int ret;
 
 	/* Write common registers */
@@ -914,6 +965,13 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
 		return ret;
 	}
 
+	bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1;
+	ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2);
+	if (ret) {
+		dev_err(ov9282->dev, "fail to write bitdepth regs");
+		return ret;
+	}
+
 	/* Write sensor mode registers */
 	reg_list = &ov9282->cur_mode->reg_list;
 	ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
@@ -1235,9 +1293,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 			  0, 1, 1, 1);
 
 	/* Read only controls */
-	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
-			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
-			  OV9282_PIXEL_RATE);
+	ov9282->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
+					       V4L2_CID_PIXEL_RATE,
+					       OV9282_PIXEL_RATE_10BIT,
+					       OV9282_PIXEL_RATE_10BIT, 1,
+					       OV9282_PIXEL_RATE_10BIT);
 
 	ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
 							&ov9282_ctrl_ops,
@@ -1319,6 +1379,7 @@ static int ov9282_probe(struct i2c_client *client)
 
 	/* Set default mode to first mode */
 	ov9282->cur_mode = &supported_modes[DEFAULT_MODE];
+	ov9282->code = MEDIA_BUS_FMT_Y10_1X10;
 	ov9282->vblank = ov9282->cur_mode->vblank;
 
 	ret = ov9282_init_controls(ov9282);
-- 
2.34.1

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Jacopo Mondi]

Hi Dave

On Fri, Oct 28, 2022 at 05:09:01PM +0100, Dave Stevenson wrote:
> The sensor supports 8 or 10 bit readout modes, but the
> driver only supported 10 bit.
>
> Add 8 bit readout.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 95 +++++++++++++++++++++++++++++++-------
>  1 file changed, 78 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index c169b532ec8b..e2a98f480b58 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -21,6 +21,10 @@
>  #define OV9282_MODE_STANDBY	0x00
>  #define OV9282_MODE_STREAMING	0x01
>
> +#define OV9282_REG_PLL_CTRL_0D	0x030d
> +#define OV9282_PLL_CTRL_0D_RAW8		0x60
> +#define OV9282_PLL_CTRL_0D_RAW10	0x50
> +
>  #define OV9282_REG_TIMING_HTS	0x380c
>  #define OV9282_TIMING_HTS_MAX	0x7fff
>
> @@ -48,6 +52,10 @@
>  /* Group hold register */
>  #define OV9282_REG_HOLD		0x3308
>
> +#define OV9282_REG_ANA_CORE_2	0x3662
> +#define OV9282_ANA_CORE2_RAW8	0x07
> +#define OV9282_ANA_CORE2_RAW10	0x05
> +
>  #define OV9282_REG_TIMING_FORMAT_1	0x3820
>  #define OV9282_REG_TIMING_FORMAT_2	0x3821
>  #define OV9282_FLIP_BIT			BIT(2)
> @@ -63,8 +71,10 @@
>  #define OV9282_NUM_DATA_LANES	2
>
>  /* Pixel rate */
> -#define OV9282_PIXEL_RATE	(OV9282_LINK_FREQ * 2 * \
> -				 OV9282_NUM_DATA_LANES / 10)
> +#define OV9282_PIXEL_RATE_10BIT		(OV9282_LINK_FREQ * 2 * \
> +					 OV9282_NUM_DATA_LANES / 10)
> +#define OV9282_PIXEL_RATE_8BIT		(OV9282_LINK_FREQ * 2 * \
> +					 OV9282_NUM_DATA_LANES / 8)
>
>  /*
>   * OV9282 native and active pixel array size.
> @@ -140,6 +150,7 @@ struct ov9282_mode {
>   * @again_ctrl: Pointer to analog gain control
>   * @vblank: Vertical blanking in lines
>   * @cur_mode: Pointer to current selected sensor mode
> + * @code: Mbus code currently selected
>   * @mutex: Mutex for serializing sensor controls
>   * @streaming: Flag indicating streaming state
>   */
> @@ -158,9 +169,11 @@ struct ov9282 {
>  		struct v4l2_ctrl *exp_ctrl;
>  		struct v4l2_ctrl *again_ctrl;
>  	};
> +	struct v4l2_ctrl *pixel_rate;
>  	u32 vblank;
>  	bool noncontinuous_clock;
>  	const struct ov9282_mode *cur_mode;
> +	u32 code;
>  	struct mutex mutex;
>  	bool streaming;
>  };
> @@ -178,7 +191,6 @@ static const s64 link_freq[] = {
>   */
>  static const struct ov9282_reg common_regs[] = {
>  	{0x0302, 0x32},
> -	{0x030d, 0x50},
>  	{0x030e, 0x02},
>  	{0x3001, 0x00},
>  	{0x3004, 0x00},
> @@ -516,19 +528,29 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
>   * ov9282_update_controls() - Update control ranges based on streaming mode
>   * @ov9282: pointer to ov9282 device
>   * @mode: pointer to ov9282_mode sensor mode
> + * @fmt: pointer to the requested mode
>   *
>   * Return: 0 if successful, error code otherwise.
>   */
>  static int ov9282_update_controls(struct ov9282 *ov9282,
> -				  const struct ov9282_mode *mode)
> +				  const struct ov9282_mode *mode,
> +				  const struct v4l2_subdev_format *fmt)
>  {
>  	u32 hblank_min;
> +	s64 pixel_rate;
>  	int ret;
>
>  	ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
>  	if (ret)
>  		return ret;
>
> +	pixel_rate = (fmt->format.code == MEDIA_BUS_FMT_Y10_1X10) ?
> +		OV9282_PIXEL_RATE_10BIT : OV9282_PIXEL_RATE_8BIT;

I wonder if OV9282_PIXEL_RATE(bpp) wouldn't be better

> +	ret = __v4l2_ctrl_modify_range(ov9282->pixel_rate, pixel_rate,
> +				       pixel_rate, 1, pixel_rate);
> +	if (ret)
> +		return ret;
> +
>  	hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
>  	ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
>  					OV9282_TIMING_HTS_MAX - mode->width, 1,
> @@ -698,10 +720,16 @@ static int ov9282_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)
>  {
> -	if (code->index > 0)
> +	switch (code->index) {
> +	case 0:
> +		code->code = MEDIA_BUS_FMT_Y10_1X10;
> +		break;
> +	case 1:
> +		code->code = MEDIA_BUS_FMT_Y8_1X8;
> +		break;
> +	default:
>  		return -EINVAL;
> -
> -	code->code = MEDIA_BUS_FMT_Y10_1X10;
> +	}
>
>  	return 0;
>  }
> @@ -721,7 +749,8 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
>  	if (fsize->index >= ARRAY_SIZE(supported_modes))
>  		return -EINVAL;
>
> -	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
> +	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10 &&
> +	    fsize->code != MEDIA_BUS_FMT_Y8_1X8)
>  		return -EINVAL;
>
>  	fsize->min_width = supported_modes[fsize->index].width;
> @@ -737,15 +766,17 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
>   *                            from selected sensor mode
>   * @ov9282: pointer to ov9282 device
>   * @mode: pointer to ov9282_mode sensor mode
> + * @code: mbus code to be stored
>   * @fmt: V4L2 sub-device format need to be filled
>   */
>  static void ov9282_fill_pad_format(struct ov9282 *ov9282,
>  				   const struct ov9282_mode *mode,
> +				   u32 code,
>  				   struct v4l2_subdev_format *fmt)
>  {
>  	fmt->format.width = mode->width;
>  	fmt->format.height = mode->height;
> -	fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
> +	fmt->format.code = code;
>  	fmt->format.field = V4L2_FIELD_NONE;
>  	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
>  	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> @@ -775,7 +806,8 @@ static int ov9282_get_pad_format(struct v4l2_subdev *sd,
>  		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
>  		fmt->format = *framefmt;
>  	} else {
> -		ov9282_fill_pad_format(ov9282, ov9282->cur_mode, fmt);
> +		ov9282_fill_pad_format(ov9282, ov9282->cur_mode, ov9282->code,
> +				       fmt);
>  	}
>
>  	mutex_unlock(&ov9282->mutex);
> @@ -797,6 +829,7 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
>  {
>  	struct ov9282 *ov9282 = to_ov9282(sd);
>  	const struct ov9282_mode *mode;
> +	u32 code;
>  	int ret = 0;
>
>  	mutex_lock(&ov9282->mutex);
> @@ -806,7 +839,12 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
>  				      width, height,
>  				      fmt->format.width,
>  				      fmt->format.height);
> -	ov9282_fill_pad_format(ov9282, mode, fmt);
> +	if (fmt->format.code == MEDIA_BUS_FMT_Y8_1X8)
> +		code = MEDIA_BUS_FMT_Y8_1X8;
> +	else
> +		code = MEDIA_BUS_FMT_Y10_1X10;
> +
> +	ov9282_fill_pad_format(ov9282, mode, code, fmt);
>
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>  		struct v4l2_mbus_framefmt *framefmt;
> @@ -814,9 +852,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
>  		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
>  		*framefmt = fmt->format;
>  	} else {
> -		ret = ov9282_update_controls(ov9282, mode);
> -		if (!ret)
> +		ret = ov9282_update_controls(ov9282, mode, fmt);
> +		if (!ret) {
>  			ov9282->cur_mode = mode;
> +			ov9282->code = code;
> +		}
>  	}
>
>  	mutex_unlock(&ov9282->mutex);
> @@ -838,7 +878,8 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_subdev_format fmt = { 0 };
>
>  	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> -	ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE], &fmt);
> +	ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE],
> +			       ov9282->code, &fmt);
>
>  	return ov9282_set_pad_format(sd, sd_state, &fmt);
>  }
> @@ -903,7 +944,17 @@ static int ov9282_get_selection(struct v4l2_subdev *sd,
>   */
>  static int ov9282_start_streaming(struct ov9282 *ov9282)
>  {
> +	const struct ov9282_reg bitdepth_regs[2][2] = {
> +		{
> +			{OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW10},
> +			{OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10},
> +		}, {
> +			{OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW8},
> +			{OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW8},
> +		}
> +	};

Same here where instead of hardcoding the values for the 10/8 bit
modes they could be constructed as

        #define OV9282_PLL_CTRL_0D              0x40
        #define OV9282_PLL_CTRL_0D_RAW8         BIT(5)
        #define OV9282_PLL_CTRL_0D_RAW10        BIT(4)

        OV9282_REG_PLL_CTRL_0D = OV9282_PLL_CTRL_0D
                               |(8bit ? OV9282_PLL_CTRL_0D_RAW8
                                      : OV9282_PLL_CTRL_0D_RAW10);

Same for ANA_CORE_2, where BIT(4) is flipped according to the selected
readout mode.

But as I presume there won't be another readout mode to add support
for, I guess hardcoding is fine ?

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

>  	const struct ov9282_reg_list *reg_list;
> +	int bitdepth_index;
>  	int ret;
>
>  	/* Write common registers */
> @@ -914,6 +965,13 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
>  		return ret;
>  	}
>
> +	bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1;
> +	ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2);
> +	if (ret) {
> +		dev_err(ov9282->dev, "fail to write bitdepth regs");
> +		return ret;
> +	}
> +
>  	/* Write sensor mode registers */
>  	reg_list = &ov9282->cur_mode->reg_list;
>  	ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
> @@ -1235,9 +1293,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  			  0, 1, 1, 1);
>
>  	/* Read only controls */
> -	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
> -			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> -			  OV9282_PIXEL_RATE);
> +	ov9282->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> +					       V4L2_CID_PIXEL_RATE,
> +					       OV9282_PIXEL_RATE_10BIT,
> +					       OV9282_PIXEL_RATE_10BIT, 1,
> +					       OV9282_PIXEL_RATE_10BIT);
>
>  	ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>  							&ov9282_ctrl_ops,
> @@ -1319,6 +1379,7 @@ static int ov9282_probe(struct i2c_client *client)
>
>  	/* Set default mode to first mode */
>  	ov9282->cur_mode = &supported_modes[DEFAULT_MODE];
> +	ov9282->code = MEDIA_BUS_FMT_Y10_1X10;
>  	ov9282->vblank = ov9282->cur_mode->vblank;
>
>  	ret = ov9282_init_controls(ov9282);
> --
> 2.34.1
>

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Alexander Stein]

Hello Dave,

Am Freitag, 28. Oktober 2022, 18:09:01 CET schrieb Dave Stevenson:
> The sensor supports 8 or 10 bit readout modes, but the
> driver only supported 10 bit.
> 
> Add 8 bit readout.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 95 +++++++++++++++++++++++++++++++-------
>  1 file changed, 78 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index c169b532ec8b..e2a98f480b58 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -21,6 +21,10 @@
>  #define OV9282_MODE_STANDBY	0x00
>  #define OV9282_MODE_STREAMING	0x01
> 
> +#define OV9282_REG_PLL_CTRL_0D	0x030d
> +#define OV9282_PLL_CTRL_0D_RAW8		0x60
> +#define OV9282_PLL_CTRL_0D_RAW10	0x50
> +
>  #define OV9282_REG_TIMING_HTS	0x380c
>  #define OV9282_TIMING_HTS_MAX	0x7fff
> 
> @@ -48,6 +52,10 @@
>  /* Group hold register */
>  #define OV9282_REG_HOLD		0x3308
> 
> +#define OV9282_REG_ANA_CORE_2	0x3662
> +#define OV9282_ANA_CORE2_RAW8	0x07
> +#define OV9282_ANA_CORE2_RAW10	0x05
> +
>  #define OV9282_REG_TIMING_FORMAT_1	0x3820
>  #define OV9282_REG_TIMING_FORMAT_2	0x3821
>  #define OV9282_FLIP_BIT			BIT(2)
> @@ -63,8 +71,10 @@
>  #define OV9282_NUM_DATA_LANES	2
> 
>  /* Pixel rate */
> -#define OV9282_PIXEL_RATE	(OV9282_LINK_FREQ * 2 * \
> -				 OV9282_NUM_DATA_LANES / 10)
> +#define OV9282_PIXEL_RATE_10BIT		(OV9282_LINK_FREQ * 2 * \
> +					 OV9282_NUM_DATA_LANES / 10)
> +#define OV9282_PIXEL_RATE_8BIT		(OV9282_LINK_FREQ * 2 * \
> +					 OV9282_NUM_DATA_LANES / 8)
> 
>  /*
>   * OV9282 native and active pixel array size.
> @@ -140,6 +150,7 @@ struct ov9282_mode {
>   * @again_ctrl: Pointer to analog gain control
>   * @vblank: Vertical blanking in lines
>   * @cur_mode: Pointer to current selected sensor mode
> + * @code: Mbus code currently selected
>   * @mutex: Mutex for serializing sensor controls
>   * @streaming: Flag indicating streaming state
>   */
> @@ -158,9 +169,11 @@ struct ov9282 {
>  		struct v4l2_ctrl *exp_ctrl;
>  		struct v4l2_ctrl *again_ctrl;
>  	};
> +	struct v4l2_ctrl *pixel_rate;
>  	u32 vblank;
>  	bool noncontinuous_clock;
>  	const struct ov9282_mode *cur_mode;
> +	u32 code;
>  	struct mutex mutex;
>  	bool streaming;
>  };
> @@ -178,7 +191,6 @@ static const s64 link_freq[] = {
>   */
>  static const struct ov9282_reg common_regs[] = {
>  	{0x0302, 0x32},
> -	{0x030d, 0x50},
>  	{0x030e, 0x02},
>  	{0x3001, 0x00},
>  	{0x3004, 0x00},
> @@ -516,19 +528,29 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
>   * ov9282_update_controls() - Update control ranges based on streaming mode
> * @ov9282: pointer to ov9282 device
>   * @mode: pointer to ov9282_mode sensor mode
> + * @fmt: pointer to the requested mode
>   *
>   * Return: 0 if successful, error code otherwise.
>   */
>  static int ov9282_update_controls(struct ov9282 *ov9282,
> -				  const struct ov9282_mode *mode)
> +				  const struct ov9282_mode *mode,
> +				  const struct v4l2_subdev_format 
*fmt)
>  {
>  	u32 hblank_min;
> +	s64 pixel_rate;
>  	int ret;
> 
>  	ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode-
>link_freq_idx);
>  	if (ret)
>  		return ret;
> 
> +	pixel_rate = (fmt->format.code == MEDIA_BUS_FMT_Y10_1X10) ?
> +		OV9282_PIXEL_RATE_10BIT : OV9282_PIXEL_RATE_8BIT;
> +	ret = __v4l2_ctrl_modify_range(ov9282->pixel_rate, pixel_rate,
> +				       pixel_rate, 1, pixel_rate);
> +	if (ret)
> +		return ret;
> +
>  	hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
>  	ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
>  					OV9282_TIMING_HTS_MAX - 
mode->width, 1,
> @@ -698,10 +720,16 @@ static int ov9282_enum_mbus_code(struct v4l2_subdev
> *sd, struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum 
*code)
>  {
> -	if (code->index > 0)
> +	switch (code->index) {
> +	case 0:
> +		code->code = MEDIA_BUS_FMT_Y10_1X10;
> +		break;
> +	case 1:
> +		code->code = MEDIA_BUS_FMT_Y8_1X8;
> +		break;
> +	default:
>  		return -EINVAL;
> -
> -	code->code = MEDIA_BUS_FMT_Y10_1X10;
> +	}
> 
>  	return 0;
>  }
> @@ -721,7 +749,8 @@ static int ov9282_enum_frame_size(struct v4l2_subdev
> *sd, if (fsize->index >= ARRAY_SIZE(supported_modes))
>  		return -EINVAL;
> 
> -	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
> +	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10 &&
> +	    fsize->code != MEDIA_BUS_FMT_Y8_1X8)
>  		return -EINVAL;
> 
>  	fsize->min_width = supported_modes[fsize->index].width;
> @@ -737,15 +766,17 @@ static int ov9282_enum_frame_size(struct v4l2_subdev
> *sd, *                            from selected sensor mode
>   * @ov9282: pointer to ov9282 device
>   * @mode: pointer to ov9282_mode sensor mode
> + * @code: mbus code to be stored
>   * @fmt: V4L2 sub-device format need to be filled
>   */
>  static void ov9282_fill_pad_format(struct ov9282 *ov9282,
>  				   const struct ov9282_mode *mode,
> +				   u32 code,
>  				   struct v4l2_subdev_format *fmt)
>  {
>  	fmt->format.width = mode->width;
>  	fmt->format.height = mode->height;
> -	fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
> +	fmt->format.code = code;
>  	fmt->format.field = V4L2_FIELD_NONE;
>  	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
>  	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> @@ -775,7 +806,8 @@ static int ov9282_get_pad_format(struct v4l2_subdev *sd,
> framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
>  		fmt->format = *framefmt;
>  	} else {
> -		ov9282_fill_pad_format(ov9282, ov9282->cur_mode, fmt);
> +		ov9282_fill_pad_format(ov9282, ov9282->cur_mode, ov9282-
>code,
> +				       fmt);
>  	}
> 
>  	mutex_unlock(&ov9282->mutex);
> @@ -797,6 +829,7 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
> {
>  	struct ov9282 *ov9282 = to_ov9282(sd);
>  	const struct ov9282_mode *mode;
> +	u32 code;
>  	int ret = 0;
> 
>  	mutex_lock(&ov9282->mutex);
> @@ -806,7 +839,12 @@ static int ov9282_set_pad_format(struct v4l2_subdev
> *sd, width, height,
>  				      fmt->format.width,
>  				      fmt->format.height);
> -	ov9282_fill_pad_format(ov9282, mode, fmt);
> +	if (fmt->format.code == MEDIA_BUS_FMT_Y8_1X8)
> +		code = MEDIA_BUS_FMT_Y8_1X8;
> +	else
> +		code = MEDIA_BUS_FMT_Y10_1X10;
> +
> +	ov9282_fill_pad_format(ov9282, mode, code, fmt);
> 
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>  		struct v4l2_mbus_framefmt *framefmt;
> @@ -814,9 +852,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev
> *sd, framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
>  		*framefmt = fmt->format;
>  	} else {
> -		ret = ov9282_update_controls(ov9282, mode);
> -		if (!ret)
> +		ret = ov9282_update_controls(ov9282, mode, fmt);
> +		if (!ret) {
>  			ov9282->cur_mode = mode;
> +			ov9282->code = code;
> +		}
>  	}
> 
>  	mutex_unlock(&ov9282->mutex);
> @@ -838,7 +878,8 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_subdev_format fmt = { 0 };
> 
>  	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : 
V4L2_SUBDEV_FORMAT_ACTIVE;
> -	ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE], 
&fmt);
> +	ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE],
> +			       ov9282->code, &fmt);
> 
>  	return ov9282_set_pad_format(sd, sd_state, &fmt);
>  }
> @@ -903,7 +944,17 @@ static int ov9282_get_selection(struct v4l2_subdev *sd,
> */
>  static int ov9282_start_streaming(struct ov9282 *ov9282)
>  {
> +	const struct ov9282_reg bitdepth_regs[2][2] = {
> +		{
> +			{OV9282_REG_PLL_CTRL_0D, 
OV9282_PLL_CTRL_0D_RAW10},
> +			{OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10},
> +		}, {
> +			{OV9282_REG_PLL_CTRL_0D, 
OV9282_PLL_CTRL_0D_RAW8},
> +			{OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW8},
> +		}
> +	};
>  	const struct ov9282_reg_list *reg_list;
> +	int bitdepth_index;
>  	int ret;
> 
>  	/* Write common registers */
> @@ -914,6 +965,13 @@ static int ov9282_start_streaming(struct ov9282
> *ov9282) return ret;
>  	}
> 
> +	bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1;
> +	ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2);
> +	if (ret) {
> +		dev_err(ov9282->dev, "fail to write bitdepth regs");
> +		return ret;
> +	}
> +
>  	/* Write sensor mode registers */
>  	reg_list = &ov9282->cur_mode->reg_list;
>  	ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list-
>num_of_regs);
> @@ -1235,9 +1293,11 @@ static int ov9282_init_controls(struct ov9282
> *ov9282) 0, 1, 1, 1);
> 
>  	/* Read only controls */
> -	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
> -			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> -			  OV9282_PIXEL_RATE);
> +	ov9282->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> +					       V4L2_CID_PIXEL_RATE,
> +					       
OV9282_PIXEL_RATE_10BIT,
> +					       
OV9282_PIXEL_RATE_10BIT, 1,
> +					       
OV9282_PIXEL_RATE_10BIT);
> 
>  	ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>  							
&ov9282_ctrl_ops,
> @@ -1319,6 +1379,7 @@ static int ov9282_probe(struct i2c_client *client)
> 
>  	/* Set default mode to first mode */
>  	ov9282->cur_mode = &supported_modes[DEFAULT_MODE];
> +	ov9282->code = MEDIA_BUS_FMT_Y10_1X10;
>  	ov9282->vblank = ov9282->cur_mode->vblank;
> 
>  	ret = ov9282_init_controls(ov9282);

Using this series I was able to do some camera playback on LVDS display on 
imx8mm based platform (TQMa8MxML). My command was 'gst-launch-1.0 v4l2src 
device=/dev/video0 ! video/x-
raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 ! videoconvert ! 
waylandsink'
But due to SW colorspace conversion this is awfully slow.
Using a testsink I get about 72FPS on 1280x720 for GREY. Is this to be 
expected?
I used 'gst-launch-1.0 v4l2src device=/dev/video0 ! video/x-
raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 ! fpsdisplaysink 
video-sink="testsink" text-overlay=false silent=false sync=false -v' for that.

Apparently gstreamer does not support Y10. Do you have a different way 
toactually use Y10?

Thanks
Alexander

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Dave Stevenson]

Hi Alexander

On Tue, 1 Nov 2022 at 11:59, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hello Dave,
>
> Am Freitag, 28. Oktober 2022, 18:09:01 CET schrieb Dave Stevenson:
> > The sensor supports 8 or 10 bit readout modes, but the
> > driver only supported 10 bit.
> >
> > Add 8 bit readout.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 95 +++++++++++++++++++++++++++++++-------
> >  1 file changed, 78 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index c169b532ec8b..e2a98f480b58 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -21,6 +21,10 @@
> >  #define OV9282_MODE_STANDBY  0x00
> >  #define OV9282_MODE_STREAMING        0x01
> >
> > +#define OV9282_REG_PLL_CTRL_0D       0x030d
> > +#define OV9282_PLL_CTRL_0D_RAW8              0x60
> > +#define OV9282_PLL_CTRL_0D_RAW10     0x50
> > +
> >  #define OV9282_REG_TIMING_HTS        0x380c
> >  #define OV9282_TIMING_HTS_MAX        0x7fff
> >
> > @@ -48,6 +52,10 @@
> >  /* Group hold register */
> >  #define OV9282_REG_HOLD              0x3308
> >
> > +#define OV9282_REG_ANA_CORE_2        0x3662
> > +#define OV9282_ANA_CORE2_RAW8        0x07
> > +#define OV9282_ANA_CORE2_RAW10       0x05
> > +
> >  #define OV9282_REG_TIMING_FORMAT_1   0x3820
> >  #define OV9282_REG_TIMING_FORMAT_2   0x3821
> >  #define OV9282_FLIP_BIT                      BIT(2)
> > @@ -63,8 +71,10 @@
> >  #define OV9282_NUM_DATA_LANES        2
> >
> >  /* Pixel rate */
> > -#define OV9282_PIXEL_RATE    (OV9282_LINK_FREQ * 2 * \
> > -                              OV9282_NUM_DATA_LANES / 10)
> > +#define OV9282_PIXEL_RATE_10BIT              (OV9282_LINK_FREQ * 2 * \
> > +                                      OV9282_NUM_DATA_LANES / 10)
> > +#define OV9282_PIXEL_RATE_8BIT               (OV9282_LINK_FREQ * 2 * \
> > +                                      OV9282_NUM_DATA_LANES / 8)
> >
> >  /*
> >   * OV9282 native and active pixel array size.
> > @@ -140,6 +150,7 @@ struct ov9282_mode {
> >   * @again_ctrl: Pointer to analog gain control
> >   * @vblank: Vertical blanking in lines
> >   * @cur_mode: Pointer to current selected sensor mode
> > + * @code: Mbus code currently selected
> >   * @mutex: Mutex for serializing sensor controls
> >   * @streaming: Flag indicating streaming state
> >   */
> > @@ -158,9 +169,11 @@ struct ov9282 {
> >               struct v4l2_ctrl *exp_ctrl;
> >               struct v4l2_ctrl *again_ctrl;
> >       };
> > +     struct v4l2_ctrl *pixel_rate;
> >       u32 vblank;
> >       bool noncontinuous_clock;
> >       const struct ov9282_mode *cur_mode;
> > +     u32 code;
> >       struct mutex mutex;
> >       bool streaming;
> >  };
> > @@ -178,7 +191,6 @@ static const s64 link_freq[] = {
> >   */
> >  static const struct ov9282_reg common_regs[] = {
> >       {0x0302, 0x32},
> > -     {0x030d, 0x50},
> >       {0x030e, 0x02},
> >       {0x3001, 0x00},
> >       {0x3004, 0x00},
> > @@ -516,19 +528,29 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
> >   * ov9282_update_controls() - Update control ranges based on streaming mode
> > * @ov9282: pointer to ov9282 device
> >   * @mode: pointer to ov9282_mode sensor mode
> > + * @fmt: pointer to the requested mode
> >   *
> >   * Return: 0 if successful, error code otherwise.
> >   */
> >  static int ov9282_update_controls(struct ov9282 *ov9282,
> > -                               const struct ov9282_mode *mode)
> > +                               const struct ov9282_mode *mode,
> > +                               const struct v4l2_subdev_format
> *fmt)
> >  {
> >       u32 hblank_min;
> > +     s64 pixel_rate;
> >       int ret;
> >
> >       ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode-
> >link_freq_idx);
> >       if (ret)
> >               return ret;
> >
> > +     pixel_rate = (fmt->format.code == MEDIA_BUS_FMT_Y10_1X10) ?
> > +             OV9282_PIXEL_RATE_10BIT : OV9282_PIXEL_RATE_8BIT;
> > +     ret = __v4l2_ctrl_modify_range(ov9282->pixel_rate, pixel_rate,
> > +                                    pixel_rate, 1, pixel_rate);
> > +     if (ret)
> > +             return ret;
> > +
> >       hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> >       ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
> >                                       OV9282_TIMING_HTS_MAX -
> mode->width, 1,
> > @@ -698,10 +720,16 @@ static int ov9282_enum_mbus_code(struct v4l2_subdev
> > *sd, struct v4l2_subdev_state *sd_state,
> >                                struct v4l2_subdev_mbus_code_enum
> *code)
> >  {
> > -     if (code->index > 0)
> > +     switch (code->index) {
> > +     case 0:
> > +             code->code = MEDIA_BUS_FMT_Y10_1X10;
> > +             break;
> > +     case 1:
> > +             code->code = MEDIA_BUS_FMT_Y8_1X8;
> > +             break;
> > +     default:
> >               return -EINVAL;
> > -
> > -     code->code = MEDIA_BUS_FMT_Y10_1X10;
> > +     }
> >
> >       return 0;
> >  }
> > @@ -721,7 +749,8 @@ static int ov9282_enum_frame_size(struct v4l2_subdev
> > *sd, if (fsize->index >= ARRAY_SIZE(supported_modes))
> >               return -EINVAL;
> >
> > -     if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
> > +     if (fsize->code != MEDIA_BUS_FMT_Y10_1X10 &&
> > +         fsize->code != MEDIA_BUS_FMT_Y8_1X8)
> >               return -EINVAL;
> >
> >       fsize->min_width = supported_modes[fsize->index].width;
> > @@ -737,15 +766,17 @@ static int ov9282_enum_frame_size(struct v4l2_subdev
> > *sd, *                            from selected sensor mode
> >   * @ov9282: pointer to ov9282 device
> >   * @mode: pointer to ov9282_mode sensor mode
> > + * @code: mbus code to be stored
> >   * @fmt: V4L2 sub-device format need to be filled
> >   */
> >  static void ov9282_fill_pad_format(struct ov9282 *ov9282,
> >                                  const struct ov9282_mode *mode,
> > +                                u32 code,
> >                                  struct v4l2_subdev_format *fmt)
> >  {
> >       fmt->format.width = mode->width;
> >       fmt->format.height = mode->height;
> > -     fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
> > +     fmt->format.code = code;
> >       fmt->format.field = V4L2_FIELD_NONE;
> >       fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> >       fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > @@ -775,7 +806,8 @@ static int ov9282_get_pad_format(struct v4l2_subdev *sd,
> > framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> >               fmt->format = *framefmt;
> >       } else {
> > -             ov9282_fill_pad_format(ov9282, ov9282->cur_mode, fmt);
> > +             ov9282_fill_pad_format(ov9282, ov9282->cur_mode, ov9282-
> >code,
> > +                                    fmt);
> >       }
> >
> >       mutex_unlock(&ov9282->mutex);
> > @@ -797,6 +829,7 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
> > {
> >       struct ov9282 *ov9282 = to_ov9282(sd);
> >       const struct ov9282_mode *mode;
> > +     u32 code;
> >       int ret = 0;
> >
> >       mutex_lock(&ov9282->mutex);
> > @@ -806,7 +839,12 @@ static int ov9282_set_pad_format(struct v4l2_subdev
> > *sd, width, height,
> >                                     fmt->format.width,
> >                                     fmt->format.height);
> > -     ov9282_fill_pad_format(ov9282, mode, fmt);
> > +     if (fmt->format.code == MEDIA_BUS_FMT_Y8_1X8)
> > +             code = MEDIA_BUS_FMT_Y8_1X8;
> > +     else
> > +             code = MEDIA_BUS_FMT_Y10_1X10;
> > +
> > +     ov9282_fill_pad_format(ov9282, mode, code, fmt);
> >
> >       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> >               struct v4l2_mbus_framefmt *framefmt;
> > @@ -814,9 +852,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev
> > *sd, framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> >               *framefmt = fmt->format;
> >       } else {
> > -             ret = ov9282_update_controls(ov9282, mode);
> > -             if (!ret)
> > +             ret = ov9282_update_controls(ov9282, mode, fmt);
> > +             if (!ret) {
> >                       ov9282->cur_mode = mode;
> > +                     ov9282->code = code;
> > +             }
> >       }
> >
> >       mutex_unlock(&ov9282->mutex);
> > @@ -838,7 +878,8 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
> >       struct v4l2_subdev_format fmt = { 0 };
> >
> >       fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY :
> V4L2_SUBDEV_FORMAT_ACTIVE;
> > -     ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE],
> &fmt);
> > +     ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE],
> > +                            ov9282->code, &fmt);
> >
> >       return ov9282_set_pad_format(sd, sd_state, &fmt);
> >  }
> > @@ -903,7 +944,17 @@ static int ov9282_get_selection(struct v4l2_subdev *sd,
> > */
> >  static int ov9282_start_streaming(struct ov9282 *ov9282)
> >  {
> > +     const struct ov9282_reg bitdepth_regs[2][2] = {
> > +             {
> > +                     {OV9282_REG_PLL_CTRL_0D,
> OV9282_PLL_CTRL_0D_RAW10},
> > +                     {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10},
> > +             }, {
> > +                     {OV9282_REG_PLL_CTRL_0D,
> OV9282_PLL_CTRL_0D_RAW8},
> > +                     {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW8},
> > +             }
> > +     };
> >       const struct ov9282_reg_list *reg_list;
> > +     int bitdepth_index;
> >       int ret;
> >
> >       /* Write common registers */
> > @@ -914,6 +965,13 @@ static int ov9282_start_streaming(struct ov9282
> > *ov9282) return ret;
> >       }
> >
> > +     bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1;
> > +     ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2);
> > +     if (ret) {
> > +             dev_err(ov9282->dev, "fail to write bitdepth regs");
> > +             return ret;
> > +     }
> > +
> >       /* Write sensor mode registers */
> >       reg_list = &ov9282->cur_mode->reg_list;
> >       ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list-
> >num_of_regs);
> > @@ -1235,9 +1293,11 @@ static int ov9282_init_controls(struct ov9282
> > *ov9282) 0, 1, 1, 1);
> >
> >       /* Read only controls */
> > -     v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
> > -                       OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> > -                       OV9282_PIXEL_RATE);
> > +     ov9282->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> > +                                            V4L2_CID_PIXEL_RATE,
> > +
> OV9282_PIXEL_RATE_10BIT,
> > +
> OV9282_PIXEL_RATE_10BIT, 1,
> > +
> OV9282_PIXEL_RATE_10BIT);
> >
> >       ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> >
> &ov9282_ctrl_ops,
> > @@ -1319,6 +1379,7 @@ static int ov9282_probe(struct i2c_client *client)
> >
> >       /* Set default mode to first mode */
> >       ov9282->cur_mode = &supported_modes[DEFAULT_MODE];
> > +     ov9282->code = MEDIA_BUS_FMT_Y10_1X10;
> >       ov9282->vblank = ov9282->cur_mode->vblank;
> >
> >       ret = ov9282_init_controls(ov9282);
>
> Using this series I was able to do some camera playback on LVDS display on
> imx8mm based platform (TQMa8MxML). My command was 'gst-launch-1.0 v4l2src
> device=/dev/video0 ! video/x-
> raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 ! videoconvert !
> waylandsink'
> But due to SW colorspace conversion this is awfully slow.
> Using a testsink I get about 72FPS on 1280x720 for GREY. Is this to be
> expected?
> I used 'gst-launch-1.0 v4l2src device=/dev/video0 ! video/x-
> raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 ! fpsdisplaysink
> video-sink="testsink" text-overlay=false silent=false sync=false -v' for that.

AFAIK v4l2src doesn't map from a caps framerate=30/1 to the relevant
V4L2_CID_VBLANK and V4L2_CID_HBLANK controls used by raw sensors for
frame rate control (see docs at [1]). The sensor will therefore stream
at whatever rate the controls get left at.
I'm assuming you're not using Media Controller either, as v4l2src
won't set up Media Controller links correctly either.

Running a Raspberry Pi in the same non-Media Controller mode:
v4l2-ctl -v width=1280,height=800,pixelformat=Y10P
v4l2-ctl --stream-mmap=3 --stream-count=1000 --stream-to=/dev/null
gives me 60.28fps.

HBLANK defaults to 176, and VBLANK to 1022:
160MPix/s / ((1280+176) * (800+1022)) = 60.3fps.

v4l2-ctl -v width=1280,height=800,pixelformat=GREY
v4l2-ctl --stream-mmap=3 --stream-count=1000 --stream-to=/dev/null
Gives me 72.33fps as neither HBLANK nor VBLANK have been altered, but
V4L2_CID_PIXEL_RATE has been increased.

Run the numbers the other way for eg 120fps
200MPix/s / ( 120fps * (width 1280 + HBLANK 176)) - height (800) = VBLANK = 344
v4l2-ctl --set-ctrl=vertical_blanking=344
Streaming with that gives me 115.17fps, so you're now making me
question the Y8 pixel rate.
192MPix/s appears to be the right value to make the numbers work.

I don't recall where I'd got the 200MPix/s value from - it's not
documented in the datasheet, but presumably from 160 * 10 / 8
(switching from 10 to 8 bits at the same output rate). You're the
first to notice the rates are off, although at least it's less than
the factor of two that this driver used to be out by.

Sakari: Do you want a new version of the patchset, or just a fixup on top?

[1] https://www.kernel.org/doc/html/latest/driver-api/media/camera-sensor.html#raw-camera-sensors

> Apparently gstreamer does not support Y10. Do you have a different way
> to actually use Y10?

We're using libcamera on Raspberry Pi. The Pi ISP will happily consume
raw 8, 10, 12, 14, or 16, and spit out YUV or RGB.
The alternative is v4l2-ctl as above.

Depending on what unpacking your platform is capable of, you may be
able to request V4L2_PIX_FMT_Y10 (10bit sample in a 16bit word) and
then pass it through GStreamer as either GRAY16_LE or GRAY16_BE.
V4L2_PIX_FMT_Y10P is a bit of a pain to handle (4 pixels in 5 bytes as
described in [2]), but is more efficient on memory usage.

Do note that this is still a raw image sensor and therefore the images
will generally have a non-zero black level, and quite probably lens
shading artifacts. They should not be considered as a standard luma
signal.

  Dave

[2]  https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-yuv-luma.html

> Thanks
> Alexander
>
>
>

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Alexander Stein]

Hi Dave,

thanks for the fast reply.

Am Dienstag, 1. November 2022, 14:47:16 CET schrieb Dave Stevenson:
> Hi Alexander
> 
> On Tue, 1 Nov 2022 at 11:59, Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hello Dave,
> > 
> > Am Freitag, 28. Oktober 2022, 18:09:01 CET schrieb Dave Stevenson:
> > > The sensor supports 8 or 10 bit readout modes, but the
> > > driver only supported 10 bit.
> > > 
> > > Add 8 bit readout.
> > > 
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > > 
> > >  drivers/media/i2c/ov9282.c | 95 +++++++++++++++++++++++++++++++-------
> > >  1 file changed, 78 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index c169b532ec8b..e2a98f480b58 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -21,6 +21,10 @@
> > > 
> > >  #define OV9282_MODE_STANDBY  0x00
> > >  #define OV9282_MODE_STREAMING        0x01
> > > 
> > > +#define OV9282_REG_PLL_CTRL_0D       0x030d
> > > +#define OV9282_PLL_CTRL_0D_RAW8              0x60
> > > +#define OV9282_PLL_CTRL_0D_RAW10     0x50
> > > +
> > > 
> > >  #define OV9282_REG_TIMING_HTS        0x380c
> > >  #define OV9282_TIMING_HTS_MAX        0x7fff
> > > 
> > > @@ -48,6 +52,10 @@
> > > 
> > >  /* Group hold register */
> > >  #define OV9282_REG_HOLD              0x3308
> > > 
> > > +#define OV9282_REG_ANA_CORE_2        0x3662
> > > +#define OV9282_ANA_CORE2_RAW8        0x07
> > > +#define OV9282_ANA_CORE2_RAW10       0x05
> > > +
> > > 
> > >  #define OV9282_REG_TIMING_FORMAT_1   0x3820
> > >  #define OV9282_REG_TIMING_FORMAT_2   0x3821
> > >  #define OV9282_FLIP_BIT                      BIT(2)
> > > 
> > > @@ -63,8 +71,10 @@
> > > 
> > >  #define OV9282_NUM_DATA_LANES        2
> > >  
> > >  /* Pixel rate */
> > > 
> > > -#define OV9282_PIXEL_RATE    (OV9282_LINK_FREQ * 2 * \
> > > -                              OV9282_NUM_DATA_LANES / 10)
> > > +#define OV9282_PIXEL_RATE_10BIT              (OV9282_LINK_FREQ * 2 * \
> > > +                                      OV9282_NUM_DATA_LANES / 10)
> > > +#define OV9282_PIXEL_RATE_8BIT               (OV9282_LINK_FREQ * 2 * \
> > > +                                      OV9282_NUM_DATA_LANES / 8)
> > > 
> > >  /*
> > >  
> > >   * OV9282 native and active pixel array size.
> > > 
> > > @@ -140,6 +150,7 @@ struct ov9282_mode {
> > > 
> > >   * @again_ctrl: Pointer to analog gain control
> > >   * @vblank: Vertical blanking in lines
> > >   * @cur_mode: Pointer to current selected sensor mode
> > > 
> > > + * @code: Mbus code currently selected
> > > 
> > >   * @mutex: Mutex for serializing sensor controls
> > >   * @streaming: Flag indicating streaming state
> > >   */
> > > 
> > > @@ -158,9 +169,11 @@ struct ov9282 {
> > > 
> > >               struct v4l2_ctrl *exp_ctrl;
> > >               struct v4l2_ctrl *again_ctrl;
> > >       
> > >       };
> > > 
> > > +     struct v4l2_ctrl *pixel_rate;
> > > 
> > >       u32 vblank;
> > >       bool noncontinuous_clock;
> > >       const struct ov9282_mode *cur_mode;
> > > 
> > > +     u32 code;
> > > 
> > >       struct mutex mutex;
> > >       bool streaming;
> > >  
> > >  };
> > > 
> > > @@ -178,7 +191,6 @@ static const s64 link_freq[] = {
> > > 
> > >   */
> > >  
> > >  static const struct ov9282_reg common_regs[] = {
> > >  
> > >       {0x0302, 0x32},
> > > 
> > > -     {0x030d, 0x50},
> > > 
> > >       {0x030e, 0x02},
> > >       {0x3001, 0x00},
> > >       {0x3004, 0x00},
> > > 
> > > @@ -516,19 +528,29 @@ static int ov9282_write_regs(struct ov9282
> > > *ov9282,
> > > 
> > >   * ov9282_update_controls() - Update control ranges based on streaming
> > >   mode
> > > 
> > > * @ov9282: pointer to ov9282 device
> > > 
> > >   * @mode: pointer to ov9282_mode sensor mode
> > > 
> > > + * @fmt: pointer to the requested mode
> > > 
> > >   *
> > >   * Return: 0 if successful, error code otherwise.
> > >   */
> > >  
> > >  static int ov9282_update_controls(struct ov9282 *ov9282,
> > > 
> > > -                               const struct ov9282_mode *mode)
> > > +                               const struct ov9282_mode *mode,
> > > +                               const struct v4l2_subdev_format
> > 
> > *fmt)
> > 
> > >  {
> > >  
> > >       u32 hblank_min;
> > > 
> > > +     s64 pixel_rate;
> > > 
> > >       int ret;
> > >       
> > >       ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode-
> > >
> > >link_freq_idx);
> > >
> > >       if (ret)
> > >       
> > >               return ret;
> > > 
> > > +     pixel_rate = (fmt->format.code == MEDIA_BUS_FMT_Y10_1X10) ?
> > > +             OV9282_PIXEL_RATE_10BIT : OV9282_PIXEL_RATE_8BIT;
> > > +     ret = __v4l2_ctrl_modify_range(ov9282->pixel_rate, pixel_rate,
> > > +                                    pixel_rate, 1, pixel_rate);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > 
> > >       hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 :
> > >       1];
> > >       ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
> > >       
> > >                                       OV9282_TIMING_HTS_MAX -
> > 
> > mode->width, 1,
> > 
> > > @@ -698,10 +720,16 @@ static int ov9282_enum_mbus_code(struct
> > > v4l2_subdev
> > > *sd, struct v4l2_subdev_state *sd_state,
> > > 
> > >                                struct v4l2_subdev_mbus_code_enum
> > 
> > *code)
> > 
> > >  {
> > > 
> > > -     if (code->index > 0)
> > > +     switch (code->index) {
> > > +     case 0:
> > > +             code->code = MEDIA_BUS_FMT_Y10_1X10;
> > > +             break;
> > > +     case 1:
> > > +             code->code = MEDIA_BUS_FMT_Y8_1X8;
> > > +             break;
> > > 
> > > +     default:
> > >               return -EINVAL;
> > > 
> > > -
> > > -     code->code = MEDIA_BUS_FMT_Y10_1X10;
> > > +     }
> > > 
> > >       return 0;
> > >  
> > >  }
> > > 
> > > @@ -721,7 +749,8 @@ static int ov9282_enum_frame_size(struct v4l2_subdev
> > > *sd, if (fsize->index >= ARRAY_SIZE(supported_modes))
> > > 
> > >               return -EINVAL;
> > > 
> > > -     if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
> > > +     if (fsize->code != MEDIA_BUS_FMT_Y10_1X10 &&
> > > +         fsize->code != MEDIA_BUS_FMT_Y8_1X8)
> > > 
> > >               return -EINVAL;
> > >       
> > >       fsize->min_width = supported_modes[fsize->index].width;
> > > 
> > > @@ -737,15 +766,17 @@ static int ov9282_enum_frame_size(struct
> > > v4l2_subdev
> > > *sd, *                            from selected sensor mode
> > > 
> > >   * @ov9282: pointer to ov9282 device
> > >   * @mode: pointer to ov9282_mode sensor mode
> > > 
> > > + * @code: mbus code to be stored
> > > 
> > >   * @fmt: V4L2 sub-device format need to be filled
> > >   */
> > >  
> > >  static void ov9282_fill_pad_format(struct ov9282 *ov9282,
> > >  
> > >                                  const struct ov9282_mode *mode,
> > > 
> > > +                                u32 code,
> > > 
> > >                                  struct v4l2_subdev_format *fmt)
> > >  
> > >  {
> > >  
> > >       fmt->format.width = mode->width;
> > >       fmt->format.height = mode->height;
> > > 
> > > -     fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
> > > +     fmt->format.code = code;
> > > 
> > >       fmt->format.field = V4L2_FIELD_NONE;
> > >       fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> > >       fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > > 
> > > @@ -775,7 +806,8 @@ static int ov9282_get_pad_format(struct v4l2_subdev
> > > *sd, framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> > > 
> > >               fmt->format = *framefmt;
> > >       
> > >       } else {
> > > 
> > > -             ov9282_fill_pad_format(ov9282, ov9282->cur_mode, fmt);
> > > +             ov9282_fill_pad_format(ov9282, ov9282->cur_mode, ov9282-
> > >
> > >code,
> > >
> > > +                                    fmt);
> > > 
> > >       }
> > >       
> > >       mutex_unlock(&ov9282->mutex);
> > > 
> > > @@ -797,6 +829,7 @@ static int ov9282_set_pad_format(struct v4l2_subdev
> > > *sd, {
> > > 
> > >       struct ov9282 *ov9282 = to_ov9282(sd);
> > >       const struct ov9282_mode *mode;
> > > 
> > > +     u32 code;
> > > 
> > >       int ret = 0;
> > >       
> > >       mutex_lock(&ov9282->mutex);
> > > 
> > > @@ -806,7 +839,12 @@ static int ov9282_set_pad_format(struct v4l2_subdev
> > > *sd, width, height,
> > > 
> > >                                     fmt->format.width,
> > >                                     fmt->format.height);
> > > 
> > > -     ov9282_fill_pad_format(ov9282, mode, fmt);
> > > +     if (fmt->format.code == MEDIA_BUS_FMT_Y8_1X8)
> > > +             code = MEDIA_BUS_FMT_Y8_1X8;
> > > +     else
> > > +             code = MEDIA_BUS_FMT_Y10_1X10;
> > > +
> > > +     ov9282_fill_pad_format(ov9282, mode, code, fmt);
> > > 
> > >       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > >       
> > >               struct v4l2_mbus_framefmt *framefmt;
> > > 
> > > @@ -814,9 +852,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev
> > > *sd, framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> > > 
> > >               *framefmt = fmt->format;
> > >       
> > >       } else {
> > > 
> > > -             ret = ov9282_update_controls(ov9282, mode);
> > > -             if (!ret)
> > > +             ret = ov9282_update_controls(ov9282, mode, fmt);
> > > +             if (!ret) {
> > > 
> > >                       ov9282->cur_mode = mode;
> > > 
> > > +                     ov9282->code = code;
> > > +             }
> > > 
> > >       }
> > >       
> > >       mutex_unlock(&ov9282->mutex);
> > > 
> > > @@ -838,7 +878,8 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev
> > > *sd,
> > > 
> > >       struct v4l2_subdev_format fmt = { 0 };
> > 
> > >       fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY :
> > V4L2_SUBDEV_FORMAT_ACTIVE;
> > 
> > > -     ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE],
> > 
> > &fmt);
> > 
> > > +     ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE],
> > > +                            ov9282->code, &fmt);
> > > 
> > >       return ov9282_set_pad_format(sd, sd_state, &fmt);
> > >  
> > >  }
> > > 
> > > @@ -903,7 +944,17 @@ static int ov9282_get_selection(struct v4l2_subdev
> > > *sd, */
> > > 
> > >  static int ov9282_start_streaming(struct ov9282 *ov9282)
> > >  {
> > > 
> > > +     const struct ov9282_reg bitdepth_regs[2][2] = {
> > > +             {
> > > +                     {OV9282_REG_PLL_CTRL_0D,
> > 
> > OV9282_PLL_CTRL_0D_RAW10},
> > 
> > > +                     {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10},
> > > +             }, {
> > > +                     {OV9282_REG_PLL_CTRL_0D,
> > 
> > OV9282_PLL_CTRL_0D_RAW8},
> > 
> > > +                     {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW8},
> > > +             }
> > > +     };
> > > 
> > >       const struct ov9282_reg_list *reg_list;
> > > 
> > > +     int bitdepth_index;
> > > 
> > >       int ret;
> > >       
> > >       /* Write common registers */
> > > 
> > > @@ -914,6 +965,13 @@ static int ov9282_start_streaming(struct ov9282
> > > *ov9282) return ret;
> > > 
> > >       }
> > > 
> > > +     bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1;
> > > +     ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2);
> > > +     if (ret) {
> > > +             dev_err(ov9282->dev, "fail to write bitdepth regs");
> > > +             return ret;
> > > +     }
> > > +
> > > 
> > >       /* Write sensor mode registers */
> > >       reg_list = &ov9282->cur_mode->reg_list;
> > >       ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list-
> > >
> > >num_of_regs);
> > >
> > > @@ -1235,9 +1293,11 @@ static int ov9282_init_controls(struct ov9282
> > > *ov9282) 0, 1, 1, 1);
> > > 
> > >       /* Read only controls */
> > > 
> > > -     v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> > > V4L2_CID_PIXEL_RATE,
> > > -                       OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> > > -                       OV9282_PIXEL_RATE);
> > > +     ov9282->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr,
> > > &ov9282_ctrl_ops,
> > > +                                            V4L2_CID_PIXEL_RATE,
> > > +
> > 
> > OV9282_PIXEL_RATE_10BIT,
> > 
> > > +
> > 
> > OV9282_PIXEL_RATE_10BIT, 1,
> > 
> > > +
> > 
> > OV9282_PIXEL_RATE_10BIT);
> > 
> > >       ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > 
> > &ov9282_ctrl_ops,
> > 
> > > @@ -1319,6 +1379,7 @@ static int ov9282_probe(struct i2c_client *client)
> > > 
> > >       /* Set default mode to first mode */
> > >       ov9282->cur_mode = &supported_modes[DEFAULT_MODE];
> > > 
> > > +     ov9282->code = MEDIA_BUS_FMT_Y10_1X10;
> > > 
> > >       ov9282->vblank = ov9282->cur_mode->vblank;
> > >       
> > >       ret = ov9282_init_controls(ov9282);
> > 
> > Using this series I was able to do some camera playback on LVDS display on
> > imx8mm based platform (TQMa8MxML). My command was 'gst-launch-1.0 v4l2src
> > device=/dev/video0 ! video/x-
> > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 ! videoconvert
> > ! waylandsink'
> > But due to SW colorspace conversion this is awfully slow.
> > Using a testsink I get about 72FPS on 1280x720 for GREY. Is this to be
> > expected?
> > I used 'gst-launch-1.0 v4l2src device=/dev/video0 ! video/x-
> > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 !
> > fpsdisplaysink video-sink="testsink" text-overlay=false silent=false
> > sync=false -v' for that.
> AFAIK v4l2src doesn't map from a caps framerate=30/1 to the relevant
> V4L2_CID_VBLANK and V4L2_CID_HBLANK controls used by raw sensors for
> frame rate control (see docs at [1]). The sensor will therefore stream
> at whatever rate the controls get left at.

Yes I noticed the framerate caps has no effect. But I lack some kind of 
reference system to decide what should work and what not.

> I'm assuming you're not using Media Controller either, as v4l2src
> won't set up Media Controller links correctly either.

Well, actually I am using Media Controller. But I need to configure it before 
gstreamer usage. There is no specific reason for gstreamer, but we use this to 
verify features on downstream kernel.

For completeness here is one of my media-ctl setup:
media-ctl -l "'ov9282 2-0060':0->'csis-32e30000.mipi-csi':0 [1]"
media-ctl -V "'ov9282 2-0060':0 [fmt:Y8_1X8/1280x720 field:none 
colorspace:raw]"
media-ctl -V "'csi':0 [fmt:Y8_1X8/1280x720 field:none colorspace:raw]"
v4l2-ctl -d0 --set-fmt-video 
width=1280,height=720,pixelformat='GREY',field=none
media-ctl -p

Media controller API version 6.1.0

Media device information
------------------------
driver          imx7-csi
model           imx-media
serial          
bus info        platform:32e20000.csi
hw revision     0x0
driver version  6.1.0

Device topology
- entity 1: csi (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
        pad0: Sink
                [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none 
ycbcr:601 quantization:full-range]
                <- "csis-32e30000.mipi-csi":1 [ENABLED,IMMUTABLE]
        pad1: Source
                [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none 
ycbcr:601 quantization:full-range]
                -> "csi capture":0 [ENABLED,IMMUTABLE]

- entity 4: csi capture (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video0
        pad0: Sink
                <- "csi":1 [ENABLED,IMMUTABLE]

- entity 10: csis-32e30000.mipi-csi (2 pads, 2 links)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev1
        pad0: Sink
                [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none]
                <- "ov9281 2-0060":0 [ENABLED]
        pad1: Source
                [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none]
                -> "csi":0 [ENABLED,IMMUTABLE]

- entity 15: ov9282 2-0060 (1 pad, 1 link)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev2
        pad0: Source
                [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none
                 crop.bounds:(8,8)/1280x800
                 crop:(8,8)/1280x720]
                -> "csis-32e30000.mipi-csi":0 [ENABLED]


> Running a Raspberry Pi in the same non-Media Controller mode:
> v4l2-ctl -v width=1280,height=800,pixelformat=Y10P
> v4l2-ctl --stream-mmap=3 --stream-count=1000 --stream-to=/dev/null
> gives me 60.28fps.
> 
> HBLANK defaults to 176, and VBLANK to 1022:
> 160MPix/s / ((1280+176) * (800+1022)) = 60.3fps.
> 
> v4l2-ctl -v width=1280,height=800,pixelformat=GREY
> v4l2-ctl --stream-mmap=3 --stream-count=1000 --stream-to=/dev/null
> Gives me 72.33fps as neither HBLANK nor VBLANK have been altered, but
> V4L2_CID_PIXEL_RATE has been increased.
> 
> Run the numbers the other way for eg 120fps
> 200MPix/s / ( 120fps * (width 1280 + HBLANK 176)) - height (800) = VBLANK =
> 344 v4l2-ctl --set-ctrl=vertical_blanking=344
> Streaming with that gives me 115.17fps, so you're now making me
> question the Y8 pixel rate.
> 192MPix/s appears to be the right value to make the numbers work.

Mh, using v4l2-ctl --set-ctrl=vertical_blanking=344 -d /dev/v4l-subdev2 I get 
109.61fps for 1280x800.

> I don't recall where I'd got the 200MPix/s value from - it's not
> documented in the datasheet, but presumably from 160 * 10 / 8
> (switching from 10 to 8 bits at the same output rate). You're the
> first to notice the rates are off, although at least it's less than
> the factor of two that this driver used to be out by.

I admit I'm not fully sure which results are correct and what they are 
expected to be. But here are some results using the v4l-ctrl approach:
     | 1280x800 | 1280x720 | 640x400 |
-----+----------+----------+---------+
GREY |  68.84   |   72.0   |  73.50  |
Y10  |  57.37   |   60.0   |  73.50  |

All using their default vertical and horizontal blanking. Especially switching 
to 640x400 and then back to 1280x720 leaves the horizontal_blanking to the old 
(640) value, resulting in lower frame rates.

> Sakari: Do you want a new version of the patchset, or just a fixup on top?
> 
> [1]
> https://www.kernel.org/doc/html/latest/driver-api/media/camera-sensor.html#
> raw-camera-sensors
> > Apparently gstreamer does not support Y10. Do you have a different way
> > to actually use Y10?
> 
> We're using libcamera on Raspberry Pi. The Pi ISP will happily consume
> raw 8, 10, 12, 14, or 16, and spit out YUV or RGB.
> The alternative is v4l2-ctl as above.

There is no ISP on imx8mm, so I'm stuck to CPU conversion for now, as OpenGL 
based conversion in gstreamer does not work currently, but that's a different 
matter (it does work downstream on imx8mp though).

> Depending on what unpacking your platform is capable of, you may be
> able to request V4L2_PIX_FMT_Y10 (10bit sample in a 16bit word) and
> then pass it through GStreamer as either GRAY16_LE or GRAY16_BE.
> V4L2_PIX_FMT_Y10P is a bit of a pain to handle (4 pixels in 5 bytes as
> described in [2]), but is more efficient on memory usage.
> 
> Do note that this is still a raw image sensor and therefore the images
> will generally have a non-zero black level, and quite probably lens
> shading artifacts. They should not be considered as a standard luma
> signal.

Thanks for the hint about using GRAY16_LE and the explanations: I'm aware that 
there is more to configure, but right now I'm focusing on correct 
configuration and data transfer for the sensor data itself.

Best regards
Alexander

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Dave Stevenson]

Hi Alexander

On Tue, 1 Nov 2022 at 15:04, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi Dave,
>
> thanks for the fast reply.
>
> Am Dienstag, 1. November 2022, 14:47:16 CET schrieb Dave Stevenson:
> > Hi Alexander
> >
> > On Tue, 1 Nov 2022 at 11:59, Alexander Stein
> >
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Hello Dave,
> > >
> > > Am Freitag, 28. Oktober 2022, 18:09:01 CET schrieb Dave Stevenson:
> > > > The sensor supports 8 or 10 bit readout modes, but the
> > > > driver only supported 10 bit.
> > > >
> > > > Add 8 bit readout.
> > > >
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > ---
> > > >
> > > >  drivers/media/i2c/ov9282.c | 95 +++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 78 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > > index c169b532ec8b..e2a98f480b58 100644
> > > > --- a/drivers/media/i2c/ov9282.c
> > > > +++ b/drivers/media/i2c/ov9282.c
> > > > @@ -21,6 +21,10 @@
> > > >
> > > >  #define OV9282_MODE_STANDBY  0x00
> > > >  #define OV9282_MODE_STREAMING        0x01
> > > >
> > > > +#define OV9282_REG_PLL_CTRL_0D       0x030d
> > > > +#define OV9282_PLL_CTRL_0D_RAW8              0x60
> > > > +#define OV9282_PLL_CTRL_0D_RAW10     0x50
> > > > +
> > > >
> > > >  #define OV9282_REG_TIMING_HTS        0x380c
> > > >  #define OV9282_TIMING_HTS_MAX        0x7fff
> > > >
> > > > @@ -48,6 +52,10 @@
> > > >
> > > >  /* Group hold register */
> > > >  #define OV9282_REG_HOLD              0x3308
> > > >
> > > > +#define OV9282_REG_ANA_CORE_2        0x3662
> > > > +#define OV9282_ANA_CORE2_RAW8        0x07
> > > > +#define OV9282_ANA_CORE2_RAW10       0x05
> > > > +
> > > >
> > > >  #define OV9282_REG_TIMING_FORMAT_1   0x3820
> > > >  #define OV9282_REG_TIMING_FORMAT_2   0x3821
> > > >  #define OV9282_FLIP_BIT                      BIT(2)
> > > >
> > > > @@ -63,8 +71,10 @@
> > > >
> > > >  #define OV9282_NUM_DATA_LANES        2
> > > >
> > > >  /* Pixel rate */
> > > >
> > > > -#define OV9282_PIXEL_RATE    (OV9282_LINK_FREQ * 2 * \
> > > > -                              OV9282_NUM_DATA_LANES / 10)
> > > > +#define OV9282_PIXEL_RATE_10BIT              (OV9282_LINK_FREQ * 2 * \
> > > > +                                      OV9282_NUM_DATA_LANES / 10)
> > > > +#define OV9282_PIXEL_RATE_8BIT               (OV9282_LINK_FREQ * 2 * \
> > > > +                                      OV9282_NUM_DATA_LANES / 8)
> > > >
> > > >  /*
> > > >
> > > >   * OV9282 native and active pixel array size.
> > > >
> > > > @@ -140,6 +150,7 @@ struct ov9282_mode {
> > > >
> > > >   * @again_ctrl: Pointer to analog gain control
> > > >   * @vblank: Vertical blanking in lines
> > > >   * @cur_mode: Pointer to current selected sensor mode
> > > >
> > > > + * @code: Mbus code currently selected
> > > >
> > > >   * @mutex: Mutex for serializing sensor controls
> > > >   * @streaming: Flag indicating streaming state
> > > >   */
> > > >
> > > > @@ -158,9 +169,11 @@ struct ov9282 {
> > > >
> > > >               struct v4l2_ctrl *exp_ctrl;
> > > >               struct v4l2_ctrl *again_ctrl;
> > > >
> > > >       };
> > > >
> > > > +     struct v4l2_ctrl *pixel_rate;
> > > >
> > > >       u32 vblank;
> > > >       bool noncontinuous_clock;
> > > >       const struct ov9282_mode *cur_mode;
> > > >
> > > > +     u32 code;
> > > >
> > > >       struct mutex mutex;
> > > >       bool streaming;
> > > >
> > > >  };
> > > >
> > > > @@ -178,7 +191,6 @@ static const s64 link_freq[] = {
> > > >
> > > >   */
> > > >
> > > >  static const struct ov9282_reg common_regs[] = {
> > > >
> > > >       {0x0302, 0x32},
> > > >
> > > > -     {0x030d, 0x50},
> > > >
> > > >       {0x030e, 0x02},
> > > >       {0x3001, 0x00},
> > > >       {0x3004, 0x00},
> > > >
> > > > @@ -516,19 +528,29 @@ static int ov9282_write_regs(struct ov9282
> > > > *ov9282,
> > > >
> > > >   * ov9282_update_controls() - Update control ranges based on streaming
> > > >   mode
> > > >
> > > > * @ov9282: pointer to ov9282 device
> > > >
> > > >   * @mode: pointer to ov9282_mode sensor mode
> > > >
> > > > + * @fmt: pointer to the requested mode
> > > >
> > > >   *
> > > >   * Return: 0 if successful, error code otherwise.
> > > >   */
> > > >
> > > >  static int ov9282_update_controls(struct ov9282 *ov9282,
> > > >
> > > > -                               const struct ov9282_mode *mode)
> > > > +                               const struct ov9282_mode *mode,
> > > > +                               const struct v4l2_subdev_format
> > >
> > > *fmt)
> > >
> > > >  {
> > > >
> > > >       u32 hblank_min;
> > > >
> > > > +     s64 pixel_rate;
> > > >
> > > >       int ret;
> > > >
> > > >       ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode-
> > > >
> > > >link_freq_idx);
> > > >
> > > >       if (ret)
> > > >
> > > >               return ret;
> > > >
> > > > +     pixel_rate = (fmt->format.code == MEDIA_BUS_FMT_Y10_1X10) ?
> > > > +             OV9282_PIXEL_RATE_10BIT : OV9282_PIXEL_RATE_8BIT;
> > > > +     ret = __v4l2_ctrl_modify_range(ov9282->pixel_rate, pixel_rate,
> > > > +                                    pixel_rate, 1, pixel_rate);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > >
> > > >       hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 :
> > > >       1];
> > > >       ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
> > > >
> > > >                                       OV9282_TIMING_HTS_MAX -
> > >
> > > mode->width, 1,
> > >
> > > > @@ -698,10 +720,16 @@ static int ov9282_enum_mbus_code(struct
> > > > v4l2_subdev
> > > > *sd, struct v4l2_subdev_state *sd_state,
> > > >
> > > >                                struct v4l2_subdev_mbus_code_enum
> > >
> > > *code)
> > >
> > > >  {
> > > >
> > > > -     if (code->index > 0)
> > > > +     switch (code->index) {
> > > > +     case 0:
> > > > +             code->code = MEDIA_BUS_FMT_Y10_1X10;
> > > > +             break;
> > > > +     case 1:
> > > > +             code->code = MEDIA_BUS_FMT_Y8_1X8;
> > > > +             break;
> > > >
> > > > +     default:
> > > >               return -EINVAL;
> > > >
> > > > -
> > > > -     code->code = MEDIA_BUS_FMT_Y10_1X10;
> > > > +     }
> > > >
> > > >       return 0;
> > > >
> > > >  }
> > > >
> > > > @@ -721,7 +749,8 @@ static int ov9282_enum_frame_size(struct v4l2_subdev
> > > > *sd, if (fsize->index >= ARRAY_SIZE(supported_modes))
> > > >
> > > >               return -EINVAL;
> > > >
> > > > -     if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
> > > > +     if (fsize->code != MEDIA_BUS_FMT_Y10_1X10 &&
> > > > +         fsize->code != MEDIA_BUS_FMT_Y8_1X8)
> > > >
> > > >               return -EINVAL;
> > > >
> > > >       fsize->min_width = supported_modes[fsize->index].width;
> > > >
> > > > @@ -737,15 +766,17 @@ static int ov9282_enum_frame_size(struct
> > > > v4l2_subdev
> > > > *sd, *                            from selected sensor mode
> > > >
> > > >   * @ov9282: pointer to ov9282 device
> > > >   * @mode: pointer to ov9282_mode sensor mode
> > > >
> > > > + * @code: mbus code to be stored
> > > >
> > > >   * @fmt: V4L2 sub-device format need to be filled
> > > >   */
> > > >
> > > >  static void ov9282_fill_pad_format(struct ov9282 *ov9282,
> > > >
> > > >                                  const struct ov9282_mode *mode,
> > > >
> > > > +                                u32 code,
> > > >
> > > >                                  struct v4l2_subdev_format *fmt)
> > > >
> > > >  {
> > > >
> > > >       fmt->format.width = mode->width;
> > > >       fmt->format.height = mode->height;
> > > >
> > > > -     fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
> > > > +     fmt->format.code = code;
> > > >
> > > >       fmt->format.field = V4L2_FIELD_NONE;
> > > >       fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> > > >       fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > > >
> > > > @@ -775,7 +806,8 @@ static int ov9282_get_pad_format(struct v4l2_subdev
> > > > *sd, framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> > > >
> > > >               fmt->format = *framefmt;
> > > >
> > > >       } else {
> > > >
> > > > -             ov9282_fill_pad_format(ov9282, ov9282->cur_mode, fmt);
> > > > +             ov9282_fill_pad_format(ov9282, ov9282->cur_mode, ov9282-
> > > >
> > > >code,
> > > >
> > > > +                                    fmt);
> > > >
> > > >       }
> > > >
> > > >       mutex_unlock(&ov9282->mutex);
> > > >
> > > > @@ -797,6 +829,7 @@ static int ov9282_set_pad_format(struct v4l2_subdev
> > > > *sd, {
> > > >
> > > >       struct ov9282 *ov9282 = to_ov9282(sd);
> > > >       const struct ov9282_mode *mode;
> > > >
> > > > +     u32 code;
> > > >
> > > >       int ret = 0;
> > > >
> > > >       mutex_lock(&ov9282->mutex);
> > > >
> > > > @@ -806,7 +839,12 @@ static int ov9282_set_pad_format(struct v4l2_subdev
> > > > *sd, width, height,
> > > >
> > > >                                     fmt->format.width,
> > > >                                     fmt->format.height);
> > > >
> > > > -     ov9282_fill_pad_format(ov9282, mode, fmt);
> > > > +     if (fmt->format.code == MEDIA_BUS_FMT_Y8_1X8)
> > > > +             code = MEDIA_BUS_FMT_Y8_1X8;
> > > > +     else
> > > > +             code = MEDIA_BUS_FMT_Y10_1X10;
> > > > +
> > > > +     ov9282_fill_pad_format(ov9282, mode, code, fmt);
> > > >
> > > >       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > >
> > > >               struct v4l2_mbus_framefmt *framefmt;
> > > >
> > > > @@ -814,9 +852,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev
> > > > *sd, framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> > > >
> > > >               *framefmt = fmt->format;
> > > >
> > > >       } else {
> > > >
> > > > -             ret = ov9282_update_controls(ov9282, mode);
> > > > -             if (!ret)
> > > > +             ret = ov9282_update_controls(ov9282, mode, fmt);
> > > > +             if (!ret) {
> > > >
> > > >                       ov9282->cur_mode = mode;
> > > >
> > > > +                     ov9282->code = code;
> > > > +             }
> > > >
> > > >       }
> > > >
> > > >       mutex_unlock(&ov9282->mutex);
> > > >
> > > > @@ -838,7 +878,8 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev
> > > > *sd,
> > > >
> > > >       struct v4l2_subdev_format fmt = { 0 };
> > >
> > > >       fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY :
> > > V4L2_SUBDEV_FORMAT_ACTIVE;
> > >
> > > > -     ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE],
> > >
> > > &fmt);
> > >
> > > > +     ov9282_fill_pad_format(ov9282, &supported_modes[DEFAULT_MODE],
> > > > +                            ov9282->code, &fmt);
> > > >
> > > >       return ov9282_set_pad_format(sd, sd_state, &fmt);
> > > >
> > > >  }
> > > >
> > > > @@ -903,7 +944,17 @@ static int ov9282_get_selection(struct v4l2_subdev
> > > > *sd, */
> > > >
> > > >  static int ov9282_start_streaming(struct ov9282 *ov9282)
> > > >  {
> > > >
> > > > +     const struct ov9282_reg bitdepth_regs[2][2] = {
> > > > +             {
> > > > +                     {OV9282_REG_PLL_CTRL_0D,
> > >
> > > OV9282_PLL_CTRL_0D_RAW10},
> > >
> > > > +                     {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10},
> > > > +             }, {
> > > > +                     {OV9282_REG_PLL_CTRL_0D,
> > >
> > > OV9282_PLL_CTRL_0D_RAW8},
> > >
> > > > +                     {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW8},
> > > > +             }
> > > > +     };
> > > >
> > > >       const struct ov9282_reg_list *reg_list;
> > > >
> > > > +     int bitdepth_index;
> > > >
> > > >       int ret;
> > > >
> > > >       /* Write common registers */
> > > >
> > > > @@ -914,6 +965,13 @@ static int ov9282_start_streaming(struct ov9282
> > > > *ov9282) return ret;
> > > >
> > > >       }
> > > >
> > > > +     bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1;
> > > > +     ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2);
> > > > +     if (ret) {
> > > > +             dev_err(ov9282->dev, "fail to write bitdepth regs");
> > > > +             return ret;
> > > > +     }
> > > > +
> > > >
> > > >       /* Write sensor mode registers */
> > > >       reg_list = &ov9282->cur_mode->reg_list;
> > > >       ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list-
> > > >
> > > >num_of_regs);
> > > >
> > > > @@ -1235,9 +1293,11 @@ static int ov9282_init_controls(struct ov9282
> > > > *ov9282) 0, 1, 1, 1);
> > > >
> > > >       /* Read only controls */
> > > >
> > > > -     v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> > > > V4L2_CID_PIXEL_RATE,
> > > > -                       OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> > > > -                       OV9282_PIXEL_RATE);
> > > > +     ov9282->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > &ov9282_ctrl_ops,
> > > > +                                            V4L2_CID_PIXEL_RATE,
> > > > +
> > >
> > > OV9282_PIXEL_RATE_10BIT,
> > >
> > > > +
> > >
> > > OV9282_PIXEL_RATE_10BIT, 1,
> > >
> > > > +
> > >
> > > OV9282_PIXEL_RATE_10BIT);
> > >
> > > >       ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > >
> > > &ov9282_ctrl_ops,
> > >
> > > > @@ -1319,6 +1379,7 @@ static int ov9282_probe(struct i2c_client *client)
> > > >
> > > >       /* Set default mode to first mode */
> > > >       ov9282->cur_mode = &supported_modes[DEFAULT_MODE];
> > > >
> > > > +     ov9282->code = MEDIA_BUS_FMT_Y10_1X10;
> > > >
> > > >       ov9282->vblank = ov9282->cur_mode->vblank;
> > > >
> > > >       ret = ov9282_init_controls(ov9282);
> > >
> > > Using this series I was able to do some camera playback on LVDS display on
> > > imx8mm based platform (TQMa8MxML). My command was 'gst-launch-1.0 v4l2src
> > > device=/dev/video0 ! video/x-
> > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 ! videoconvert
> > > ! waylandsink'
> > > But due to SW colorspace conversion this is awfully slow.
> > > Using a testsink I get about 72FPS on 1280x720 for GREY. Is this to be
> > > expected?
> > > I used 'gst-launch-1.0 v4l2src device=/dev/video0 ! video/x-
> > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 !
> > > fpsdisplaysink video-sink="testsink" text-overlay=false silent=false
> > > sync=false -v' for that.
> > AFAIK v4l2src doesn't map from a caps framerate=30/1 to the relevant
> > V4L2_CID_VBLANK and V4L2_CID_HBLANK controls used by raw sensors for
> > frame rate control (see docs at [1]). The sensor will therefore stream
> > at whatever rate the controls get left at.
>
> Yes I noticed the framerate caps has no effect. But I lack some kind of
> reference system to decide what should work and what not.

As per the docs link, raw sensors will be using the HBLANK and VBLANK
controls, not VIDIOC_S_PARM.
I don't know whether the GStreamer folks wish to add support to
v4l2src to set those - libcamerasrc is going to be the more normal
user of these sensors, but that generally means needing an ISP of some
form. With just v4l2src you've got no AE / AGC control loops, so it is
only of use in controlled lighting conditions.

I don't know the full details of the imx8 range, but believe the
libcamera folk were working with one of the imx8 platforms.

> > I'm assuming you're not using Media Controller either, as v4l2src
> > won't set up Media Controller links correctly either.
>
> Well, actually I am using Media Controller. But I need to configure it before
> gstreamer usage. There is no specific reason for gstreamer, but we use this to
> verify features on downstream kernel.
>
> For completeness here is one of my media-ctl setup:
> media-ctl -l "'ov9282 2-0060':0->'csis-32e30000.mipi-csi':0 [1]"
> media-ctl -V "'ov9282 2-0060':0 [fmt:Y8_1X8/1280x720 field:none
> colorspace:raw]"
> media-ctl -V "'csi':0 [fmt:Y8_1X8/1280x720 field:none colorspace:raw]"
> v4l2-ctl -d0 --set-fmt-video
> width=1280,height=720,pixelformat='GREY',field=none
> media-ctl -p
>
> Media controller API version 6.1.0
>
> Media device information
> ------------------------
> driver          imx7-csi
> model           imx-media
> serial
> bus info        platform:32e20000.csi
> hw revision     0x0
> driver version  6.1.0
>
> Device topology
> - entity 1: csi (2 pads, 2 links)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev0
>         pad0: Sink
>                 [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none
> ycbcr:601 quantization:full-range]
>                 <- "csis-32e30000.mipi-csi":1 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none
> ycbcr:601 quantization:full-range]
>                 -> "csi capture":0 [ENABLED,IMMUTABLE]
>
> - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video0
>         pad0: Sink
>                 <- "csi":1 [ENABLED,IMMUTABLE]
>
> - entity 10: csis-32e30000.mipi-csi (2 pads, 2 links)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev1
>         pad0: Sink
>                 [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none]
>                 <- "ov9281 2-0060":0 [ENABLED]
>         pad1: Source
>                 [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none]
>                 -> "csi":0 [ENABLED,IMMUTABLE]
>
> - entity 15: ov9282 2-0060 (1 pad, 1 link)
>              type V4L2 subdev subtype Sensor flags 0
>              device node name /dev/v4l-subdev2
>         pad0: Source
>                 [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none
>                  crop.bounds:(8,8)/1280x800
>                  crop:(8,8)/1280x720]
>                 -> "csis-32e30000.mipi-csi":0 [ENABLED]
>
>
> > Running a Raspberry Pi in the same non-Media Controller mode:
> > v4l2-ctl -v width=1280,height=800,pixelformat=Y10P
> > v4l2-ctl --stream-mmap=3 --stream-count=1000 --stream-to=/dev/null
> > gives me 60.28fps.
> >
> > HBLANK defaults to 176, and VBLANK to 1022:
> > 160MPix/s / ((1280+176) * (800+1022)) = 60.3fps.
> >
> > v4l2-ctl -v width=1280,height=800,pixelformat=GREY
> > v4l2-ctl --stream-mmap=3 --stream-count=1000 --stream-to=/dev/null
> > Gives me 72.33fps as neither HBLANK nor VBLANK have been altered, but
> > V4L2_CID_PIXEL_RATE has been increased.
> >
> > Run the numbers the other way for eg 120fps
> > 200MPix/s / ( 120fps * (width 1280 + HBLANK 176)) - height (800) = VBLANK =
> > 344 v4l2-ctl --set-ctrl=vertical_blanking=344
> > Streaming with that gives me 115.17fps, so you're now making me
> > question the Y8 pixel rate.
> > 192MPix/s appears to be the right value to make the numbers work.
>
> Mh, using v4l2-ctl --set-ctrl=vertical_blanking=344 -d /dev/v4l-subdev2 I get
> 109.61fps for 1280x800.

The sensor has an external clock signal (XVCLK) which can be between 6
and 27MHz. The driver only supports 24MHz. Is your module using a
24MHz clock?

> > I don't recall where I'd got the 200MPix/s value from - it's not
> > documented in the datasheet, but presumably from 160 * 10 / 8
> > (switching from 10 to 8 bits at the same output rate). You're the
> > first to notice the rates are off, although at least it's less than
> > the factor of two that this driver used to be out by.
>
> I admit I'm not fully sure which results are correct and what they are
> expected to be. But here are some results using the v4l-ctrl approach:
>      | 1280x800 | 1280x720 | 640x400 |
> -----+----------+----------+---------+
> GREY |  68.84   |   72.0   |  73.50  |
> Y10  |  57.37   |   60.0   |  73.50  |
>
> All using their default vertical and horizontal blanking. Especially switching
> to 640x400 and then back to 1280x720 leaves the horizontal_blanking to the old
> (640) value, resulting in lower frame rates.

IMHO This isn't clear in the docs.
My understanding is that controls shouldn't change value when changing
modes unless the new mode requires updating the range for the control
such that the current value is invalid. This does mean that the
framerate will change if you change modes without reprogramming, but
what heuristics should be used if you did update it? Options:
1) retain the current frame rate by recomputing VBLANK, but there will
be conditions where you can't achieve the same frame rate in all
modes.
2) reset to a default frame rate, but how do you define that? Do you
have to detect change of mode vs just calling S_FMT with the same
mode?
3) adjust the limits but otherwise leave the control alone.
4) as 3, but update the default value to reflect some standard
framerate (but how do you define that standard?)

Different sensors are currently doing different things, so the only
approach you can really take is for userspace to set the controls
explicitly after setting a mode.
Sakari will normally point to the CCS driver as a model for raw
sensors, and that appears to adopt option 3. There was a thread with
Jacopo recently over this same subject, but no resolution. I think it
was on the ar0521 patchset.

Clean boot and testing in this order:
- 1280x720 Y10P 63.05fps
- 1280x800 Y10P 60.28fps
- 640x400 Y10P 77.22fps
Reboot so that the HBLANK change is reset
- 1280x720 GREY 75.65fps
- 1280x800 GREY 72.33fps
- 640x400 GREY 92.67fps.

I don't believe your GREY 640x400 number as it's the same as your
640x400 Y10 value, but all your other values except 1280x800 Y10
differ from mine by a factor of 1.0507. (1280x800 Y10 is x1.099).
I'd suggest measuring your XVCLK clock signal with an oscilloscope or
frequency counter as I suspect it isn't 24MHz. 22.8MHz would give
these results, but is a slightly strange frequency if from a dedicated
oscillator rather than a PLL.
Adding support for additional XVCLK frequencies isn't a huge task, but
involves computing the internal PLL settings. My datasheet only gives
settings for 24MHz, so it'd be back to the basic principles of PLL
config to do it.

  Dave

> > Sakari: Do you want a new version of the patchset, or just a fixup on top?
> >
> > [1]
> > https://www.kernel.org/doc/html/latest/driver-api/media/camera-sensor.html#
> > raw-camera-sensors
> > > Apparently gstreamer does not support Y10. Do you have a different way
> > > to actually use Y10?
> >
> > We're using libcamera on Raspberry Pi. The Pi ISP will happily consume
> > raw 8, 10, 12, 14, or 16, and spit out YUV or RGB.
> > The alternative is v4l2-ctl as above.
>
> There is no ISP on imx8mm, so I'm stuck to CPU conversion for now, as OpenGL
> based conversion in gstreamer does not work currently, but that's a different
> matter (it does work downstream on imx8mp though).
>
> > Depending on what unpacking your platform is capable of, you may be
> > able to request V4L2_PIX_FMT_Y10 (10bit sample in a 16bit word) and
> > then pass it through GStreamer as either GRAY16_LE or GRAY16_BE.
> > V4L2_PIX_FMT_Y10P is a bit of a pain to handle (4 pixels in 5 bytes as
> > described in [2]), but is more efficient on memory usage.
> >
> > Do note that this is still a raw image sensor and therefore the images
> > will generally have a non-zero black level, and quite probably lens
> > shading artifacts. They should not be considered as a standard luma
> > signal.
>
> Thanks for the hint about using GRAY16_LE and the explanations: I'm aware that
> there is more to configure, but right now I'm focusing on correct
> configuration and data transfer for the sensor data itself.
>
> Best regards
> Alexander
>
>
>

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Kieran Bingham]

Hi Alex,

Quoting Dave Stevenson (2022-11-01 18:20:47)
> Hi Alexander
> 
> On Tue, 1 Nov 2022 at 15:04, Alexander Stein
> <alexander.stein@ew.tq-group.com> wrote:
> >
> > Hi Dave,
> >
> > thanks for the fast reply.
> >
> > Am Dienstag, 1. November 2022, 14:47:16 CET schrieb Dave Stevenson:
> > > Hi Alexander
> > >
> > > On Tue, 1 Nov 2022 at 11:59, Alexander Stein
> > >

<snip>

> > > >
> > > > Using this series I was able to do some camera playback on LVDS display on
> > > > imx8mm based platform (TQMa8MxML). My command was 'gst-launch-1.0 v4l2src
> > > > device=/dev/video0 ! video/x-
> > > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 ! videoconvert
> > > > ! waylandsink'
> > > > But due to SW colorspace conversion this is awfully slow.
> > > > Using a testsink I get about 72FPS on 1280x720 for GREY. Is this to be
> > > > expected?
> > > > I used 'gst-launch-1.0 v4l2src device=/dev/video0 ! video/x-
> > > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 !
> > > > fpsdisplaysink video-sink="testsink" text-overlay=false silent=false
> > > > sync=false -v' for that.
> > > AFAIK v4l2src doesn't map from a caps framerate=30/1 to the relevant
> > > V4L2_CID_VBLANK and V4L2_CID_HBLANK controls used by raw sensors for
> > > frame rate control (see docs at [1]). The sensor will therefore stream
> > > at whatever rate the controls get left at.
> >
> > Yes I noticed the framerate caps has no effect. But I lack some kind of
> > reference system to decide what should work and what not.
> 
> As per the docs link, raw sensors will be using the HBLANK and VBLANK
> controls, not VIDIOC_S_PARM.
> I don't know whether the GStreamer folks wish to add support to
> v4l2src to set those - libcamerasrc is going to be the more normal
> user of these sensors, but that generally means needing an ISP of some
> form. With just v4l2src you've got no AE / AGC control loops, so it is
> only of use in controlled lighting conditions.
> 
> I don't know the full details of the imx8 range, but believe the
> libcamera folk were working with one of the imx8 platforms.

We have the i.MX8MP working with the ISP available on that variant. I
think we can also anticipate some support for other i.MX8 ranges with a
GPU based 'ISP' in the (nearish) future, but I don't know what the
timescales will be yet.

--
Kieran

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Alexander Stein]

Hi Kieran,

Am Dienstag, 1. November 2022, 21:37:09 CET schrieb Kieran Bingham:
> Hi Alex,
> 
> Quoting Dave Stevenson (2022-11-01 18:20:47)
> 
> > Hi Alexander
> > 
> > On Tue, 1 Nov 2022 at 15:04, Alexander Stein
> > 
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Hi Dave,
> > > 
> > > thanks for the fast reply.
> > > 
> > > Am Dienstag, 1. November 2022, 14:47:16 CET schrieb Dave Stevenson:
> > > > Hi Alexander
> > > > 
> > > > On Tue, 1 Nov 2022 at 11:59, Alexander Stein
> 
> <snip>
> 
> > > > > Using this series I was able to do some camera playback on LVDS
> > > > > display on
> > > > > imx8mm based platform (TQMa8MxML). My command was 'gst-launch-1.0
> > > > > v4l2src
> > > > > device=/dev/video0 ! video/x-
> > > > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 !
> > > > > videoconvert
> > > > > ! waylandsink'
> > > > > But due to SW colorspace conversion this is awfully slow.
> > > > > Using a testsink I get about 72FPS on 1280x720 for GREY. Is this to
> > > > > be
> > > > > expected?
> > > > > I used 'gst-launch-1.0 v4l2src device=/dev/video0 ! video/x-
> > > > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 !
> > > > > fpsdisplaysink video-sink="testsink" text-overlay=false silent=false
> > > > > sync=false -v' for that.
> > > > 
> > > > AFAIK v4l2src doesn't map from a caps framerate=30/1 to the relevant
> > > > V4L2_CID_VBLANK and V4L2_CID_HBLANK controls used by raw sensors for
> > > > frame rate control (see docs at [1]). The sensor will therefore stream
> > > > at whatever rate the controls get left at.
> > > 
> > > Yes I noticed the framerate caps has no effect. But I lack some kind of
> > > reference system to decide what should work and what not.
> > 
> > As per the docs link, raw sensors will be using the HBLANK and VBLANK
> > controls, not VIDIOC_S_PARM.
> > I don't know whether the GStreamer folks wish to add support to
> > v4l2src to set those - libcamerasrc is going to be the more normal
> > user of these sensors, but that generally means needing an ISP of some
> > form. With just v4l2src you've got no AE / AGC control loops, so it is
> > only of use in controlled lighting conditions.
> > 
> > I don't know the full details of the imx8 range, but believe the
> > libcamera folk were working with one of the imx8 platforms.
> 
> We have the i.MX8MP working with the ISP available on that variant. I
> think we can also anticipate some support for other i.MX8 ranges with a
> GPU based 'ISP' in the (nearish) future, but I don't know what the
> timescales will be yet.

You are referring to (mainly) Paul Elder's patches to rkisp1, right? I noticed 
them, but didn't get a chance for testing.

I noticed that using 'glupload ! glcolorconvert ! glcolorscale ! 
glcolorconvert ! gldownload' in a gstreamer Pipeline for converting Y8/GREY to 
RGBA doesn't work, because mesa rejects creating appropriate EGL buffers due 
to lack of some hardware features.

Best regards,
Alexander

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Kieran Bingham]

Quoting Alexander Stein (2022-11-03 08:49:48)
> Hi Kieran,
> 
> Am Dienstag, 1. November 2022, 21:37:09 CET schrieb Kieran Bingham:
> > Hi Alex,
> > 
> > Quoting Dave Stevenson (2022-11-01 18:20:47)
> > 
> > > Hi Alexander
> > > 
> > > On Tue, 1 Nov 2022 at 15:04, Alexander Stein
> > > 
> > > <alexander.stein@ew.tq-group.com> wrote:
> > > > Hi Dave,
> > > > 
> > > > thanks for the fast reply.
> > > > 
> > > > Am Dienstag, 1. November 2022, 14:47:16 CET schrieb Dave Stevenson:
> > > > > Hi Alexander
> > > > > 
> > > > > On Tue, 1 Nov 2022 at 11:59, Alexander Stein
> > 
> > <snip>
> > 
> > > > > > Using this series I was able to do some camera playback on LVDS
> > > > > > display on
> > > > > > imx8mm based platform (TQMa8MxML). My command was 'gst-launch-1.0
> > > > > > v4l2src
> > > > > > device=/dev/video0 ! video/x-
> > > > > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 !
> > > > > > videoconvert
> > > > > > ! waylandsink'
> > > > > > But due to SW colorspace conversion this is awfully slow.
> > > > > > Using a testsink I get about 72FPS on 1280x720 for GREY. Is this to
> > > > > > be
> > > > > > expected?
> > > > > > I used 'gst-launch-1.0 v4l2src device=/dev/video0 ! video/x-
> > > > > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 !
> > > > > > fpsdisplaysink video-sink="testsink" text-overlay=false silent=false
> > > > > > sync=false -v' for that.
> > > > > 
> > > > > AFAIK v4l2src doesn't map from a caps framerate=30/1 to the relevant
> > > > > V4L2_CID_VBLANK and V4L2_CID_HBLANK controls used by raw sensors for
> > > > > frame rate control (see docs at [1]). The sensor will therefore stream
> > > > > at whatever rate the controls get left at.
> > > > 
> > > > Yes I noticed the framerate caps has no effect. But I lack some kind of
> > > > reference system to decide what should work and what not.
> > > 
> > > As per the docs link, raw sensors will be using the HBLANK and VBLANK
> > > controls, not VIDIOC_S_PARM.
> > > I don't know whether the GStreamer folks wish to add support to
> > > v4l2src to set those - libcamerasrc is going to be the more normal
> > > user of these sensors, but that generally means needing an ISP of some
> > > form. With just v4l2src you've got no AE / AGC control loops, so it is
> > > only of use in controlled lighting conditions.
> > > 
> > > I don't know the full details of the imx8 range, but believe the
> > > libcamera folk were working with one of the imx8 platforms.
> > 
> > We have the i.MX8MP working with the ISP available on that variant. I
> > think we can also anticipate some support for other i.MX8 ranges with a
> > GPU based 'ISP' in the (nearish) future, but I don't know what the
> > timescales will be yet.
> 
> You are referring to (mainly) Paul Elder's patches to rkisp1, right? I noticed 
> them, but didn't get a chance for testing.

Yes, that's right - but you can only test those on an i.MX8MP not an
i.MX8MM as I understand it.

> I noticed that using 'glupload ! glcolorconvert ! glcolorscale ! 
> glcolorconvert ! gldownload' in a gstreamer Pipeline for converting Y8/GREY to 
> RGBA doesn't work, because mesa rejects creating appropriate EGL buffers due 
> to lack of some hardware features.

I haven't looked into the gstreamer EGL side of things I'm afraid.

--
Kieran


> 
> Best regards,
> Alexander
> 
> 
>

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Alexander Stein]

Am Donnerstag, 3. November 2022, 10:53:49 CET schrieb Kieran Bingham:
> Quoting Alexander Stein (2022-11-03 08:49:48)
> 
> > Hi Kieran,
> > 
> > Am Dienstag, 1. November 2022, 21:37:09 CET schrieb Kieran Bingham:
> > > Hi Alex,
> > > 
> > > Quoting Dave Stevenson (2022-11-01 18:20:47)
> > > 
> > > > Hi Alexander
> > > > 
> > > > On Tue, 1 Nov 2022 at 15:04, Alexander Stein
> > > > 
> > > > <alexander.stein@ew.tq-group.com> wrote:
> > > > > Hi Dave,
> > > > > 
> > > > > thanks for the fast reply.
> > > > > 
> > > > > Am Dienstag, 1. November 2022, 14:47:16 CET schrieb Dave Stevenson:
> > > > > > Hi Alexander
> > > > > > 
> > > > > > On Tue, 1 Nov 2022 at 11:59, Alexander Stein
> > > 
> > > <snip>
> > > 
> > > > > > > Using this series I was able to do some camera playback on LVDS
> > > > > > > display on
> > > > > > > imx8mm based platform (TQMa8MxML). My command was
> > > > > > > 'gst-launch-1.0
> > > > > > > v4l2src
> > > > > > > device=/dev/video0 ! video/x-
> > > > > > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 !
> > > > > > > videoconvert
> > > > > > > ! waylandsink'
> > > > > > > But due to SW colorspace conversion this is awfully slow.
> > > > > > > Using a testsink I get about 72FPS on 1280x720 for GREY. Is this
> > > > > > > to
> > > > > > > be
> > > > > > > expected?
> > > > > > > I used 'gst-launch-1.0 v4l2src device=/dev/video0 ! video/x-
> > > > > > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 !
> > > > > > > fpsdisplaysink video-sink="testsink" text-overlay=false
> > > > > > > silent=false
> > > > > > > sync=false -v' for that.
> > > > > > 
> > > > > > AFAIK v4l2src doesn't map from a caps framerate=30/1 to the
> > > > > > relevant
> > > > > > V4L2_CID_VBLANK and V4L2_CID_HBLANK controls used by raw sensors
> > > > > > for
> > > > > > frame rate control (see docs at [1]). The sensor will therefore
> > > > > > stream
> > > > > > at whatever rate the controls get left at.
> > > > > 
> > > > > Yes I noticed the framerate caps has no effect. But I lack some kind
> > > > > of
> > > > > reference system to decide what should work and what not.
> > > > 
> > > > As per the docs link, raw sensors will be using the HBLANK and VBLANK
> > > > controls, not VIDIOC_S_PARM.
> > > > I don't know whether the GStreamer folks wish to add support to
> > > > v4l2src to set those - libcamerasrc is going to be the more normal
> > > > user of these sensors, but that generally means needing an ISP of some
> > > > form. With just v4l2src you've got no AE / AGC control loops, so it is
> > > > only of use in controlled lighting conditions.
> > > > 
> > > > I don't know the full details of the imx8 range, but believe the
> > > > libcamera folk were working with one of the imx8 platforms.
> > > 
> > > We have the i.MX8MP working with the ISP available on that variant. I
> > > think we can also anticipate some support for other i.MX8 ranges with a
> > > GPU based 'ISP' in the (nearish) future, but I don't know what the
> > > timescales will be yet.
> > 
> > You are referring to (mainly) Paul Elder's patches to rkisp1, right? I
> > noticed them, but didn't get a chance for testing.
> 
> Yes, that's right - but you can only test those on an i.MX8MP not an
> i.MX8MM as I understand it.

Yes, i.MX8MM does not have an ISP. Let's see if I get the chance to try on 
i.MX8MP.

> > I noticed that using 'glupload ! glcolorconvert ! glcolorscale !
> > glcolorconvert ! gldownload' in a gstreamer Pipeline for converting
> > Y8/GREY to RGBA doesn't work, because mesa rejects creating appropriate
> > EGL buffers due to lack of some hardware features.
> 
> I haven't looked into the gstreamer EGL side of things I'm afraid.

No worries, I noticed that GPU on i.MX8MM, BTW the only one in i.MX8M series, 
does not support OpenGLES3 which might be the cause I'm stuck here. But I do 
not want to dig in that rabbit hole even further :)

Best regards
Alexander

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Alexander Stein]

Hi Dave,

Am Dienstag, 1. November 2022, 19:20:47 CET schrieb Dave Stevenson:
> Hi Alexander
> 
> On Tue, 1 Nov 2022 at 15:04, Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hi Dave,
> > 
> > thanks for the fast reply.
> > 
> > Am Dienstag, 1. November 2022, 14:47:16 CET schrieb Dave Stevenson:
> > > Hi Alexander
> > > 
> > > On Tue, 1 Nov 2022 at 11:59, Alexander Stein
> > > 
> > > <alexander.stein@ew.tq-group.com> wrote:
[snip]
> > > > Using this series I was able to do some camera playback on LVDS
> > > > display on
> > > > imx8mm based platform (TQMa8MxML). My command was 'gst-launch-1.0
> > > > v4l2src
> > > > device=/dev/video0 ! video/x-
> > > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 !
> > > > videoconvert
> > > > ! waylandsink'
> > > > But due to SW colorspace conversion this is awfully slow.
> > > > Using a testsink I get about 72FPS on 1280x720 for GREY. Is this to be
> > > > expected?
> > > > I used 'gst-launch-1.0 v4l2src device=/dev/video0 ! video/x-
> > > > raw,format=GRAY8,bpp=8,width=1280,height=720,framerate=30/1 !
> > > > fpsdisplaysink video-sink="testsink" text-overlay=false silent=false
> > > > sync=false -v' for that.
> > > 
> > > AFAIK v4l2src doesn't map from a caps framerate=30/1 to the relevant
> > > V4L2_CID_VBLANK and V4L2_CID_HBLANK controls used by raw sensors for
> > > frame rate control (see docs at [1]). The sensor will therefore stream
> > > at whatever rate the controls get left at.
> > 
> > Yes I noticed the framerate caps has no effect. But I lack some kind of
> > reference system to decide what should work and what not.
> 
> As per the docs link, raw sensors will be using the HBLANK and VBLANK
> controls, not VIDIOC_S_PARM.
> I don't know whether the GStreamer folks wish to add support to
> v4l2src to set those - libcamerasrc is going to be the more normal
> user of these sensors, but that generally means needing an ISP of some
> form. With just v4l2src you've got no AE / AGC control loops, so it is
> only of use in controlled lighting conditions.

I am aware that v4l2src is rather some raw accessor to cameras. What video 
format is libcamerasrc supposed to provide? Raw formats (Y8, bayer patterns) 
or already converted to RBGA etc.?

> I don't know the full details of the imx8 range, but believe the
> libcamera folk were working with one of the imx8 platforms.

If an ISP is required then it will probably be imx8mp.

> > > I'm assuming you're not using Media Controller either, as v4l2src
> > > won't set up Media Controller links correctly either.
> > 
> > Well, actually I am using Media Controller. But I need to configure it
> > before gstreamer usage. There is no specific reason for gstreamer, but we
> > use this to verify features on downstream kernel.
> > 
> > For completeness here is one of my media-ctl setup:
> > media-ctl -l "'ov9282 2-0060':0->'csis-32e30000.mipi-csi':0 [1]"
> > media-ctl -V "'ov9282 2-0060':0 [fmt:Y8_1X8/1280x720 field:none
> > colorspace:raw]"
> > media-ctl -V "'csi':0 [fmt:Y8_1X8/1280x720 field:none colorspace:raw]"
> > v4l2-ctl -d0 --set-fmt-video
> > width=1280,height=720,pixelformat='GREY',field=none
> > media-ctl -p
> > 
> > Media controller API version 6.1.0
> > 
> > Media device information
> > ------------------------
> > driver          imx7-csi
> > model           imx-media
> > serial
> > bus info        platform:32e20000.csi
> > hw revision     0x0
> > driver version  6.1.0
> > 
> > Device topology
> > - entity 1: csi (2 pads, 2 links)
> > 
> >             type V4L2 subdev subtype Unknown flags 0
> >             device node name /dev/v4l-subdev0
> >         
> >         pad0: Sink
> >         
> >                 [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none
> > 
> > ycbcr:601 quantization:full-range]
> > 
> >                 <- "csis-32e30000.mipi-csi":1 [ENABLED,IMMUTABLE]
> >         
> >         pad1: Source
> >         
> >                 [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none
> > 
> > ycbcr:601 quantization:full-range]
> > 
> >                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 4: csi capture (1 pad, 1 link)
> > 
> >             type Node subtype V4L flags 0
> >             device node name /dev/video0
> >         
> >         pad0: Sink
> >         
> >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > 
> > - entity 10: csis-32e30000.mipi-csi (2 pads, 2 links)
> > 
> >              type V4L2 subdev subtype Unknown flags 0
> >              device node name /dev/v4l-subdev1
> >         
> >         pad0: Sink
> >         
> >                 [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none]
> >                 <- "ov9281 2-0060":0 [ENABLED]
> >         
> >         pad1: Source
> >         
> >                 [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none]
> >                 -> "csi":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 15: ov9282 2-0060 (1 pad, 1 link)
> > 
> >              type V4L2 subdev subtype Sensor flags 0
> >              device node name /dev/v4l-subdev2
> >         
> >         pad0: Source
> >         
> >                 [fmt:Y8_1X8/1280x720 field:none colorspace:raw xfer:none
> >                 
> >                  crop.bounds:(8,8)/1280x800
> >                  crop:(8,8)/1280x720]
> >                 
> >                 -> "csis-32e30000.mipi-csi":0 [ENABLED]
> > > 
> > > Running a Raspberry Pi in the same non-Media Controller mode:
> > > v4l2-ctl -v width=1280,height=800,pixelformat=Y10P
> > > v4l2-ctl --stream-mmap=3 --stream-count=1000 --stream-to=/dev/null
> > > gives me 60.28fps.
> > > 
> > > HBLANK defaults to 176, and VBLANK to 1022:
> > > 160MPix/s / ((1280+176) * (800+1022)) = 60.3fps.
> > > 
> > > v4l2-ctl -v width=1280,height=800,pixelformat=GREY
> > > v4l2-ctl --stream-mmap=3 --stream-count=1000 --stream-to=/dev/null
> > > Gives me 72.33fps as neither HBLANK nor VBLANK have been altered, but
> > > V4L2_CID_PIXEL_RATE has been increased.
> > > 
> > > Run the numbers the other way for eg 120fps
> > > 200MPix/s / ( 120fps * (width 1280 + HBLANK 176)) - height (800) =
> > > VBLANK =
> > > 344 v4l2-ctl --set-ctrl=vertical_blanking=344
> > > Streaming with that gives me 115.17fps, so you're now making me
> > > question the Y8 pixel rate.
> > > 192MPix/s appears to be the right value to make the numbers work.
> > 
> > Mh, using v4l2-ctl --set-ctrl=vertical_blanking=344 -d /dev/v4l-subdev2 I
> > get 109.61fps for 1280x800.
> 
> The sensor has an external clock signal (XVCLK) which can be between 6
> and 27MHz. The driver only supports 24MHz. Is your module using a
> 24MHz clock?

Well, I assume it is. We are using an OV9281 camera from vision components.

> > > I don't recall where I'd got the 200MPix/s value from - it's not
> > > documented in the datasheet, but presumably from 160 * 10 / 8
> > > (switching from 10 to 8 bits at the same output rate). You're the
> > > first to notice the rates are off, although at least it's less than
> > > the factor of two that this driver used to be out by.
> > 
> > I admit I'm not fully sure which results are correct and what they are
> > 
> > expected to be. But here are some results using the v4l-ctrl approach:
> >      | 1280x800 | 1280x720 | 640x400 |
> > 
> > -----+----------+----------+---------+
> > GREY |  68.84   |   72.0   |  73.50  |
> > Y10  |  57.37   |   60.0   |  73.50  |
> > 
> > All using their default vertical and horizontal blanking. Especially
> > switching to 640x400 and then back to 1280x720 leaves the
> > horizontal_blanking to the old (640) value, resulting in lower frame
> > rates.
> 
> IMHO This isn't clear in the docs.
> My understanding is that controls shouldn't change value when changing
> modes unless the new mode requires updating the range for the control
> such that the current value is invalid. This does mean that the
> framerate will change if you change modes without reprogramming, but
> what heuristics should be used if you did update it?

For 1280 the default horizontal_blanking is 250, but when changing to 640 the 
minimum also increased to 890. When switching back horizontal_blanking stays 
at 890, as it is still a valid value.

> Options:
> 1) retain the current frame rate by recomputing VBLANK, but there will
> be conditions where you can't achieve the same frame rate in all
> modes.
> 2) reset to a default frame rate, but how do you define that? Do you
> have to detect change of mode vs just calling S_FMT with the same
> mode?
> 3) adjust the limits but otherwise leave the control alone.
> 4) as 3, but update the default value to reflect some standard
> framerate (but how do you define that standard?)
> 
> Different sensors are currently doing different things, so the only
> approach you can really take is for userspace to set the controls
> explicitly after setting a mode.

I'm not sure what is the best way to go, all options have different use cases 
in mind. At least one should be aware that some controls might change when 
switching modes.

> Sakari will normally point to the CCS driver as a model for raw
> sensors, and that appears to adopt option 3. There was a thread with
> Jacopo recently over this same subject, but no resolution. I think it
> was on the ar0521 patchset.
> 
> Clean boot and testing in this order:
> - 1280x720 Y10P 63.05fps
> - 1280x800 Y10P 60.28fps
> - 640x400 Y10P 77.22fps
> Reboot so that the HBLANK change is reset
> - 1280x720 GREY 75.65fps
> - 1280x800 GREY 72.33fps
> - 640x400 GREY 92.67fps.
> 
> I don't believe your GREY 640x400 number as it's the same as your
> 640x400 Y10 value, but all your other values except 1280x800 Y10
> differ from mine by a factor of 1.0507. (1280x800 Y10 is x1.099).
> I'd suggest measuring your XVCLK clock signal with an oscilloscope or
> frequency counter as I suspect it isn't 24MHz. 22.8MHz would give
> these results, but is a slightly strange frequency if from a dedicated
> oscillator rather than a PLL.

I lack technical documentation for the camera hardware module, so I do not see 
a way to actually measuring XVCLK. AFAIK there is also an FPGA mounted which 
might affect the clock frequency as well.

> Adding support for additional XVCLK frequencies isn't a huge task, but
> involves computing the internal PLL settings. My datasheet only gives
> settings for 24MHz, so it'd be back to the basic principles of PLL
> config to do it.

Until things are more clear I would skip that for now as this module should be 
running on a 24 MHz, I assume.

Best regards,
Alexander

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Dave Stevenson]

Hi Alex

On Thu, 3 Nov 2022 at 09:09, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi Dave,
>
> Am Dienstag, 1. November 2022, 19:20:47 CET schrieb Dave Stevenson:
> > Hi Alexander
> >
> > On Tue, 1 Nov 2022 at 15:04, Alexander Stein
> >
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Hi Dave,
> > >
> > > thanks for the fast reply.
> > >
> > > Am Dienstag, 1. November 2022, 14:47:16 CET schrieb Dave Stevenson:
> > > > Hi Alexander
> > > >
> > > > On Tue, 1 Nov 2022 at 11:59, Alexander Stein
> > > >
> > > > <alexander.stein@ew.tq-group.com> wrote:
> > As per the docs link, raw sensors will be using the HBLANK and VBLANK
> > controls, not VIDIOC_S_PARM.
> > I don't know whether the GStreamer folks wish to add support to
> > v4l2src to set those - libcamerasrc is going to be the more normal
> > user of these sensors, but that generally means needing an ISP of some
> > form. With just v4l2src you've got no AE / AGC control loops, so it is
> > only of use in controlled lighting conditions.
>
> I am aware that v4l2src is rather some raw accessor to cameras. What video
> format is libcamerasrc supposed to provide? Raw formats (Y8, bayer patterns)
> or already converted to RBGA etc.?

It depends on the pipeline handler.
Raw streams will be the unprocessed images from the sensor, so
typically Bayer or mono.
Processed streams depend on the ISP capabilities, but generally YUV or RGB/RGBx.

> > I don't know the full details of the imx8 range, but believe the
> > libcamera folk were working with one of the imx8 platforms.
>
> If an ISP is required then it will probably be imx8mp.
>
> > > > I'm assuming you're not using Media Controller either, as v4l2src
> > > > won't set up Media Controller links correctly either.
> > >
> > > Well, actually I am using Media Controller. But I need to configure it
> > > before gstreamer usage. There is no specific reason for gstreamer, but we
> > > use this to verify features on downstream kernel.
> > >
<snip>
> > > Mh, using v4l2-ctl --set-ctrl=vertical_blanking=344 -d /dev/v4l-subdev2 I
> > > get 109.61fps for 1280x800.
> >
> > The sensor has an external clock signal (XVCLK) which can be between 6
> > and 27MHz. The driver only supports 24MHz. Is your module using a
> > 24MHz clock?
>
> Well, I assume it is. We are using an OV9281 camera from vision components.

Thank you - I happen to have one of those, so I can test. (I also have
OV9281 modules from InnoVision, Arducam, and possibly others).
Vision Components modules do have the joys of not being able to do a 16bit read.

> > > > I don't recall where I'd got the 200MPix/s value from - it's not
> > > > documented in the datasheet, but presumably from 160 * 10 / 8
> > > > (switching from 10 to 8 bits at the same output rate). You're the
> > > > first to notice the rates are off, although at least it's less than
> > > > the factor of two that this driver used to be out by.
> > >
> > > I admit I'm not fully sure which results are correct and what they are
> > >
> > > expected to be. But here are some results using the v4l-ctrl approach:
> > >      | 1280x800 | 1280x720 | 640x400 |
> > >
> > > -----+----------+----------+---------+
> > > GREY |  68.84   |   72.0   |  73.50  |
> > > Y10  |  57.37   |   60.0   |  73.50  |
> > >
> > > All using their default vertical and horizontal blanking. Especially
> > > switching to 640x400 and then back to 1280x720 leaves the
> > > horizontal_blanking to the old (640) value, resulting in lower frame
> > > rates.
> >
> > IMHO This isn't clear in the docs.
> > My understanding is that controls shouldn't change value when changing
> > modes unless the new mode requires updating the range for the control
> > such that the current value is invalid. This does mean that the
> > framerate will change if you change modes without reprogramming, but
> > what heuristics should be used if you did update it?
>
> For 1280 the default horizontal_blanking is 250, but when changing to 640 the
> minimum also increased to 890. When switching back horizontal_blanking stays
> at 890, as it is still a valid value.
>
> > Options:
> > 1) retain the current frame rate by recomputing VBLANK, but there will
> > be conditions where you can't achieve the same frame rate in all
> > modes.
> > 2) reset to a default frame rate, but how do you define that? Do you
> > have to detect change of mode vs just calling S_FMT with the same
> > mode?
> > 3) adjust the limits but otherwise leave the control alone.
> > 4) as 3, but update the default value to reflect some standard
> > framerate (but how do you define that standard?)
> >
> > Different sensors are currently doing different things, so the only
> > approach you can really take is for userspace to set the controls
> > explicitly after setting a mode.
>
> I'm not sure what is the best way to go, all options have different use cases
> in mind. At least one should be aware that some controls might change when
> switching modes.

Switching is the interesting part.
I had an annoyance trying to test variable HBLANK support on imx290
before libcamera supported it. libcamera (at least on the Pi) always
explicitly sets the mode when run, therefore the HBLANK I was setting
was always being reset due to s_fmt being called on the driver, hence
my comment above about detecting a change of mode or not.

Without any definition of the correct behaviour you find different
drivers do different things, and userspace has to handle all
parameters or suffer unexpected results :-(

> > Sakari will normally point to the CCS driver as a model for raw
> > sensors, and that appears to adopt option 3. There was a thread with
> > Jacopo recently over this same subject, but no resolution. I think it
> > was on the ar0521 patchset.
> >
> > Clean boot and testing in this order:
> > - 1280x720 Y10P 63.05fps
> > - 1280x800 Y10P 60.28fps
> > - 640x400 Y10P 77.22fps
> > Reboot so that the HBLANK change is reset
> > - 1280x720 GREY 75.65fps
> > - 1280x800 GREY 72.33fps
> > - 640x400 GREY 92.67fps.

Tested with my Vision Components OV9281, and I get identical numbers
to those above.

> > I don't believe your GREY 640x400 number as it's the same as your
> > 640x400 Y10 value, but all your other values except 1280x800 Y10
> > differ from mine by a factor of 1.0507. (1280x800 Y10 is x1.099).
> > I'd suggest measuring your XVCLK clock signal with an oscilloscope or
> > frequency counter as I suspect it isn't 24MHz. 22.8MHz would give
> > these results, but is a slightly strange frequency if from a dedicated
> > oscillator rather than a PLL.
>
> I lack technical documentation for the camera hardware module, so I do not see
> a way to actually measuring XVCLK. AFAIK there is also an FPGA mounted which
> might affect the clock frequency as well.
>
> > Adding support for additional XVCLK frequencies isn't a huge task, but
> > involves computing the internal PLL settings. My datasheet only gives
> > settings for 24MHz, so it'd be back to the basic principles of PLL
> > config to do it.
>
> Until things are more clear I would skip that for now as this module should be
> running on a 24 MHz, I assume.

I've measured the output of the oscillator on my Vision Components
OV9281, and it is 24MHz.
The oscillator is the square silver package on the back of my module
by one of the screw holes. It is stamped U24 which would also indicate
24MHz. Holding the sensor with that silver component at the bottom
edge, the clock output is the top right pin of the package.

Unless yours is different for some reason, I'm out of ideas why you're
seeing different frame rates.

  Dave

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Alexander Stein]

Hi Dave,

Am Donnerstag, 3. November 2022, 14:05:47 CET schrieb Dave Stevenson:
> Hi Alex
> 
> On Thu, 3 Nov 2022 at 09:09, Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hi Dave,
> > 
> > Am Dienstag, 1. November 2022, 19:20:47 CET schrieb Dave Stevenson:
> > > Hi Alexander
> > > 
> > > On Tue, 1 Nov 2022 at 15:04, Alexander Stein
> > > 
> > > <alexander.stein@ew.tq-group.com> wrote:
> > > > Hi Dave,
> > > > 
> > > > thanks for the fast reply.
> > > > 
> > > > Am Dienstag, 1. November 2022, 14:47:16 CET schrieb Dave Stevenson:
> > > > > Hi Alexander
> > > > > 
> > > > > On Tue, 1 Nov 2022 at 11:59, Alexander Stein
> > > 
> > > > > <alexander.stein@ew.tq-group.com> wrote:
> > > As per the docs link, raw sensors will be using the HBLANK and VBLANK
> > > controls, not VIDIOC_S_PARM.
> > > I don't know whether the GStreamer folks wish to add support to
> > > v4l2src to set those - libcamerasrc is going to be the more normal
> > > user of these sensors, but that generally means needing an ISP of some
> > > form. With just v4l2src you've got no AE / AGC control loops, so it is
> > > only of use in controlled lighting conditions.
> > 
> > I am aware that v4l2src is rather some raw accessor to cameras. What video
> > format is libcamerasrc supposed to provide? Raw formats (Y8, bayer
> > patterns) or already converted to RBGA etc.?
> 
> It depends on the pipeline handler.
> Raw streams will be the unprocessed images from the sensor, so
> typically Bayer or mono.
> Processed streams depend on the ISP capabilities, but generally YUV or
> RGB/RGBx.

Okay, thanks for the information. I'll try at some time, but this sounds very 
promising.

> > > I don't know the full details of the imx8 range, but believe the
> > > libcamera folk were working with one of the imx8 platforms.
> > 
> > If an ISP is required then it will probably be imx8mp.
> > 
> > > > > I'm assuming you're not using Media Controller either, as v4l2src
> > > > > won't set up Media Controller links correctly either.
> > > > 
> > > > Well, actually I am using Media Controller. But I need to configure it
> > > > before gstreamer usage. There is no specific reason for gstreamer, but
> > > > we
> > > > use this to verify features on downstream kernel.
> 
> <snip>
> 
> > > > Mh, using v4l2-ctl --set-ctrl=vertical_blanking=344 -d
> > > > /dev/v4l-subdev2 I
> > > > get 109.61fps for 1280x800.
> > > 
> > > The sensor has an external clock signal (XVCLK) which can be between 6
> > > and 27MHz. The driver only supports 24MHz. Is your module using a
> > > 24MHz clock?
> > 
> > Well, I assume it is. We are using an OV9281 camera from vision
> > components.
> 
> Thank you - I happen to have one of those, so I can test. (I also have
> OV9281 modules from InnoVision, Arducam, and possibly others).
> Vision Components modules do have the joys of not being able to do a 16bit
> read.

Dealing with broken auto-increment read can be handled by this change. There 
was also some discussion with Laurent about a generic i2c DT property for 
adding this flag.
--8<--
diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c689a4e97fcd..1be32ad1e285 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -194,6 +194,7 @@ struct ov9282 {
 static const struct regmap_config ov9282_regmap_config = {
        .reg_bits = 16,
        .val_bits = 8,
+       .use_single_read = true,
 };
 
 static const s64 link_freq[] = {
--8<--

> > > > > I don't recall where I'd got the 200MPix/s value from - it's not
> > > > > documented in the datasheet, but presumably from 160 * 10 / 8
> > > > > (switching from 10 to 8 bits at the same output rate). You're the
> > > > > first to notice the rates are off, although at least it's less than
> > > > > the factor of two that this driver used to be out by.
> > > > 
> > > > I admit I'm not fully sure which results are correct and what they are
> > > > 
> > > > expected to be. But here are some results using the v4l-ctrl approach:
> > > >      | 1280x800 | 1280x720 | 640x400 |
> > > > 
> > > > -----+----------+----------+---------+
> > > > GREY |  68.84   |   72.0   |  73.50  |
> > > > Y10  |  57.37   |   60.0   |  73.50  |
> > > > 
> > > > All using their default vertical and horizontal blanking. Especially
> > > > switching to 640x400 and then back to 1280x720 leaves the
> > > > horizontal_blanking to the old (640) value, resulting in lower frame
> > > > rates.
> > > 
> > > IMHO This isn't clear in the docs.
> > > My understanding is that controls shouldn't change value when changing
> > > modes unless the new mode requires updating the range for the control
> > > such that the current value is invalid. This does mean that the
> > > framerate will change if you change modes without reprogramming, but
> > > what heuristics should be used if you did update it?
> > 
> > For 1280 the default horizontal_blanking is 250, but when changing to 640
> > the minimum also increased to 890. When switching back
> > horizontal_blanking stays at 890, as it is still a valid value.
> > 
> > > Options:
> > > 1) retain the current frame rate by recomputing VBLANK, but there will
> > > be conditions where you can't achieve the same frame rate in all
> > > modes.
> > > 2) reset to a default frame rate, but how do you define that? Do you
> > > have to detect change of mode vs just calling S_FMT with the same
> > > mode?
> > > 3) adjust the limits but otherwise leave the control alone.
> > > 4) as 3, but update the default value to reflect some standard
> > > framerate (but how do you define that standard?)
> > > 
> > > Different sensors are currently doing different things, so the only
> > > approach you can really take is for userspace to set the controls
> > > explicitly after setting a mode.
> > 
> > I'm not sure what is the best way to go, all options have different use
> > cases in mind. At least one should be aware that some controls might
> > change when switching modes.
> 
> Switching is the interesting part.
> I had an annoyance trying to test variable HBLANK support on imx290
> before libcamera supported it. libcamera (at least on the Pi) always
> explicitly sets the mode when run, therefore the HBLANK I was setting
> was always being reset due to s_fmt being called on the driver, hence
> my comment above about detecting a change of mode or not.
> 
> Without any definition of the correct behaviour you find different
> drivers do different things, and userspace has to handle all
> parameters or suffer unexpected results :-(

That's unfortunate, but if you know about this you can deal with it somehow.
While fiddling with the controls on this driver I noticed too there is no 
definite answer how you should set the blanks when switching modes, each 
approach has their pros and cons.
I used raw configuration until now, being unaware of libcamera doing all the 
necessary things.

> > > Sakari will normally point to the CCS driver as a model for raw
> > > sensors, and that appears to adopt option 3. There was a thread with
> > > Jacopo recently over this same subject, but no resolution. I think it
> > > was on the ar0521 patchset.
> > > 
> > > Clean boot and testing in this order:
> > > - 1280x720 Y10P 63.05fps
> > > - 1280x800 Y10P 60.28fps
> > > - 640x400 Y10P 77.22fps
> > > Reboot so that the HBLANK change is reset
> > > - 1280x720 GREY 75.65fps
> > > - 1280x800 GREY 72.33fps
> > > - 640x400 GREY 92.67fps.
> 
> Tested with my Vision Components OV9281, and I get identical numbers
> to those above.

Okay, that's nice. At least we know the sensor driver is doing things right.

> > > I don't believe your GREY 640x400 number as it's the same as your
> > > 640x400 Y10 value, but all your other values except 1280x800 Y10
> > > differ from mine by a factor of 1.0507. (1280x800 Y10 is x1.099).
> > > I'd suggest measuring your XVCLK clock signal with an oscilloscope or
> > > frequency counter as I suspect it isn't 24MHz. 22.8MHz would give
> > > these results, but is a slightly strange frequency if from a dedicated
> > > oscillator rather than a PLL.
> > 
> > I lack technical documentation for the camera hardware module, so I do not
> > see a way to actually measuring XVCLK. AFAIK there is also an FPGA
> > mounted which might affect the clock frequency as well.
> > 
> > > Adding support for additional XVCLK frequencies isn't a huge task, but
> > > involves computing the internal PLL settings. My datasheet only gives
> > > settings for 24MHz, so it'd be back to the basic principles of PLL
> > > config to do it.
> > 
> > Until things are more clear I would skip that for now as this module
> > should be running on a 24 MHz, I assume.
> 
> I've measured the output of the oscillator on my Vision Components
> OV9281, and it is 24MHz.
> The oscillator is the square silver package on the back of my module
> by one of the screw holes. It is stamped U24 which would also indicate
> 24MHz. Holding the sensor with that silver component at the bottom
> edge, the clock output is the top right pin of the package.

Thanks, using this description I was able to verify mine is 24MHz (23.9-24.1) 
as well.

> Unless yours is different for some reason, I'm out of ideas why you're
> seeing different frame rates.

Can this be caused by something withing my platform (imx8mm) CSI-2 path which 
is different to yours?

Best regards,
Alexander

Re: [PATCH v2 15/16] media: i2c: ov9282: Add support for 8bit readout

[Dave Stevenson]

Hi Alexander

On Fri, 4 Nov 2022 at 07:55, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi Dave,
>
> Am Donnerstag, 3. November 2022, 14:05:47 CET schrieb Dave Stevenson:
> > Hi Alex
> >
> > On Thu, 3 Nov 2022 at 09:09, Alexander Stein
> >
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Hi Dave,
> > >
> > > Am Dienstag, 1. November 2022, 19:20:47 CET schrieb Dave Stevenson:
> > > > Hi Alexander
> > > >
> > > > On Tue, 1 Nov 2022 at 15:04, Alexander Stein
> > > >
> > > > <alexander.stein@ew.tq-group.com> wrote:
> > > > > Hi Dave,
> > > > >
> > > > > thanks for the fast reply.
> > > > >
> > > > > Am Dienstag, 1. November 2022, 14:47:16 CET schrieb Dave Stevenson:
> > > > > > Hi Alexander
> > > > > >
> > > > > > On Tue, 1 Nov 2022 at 11:59, Alexander Stein
> > > >
> > > > > > <alexander.stein@ew.tq-group.com> wrote:
> > > > As per the docs link, raw sensors will be using the HBLANK and VBLANK
> > > > controls, not VIDIOC_S_PARM.
> > > > I don't know whether the GStreamer folks wish to add support to
> > > > v4l2src to set those - libcamerasrc is going to be the more normal
> > > > user of these sensors, but that generally means needing an ISP of some
> > > > form. With just v4l2src you've got no AE / AGC control loops, so it is
> > > > only of use in controlled lighting conditions.
> > >
> > > I am aware that v4l2src is rather some raw accessor to cameras. What video
> > > format is libcamerasrc supposed to provide? Raw formats (Y8, bayer
> > > patterns) or already converted to RBGA etc.?
> >
> > It depends on the pipeline handler.
> > Raw streams will be the unprocessed images from the sensor, so
> > typically Bayer or mono.
> > Processed streams depend on the ISP capabilities, but generally YUV or
> > RGB/RGBx.
>
> Okay, thanks for the information. I'll try at some time, but this sounds very
> promising.
>
> > > > I don't know the full details of the imx8 range, but believe the
> > > > libcamera folk were working with one of the imx8 platforms.
> > >
> > > If an ISP is required then it will probably be imx8mp.
> > >
> > > > > > I'm assuming you're not using Media Controller either, as v4l2src
> > > > > > won't set up Media Controller links correctly either.
> > > > >
> > > > > Well, actually I am using Media Controller. But I need to configure it
> > > > > before gstreamer usage. There is no specific reason for gstreamer, but
> > > > > we
> > > > > use this to verify features on downstream kernel.
> >
> > <snip>
> >
> > > > > Mh, using v4l2-ctl --set-ctrl=vertical_blanking=344 -d
> > > > > /dev/v4l-subdev2 I
> > > > > get 109.61fps for 1280x800.
> > > >
> > > > The sensor has an external clock signal (XVCLK) which can be between 6
> > > > and 27MHz. The driver only supports 24MHz. Is your module using a
> > > > 24MHz clock?
> > >
> > > Well, I assume it is. We are using an OV9281 camera from vision
> > > components.
> >
> > Thank you - I happen to have one of those, so I can test. (I also have
> > OV9281 modules from InnoVision, Arducam, and possibly others).
> > Vision Components modules do have the joys of not being able to do a 16bit
> > read.
>
> Dealing with broken auto-increment read can be handled by this change. There
> was also some discussion with Laurent about a generic i2c DT property for
> adding this flag.
> --8<--
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index c689a4e97fcd..1be32ad1e285 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -194,6 +194,7 @@ struct ov9282 {
>  static const struct regmap_config ov9282_regmap_config = {
>         .reg_bits = 16,
>         .val_bits = 8,
> +       .use_single_read = true,
>  };
>
>  static const s64 link_freq[] = {
> --8<--

I forgot that one from your patchset. I just removed the one read from
the driver :-)

> > > > > > I don't recall where I'd got the 200MPix/s value from - it's not
> > > > > > documented in the datasheet, but presumably from 160 * 10 / 8
> > > > > > (switching from 10 to 8 bits at the same output rate). You're the
> > > > > > first to notice the rates are off, although at least it's less than
> > > > > > the factor of two that this driver used to be out by.
> > > > >
> > > > > I admit I'm not fully sure which results are correct and what they are
> > > > >
> > > > > expected to be. But here are some results using the v4l-ctrl approach:
> > > > >      | 1280x800 | 1280x720 | 640x400 |
> > > > >
> > > > > -----+----------+----------+---------+
> > > > > GREY |  68.84   |   72.0   |  73.50  |
> > > > > Y10  |  57.37   |   60.0   |  73.50  |
> > > > >
> > > > > All using their default vertical and horizontal blanking. Especially
> > > > > switching to 640x400 and then back to 1280x720 leaves the
> > > > > horizontal_blanking to the old (640) value, resulting in lower frame
> > > > > rates.
> > > >
> > > > IMHO This isn't clear in the docs.
> > > > My understanding is that controls shouldn't change value when changing
> > > > modes unless the new mode requires updating the range for the control
> > > > such that the current value is invalid. This does mean that the
> > > > framerate will change if you change modes without reprogramming, but
> > > > what heuristics should be used if you did update it?
> > >
> > > For 1280 the default horizontal_blanking is 250, but when changing to 640
> > > the minimum also increased to 890. When switching back
> > > horizontal_blanking stays at 890, as it is still a valid value.
> > >
> > > > Options:
> > > > 1) retain the current frame rate by recomputing VBLANK, but there will
> > > > be conditions where you can't achieve the same frame rate in all
> > > > modes.
> > > > 2) reset to a default frame rate, but how do you define that? Do you
> > > > have to detect change of mode vs just calling S_FMT with the same
> > > > mode?
> > > > 3) adjust the limits but otherwise leave the control alone.
> > > > 4) as 3, but update the default value to reflect some standard
> > > > framerate (but how do you define that standard?)
> > > >
> > > > Different sensors are currently doing different things, so the only
> > > > approach you can really take is for userspace to set the controls
> > > > explicitly after setting a mode.
> > >
> > > I'm not sure what is the best way to go, all options have different use
> > > cases in mind. At least one should be aware that some controls might
> > > change when switching modes.
> >
> > Switching is the interesting part.
> > I had an annoyance trying to test variable HBLANK support on imx290
> > before libcamera supported it. libcamera (at least on the Pi) always
> > explicitly sets the mode when run, therefore the HBLANK I was setting
> > was always being reset due to s_fmt being called on the driver, hence
> > my comment above about detecting a change of mode or not.
> >
> > Without any definition of the correct behaviour you find different
> > drivers do different things, and userspace has to handle all
> > parameters or suffer unexpected results :-(
>
> That's unfortunate, but if you know about this you can deal with it somehow.
> While fiddling with the controls on this driver I noticed too there is no
> definite answer how you should set the blanks when switching modes, each
> approach has their pros and cons.
> I used raw configuration until now, being unaware of libcamera doing all the
> necessary things.

I fully acknowledge that writing documentation isn't the fun part of
development, and writing good documentation is hard.
However we do have a need. I'm trying to sort a basic checklist for
sensor drivers, but I have other priorities at present.

> > > > Sakari will normally point to the CCS driver as a model for raw
> > > > sensors, and that appears to adopt option 3. There was a thread with
> > > > Jacopo recently over this same subject, but no resolution. I think it
> > > > was on the ar0521 patchset.
> > > >
> > > > Clean boot and testing in this order:
> > > > - 1280x720 Y10P 63.05fps
> > > > - 1280x800 Y10P 60.28fps
> > > > - 640x400 Y10P 77.22fps
> > > > Reboot so that the HBLANK change is reset
> > > > - 1280x720 GREY 75.65fps
> > > > - 1280x800 GREY 72.33fps
> > > > - 640x400 GREY 92.67fps.
> >
> > Tested with my Vision Components OV9281, and I get identical numbers
> > to those above.
>
> Okay, that's nice. At least we know the sensor driver is doing things right.
>
> > > > I don't believe your GREY 640x400 number as it's the same as your
> > > > 640x400 Y10 value, but all your other values except 1280x800 Y10
> > > > differ from mine by a factor of 1.0507. (1280x800 Y10 is x1.099).
> > > > I'd suggest measuring your XVCLK clock signal with an oscilloscope or
> > > > frequency counter as I suspect it isn't 24MHz. 22.8MHz would give
> > > > these results, but is a slightly strange frequency if from a dedicated
> > > > oscillator rather than a PLL.
> > >
> > > I lack technical documentation for the camera hardware module, so I do not
> > > see a way to actually measuring XVCLK. AFAIK there is also an FPGA
> > > mounted which might affect the clock frequency as well.
> > >
> > > > Adding support for additional XVCLK frequencies isn't a huge task, but
> > > > involves computing the internal PLL settings. My datasheet only gives
> > > > settings for 24MHz, so it'd be back to the basic principles of PLL
> > > > config to do it.
> > >
> > > Until things are more clear I would skip that for now as this module
> > > should be running on a 24 MHz, I assume.
> >
> > I've measured the output of the oscillator on my Vision Components
> > OV9281, and it is 24MHz.
> > The oscillator is the square silver package on the back of my module
> > by one of the screw holes. It is stamped U24 which would also indicate
> > 24MHz. Holding the sensor with that silver component at the bottom
> > edge, the clock output is the top right pin of the package.
>
> Thanks, using this description I was able to verify mine is 24MHz (23.9-24.1)
> as well.
>
> > Unless yours is different for some reason, I'm out of ideas why you're
> > seeing different frame rates.
>
> Can this be caused by something withing my platform (imx8mm) CSI-2 path which
> is different to yours?

The only way I can think of for getting lower framerates out than
expected is dropping frames, so I'd suggest looking at timestamps.
Adding --verbose to the v4l2-ctl --stream-mmap.... line will print the
timestamp for each buffers, as well as the fps. Are the timestamp
deltas as expected, and are there any that are doubled due to a
dropped frame. If the timestamps are even at some other value then I'm
totally stumped!

Is your receiver correctly configured for whether it is expecting
continuous or non-continuous clock? That can sometimes cause the
receiver to get confused and corrupt data - I seem to recall having
issues with that on ov5647.

  Dave

> Best regards,
> Alexander
>
>
>

[PATCH v2 16/16] media: i2c: ov9282: Support event handlers

[Dave Stevenson]

As noted in the headers for V4L2_SUBDEV_FL_HAS_EVENTS,
"controls can send events, thus drivers exposing controls
should set this flag".

This driver exposes controls, but didn't reflect that it
could generate events. Correct this, and add the default
event handler functions.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index e2a98f480b58..f2ec92deb5ec 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -13,6 +13,7 @@
 #include <linux/pm_runtime.h>
 
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
@@ -1161,6 +1162,11 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
 }
 
 /* V4l2 subdevice ops */
+static const struct v4l2_subdev_core_ops ov9282_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
 static const struct v4l2_subdev_video_ops ov9282_video_ops = {
 	.s_stream = ov9282_set_stream,
 };
@@ -1175,6 +1181,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
 };
 
 static const struct v4l2_subdev_ops ov9282_subdev_ops = {
+	.core = &ov9282_core_ops,
 	.video = &ov9282_video_ops,
 	.pad = &ov9282_pad_ops,
 };
@@ -1389,7 +1396,8 @@ static int ov9282_probe(struct i2c_client *client)
 	}
 
 	/* Initialize subdev */
-	ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			    V4L2_SUBDEV_FL_HAS_EVENTS;
 	ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 
 	/* Initialize source pad */
-- 
2.34.1

Re: [PATCH v2 16/16] media: i2c: ov9282: Support event handlers

[Jacopo Mondi]

Hi Dave,

On Fri, Oct 28, 2022 at 05:09:02PM +0100, Dave Stevenson wrote:
> As noted in the headers for V4L2_SUBDEV_FL_HAS_EVENTS,
> "controls can send events, thus drivers exposing controls
> should set this flag".
>
> This driver exposes controls, but didn't reflect that it
> could generate events. Correct this, and add the default
> event handler functions.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Sending a tag as I don't see anything worrying, but I'm just not sure
how this mechanism is intended to be used, so please wait for others
to comment!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> ---
>  drivers/media/i2c/ov9282.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index e2a98f480b58..f2ec92deb5ec 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -13,6 +13,7 @@
>  #include <linux/pm_runtime.h>
>
>  #include <media/v4l2-ctrls.h>
> +#include <media/v4l2-event.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>
> @@ -1161,6 +1162,11 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
>  }
>
>  /* V4l2 subdevice ops */
> +static const struct v4l2_subdev_core_ops ov9282_core_ops = {
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
>  static const struct v4l2_subdev_video_ops ov9282_video_ops = {
>  	.s_stream = ov9282_set_stream,
>  };
> @@ -1175,6 +1181,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
>  };
>
>  static const struct v4l2_subdev_ops ov9282_subdev_ops = {
> +	.core = &ov9282_core_ops,
>  	.video = &ov9282_video_ops,
>  	.pad = &ov9282_pad_ops,
>  };
> @@ -1389,7 +1396,8 @@ static int ov9282_probe(struct i2c_client *client)
>  	}
>
>  	/* Initialize subdev */
> -	ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +			    V4L2_SUBDEV_FL_HAS_EVENTS;
>  	ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>
>  	/* Initialize source pad */
> --
> 2.34.1
>