On 02/21/2018 04:47 PM, jacopo mondi wrote: > Hello again, > > On Tue, Feb 20, 2018 at 09:58:57AM +0100, jacopo mondi wrote: >> Hi Laurent, >> >> On Mon, Feb 19, 2018 at 09:19:32PM +0200, Laurent Pinchart wrote: >>> Hi Jacopo, >>> >>> Thank you for the patch. >>> >>> 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.. >> > > What if I repropose this as separate patch not part of the CEU series > in order not to hold back v10 (which I hope will be the last CEU > iteration)? I would definitely be fine with that. We first need to define what exactly should be done by drivers. See also this thread: https://www.spinics.net/lists/linux-renesas-soc/msg23888.html I'll work on that going forward as part of the compliance tests. Regards, Hans > >> Thanks >> j >> >>>> info->format = *fmt; >>>> >>>> return 0; >>> >>> -- >>> Regards, >>> >>> Laurent Pinchart >>> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
