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=-6.7 required=3.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 9F3E0C1975A for ; Wed, 25 Mar 2020 08:53:56 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 705452076A for ; Wed, 25 Mar 2020 08:53:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ty4AVnhY"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="BHBS7Rgh" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 705452076A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-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=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=fbr8Yyez4TLZJFMqN9YXAvDW/9jgiiqIm8ZcEAG1T6s=; b=ty4AVnhYaGKBbL q14HmW3Bj2VPp9mxh1zIaSqwT0O94h1QlEV0dqM1tkJGvSZPlm4+l6huIYxOE9VlzlUjHNFbJiEKk Fxz6aWuu6Il0Jy8IVxMoCKXLUJodYW7PJQQEL1CN1VoI9Jv7i7mcjDZy9ZqmXfc89cdaLwKkkqss7 vjRSgdXDRhxUE6JoDQnZYxFppB0dpXcPETbxu2JuCy70JLWERya8XRSogvBRRyg/0iiupBbpULj3m ppr5Gs4FSNhW0h9dt9HbN+lDHSxCM3qoQglj624S/19WJ9jG0bEooe1EUk9X01jChGcbUgmfDKNN6 J3r5x4cWaldaYBr60eKg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jH1nN-0006Ud-M2; Wed, 25 Mar 2020 08:53:53 +0000 Received: from mail-ua1-x944.google.com ([2607:f8b0:4864:20::944]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jH1lh-0004eO-Ks; Wed, 25 Mar 2020 08:52:11 +0000 Received: by mail-ua1-x944.google.com with SMTP id o16so447047uap.6; Wed, 25 Mar 2020 01:52:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=yYnFhIZxNvMtmB7aBJxkfTBHSBTcXgduysT/F6+Ymfc=; b=BHBS7RghA9uWsVD6f4eLZJhFtirlujP00SAjRQCsJNGqU3VMNGi5ShNXVJ/Ldjv0Og APZBv2DLQ5gFyd4faO9bVbVEju4vzvW/gcZdtQTQwVW5mRe0ui3WRnx4F4Zsw1piNahM ez/6cjagh0OFuOwV38RHdy5GD0gDpaZ3lBl1SPoyfYbdCeTom+eHbUXnBZpTmQCvCmL2 az0q5Aeo2As82O4+HvfNucVMWHWkVQxUoFxG+uncFSMw/b0IrjrQUIdEx7BT5H9FpBhs DZUXimSBF/B8rWtuaE8uy305oLzla5aTTmJ+oW8hTNsrzF/FjDeVSfYeShUmtNRJmHoV M/Ww== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=yYnFhIZxNvMtmB7aBJxkfTBHSBTcXgduysT/F6+Ymfc=; b=D++Dgf9qjEetclXig8IphLOxXXkrq+uG+AOU/pqm0x3IUxW1NWh1wDmGY1qjdrBN+j ghuMOdva00MOrNX//QCC425CDpoE7tPepkzdcARz9n8zChIOLDj0+bpnwSsV4sCeBHuj Ir4CktQ0BSWntbwgyWHNBYNm2mcyqvqEJqXne2Gaq4OY1D40VJ3sSPas7PGsXU9F7KaR mDKZiatcEKMKFkXveCMw2Zds5cdPg81IswZz5uioG4RkIn2aRd3HBe9sEeIJUcBbGOEb X2uZiKviNomgCb3X95bglufws8rgvyBdr9LVejZ+6FY5IgUk52n4zGwezsQcN26kiAr/ py4A== X-Gm-Message-State: ANhLgQ2M4mpfDbgaVkPG1g+rx1/PJCCIL19OhixxlZS2upW2TdXJzsMi 9C5Eqru7jahVM2Ivdyfir7GsXvPhN+kZ6tF+W8M= X-Google-Smtp-Source: ADFU+vvAKMaM/hglw/i2e4ScpUNtwm+Nlbec+LtSOatUukLUR/yGA5aOBccM9tHGp1kfRavIl3YBYg2tDXRB8lTgnFk= X-Received: by 2002:ab0:72c6:: with SMTP id g6mr1582007uap.24.1585126328145; Wed, 25 Mar 2020 01:52:08 -0700 (PDT) MIME-Version: 1.0 References: <20190730184256.30338-1-helen.koike@collabora.com> <20190730184256.30338-6-helen.koike@collabora.com> <86e17716-193f-ca49-1104-9c599a667eeb@collabora.com> <20190815193511.GB5011@pendragon.ideasonboard.com> <20200325071157.GA4760@pendragon.ideasonboard.com> In-Reply-To: <20200325071157.GA4760@pendragon.ideasonboard.com> From: Dafna Hirschfeld Date: Wed, 25 Mar 2020 09:51:51 +0100 Message-ID: Subject: Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver To: Laurent Pinchart X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200325_015209_772846_0D7F750C X-CRM114-Status: GOOD ( 41.85 ) 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: devicetree@vger.kernel.org, eddie.cai.linux@gmail.com, kernel@collabora.com, heiko@sntech.de, jacob2.chen@rock-chips.com, jeffy.chen@rock-chips.com, zyc@rock-chips.com, linux-kernel@vger.kernel.org, Tomasz Figa , Hans Verkuil , linux-rockchip@lists.infradead.org, Helen Koike , Jacob Chen , hans.verkuil@cisco.com, sakari.ailus@linux.intel.com, zhengsq@rock-chips.com, Allon Huang , mchehab@kernel.org, Ezequiel Garcia , linux-arm-kernel@lists.infradead.org, Linux Media Mailing List Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Mar 25, 2020 at 8:12 AM Laurent Pinchart wrote: > > Hi Dafna, > > On Wed, Mar 25, 2020 at 07:34:37AM +0100, Dafna Hirschfeld wrote: > > On Thu, Aug 15, 2019 at 10:17 PM Laurent Pinchart wrote: > > > On Wed, Aug 07, 2019 at 12:39:17PM +0200, Hans Verkuil wrote: > > > > On 8/6/19 8:51 PM, Helen Koike wrote: > > > > > On 7/30/19 3:42 PM, Helen Koike wrote: > > > > >> From: Jacob Chen > > > > >> > > > > >> Add the subdev driver for rockchip isp1. > > > > >> > > > > >> Signed-off-by: Jacob Chen > > > > >> Signed-off-by: Shunqian Zheng > > > > >> Signed-off-by: Yichong Zhong > > > > >> Signed-off-by: Jacob Chen > > > > >> Signed-off-by: Eddie Cai > > > > >> Signed-off-by: Jeffy Chen > > > > >> Signed-off-by: Allon Huang > > > > >> Signed-off-by: Tomasz Figa > > > > >> [fixed unknown entity type / switched to PIXEL_RATE] > > > > >> Signed-off-by: Ezequiel Garcia > > > > >> [update for upstream] > > > > >> Signed-off-by: Helen Koike > > > > >> > > > > >> --- > > > > >> > > > > >> Changes in v8: None > > > > >> Changes in v7: > > > > >> - fixed warning because of unknown entity type > > > > >> - fixed v4l2-compliance errors regarding rkisp1 formats, try formats > > > > >> and default values > > > > >> - fix typo riksp1/rkisp1 > > > > >> - redesign: remove mipi/csi subdevice, sensors connect directly to the > > > > >> isp subdevice in the media topology now. As a consequence, remove the > > > > >> hack in mipidphy_g_mbus_config() where information from the sensor was > > > > >> being propagated through the topology. > > > > >> - From the old dphy: > > > > >> * cache get_remote_sensor() in s_stream > > > > >> * use V4L2_CID_PIXEL_RATE instead of V4L2_CID_LINK_FREQ > > > > >> - Replace stream state with a boolean > > > > >> - code styling and checkpatch fixes > > > > >> - fix stop_stream (return after calling stop, do not reenable the stream) > > > > >> - fix rkisp1_isp_sd_get_selection when V4L2_SUBDEV_FORMAT_TRY is set > > > > >> - fix get format in output (isp_sd->out_fmt.mbus_code was being ignored) > > > > >> - s/intput/input > > > > >> - remove #define sd_to_isp_sd(_sd), add a static inline as it will be > > > > >> reused by the capture > > > > >> > > > > >> drivers/media/platform/rockchip/isp1/rkisp1.c | 1286 +++++++++++++++++ > > > > >> drivers/media/platform/rockchip/isp1/rkisp1.h | 111 ++ > > > > >> 2 files changed, 1397 insertions(+) > > > > >> create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.c > > > > >> create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.h > > > > >> > > > > >> diff --git a/drivers/media/platform/rockchip/isp1/rkisp1.c b/drivers/media/platform/rockchip/isp1/rkisp1.c > > > > >> new file mode 100644 > > > > >> index 000000000000..6d0c0ffb5e03 > > > > >> --- /dev/null > > > > >> +++ b/drivers/media/platform/rockchip/isp1/rkisp1.c > > > > >> @@ -0,0 +1,1286 @@ > > > > > > > > > > > > > > > > >> +static int rkisp1_isp_sd_get_fmt(struct v4l2_subdev *sd, > > > > >> + struct v4l2_subdev_pad_config *cfg, > > > > >> + struct v4l2_subdev_format *fmt) > > > > >> +{ > > > > >> + struct rkisp1_isp_subdev *isp_sd = sd_to_isp_sd(sd); > > > > >> + struct v4l2_mbus_framefmt *mf = &fmt->format; > > > > >> + > > > > >> + if ((fmt->pad != RKISP1_ISP_PAD_SINK) && > > > > >> + (fmt->pad != RKISP1_ISP_PAD_SOURCE_PATH)) { > > > > >> + fmt->format.code = MEDIA_BUS_FMT_FIXED; > > > > >> + /* > > > > >> + * NOTE: setting a format here doesn't make much sense > > > > >> + * but v4l2-compliance complains > > > > >> + */ > > > > >> + fmt->format.width = RKISP1_DEFAULT_WIDTH; > > > > >> + fmt->format.height = RKISP1_DEFAULT_HEIGHT; > > > > > > > > > > As I had mentioned to you, this is called for the isp pads connected to the > > > > > DMA engines for statistics and parameters (meta data). > > > > > > > > > > If I remove those, I get the following errors: > > > > > > > > > > Sub-Device ioctls (Sink Pad 1): > > > > > test Try VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK > > > > > fail: v4l2-test-subdevs.cpp(311): fmt.width == 0 || fmt.width > 65536 > > > > > fail: v4l2-test-subdevs.cpp(356): checkMBusFrameFmt(node, fmt.format) > > > > > test Try VIDIOC_SUBDEV_G/S_FMT: FAIL > > > > > test Try VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK > > > > > test Active VIDIOC_SUBDEV_ENUM_MBUS_CODE/FRAME_SIZE/FRAME_INTERVAL: OK > > > > > fail: v4l2-test-subdevs.cpp(311): fmt.width == 0 || fmt.width > 65536 > > > > > fail: v4l2-test-subdevs.cpp(356): checkMBusFrameFmt(node, fmt.format) > > > > > test Active VIDIOC_SUBDEV_G/S_FMT: FAIL > > > > > test Active VIDIOC_SUBDEV_G/S_SELECTION/CROP: OK > > > > > test VIDIOC_SUBDEV_G/S_FRAME_INTERVAL: OK (Not Supported) > > > > > > > > > > Here is the full log: http://ix.io/1QNt > > > > > > > > > > Is this a bug in v4l2-compliance? > > > > > > > > Yes and no :-) > > > > > > > > Currently v4l2-compliance assumes that only video is transferred over a media bus. > > > > But that's not the case here, and testing the code field doesn't help v4l2-compliance > > > > since MEDIA_BUS_FMT_FIXED is also still used by some older subdev drivers for video. > > > > > > > > I think we need a new bus format: MEDIA_BUS_FMT_FIXED_METADATA. Then v4l2-compliance > > > > can tell it apart from the regular fixed video bus format. > > > > > > Wouldn't a pad flag that identifies the type of data transmitted by a > > > pad be a better, backward-compatible option ? This could be useful for > > > audio as well. > > > > Hi, > > Can you explain what pad flag do you mean? > > Do you mean adding a flag in the 'MEDIA_LNK_FL_*' list ? > > I meant MEDIA_PAD_FL_*. We could reserve a few bits in > media_pad_desc.flags to tell what type of data is being transported. > So the idea is that when the MBUS format is MEDIA_BUS_FMT_FIXED, then userspace should look at the flags of the pad to see what format is it? So if I add a flag 'MEDIA_PAD_FL_METADATA' it knows it is a metadata ? What makes is more backward-comaptible ? > > Also, some valid value should be set to 'fmt->format.code' so with > > the flags solution > > that you suggest it should stay MEDIA_BUS_FMT_FIXED ? > > Correct. > > > > > If I do a 'git grep MEDIA_BUS_FMT_FIXED' then I see that it is also in use by vsp1 > > > > for histogram information, so that should also be converted to use the new FIXED_METADATA > > > > format, although that might be too late (there might be userspace complications). > > > > > > Yes, probably not a good idea. > > > > > > > >> + fmt->format.field = V4L2_FIELD_NONE; > > > > >> + return 0; > > > > >> + } > > > > >> + > > > > >> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > > > > >> + mf = v4l2_subdev_get_try_format(sd, cfg, fmt->pad); > > > > >> + fmt->format = *mf; > > > > >> + return 0; > > > > >> + } > > > > >> + > > > > >> + if (fmt->pad == RKISP1_ISP_PAD_SINK) { > > > > >> + *mf = isp_sd->in_frm; > > > > >> + } else if (fmt->pad == RKISP1_ISP_PAD_SOURCE_PATH) { > > > > >> + /* format of source pad */ > > > > >> + *mf = isp_sd->in_frm; > > > > >> + mf->code = isp_sd->out_fmt.mbus_code; > > > > >> + /* window size of source pad */ > > > > >> + mf->width = isp_sd->out_crop.width; > > > > >> + mf->height = isp_sd->out_crop.height; > > > > >> + mf->quantization = isp_sd->quantization; > > > > >> + } > > > > >> + mf->field = V4L2_FIELD_NONE; > > > > >> + > > > > >> + return 0; > > > > >> +} > > -- > Regards, > > Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel