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