[PATCH] media: i2c: imx258: correct mode to GBGB/RGRG

[PATCH] media: i2c: imx258: correct mode to GBGB/RGRG

[Krzysztof Kozlowski]

The IMX258 sensor outputs pixels in GBGB/RGRG mode.  This is described
explicitly in datasheet and was actually mentioned in a comment inside
the driver.  Using other - wrong mode - leads to pinkish pictures.

Fixes: e4802cb00bfe ("media: imx258: Add imx258 camera sensor driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/media/i2c/imx258.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index ef069333a969..bf75d4e597af 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -715,7 +715,7 @@ static int imx258_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 	/* Initialize try_fmt */
 	try_fmt->width = supported_modes[0].width;
 	try_fmt->height = supported_modes[0].height;
-	try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
+	try_fmt->code = MEDIA_BUS_FMT_SGBRG10_1X10;
 	try_fmt->field = V4L2_FIELD_NONE;
 
 	return 0;
@@ -827,7 +827,7 @@ static int imx258_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index > 0)
 		return -EINVAL;
 
-	code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
+	code->code = MEDIA_BUS_FMT_SGBRG10_1X10;
 
 	return 0;
 }
@@ -839,7 +839,7 @@ static int imx258_enum_frame_size(struct v4l2_subdev *sd,
 	if (fse->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
 
-	if (fse->code != MEDIA_BUS_FMT_SGRBG10_1X10)
+	if (fse->code != MEDIA_BUS_FMT_SGBRG10_1X10)
 		return -EINVAL;
 
 	fse->min_width = supported_modes[fse->index].width;
@@ -855,7 +855,7 @@ static void imx258_update_pad_format(const struct imx258_mode *mode,
 {
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
-	fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10;
+	fmt->format.code = MEDIA_BUS_FMT_SGBRG10_1X10;
 	fmt->format.field = V4L2_FIELD_NONE;
 }
 
@@ -902,7 +902,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
 	mutex_lock(&imx258->mutex);
 
 	/* Only one raw bayer(GBRG) order is supported */
-	fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10;
+	fmt->format.code = MEDIA_BUS_FMT_SGBRG10_1X10;
 
 	mode = v4l2_find_nearest_size(supported_modes,
 		ARRAY_SIZE(supported_modes), width, height,
-- 
2.25.1

Re: [PATCH] media: i2c: imx258: correct mode to GBGB/RGRG

[Krzysztof Kozlowski]

On Wed, 28 Oct 2020 at 10:43, Yeh, Andy <andy.yeh@intel.com> wrote:
>
> But the sensor settings for the original submission is to output GRBG Bayer RAW.
>
> Regards, Andy

No, not to my knowledge. There are no settings for color output
because it is fixed to GBGB/RGRG. I was looking a lot into this driver
(I have few other problems with it, already few other patches posted)
and I could not find a setting for this in datasheet. If you know the
setting for the other color - can you point me to it?

Best regards,
Krzysztof

> >-----Original Message-----
> >From: Krzysztof Kozlowski <krzk@kernel.org>
> >Sent: Wednesday, October 28, 2020 5:20 PM
> >To: Sakari Ailus <sakari.ailus@linux.intel.com>; Mauro Carvalho Chehab
> ><mchehab@kernel.org>; Tomasz Figa <tfiga@chromium.org>; Jason Chen
> ><jasonx.z.chen@intel.com>; Yeh, Andy <andy.yeh@intel.com>; Alan Chiang
> ><alanx.chiang@intel.com>; linux-media@vger.kernel.org; linux-
> >kernel@vger.kernel.org
> >Cc: Krzysztof Kozlowski <krzk@kernel.org>; stable@vger.kernel.org
> >Subject: [PATCH] media: i2c: imx258: correct mode to GBGB/RGRG
> >
> >The IMX258 sensor outputs pixels in GBGB/RGRG mode.  This is described
> >explicitly in datasheet and was actually mentioned in a comment inside the
> >driver.  Using other - wrong mode - leads to pinkish pictures.
> >
> >Fixes: e4802cb00bfe ("media: imx258: Add imx258 camera sensor driver")
> >Cc: <stable@vger.kernel.org>
> >Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> >---
> > drivers/media/i2c/imx258.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c index
> >ef069333a969..bf75d4e597af 100644
> >--- a/drivers/media/i2c/imx258.c
> >+++ b/drivers/media/i2c/imx258.c
> >@@ -715,7 +715,7 @@ static int imx258_open(struct v4l2_subdev *sd, struct
> >v4l2_subdev_fh *fh)
> >       /* Initialize try_fmt */
> >       try_fmt->width = supported_modes[0].width;
> >       try_fmt->height = supported_modes[0].height;
> >-      try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> >+      try_fmt->code = MEDIA_BUS_FMT_SGBRG10_1X10;
> >       try_fmt->field = V4L2_FIELD_NONE;
> >
> >       return 0;
> >@@ -827,7 +827,7 @@ static int imx258_enum_mbus_code(struct
> >v4l2_subdev *sd,
> >       if (code->index > 0)
> >               return -EINVAL;
> >
> >-      code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> >+      code->code = MEDIA_BUS_FMT_SGBRG10_1X10;
> >
> >       return 0;
> > }
> >@@ -839,7 +839,7 @@ static int imx258_enum_frame_size(struct
> >v4l2_subdev *sd,
> >       if (fse->index >= ARRAY_SIZE(supported_modes))
> >               return -EINVAL;
> >
> >-      if (fse->code != MEDIA_BUS_FMT_SGRBG10_1X10)
> >+      if (fse->code != MEDIA_BUS_FMT_SGBRG10_1X10)
> >               return -EINVAL;
> >
> >       fse->min_width = supported_modes[fse->index].width;
> >@@ -855,7 +855,7 @@ static void imx258_update_pad_format(const struct
> >imx258_mode *mode,  {
> >       fmt->format.width = mode->width;
> >       fmt->format.height = mode->height;
> >-      fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10;
> >+      fmt->format.code = MEDIA_BUS_FMT_SGBRG10_1X10;
> >       fmt->format.field = V4L2_FIELD_NONE;
> > }
> >
> >@@ -902,7 +902,7 @@ static int imx258_set_pad_format(struct v4l2_subdev
> >*sd,
> >       mutex_lock(&imx258->mutex);
> >
> >       /* Only one raw bayer(GBRG) order is supported */
> >-      fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10;
> >+      fmt->format.code = MEDIA_BUS_FMT_SGBRG10_1X10;
> >
> >       mode = v4l2_find_nearest_size(supported_modes,
> >               ARRAY_SIZE(supported_modes), width, height,
> >--
> >2.25.1
>

Re: [PATCH] media: i2c: imx258: correct mode to GBGB/RGRG

[Krzysztof Kozlowski]

On Wed, 28 Oct 2020 at 10:45, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Wed, 28 Oct 2020 at 10:43, Yeh, Andy <andy.yeh@intel.com> wrote:
> >
> > But the sensor settings for the original submission is to output GRBG Bayer RAW.
> >
> > Regards, Andy
>
> No, not to my knowledge. There are no settings for color output
> because it is fixed to GBGB/RGRG. I was looking a lot into this driver
> (I have few other problems with it, already few other patches posted)
> and I could not find a setting for this in datasheet. If you know the
> setting for the other color - can you point me to it?

And except the datasheet which mentions the specific format, the
testing confirms it. With original color the pictures are pink/purple.
With proper color, the pictures are correct (with more green color as
expected for bayer).

Best regards,
Krzysztof

Re: [PATCH] media: i2c: imx258: correct mode to GBGB/RGRG

[Krzysztof Kozlowski]

On Wed, 28 Oct 2020 at 11:03, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> On Wed, Oct 28, 2020 at 10:56:55AM +0100, Krzysztof Kozlowski wrote:
> > On Wed, 28 Oct 2020 at 10:45, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > On Wed, 28 Oct 2020 at 10:43, Yeh, Andy <andy.yeh@intel.com> wrote:
> > > >
> > > > But the sensor settings for the original submission is to output GRBG Bayer RAW.
> > > >
> > > > Regards, Andy
> > >
> > > No, not to my knowledge. There are no settings for color output
> > > because it is fixed to GBGB/RGRG. I was looking a lot into this driver
> > > (I have few other problems with it, already few other patches posted)
> > > and I could not find a setting for this in datasheet. If you know the
> > > setting for the other color - can you point me to it?
> >
> > And except the datasheet which mentions the specific format, the
> > testing confirms it. With original color the pictures are pink/purple.
> > With proper color, the pictures are correct (with more green color as
> > expected for bayer).
>
> Quoting the driver's start_streaming function:
>
>         /* Set Orientation be 180 degree */
>         ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
>                                IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);

I understand that you think it will replace the lines and columns and
the first line will be RG, instead of GB.... or actually BG because it
flips horizontal and vertical? So why does it not work?

BTW, this nicely points that the comment around
device_property_read_u32() for rotation is a little bit misleading :)

>         if (ret) {
>                 dev_err(&client->dev, "%s failed to set orientation\n",
>                         __func__);
>                 return ret;
>         }
>
> Could it be you're taking pictures of pink objects? ;-)

I can send a few sample pictures taken with GStreamer (RAW8, not
original RAW10)...

Best regards,
Krzysztof

Re: [PATCH] media: i2c: imx258: correct mode to GBGB/RGRG

[Tomasz Figa]

On Wed, Oct 28, 2020 at 11:15 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Wed, 28 Oct 2020 at 11:03, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > On Wed, Oct 28, 2020 at 10:56:55AM +0100, Krzysztof Kozlowski wrote:
> > > On Wed, 28 Oct 2020 at 10:45, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > On Wed, 28 Oct 2020 at 10:43, Yeh, Andy <andy.yeh@intel.com> wrote:
> > > > >
> > > > > But the sensor settings for the original submission is to output GRBG Bayer RAW.
> > > > >
> > > > > Regards, Andy
> > > >
> > > > No, not to my knowledge. There are no settings for color output
> > > > because it is fixed to GBGB/RGRG. I was looking a lot into this driver
> > > > (I have few other problems with it, already few other patches posted)
> > > > and I could not find a setting for this in datasheet. If you know the
> > > > setting for the other color - can you point me to it?
> > >
> > > And except the datasheet which mentions the specific format, the
> > > testing confirms it. With original color the pictures are pink/purple.
> > > With proper color, the pictures are correct (with more green color as
> > > expected for bayer).
> >
> > Quoting the driver's start_streaming function:
> >
> >         /* Set Orientation be 180 degree */
> >         ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> >                                IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);
>
> I understand that you think it will replace the lines and columns and
> the first line will be RG, instead of GB.... or actually BG because it
> flips horizontal and vertical? So why does it not work?

Any chance your SoC capture interface performs this flipping on its own as well?

>
> BTW, this nicely points that the comment around
> device_property_read_u32() for rotation is a little bit misleading :)
>

Are you referring to the comment below?

/*
* Check that the device is mounted upside down. The driver only
* supports a single pixel order right now.
*/
ret = device_property_read_u32(&client->dev, "rotation", &val);
if (ret || val != 180)
    return -EINVAL;

What's misleading about it?

> >         if (ret) {
> >                 dev_err(&client->dev, "%s failed to set orientation\n",
> >                         __func__);
> >                 return ret;
> >         }
> >
> > Could it be you're taking pictures of pink objects? ;-)
>
> I can send a few sample pictures taken with GStreamer (RAW8, not
> original RAW10)...
>
> Best regards,
> Krzysztof

Re: [PATCH] media: i2c: imx258: correct mode to GBGB/RGRG

[Krzysztof Kozlowski]

On Wed, Oct 28, 2020 at 10:14:52AM +0000, Yeh, Andy wrote:
> >-----Original Message-----
> >From: Sakari Ailus <sakari.ailus@linux.intel.com>
> >Sent: Wednesday, October 28, 2020 6:03 PM
> >To: Krzysztof Kozlowski <krzk@kernel.org>
> >Cc: Yeh, Andy <andy.yeh@intel.com>; Mauro Carvalho Chehab
> ><mchehab@kernel.org>; Tomasz Figa <tfiga@chromium.org>; Jason Chen
> ><jasonx.z.chen@intel.com>; Alan Chiang <alanx.chiang@intel.com>; linux-
> >media@vger.kernel.org; linux-kernel@vger.kernel.org;
> >stable@vger.kernel.org
> >Subject: Re: [PATCH] media: i2c: imx258: correct mode to GBGB/RGRG
> >
> >On Wed, Oct 28, 2020 at 10:56:55AM +0100, Krzysztof Kozlowski wrote:
> >> On Wed, 28 Oct 2020 at 10:45, Krzysztof Kozlowski <krzk@kernel.org>
> >wrote:
> >> >
> >> > On Wed, 28 Oct 2020 at 10:43, Yeh, Andy <andy.yeh@intel.com> wrote:
> >> > >
> >> > > But the sensor settings for the original submission is to output GRBG
> >Bayer RAW.
> >> > >
> >> > > Regards, Andy
> >> >
> >> > No, not to my knowledge. There are no settings for color output
> >> > because it is fixed to GBGB/RGRG. I was looking a lot into this
> >> > driver (I have few other problems with it, already few other patches
> >> > posted) and I could not find a setting for this in datasheet. If you
> >> > know the setting for the other color - can you point me to it?
> >>
> >> And except the datasheet which mentions the specific format, the
> >> testing confirms it. With original color the pictures are pink/purple.
> >> With proper color, the pictures are correct (with more green color as
> >> expected for bayer).
> >
> >Quoting the driver's start_streaming function:
> >
> >        /* Set Orientation be 180 degree */
> >        ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> >                               IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);
> >        if (ret) {
> >                dev_err(&client->dev, "%s failed to set orientation\n",
> >                        __func__);
> >                return ret;
> >        }
> >
> >Could it be you're taking pictures of pink objects? ;-)
> >
> >--
> >Sakari Ailus
> 
> Sakari is right. IIRC since the default sensor settings outputs in GBRG, and after mirro/flip (it is the original application when submit the driver) the bayer order will be GRBG.

Yes, you are all right. It seems I was using wrong color mode for
Gstreamer, or I messed up something more.

Thanks for help everyone!

The patch can be dropped.

Best regards,
Krzysztof

Re: [PATCH] media: i2c: imx258: correct mode to GBGB/RGRG

[Krzysztof Kozlowski]

On Wed, Oct 28, 2020 at 01:08:28PM +0100, Tomasz Figa wrote:
> On Wed, Oct 28, 2020 at 11:15 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On Wed, 28 Oct 2020 at 11:03, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > >
> > > On Wed, Oct 28, 2020 at 10:56:55AM +0100, Krzysztof Kozlowski wrote:
> > > > On Wed, 28 Oct 2020 at 10:45, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > >
> > > > > On Wed, 28 Oct 2020 at 10:43, Yeh, Andy <andy.yeh@intel.com> wrote:
> > > > > >
> > > > > > But the sensor settings for the original submission is to output GRBG Bayer RAW.
> > > > > >
> > > > > > Regards, Andy
> > > > >
> > > > > No, not to my knowledge. There are no settings for color output
> > > > > because it is fixed to GBGB/RGRG. I was looking a lot into this driver
> > > > > (I have few other problems with it, already few other patches posted)
> > > > > and I could not find a setting for this in datasheet. If you know the
> > > > > setting for the other color - can you point me to it?
> > > >
> > > > And except the datasheet which mentions the specific format, the
> > > > testing confirms it. With original color the pictures are pink/purple.
> > > > With proper color, the pictures are correct (with more green color as
> > > > expected for bayer).
> > >
> > > Quoting the driver's start_streaming function:
> > >
> > >         /* Set Orientation be 180 degree */
> > >         ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> > >                                IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);
> >
> > I understand that you think it will replace the lines and columns and
> > the first line will be RG, instead of GB.... or actually BG because it
> > flips horizontal and vertical? So why does it not work?
> 
> Any chance your SoC capture interface performs this flipping on its own as well?

I messed up GStreamer as well. The color mode is correct in the driver.

> 
> >
> > BTW, this nicely points that the comment around
> > device_property_read_u32() for rotation is a little bit misleading :)
> >
> 
> Are you referring to the comment below?
> 
> /*
> * Check that the device is mounted upside down. The driver only
> * supports a single pixel order right now.
> */
> ret = device_property_read_u32(&client->dev, "rotation", &val);
> if (ret || val != 180)
>     return -EINVAL;
> 
> What's misleading about it?

It's interpretation. To me the comment does not say that device
*have to* be mounted upside down. To me, it says that from all rotations
(90, -90, 180), it supports only upside down, except of course normal
mounting (no rotation).

Best regards,
Krzysztof