Message-ID: <20201123082122.49a08ebb@bootlin.com>
Date:   Mon, 23 Nov 2020 08:21:22 +0100
From:   Maxime Chevallier <maxime.chevallier@...tlin.com>
To:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Mark Rutland <mark.rutland@....com>,
        Heiko Stuebner <heiko@...ech.de>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Helen Koike <helen.koike@...labora.com>,
        Dafna Hirschfeld <dafna.hirschfeld@...labora.com>,
        linux-media <linux-media@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Subject: Re: [PATCH v3 2/3] media: rockchip: Introduce driver for Rockhip's
 camera interface

Hi Ezequiel, and thanks a lot for the review !

On Fri, 2 Oct 2020 14:35:28 -0300
Ezequiel Garcia <ezequiel@...guardiasur.com.ar> wrote:

> Hi Maxime,
>
>Thanks to Dafna, I found the patch ^_^
>
>The driver looks real good. Just a few comments below.
>
>Is the driver passing latest v4l2-compliance tests?

I'll post them along with the next iteration of the series.

>> +config VIDEO_ROCKCHIP_VIP
>> +       tristate "Rockchip VIP (Video InPut) Camera Interface"
>> +       depends on VIDEO_DEV && VIDEO_V4L2
>> +       depends on ARCH_ROCKCHIP || COMPILE_TEST
>> +       select VIDEOBUF2_DMA_SG
>> +       select VIDEOBUF2_DMA_CONTIG
>> +       select V4L2_FWNODE
>> +       select V4L2_MEM2MEM_DEV
>> +       help
>> +         This is a v4l2 driver for Rockchip SOC Camera interface.
>> +
>> +         To compile this driver as a module choose m here.
>> +  
>
>Please add ... "the module will be called {the name}".

Sure, I will do !

[...]

>> +#define VIP_REQ_BUFS_MIN       1  
>
>I think you might want to have more than 1 buffer
>as minimum. How about 3? Two for the ping and pong,
>and one more in the queue.

Yes you're correct, 3 should be the strict minimum required buffers
here, I didn't update that after adding the dual-buffering mode.

>> +#define VIP_MIN_WIDTH          64
>> +#define VIP_MIN_HEIGHT         64
>> +#define VIP_MAX_WIDTH          8192
>> +#define VIP_MAX_HEIGHT         8192
>> +
>> +#define RK_VIP_PLANE_Y                 0
>> +#define RK_VIP_PLANE_CBCR              1
>> +
>> +#define VIP_FETCH_Y_LAST_LINE(VAL) ((VAL) & 0x1fff)
>> +/* Check if swap y and c in bt1120 mode */
>> +#define VIP_FETCH_IS_Y_FIRST(VAL) ((VAL) & 0xf)
>> +
>> +/* Get xsubs and ysubs for fourcc formats
>> + *
>> + * @xsubs: horizontal color samples in a 4*4 matrix, for yuv
>> + * @ysubs: vertical color samples in a 4*4 matrix, for yuv
>> + */
>> +static int fcc_xysubs(u32 fcc, u32 *xsubs, u32 *ysubs)  
>
>See below, you should be using v4l2_fill_pixfmt_mp.
>
>> +{
>> +       switch (fcc) {
>> +       case V4L2_PIX_FMT_NV16:
>> +       case V4L2_PIX_FMT_NV61:
>> +               *xsubs = 2;
>> +               *ysubs = 1;
>> +               break;
>> +       case V4L2_PIX_FMT_NV21:
>> +       case V4L2_PIX_FMT_NV12:
>> +               *xsubs = 2;
>> +               *ysubs = 2;
>> +               break;
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct vip_output_fmt out_fmts[] = {
>> +       {
>> +               .fourcc = V4L2_PIX_FMT_NV16,
>> +               .cplanes = 2,  
>
>>From what I can see, you are only using this
>information to calculate bytesperline and sizeimage,
>so you should be using the v4l2_fill_pixfmt_mp() helper.

You're correct, it indeed makes things much easier and allowed to
removed a lot of redundant info here !


>> +static void rk_vip_set_fmt(struct rk_vip_stream *stream,
>> +                          struct v4l2_pix_format_mplane *pixm,
>> +                          bool try)
>> +{
>> +       struct rk_vip_device *dev = stream->vipdev;
>> +       struct v4l2_subdev_format sd_fmt;
>> +       const struct vip_output_fmt *fmt;
>> +       struct v4l2_rect input_rect;
>> +       unsigned int planes, imagesize = 0;
>> +       u32 xsubs = 1, ysubs = 1;
>> +       int i;
>> +  
>
>I was expecting to see some is_busy or is_streaming check
>here, have you tested what happens if you change the format
>while streaming, or after buffers are queued?

Yes correct. I used the stream->state private flag here, but I it was
also brought to my attention that there also exists a vb2_is_busy()
helper, but I'm unsure if it would be correct to use it here.


>> +
>> +static int rk_vip_g_input(struct file *file, void *fh, unsigned int *i)
>> +{
>> +       *i = 0;
>> +       return 0;
>> +}
>> +
>> +static int rk_vip_s_input(struct file *file, void *fh, unsigned int i)
>> +{  
>
>Only one input, why do you need to support this ioctl at all?

I actually saw a fair amount of existing drivers implementing these
callbacks even for only one input, so I don't really know if I should
remove it or not ?

[...]

>> +
>> +static void rk_vip_vb_done(struct rk_vip_stream *stream,
>> +                          struct vb2_v4l2_buffer *vb_done)
>> +{
>> +       vb2_set_plane_payload(&vb_done->vb2_buf, 0,
>> +                             stream->pixm.plane_fmt[0].sizeimage);
>> +       vb_done->vb2_buf.timestamp = ktime_get_ns();  
>
>vb_done->vb2_buf.sequence = stream->frame_idx; ?

Good catch, thanks !

>> +       vb2_buffer_done(&vb_done->vb2_buf, VB2_BUF_STATE_DONE);
>> +}
>> +

[...]

>> diff --git a/drivers/media/platform/rockchip/vip/dev.c b/drivers/media/platform/rockchip/vip/dev.c
>> new file mode 100644
>> index 000000000000..d9b64f088c37
>> --- /dev/null
>> +++ b/drivers/media/platform/rockchip/vip/dev.c
>> @@ -0,0 +1,408 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Rockchip VIP Camera Interface Driver
>> + *
>> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
>> + * Copyright (C) 2020 Maxime Chevallier <maxime.chevallier@...tlin.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_reserved_mem.h>
>> +#include <linux/reset.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <media/v4l2-fwnode.h>
>> +
>> +#include "dev.h"
>> +#include "regs.h"
>> +
>> +#define RK_VIP_VERNO_LEN               10
>> +

[...]

>> +static const char * const px30_vip_clks[] = {
>> +       "aclk_vip",
>> +       "hclk_vip",
>> +       "pclk_vip",
>> +       "vip_out",  
>
>These clock names don't seem to match with the devicetree.
>I wonder how have you been testing this, to be honest ^_^  ?
>
>Isn't probe failing with mismatching clock names?

Aw you're correct, the change was in my tree but wasn't commited, my
bad. Sorry about that.

>> +};
>> +
>> +static const char * const px30_vip_rsts[] = {
>> +       "rst_vip_a",
>> +       "rst_vip_h",
>> +       "rst_vip_pclkin",
>> +};
>> +
>> +static const struct vip_match_data px30_vip_match_data = {
>> +       .chip_id = CHIP_PX30_VIP,
>> +       .clks = px30_vip_clks,
>> +       .clks_num = ARRAY_SIZE(px30_vip_clks),
>> +       .rsts = px30_vip_rsts,
>> +       .rsts_num = ARRAY_SIZE(px30_vip_rsts),
>> +};
>> +
>> +static const struct of_device_id rk_vip_plat_of_match[] = {
>> +       {
>> +               .compatible = "rockchip,px30-vip",
>> +               .data = &px30_vip_match_data,
>> +       },
>> +       {},
>> +};
>> +
>> +static irqreturn_t rk_vip_irq_handler(int irq, void *ctx)
>> +{
>> +       struct device *dev = ctx;
>> +       struct rk_vip_device *vip_dev = dev_get_drvdata(dev);
>> +
>> +       rk_vip_irq_pingpong(vip_dev);
>> +  
>
>Why not just making rk_vip_irq_pingpong the interrupt handler?

You're also correct, this is a remaining piece of code from when both
single-buffer and double-buffer'd acquisition were coexisting.

>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static void rk_vip_disable_sys_clk(struct rk_vip_device *vip_dev)
>> +{
>> +       int i;
>> +
>> +       for (i = vip_dev->clk_size - 1; i >= 0; i--)
>> +               clk_disable_unprepare(vip_dev->clks[i]);  
>
>Please use clk_bulk_disable_unprepare.
>> +}
>> +
>> +static int rk_vip_enable_sys_clk(struct rk_vip_device *vip_dev)
>> +{
>> +       int i, ret = -EINVAL;
>> +
>> +       for (i = 0; i < vip_dev->clk_size; i++) {
>> +               ret = clk_prepare_enable(vip_dev->clks[i]);
>> +  
>
>Please use clk_bulk_prepare_enable.

I'll switch to that, thanks

[...]

>> +       ret = of_reserved_mem_device_init(dev);
>> +       if (ret)
>> +               v4l2_warn(v4l2_dev, "No reserved memory region assign to VIP\n");
>> +  
>
>How about
>
>        ret = of_reserved_mem_device_init(dev);
>        if (ret && ret != -ENODEV)
>                return ret;
>
>instead?
>
>Also, it seems you are missing a of_reserved_mem_device_release
>on the error paths and plat_remove.

You're correct, I'll add that.

Thanks again for the thourough review !

Maxime

>Thanks,
>Ezequiel

Powered by blists - more mailing lists