Received: by 2002:a25:6193:0:0:0:0:0 with SMTP id v141csp335569ybb; Wed, 25 Mar 2020 00:12:53 -0700 (PDT) X-Google-Smtp-Source: ADFU+vvyOWT3iYUoSPKmV5SV0TGCZwjv+ysY6fASqhT+8TDMUIsbgb9ZSeQGDU9r0b1gb2IG5SOg X-Received: by 2002:a05:6830:22d9:: with SMTP id q25mr1438812otc.164.1585120373838; Wed, 25 Mar 2020 00:12:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1585120373; cv=none; d=google.com; s=arc-20160816; b=ye697KHRwOoLsplROmJXlp0kBIEvj0+HaSPPKou4dzjzt8MZUkkbaFIaWizTt1W1bM AxgtFLlTCFo4guoQjmDr/qjkJMTiwQ7RIZE3vlRp2KVP2uyIiXPbuKCBJQy/J5FS0DLt H1i5KpzUL4KSFMhc7FcUR4pC7avUCjMxTMzsfU4fYN99V3jQprLqNnJ7szA2CfyLiYPv 9lEuzcUqOLZpoMYl/1bfAGh+sN70o1fqITUTBbRrl0a3QdkaXQRinbniQO9rkugA572y OityLjNX4W1SLtT4tFrg7U14F8AMlyplaG/aHmEhp31jceCnckv8HaFlEBxsgIGDShRl MgiQ== 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=bIBbYhBomiE/gj4wH2cNQVEz1Z0UIy5RDda3KRIP9r8=; b=dtcAOStiLCmu7zmiaVgJX+kvbsl5r2l05r1Xmmp5vlcnKObS8QE14fO7eVnvJ5kIQK V7fxNSf6jvtECxxefNecgYTR16/8lzn+RMBGgG0afeeAO9MfZgT1PfFcoDtGNO9GneLG tK42KNSN9evCBJv/j3ezaNJvXLbNxQutUepumMlt1Jj86BTQUTHIiXmXyOXHF2A4IUMv wPrkeibcl9qY/ifluDEDSYrJXFgWpcypVdMSS03ECLwcvJnZxAb9pTI+vOQzeyPs5wd6 cLMs9ccuQHY/lC/iuzaFH37hqsYhvt7dq6NyEohSIqUSv9dc8qNrcNpUoU5gezV7XZX6 gKGg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=VHRKpkF3; 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 p124si9855614oif.113.2020.03.25.00.12.42; Wed, 25 Mar 2020 00:12:53 -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=VHRKpkF3; 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 S1727413AbgCYHMF (ORCPT + 99 others); Wed, 25 Mar 2020 03:12:05 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:37882 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727262AbgCYHMF (ORCPT ); Wed, 25 Mar 2020 03:12:05 -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 D2BCB80C; Wed, 25 Mar 2020 08:12:00 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1585120321; bh=FSqDVsI6A0y3vtyhZ1Fm263/rnwL70jwXZ95f1VAuQY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=VHRKpkF3nDa+UlyBfoi8825Haq86enBr9HrqHxchx/izp68QwhV/YTlYkFlz7nu9T jR6e71qbJ7mraZyHGduB8A+lRpX8bniR++WO7VWdamfYzxG4sWq1/zdkJCE5z4UiX1 bjor2Vh2bxkw4ioUiJrMI0pCoMP55ZdYZ5l7I6bM= Date: Wed, 25 Mar 2020 09:11:57 +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: <20200325071157.GA4760@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> 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 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. > 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