From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A454FC2D0E4 for ; Mon, 23 Nov 2020 07:22:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4D25D20731 for ; Mon, 23 Nov 2020 07:22:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727995AbgKWHWC (ORCPT ); Mon, 23 Nov 2020 02:22:02 -0500 Received: from relay11.mail.gandi.net ([217.70.178.231]:42341 "EHLO relay11.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726302AbgKWHWC (ORCPT ); Mon, 23 Nov 2020 02:22:02 -0500 Received: from bootlin.com (atoulouse-258-1-33-168.w90-55.abo.wanadoo.fr [90.55.104.168]) (Authenticated sender: maxime.chevallier@bootlin.com) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 4592F100009; Mon, 23 Nov 2020 07:21:24 +0000 (UTC) Date: Mon, 23 Nov 2020 08:21:22 +0100 From: Maxime Chevallier To: Ezequiel Garcia Cc: Mauro Carvalho Chehab , Rob Herring , Robin Murphy , Mark Rutland , Heiko Stuebner , Hans Verkuil , Helen Koike , Dafna Hirschfeld , linux-media , devicetree , linux-arm-kernel , "open list:ARM/Rockchip SoC..." , Linux Kernel Mailing List , Thomas Petazzoni , Miquel Raynal , Paul Kocialkowski Subject: Re: [PATCH v3 2/3] media: rockchip: Introduce driver for Rockhip's camera interface Message-ID: <20201123082122.49a08ebb@bootlin.com> In-Reply-To: References: <20200922165535.1356622-1-maxime.chevallier@bootlin.com> <20200922165535.1356622-3-maxime.chevallier@bootlin.com> X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ezequiel, and thanks a lot for the review ! On Fri, 2 Oct 2020 14:35:28 -0300 Ezequiel Garcia 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 >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 71EB1C2D0E4 for ; Mon, 23 Nov 2020 07:22:16 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id BC9D32072C for ; Mon, 23 Nov 2020 07:22:15 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="vyGCw9Ul" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BC9D32072C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=bootlin.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=vQOuIvpy7Po454m4ov8oCVYXlj/9jBGbc+b/BSDl6C0=; b=vyGCw9Ulb5pS4m43YvOY9xJRn aotvsujNRwwfLEQKF2cE39dqbOsdO9co6fOFFKm482g1J2lH02U+wga0Ed8M/JNc1jmleZ8s7399R XDSr9fyBQ6Pr2GQ6Qu40N21uwh9EYwx1dTVLjPfs9GEsOv+EzjRJDIr7161E1yeqhWvlm6tcVCbXN RD/J8O5lLQQyvOKip9jy9KV/HROr9I7OJR5ovzQrL1HmgTwn65M9JUPkaX5GiRjANnuubz9mC7KIr rT5GCxcDk4D1jvp08J3f6X2MQdoI+kYdwxrBWyfS12aB1RQbuN8U5Cs7/kQmyjGoeGWVjbwvHXmvN 02ZqT+FKg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kh6Ap-0002FZ-Si; Mon, 23 Nov 2020 07:22:07 +0000 Received: from relay11.mail.gandi.net ([217.70.178.231]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kh6Ak-0002ES-OH; Mon, 23 Nov 2020 07:22:04 +0000 Received: from bootlin.com (atoulouse-258-1-33-168.w90-55.abo.wanadoo.fr [90.55.104.168]) (Authenticated sender: maxime.chevallier@bootlin.com) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 4592F100009; Mon, 23 Nov 2020 07:21:24 +0000 (UTC) Date: Mon, 23 Nov 2020 08:21:22 +0100 From: Maxime Chevallier To: Ezequiel Garcia Subject: Re: [PATCH v3 2/3] media: rockchip: Introduce driver for Rockhip's camera interface Message-ID: <20201123082122.49a08ebb@bootlin.com> In-Reply-To: References: <20200922165535.1356622-1-maxime.chevallier@bootlin.com> <20200922165535.1356622-3-maxime.chevallier@bootlin.com> X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201123_022203_011824_F7FAE373 X-CRM114-Status: GOOD ( 33.79 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , devicetree , Dafna Hirschfeld , Heiko Stuebner , Linux Kernel Mailing List , Paul Kocialkowski , "open list:ARM/Rockchip SoC..." , Helen Koike , Rob Herring , Thomas Petazzoni , Miquel Raynal , Hans Verkuil , Mauro Carvalho Chehab , Robin Murphy , linux-arm-kernel , linux-media Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org Hi Ezequiel, and thanks a lot for the review ! On Fri, 2 Oct 2020 14:35:28 -0300 Ezequiel Garcia 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 >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B0DF5C2D0E4 for ; Mon, 23 Nov 2020 07:22:39 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3E0632072C for ; Mon, 23 Nov 2020 07:22:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="h6z2FlXm" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3E0632072C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=bootlin.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=iSq0rhbIzK1JOWeqF3xNo+3TM6pLVkcNfD4Jzj3ou+E=; b=h6z2FlXmB2/fqHFRajAUiI7Fp CMwT3Z3rSH6GIkahdb30642fjKkKy3haDeI5I7FA8sQ1+wnWyFU5ZjUn31IIE2B76ksbcVk5M7ZoK X/q8BuuGcTOJCemxorRCqov9+p+BPJytYL/8mvZDSpI12emB/qDQrk5+gmnGteJKIPE86MkfJGQ73 ceHsMC4FSJPSnxXELlB896XKAHMbg2GfOAaX8MK7hbTE8NggLyZgPmDXuA8nPcWitwcee2S65lzBv AqzlvnVxdhYF8YWdogMw1kI1LM0Kqv2ivwUiiPTnaLOj5MZrtJfs5qIas94Rojl93sVc8ghZEQWm8 C4+eb9Yfg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kh6Ao-0002Ew-6u; Mon, 23 Nov 2020 07:22:06 +0000 Received: from relay11.mail.gandi.net ([217.70.178.231]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kh6Ak-0002ES-OH; Mon, 23 Nov 2020 07:22:04 +0000 Received: from bootlin.com (atoulouse-258-1-33-168.w90-55.abo.wanadoo.fr [90.55.104.168]) (Authenticated sender: maxime.chevallier@bootlin.com) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 4592F100009; Mon, 23 Nov 2020 07:21:24 +0000 (UTC) Date: Mon, 23 Nov 2020 08:21:22 +0100 From: Maxime Chevallier To: Ezequiel Garcia Subject: Re: [PATCH v3 2/3] media: rockchip: Introduce driver for Rockhip's camera interface Message-ID: <20201123082122.49a08ebb@bootlin.com> In-Reply-To: References: <20200922165535.1356622-1-maxime.chevallier@bootlin.com> <20200922165535.1356622-3-maxime.chevallier@bootlin.com> X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201123_022203_011824_F7FAE373 X-CRM114-Status: GOOD ( 33.79 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , devicetree , Dafna Hirschfeld , Heiko Stuebner , Linux Kernel Mailing List , Paul Kocialkowski , "open list:ARM/Rockchip SoC..." , Helen Koike , Rob Herring , Thomas Petazzoni , Miquel Raynal , Hans Verkuil , Mauro Carvalho Chehab , Robin Murphy , linux-arm-kernel , linux-media Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Ezequiel, and thanks a lot for the review ! On Fri, 2 Oct 2020 14:35:28 -0300 Ezequiel Garcia 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 >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel