Received: by 2002:a25:6193:0:0:0:0:0 with SMTP id v141csp411447ybb; Wed, 25 Mar 2020 02:12:51 -0700 (PDT) X-Google-Smtp-Source: ADFU+vs8be+fNJWa4J+5bzXNq1xmWcZxCoPMUUyznspHHjthoHhQpA/7s/ukmtFbz/cyOflMXnFE X-Received: by 2002:aca:534d:: with SMTP id h74mr1683834oib.173.1585127571563; Wed, 25 Mar 2020 02:12:51 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1585127571; cv=none; d=google.com; s=arc-20160816; b=dOdUfmu7DexCco0wnOV1SVMdiyCE/KilIs7el0yvGBWqujLK4UbaDhQIBNM2bcnwQe luY7iDM0J18OyxzSAfmxA1TVrygvaktHdPO3MNui8vN+9hGFYO0J1rt0XaWYv7oBDft9 2XWoG7FfzaHm3L1dV987YCmcPDhdPX4JVa5w6OhCW9ly4n9C5uo72ke8YayaVxwP6KCh 8HMQf6T/DuJUQ9uLoHJoA9mSgGgvkr+LnBwQEcts0d6nqsphCBDAmwHhHqFRr93TyHL3 gDdQXHmBzmRz9afkOm2k0zy+WfZDANvGhBbaGDwDfkSiJb1pRU7MUdBafbry/ez4CVQR uQ6A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=YyEqJuNoltES8oqnWG340bVGZahdMZkX210hcgH3E1Q=; b=0NLWXEn2eo3KQS15MHY/vaqqIRZ8oCJVu7iB90MwJBb/23xo0Ggy+xazWdDiedYICL YdIHcbCJOZaVcZm5PYl3XsNRmCK7LjwYIyQK/y7a/Gs2mtWX26/19Pbof4hjA2g7d9Fx e8vM275mlLh7XwttdXsnZna8XLx91wuMpPL6B809rHwe9+SvDWGFRyFSZMsItrIHpX7Y l9KHATrplhAWQGEBjDGnmqip5yXEzYD3qEl782hR+gZmsfmb5RDdddyjKmk2qM9k4Qs8 eUPMVkaeuJnDD/rsr9SsCql3K5W7nZ4/tp75duskQ6qljVxJVwhps2WMTwWkRUsx7kT3 wDdA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=Aq8uYCH+; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w85si9936503oif.88.2020.03.25.02.12.38; Wed, 25 Mar 2020 02:12:51 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=Aq8uYCH+; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726262AbgCYJLL (ORCPT + 99 others); Wed, 25 Mar 2020 05:11:11 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:39062 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725873AbgCYJLL (ORCPT ); Wed, 25 Mar 2020 05:11:11 -0400 Received: from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id CFFD580C; Wed, 25 Mar 2020 10:11:08 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1585127469; bh=h0S77ZM+eorU0Bj5tWHi+EgzvOZ1BT+YGd02enoTmdk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Aq8uYCH+3nd88qxPAUWcGyHandJr27L+DHMSaB2DpJeVoITAH5c1uafzoKIv0oJ6+ cKnqaT1IS/A77GYy87L73q2OEWSjaptG5+vBMPXSGK1PuEaRlLCeFCktKlJKUjbBmE D09qm3urkl0FsKoEHv5u6Hfk0k2O4VPOQeaVKQJo= Date: Wed, 25 Mar 2020 11:11:07 +0200 From: Laurent Pinchart To: Dafna Hirschfeld Cc: Hans Verkuil , Helen Koike , hans.verkuil@cisco.com, linux-rockchip@lists.infradead.org, devicetree@vger.kernel.org, eddie.cai.linux@gmail.com, mchehab@kernel.org, heiko@sntech.de, jacob2.chen@rock-chips.com, jeffy.chen@rock-chips.com, zyc@rock-chips.com, linux-kernel@vger.kernel.org, Tomasz Figa , sakari.ailus@linux.intel.com, kernel@collabora.com, Ezequiel Garcia , Linux Media Mailing List , linux-arm-kernel@lists.infradead.org, zhengsq@rock-chips.com, Jacob Chen , Allon Huang Subject: Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver Message-ID: <20200325091107.GB4760@pendragon.ideasonboard.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Dafna, On Wed, Mar 25, 2020 at 09:51:51AM +0100, Dafna Hirschfeld wrote: > On Wed, Mar 25, 2020 at 8:12 AM Laurent Pinchart wrote: > > 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 ? Not just when it's MEDIA_BUS_FMT_FIXED. Userspace can look at the pad flags to determine the type of data carried by the pad. This can, for instance, allow userspace to walk graphs to find video data paths without going down metadata or audio links. > What makes is more backward-comaptible ? We won't need to change the current format used on those pads (MEDIA_BUS_FMT_FIXED), which would break userspace that relies on that format. Of course, if it's just for the rkisp1 driver, we don't care much about breakages as the driver is in staging :-) > > > 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