Re: [PATCH v9 11/11] media: i2c: ov7670: Fully set mbus frame fmt

[jacopo mondi]

Hi Laurent,

On Thu, Feb 22, 2018 at 02:14:53PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thursday, 22 February 2018 14:04:12 EET jacopo mondi wrote:
> > On Wed, Feb 21, 2018 at 10:28:06PM +0200, Laurent Pinchart wrote:
> > > On Tuesday, 20 February 2018 10:58:57 EET jacopo mondi wrote:
> > > > On Mon, Feb 19, 2018 at 09:19:32PM +0200, Laurent Pinchart wrote:
> > > > > On Monday, 19 February 2018 18:59:44 EET Jacopo Mondi wrote:
> > > > >> The sensor driver sets mbus format colorspace information and sizes,
> > > > >> but not ycbcr encoding, quantization and xfer function. When supplied
> > > > >> with an badly initialized mbus frame format structure, those fields
> > > > >> need to be set explicitly not to leave them uninitialized. This is
> > > > >> tested by v4l2-compliance, which supplies a mbus format description
> > > > >> structure and checks for all fields to be properly set.
> > > > >>
> > > > >> Without this commit, v4l2-compliance fails when testing formats with:
> > > > >> fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff
> > > > >>
> > > > >> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > > > >> ---
> > > > >>
> > > > >>  drivers/media/i2c/ov7670.c | 4 ++++
> > > > >>  1 file changed, 4 insertions(+)
> > > > >>
> > > > >> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > > > >> index 25b26d4..61c472e 100644
> > > > >> --- a/drivers/media/i2c/ov7670.c
> > > > >> +++ b/drivers/media/i2c/ov7670.c
> > > > >> @@ -996,6 +996,10 @@ static int ov7670_try_fmt_internal(struct
> > > > >> v4l2_subdev
> > > > >> *sd, fmt->height = wsize->height;
> > > > >>
> > > > >>  	fmt->colorspace = ov7670_formats[index].colorspace;
> > > > >
> > > > > On a side note, if I'm not mistaken the colorspace field is set to
> > > > > SRGB
> > > > > for all entries. Shouldn't you hardcode it here and remove the field ?
> > > > >
> > > > >> +	fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > > > >> +	fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > > > >> +	fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> > > > >
> > > > > How about setting the values explicitly instead of relying on defaults
> > > > > ?
> > > > > That would be V4L2_YCBCR_ENC_601, V4L2_QUANTIZATION_LIM_RANGE and
> > > > > V4L2_XFER_FUNC_SRGB. And could you then check a captured frame to see
> > > > > if
> > > > > the sensor outputs limited or full range ?
> > > >
> > > > This actually makes me wonder if those informations (ycbcb_enc,
> > > > quantization and xfer_func) shouldn't actually be part of the
> > > > supported format list... I blindly added those default fields in the
> > > > try_fmt function, but I doubt they applies to all supported formats.
> > > >
> > > > Eg. the sensor supports YUYV as well as 2 RGB encodings (RGB444 and
> > > > RGB 565) and 1 raw format (BGGR). I now have a question here:
> > > >
> > > > 1) ycbcr_enc transforms non-linear R'G'B' to Y'CbCr: does this
> > > > applies to RGB and raw formats? I don't think so, and what value is
> > > > the correct one for the ycbcr_enc field in this case? I assume
> > > > xfer_func and quantization applies to all formats instead..
> > >
> > > There's no encoding for RGB formats if I understand things correctly, so
> > > I'd set ycbcr_enc to V4L2_YCBCR_ENC_DEFAULT. The transfer function and
> > > the quantization apply to all formats, but I'd be surprised to find a
> > > sensor outputting limited range for RGB.
> >
> > Ack, we got the same understanding for RGB formats. I wonder if for
> > those formats we wouldn't need a V4L2_YCBCR_ENC_NONE or similar...
>
> That, or explicitly documenting that when the format is not YUV the field
> should be set by both drivers and applications to V4L2_YCBCR_ENC_DEFAULT when
> written and ignored when read.
>

Well, if no encoding is performed because the color encoding scheme is
RGB, the colorspace does anyway define an encoding method, so it seems
to me the latter is more appropriate (use DEFAULT and ignore when read).

That's anyway just my opinion, but I could send a patch for
documentation and see what feedback it gets.

> > > Have you been able to check whether the sensor outputs limited range of
> > > full range YUV ? If it outputs full range you can hardcode quantization
> > > to V4L2_QUANTIZATION_FULL_RANGE for all formats.
> >
> > In YUYV mode, I see values > 0xf0 ( > 240, which is the max value for
> > CbCr samples in limited quantization range), so I assume quantization
> > is full range.
>
> It should be, yes. What's the minimum and maximum values you get ?

>From a white surface:
min = 0x39
max = 0xfc

>From a black surface:
min = 0x00 (with 62 occurrences)
max = 0x8b

I guess that's indeed full range quantization

>
> > Actually the hardest part here was having a white enough surface to
> > point the sensor to :)
>
> Pointing a flashlight to the sensor usually does the trick.

I had a yellowish light that didn't work that well, I ended up putting
a white paper sheet in front of it and then took the picture.

Thanks
  j

>
> > >>>>  	info->format = *fmt;
> > >>>>
> > >>>>  	return 0;
>
> --
> Regards,
>
> Laurent Pinchart
>