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

Message ID	20200228165503.18054-3-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=r8kb=4Q=vger.kernel.org=linux-media-owner@kernel.org> From: Lad Prabhakar <prabhakar.csengg@gmail.com> To: Mauro Carvalho Chehab <mchehab@kernel.org>, Dave Stevenson <dave.stevenson@raspberrypi.com> Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Subject: [PATCH 2/3] media: i2c: imx219: Add support for SRGGB8_1X8 format Date: Fri, 28 Feb 2020 16:55:02 +0000 Message-Id: <20200228165503.18054-3-prabhakar.mahadev-lad.rj@bp.renesas.com> In-Reply-To: <20200228165503.18054-1-prabhakar.mahadev-lad.rj@bp.renesas.com> References: <20200228165503.18054-1-prabhakar.mahadev-lad.rj@bp.renesas.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk
Series	media: i2c: imx219: Feature enhancements \| expand [0/3] media: i2c: imx219: Feature enhancements [1/3] media: i2c: imx219: Fix power sequence [2/3] media: i2c: imx219: Add support for SRGGB8_1X8 format [3/3] media: i2c: imx219: Add support 640x480

Prabhakar Feb. 28, 2020, 4:55 p.m. UTC

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(-)

Dave Stevenson March 2, 2020, 3:12 p.m. UTC | #1

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
>

Andrey Konovalov March 2, 2020, 3:13 p.m. UTC | #2

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)
>

Dave Stevenson March 2, 2020, 5:44 p.m. UTC | #3

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

Prabhakar March 3, 2020, 7:21 a.m. UTC | #4

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
> >

Prabhakar March 3, 2020, 7:38 a.m. UTC | #5

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)
> >

Prabhakar March 3, 2020, 7:42 a.m. UTC | #6

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

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

Commit Message

Comments

Patch