On 21/01/18 18:49, jacopo mondi wrote: > Hi Hans, > > On Fri, Jan 19, 2018 at 12:26:09PM +0100, Hans Verkuil wrote: >> Hi Jacopo, >> >> On 01/16/18 22:44, Jacopo Mondi wrote: >>> Hello, >>> new version of CEU after Hans' review. >>> >>> Added his Acked-by to most patches and closed review comments. >>> Running v4l2-compliance, I noticed a new failure introduced by the way I now >>> calculate the plane sizes in set/try_fmt. >>> >>> This is the function used to update per-plane bytesperline and sizeimage: >>> >>> static void ceu_update_plane_sizes(struct v4l2_plane_pix_format *plane, >>> unsigned int bpl, unsigned int szimage) >>> { >>> if (plane->bytesperline < bpl) >>> plane->bytesperline = bpl; >>> if (plane->sizeimage < szimage) >>> plane->sizeimage = szimage; >>> } >>> >>> I'm seeing a failure as v4l2-compliance requires buffers with both >>> bytesperline >>> and sizeimage set to MAX_INT . Hans, is this expected from v4l2-compliance? >>> How should I handle this, if that has to be handled by the single drivers? >> >> I commented on this in my review of patch 3/9. > > Fixed thank you. > >> >>> >>> Apart from that, here it is the output of v4l2-compliance, with the last >>> tests >>> failing due to the above stated reason, and two errors in try/set format >>> due to >>> the fact the driver is not setting ycbcr encoding after it receives an >>> invalid >> >> Which driver? The CEU driver or the sensor driver? I don't actually see where >> it fails. >> > > Here it is: > > fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff > fail: v4l2-test-formats.cpp(451): > testColorspace(pix_mp.pixelformat, pix_mp.colorspace, pix_mp.ycbcr_enc, > pix_mp.quantization) > fail: v4l2-test-formats.cpp(736): Video Capture Multiplanar > is valid, but TRY_FMT failed to return a format > test VIDIOC_TRY_FMT: FAIL > fail: v4l2-test-formats.cpp(335): ycbcr_enc >= 0xff > fail: v4l2-test-formats.cpp(451): > testColorspace(pix_mp.pixelformat, pix_mp.colorspace, pix_mp.ycbcr_enc, > pix_mp.quantization) > fail: v4l2-test-formats.cpp(996): Video Capture Multiplanar > is valid, but no S_FMT was implemented > test VIDIOC_S_FMT: FAIL
Sorry, I was perhaps confusing. I meant that I couldn't see where in the code ycbcr_enc was not overwritten by 0 (which should have happened). You will need to debug a bit, I think. It could be a bug in the ceu driver or the sensor driver. > > >>> format. I would set those, but I'm not sure what it the correct value and >>> not >>> all mainline drivers do that. >> >> In any case, the default for ycbcr_enc, xfer_func and quantization is 0. >> > > Thanks again. I do expect to be the sensor driver to set ycbcr_enc and > quantization, but from a very trivial grep on media/i2c/ I see only a > few drivers taking care of them (adv7511 and adv7842). What about the > others? I assume v4l2-compliance would not fail on them as it does on > ov7670, but I don't see where ycbr_enc (and others) are managed. In most cases these values are initialized to 0 (either a memset or by initializing the struct), which is typically all you need for sensor drivers. HDMI drivers are more complicated which is why you see explicit handling of these fields only there. > > Overall, with this addressed, the other issue I mentioned on patch > [3/9] on readbuffers clarified and frameinterval handled for ov722x, I > hope we're done with this series. Thanks again your continued effort > in reviews and guidance. My pleasure! Regards, Hans