by Randy Dunlap

[permalink] [raw]

Subject: Re: [PATCH v6 0/9] Renesas Capture Engine Unit (CEU) V4L2 driver

On 01/16/2018 01:44 PM, 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.

I would expect that you have already seen this, but I get a build error
in renesas-ceu.c. Here is a small patch for it.

---
From: Randy Dunlap <[email protected]>

Fix build error (on x86 with COMPILE_TEST):

../drivers/media/platform/renesas-ceu.c: In function 'ceu_parse_dt':
../drivers/media/platform/renesas-ceu.c:1497:27: error: request for member 'fwnode' in something not a structure or union
ceu_sd->asd.match.fwnode.fwnode =

Signed-off-by: Randy Dunlap <[email protected]>
---
drivers/media/platform/renesas-ceu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-next-20180119.orig/drivers/media/platform/renesas-ceu.c
+++ linux-next-20180119/drivers/media/platform/renesas-ceu.c
@@ -1494,7 +1494,7 @@ static int ceu_parse_dt(struct ceu_devic

ceu_sd->mbus_flags = fw_ep.bus.parallel.flags;
ceu_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
- ceu_sd->asd.match.fwnode.fwnode =
+ ceu_sd->asd.match.fwnode =
fwnode_graph_get_remote_port_parent(
of_fwnode_handle(ep));

2018-01-19 22:37:47

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v6 3/9] v4l: platform: Add Renesas CEU driver

Hi Jacopo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.15-rc8]
[cannot apply to linuxtv-media/master next-20180119]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Jacopo-Mondi/Renesas-Capture-Engine-Unit-CEU-V4L2-driver/20180120-053007
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

drivers/media/platform/renesas-ceu.c: In function 'ceu_start_streaming':
>> drivers/media/platform/renesas-ceu.c:287:2: warning: 'cdwdr' may be used uninitialized in this function [-Wmaybe-uninitialized]
iowrite32(data, priv->base + reg_offs);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/platform/renesas-ceu.c:335:27: note: 'cdwdr' was declared here
u32 camcr, cdocr, cfzsr, cdwdr, capwr;
^~~~~
>> drivers/media/platform/renesas-ceu.c:287:2: warning: 'cfzsr' may be used uninitialized in this function [-Wmaybe-uninitialized]
iowrite32(data, priv->base + reg_offs);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/platform/renesas-ceu.c:335:20: note: 'cfzsr' was declared here
u32 camcr, cdocr, cfzsr, cdwdr, capwr;
^~~~~
>> drivers/media/platform/renesas-ceu.c:415:8: warning: 'camcr' may be used uninitialized in this function [-Wmaybe-uninitialized]
camcr |= mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW ? 1 << 0 : 0;
~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/platform/renesas-ceu.c:335:6: note: 'camcr' was declared here
u32 camcr, cdocr, cfzsr, cdwdr, capwr;
^~~~~
drivers/media/platform/renesas-ceu.c: In function 'ceu_probe':
>> drivers/media/platform/renesas-ceu.c:1621:9: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
return ret;
^~~

vim +/cdwdr +287 drivers/media/platform/renesas-ceu.c

284
285 static void ceu_write(struct ceu_device *priv, unsigned int reg_offs, u32 data)
286 {
> 287 iowrite32(data, priv->base + reg_offs);
288 }
289
290 static u32 ceu_read(struct ceu_device *priv, unsigned int reg_offs)
291 {
292 return ioread32(priv->base + reg_offs);
293 }
294
295 /*
296 * ceu_soft_reset() - Software reset the CEU interface.
297 * @ceu_device: CEU device.
298 *
299 * Returns 0 for success, -EIO for error.
300 */
301 static int ceu_soft_reset(struct ceu_device *ceudev)
302 {
303 unsigned int i;
304
305 ceu_write(ceudev, CEU_CAPSR, CEU_CAPSR_CPKIL);
306
307 for (i = 0; i < 100; i++) {
308 if (!(ceu_read(ceudev, CEU_CSTSR) & CEU_CSTRST_CPTON))
309 break;
310 udelay(1);
311 }
312
313 if (i == 100) {
314 dev_err(ceudev->dev, "soft reset time out\n");
315 return -EIO;
316 }
317
318 for (i = 0; i < 100; i++) {
319 if (!(ceu_read(ceudev, CEU_CAPSR) & CEU_CAPSR_CPKIL))
320 return 0;
321 udelay(1);
322 }
323
324 /* If we get here, CEU has not reset properly. */
325 return -EIO;
326 }
327
328 /* --- CEU Capture Operations --- */
329
330 /*
331 * ceu_hw_config() - Configure CEU interface registers.
332 */
333 static int ceu_hw_config(struct ceu_device *ceudev)
334 {
> 335 u32 camcr, cdocr, cfzsr, cdwdr, capwr;
336 struct v4l2_pix_format_mplane *pix = &ceudev->v4l2_pix;
337 struct ceu_subdev *ceu_sd = ceudev->sd;
338 struct ceu_mbus_fmt *mbus_fmt = &ceu_sd->mbus_fmt;
339 unsigned int mbus_flags = ceu_sd->mbus_flags;
340
341 /* Start configuring CEU registers */
342 ceu_write(ceudev, CEU_CAIFR, 0);
343 ceu_write(ceudev, CEU_CFWCR, 0);
344 ceu_write(ceudev, CEU_CRCNTR, 0);
345 ceu_write(ceudev, CEU_CRCMPR, 0);
346
347 /* Set the frame capture period for both image capture and data sync. */
348 capwr = (pix->height << 16) | pix->width * mbus_fmt->bpp / 8;
349
350 /*
351 * Swap input data endianness by default.
352 * In data fetch mode bytes are received in chunks of 8 bytes.
353 * D0, D1, D2, D3, D4, D5, D6, D7 (D0 received first)
354 * The data is however by default written to memory in reverse order:
355 * D7, D6, D5, D4, D3, D2, D1, D0 (D7 written to lowest byte)
356 *
357 * Use CEU_CDOCR[2:0] to swap data ordering.
358 */
359 cdocr = CEU_CDOCR_SWAP_ENDIANNESS;
360
361 /*
362 * Configure CAMCR and CDOCR:
363 * match input components ordering with memory output format and
364 * handle downsampling to YUV420.
365 *
366 * If the memory output planar format is 'swapped' (Cr before Cb) and
367 * input format is not, use the swapped version of CAMCR.DTARY.
368 *
369 * If the memory output planar format is not 'swapped' (Cb before Cr)
370 * and input format is, use the swapped version of CAMCR.DTARY.
371 *
372 * CEU by default downsample to planar YUV420 (CDCOR[4] = 0).
373 * If output is planar YUV422 set CDOCR[4] = 1
374 *
375 * No downsample for data fetch sync mode.
376 */
377 switch (pix->pixelformat) {
378 /* Data fetch sync mode */
379 case V4L2_PIX_FMT_YUYV:
380 /* TODO: handle YUYV permutations through DTARY bits. */
381 camcr = CEU_CAMCR_JPEG;
382 cdocr |= CEU_CDOCR_NO_DOWSAMPLE;
383 cfzsr = (pix->height << 16) | pix->width;
384 cdwdr = pix->plane_fmt[0].bytesperline;
385 break;
386
387 /* Non-swapped planar image capture mode. */
388 case V4L2_PIX_FMT_NV16:
389 cdocr |= CEU_CDOCR_NO_DOWSAMPLE;
390 case V4L2_PIX_FMT_NV12:
391 if (mbus_fmt->swapped)
392 camcr = mbus_fmt->fmt_order_swap;
393 else
394 camcr = mbus_fmt->fmt_order;
395
396 cfzsr = (pix->height << 16) | pix->width;
397 cdwdr = pix->width;
398 break;
399
400 /* Swapped planar image capture mode. */
401 case V4L2_PIX_FMT_NV61:
402 cdocr |= CEU_CDOCR_NO_DOWSAMPLE;
403 case V4L2_PIX_FMT_NV21:
404 if (mbus_fmt->swapped)
405 camcr = mbus_fmt->fmt_order;
406 else
407 camcr = mbus_fmt->fmt_order_swap;
408
409 cfzsr = (pix->height << 16) | pix->width;
410 cdwdr = pix->width;
411 break;
412 }
413
414 camcr |= mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW ? 1 << 1 : 0;
> 415 camcr |= mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW ? 1 << 0 : 0;
416
417 /* TODO: handle 16 bit bus width with DTIF bit in CAMCR */
418 ceu_write(ceudev, CEU_CAMCR, camcr);
419 ceu_write(ceudev, CEU_CDOCR, cdocr);
420 ceu_write(ceudev, CEU_CAPCR, CEU_CAPCR_BUS_WIDTH256);
421
422 /*
423 * TODO: make CAMOR offsets configurable.
424 * CAMOR wants to know the number of blanks between a VS/HS signal
425 * and valid data. This value should actually come from the sensor...
426 */
427 ceu_write(ceudev, CEU_CAMOR, 0);
428
429 /* TODO: 16 bit bus width require re-calculation of cdwdr and cfzsr */
430 ceu_write(ceudev, CEU_CAPWR, capwr);
431 ceu_write(ceudev, CEU_CFSZR, cfzsr);
432 ceu_write(ceudev, CEU_CDWDR, cdwdr);
433
434 return 0;
435 }
436

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (7.91 kB)
.config.gz (48.63 kB)
Download all attachments

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