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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 7FFC3C6FD1C for ; Sat, 25 Mar 2023 21:25:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=7ElUnUjtuajac+qcnGOik6SSvBWEOPovGJjiPxaafFw=; b=WIdoz9b7x4QmhH ZYd+n/d/W309U+zfF6C3dCiVApoJtegtKlf1250wACNVqfO0ymW1krBjKdQtPnCV4mrefktMmQpAc jTfE4bd4bE9X273OvpovtpwFL1hjz0Z6RtfnvQMnf+n1jXAX7mfre0tXRvV5x1QSwSV6eu4FvU3bL yQgiMSxha5pgA8ixzF1KvmAH+KTKimzUEDPF+FCkF2wy+05tG7oivX8PJlj6vJrDxWqm/boREpdvc Pr4nYSS+oujPXFzQvWmKNnSVPv3xYQU9wzMPE1u0oxbc2iGgSkJ+PaH7oWN4DwL5F7sYnbyKb3K/7 1XZvmyDpACJE+Eqy1PZQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pgBMy-007Pu7-14; Sat, 25 Mar 2023 21:24:12 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pgBMu-007PtD-0T for linux-arm-kernel@lists.infradead.org; Sat, 25 Mar 2023 21:24:09 +0000 Received: from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi [213.243.189.158]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D4688BE; Sat, 25 Mar 2023 22:24:06 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1679779446; bh=XFT2/Of6tXbRCWRlq7XJOvawMJpq1iSEbwMuAA7jtmI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=NTSayLYiolEyRvq25V/zs2k6izOy6dAzrT+uZ8qCzA+1UXaAlXEIfRF40WPFAMx12 KOZh0OUz2B6gETQ/ItGT1UT56v91NN6/heRGl6fBjYIa2R0apPrV2M1u4XbVYpJmqZ FVl9aYGeQeDufZFDdXGVjATAdLixE+77Yl8LhelA= Date: Sat, 25 Mar 2023 23:24:12 +0200 From: Laurent Pinchart To: Paul Kocialkowski Cc: linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev, Mauro Carvalho Chehab , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Adam Pigg , Thomas Petazzoni Subject: Re: [PATCH 5/9] media: sun6i-csi: capture: Rework and separate format validation Message-ID: <20230325212412.GC22214@pendragon.ideasonboard.com> References: <20230324151228.2778112-1-paul.kocialkowski@bootlin.com> <20230324151228.2778112-6-paul.kocialkowski@bootlin.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230324151228.2778112-6-paul.kocialkowski@bootlin.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230325_142408_356360_F5B211CF X-CRM114-Status: GOOD ( 31.26 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 Paul, Thank you for the patch. On Fri, Mar 24, 2023 at 04:12:24PM +0100, Paul Kocialkowski wrote: > Introduce a new sun6i_csi_capture_format_check helper to indicate > whether a set of pixel format and mbus code are compatible. > Most of the logic is taken from sun6i_csi_capture_link_validate, > with extra checks added along the way. > > Note that v4l2_format_info is now used for all pixel formats > since they should all be listed in the helper at this point. > > The motivation behind this change is to pave the way for supporting > the mc-style enum_fmt. > > Signed-off-by: Paul Kocialkowski > --- > .../sunxi/sun6i-csi/sun6i_csi_capture.c | 95 ++++++++++--------- > 1 file changed, 49 insertions(+), 46 deletions(-) > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c > index cf6aadbc130b..6ce7f1d3ed57 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c > @@ -327,6 +327,52 @@ static bool sun6i_csi_capture_format_match(u32 pixelformat, u32 mbus_code) > return false; > } > > +static bool sun6i_csi_capture_format_check(u32 pixelformat, u32 mbus_code) > +{ > + const struct sun6i_csi_capture_format *capture_format; > + const struct sun6i_csi_bridge_format *bridge_format; I think I would have named those output_format and input_format respectively, as "capture" and "bridge" take a bit more mental processing when reading the code. That may be because I'm not familiar with the driver though, so feel free to ignore this. > + const struct v4l2_format_info *format_info; > + > + format_info = v4l2_format_info(pixelformat); > + if (WARN_ON(!format_info)) > + return false; > + > + capture_format = sun6i_csi_capture_format_find(pixelformat); > + if (WARN_ON(!capture_format)) > + return false; > + > + bridge_format = sun6i_csi_bridge_format_find(mbus_code); > + if (WARN_ON(!bridge_format)) > + return false; > + > + /* Raw input is required for non-YUV formats. */ > + if (bridge_format->input_format != SUN6I_CSI_INPUT_FMT_RAW && > + (format_info->pixel_enc == V4L2_PIXEL_ENC_BAYER || > + format_info->pixel_enc == V4L2_PIXEL_ENC_RGB || > + format_info->pixel_enc == V4L2_PIXEL_ENC_COMPRESSED)) Unless I'm mistaken, the compressed format check is new. You could split it to a separate patch, or at least mention it in the commit message. > + return false; > + > + if (format_info->pixel_enc == V4L2_PIXEL_ENC_YUV) { > + /* YUV input is required for YUV pixels. */ > + if (bridge_format->input_format != SUN6I_CSI_INPUT_FMT_YUV420 && > + bridge_format->input_format != SUN6I_CSI_INPUT_FMT_YUV422) > + return false; > + > + /* YUV420 input can't produce (upsampled) YUV422 output. */ > + if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_YUV420 && > + format_info->vdiv == 1) > + return false; > + } > + > + /* Raw input requires a 1:1 match between input and output. */ > + if ((bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW || > + capture_format->input_format_raw) && > + !sun6i_csi_capture_format_match(pixelformat, mbus_code)) > + return false; Wrong indentation. With this fixed, Reviewed-by: Laurent Pinchart > + > + return true; > +} > + > /* Capture */ > > static void > @@ -890,28 +936,16 @@ static int sun6i_csi_capture_link_validate(struct media_link *link) > media_entity_to_video_device(link->sink->entity); > struct sun6i_csi_device *csi_dev = video_get_drvdata(video_dev); > struct v4l2_device *v4l2_dev = csi_dev->v4l2_dev; > - const struct sun6i_csi_capture_format *capture_format; > - const struct sun6i_csi_bridge_format *bridge_format; > unsigned int capture_width, capture_height; > unsigned int bridge_width, bridge_height; > - const struct v4l2_format_info *format_info; > u32 pixelformat, capture_field; > u32 mbus_code, bridge_field; > - bool match; > > sun6i_csi_capture_dimensions(csi_dev, &capture_width, &capture_height); > - > sun6i_csi_capture_format(csi_dev, &pixelformat, &capture_field); > - capture_format = sun6i_csi_capture_format_find(pixelformat); > - if (WARN_ON(!capture_format)) > - return -EINVAL; > > sun6i_csi_bridge_dimensions(csi_dev, &bridge_width, &bridge_height); > - > sun6i_csi_bridge_format(csi_dev, &mbus_code, &bridge_field); > - bridge_format = sun6i_csi_bridge_format_find(mbus_code); > - if (WARN_ON(!bridge_format)) > - return -EINVAL; > > /* No cropping/scaling is supported. */ > if (capture_width != bridge_width || capture_height != bridge_height) { > @@ -922,43 +956,12 @@ static int sun6i_csi_capture_link_validate(struct media_link *link) > return -EINVAL; > } > > - format_info = v4l2_format_info(pixelformat); > - /* Some formats are not listed. */ > - if (!format_info) > - return 0; > - > - if (format_info->pixel_enc == V4L2_PIXEL_ENC_BAYER && > - bridge_format->input_format != SUN6I_CSI_INPUT_FMT_RAW) > - goto invalid; > - > - if (format_info->pixel_enc == V4L2_PIXEL_ENC_RGB && > - bridge_format->input_format != SUN6I_CSI_INPUT_FMT_RAW) > - goto invalid; > - > - if (format_info->pixel_enc == V4L2_PIXEL_ENC_YUV) { > - if (bridge_format->input_format != SUN6I_CSI_INPUT_FMT_YUV420 && > - bridge_format->input_format != SUN6I_CSI_INPUT_FMT_YUV422) > - goto invalid; > - > - /* YUV420 input can't produce YUV422 output. */ > - if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_YUV420 && > - format_info->vdiv == 1) > - goto invalid; > - } > - > - /* With raw input mode, we need a 1:1 match between input and output. */ > - if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW || > - capture_format->input_format_raw) { > - match = sun6i_csi_capture_format_match(pixelformat, mbus_code); > - if (!match) > - goto invalid; > + if (!sun6i_csi_capture_format_check(pixelformat, mbus_code)) { > + v4l2_err(v4l2_dev, "invalid input/output format combination\n"); > + return -EINVAL; > } > > return 0; > - > -invalid: > - v4l2_err(v4l2_dev, "invalid input/output format combination\n"); > - return -EINVAL; > } > > static const struct media_entity_operations sun6i_csi_capture_media_ops = { -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel