[PATCH 0/3] media: i2c: imx219: Feature enhancements

[PATCH 0/3] media: i2c: imx219: Feature enhancements

[Lad Prabhakar]

This patch series does the following:
1: Makes sure the sensor is LP11 state on power up
2: Adds support for RAW8
3: Adds support for 640x480 resolution

Lad Prabhakar (3):
  media: i2c: imx219: Fix power sequence
  media: i2c: imx219: Add support for SRGGB8_1X8 format
  media: i2c: imx219: Add support 640x480

 drivers/media/i2c/imx219.c | 225 ++++++++++++++++++++++++++++++++-----
 1 file changed, 199 insertions(+), 26 deletions(-)

-- 
2.20.1

[PATCH 1/3] media: i2c: imx219: Fix power sequence

[Lad Prabhakar]

When supporting Rpi Camera v2 Module on the RZ/G2E, found the driver had
some issues with rcar mipi-csi driver. The sesnosr never entered into LP-11
state.

The powerup sequence in the datasheet[1] shows the sensor entering into
LP-11 in streaming mode, so to fix this issue transitions are performed
from "standby -> streaming -> standby" in the probe().

With this commit the sensor is able to enter LP-11 mode during power up,
as expected by some CSI-2 controllers.

[1] https://publiclab.org/system/images/photos/000/023/294/original/
RASPBERRY_PI_CAMERA_V2_DATASHEET_IMX219PQH5_7.0.0_Datasheet_XXX.PDF

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/imx219.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index f1effb5a5f66..8b48e148f2d0 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -1171,6 +1171,7 @@ static int imx219_check_hwcfg(struct device *dev)
 
 static int imx219_probe(struct i2c_client *client)
 {
+	const struct imx219_reg_list *reg_list;
 	struct device *dev = &client->dev;
 	struct imx219 *imx219;
 	int ret;
@@ -1224,6 +1225,38 @@ static int imx219_probe(struct i2c_client *client)
 	/* Set default mode to max resolution */
 	imx219->mode = &supported_modes[0];
 
+	/* sensor doesn't enter to LP-11 state upon power up until and unless
+	 * streaming is started, so upon power up set the default format and
+	 * switch the modes: standby -> streaming -> standby
+	 */
+	/* getting sensor out of sleep */
+	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
+			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
+	if (ret < 0)
+		goto error_power_off;
+	usleep_range(100, 110);
+
+	reg_list = &imx219->mode->reg_list;
+	ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
+	if (ret) {
+		dev_err(&client->dev, "%s failed to default mode\n", __func__);
+		goto error_power_off;
+	}
+
+	/* getting sensor out of sleep */
+	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
+			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STREAMING);
+	if (ret < 0)
+		goto error_power_off;
+	usleep_range(100, 110);
+
+	/* put sensor back to standby mode */
+	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
+			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
+	if (ret < 0)
+		goto error_power_off;
+	usleep_range(100, 110);
+
 	ret = imx219_init_controls(imx219);
 	if (ret)
 		goto error_power_off;
-- 
2.20.1

Re: [PATCH 1/3] media: i2c: imx219: Fix power sequence

[Dave Stevenson]

Hi Lad.

Thanks again for the patch.

On Fri, 28 Feb 2020 at 16:55, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> When supporting Rpi Camera v2 Module on the RZ/G2E, found the driver had
> some issues with rcar mipi-csi driver. The sesnosr never entered into LP-11

s/sesnosr/sensor

> state.
>
> The powerup sequence in the datasheet[1] shows the sensor entering into
> LP-11 in streaming mode, so to fix this issue transitions are performed
> from "standby -> streaming -> standby" in the probe().
>
> With this commit the sensor is able to enter LP-11 mode during power up,
> as expected by some CSI-2 controllers.

I guess I'm lucky that the CSI2 receiver I deal with doesn't care on this front.
The datasheet does seem to imply that the line is left in what appears
to be LP-00 after power up, but this feels like a huge amount of stuff
to do.

> [1] https://publiclab.org/system/images/photos/000/023/294/original/
> RASPBERRY_PI_CAMERA_V2_DATASHEET_IMX219PQH5_7.0.0_Datasheet_XXX.PDF
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/imx219.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index f1effb5a5f66..8b48e148f2d0 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -1171,6 +1171,7 @@ static int imx219_check_hwcfg(struct device *dev)
>
>  static int imx219_probe(struct i2c_client *client)
>  {
> +       const struct imx219_reg_list *reg_list;
>         struct device *dev = &client->dev;
>         struct imx219 *imx219;
>         int ret;
> @@ -1224,6 +1225,38 @@ static int imx219_probe(struct i2c_client *client)
>         /* Set default mode to max resolution */
>         imx219->mode = &supported_modes[0];
>
> +       /* sensor doesn't enter to LP-11 state upon power up until and unless

Remove "to"

> +        * streaming is started, so upon power up set the default format and
> +        * switch the modes: standby -> streaming -> standby
> +        */
> +       /* getting sensor out of sleep */
> +       ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> +                              IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);

The datasheet says the default for IMX219_REG_MODE_SELECT is already 0
/ STANDY, so this should be unnecessary as we've only just powered up.

> +       if (ret < 0)
> +               goto error_power_off;
> +       usleep_range(100, 110);
> +
> +       reg_list = &imx219->mode->reg_list;
> +       ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
> +       if (ret) {
> +               dev_err(&client->dev, "%s failed to default mode\n", __func__);
> +               goto error_power_off;
> +       }

Seeing as we don't want the images produced, and we're about to power
the sensor back down again, do the default register settings do enough
to allow the shift to LP-11? ie can we drop writing any mode setup
registers here, and just got to STREAMING and back to STANDBY?

> +       /* getting sensor out of sleep */

We already did that above. This is standby->streaming.

> +       ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> +                              IMX219_REG_VALUE_08BIT, IMX219_MODE_STREAMING);
> +       if (ret < 0)
> +               goto error_power_off;
> +       usleep_range(100, 110);
> +
> +       /* put sensor back to standby mode */
> +       ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> +                              IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
> +       if (ret < 0)
> +               goto error_power_off;
> +       usleep_range(100, 110);
> +
>         ret = imx219_init_controls(imx219);
>         if (ret)
>                 goto error_power_off;
> --
> 2.20.1

Cheers,
  Dave

Re: [PATCH 1/3] media: i2c: imx219: Fix power sequence

[Lad, Prabhakar]

Hi Dave,

Thank you for the review.

On Mon, Mar 2, 2020 at 3:24 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Lad.
>
> Thanks again for the patch.
>
> On Fri, 28 Feb 2020 at 16:55, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > When supporting Rpi Camera v2 Module on the RZ/G2E, found the driver had
> > some issues with rcar mipi-csi driver. The sesnosr never entered into LP-11
>
> s/sesnosr/sensor
>
my bad shall fix that.

> > state.
> >
> > The powerup sequence in the datasheet[1] shows the sensor entering into
> > LP-11 in streaming mode, so to fix this issue transitions are performed
> > from "standby -> streaming -> standby" in the probe().
> >
> > With this commit the sensor is able to enter LP-11 mode during power up,
> > as expected by some CSI-2 controllers.
>
> I guess I'm lucky that the CSI2 receiver I deal with doesn't care on this front.
> The datasheet does seem to imply that the line is left in what appears
> to be LP-00 after power up, but this feels like a huge amount of stuff
> to do.
>
> > [1] https://publiclab.org/system/images/photos/000/023/294/original/
> > RASPBERRY_PI_CAMERA_V2_DATASHEET_IMX219PQH5_7.0.0_Datasheet_XXX.PDF
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/imx219.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index f1effb5a5f66..8b48e148f2d0 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -1171,6 +1171,7 @@ static int imx219_check_hwcfg(struct device *dev)
> >
> >  static int imx219_probe(struct i2c_client *client)
> >  {
> > +       const struct imx219_reg_list *reg_list;
> >         struct device *dev = &client->dev;
> >         struct imx219 *imx219;
> >         int ret;
> > @@ -1224,6 +1225,38 @@ static int imx219_probe(struct i2c_client *client)
> >         /* Set default mode to max resolution */
> >         imx219->mode = &supported_modes[0];
> >
> > +       /* sensor doesn't enter to LP-11 state upon power up until and unless
>
> Remove "to"
>
will do.

> > +        * streaming is started, so upon power up set the default format and
> > +        * switch the modes: standby -> streaming -> standby
> > +        */
> > +       /* getting sensor out of sleep */
> > +       ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> > +                              IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
>
> The datasheet says the default for IMX219_REG_MODE_SELECT is already 0
> / STANDY, so this should be unnecessary as we've only just powered up.
>
Agreed.

> > +       if (ret < 0)
> > +               goto error_power_off;
> > +       usleep_range(100, 110);
> > +
> > +       reg_list = &imx219->mode->reg_list;
> > +       ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
> > +       if (ret) {
> > +               dev_err(&client->dev, "%s failed to default mode\n", __func__);
> > +               goto error_power_off;
> > +       }
>
> Seeing as we don't want the images produced, and we're about to power
> the sensor back down again, do the default register settings do enough
> to allow the shift to LP-11? ie can we drop writing any mode setup
> registers here, and just got to STREAMING and back to STANDBY?
>
Yes shall replace the sequence.

> > +       /* getting sensor out of sleep */
>
> We already did that above. This is standby->streaming.
>
agreed.

> > +       ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> > +                              IMX219_REG_VALUE_08BIT, IMX219_MODE_STREAMING);
> > +       if (ret < 0)
> > +               goto error_power_off;
> > +       usleep_range(100, 110);
> > +
> > +       /* put sensor back to standby mode */
> > +       ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> > +                              IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
> > +       if (ret < 0)
> > +               goto error_power_off;
> > +       usleep_range(100, 110);
> > +
Just the above two write_reg's should do the trick.

Cheers,
--Prabhakar

> >         ret = imx219_init_controls(imx219);
> >         if (ret)
> >                 goto error_power_off;
> > --
> > 2.20.1
>
> Cheers,
>   Dave

[PATCH 2/3] media: i2c: imx219: Add support for SRGGB8_1X8 format

[Lad Prabhakar]

imx219 sensor is capable for RAW8/RAW10 modes, this commit adds support
for SRGGB8_1X8 format.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/imx219.c | 122 +++++++++++++++++++++++++++++--------
 1 file changed, 96 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 8b48e148f2d0..1388c9bc00bb 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -90,6 +90,9 @@
 
 #define IMX219_REG_ORIENTATION		0x0172
 
+#define IMX219_CSI_DATA_FORMAT_A_0_7	0x018c
+#define IMX219_CSI_DATA_FORMAT_A_8_15	0x018d
+
 /* Test Pattern Control */
 #define IMX219_REG_TEST_PATTERN		0x0600
 #define IMX219_TEST_PATTERN_DISABLE	0
@@ -135,6 +138,16 @@ struct imx219_mode {
 	struct imx219_reg_list reg_list;
 };
 
+struct imx219_pixfmt {
+	u32 code;
+	u32 colorspace;
+};
+
+static const struct imx219_pixfmt imx219_formats[] = {
+	{ MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_COLORSPACE_SRGB, },
+	{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB },
+};
+
 /*
  * Register sets lifted off the i2C interface from the Raspberry Pi firmware
  * driver.
@@ -168,8 +181,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
 	{0x0171, 0x01},
 	{0x0174, 0x00},
 	{0x0175, 0x00},
-	{0x018c, 0x0a},
-	{0x018d, 0x0a},
 	{0x0301, 0x05},
 	{0x0303, 0x01},
 	{0x0304, 0x03},
@@ -230,8 +241,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
 	{0x0171, 0x01},
 	{0x0174, 0x00},
 	{0x0175, 0x00},
-	{0x018c, 0x0a},
-	{0x018d, 0x0a},
 	{0x0301, 0x05},
 	{0x0303, 0x01},
 	{0x0304, 0x03},
@@ -290,8 +299,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
 	{0x0171, 0x01},
 	{0x0174, 0x01},
 	{0x0175, 0x01},
-	{0x018c, 0x0a},
-	{0x018d, 0x0a},
 	{0x0301, 0x05},
 	{0x0303, 0x01},
 	{0x0304, 0x03},
@@ -413,6 +420,8 @@ struct imx219 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 
+	struct v4l2_mbus_framefmt fmt;
+
 	struct clk *xclk; /* system clock to IMX219 */
 	u32 xclk_freq;
 
@@ -519,19 +528,26 @@ static int imx219_write_regs(struct imx219 *imx219,
 }
 
 /* Get bayer order based on flip setting. */
-static u32 imx219_get_format_code(struct imx219 *imx219)
+static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
 {
-	/*
-	 * Only one bayer order is supported.
-	 * It depends on the flip settings.
-	 */
-	static const u32 codes[2][2] = {
+	static const u32 codes10[2][2] = {
 		{ MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
 		{ MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
 	};
+	static const u32 codes8[2][2] = {
+		{ MEDIA_BUS_FMT_SRGGB8_1X8, MEDIA_BUS_FMT_SGRBG8_1X8, },
+		{ MEDIA_BUS_FMT_SGBRG8_1X8, MEDIA_BUS_FMT_SBGGR8_1X8, },
+	};
 
 	lockdep_assert_held(&imx219->mutex);
-	return codes[imx219->vflip->val][imx219->hflip->val];
+
+	if (code == MEDIA_BUS_FMT_SRGGB10_1X10 ||
+	    code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
+	    code == MEDIA_BUS_FMT_SGBRG10_1X10 ||
+	    code == MEDIA_BUS_FMT_SBGGR10_1X10)
+		return codes10[imx219->vflip->val][imx219->hflip->val];
+
+	return codes8[imx219->vflip->val][imx219->hflip->val];
 }
 
 static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
@@ -539,13 +555,26 @@ static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 	struct imx219 *imx219 = to_imx219(sd);
 	struct v4l2_mbus_framefmt *try_fmt =
 		v4l2_subdev_get_try_format(sd, fh->pad, 0);
+	struct v4l2_mbus_framefmt *fmt;
 
 	mutex_lock(&imx219->mutex);
 
+	fmt = &imx219->fmt;
+	fmt->code = MEDIA_BUS_FMT_SRGGB8_1X8;
+	fmt->colorspace = V4L2_COLORSPACE_SRGB;
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+							  fmt->colorspace,
+							  fmt->ycbcr_enc);
+	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
+	fmt->width = supported_modes[0].width;
+	fmt->height = supported_modes[0].height;
+	fmt->field = V4L2_FIELD_NONE;
+
 	/* Initialize try_fmt */
 	try_fmt->width = supported_modes[0].width;
 	try_fmt->height = supported_modes[0].height;
-	try_fmt->code = imx219_get_format_code(imx219);
+	try_fmt->code = imx219_get_format_code(imx219, fmt->code);
 	try_fmt->field = V4L2_FIELD_NONE;
 
 	mutex_unlock(&imx219->mutex);
@@ -646,16 +675,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_pad_config *cfg,
 				 struct v4l2_subdev_mbus_code_enum *code)
 {
-	struct imx219 *imx219 = to_imx219(sd);
-
-	/*
-	 * Only one bayer order is supported (though it depends on the flip
-	 * settings)
-	 */
-	if (code->index > 0)
+	if (code->pad != 0)
+		return -EINVAL;
+	if (code->index >= ARRAY_SIZE(imx219_formats))
 		return -EINVAL;
 
-	code->code = imx219_get_format_code(imx219);
+	code->code = imx219_formats[code->index].code;
 
 	return 0;
 }
@@ -669,7 +694,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
 	if (fse->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
 
-	if (fse->code != imx219_get_format_code(imx219))
+	if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
 		return -EINVAL;
 
 	fse->min_width = supported_modes[fse->index].width;
@@ -696,7 +721,7 @@ static void imx219_update_pad_format(struct imx219 *imx219,
 {
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
-	fmt->format.code = imx219_get_format_code(imx219);
+	fmt->format.code = imx219_get_format_code(imx219, imx219->fmt.code);
 	fmt->format.field = V4L2_FIELD_NONE;
 
 	imx219_reset_colorspace(&fmt->format);
@@ -710,7 +735,7 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
 		struct v4l2_mbus_framefmt *try_fmt =
 			v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
 		/* update the code which could change due to vflip or hflip: */
-		try_fmt->code = imx219_get_format_code(imx219);
+		try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
 		fmt->format = *try_fmt;
 	} else {
 		imx219_update_pad_format(imx219, imx219->mode, fmt);
@@ -741,11 +766,19 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	const struct imx219_mode *mode;
 	struct v4l2_mbus_framefmt *framefmt;
 	int exposure_max, exposure_def, hblank;
+	int i;
 
 	mutex_lock(&imx219->mutex);
 
+	for (i = 0; i < ARRAY_SIZE(imx219_formats); i++)
+		if (imx219_formats[i].code == fmt->format.code)
+			break;
+	if (i >= ARRAY_SIZE(imx219_formats))
+		i = 0;
+
 	/* Bayer order varies with flips */
-	fmt->format.code = imx219_get_format_code(imx219);
+	fmt->format.code = imx219_get_format_code(imx219,
+						  imx219_formats[i].code);
 
 	mode = v4l2_find_nearest_size(supported_modes,
 				      ARRAY_SIZE(supported_modes),
@@ -756,6 +789,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 		framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
 		*framefmt = fmt->format;
 	} else if (imx219->mode != mode) {
+		imx219->fmt = fmt->format;
 		imx219->mode = mode;
 		/* Update limits and set FPS to default */
 		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
@@ -786,6 +820,36 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int imx219_set_framefmt(struct imx219 *imx219)
+{
+	int ret;
+
+	switch (imx219->fmt.code) {
+	case MEDIA_BUS_FMT_SRGGB8_1X8:
+	case MEDIA_BUS_FMT_SGRBG8_1X8:
+	case MEDIA_BUS_FMT_SGBRG8_1X8:
+	case MEDIA_BUS_FMT_SBGGR8_1X8:
+		ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
+				       IMX219_REG_VALUE_08BIT, 0x08);
+		ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
+				       IMX219_REG_VALUE_08BIT, 0x08);
+		break;
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+	case MEDIA_BUS_FMT_SGRBG10_1X10:
+	case MEDIA_BUS_FMT_SGBRG10_1X10:
+	case MEDIA_BUS_FMT_SBGGR10_1X10:
+		ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
+				       IMX219_REG_VALUE_08BIT, 0x0a);
+		ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
+				       IMX219_REG_VALUE_08BIT, 0x0a);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static int imx219_start_streaming(struct imx219 *imx219)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
@@ -800,6 +864,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
 		return ret;
 	}
 
+	ret = imx219_set_framefmt(imx219);
+	if (ret) {
+		dev_err(&client->dev, "%s failed to set format\n", __func__);
+		return ret;
+	}
+
 	/* Apply customized values from user */
 	ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
 	if (ret)
-- 
2.20.1

Re: [PATCH 2/3] media: i2c: imx219: Add support for SRGGB8_1X8 format

[Dave Stevenson]

Hi Lad.

Thanks for the patch. A few things look wrong with it though.

On Fri, 28 Feb 2020 at 16:55, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> imx219 sensor is capable for RAW8/RAW10 modes, this commit adds support
> for SRGGB8_1X8 format.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/imx219.c | 122 +++++++++++++++++++++++++++++--------
>  1 file changed, 96 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 8b48e148f2d0..1388c9bc00bb 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -90,6 +90,9 @@
>
>  #define IMX219_REG_ORIENTATION         0x0172
>
> +#define IMX219_CSI_DATA_FORMAT_A_0_7   0x018c
> +#define IMX219_CSI_DATA_FORMAT_A_8_15  0x018d
> +
>  /* Test Pattern Control */
>  #define IMX219_REG_TEST_PATTERN                0x0600
>  #define IMX219_TEST_PATTERN_DISABLE    0
> @@ -135,6 +138,16 @@ struct imx219_mode {
>         struct imx219_reg_list reg_list;
>  };
>
> +struct imx219_pixfmt {
> +       u32 code;
> +       u32 colorspace;
> +};
> +
> +static const struct imx219_pixfmt imx219_formats[] = {
> +       { MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_COLORSPACE_SRGB, },
> +       { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB },

Why do we need the colorspace here when they are both the same? I
don't see any additional formats ever being added  as the sensor
doesn't support them, so this seems redundant.

> +};
> +
>  /*
>   * Register sets lifted off the i2C interface from the Raspberry Pi firmware
>   * driver.
> @@ -168,8 +181,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
>         {0x0171, 0x01},
>         {0x0174, 0x00},
>         {0x0175, 0x00},
> -       {0x018c, 0x0a},
> -       {0x018d, 0x0a},
>         {0x0301, 0x05},
>         {0x0303, 0x01},
>         {0x0304, 0x03},
> @@ -230,8 +241,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
>         {0x0171, 0x01},
>         {0x0174, 0x00},
>         {0x0175, 0x00},
> -       {0x018c, 0x0a},
> -       {0x018d, 0x0a},
>         {0x0301, 0x05},
>         {0x0303, 0x01},
>         {0x0304, 0x03},
> @@ -290,8 +299,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
>         {0x0171, 0x01},
>         {0x0174, 0x01},
>         {0x0175, 0x01},
> -       {0x018c, 0x0a},
> -       {0x018d, 0x0a},
>         {0x0301, 0x05},
>         {0x0303, 0x01},
>         {0x0304, 0x03},
>
> @@ -413,6 +420,8 @@ struct imx219 {
>         struct v4l2_subdev sd;
>         struct media_pad pad;
>
> +       struct v4l2_mbus_framefmt fmt;
> +
>         struct clk *xclk; /* system clock to IMX219 */
>         u32 xclk_freq;
>
> @@ -519,19 +528,26 @@ static int imx219_write_regs(struct imx219 *imx219,
>  }
>
>  /* Get bayer order based on flip setting. */
> -static u32 imx219_get_format_code(struct imx219 *imx219)
> +static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>  {
> -       /*
> -        * Only one bayer order is supported.
> -        * It depends on the flip settings.
> -        */
> -       static const u32 codes[2][2] = {
> +       static const u32 codes10[2][2] = {
>                 { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
>                 { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
>         };
> +       static const u32 codes8[2][2] = {
> +               { MEDIA_BUS_FMT_SRGGB8_1X8, MEDIA_BUS_FMT_SGRBG8_1X8, },
> +               { MEDIA_BUS_FMT_SGBRG8_1X8, MEDIA_BUS_FMT_SBGGR8_1X8, },
> +       };
>
>         lockdep_assert_held(&imx219->mutex);
> -       return codes[imx219->vflip->val][imx219->hflip->val];
> +
> +       if (code == MEDIA_BUS_FMT_SRGGB10_1X10 ||
> +           code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
> +           code == MEDIA_BUS_FMT_SGBRG10_1X10 ||
> +           code == MEDIA_BUS_FMT_SBGGR10_1X10)
> +               return codes10[imx219->vflip->val][imx219->hflip->val];
> +
> +       return codes8[imx219->vflip->val][imx219->hflip->val];

Why defaulting to 8 bit? It's changing the behaviour for existing users.

>  }
>
>  static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> @@ -539,13 +555,26 @@ static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>         struct imx219 *imx219 = to_imx219(sd);
>         struct v4l2_mbus_framefmt *try_fmt =
>                 v4l2_subdev_get_try_format(sd, fh->pad, 0);
> +       struct v4l2_mbus_framefmt *fmt;
>
>         mutex_lock(&imx219->mutex);
>
> +       fmt = &imx219->fmt;
> +       fmt->code = MEDIA_BUS_FMT_SRGGB8_1X8;

Again, why defaulting to 8 bit? It's changing the behaviour for existing users.

> +       fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +       fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> +       fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> +                                                         fmt->colorspace,
> +                                                         fmt->ycbcr_enc);
> +       fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +       fmt->width = supported_modes[0].width;
> +       fmt->height = supported_modes[0].height;
> +       fmt->field = V4L2_FIELD_NONE;
> +
>         /* Initialize try_fmt */
>         try_fmt->width = supported_modes[0].width;
>         try_fmt->height = supported_modes[0].height;
> -       try_fmt->code = imx219_get_format_code(imx219);
> +       try_fmt->code = imx219_get_format_code(imx219, fmt->code);
>         try_fmt->field = V4L2_FIELD_NONE;
>
>         mutex_unlock(&imx219->mutex);
> @@ -646,16 +675,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
>                                  struct v4l2_subdev_pad_config *cfg,
>                                  struct v4l2_subdev_mbus_code_enum *code)
>  {
> -       struct imx219 *imx219 = to_imx219(sd);
> -
> -       /*
> -        * Only one bayer order is supported (though it depends on the flip
> -        * settings)
> -        */
> -       if (code->index > 0)
> +       if (code->pad != 0)
> +               return -EINVAL;
> +       if (code->index >= ARRAY_SIZE(imx219_formats))
>                 return -EINVAL;
>
> -       code->code = imx219_get_format_code(imx219);
> +       code->code = imx219_formats[code->index].code;

This can't be right as it will only ever advertise
MEDIA_BUS_FMT_SRGGB8_1X8 or MEDIA_BUS_FMT_SRGGB10_1X10, when the
actual formats supported will change based on the H&V flips.
MEDIA_BUS_FMT_SRGGB8_1X8. A caller therefore can't know the correct
format should H or V flip be active, therefore can't set the right
thing.

code->code = imx219_get_format_code(imx219, imx219_formats[code->index].code);
would look more plausible.

>         return 0;
>  }
> @@ -669,7 +694,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>         if (fse->index >= ARRAY_SIZE(supported_modes))
>                 return -EINVAL;
>
> -       if (fse->code != imx219_get_format_code(imx219))
> +       if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
>                 return -EINVAL;
>
>         fse->min_width = supported_modes[fse->index].width;
> @@ -696,7 +721,7 @@ static void imx219_update_pad_format(struct imx219 *imx219,
>  {
>         fmt->format.width = mode->width;
>         fmt->format.height = mode->height;
> -       fmt->format.code = imx219_get_format_code(imx219);
> +       fmt->format.code = imx219_get_format_code(imx219, imx219->fmt.code);
>         fmt->format.field = V4L2_FIELD_NONE;
>
>         imx219_reset_colorspace(&fmt->format);
> @@ -710,7 +735,7 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
>                 struct v4l2_mbus_framefmt *try_fmt =
>                         v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
>                 /* update the code which could change due to vflip or hflip: */
> -               try_fmt->code = imx219_get_format_code(imx219);
> +               try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
>                 fmt->format = *try_fmt;
>         } else {
>                 imx219_update_pad_format(imx219, imx219->mode, fmt);
> @@ -741,11 +766,19 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>         const struct imx219_mode *mode;
>         struct v4l2_mbus_framefmt *framefmt;
>         int exposure_max, exposure_def, hblank;
> +       int i;
>
>         mutex_lock(&imx219->mutex);
>
> +       for (i = 0; i < ARRAY_SIZE(imx219_formats); i++)
> +               if (imx219_formats[i].code == fmt->format.code)
> +                       break;
> +       if (i >= ARRAY_SIZE(imx219_formats))
> +               i = 0;
> +

Again, this doesn't take into account the H&V flips altering the Bayer
format. If either are engaged then you can't change between 8 & 10 bit
formats.

It feels like having imx219_formats is the wrong approach.
We already have all the formats available in a combination of codes8
and codes10 (admittedly static to imx219_get_format_code). Is it
better to make it into a single array where there is a strict
requirement for the formats to be in the correct order of (eg) no
flip, h flip, v flip, h&v flip. A lookup can then be a straight scan
of the list. A correction for flip order is then index = (index & ~3)
| (v_flip ? 2 : 0) | (h_flip ? 1 : 0);

>         /* Bayer order varies with flips */
> -       fmt->format.code = imx219_get_format_code(imx219);
> +       fmt->format.code = imx219_get_format_code(imx219,
> +                                                 imx219_formats[i].code);
>
>         mode = v4l2_find_nearest_size(supported_modes,
>                                       ARRAY_SIZE(supported_modes),
> @@ -756,6 +789,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>                 framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
>                 *framefmt = fmt->format;
>         } else if (imx219->mode != mode) {
> +               imx219->fmt = fmt->format;
>                 imx219->mode = mode;
>                 /* Update limits and set FPS to default */
>                 __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -786,6 +820,36 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>         return 0;
>  }
>
> +static int imx219_set_framefmt(struct imx219 *imx219)
> +{
> +       int ret;
> +
> +       switch (imx219->fmt.code) {
> +       case MEDIA_BUS_FMT_SRGGB8_1X8:
> +       case MEDIA_BUS_FMT_SGRBG8_1X8:
> +       case MEDIA_BUS_FMT_SGBRG8_1X8:
> +       case MEDIA_BUS_FMT_SBGGR8_1X8:
> +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> +                                      IMX219_REG_VALUE_08BIT, 0x08);
> +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> +                                      IMX219_REG_VALUE_08BIT, 0x08);
> +               break;
> +       case MEDIA_BUS_FMT_SRGGB10_1X10:
> +       case MEDIA_BUS_FMT_SGRBG10_1X10:
> +       case MEDIA_BUS_FMT_SGBRG10_1X10:
> +       case MEDIA_BUS_FMT_SBGGR10_1X10:
> +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> +               break;

As just queried on your patch adding the 640x480 mode, do we not need
to modify 0x0309 / OPPXCK_DIV to match the pixel format?

How do you propose handling matching pixel rate vs link frequency
between the two modes?
I'm seeing corrupted images, which probably implies the FIFO between
"pipeline" and "MIPI" shown in Figure 43 of the datasheet is under or
over flowing.

> +       default:
> +               ret = -EINVAL;
> +       }
> +
> +       return ret;
> +}
> +
>  static int imx219_start_streaming(struct imx219 *imx219)
>  {
>         struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> @@ -800,6 +864,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
>                 return ret;
>         }
>
> +       ret = imx219_set_framefmt(imx219);
> +       if (ret) {
> +               dev_err(&client->dev, "%s failed to set format\n", __func__);
> +               return ret;
> +       }
> +
>         /* Apply customized values from user */
>         ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
>         if (ret)
> --
> 2.20.1
>

Re: [PATCH 2/3] media: i2c: imx219: Add support for SRGGB8_1X8 format

[Dave Stevenson]

On Mon, 2 Mar 2020 at 15:12, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Lad.
>
> Thanks for the patch. A few things look wrong with it though.
>
> On Fri, 28 Feb 2020 at 16:55, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > imx219 sensor is capable for RAW8/RAW10 modes, this commit adds support
> > for SRGGB8_1X8 format.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/imx219.c | 122 +++++++++++++++++++++++++++++--------
> >  1 file changed, 96 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 8b48e148f2d0..1388c9bc00bb 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -90,6 +90,9 @@
> >
> >  #define IMX219_REG_ORIENTATION         0x0172
> >
> > +#define IMX219_CSI_DATA_FORMAT_A_0_7   0x018c
> > +#define IMX219_CSI_DATA_FORMAT_A_8_15  0x018d
> > +
> >  /* Test Pattern Control */
> >  #define IMX219_REG_TEST_PATTERN                0x0600
> >  #define IMX219_TEST_PATTERN_DISABLE    0
> > @@ -135,6 +138,16 @@ struct imx219_mode {
> >         struct imx219_reg_list reg_list;
> >  };
> >
> > +struct imx219_pixfmt {
> > +       u32 code;
> > +       u32 colorspace;
> > +};
> > +
> > +static const struct imx219_pixfmt imx219_formats[] = {
> > +       { MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_COLORSPACE_SRGB, },
> > +       { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB },
>
> Why do we need the colorspace here when they are both the same? I
> don't see any additional formats ever being added  as the sensor
> doesn't support them, so this seems redundant.
>
> > +};
> > +
> >  /*
> >   * Register sets lifted off the i2C interface from the Raspberry Pi firmware
> >   * driver.
> > @@ -168,8 +181,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> >         {0x0171, 0x01},
> >         {0x0174, 0x00},
> >         {0x0175, 0x00},
> > -       {0x018c, 0x0a},
> > -       {0x018d, 0x0a},
> >         {0x0301, 0x05},
> >         {0x0303, 0x01},
> >         {0x0304, 0x03},
> > @@ -230,8 +241,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> >         {0x0171, 0x01},
> >         {0x0174, 0x00},
> >         {0x0175, 0x00},
> > -       {0x018c, 0x0a},
> > -       {0x018d, 0x0a},
> >         {0x0301, 0x05},
> >         {0x0303, 0x01},
> >         {0x0304, 0x03},
> > @@ -290,8 +299,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> >         {0x0171, 0x01},
> >         {0x0174, 0x01},
> >         {0x0175, 0x01},
> > -       {0x018c, 0x0a},
> > -       {0x018d, 0x0a},
> >         {0x0301, 0x05},
> >         {0x0303, 0x01},
> >         {0x0304, 0x03},
> >
> > @@ -413,6 +420,8 @@ struct imx219 {
> >         struct v4l2_subdev sd;
> >         struct media_pad pad;
> >
> > +       struct v4l2_mbus_framefmt fmt;
> > +
> >         struct clk *xclk; /* system clock to IMX219 */
> >         u32 xclk_freq;
> >
> > @@ -519,19 +528,26 @@ static int imx219_write_regs(struct imx219 *imx219,
> >  }
> >
> >  /* Get bayer order based on flip setting. */
> > -static u32 imx219_get_format_code(struct imx219 *imx219)
> > +static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> >  {
> > -       /*
> > -        * Only one bayer order is supported.
> > -        * It depends on the flip settings.
> > -        */
> > -       static const u32 codes[2][2] = {
> > +       static const u32 codes10[2][2] = {
> >                 { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> >                 { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> >         };
> > +       static const u32 codes8[2][2] = {
> > +               { MEDIA_BUS_FMT_SRGGB8_1X8, MEDIA_BUS_FMT_SGRBG8_1X8, },
> > +               { MEDIA_BUS_FMT_SGBRG8_1X8, MEDIA_BUS_FMT_SBGGR8_1X8, },
> > +       };
> >
> >         lockdep_assert_held(&imx219->mutex);
> > -       return codes[imx219->vflip->val][imx219->hflip->val];
> > +
> > +       if (code == MEDIA_BUS_FMT_SRGGB10_1X10 ||
> > +           code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
> > +           code == MEDIA_BUS_FMT_SGBRG10_1X10 ||
> > +           code == MEDIA_BUS_FMT_SBGGR10_1X10)
> > +               return codes10[imx219->vflip->val][imx219->hflip->val];
> > +
> > +       return codes8[imx219->vflip->val][imx219->hflip->val];
>
> Why defaulting to 8 bit? It's changing the behaviour for existing users.
>
> >  }
> >
> >  static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > @@ -539,13 +555,26 @@ static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> >         struct imx219 *imx219 = to_imx219(sd);
> >         struct v4l2_mbus_framefmt *try_fmt =
> >                 v4l2_subdev_get_try_format(sd, fh->pad, 0);
> > +       struct v4l2_mbus_framefmt *fmt;
> >
> >         mutex_lock(&imx219->mutex);
> >
> > +       fmt = &imx219->fmt;
> > +       fmt->code = MEDIA_BUS_FMT_SRGGB8_1X8;
>
> Again, why defaulting to 8 bit? It's changing the behaviour for existing users.
>
> > +       fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > +       fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > +       fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > +                                                         fmt->colorspace,
> > +                                                         fmt->ycbcr_enc);
> > +       fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > +       fmt->width = supported_modes[0].width;
> > +       fmt->height = supported_modes[0].height;
> > +       fmt->field = V4L2_FIELD_NONE;
> > +
> >         /* Initialize try_fmt */
> >         try_fmt->width = supported_modes[0].width;
> >         try_fmt->height = supported_modes[0].height;
> > -       try_fmt->code = imx219_get_format_code(imx219);
> > +       try_fmt->code = imx219_get_format_code(imx219, fmt->code);
> >         try_fmt->field = V4L2_FIELD_NONE;
> >
> >         mutex_unlock(&imx219->mutex);
> > @@ -646,16 +675,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> >                                  struct v4l2_subdev_pad_config *cfg,
> >                                  struct v4l2_subdev_mbus_code_enum *code)
> >  {
> > -       struct imx219 *imx219 = to_imx219(sd);
> > -
> > -       /*
> > -        * Only one bayer order is supported (though it depends on the flip
> > -        * settings)
> > -        */
> > -       if (code->index > 0)
> > +       if (code->pad != 0)
> > +               return -EINVAL;
> > +       if (code->index >= ARRAY_SIZE(imx219_formats))
> >                 return -EINVAL;
> >
> > -       code->code = imx219_get_format_code(imx219);
> > +       code->code = imx219_formats[code->index].code;
>
> This can't be right as it will only ever advertise
> MEDIA_BUS_FMT_SRGGB8_1X8 or MEDIA_BUS_FMT_SRGGB10_1X10, when the
> actual formats supported will change based on the H&V flips.
> MEDIA_BUS_FMT_SRGGB8_1X8. A caller therefore can't know the correct
> format should H or V flip be active, therefore can't set the right
> thing.
>
> code->code = imx219_get_format_code(imx219, imx219_formats[code->index].code);
> would look more plausible.
>
> >         return 0;
> >  }
> > @@ -669,7 +694,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> >         if (fse->index >= ARRAY_SIZE(supported_modes))
> >                 return -EINVAL;
> >
> > -       if (fse->code != imx219_get_format_code(imx219))
> > +       if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
> >                 return -EINVAL;
> >
> >         fse->min_width = supported_modes[fse->index].width;
> > @@ -696,7 +721,7 @@ static void imx219_update_pad_format(struct imx219 *imx219,
> >  {
> >         fmt->format.width = mode->width;
> >         fmt->format.height = mode->height;
> > -       fmt->format.code = imx219_get_format_code(imx219);
> > +       fmt->format.code = imx219_get_format_code(imx219, imx219->fmt.code);
> >         fmt->format.field = V4L2_FIELD_NONE;
> >
> >         imx219_reset_colorspace(&fmt->format);
> > @@ -710,7 +735,7 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
> >                 struct v4l2_mbus_framefmt *try_fmt =
> >                         v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
> >                 /* update the code which could change due to vflip or hflip: */
> > -               try_fmt->code = imx219_get_format_code(imx219);
> > +               try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> >                 fmt->format = *try_fmt;
> >         } else {
> >                 imx219_update_pad_format(imx219, imx219->mode, fmt);
> > @@ -741,11 +766,19 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         const struct imx219_mode *mode;
> >         struct v4l2_mbus_framefmt *framefmt;
> >         int exposure_max, exposure_def, hblank;
> > +       int i;
> >
> >         mutex_lock(&imx219->mutex);
> >
> > +       for (i = 0; i < ARRAY_SIZE(imx219_formats); i++)
> > +               if (imx219_formats[i].code == fmt->format.code)
> > +                       break;
> > +       if (i >= ARRAY_SIZE(imx219_formats))
> > +               i = 0;
> > +
>
> Again, this doesn't take into account the H&V flips altering the Bayer
> format. If either are engaged then you can't change between 8 & 10 bit
> formats.
>
> It feels like having imx219_formats is the wrong approach.
> We already have all the formats available in a combination of codes8
> and codes10 (admittedly static to imx219_get_format_code). Is it
> better to make it into a single array where there is a strict
> requirement for the formats to be in the correct order of (eg) no
> flip, h flip, v flip, h&v flip. A lookup can then be a straight scan
> of the list. A correction for flip order is then index = (index & ~3)
> | (v_flip ? 2 : 0) | (h_flip ? 1 : 0);
>
> >         /* Bayer order varies with flips */
> > -       fmt->format.code = imx219_get_format_code(imx219);
> > +       fmt->format.code = imx219_get_format_code(imx219,
> > +                                                 imx219_formats[i].code);
> >
> >         mode = v4l2_find_nearest_size(supported_modes,
> >                                       ARRAY_SIZE(supported_modes),
> > @@ -756,6 +789,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >                 framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> >                 *framefmt = fmt->format;
> >         } else if (imx219->mode != mode) {
> > +               imx219->fmt = fmt->format;
> >                 imx219->mode = mode;
> >                 /* Update limits and set FPS to default */
> >                 __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -786,6 +820,36 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         return 0;
> >  }
> >
> > +static int imx219_set_framefmt(struct imx219 *imx219)
> > +{
> > +       int ret;
> > +
> > +       switch (imx219->fmt.code) {
> > +       case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +       case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +       case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +       case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > +                                      IMX219_REG_VALUE_08BIT, 0x08);
> > +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > +                                      IMX219_REG_VALUE_08BIT, 0x08);
> > +               break;
> > +       case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +       case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +       case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +       case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> > +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> > +               break;
>
> As just queried on your patch adding the 640x480 mode, do we not need
> to modify 0x0309 / OPPXCK_DIV to match the pixel format?

To answer my own question, yes we appear to need to modify 0x0309 as
well in order to get correct images out.

> How do you propose handling matching pixel rate vs link frequency
> between the two modes?

I don't have useful tools here to determine the correct link frequency.
The pixel rate clock tree hasn't been modified, therefore must still
be the same. Indeed I'm getting the same frame rate out whether in 8
or 10 bit mode.

The division by 8 instead of 10 in OPPXCK would presumably drop the
link frequency, but the mipi_CLK feeding the MIPI block hasn't been
modified, therefore has the link frequency actually changed?

> I'm seeing corrupted images, which probably implies the FIFO between
> "pipeline" and "MIPI" shown in Figure 43 of the datasheet is under or
> over flowing.
>
> > +       default:
> > +               ret = -EINVAL;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> >  static int imx219_start_streaming(struct imx219 *imx219)
> >  {
> >         struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > @@ -800,6 +864,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
> >                 return ret;
> >         }
> >
> > +       ret = imx219_set_framefmt(imx219);
> > +       if (ret) {
> > +               dev_err(&client->dev, "%s failed to set format\n", __func__);
> > +               return ret;
> > +       }
> > +
> >         /* Apply customized values from user */
> >         ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> >         if (ret)
> > --
> > 2.20.1
> >

I've had a quick play, and I think there's a further issue with
switching between 8 and 10 bit modes when choosing the same
resolution.
imx219_set_pad_format checks that the mode is actually changing before
jumping through the hoops of updating the internal state, therefore
the change of format is ignored. An extra clause checking the format
is the minimum needed there.

I've pushed my hacks on top of your patches to
https://github.com/6by9/linux/tree/imx219
Whilst it's based on a 5.4 tree, the top few commits are applying the
mainlined driver, adding your patches, and then my fixup.
I've mainly tested that it streams sensible images in a few
resolutions and 8/10 bit modes, not that everything is perfect.

  Dave

Re: [PATCH 2/3] media: i2c: imx219: Add support for SRGGB8_1X8 format

[Lad, Prabhakar]

Hi Dave,

On Mon, Mar 2, 2020 at 5:44 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> On Mon, 2 Mar 2020 at 15:12, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi Lad.
> >
> > Thanks for the patch. A few things look wrong with it though.
> >
> > On Fri, 28 Feb 2020 at 16:55, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > >
> > > imx219 sensor is capable for RAW8/RAW10 modes, this commit adds support
> > > for SRGGB8_1X8 format.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/media/i2c/imx219.c | 122 +++++++++++++++++++++++++++++--------
> > >  1 file changed, 96 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index 8b48e148f2d0..1388c9bc00bb 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -90,6 +90,9 @@
> > >
> > >  #define IMX219_REG_ORIENTATION         0x0172
> > >
> > > +#define IMX219_CSI_DATA_FORMAT_A_0_7   0x018c
> > > +#define IMX219_CSI_DATA_FORMAT_A_8_15  0x018d
> > > +
> > >  /* Test Pattern Control */
> > >  #define IMX219_REG_TEST_PATTERN                0x0600
> > >  #define IMX219_TEST_PATTERN_DISABLE    0
> > > @@ -135,6 +138,16 @@ struct imx219_mode {
> > >         struct imx219_reg_list reg_list;
> > >  };
> > >
> > > +struct imx219_pixfmt {
> > > +       u32 code;
> > > +       u32 colorspace;
> > > +};
> > > +
> > > +static const struct imx219_pixfmt imx219_formats[] = {
> > > +       { MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_COLORSPACE_SRGB, },
> > > +       { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB },
> >
> > Why do we need the colorspace here when they are both the same? I
> > don't see any additional formats ever being added  as the sensor
> > doesn't support them, so this seems redundant.
> >
> > > +};
> > > +
> > >  /*
> > >   * Register sets lifted off the i2C interface from the Raspberry Pi firmware
> > >   * driver.
> > > @@ -168,8 +181,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> > >         {0x0171, 0x01},
> > >         {0x0174, 0x00},
> > >         {0x0175, 0x00},
> > > -       {0x018c, 0x0a},
> > > -       {0x018d, 0x0a},
> > >         {0x0301, 0x05},
> > >         {0x0303, 0x01},
> > >         {0x0304, 0x03},
> > > @@ -230,8 +241,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> > >         {0x0171, 0x01},
> > >         {0x0174, 0x00},
> > >         {0x0175, 0x00},
> > > -       {0x018c, 0x0a},
> > > -       {0x018d, 0x0a},
> > >         {0x0301, 0x05},
> > >         {0x0303, 0x01},
> > >         {0x0304, 0x03},
> > > @@ -290,8 +299,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> > >         {0x0171, 0x01},
> > >         {0x0174, 0x01},
> > >         {0x0175, 0x01},
> > > -       {0x018c, 0x0a},
> > > -       {0x018d, 0x0a},
> > >         {0x0301, 0x05},
> > >         {0x0303, 0x01},
> > >         {0x0304, 0x03},
> > >
> > > @@ -413,6 +420,8 @@ struct imx219 {
> > >         struct v4l2_subdev sd;
> > >         struct media_pad pad;
> > >
> > > +       struct v4l2_mbus_framefmt fmt;
> > > +
> > >         struct clk *xclk; /* system clock to IMX219 */
> > >         u32 xclk_freq;
> > >
> > > @@ -519,19 +528,26 @@ static int imx219_write_regs(struct imx219 *imx219,
> > >  }
> > >
> > >  /* Get bayer order based on flip setting. */
> > > -static u32 imx219_get_format_code(struct imx219 *imx219)
> > > +static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> > >  {
> > > -       /*
> > > -        * Only one bayer order is supported.
> > > -        * It depends on the flip settings.
> > > -        */
> > > -       static const u32 codes[2][2] = {
> > > +       static const u32 codes10[2][2] = {
> > >                 { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> > >                 { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> > >         };
> > > +       static const u32 codes8[2][2] = {
> > > +               { MEDIA_BUS_FMT_SRGGB8_1X8, MEDIA_BUS_FMT_SGRBG8_1X8, },
> > > +               { MEDIA_BUS_FMT_SGBRG8_1X8, MEDIA_BUS_FMT_SBGGR8_1X8, },
> > > +       };
> > >
> > >         lockdep_assert_held(&imx219->mutex);
> > > -       return codes[imx219->vflip->val][imx219->hflip->val];
> > > +
> > > +       if (code == MEDIA_BUS_FMT_SRGGB10_1X10 ||
> > > +           code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
> > > +           code == MEDIA_BUS_FMT_SGBRG10_1X10 ||
> > > +           code == MEDIA_BUS_FMT_SBGGR10_1X10)
> > > +               return codes10[imx219->vflip->val][imx219->hflip->val];
> > > +
> > > +       return codes8[imx219->vflip->val][imx219->hflip->val];
> >
> > Why defaulting to 8 bit? It's changing the behaviour for existing users.
> >
> > >  }
> > >
> > >  static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > > @@ -539,13 +555,26 @@ static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > >         struct imx219 *imx219 = to_imx219(sd);
> > >         struct v4l2_mbus_framefmt *try_fmt =
> > >                 v4l2_subdev_get_try_format(sd, fh->pad, 0);
> > > +       struct v4l2_mbus_framefmt *fmt;
> > >
> > >         mutex_lock(&imx219->mutex);
> > >
> > > +       fmt = &imx219->fmt;
> > > +       fmt->code = MEDIA_BUS_FMT_SRGGB8_1X8;
> >
> > Again, why defaulting to 8 bit? It's changing the behaviour for existing users.
> >
> > > +       fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > > +       fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > > +       fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > > +                                                         fmt->colorspace,
> > > +                                                         fmt->ycbcr_enc);
> > > +       fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > > +       fmt->width = supported_modes[0].width;
> > > +       fmt->height = supported_modes[0].height;
> > > +       fmt->field = V4L2_FIELD_NONE;
> > > +
> > >         /* Initialize try_fmt */
> > >         try_fmt->width = supported_modes[0].width;
> > >         try_fmt->height = supported_modes[0].height;
> > > -       try_fmt->code = imx219_get_format_code(imx219);
> > > +       try_fmt->code = imx219_get_format_code(imx219, fmt->code);
> > >         try_fmt->field = V4L2_FIELD_NONE;
> > >
> > >         mutex_unlock(&imx219->mutex);
> > > @@ -646,16 +675,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> > >                                  struct v4l2_subdev_pad_config *cfg,
> > >                                  struct v4l2_subdev_mbus_code_enum *code)
> > >  {
> > > -       struct imx219 *imx219 = to_imx219(sd);
> > > -
> > > -       /*
> > > -        * Only one bayer order is supported (though it depends on the flip
> > > -        * settings)
> > > -        */
> > > -       if (code->index > 0)
> > > +       if (code->pad != 0)
> > > +               return -EINVAL;
> > > +       if (code->index >= ARRAY_SIZE(imx219_formats))
> > >                 return -EINVAL;
> > >
> > > -       code->code = imx219_get_format_code(imx219);
> > > +       code->code = imx219_formats[code->index].code;
> >
> > This can't be right as it will only ever advertise
> > MEDIA_BUS_FMT_SRGGB8_1X8 or MEDIA_BUS_FMT_SRGGB10_1X10, when the
> > actual formats supported will change based on the H&V flips.
> > MEDIA_BUS_FMT_SRGGB8_1X8. A caller therefore can't know the correct
> > format should H or V flip be active, therefore can't set the right
> > thing.
> >
> > code->code = imx219_get_format_code(imx219, imx219_formats[code->index].code);
> > would look more plausible.
> >
> > >         return 0;
> > >  }
> > > @@ -669,7 +694,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> > >         if (fse->index >= ARRAY_SIZE(supported_modes))
> > >                 return -EINVAL;
> > >
> > > -       if (fse->code != imx219_get_format_code(imx219))
> > > +       if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
> > >                 return -EINVAL;
> > >
> > >         fse->min_width = supported_modes[fse->index].width;
> > > @@ -696,7 +721,7 @@ static void imx219_update_pad_format(struct imx219 *imx219,
> > >  {
> > >         fmt->format.width = mode->width;
> > >         fmt->format.height = mode->height;
> > > -       fmt->format.code = imx219_get_format_code(imx219);
> > > +       fmt->format.code = imx219_get_format_code(imx219, imx219->fmt.code);
> > >         fmt->format.field = V4L2_FIELD_NONE;
> > >
> > >         imx219_reset_colorspace(&fmt->format);
> > > @@ -710,7 +735,7 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
> > >                 struct v4l2_mbus_framefmt *try_fmt =
> > >                         v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
> > >                 /* update the code which could change due to vflip or hflip: */
> > > -               try_fmt->code = imx219_get_format_code(imx219);
> > > +               try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> > >                 fmt->format = *try_fmt;
> > >         } else {
> > >                 imx219_update_pad_format(imx219, imx219->mode, fmt);
> > > @@ -741,11 +766,19 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >         const struct imx219_mode *mode;
> > >         struct v4l2_mbus_framefmt *framefmt;
> > >         int exposure_max, exposure_def, hblank;
> > > +       int i;
> > >
> > >         mutex_lock(&imx219->mutex);
> > >
> > > +       for (i = 0; i < ARRAY_SIZE(imx219_formats); i++)
> > > +               if (imx219_formats[i].code == fmt->format.code)
> > > +                       break;
> > > +       if (i >= ARRAY_SIZE(imx219_formats))
> > > +               i = 0;
> > > +
> >
> > Again, this doesn't take into account the H&V flips altering the Bayer
> > format. If either are engaged then you can't change between 8 & 10 bit
> > formats.
> >
> > It feels like having imx219_formats is the wrong approach.
> > We already have all the formats available in a combination of codes8
> > and codes10 (admittedly static to imx219_get_format_code). Is it
> > better to make it into a single array where there is a strict
> > requirement for the formats to be in the correct order of (eg) no
> > flip, h flip, v flip, h&v flip. A lookup can then be a straight scan
> > of the list. A correction for flip order is then index = (index & ~3)
> > | (v_flip ? 2 : 0) | (h_flip ? 1 : 0);
> >
> > >         /* Bayer order varies with flips */
> > > -       fmt->format.code = imx219_get_format_code(imx219);
> > > +       fmt->format.code = imx219_get_format_code(imx219,
> > > +                                                 imx219_formats[i].code);
> > >
> > >         mode = v4l2_find_nearest_size(supported_modes,
> > >                                       ARRAY_SIZE(supported_modes),
> > > @@ -756,6 +789,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >                 framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> > >                 *framefmt = fmt->format;
> > >         } else if (imx219->mode != mode) {
> > > +               imx219->fmt = fmt->format;
> > >                 imx219->mode = mode;
> > >                 /* Update limits and set FPS to default */
> > >                 __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > > @@ -786,6 +820,36 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >         return 0;
> > >  }
> > >
> > > +static int imx219_set_framefmt(struct imx219 *imx219)
> > > +{
> > > +       int ret;
> > > +
> > > +       switch (imx219->fmt.code) {
> > > +       case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > +       case MEDIA_BUS_FMT_SGRBG8_1X8:
> > > +       case MEDIA_BUS_FMT_SGBRG8_1X8:
> > > +       case MEDIA_BUS_FMT_SBGGR8_1X8:
> > > +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > > +                                      IMX219_REG_VALUE_08BIT, 0x08);
> > > +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > > +                                      IMX219_REG_VALUE_08BIT, 0x08);
> > > +               break;
> > > +       case MEDIA_BUS_FMT_SRGGB10_1X10:
> > > +       case MEDIA_BUS_FMT_SGRBG10_1X10:
> > > +       case MEDIA_BUS_FMT_SGBRG10_1X10:
> > > +       case MEDIA_BUS_FMT_SBGGR10_1X10:
> > > +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > > +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> > > +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > > +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> > > +               break;
> >
> > As just queried on your patch adding the 640x480 mode, do we not need
> > to modify 0x0309 / OPPXCK_DIV to match the pixel format?
>
> To answer my own question, yes we appear to need to modify 0x0309 as
> well in order to get correct images out.
>
> > How do you propose handling matching pixel rate vs link frequency
> > between the two modes?
>
> I don't have useful tools here to determine the correct link frequency.
> The pixel rate clock tree hasn't been modified, therefore must still
> be the same. Indeed I'm getting the same frame rate out whether in 8
> or 10 bit mode.
>
> The division by 8 instead of 10 in OPPXCK would presumably drop the
> link frequency, but the mipi_CLK feeding the MIPI block hasn't been
> modified, therefore has the link frequency actually changed?
>
Ill do some measurements on my end check the frequencies.

> > I'm seeing corrupted images, which probably implies the FIFO between
> > "pipeline" and "MIPI" shown in Figure 43 of the datasheet is under or
> > over flowing.
> >
> > > +       default:
> > > +               ret = -EINVAL;
> > > +       }
> > > +
> > > +       return ret;
> > > +}
> > > +
> > >  static int imx219_start_streaming(struct imx219 *imx219)
> > >  {
> > >         struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > > @@ -800,6 +864,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > >                 return ret;
> > >         }
> > >
> > > +       ret = imx219_set_framefmt(imx219);
> > > +       if (ret) {
> > > +               dev_err(&client->dev, "%s failed to set format\n", __func__);
> > > +               return ret;
> > > +       }
> > > +
> > >         /* Apply customized values from user */
> > >         ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> > >         if (ret)
> > > --
> > > 2.20.1
> > >
>
> I've had a quick play, and I think there's a further issue with
> switching between 8 and 10 bit modes when choosing the same
> resolution.
> imx219_set_pad_format checks that the mode is actually changing before
> jumping through the hoops of updating the internal state, therefore
> the change of format is ignored. An extra clause checking the format
> is the minimum needed there.
>
> I've pushed my hacks on top of your patches to
> https://github.com/6by9/linux/tree/imx219
> Whilst it's based on a 5.4 tree, the top few commits are applying the
> mainlined driver, adding your patches, and then my fixup.
> I've mainly tested that it streams sensible images in a few
> resolutions and 8/10 bit modes, not that everything is perfect.
>
Appreciate the effort. I shall test it on my platform and the squash
into a single patch
if all goes well.

Cheers,
--Prabhakar

>   Dave

Re: [PATCH 2/3] media: i2c: imx219: Add support for SRGGB8_1X8 format

[Lad, Prabhakar]

Hi Dave,

Thank you for the review.

On Mon, Mar 2, 2020 at 3:12 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Lad.
>
> Thanks for the patch. A few things look wrong with it though.
>
> On Fri, 28 Feb 2020 at 16:55, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > imx219 sensor is capable for RAW8/RAW10 modes, this commit adds support
> > for SRGGB8_1X8 format.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/imx219.c | 122 +++++++++++++++++++++++++++++--------
> >  1 file changed, 96 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 8b48e148f2d0..1388c9bc00bb 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -90,6 +90,9 @@
> >
> >  #define IMX219_REG_ORIENTATION         0x0172
> >
> > +#define IMX219_CSI_DATA_FORMAT_A_0_7   0x018c
> > +#define IMX219_CSI_DATA_FORMAT_A_8_15  0x018d
> > +
> >  /* Test Pattern Control */
> >  #define IMX219_REG_TEST_PATTERN                0x0600
> >  #define IMX219_TEST_PATTERN_DISABLE    0
> > @@ -135,6 +138,16 @@ struct imx219_mode {
> >         struct imx219_reg_list reg_list;
> >  };
> >
> > +struct imx219_pixfmt {
> > +       u32 code;
> > +       u32 colorspace;
> > +};
> > +
> > +static const struct imx219_pixfmt imx219_formats[] = {
> > +       { MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_COLORSPACE_SRGB, },
> > +       { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB },
>
> Why do we need the colorspace here when they are both the same? I
> don't see any additional formats ever being added  as the sensor
> doesn't support them, so this seems redundant.
>
agreed will drop it.

> > +};
> > +
> >  /*
> >   * Register sets lifted off the i2C interface from the Raspberry Pi firmware
> >   * driver.
> > @@ -168,8 +181,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> >         {0x0171, 0x01},
> >         {0x0174, 0x00},
> >         {0x0175, 0x00},
> > -       {0x018c, 0x0a},
> > -       {0x018d, 0x0a},
> >         {0x0301, 0x05},
> >         {0x0303, 0x01},
> >         {0x0304, 0x03},
> > @@ -230,8 +241,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> >         {0x0171, 0x01},
> >         {0x0174, 0x00},
> >         {0x0175, 0x00},
> > -       {0x018c, 0x0a},
> > -       {0x018d, 0x0a},
> >         {0x0301, 0x05},
> >         {0x0303, 0x01},
> >         {0x0304, 0x03},
> > @@ -290,8 +299,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> >         {0x0171, 0x01},
> >         {0x0174, 0x01},
> >         {0x0175, 0x01},
> > -       {0x018c, 0x0a},
> > -       {0x018d, 0x0a},
> >         {0x0301, 0x05},
> >         {0x0303, 0x01},
> >         {0x0304, 0x03},
> >
> > @@ -413,6 +420,8 @@ struct imx219 {
> >         struct v4l2_subdev sd;
> >         struct media_pad pad;
> >
> > +       struct v4l2_mbus_framefmt fmt;
> > +
> >         struct clk *xclk; /* system clock to IMX219 */
> >         u32 xclk_freq;
> >
> > @@ -519,19 +528,26 @@ static int imx219_write_regs(struct imx219 *imx219,
> >  }
> >
> >  /* Get bayer order based on flip setting. */
> > -static u32 imx219_get_format_code(struct imx219 *imx219)
> > +static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> >  {
> > -       /*
> > -        * Only one bayer order is supported.
> > -        * It depends on the flip settings.
> > -        */
> > -       static const u32 codes[2][2] = {
> > +       static const u32 codes10[2][2] = {
> >                 { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> >                 { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> >         };
> > +       static const u32 codes8[2][2] = {
> > +               { MEDIA_BUS_FMT_SRGGB8_1X8, MEDIA_BUS_FMT_SGRBG8_1X8, },
> > +               { MEDIA_BUS_FMT_SGBRG8_1X8, MEDIA_BUS_FMT_SBGGR8_1X8, },
> > +       };
> >
> >         lockdep_assert_held(&imx219->mutex);
> > -       return codes[imx219->vflip->val][imx219->hflip->val];
> > +
> > +       if (code == MEDIA_BUS_FMT_SRGGB10_1X10 ||
> > +           code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
> > +           code == MEDIA_BUS_FMT_SGBRG10_1X10 ||
> > +           code == MEDIA_BUS_FMT_SBGGR10_1X10)
> > +               return codes10[imx219->vflip->val][imx219->hflip->val];
> > +
> > +       return codes8[imx219->vflip->val][imx219->hflip->val];
>
> Why defaulting to 8 bit? It's changing the behaviour for existing users.codes10
>
No it doesn't if, the format is set to 10-bit it shall look up from
codes10 and if its 8-bit
it does look from codes8.

> >  }
> >
> >  static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > @@ -539,13 +555,26 @@ static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> >         struct imx219 *imx219 = to_imx219(sd);
> >         struct v4l2_mbus_framefmt *try_fmt =
> >                 v4l2_subdev_get_try_format(sd, fh->pad, 0);
> > +       struct v4l2_mbus_framefmt *fmt;
> >
> >         mutex_lock(&imx219->mutex);
> >
> > +       fmt = &imx219->fmt;
> > +       fmt->code = MEDIA_BUS_FMT_SRGGB8_1X8;
>
> Again, why defaulting to 8 bit? It's changing the behaviour for existing users.
>
agreed.

> > +       fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > +       fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > +       fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > +                                                         fmt->colorspace,
> > +                                                         fmt->ycbcr_enc);
> > +       fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > +       fmt->width = supported_modes[0].width;
> > +       fmt->height = supported_modes[0].height;
> > +       fmt->field = V4L2_FIELD_NONE;
> > +
> >         /* Initialize try_fmt */
> >         try_fmt->width = supported_modes[0].width;
> >         try_fmt->height = supported_modes[0].height;
> > -       try_fmt->code = imx219_get_format_code(imx219);
> > +       try_fmt->code = imx219_get_format_code(imx219, fmt->code);
> >         try_fmt->field = V4L2_FIELD_NONE;
> >
> >         mutex_unlock(&imx219->mutex);
> > @@ -646,16 +675,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> >                                  struct v4l2_subdev_pad_config *cfg,
> >                                  struct v4l2_subdev_mbus_code_enum *code)
> >  {
> > -       struct imx219 *imx219 = to_imx219(sd);
> > -
> > -       /*
> > -        * Only one bayer order is supported (though it depends on the flip
> > -        * settings)
> > -        */
> > -       if (code->index > 0)
> > +       if (code->pad != 0)
> > +               return -EINVAL;
> > +       if (code->index >= ARRAY_SIZE(imx219_formats))
> >                 return -EINVAL;
> >
> > -       code->code = imx219_get_format_code(imx219);
> > +       code->code = imx219_formats[code->index].code;
>
> This can't be right as it will only ever advertise
> MEDIA_BUS_FMT_SRGGB8_1X8 or MEDIA_BUS_FMT_SRGGB10_1X10, when the
> actual formats supported will change based on the H&V flips.
> MEDIA_BUS_FMT_SRGGB8_1X8. A caller therefore can't know the correct
> format should H or V flip be active, therefore can't set the right
> thing.
>
my understanding was the current driver implementation did the same it
just exposed
MEDIA_BUS_FMT_SRGGB10_1X10 and internally driver changed it
accordingly with flip settings.

> code->code = imx219_get_format_code(imx219, imx219_formats[code->index].code);
> would look more plausible.
>
> >         return 0;
> >  }
> > @@ -669,7 +694,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> >         if (fse->index >= ARRAY_SIZE(supported_modes))
> >                 return -EINVAL;
> >
> > -       if (fse->code != imx219_get_format_code(imx219))
> > +       if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
> >                 return -EINVAL;
> >
> >         fse->min_width = supported_modes[fse->index].width;
> > @@ -696,7 +721,7 @@ static void imx219_update_pad_format(struct imx219 *imx219,
> >  {
> >         fmt->format.width = mode->width;
> >         fmt->format.height = mode->height;
> > -       fmt->format.code = imx219_get_format_code(imx219);
> > +       fmt->format.code = imx219_get_format_code(imx219, imx219->fmt.code);
> >         fmt->format.field = V4L2_FIELD_NONE;
> >
> >         imx219_reset_colorspace(&fmt->format);
> > @@ -710,7 +735,7 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
> >                 struct v4l2_mbus_framefmt *try_fmt =
> >                         v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
> >                 /* update the code which could change due to vflip or hflip: */
> > -               try_fmt->code = imx219_get_format_code(imx219);
> > +               try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> >                 fmt->format = *try_fmt;
> >         } else {
> >                 imx219_update_pad_format(imx219, imx219->mode, fmt);
> > @@ -741,11 +766,19 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         const struct imx219_mode *mode;
> >         struct v4l2_mbus_framefmt *framefmt;
> >         int exposure_max, exposure_def, hblank;
> > +       int i;
> >
> >         mutex_lock(&imx219->mutex);
> >
> > +       for (i = 0; i < ARRAY_SIZE(imx219_formats); i++)
> > +               if (imx219_formats[i].code == fmt->format.code)
> > +                       break;
> > +       if (i >= ARRAY_SIZE(imx219_formats))
> > +               i = 0;
> > +
>
> Again, this doesn't take into account the H&V flips altering the Bayer
> format. If either are engaged then you can't change between 8 & 10 bit
> formats.
>
> It feels like having imx219_formats is the wrong approach.
> We already have all the formats available in a combination of codes8
> and codes10 (admittedly static to imx219_get_format_code). Is it
> better to make it into a single array where there is a strict
> requirement for the formats to be in the correct order of (eg) no
> flip, h flip, v flip, h&v flip. A lookup can then be a straight scan
> of the list. A correction for flip order is then index = (index & ~3)
> | (v_flip ? 2 : 0) | (h_flip ? 1 : 0);
>
Agreed that would make it more cleaner.

> >         /* Bayer order varies with flips */
> > -       fmt->format.code = imx219_get_format_code(imx219);
> > +       fmt->format.code = imx219_get_format_code(imx219,
> > +                                                 imx219_formats[i].code);
> >
> >         mode = v4l2_find_nearest_size(supported_modes,
> >                                       ARRAY_SIZE(supported_modes),
> > @@ -756,6 +789,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >                 framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> >                 *framefmt = fmt->format;
> >         } else if (imx219->mode != mode) {
> > +               imx219->fmt = fmt->format;
> >                 imx219->mode = mode;
> >                 /* Update limits and set FPS to default */
> >                 __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -786,6 +820,36 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         return 0;
> >  }
> >
> > +static int imx219_set_framefmt(struct imx219 *imx219)
> > +{
> > +       int ret;
> > +
> > +       switch (imx219->fmt.code) {
> > +       case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +       case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +       case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +       case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > +                                      IMX219_REG_VALUE_08BIT, 0x08);
> > +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > +                                      IMX219_REG_VALUE_08BIT, 0x08);
> > +               break;
> > +       case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +       case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +       case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +       case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> > +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> > +               break;
>
> As just queried on your patch adding the 640x480 mode, do we not need
> to modify 0x0309 / OPPXCK_DIV to match the pixel format?
>
Yes.

> How do you propose handling matching pixel rate vs link frequency
> between the two modes?
> I'm seeing corrupted images, which probably implies the FIFO between
> "pipeline" and "MIPI" shown in Figure 43 of the datasheet is under or
> over flowing.
>
Ill do further investigation on it.

Cheers,
--Prabhakar

> > +       default:
> > +               ret = -EINVAL;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> >  static int imx219_start_streaming(struct imx219 *imx219)
> >  {
> >         struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > @@ -800,6 +864,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
> >                 return ret;
> >         }
> >
> > +       ret = imx219_set_framefmt(imx219);
> > +       if (ret) {
> > +               dev_err(&client->dev, "%s failed to set format\n", __func__);
> > +               return ret;
> > +       }
> > +
> >         /* Apply customized values from user */
> >         ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> >         if (ret)
> > --
> > 2.20.1
> >

Re: [PATCH 2/3] media: i2c: imx219: Add support for SRGGB8_1X8 format

[Andrey Konovalov]

Hi Lad,

Thanks for the patch!

On 28.02.2020 19:55, Lad Prabhakar wrote:
> imx219 sensor is capable for RAW8/RAW10 modes, this commit adds support
> for SRGGB8_1X8 format.

... but not for SGRBG8_1X8, SGBRG8_1X8, and SBGGR8_1X8?

> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>   drivers/media/i2c/imx219.c | 122 +++++++++++++++++++++++++++++--------
>   1 file changed, 96 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 8b48e148f2d0..1388c9bc00bb 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -90,6 +90,9 @@
>   
>   #define IMX219_REG_ORIENTATION		0x0172
>   
> +#define IMX219_CSI_DATA_FORMAT_A_0_7	0x018c
> +#define IMX219_CSI_DATA_FORMAT_A_8_15	0x018d
> +
>   /* Test Pattern Control */
>   #define IMX219_REG_TEST_PATTERN		0x0600
>   #define IMX219_TEST_PATTERN_DISABLE	0
> @@ -135,6 +138,16 @@ struct imx219_mode {
>   	struct imx219_reg_list reg_list;
>   };
>   
> +struct imx219_pixfmt {
> +	u32 code;
> +	u32 colorspace;
> +};
> +
> +static const struct imx219_pixfmt imx219_formats[] = {
> +	{ MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_COLORSPACE_SRGB, },
> +	{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB },
> +};

This table has only one (RGGB) layout (out of 4 possible) for 8-bit and for 10-bit
modes.

> +
>   /*
>    * Register sets lifted off the i2C interface from the Raspberry Pi firmware
>    * driver.
> @@ -168,8 +181,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
>   	{0x0171, 0x01},
>   	{0x0174, 0x00},
>   	{0x0175, 0x00},
> -	{0x018c, 0x0a},
> -	{0x018d, 0x0a},
>   	{0x0301, 0x05},
>   	{0x0303, 0x01},
>   	{0x0304, 0x03},
> @@ -230,8 +241,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
>   	{0x0171, 0x01},
>   	{0x0174, 0x00},
>   	{0x0175, 0x00},
> -	{0x018c, 0x0a},
> -	{0x018d, 0x0a},
>   	{0x0301, 0x05},
>   	{0x0303, 0x01},
>   	{0x0304, 0x03},
> @@ -290,8 +299,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
>   	{0x0171, 0x01},
>   	{0x0174, 0x01},
>   	{0x0175, 0x01},
> -	{0x018c, 0x0a},
> -	{0x018d, 0x0a},
>   	{0x0301, 0x05},
>   	{0x0303, 0x01},
>   	{0x0304, 0x03},
> @@ -413,6 +420,8 @@ struct imx219 {
>   	struct v4l2_subdev sd;
>   	struct media_pad pad;
>   
> +	struct v4l2_mbus_framefmt fmt;
> +

- adding the whole struct v4l2_mbus_framefmt for "is it RAW10 or RAW8" looks
like an overkill (and adds some duplication of code - see below).

>   	struct clk *xclk; /* system clock to IMX219 */
>   	u32 xclk_freq;
>   
> @@ -519,19 +528,26 @@ static int imx219_write_regs(struct imx219 *imx219,
>   }
>   
>   /* Get bayer order based on flip setting. */
> -static u32 imx219_get_format_code(struct imx219 *imx219)
> +static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)

Naming the 2nd argument as 'code' seems a bit confusing to me, as it
used to select between MEDIA_BUS_FMT_*_1X10 and MEDIA_BUS_FMT_*_1X8.
While the Bayer order depends on the current hflip/vflip settings
(they are stored in struct imx219).

>   {
> -	/*
> -	 * Only one bayer order is supported.
> -	 * It depends on the flip settings.
> -	 */
> -	static const u32 codes[2][2] = {
> +	static const u32 codes10[2][2] = {
>   		{ MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
>   		{ MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
>   	};
> +	static const u32 codes8[2][2] = {
> +		{ MEDIA_BUS_FMT_SRGGB8_1X8, MEDIA_BUS_FMT_SGRBG8_1X8, },
> +		{ MEDIA_BUS_FMT_SGBRG8_1X8, MEDIA_BUS_FMT_SBGGR8_1X8, },
> +	};
>   
>   	lockdep_assert_held(&imx219->mutex);
> -	return codes[imx219->vflip->val][imx219->hflip->val];
> +
> +	if (code == MEDIA_BUS_FMT_SRGGB10_1X10 ||
> +	    code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
> +	    code == MEDIA_BUS_FMT_SGBRG10_1X10 ||
> +	    code == MEDIA_BUS_FMT_SBGGR10_1X10)
> +		return codes10[imx219->vflip->val][imx219->hflip->val];
> +
> +	return codes8[imx219->vflip->val][imx219->hflip->val];
>   }

Wouldn't extending the original table be a better and simpler solution?
Something like:

static u32 imx219_get_format_code(struct imx219 *imx219, int is_8_bit)
{
	static const u32 codes[2][2][2] = {
		{
	   		{ MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
	   		{ MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
		},
		{
			{ MEDIA_BUS_FMT_SRGGB8_1X8, MEDIA_BUS_FMT_SGRBG8_1X8, },
			{ MEDIA_BUS_FMT_SGBRG8_1X8, MEDIA_BUS_FMT_SBGGR8_1X8, },
		},
	};

  	lockdep_assert_held(&imx219->mutex);
	return codes[is_8bit][imx219->vflip->val][imx219->hflip->val];
}

"is_8bit" a member of struct imx219 which equals to 0 if the sensor if RAW10 mode,
and is 1 for RAW8 mode.

>   
>   static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> @@ -539,13 +555,26 @@ static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>   	struct imx219 *imx219 = to_imx219(sd);
>   	struct v4l2_mbus_framefmt *try_fmt =
>   		v4l2_subdev_get_try_format(sd, fh->pad, 0);
> +	struct v4l2_mbus_framefmt *fmt;
>   
>   	mutex_lock(&imx219->mutex);
>   
> +	fmt = &imx219->fmt;
> +	fmt->code = MEDIA_BUS_FMT_SRGGB8_1X8;
> +	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> +	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> +							  fmt->colorspace,
> +							  fmt->ycbcr_enc);
> +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +	fmt->width = supported_modes[0].width;
> +	fmt->height = supported_modes[0].height;
> +	fmt->field = V4L2_FIELD_NONE;
> +
>   	/* Initialize try_fmt */
>   	try_fmt->width = supported_modes[0].width;
>   	try_fmt->height = supported_modes[0].height;
> -	try_fmt->code = imx219_get_format_code(imx219);
> +	try_fmt->code = imx219_get_format_code(imx219, fmt->code);
>   	try_fmt->field = V4L2_FIELD_NONE;
>   
>   	mutex_unlock(&imx219->mutex);
> @@ -646,16 +675,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
>   				 struct v4l2_subdev_pad_config *cfg,
>   				 struct v4l2_subdev_mbus_code_enum *code)
>   {
> -	struct imx219 *imx219 = to_imx219(sd);
> -
> -	/*
> -	 * Only one bayer order is supported (though it depends on the flip
> -	 * settings)
> -	 */

Guess the above comment still holds

> -	if (code->index > 0)

Though two media bus formats will be available: 8-bit and 10-bit ones, both
with the same Bayer order.

> +	if (code->pad != 0)
> +		return -EINVAL;
> +	if (code->index >= ARRAY_SIZE(imx219_formats))
>   		return -EINVAL;
>   
> -	code->code = imx219_get_format_code(imx219);
> +	code->code = imx219_formats[code->index].code;

This excludes all the Bayer orders except RGGB thus breaking the hflip/vflip support, right?

I would better check (as the sensor can only support two media bus formats at a time):
	if (code->index > 1)
		return -EINVAL;
and use imx219_get_format_code(struct imx219 *imx219, int is_8_bit) to set the code->code:
	code->code = imx219_get_format_code(imx219, code->index);

>   
>   	return 0;
>   }
> @@ -669,7 +694,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>   	if (fse->index >= ARRAY_SIZE(supported_modes))
>   		return -EINVAL;
>   
> -	if (fse->code != imx219_get_format_code(imx219))
> +	if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
>   		return -EINVAL;
>   
>   	fse->min_width = supported_modes[fse->index].width;

Looking at the "[PATCH 3/3] media: i2c: imx219: Add support 640x480",
the RAW8 mode is added to the end of the supported_modes[] array after the three RAW10 modes.
Guess this breaks VIDIOC_SUBDEV_ENUM_FRAME_SIZE ioctl for RAW8, as in the RAW8 case
it would return 0 (and the supported frame sizes) for fse->index=3 only, while for all
the other values, fse->index=0 included, it will return -EINVAL ...

> @@ -696,7 +721,7 @@ static void imx219_update_pad_format(struct imx219 *imx219,
>   {
>   	fmt->format.width = mode->width;
>   	fmt->format.height = mode->height;
> -	fmt->format.code = imx219_get_format_code(imx219);
> +	fmt->format.code = imx219_get_format_code(imx219, imx219->fmt.code);
>   	fmt->format.field = V4L2_FIELD_NONE;
>   
>   	imx219_reset_colorspace(&fmt->format);
> @@ -710,7 +735,7 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
>   		struct v4l2_mbus_framefmt *try_fmt =
>   			v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
>   		/* update the code which could change due to vflip or hflip: */
> -		try_fmt->code = imx219_get_format_code(imx219);
> +		try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
>   		fmt->format = *try_fmt;
>   	} else {
>   		imx219_update_pad_format(imx219, imx219->mode, fmt);
> @@ -741,11 +766,19 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>   	const struct imx219_mode *mode;
>   	struct v4l2_mbus_framefmt *framefmt;
>   	int exposure_max, exposure_def, hblank;
> +	int i;
>   
>   	mutex_lock(&imx219->mutex);
>   
> +	for (i = 0; i < ARRAY_SIZE(imx219_formats); i++)
> +		if (imx219_formats[i].code == fmt->format.code)

This excludes all the Bayer orders except RGGB thus breaking the hflip/vflip support, right?

> +			break;
> +	if (i >= ARRAY_SIZE(imx219_formats))
> +		i = 0;
> +
>   	/* Bayer order varies with flips */
> -	fmt->format.code = imx219_get_format_code(imx219);
> +	fmt->format.code = imx219_get_format_code(imx219,
> +						  imx219_formats[i].code);
>   
>   	mode = v4l2_find_nearest_size(supported_modes,
>   				      ARRAY_SIZE(supported_modes),
> @@ -756,6 +789,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>   		framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
>   		*framefmt = fmt->format;
>   	} else if (imx219->mode != mode) {
> +		imx219->fmt = fmt->format;
>   		imx219->mode = mode;
>   		/* Update limits and set FPS to default */
>   		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -786,6 +820,36 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>   	return 0;
>   }
>   
> +static int imx219_set_framefmt(struct imx219 *imx219)
> +{
> +	int ret;
> +
> +	switch (imx219->fmt.code) {
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +		ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> +				       IMX219_REG_VALUE_08BIT, 0x08);
> +		ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> +				       IMX219_REG_VALUE_08BIT, 0x08);
> +		break;
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +		ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> +				       IMX219_REG_VALUE_08BIT, 0x0a);
> +		ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> +				       IMX219_REG_VALUE_08BIT, 0x0a);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>   static int imx219_start_streaming(struct imx219 *imx219)
>   {
>   	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> @@ -800,6 +864,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
>   		return ret;
>   	}
>   
> +	ret = imx219_set_framefmt(imx219);

I just wonder if it makes sense to remove setting IMX219_CSI_DATA_FORMAT_A register
from the struct imx219_reg mode_x_y_regs[] tables and to introduce new imx219_set_framefmt()
function which is called right after the settings from the mode_x_y_regs[] are written
into the sensor, and is never called otherwise?

> +	if (ret) {
> +		dev_err(&client->dev, "%s failed to set format\n", __func__);
> +		return ret;
> +	}
> +
>   	/* Apply customized values from user */
>   	ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
>   	if (ret)
> 

Re: [PATCH 2/3] media: i2c: imx219: Add support for SRGGB8_1X8 format

[Lad, Prabhakar]

Hi Andrey,

Thank you for the review.

On Mon, Mar 2, 2020 at 3:13 PM Andrey Konovalov
<andrey.konovalov@linaro.org> wrote:
>
> Hi Lad,
>
> Thanks for the patch!
>
> On 28.02.2020 19:55, Lad Prabhakar wrote:
> > imx219 sensor is capable for RAW8/RAW10 modes, this commit adds support
> > for SRGGB8_1X8 format.
>
> ... but not for SGRBG8_1X8, SGBRG8_1X8, and SBGGR8_1X8?
>
Yes, will update the commit  message.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >   drivers/media/i2c/imx219.c | 122 +++++++++++++++++++++++++++++--------
> >   1 file changed, 96 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 8b48e148f2d0..1388c9bc00bb 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -90,6 +90,9 @@
> >
> >   #define IMX219_REG_ORIENTATION              0x0172
> >
> > +#define IMX219_CSI_DATA_FORMAT_A_0_7 0x018c
> > +#define IMX219_CSI_DATA_FORMAT_A_8_15        0x018d
> > +
> >   /* Test Pattern Control */
> >   #define IMX219_REG_TEST_PATTERN             0x0600
> >   #define IMX219_TEST_PATTERN_DISABLE 0
> > @@ -135,6 +138,16 @@ struct imx219_mode {
> >       struct imx219_reg_list reg_list;
> >   };
> >
> > +struct imx219_pixfmt {
> > +     u32 code;
> > +     u32 colorspace;
> > +};
> > +
> > +static const struct imx219_pixfmt imx219_formats[] = {
> > +     { MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_COLORSPACE_SRGB, },
> > +     { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB },
> > +};
>
> This table has only one (RGGB) layout (out of 4 possible) for 8-bit and for 10-bit
> modes.
>
> > +
> >   /*
> >    * Register sets lifted off the i2C interface from the Raspberry Pi firmware
> >    * driver.
> > @@ -168,8 +181,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> >       {0x0171, 0x01},
> >       {0x0174, 0x00},
> >       {0x0175, 0x00},
> > -     {0x018c, 0x0a},
> > -     {0x018d, 0x0a},
> >       {0x0301, 0x05},
> >       {0x0303, 0x01},
> >       {0x0304, 0x03},
> > @@ -230,8 +241,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> >       {0x0171, 0x01},
> >       {0x0174, 0x00},
> >       {0x0175, 0x00},
> > -     {0x018c, 0x0a},
> > -     {0x018d, 0x0a},
> >       {0x0301, 0x05},
> >       {0x0303, 0x01},
> >       {0x0304, 0x03},
> > @@ -290,8 +299,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> >       {0x0171, 0x01},
> >       {0x0174, 0x01},
> >       {0x0175, 0x01},
> > -     {0x018c, 0x0a},
> > -     {0x018d, 0x0a},
> >       {0x0301, 0x05},
> >       {0x0303, 0x01},
> >       {0x0304, 0x03},
> > @@ -413,6 +420,8 @@ struct imx219 {
> >       struct v4l2_subdev sd;
> >       struct media_pad pad;
> >
> > +     struct v4l2_mbus_framefmt fmt;
> > +
>
> - adding the whole struct v4l2_mbus_framefmt for "is it RAW10 or RAW8" looks
> like an overkill (and adds some duplication of code - see below).
>
> >       struct clk *xclk; /* system clock to IMX219 */
> >       u32 xclk_freq;
> >
> > @@ -519,19 +528,26 @@ static int imx219_write_regs(struct imx219 *imx219,
> >   }
> >
> >   /* Get bayer order based on flip setting. */
> > -static u32 imx219_get_format_code(struct imx219 *imx219)
> > +static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>
> Naming the 2nd argument as 'code' seems a bit confusing to me, as it
> used to select between MEDIA_BUS_FMT_*_1X10 and MEDIA_BUS_FMT_*_1X8.
> While the Bayer order depends on the current hflip/vflip settings
> (they are stored in struct imx219).
>
Agreed, I shall replace it with something better.
> >   {
> > -     /*
> > -      * Only one bayer order is supported.
> > -      * It depends on the flip settings.
> > -      */
> > -     static const u32 codes[2][2] = {
> > +     static const u32 codes10[2][2] = {
> >               { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> >               { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> >       };
> > +     static const u32 codes8[2][2] = {
> > +             { MEDIA_BUS_FMT_SRGGB8_1X8, MEDIA_BUS_FMT_SGRBG8_1X8, },
> > +             { MEDIA_BUS_FMT_SGBRG8_1X8, MEDIA_BUS_FMT_SBGGR8_1X8, },
> > +     };
> >
> >       lockdep_assert_held(&imx219->mutex);
> > -     return codes[imx219->vflip->val][imx219->hflip->val];
> > +
> > +     if (code == MEDIA_BUS_FMT_SRGGB10_1X10 ||
> > +         code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
> > +         code == MEDIA_BUS_FMT_SGBRG10_1X10 ||
> > +         code == MEDIA_BUS_FMT_SBGGR10_1X10)
> > +             return codes10[imx219->vflip->val][imx219->hflip->val];
> > +
> > +     return codes8[imx219->vflip->val][imx219->hflip->val];
> >   }
>
> Wouldn't extending the original table be a better and simpler solution?
> Something like:
>
> static u32 imx219_get_format_code(struct imx219 *imx219, int is_8_bit)
> {
>         static const u32 codes[2][2][2] = {
>                 {
>                         { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
>                         { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
>                 },
>                 {
>                         { MEDIA_BUS_FMT_SRGGB8_1X8, MEDIA_BUS_FMT_SGRBG8_1X8, },
>                         { MEDIA_BUS_FMT_SGBRG8_1X8, MEDIA_BUS_FMT_SBGGR8_1X8, },
>                 },
>         };
>
>         lockdep_assert_held(&imx219->mutex);
>         return codes[is_8bit][imx219->vflip->val][imx219->hflip->val];
> }
>
> "is_8bit" a member of struct imx219 which equals to 0 if the sensor if RAW10 mode,
> and is 1 for RAW8 mode.
>
Agreed.

> >
> >   static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > @@ -539,13 +555,26 @@ static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> >       struct imx219 *imx219 = to_imx219(sd);
> >       struct v4l2_mbus_framefmt *try_fmt =
> >               v4l2_subdev_get_try_format(sd, fh->pad, 0);
> > +     struct v4l2_mbus_framefmt *fmt;
> >
> >       mutex_lock(&imx219->mutex);
> >
> > +     fmt = &imx219->fmt;
> > +     fmt->code = MEDIA_BUS_FMT_SRGGB8_1X8;
> > +     fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > +     fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > +     fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > +                                                       fmt->colorspace,
> > +                                                       fmt->ycbcr_enc);
> > +     fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > +     fmt->width = supported_modes[0].width;
> > +     fmt->height = supported_modes[0].height;
> > +     fmt->field = V4L2_FIELD_NONE;
> > +
> >       /* Initialize try_fmt */
> >       try_fmt->width = supported_modes[0].width;
> >       try_fmt->height = supported_modes[0].height;
> > -     try_fmt->code = imx219_get_format_code(imx219);
> > +     try_fmt->code = imx219_get_format_code(imx219, fmt->code);
> >       try_fmt->field = V4L2_FIELD_NONE;
> >
> >       mutex_unlock(&imx219->mutex);
> > @@ -646,16 +675,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> >                                struct v4l2_subdev_pad_config *cfg,
> >                                struct v4l2_subdev_mbus_code_enum *code)
> >   {
> > -     struct imx219 *imx219 = to_imx219(sd);
> > -
> > -     /*
> > -      * Only one bayer order is supported (though it depends on the flip
> > -      * settings)
> > -      */
>
> Guess the above comment still holds
>
> > -     if (code->index > 0)
>
> Though two media bus formats will be available: 8-bit and 10-bit ones, both
> with the same Bayer order.
>
> > +     if (code->pad != 0)
> > +             return -EINVAL;
> > +     if (code->index >= ARRAY_SIZE(imx219_formats))
> >               return -EINVAL;
> >
> > -     code->code = imx219_get_format_code(imx219);
> > +     code->code = imx219_formats[code->index].code;
>
> This excludes all the Bayer orders except RGGB thus breaking the hflip/vflip support, right?
>
> I would better check (as the sensor can only support two media bus formats at a time):
>         if (code->index > 1)
>                 return -EINVAL;
> and use imx219_get_format_code(struct imx219 *imx219, int is_8_bit) to set the code->code:
>         code->code = imx219_get_format_code(imx219, code->index);
>
Yes agreed.

> >
> >       return 0;
> >   }
> > @@ -669,7 +694,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> >       if (fse->index >= ARRAY_SIZE(supported_modes))
> >               return -EINVAL;
> >
> > -     if (fse->code != imx219_get_format_code(imx219))
> > +     if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
> >               return -EINVAL;
> >
> >       fse->min_width = supported_modes[fse->index].width;
>
> Looking at the "[PATCH 3/3] media: i2c: imx219: Add support 640x480",
> the RAW8 mode is added to the end of the supported_modes[] array after the three RAW10 modes.
> Guess this breaks VIDIOC_SUBDEV_ENUM_FRAME_SIZE ioctl for RAW8, as in the RAW8 case
> it would return 0 (and the supported frame sizes) for fse->index=3 only, while for all
> the other values, fse->index=0 included, it will return -EINVAL ...
>
Not really 640x480 can be either 8/10 bit.

> > @@ -696,7 +721,7 @@ static void imx219_update_pad_format(struct imx219 *imx219,
> >   {
> >       fmt->format.width = mode->width;
> >       fmt->format.height = mode->height;
> > -     fmt->format.code = imx219_get_format_code(imx219);
> > +     fmt->format.code = imx219_get_format_code(imx219, imx219->fmt.code);
> >       fmt->format.field = V4L2_FIELD_NONE;
> >
> >       imx219_reset_colorspace(&fmt->format);
> > @@ -710,7 +735,7 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
> >               struct v4l2_mbus_framefmt *try_fmt =
> >                       v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
> >               /* update the code which could change due to vflip or hflip: */
> > -             try_fmt->code = imx219_get_format_code(imx219);
> > +             try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> >               fmt->format = *try_fmt;
> >       } else {
> >               imx219_update_pad_format(imx219, imx219->mode, fmt);
> > @@ -741,11 +766,19 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >       const struct imx219_mode *mode;
> >       struct v4l2_mbus_framefmt *framefmt;
> >       int exposure_max, exposure_def, hblank;
> > +     int i;
> >
> >       mutex_lock(&imx219->mutex);
> >
> > +     for (i = 0; i < ARRAY_SIZE(imx219_formats); i++)
> > +             if (imx219_formats[i].code == fmt->format.code)
>
> This excludes all the Bayer orders except RGGB thus breaking the hflip/vflip support, right?
>
Yes my bad.

> > +                     break;
> > +     if (i >= ARRAY_SIZE(imx219_formats))
> > +             i = 0;
> > +
> >       /* Bayer order varies with flips */
> > -     fmt->format.code = imx219_get_format_code(imx219);
> > +     fmt->format.code = imx219_get_format_code(imx219,
> > +                                               imx219_formats[i].code);
> >
> >       mode = v4l2_find_nearest_size(supported_modes,
> >                                     ARRAY_SIZE(supported_modes),
> > @@ -756,6 +789,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >               framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> >               *framefmt = fmt->format;
> >       } else if (imx219->mode != mode) {
> > +             imx219->fmt = fmt->format;
> >               imx219->mode = mode;
> >               /* Update limits and set FPS to default */
> >               __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -786,6 +820,36 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >       return 0;
> >   }
> >
> > +static int imx219_set_framefmt(struct imx219 *imx219)
> > +{
> > +     int ret;
> > +
> > +     switch (imx219->fmt.code) {
> > +     case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +     case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +     case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +     case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +             ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > +                                    IMX219_REG_VALUE_08BIT, 0x08);
> > +             ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > +                                    IMX219_REG_VALUE_08BIT, 0x08);
> > +             break;
> > +     case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +     case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +     case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +     case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +             ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > +                                    IMX219_REG_VALUE_08BIT, 0x0a);
> > +             ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > +                                    IMX219_REG_VALUE_08BIT, 0x0a);
> > +             break;
> > +     default:
> > +             ret = -EINVAL;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >   static int imx219_start_streaming(struct imx219 *imx219)
> >   {
> >       struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > @@ -800,6 +864,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
> >               return ret;
> >       }
> >
> > +     ret = imx219_set_framefmt(imx219);
>
> I just wonder if it makes sense to remove setting IMX219_CSI_DATA_FORMAT_A register
> from the struct imx219_reg mode_x_y_regs[] tables and to introduce new imx219_set_framefmt()
> function which is called right after the settings from the mode_x_y_regs[] are written
> into the sensor, and is never called otherwise?
>
These register settings depend upon the format being set either
8/10bit, as a result a imx219_set_framefmt()
is introduced to set it accordingly just before starting the streaming.

Cheers,
--Prabhakar

> > +     if (ret) {
> > +             dev_err(&client->dev, "%s failed to set format\n", __func__);
> > +             return ret;
> > +     }
> > +
> >       /* Apply customized values from user */
> >       ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> >       if (ret)
> >

[PATCH 3/3] media: i2c: imx219: Add support 640x480

[Lad Prabhakar]

This patch adds support to 640x480 cropped resolution for the sensor

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/imx219.c | 70 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 1388c9bc00bb..232ebf41063a 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -54,6 +54,7 @@
 #define IMX219_VTS_15FPS		0x0dc6
 #define IMX219_VTS_30FPS_1080P		0x06e3
 #define IMX219_VTS_30FPS_BINNED		0x06e3
+#define IMX219_VTS_30FPS_640x480	0x0239
 #define IMX219_VTS_MAX			0xffff
 
 #define IMX219_VBLANK_MIN		4
@@ -329,6 +330,65 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
 	{0x0163, 0x78},
 };
 
+static const struct imx219_reg mode_640_480_regs[] = {
+	{0x0100, 0x00},
+	{0x30eb, 0x0c},
+	{0x30eb, 0x05},
+	{0x300a, 0xff},
+	{0x300b, 0xff},
+	{0x30eb, 0x05},
+	{0x30eb, 0x09},
+	{0x0114, 0x01},
+	{0x0128, 0x01},
+	{0x012a, 0x18},
+	{0x012b, 0x00},
+	{0x0162, 0x0d},
+	{0x0163, 0xe7},
+	{0x0164, 0x03},
+	{0x0165, 0xe8},
+	{0x0166, 0x08},
+	{0x0167, 0xe7},
+	{0x0168, 0x02},
+	{0x0169, 0xf0},
+	{0x016a, 0x06},
+	{0x016b, 0xaf},
+	{0x016c, 0x02},
+	{0x016d, 0x80},
+	{0x016e, 0x01},
+	{0x016f, 0xe0},
+	{0x0170, 0x01},
+	{0x0171, 0x01},
+	{0x0172, 0x00},
+	{0x0174, 0x03},
+	{0x0175, 0x03},
+	{0x0301, 0x05},
+	{0x0303, 0x01},
+	{0x0304, 0x03},
+	{0x0305, 0x03},
+	{0x0306, 0x00},
+	{0x0307, 0x39},
+	{0x0309, 0x08},
+	{0x030b, 0x01},
+	{0x030c, 0x00},
+	{0x030d, 0x72},
+	{0x0624, 0x06},
+	{0x0625, 0x68},
+	{0x0626, 0x04},
+	{0x0627, 0xd0},
+	{0x455e, 0x00},
+	{0x471e, 0x4b},
+	{0x4767, 0x0f},
+	{0x4750, 0x14},
+	{0x4540, 0x00},
+	{0x47b4, 0x14},
+	{0x4713, 0x30},
+	{0x478b, 0x10},
+	{0x478f, 0x10},
+	{0x4793, 0x10},
+	{0x4797, 0x0e},
+	{0x479b, 0x0e},
+};
+
 static const char * const imx219_test_pattern_menu[] = {
 	"Disabled",
 	"Color Bars",
@@ -414,6 +474,16 @@ static const struct imx219_mode supported_modes[] = {
 			.regs = mode_1640_1232_regs,
 		},
 	},
+	{
+		/* 640x480 30fps mode */
+		.width = 640,
+		.height = 480,
+		.vts_def = IMX219_VTS_30FPS_640x480,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_640_480_regs),
+			.regs = mode_640_480_regs,
+		},
+	},
 };
 
 struct imx219 {
-- 
2.20.1

Re: [PATCH 3/3] media: i2c: imx219: Add support 640x480

[Dave Stevenson]

Hi Lad.

Thanks for the patch.

On Fri, 28 Feb 2020 at 16:55, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> This patch adds support to 640x480 cropped resolution for the sensor

I was a little hesitant to add cropped modes without good reason.
Processing them through an ISP with something like lens shading
compensation requires the ISP to know the crop, so ideally it should
be reflected through the selection API (probably read-only - I'm not
sure you can modify the register set totally dynamically for
cropping).
I know we have the 1080p mode in there already which is cropped, but
that was mainly as it is the only way to get 30fps 1080p over two
CSI-2 lanes. I wonder if there is a better way of reflecting this.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/imx219.c | 70 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 1388c9bc00bb..232ebf41063a 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -54,6 +54,7 @@
>  #define IMX219_VTS_15FPS               0x0dc6
>  #define IMX219_VTS_30FPS_1080P         0x06e3
>  #define IMX219_VTS_30FPS_BINNED                0x06e3
> +#define IMX219_VTS_30FPS_640x480       0x0239
>  #define IMX219_VTS_MAX                 0xffff
>
>  #define IMX219_VBLANK_MIN              4
> @@ -329,6 +330,65 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
>         {0x0163, 0x78},
>  };
>
> +static const struct imx219_reg mode_640_480_regs[] = {

Can I ask where these register settings came from? They differ from
references I have in a few odd ways.

There's also a comment at the top of mode arrays declaring the
supported modes and where they came from. Could you update that
please?

> +       {0x0100, 0x00},
> +       {0x30eb, 0x0c},
> +       {0x30eb, 0x05},
> +       {0x300a, 0xff},
> +       {0x300b, 0xff},
> +       {0x30eb, 0x05},
> +       {0x30eb, 0x09},

Datasheet section 3-4 says these are to access manufacturer specific
registers, but the access sequence should be
0x30eb 0x05
0x30eb 0x0c
0x300a 0xff
0x300b 0xff
0x30eb 0x05
0x30eb 0x09
Is there a reason your first two writes are reversed compared to this
published order?

> +       {0x0114, 0x01},
> +       {0x0128, 0x01},

DPHY_CTRL RW MIPI Global timing setting
0:auto mode, 1:manual mode

All the other modes have this as auto mode. Why does this mode need
manual settings, and is something else configuring those manual
values?

> +       {0x012a, 0x18},
> +       {0x012b, 0x00},
> +       {0x0162, 0x0d},
> +       {0x0163, 0xe7},

All the other modes have set line length to 0x0d78 (3448 decimal)
rather than your 0xd37 (3559).
Is there any specific reason for this? If we need a different value,
then we also need to vary IMX219_PPL_DEFAULT and V4L2_CID_HBLANK
depending on mode. Or probably better would be to make it variable,
but that has a load of other implications.

> +       {0x0164, 0x03},
> +       {0x0165, 0xe8},
> +       {0x0166, 0x08},
> +       {0x0167, 0xe7},
> +       {0x0168, 0x02},
> +       {0x0169, 0xf0},
> +       {0x016a, 0x06},
> +       {0x016b, 0xaf},
> +       {0x016c, 0x02},
> +       {0x016d, 0x80},
> +       {0x016e, 0x01},
> +       {0x016f, 0xe0},
> +       {0x0170, 0x01},
> +       {0x0171, 0x01},
> +       {0x0172, 0x00},

0x0172 is IMAGE_ORIENTATION_A, which is handled via V4L2_CID_HFLIP /
V4L2_CID_VFLIP, not in the mode table.

> +       {0x0174, 0x03},
> +       {0x0175, 0x03},
> +       {0x0301, 0x05},
> +       {0x0303, 0x01},
> +       {0x0304, 0x03},
> +       {0x0305, 0x03},
> +       {0x0306, 0x00},
> +       {0x0307, 0x39},
> +       {0x0309, 0x08},

"OPPXCK_DIV. Ouptut pixel clock divider value, default 0x0A."
This looks like it is a change that should be part of the support for
8bit formats.
Have you tested this mode with 10bit readout? Are the data rates correct?

> +       {0x030b, 0x01},
> +       {0x030c, 0x00},
> +       {0x030d, 0x72},
> +       {0x0624, 0x06},
> +       {0x0625, 0x68},
> +       {0x0626, 0x04},
> +       {0x0627, 0xd0},
> +       {0x455e, 0x00},
> +       {0x471e, 0x4b},
> +       {0x4767, 0x0f},
> +       {0x4750, 0x14},
> +       {0x4540, 0x00},
> +       {0x47b4, 0x14},
> +       {0x4713, 0x30},
> +       {0x478b, 0x10},
> +       {0x478f, 0x10},
> +       {0x4793, 0x10},
> +       {0x4797, 0x0e},
> +       {0x479b, 0x0e},
> +};
> +
>  static const char * const imx219_test_pattern_menu[] = {
>         "Disabled",
>         "Color Bars",
> @@ -414,6 +474,16 @@ static const struct imx219_mode supported_modes[] = {
>                         .regs = mode_1640_1232_regs,
>                 },
>         },
> +       {
> +               /* 640x480 30fps mode */
>
> +               .width = 640,
> +               .height = 480,
> +               .vts_def = IMX219_VTS_30FPS_640x480,

I've just run this mode on a Pi and I get a default of about 84fps via
v4l2-ctl to /dev/null. Is the default frame rate expected to be 30fps?
In which case I think the value of IMX219_VTS_30FPS_640x480 is wrong
(I'd expect 0x6e3 again, same as the other modes), or the comments and
define names are wrong. One or other ought to be fixed.

My calculations say that with:
- VBLANK set to 89
- a pixel rate of 182400000 (based on IMX219_PIXEL_RATE)
- HBLANK fixed at 2808
- frame being 640x480
The overall frame size is therefore (640+2808) * (480+89) = 1961912
pixel clocks. That would at first glance appear to give a frame rate
of 92fps. Testing with an alternate tool is giving me timings for
90fps but with a few dropped frames (the dropped frames would explain
v4l2-ctl reading slightly low).

If I amend OPPXCK_DIV to be 0xA (the same as the other modes), then it
doesn't appear to change.
However hold off on investigating the specifics for now - I appear to
be unable to select the 10bit/pixel formats, so I suspect something is
up with patch 2 that added the 8bit support (I was about to review
that anyway).

  Dave

> +               .reg_list = {
> +                       .num_of_regs = ARRAY_SIZE(mode_640_480_regs),
> +                       .regs = mode_640_480_regs,
> +               },
> +       },
>  };
>
>  struct imx219 {
> --
> 2.20.1
>

Re: [PATCH 3/3] media: i2c: imx219: Add support 640x480

[Lad, Prabhakar]

Hi Dave,

Thank you for the review.

On Mon, Mar 2, 2020 at 12:50 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Lad.
>
> Thanks for the patch.
>
> On Fri, 28 Feb 2020 at 16:55, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > This patch adds support to 640x480 cropped resolution for the sensor
>
> I was a little hesitant to add cropped modes without good reason.
> Processing them through an ISP with something like lens shading
> compensation requires the ISP to know the crop, so ideally it should
> be reflected through the selection API (probably read-only - I'm not
> sure you can modify the register set totally dynamically for
> cropping).
> I know we have the 1080p mode in there already which is cropped, but
> that was mainly as it is the only way to get 30fps 1080p over two
> CSI-2 lanes. I wonder if there is a better way of reflecting this.
>
The CSI controller which I am using doesn't support capture of higher
resolutions
as a result I have added support for a lower resolution.

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/imx219.c | 70 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 1388c9bc00bb..232ebf41063a 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -54,6 +54,7 @@
> >  #define IMX219_VTS_15FPS               0x0dc6
> >  #define IMX219_VTS_30FPS_1080P         0x06e3
> >  #define IMX219_VTS_30FPS_BINNED                0x06e3
> > +#define IMX219_VTS_30FPS_640x480       0x0239
> >  #define IMX219_VTS_MAX                 0xffff
> >
> >  #define IMX219_VBLANK_MIN              4
> > @@ -329,6 +330,65 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> >         {0x0163, 0x78},
> >  };
> >
> > +static const struct imx219_reg mode_640_480_regs[] = {
>
> Can I ask where these register settings came from? They differ from
> references I have in a few odd ways.
>
This driver was developed in house for imx219 sensor.

> There's also a comment at the top of mode arrays declaring the
> supported modes and where they came from. Could you update that
> please?
>
Sure ill  update it to the following:
/*
 * Register sets lifted off the i2C interface from the Raspberry Pi firmware
 * driver for resolutions 3280x2464, 1920x1080 1640x1232.
 * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 1.
 */

> > +       {0x0100, 0x00},
> > +       {0x30eb, 0x0c},
> > +       {0x30eb, 0x05},
> > +       {0x300a, 0xff},
> > +       {0x300b, 0xff},
> > +       {0x30eb, 0x05},
> > +       {0x30eb, 0x09},
>
> Datasheet section 3-4 says these are to access manufacturer specific
> registers, but the access sequence should be
> 0x30eb 0x05
> 0x30eb 0x0c
> 0x300a 0xff
> 0x300b 0xff
> 0x30eb 0x05
> 0x30eb 0x09
> Is there a reason your first two writes are reversed compared to this
> published order?
>
Agreed I have tested both the sequence works, I have now replaced as
mentioned in datasheet.

> > +       {0x0114, 0x01},
> > +       {0x0128, 0x01},
>
> DPHY_CTRL RW MIPI Global timing setting
> 0:auto mode, 1:manual mode
>
> All the other modes have this as auto mode. Why does this mode need
> manual settings, and is something else configuring those manual
> values?
>
I have reverted it to auto mode.

> > +       {0x012a, 0x18},
> > +       {0x012b, 0x00},
> > +       {0x0162, 0x0d},
> > +       {0x0163, 0xe7},
>
> All the other modes have set line length to 0x0d78 (3448 decimal)
> rather than your 0xd37 (3559).
> Is there any specific reason for this? If we need a different value,
> then we also need to vary IMX219_PPL_DEFAULT and V4L2_CID_HBLANK
> depending on mode. Or probably better would be to make it variable,
> but that has a load of other implications.
>
line length of 0x0d78 works as expected, so I have changed it now.

> > +       {0x0164, 0x03},
> > +       {0x0165, 0xe8},
> > +       {0x0166, 0x08},
> > +       {0x0167, 0xe7},
> > +       {0x0168, 0x02},
> > +       {0x0169, 0xf0},
> > +       {0x016a, 0x06},
> > +       {0x016b, 0xaf},
> > +       {0x016c, 0x02},
> > +       {0x016d, 0x80},
> > +       {0x016e, 0x01},
> > +       {0x016f, 0xe0},
> > +       {0x0170, 0x01},
> > +       {0x0171, 0x01},
> > +       {0x0172, 0x00},
>
> 0x0172 is IMAGE_ORIENTATION_A, which is handled via V4L2_CID_HFLIP /
> V4L2_CID_VFLIP, not in the mode table.
>
dropped this setting.

> > +       {0x0174, 0x03},
> > +       {0x0175, 0x03},
> > +       {0x0301, 0x05},
> > +       {0x0303, 0x01},
> > +       {0x0304, 0x03},
> > +       {0x0305, 0x03},
> > +       {0x0306, 0x00},
> > +       {0x0307, 0x39},
> > +       {0x0309, 0x08},
>
> "OPPXCK_DIV. Ouptut pixel clock divider value, default 0x0A."
> This looks like it is a change that should be part of the support for
> 8bit formats.
> Have you tested this mode with 10bit readout? Are the data rates correct?
>
as you discovered the vlaue should be 0x0A for 640x480. No I havent
tested it for
10bit.

> > +       {0x030b, 0x01},
> > +       {0x030c, 0x00},
> > +       {0x030d, 0x72},
> > +       {0x0624, 0x06},
> > +       {0x0625, 0x68},
> > +       {0x0626, 0x04},
> > +       {0x0627, 0xd0},
> > +       {0x455e, 0x00},
> > +       {0x471e, 0x4b},
> > +       {0x4767, 0x0f},
> > +       {0x4750, 0x14},
> > +       {0x4540, 0x00},
> > +       {0x47b4, 0x14},
> > +       {0x4713, 0x30},
> > +       {0x478b, 0x10},
> > +       {0x478f, 0x10},
> > +       {0x4793, 0x10},
> > +       {0x4797, 0x0e},
> > +       {0x479b, 0x0e},
> > +};
> > +
> >  static const char * const imx219_test_pattern_menu[] = {
> >         "Disabled",
> >         "Color Bars",
> > @@ -414,6 +474,16 @@ static const struct imx219_mode supported_modes[] = {
> >                         .regs = mode_1640_1232_regs,
> >                 },
> >         },
> > +       {
> > +               /* 640x480 30fps mode */
> >
> > +               .width = 640,
> > +               .height = 480,
> > +               .vts_def = IMX219_VTS_30FPS_640x480,
>
> I've just run this mode on a Pi and I get a default of about 84fps via
> v4l2-ctl to /dev/null. Is the default frame rate expected to be 30fps?
> In which case I think the value of IMX219_VTS_30FPS_640x480 is wrong
> (I'd expect 0x6e3 again, same as the other modes), or the comments and
> define names are wrong. One or other ought to be fixed.
>
> My calculations say that with:
> - VBLANK set to 89
> - a pixel rate of 182400000 (based on IMX219_PIXEL_RATE)
> - HBLANK fixed at 2808
> - frame being 640x480
> The overall frame size is therefore (640+2808) * (480+89) = 1961912
> pixel clocks. That would at first glance appear to give a frame rate
> of 92fps. Testing with an alternate tool is giving me timings for
> 90fps but with a few dropped frames (the dropped frames would explain
> v4l2-ctl reading slightly low).
>
I have set the value 0x06e3 and yavta reports 30fps:
Device /dev/video0 opened.ideo0 -c0 -n10 -s640x480 -fSRGGB8  --field
none -R80 -F
Device `R_Car_VIN' on `platform:e6ef4000.video' is a video output
(without mplanes) device.
Video format set: SRGGB8 (42474752) 640x480 (stride 640) field none
buffer size 307200
Video format: SRGGB8 (42474752) 640x480 (stride 640) field none buffer
size 307200
10 buffers requested.
length: 307200 offset: 0 timestamp type/source: mono/EoF
Buffer 0/0 mapped at address 0xffffa8126000.
length: 307200 offset: 307200 timestamp type/source: mono/EoF
Buffer 1/0 mapped at address 0xffffa80db000.
length: 307200 offset: 614400 timestamp type/source: mono/EoF
Buffer 2/0 mapped at address 0xffffa8090000.
length: 307200 offset: 921600 timestamp type/source: mono/EoF
Buffer 3/0 mapped at address 0xffffa8045000.
length: 307200 offset: 1228800 timestamp type/source: mono/EoF
Buffer 4/0 mapped at address 0xffffa7ffa000.
length: 307200 offset: 1536000 timestamp type/source: mono/EoF
Buffer 5/0 mapped at address 0xffffa7faf000.
length: 307200 offset: 1843200 timestamp type/source: mono/EoF
Buffer 6/0 mapped at address 0xffffa7f64000.
length: 307200 offset: 2150400 timestamp type/source: mono/EoF
Buffer 7/0 mapped at address 0xffffa7f19000.
length: 307200 offset: 2457600 timestamp type/source: mono/EoF
Buffer 8/0 mapped at address 0xffffa7ece000.
length: 307200 offset: 2764800 timestamp type/source: mono/EoF
Buffer 9/0 mapped at address 0xffffa7e83000.
0 (0) [-] none 0 307200 B 2227.060205 2227.060268 10.119 fps ts mono/EoF
1 (1) [-] none 1 307200 B 2227.093536 2227.105537 30.002 fps ts mono/EoF
2 (2) [-] none 2 307200 B 2227.126860 2227.301819 30.008 fps ts mono/EoF
3 (3) [-] none 3 307200 B 2227.160185 2227.340688 30.008 fps ts mono/EoF
4 (4) [-] none 4 307200 B 2227.193511 2227.384831 30.007 fps ts mono/EoF
5 (5) [-] none 5 307200 B 2227.226834 2227.431937 30.009 fps ts mono/EoF
6 (6) [-] none 6 307200 B 2227.260160 2227.476214 30.007 fps ts mono/EoF
7 (7) [-] none 7 307200 B 2227.293486 2227.522586 30.007 fps ts mono/EoF
8 (8) [-] none 8 307200 B 2227.326817 2227.564954 30.002 fps ts mono/EoF
9 (9) [-] none 9 307200 B 2227.360143 2227.610001 30.007 fps ts mono/EoF
Captured 10 frames in 0.648624 seconds (15.417250 fps, 4736179.062103 B/s).
10 buffers released.
root@ek874:~#

Cheers,
--Prabhakar

> If I amend OPPXCK_DIV to be 0xA (the same as the other modes), then it
> doesn't appear to change.
> However hold off on investigating the specifics for now - I appear to
> be unable to select the 10bit/pixel formats, so I suspect something is
> up with patch 2 that added the 8bit support (I was about to review
> that anyway).
>
>   Dave
>
> > +               .reg_list = {
> > +                       .num_of_regs = ARRAY_SIZE(mode_640_480_regs),
> > +                       .regs = mode_640_480_regs,
> > +               },
> > +       },
> >  };
> >
> >  struct imx219 {
> > --
> > 2.20.1
> >