| Message ID | 20230220124041.1010289-1-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: ov5647: Add test pattern support | expand |
Hi Jacopo, On Mon, Feb 20, 2023 at 01:40:41PM +0100, Jacopo Mondi wrote: > From: Valentine Barshak <valentine.barshak@cogentembedded.com> > > This adds V4L2_CID_TEST_PATTERN control support. > > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > drivers/media/i2c/ov5647.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c > index 847a7bbb69c5..0b88ac6dee41 100644 > --- a/drivers/media/i2c/ov5647.c > +++ b/drivers/media/i2c/ov5647.c > @@ -58,6 +58,7 @@ > #define OV5647_REG_MIPI_CTRL00 0x4800 > #define OV5647_REG_MIPI_CTRL14 0x4814 > #define OV5647_REG_AWB 0x5001 > +#define OV5647_REG_ISPCTRL3D 0x503d > > #define REG_TERM 0xfffe > #define VAL_TERM 0xfe > @@ -116,6 +117,22 @@ static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd) > return container_of(sd, struct ov5647, sd); > } > > +static const char * const ov5647_test_pattern_menu[] = { > + "Disabled", > + "Color Bars", > + "Color Squares", > + "Random Data", > + "Input Data" > +}; > + > +static u8 ov5647_test_pattern_val[] = { > + 0x00, /* Disabled */ > + 0x80, /* Color Bars */ > + 0x82, /* Color Squares */ > + 0x81, /* Random Data */ > + 0x83, /* Input Data */ > +}; > + > static const struct regval_list sensor_oe_disable_regs[] = { > {0x3000, 0x00}, > {0x3001, 0x00}, > @@ -1242,6 +1259,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl) > ret = ov5647_write16(sd, OV5647_REG_VTS_HI, > sensor->mode->format.height + ctrl->val); > break; > + case V4L2_CID_TEST_PATTERN: > + ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D, > + ov5647_test_pattern_val[ctrl->val]); > + break; > > /* Read-only, but we adjust it based on mode. */ > case V4L2_CID_PIXEL_RATE: > @@ -1270,7 +1291,7 @@ static int ov5647_init_controls(struct ov5647 *sensor) > struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd); > int hblank, exposure_max, exposure_def; > > - v4l2_ctrl_handler_init(&sensor->ctrls, 8); > + v4l2_ctrl_handler_init(&sensor->ctrls, 9); > > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops, > V4L2_CID_AUTOGAIN, 0, 1, 1, 0); > @@ -1314,6 +1335,11 @@ static int ov5647_init_controls(struct ov5647 *sensor) > sensor->mode->vts - > sensor->mode->format.height); > > + v4l2_ctrl_new_std_menu_items(&sensor->ctrls, &ov5647_ctrl_ops, > + V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(ov5647_test_pattern_menu) - 1, > + 0, 0, ov5647_test_pattern_menu); > + > if (sensor->ctrls.error) > goto handler_free; > > -- > 2.39.0 > Looks good to me. Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com> Thanks, Tommaso Tommaso
Hi Jacopo On Mon, 20 Feb 2023 at 12:40, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > From: Valentine Barshak <valentine.barshak@cogentembedded.com> > > This adds V4L2_CID_TEST_PATTERN control support. > > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > drivers/media/i2c/ov5647.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c > index 847a7bbb69c5..0b88ac6dee41 100644 > --- a/drivers/media/i2c/ov5647.c > +++ b/drivers/media/i2c/ov5647.c > @@ -58,6 +58,7 @@ > #define OV5647_REG_MIPI_CTRL00 0x4800 > #define OV5647_REG_MIPI_CTRL14 0x4814 > #define OV5647_REG_AWB 0x5001 > +#define OV5647_REG_ISPCTRL3D 0x503d > > #define REG_TERM 0xfffe > #define VAL_TERM 0xfe > @@ -116,6 +117,22 @@ static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd) > return container_of(sd, struct ov5647, sd); > } > > +static const char * const ov5647_test_pattern_menu[] = { > + "Disabled", > + "Color Bars", > + "Color Squares", > + "Random Data", > + "Input Data" "Input Data" appears to give me just a black image. Have I missed something? What's it meant to be the input to? Is it worth adding 0x92 for a black and white checkboard as well? Whichever way: Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Just as a note, the test patterns appear to be valid only if 0x3820 bit 1 = 0 and 0x3821 bit 1 = 1 (V & H flips respectively). The sensor appears to be assuming one particular colour pattern (BGGR) when producing a test pattern, so flips altering the format give some very weird effects. I do have patches that add the V4L2 flip controls, so those expose some interesting effects :-/ Dave > +}; > + > +static u8 ov5647_test_pattern_val[] = { > + 0x00, /* Disabled */ > + 0x80, /* Color Bars */ > + 0x82, /* Color Squares */ > + 0x81, /* Random Data */ > + 0x83, /* Input Data */ > +}; > + > static const struct regval_list sensor_oe_disable_regs[] = { > {0x3000, 0x00}, > {0x3001, 0x00}, > @@ -1242,6 +1259,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl) > ret = ov5647_write16(sd, OV5647_REG_VTS_HI, > sensor->mode->format.height + ctrl->val); > break; > + case V4L2_CID_TEST_PATTERN: > + ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D, > + ov5647_test_pattern_val[ctrl->val]); > + break; > > /* Read-only, but we adjust it based on mode. */ > case V4L2_CID_PIXEL_RATE: > @@ -1270,7 +1291,7 @@ static int ov5647_init_controls(struct ov5647 *sensor) > struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd); > int hblank, exposure_max, exposure_def; > > - v4l2_ctrl_handler_init(&sensor->ctrls, 8); > + v4l2_ctrl_handler_init(&sensor->ctrls, 9); > > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops, > V4L2_CID_AUTOGAIN, 0, 1, 1, 0); > @@ -1314,6 +1335,11 @@ static int ov5647_init_controls(struct ov5647 *sensor) > sensor->mode->vts - > sensor->mode->format.height); > > + v4l2_ctrl_new_std_menu_items(&sensor->ctrls, &ov5647_ctrl_ops, > + V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(ov5647_test_pattern_menu) - 1, > + 0, 0, ov5647_test_pattern_menu); > + > if (sensor->ctrls.error) > goto handler_free; > > -- > 2.39.0 >
Hi Dave On Mon, Feb 20, 2023 at 06:18:14PM +0000, Dave Stevenson wrote: > Hi Jacopo > > On Mon, 20 Feb 2023 at 12:40, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > From: Valentine Barshak <valentine.barshak@cogentembedded.com> > > > > This adds V4L2_CID_TEST_PATTERN control support. > > > > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > drivers/media/i2c/ov5647.c | 28 +++++++++++++++++++++++++++- > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c > > index 847a7bbb69c5..0b88ac6dee41 100644 > > --- a/drivers/media/i2c/ov5647.c > > +++ b/drivers/media/i2c/ov5647.c > > @@ -58,6 +58,7 @@ > > #define OV5647_REG_MIPI_CTRL00 0x4800 > > #define OV5647_REG_MIPI_CTRL14 0x4814 > > #define OV5647_REG_AWB 0x5001 > > +#define OV5647_REG_ISPCTRL3D 0x503d > > > > #define REG_TERM 0xfffe > > #define VAL_TERM 0xfe > > @@ -116,6 +117,22 @@ static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd) > > return container_of(sd, struct ov5647, sd); > > } > > > > +static const char * const ov5647_test_pattern_menu[] = { > > + "Disabled", > > + "Color Bars", > > + "Color Squares", > > + "Random Data", > > + "Input Data" > > "Input Data" appears to give me just a black image. Have I missed > something? What's it meant to be the input to? I noticed as well, but not knowing what "input data" meant I just "meh" and moved on Should it be dropped in your opinion ? > Is it worth adding 0x92 for a black and white checkboard as well? I could > > Whichever way: > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Just as a note, the test patterns appear to be valid only if 0x3820 > bit 1 = 0 and 0x3821 bit 1 = 1 (V & H flips respectively). Unrelated: why do I see the 2592x1944 mode with {0x3821, 0x06} ? I only tested whatever qcam gave me, I should better re-check > The sensor appears to be assuming one particular colour pattern (BGGR) > when producing a test pattern, so flips altering the format give some > very weird effects. I do have patches that add the V4L2 flip controls, Ah, that surprises me, I recall we discussed in the past the fact that flip shouldn't alter test patterns... > so those expose some interesting effects :-/ > > Dave > > > +}; > > + > > +static u8 ov5647_test_pattern_val[] = { > > + 0x00, /* Disabled */ > > + 0x80, /* Color Bars */ > > + 0x82, /* Color Squares */ > > + 0x81, /* Random Data */ > > + 0x83, /* Input Data */ > > +}; > > + > > static const struct regval_list sensor_oe_disable_regs[] = { > > {0x3000, 0x00}, > > {0x3001, 0x00}, > > @@ -1242,6 +1259,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl) > > ret = ov5647_write16(sd, OV5647_REG_VTS_HI, > > sensor->mode->format.height + ctrl->val); > > break; > > + case V4L2_CID_TEST_PATTERN: > > + ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D, > > + ov5647_test_pattern_val[ctrl->val]); > > + break; > > > > /* Read-only, but we adjust it based on mode. */ > > case V4L2_CID_PIXEL_RATE: > > @@ -1270,7 +1291,7 @@ static int ov5647_init_controls(struct ov5647 *sensor) > > struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd); > > int hblank, exposure_max, exposure_def; > > > > - v4l2_ctrl_handler_init(&sensor->ctrls, 8); > > + v4l2_ctrl_handler_init(&sensor->ctrls, 9); > > > > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops, > > V4L2_CID_AUTOGAIN, 0, 1, 1, 0); > > @@ -1314,6 +1335,11 @@ static int ov5647_init_controls(struct ov5647 *sensor) > > sensor->mode->vts - > > sensor->mode->format.height); > > > > + v4l2_ctrl_new_std_menu_items(&sensor->ctrls, &ov5647_ctrl_ops, > > + V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(ov5647_test_pattern_menu) - 1, > > + 0, 0, ov5647_test_pattern_menu); > > + > > if (sensor->ctrls.error) > > goto handler_free; > > > > -- > > 2.39.0 > >
Hi Jacopo On Tue, 21 Feb 2023 at 08:25, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Dave > > On Mon, Feb 20, 2023 at 06:18:14PM +0000, Dave Stevenson wrote: > > Hi Jacopo > > > > On Mon, 20 Feb 2023 at 12:40, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > From: Valentine Barshak <valentine.barshak@cogentembedded.com> > > > > > > This adds V4L2_CID_TEST_PATTERN control support. > > > > > > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > drivers/media/i2c/ov5647.c | 28 +++++++++++++++++++++++++++- > > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c > > > index 847a7bbb69c5..0b88ac6dee41 100644 > > > --- a/drivers/media/i2c/ov5647.c > > > +++ b/drivers/media/i2c/ov5647.c > > > @@ -58,6 +58,7 @@ > > > #define OV5647_REG_MIPI_CTRL00 0x4800 > > > #define OV5647_REG_MIPI_CTRL14 0x4814 > > > #define OV5647_REG_AWB 0x5001 > > > +#define OV5647_REG_ISPCTRL3D 0x503d > > > > > > #define REG_TERM 0xfffe > > > #define VAL_TERM 0xfe > > > @@ -116,6 +117,22 @@ static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd) > > > return container_of(sd, struct ov5647, sd); > > > } > > > > > > +static const char * const ov5647_test_pattern_menu[] = { > > > + "Disabled", > > > + "Color Bars", > > > + "Color Squares", > > > + "Random Data", > > > + "Input Data" > > > > "Input Data" appears to give me just a black image. Have I missed > > something? What's it meant to be the input to? > > I noticed as well, but not knowing what "input data" meant I just > "meh" and moved on > > Should it be dropped in your opinion ? Personally I would drop it as it is undefined what that really means. > > Is it worth adding 0x92 for a black and white checkboard as well? > > I could I'll leave that one up to you. I have no major interest in test patterns. > > > > Whichever way: > > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > Just as a note, the test patterns appear to be valid only if 0x3820 > > bit 1 = 0 and 0x3821 bit 1 = 1 (V & H flips respectively). > > Unrelated: why do I see the 2592x1944 mode with {0x3821, 0x06} ? > I only tested whatever qcam gave me, I should better re-check The sensor appears to have an inherent H-flip, so for "normal" readout you need to set 0x3821 bit 1 as you see in the driver. I don't remember if I've ever really experimented with the difference between r_mirror_isp (bit 2) and r_mirror_snr (bit 1) > > The sensor appears to be assuming one particular colour pattern (BGGR) > > when producing a test pattern, so flips altering the format give some > > very weird effects. I do have patches that add the V4L2 flip controls, > > Ah, that surprises me, I recall we discussed in the past the fact that > flip shouldn't alter test patterns... I don't recall the exact discussion. Was it around a sensor which deliberately changed the flip register as it enabled the pattern? In my experience it's not uncommon that they are affected by flips. Flips are implemented by changing the readout order on the sensor array. The sensor itself doesn't care that doing so changes the Bayer order as it knows nothing about the colour filter or even if it is present. Test patterns are generally implemented further down the pipeline by forcing the pixel state of each pixel in turn, ignoring flips. If the receiver pipeline is working on the basis that it knows the flips and how that would normally affect the Bayer order, then you get wrong colours. V4L2_CID_TEST_PATTERN_[RED|GREENR|GREENB|BLUE] exist, and I see they have been implemented for CCS and imx219 (IIRC done by others rather than myself, but in the original upstreamed version of the driver). Testing with imx219 confirms that it also has the same issue - colours are only correct if hflip = 0 and vflip = 0. V4L2_CID_TEST_PATTERN ought to set MODIFY_LAYOUT, override the Bayer order, and be grabbed at start/stop streaming. Of course this gets even more confusing when device tree can advertise a mounting rotation though V4L2_CID_CAMERA_SENSOR_ROTATION, so libcamera may enable flips automatically when told to stream in the native orientation, and then the test patterns are unexpectedly wrong as the user didn't ask for flips. It's one of the reasons I generally don't care about test patterns - they're generally of little use other than to prove the sensor streams, so how much effort is it worth expending in making them work perfectly under all conditions? Dave > > so those expose some interesting effects :-/ > > > > Dave > > > > > +}; > > > + > > > +static u8 ov5647_test_pattern_val[] = { > > > + 0x00, /* Disabled */ > > > + 0x80, /* Color Bars */ > > > + 0x82, /* Color Squares */ > > > + 0x81, /* Random Data */ > > > + 0x83, /* Input Data */ > > > +}; > > > + > > > static const struct regval_list sensor_oe_disable_regs[] = { > > > {0x3000, 0x00}, > > > {0x3001, 0x00}, > > > @@ -1242,6 +1259,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl) > > > ret = ov5647_write16(sd, OV5647_REG_VTS_HI, > > > sensor->mode->format.height + ctrl->val); > > > break; > > > + case V4L2_CID_TEST_PATTERN: > > > + ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D, > > > + ov5647_test_pattern_val[ctrl->val]); > > > + break; > > > > > > /* Read-only, but we adjust it based on mode. */ > > > case V4L2_CID_PIXEL_RATE: > > > @@ -1270,7 +1291,7 @@ static int ov5647_init_controls(struct ov5647 *sensor) > > > struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd); > > > int hblank, exposure_max, exposure_def; > > > > > > - v4l2_ctrl_handler_init(&sensor->ctrls, 8); > > > + v4l2_ctrl_handler_init(&sensor->ctrls, 9); > > > > > > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops, > > > V4L2_CID_AUTOGAIN, 0, 1, 1, 0); > > > @@ -1314,6 +1335,11 @@ static int ov5647_init_controls(struct ov5647 *sensor) > > > sensor->mode->vts - > > > sensor->mode->format.height); > > > > > > + v4l2_ctrl_new_std_menu_items(&sensor->ctrls, &ov5647_ctrl_ops, > > > + V4L2_CID_TEST_PATTERN, > > > + ARRAY_SIZE(ov5647_test_pattern_menu) - 1, > > > + 0, 0, ov5647_test_pattern_menu); > > > + > > > if (sensor->ctrls.error) > > > goto handler_free; > > > > > > -- > > > 2.39.0 > > >
diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c index 847a7bbb69c5..0b88ac6dee41 100644 --- a/drivers/media/i2c/ov5647.c +++ b/drivers/media/i2c/ov5647.c @@ -58,6 +58,7 @@ #define OV5647_REG_MIPI_CTRL00 0x4800 #define OV5647_REG_MIPI_CTRL14 0x4814 #define OV5647_REG_AWB 0x5001 +#define OV5647_REG_ISPCTRL3D 0x503d #define REG_TERM 0xfffe #define VAL_TERM 0xfe @@ -116,6 +117,22 @@ static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd) return container_of(sd, struct ov5647, sd); } +static const char * const ov5647_test_pattern_menu[] = { + "Disabled", + "Color Bars", + "Color Squares", + "Random Data", + "Input Data" +}; + +static u8 ov5647_test_pattern_val[] = { + 0x00, /* Disabled */ + 0x80, /* Color Bars */ + 0x82, /* Color Squares */ + 0x81, /* Random Data */ + 0x83, /* Input Data */ +}; + static const struct regval_list sensor_oe_disable_regs[] = { {0x3000, 0x00}, {0x3001, 0x00}, @@ -1242,6 +1259,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl) ret = ov5647_write16(sd, OV5647_REG_VTS_HI, sensor->mode->format.height + ctrl->val); break; + case V4L2_CID_TEST_PATTERN: + ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D, + ov5647_test_pattern_val[ctrl->val]); + break; /* Read-only, but we adjust it based on mode. */ case V4L2_CID_PIXEL_RATE: @@ -1270,7 +1291,7 @@ static int ov5647_init_controls(struct ov5647 *sensor) struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd); int hblank, exposure_max, exposure_def; - v4l2_ctrl_handler_init(&sensor->ctrls, 8); + v4l2_ctrl_handler_init(&sensor->ctrls, 9); v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops, V4L2_CID_AUTOGAIN, 0, 1, 1, 0); @@ -1314,6 +1335,11 @@ static int ov5647_init_controls(struct ov5647 *sensor) sensor->mode->vts - sensor->mode->format.height); + v4l2_ctrl_new_std_menu_items(&sensor->ctrls, &ov5647_ctrl_ops, + V4L2_CID_TEST_PATTERN, + ARRAY_SIZE(ov5647_test_pattern_menu) - 1, + 0, 0, ov5647_test_pattern_menu); + if (sensor->ctrls.error) goto handler_free;