diff mbox series

[v2,4/7] media: i2c: imx219: Fix colorspace info

Message ID 20230710155203.92366-5-jacopo.mondi@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: i2c: imx219: Use subdev active state | expand

Commit Message

Jacopo Mondi July 10, 2023, 3:52 p.m. UTC 
The IMX219 is a RAW sensor. Fix the colorspace configuration by
using V4L2_COLORSPACE_RAW and adjust the quantization and transfer
function values. Drop ycbcr_enc as it doesn't apply to RAW sensors.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

Comments

Umang Jain July 29, 2023, 2:36 p.m. UTC | #1 
Hi Jaocpo,

On 7/10/23 9:22 PM, Jacopo Mondi wrote:
> The IMX219 is a RAW sensor. Fix the colorspace configuration by
> using V4L2_COLORSPACE_RAW and adjust the quantization and transfer
> function values. Drop ycbcr_enc as it doesn't apply to RAW sensors.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>   drivers/media/i2c/imx219.c | 26 +++++++++-----------------
>   1 file changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index cd43a897391c..6963e24e1bc2 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -597,15 +597,12 @@ static void imx219_set_default_format(struct imx219 *imx219)
>   
>   	fmt = &imx219->fmt;
>   	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> -	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->colorspace = V4L2_COLORSPACE_RAW;
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>   	fmt->width = supported_modes[0].width;
>   	fmt->height = supported_modes[0].height;
>   	fmt->field = V4L2_FIELD_NONE;
> +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>   }
>   
>   static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> @@ -714,12 +711,10 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>   	format->code = imx219_get_format_code(imx219,
>   					      MEDIA_BUS_FMT_SRGGB10_1X10);
>   	format->field = V4L2_FIELD_NONE;
> -	format->colorspace = V4L2_COLORSPACE_SRGB;
> +	format->colorspace = V4L2_COLORSPACE_RAW;
>   	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);

forgot to remove ycbcr_enc from here ?

> -	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> -							     format->colorspace,
> -							     format->ycbcr_enc);
> -	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> +	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	format->xfer_func = V4L2_XFER_FUNC_NONE;
>   
>   	/* Initialize crop rectangle. */
>   	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> @@ -775,12 +770,9 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>   
>   static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
>   {
> -	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->colorspace = V4L2_COLORSPACE_RAW;
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>   }
>   
>   static void imx219_update_pad_format(struct imx219 *imx219,
Laurent Pinchart July 29, 2023, 5:13 p.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.


On Mon, Jul 10, 2023 at 05:52:00PM +0200, Jacopo Mondi wrote:
> The IMX219 is a RAW sensor. Fix the colorspace configuration by
> using V4L2_COLORSPACE_RAW and adjust the quantization and transfer
> function values. Drop ycbcr_enc as it doesn't apply to RAW sensors.

So this addresses part of my comments for 3/7, nice :-)

> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/imx219.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index cd43a897391c..6963e24e1bc2 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -597,15 +597,12 @@ static void imx219_set_default_format(struct imx219 *imx219)
>  
>  	fmt = &imx219->fmt;
>  	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> -	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->colorspace = V4L2_COLORSPACE_RAW;
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>  	fmt->width = supported_modes[0].width;
>  	fmt->height = supported_modes[0].height;
>  	fmt->field = V4L2_FIELD_NONE;
> +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;

Any reason to not group xfer_func with colorspace and quantization ?

>  }
>  
>  static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> @@ -714,12 +711,10 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>  	format->code = imx219_get_format_code(imx219,
>  					      MEDIA_BUS_FMT_SRGGB10_1X10);
>  	format->field = V4L2_FIELD_NONE;
> -	format->colorspace = V4L2_COLORSPACE_SRGB;
> +	format->colorspace = V4L2_COLORSPACE_RAW;
>  	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);

You're keeping ycbcr_enc here while you've dropped it in the two other
locations. Was that on purpose ?

While the encoding doesn't apply to raw formats, I think it should still
be set explicitly, or it will end up having a random value.

> -	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> -							     format->colorspace,
> -							     format->ycbcr_enc);
> -	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> +	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	format->xfer_func = V4L2_XFER_FUNC_NONE;
>  
>  	/* Initialize crop rectangle. */
>  	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> @@ -775,12 +770,9 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>  
>  static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
>  {
> -	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->colorspace = V4L2_COLORSPACE_RAW;
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>  }
>  
>  static void imx219_update_pad_format(struct imx219 *imx219,
Jacopo Mondi July 31, 2023, 7:32 a.m. UTC | #3 
Hi Lauren, Umang

On Sat, Jul 29, 2023 at 08:13:00PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
>
> On Mon, Jul 10, 2023 at 05:52:00PM +0200, Jacopo Mondi wrote:
> > The IMX219 is a RAW sensor. Fix the colorspace configuration by
> > using V4L2_COLORSPACE_RAW and adjust the quantization and transfer
> > function values. Drop ycbcr_enc as it doesn't apply to RAW sensors.
>
> So this addresses part of my comments for 3/7, nice :-)
>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx219.c | 26 +++++++++-----------------
> >  1 file changed, 9 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index cd43a897391c..6963e24e1bc2 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -597,15 +597,12 @@ static void imx219_set_default_format(struct imx219 *imx219)
> >
> >  	fmt = &imx219->fmt;
> >  	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > -	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->colorspace = V4L2_COLORSPACE_RAW;
> > +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> >  	fmt->width = supported_modes[0].width;
> >  	fmt->height = supported_modes[0].height;
> >  	fmt->field = V4L2_FIELD_NONE;
> > +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>
> Any reason to not group xfer_func with colorspace and quantization ?
>
> >  }
> >
> >  static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > @@ -714,12 +711,10 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
> >  	format->code = imx219_get_format_code(imx219,
> >  					      MEDIA_BUS_FMT_SRGGB10_1X10);
> >  	format->field = V4L2_FIELD_NONE;
> > -	format->colorspace = V4L2_COLORSPACE_SRGB;
> > +	format->colorspace = V4L2_COLORSPACE_RAW;
> >  	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
>
> You're keeping ycbcr_enc here while you've dropped it in the two other
> locations. Was that on purpose ?

Umang had the same comment, and I clearly forgot to remove this one

>
> While the encoding doesn't apply to raw formats, I think it should still
> be set explicitly, or it will end up having a random value.
>

Should I use V4L2_YCBCR_ENC_DEFAULT even if for RAW sensors it doesn't
make much sense ?

Thanks
   j

> > -	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > -							     format->colorspace,
> > -							     format->ycbcr_enc);
> > -	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> > +	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +	format->xfer_func = V4L2_XFER_FUNC_NONE;
> >
> >  	/* Initialize crop rectangle. */
> >  	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> > @@ -775,12 +770,9 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> >
> >  static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
> >  {
> > -	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->colorspace = V4L2_COLORSPACE_RAW;
> > +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> >  }
> >
> >  static void imx219_update_pad_format(struct imx219 *imx219,
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart July 31, 2023, 8:17 a.m. UTC | #4 
On Mon, Jul 31, 2023 at 09:32:08AM +0200, Jacopo Mondi wrote:
> On Sat, Jul 29, 2023 at 08:13:00PM +0300, Laurent Pinchart wrote:
> > On Mon, Jul 10, 2023 at 05:52:00PM +0200, Jacopo Mondi wrote:
> > > The IMX219 is a RAW sensor. Fix the colorspace configuration by
> > > using V4L2_COLORSPACE_RAW and adjust the quantization and transfer
> > > function values. Drop ycbcr_enc as it doesn't apply to RAW sensors.
> >
> > So this addresses part of my comments for 3/7, nice :-)
> >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/imx219.c | 26 +++++++++-----------------
> > >  1 file changed, 9 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index cd43a897391c..6963e24e1bc2 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -597,15 +597,12 @@ static void imx219_set_default_format(struct imx219 *imx219)
> > >
> > >  	fmt = &imx219->fmt;
> > >  	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > > -	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->colorspace = V4L2_COLORSPACE_RAW;
> > > +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > >  	fmt->width = supported_modes[0].width;
> > >  	fmt->height = supported_modes[0].height;
> > >  	fmt->field = V4L2_FIELD_NONE;
> > > +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> >
> > Any reason to not group xfer_func with colorspace and quantization ?
> >
> > >  }
> > >
> > >  static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > > @@ -714,12 +711,10 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
> > >  	format->code = imx219_get_format_code(imx219,
> > >  					      MEDIA_BUS_FMT_SRGGB10_1X10);
> > >  	format->field = V4L2_FIELD_NONE;
> > > -	format->colorspace = V4L2_COLORSPACE_SRGB;
> > > +	format->colorspace = V4L2_COLORSPACE_RAW;
> > >  	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> >
> > You're keeping ycbcr_enc here while you've dropped it in the two other
> > locations. Was that on purpose ?
> 
> Umang had the same comment, and I clearly forgot to remove this one
> 
> > While the encoding doesn't apply to raw formats, I think it should still
> > be set explicitly, or it will end up having a random value.
> 
> Should I use V4L2_YCBCR_ENC_DEFAULT even if for RAW sensors it doesn't
> make much sense ?

For the transfer function we have V4L2_XFER_FUNC_NONE, but for the
encoding we have no similar value. I recall that V4L2_YCBCR_ENC_DEFAULT
is meant for applications only, and that drivers must always return an
appropriate non-default value, but I can't find at the moment where that
is documented. It may have been from a discussion with Hans, and
v4l2-compliance may enforce it, I haven't checked.

We could add a new V4L2_YCBCR_ENC_NONE value, allow
V4L2_YCBCR_ENC_DEFAULT for this use case, or return V4L2_YCBCR_ENC_601
as done by other drivers (this is the value returned by
V4L2_MAP_YCBCR_ENC_DEFAULT() for a RAW colorspace).

> > > -	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > > -							     format->colorspace,
> > > -							     format->ycbcr_enc);
> > > -	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> > > +	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > +	format->xfer_func = V4L2_XFER_FUNC_NONE;
> > >
> > >  	/* Initialize crop rectangle. */
> > >  	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> > > @@ -775,12 +770,9 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> > >
> > >  static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
> > >  {
> > > -	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->colorspace = V4L2_COLORSPACE_RAW;
> > > +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> > >  }
> > >
> > >  static void imx219_update_pad_format(struct imx219 *imx219,
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index cd43a897391c..6963e24e1bc2 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -597,15 +597,12 @@  static void imx219_set_default_format(struct imx219 *imx219)
 
 	fmt = &imx219->fmt;
 	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
-	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->colorspace = V4L2_COLORSPACE_RAW;
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
 	fmt->width = supported_modes[0].width;
 	fmt->height = supported_modes[0].height;
 	fmt->field = V4L2_FIELD_NONE;
+	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
 }
 
 static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
@@ -714,12 +711,10 @@  static int imx219_init_cfg(struct v4l2_subdev *sd,
 	format->code = imx219_get_format_code(imx219,
 					      MEDIA_BUS_FMT_SRGGB10_1X10);
 	format->field = V4L2_FIELD_NONE;
-	format->colorspace = V4L2_COLORSPACE_SRGB;
+	format->colorspace = V4L2_COLORSPACE_RAW;
 	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
-	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
-							     format->colorspace,
-							     format->ycbcr_enc);
-	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
+	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	format->xfer_func = V4L2_XFER_FUNC_NONE;
 
 	/* Initialize crop rectangle. */
 	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
@@ -775,12 +770,9 @@  static int imx219_enum_frame_size(struct v4l2_subdev *sd,
 
 static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
 {
-	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->colorspace = V4L2_COLORSPACE_RAW;
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
 }
 
 static void imx219_update_pad_format(struct imx219 *imx219,